Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: updownv2: improved firewall script #1283

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Thermi
Copy link
Contributor

@Thermi Thermi commented Sep 6, 2022

WIP progress for an updown plugin and script that enable correct handling of inserting of forwarding rules or whatever else users want to do

TODO

  • implement iptables/nftables rule insertion with iptables-restore and nft tool
  • cleanup unparametrized script
  • parametrize script as with existing updown script
  • documentation

@tobiasbrunner
Copy link
Member

Just two quick questions:

  1. Why has this to be a new plugin? Can't the updown plugin be changed? (With 6.0 we have a major release coming up where backward compatibility might not be a concern).
  2. On the other hand, couldn't this just be done via vici without adding a new plugin or changing the existing one at all? (There is a python example for an "updown" script in the route-based/net2net-xfrmi-ike scenario.)

@Thermi
Copy link
Contributor Author

Thermi commented Sep 7, 2022

Because the behaviour is entirely incompatible with the current one and I do not want to break existing installations. It should be an explicite choice to use this new implementation.
It is called exactly once for each event (up, down, rekey, IKE address update), instead of one time for each traffic selector combination. That way users can easier implement management of firewall rules or other things. The implementation also provides several different attributes like the SPIs, IKE IDs, and others that enable constructing a unique key for each CHILD_SA and hence allow differentiation of different CHILD_SAs.

That could probably be done via VICI too, but it wouldn't be as easy or straight forward to use because running a script for each event is relatively commonly used to implement customizable behaviour. Having a different process connect to the daemon is not that common. It is more the exception.

@tobiasbrunner
Copy link
Member

Because the behaviour is entirely incompatible with the current one and I do not want to break existing installations. It should be an explicite choice to use this new implementation.

A new behavior could be triggered by a prefix in the <child>.updown option (e.g. v2:/path/to/script) or there could be a global option for the plugin (probably easier as that could e.g. register a different listener - mixing both behaviors is probably not that useful anyway). But again, we could also make breaking changes, as I don't really see much of a future for the existing plugin, which e.g. is completely incompatible with make-before-break reauthentication (well, part of it is due to the default script, which also only supports iptables, but still). However, I still feel there is no need for a new or updated plugin as vici already provides pretty much everything that's needed.

That could probably be done via VICI too, but it wouldn't be as easy or straight forward to use because running a script for each event is relatively commonly used to implement customizable behaviour. Having a different process connect to the daemon is not that common. It is more the exception.

The vici client could basically be the plugin you intend to write here and call a script anyway you or the user likes (might even be easier to implement in e.g. Python than in C). The user would then not have to bother with the client itself, other than enabling it e.g. via charon.start-scripts. It would also be possible to pass the <child>.updown option in the vici child-updown event (and perhaps child-rekey too, although it's unlikely to change), so it would be usable in a vici client (that option wouldn't even have to be limited to a script path or command line, but could contain JSON or whatever the vici client can use for child-specific options). Since the ike-update event isn't child-specific, the client would have to cache that information for each CHILD_SA it knows about, though (but to support make-before-break reauthentication it would have to track some stuff anyway).

@Thermi
Copy link
Contributor Author

Thermi commented Sep 16, 2022

I want to keep the changes relatively minimal and without having to, for now, think about coexistence with existing code in the updown plugin. It's why right now at least it's its own plugin. I can of course merge it back into the updown one and have a condition to chnage the behaviour as you proposed.

If I find a good way to port the updown mechanics to VICI I could of course do that. I presume that would also imply that regardless of the actual execution time of the program receiving the VICI messages there will be a race condition between the script doing whatever needs to be done locally to enable proper processing of traffic (disregarding iptables/XFRM) and the peer sending the first packets over the CHILD_SA. I guess for proper integration into a system with such preconditions would require some integral changes to delay the sending of the final message for confirmation of the CHILD_SA eastablishment.

The ike-update case is handled once for all CHILD_SAs, that's enabled by the most critical part of the updownv2 plugin: the CHILD_SAs are listed in apropriately named variables; so basically the variable name has to be constructed within the script handling the events needs to build the variable names first (because the number of CHILD_SAs per IKE_SA is variable of course).

With the current code in the plugin all cases should be handleable properly, even make before break (you can construct a unique key per CHILD_SA with the SPIs and put them into the comment field in nftables or the comment match in iptables, then match on them when you remove the rules).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants