pppd/ipv6cp: Add ipv6-pre-up script. #341
Conversation
|
I do not like this pre-up script feature. For me it looks like requirement for some buggy behavior. By default firewall should not have "allow all" behavior. If default behavior is "deny all" then configuring per-interface firewall settings could be done in (post-)up script. There is already ipv4 pre-up script, so probably it could make sense to have also ipv6 pre-up script... As I said I'm not a big fan of it, but I'm not against of it. Ad wildcard-interface e.g. -i/-o ppp+. This does not work in scenario, if you let pppd/kernel to choose interface name (to choose interface suffix number to some free value) and you want to configure specific per-interface route settings. E.g. if for PPPoE over eth1 (interface ppp<dynamic_assigned2>) you want to have different route settings as for PPPoE over eth2 (interface ppp<dynamic_assigned1>). Such settings are required on PPPoE concentrator server if some connections should be in routed differently than others (matched e.g. by authenticated user or client MAC address). (This argument is about wildcards, not decision if it should be in pre or post script) |
|
@pali - That's fair. I still don't think (or like) the pre-up scripts either. Your argumentation is probably better than mine. I think @jkroonza will need to provide a compelling reason to make a change, and we can discuss the pre-up scripts and possible alternate solutions. Still don't think we understand the root cause here. |
|
I think the principle is useful. To basically have access to the configured but not yet UP interface. It's not always possible to make firewall and shaping changes using patterns such as ppp+ (we do this ourselves, unfortunately there are cases where this simply doesn't work or doesn't work well and we end up adding interfaces to sets, eg, we have pppoe and/or VPN clients connecting and depending on authentication details needs to be placed in different sets to apply the right rules). One other strategy could be to rename interfaces, but we can't do this using the standard ppp mechanisms, and whilst there are comments in the source about a script changing the name of the interface, we don't believe this is a workable (or at least simple and reliable) solution either, especially with multiple protocols involved, eg, ipv4 and ipv6, does both CPs check for interface name changes? And at what times? So we believe it's simpler to just dispatch ppp+ in nft/iptables to a dedicated chain that then matches more fine-grained based on an interface set). A wholly better solution compared to ${proto}-pre-up may simply be a consolidated pre-up script, and I'd be all for that. auth-up could in some cases work for that, auth-up doesn't execute for all connections where we need this (eg, to add a dial-out link to a "red" interface set). Granted, the default policy should be "red", but we like being thorough. |
|
Thinking about this a little bit more: I'd be in favour of a single consolidated pre-up script, where it can also be mentioned explicitly that the script it allowed to rename the interface (this makes a whole other PR re interface naming completely moot). Then mark ip-pre-up as deprecated, to be removed in future (issue a warning if it's used kind of thing). |
This is required because either ipcp or ipv6cp can come up first, or as the only protocol, and once IPv6 parameters has been established, there are actions that may need to be executed prior to bringing the interface up. Prior to this we can have an execution order like: pppd[26615]: Script /etc/ppp/ipv6-up started (pid 28183) pppd[26615]: Script /etc/ppp/ip-pre-up started (pid 28186) pppd[26615]: Script /etc/ppp/ip-pre-up finished (pid 28186), status = 0x0 pppd[26615]: Script /etc/ppp/ip-up started (pid 28238) pppd[26615]: Script /etc/ppp/ipv6-up finished (pid 28183), status = 0x0 pppd[26615]: Script /etc/ppp/ip-up finished (pid 28238), status = 0x0 ip ad sh shows that during ip-pre-up the interface state was already up: ppp-ip-pre-up(ppp1)[28208]: 423: ppp1: <POINTOPOINT,MULTICAST,NOARP,UP,LOWER_UP> mtu 1480 qdisc pfifo_fast state UNKNOWN group default qlen 3 ppp-ip-pre-up(ppp1)[28208]: link/ppp ppp-ip-pre-up(ppp1)[28208]: inet 10.1.0.0 peer 192.168.50.0/32 scope global ppp1 ppp-ip-pre-up(ppp1)[28208]: valid_lft forever preferred_lft forever ulsdns_monitor[28216]: 192.168.50.0 dev ppp1 proto kernel scope link src 10.1.0.0 ppp-ip-pre-up(ppp1)[28208]: inet6 fe80::b4a3:c896:22bc:151f peer fe80::6/128 scope link ppp-ip-pre-up(ppp1)[28208]: valid_lft forever preferred_lft forever In order to make this work properly, a system admin would need to take the same action from ip-up and ipv6-up (probably in a locked manner) if and only if the interface oper status is down. As things stand one cannot depende on ip-up being executed whilst interface is still in down state, and as such, the scription in the man page is wrong too. Signed-off-by: Jaco Kroon <[email protected]>
|
@enaess ok, so this is a possible suggestion then: Deprecate ip-pre-up and just a more generic pre-up? Related to this, the lcp-up/down, which looking at that again, I suggest a consolidated patch which introduces three scripts:
motivation for general init: We need the ability to initialize some state container on our end, this currently happens in the proposed lcp-up script, but it doesn't really matter if this is linked to lcp or not, but we do need to execute stuff very early on, and have certainty in other hooks that the state has been properly reset. There is no other specific hook we can state with 100% certainty that it'll be the first hook, and as such no other script we can rely on to do this. pre-up: As per various discussions we've had now, there is debatable need/use for this, but we still believe that certain actions should be performed PRIOR to the interface being brought up. Yes, the defaults should be secure, and who cares if shaping takes a few seconds after link establishment to come up ... but it's still a good thing to be able to do things like set up NAT prior to allowing routing to happen:
post-down: Well, shutdown really, ie, link has gone down. Just to perform general state cleanup, again, using ip*-post-down for this for protocol agnostic issues is tricky since how does the script know that all relevant protocols have been brought down and no protocols are running any more? |
|
Frankly, this is an area of the code I don't know very well (yet). The introduction of a wait make me a bit eerie, and I too have had to deal with ipv6cp and ipcp coming up independent of each other (it's racy, and it depends on the nature of which protocol completes the message exchanges first). Having a single ip-pre-up to work for both could also seems reasonable, I don't know. As far as I know pppd, it was designed to configure the device and apply values at the earliest convenience. For example, LCP does MTU negotiations and it's based on the device capability for the OS. If the peer A receives an MRU option from peer B, it will try to apply it to the adapter; and the LCP ACK, NAK, or failure depends on the result of the ioctl() being issued are replied back to peer B. Same thing should technically happen to IP(v6) control protocol as well. The protocol is considered UP when both peers agrees on the negotiated parameters. If you refer to RFC1661, what you are looking for in this case is a Network Layer Initialize (Network prior to state UP). Network is UP when all Network based Control Protocols are considered Up. At this point the ppp device is known, the MTU is set, and Authentication is complete. I believe you can write a PPP plugin to act on the state transitions too. In this ip-pre-up, and ipv6-pre-up, what information does your script require? Also, was ip-pre-up introduced after 2.4.9? In case, it hasn't been released, and a collective change to do a network-up script initialization would be preferable. If it has already been released, then maybe support both ip-pre-up and ip6-pre-up. If a ip-pre-down isn't needed, then maybe have it omitted or do the cleanup in ip6-down or ip-down Does this make sense? |
I'd personally just call it pre-up but yes that would actually work, state plainly in docs this is post authentication, but before any sub(?) protocols such as ipcp and/or ipv6cp (ipx has been dropped, but in theory it's possible for others, eg mplscp?), so this isn't prior to IP protocols, coming up, it's before ANY protocols are brought up. The man page is quite plain about blocking too long in many of these scripts, and some of them even go as far as NOT WAITING, pre-up will HAVE to wait by the nature of it, because pppd should not bring up any protocols before it's finished. In theory we can fork it off, but then have a blocking wait on it's completion prior to actually performing "ip li set up", but given that at least my use-case won't block for more than 10-15ms at most this should be perfectly fine. Hell, based on what I understand from the way pppd works, even a blockage of a second or two should be OK even if it may not be ideal.
Correct. lcp-up and lcp-down was probably not the right concept, I suspect init and shutdown are probably better, and we invoke those prior to even starting LCP etc ... and we invoke shutdown last thing before exit kind of thing. Yes, in theory this can be handled by the process invoking pppd, but that's not always viable either, and we do want the unit number during out init stuff, so we can invoke init the moment after the unit number is available, and just prior to nuking the interface again would be good.
Yea, so ip{,v6}-{up,down} executes just after bringing IPCP and IPv6CP up, and just after deconfiguring the specific protocol (if ondemand is set returning to the state wherein ondemand can happen is set up first).
ip-pre-up has been available for as long as I can remember. Looking at the git logs @paulusmack added it back in 2005, even though I could have sworn debian had it already in 2003 when I first set up pppd. No ipv6-pre-up, I suggested it here, but I do tend to agree with you that a single consolidate pre-up is a better approach. Yes, I do want some kind of script that executes after auth before protocols are brought up. The work that should happen here should all be protocol agnostic since without a consolidated pre-up we have no guarantee that the interface will be down. At this stage we care about things like shaping parameters from radius. I would also like to suggest that interface renaming should be possible from this script via things like "ip il set dev pppX name ppp-whatever" (which suggests that auth details should be available to the script). This also reduces the need for the other (seeminly) complicated ifname patch that's being developed on another PR.
Perfect. And I thing we're (mostly?) in agreement. Thus my suggestion of a general init (pppd unit created), pre-up (pre protocol up, ie, up to auth completed) and post-down (all protos down, but unit still available). |
|
I think you are on the right track here. Instead of introducing the ip6cp_pre_up script (with possible race conditions), the entire ip-pre-up should really be renamed to pre-up, or net_init. Though, since this feature has existed for quite some time, and despite the name 'pre-up' may not be the best name, maybe the simplest thing is to accept the name as it is, but move the logic to spawn the script to the right place of the code which make it protocol agnostic? Alternatively, add another hook 'net-init' in addition to 'ip-pre-up' if we feel it would break existing scripts? I tend to favor the former as it eliminates ambiguity (and if this is going to be version 2.5, we already have broken backwards compatibility). Sounds like you and @EasyNetDev should have a conversation of interface naming and how that is done. That could possibly help him accomplish what he needs without an extensive patch set? |
|
Could you contact NetworkManager people? I think they use those scripts and hook a lot, so maybe they could have inputs if changing spawn logic could break something or not. |
|
@pali, I can have a look and reach out to them. Which particular project did you have in mind? The network-manager stuff also include all the vpn plugins. |
Thank you. The race already exists, the man page has this to say about ip-pre-up: There are a few things I'd like to note here: "just before the ppp network interface is brought up" - this statement is no longer true, if it ever truly was, I do think it was "mostly true" due to only ipv4 being typically used (at least, for a portion of the two decades I've been using pppd prior to IPv6 becoming more mainstream). So currently, if IPv6CP (or another CP other than ipv4) causes the interface to become "up" then this statement is false. "At this point the interface exists and has IP addresses assigned but is still down." - the latter portion of this statement (but is still down) accentuates the discussion above, but the portion before that is the important snippet here, in that it also means this logic cannot trivially move and retain the same name since certain scripts may depend on IPv4 information being available already. This part of the documentation is also accurate based on the codebase. "pppd will wait for this script to finish" ... not if IPv6CP comes up first. I like net-init here - any objections? Would suggest we implement net-init, and then flag ip-pre-up as DEPRECATED with the above explanation, just better phrased in some way?
Refer above.
Well, then why not make the breakage spectacular and completely bomb out with an error message about ip-pre-up being broken and should convert to net-init if ip-pre-up exists unless a parameter like ignore-ip-pre-up is passed (aka, "i know what I'm doing")
@EasyNetDev if you look at pppd/ipcp.c, around lines 1938 through 1943, there is logic currently to pick up if the interface name has been changed in the ip-pre-up script - if this moves along to the net-init concept - would you be able to utilize the net-init hook script to perform your interface renaming in an adequate manner? Not that I'm against your conceptual patch (in fact, I like it), it just that maybe this might be "simpler"? So the ppp starts out as pppX, and then in net-init phase you can rename however you like, and pppd will just pick up on that? @enaess there is already ref counts in sifup and sifdown - as such, I'm thinking the following set of scripts are best suited: net-pre-up - executes just prior to the first setifstate call. If setifstate fails there is a possible problem in that a future call to sifup will trigger another invocation. Therefor a better location to add this hook may be just prior to initiating the IP*CP protocols. To accomodate @EasyNetDev and possibly easier conversion of ip-pre-up scripts, accomodate interface renaming here. net-down - similar. Do we invoke it PRIOR to sifstate of after? Also, there are separate logic for counting the refs w.r.t. if_is_up and if6_is_up (simple boolean flag) which seems suspect in terms of logic. What happens if MPLSCP gets added? So pure ref counting here would be best, but I'd pass a string to indicate which protocol (ipcp, ipv6cp, mplscp etc ...) triggered the ups and downs, along with a number of warn/error triggers to hopefully find mismatches or at least enable mismatch detection (or we could even keep a list of "up" protocols so that we can indicate which protocol double-up'ed or down'ed without being up?) Not sure if these are the best places to hook, possibly a more general net-init and net-down script when auth phase is done, and before starting IP*CP, and net-down just prior to tearing down the ppp interface unit. init - general init script, to be invoked directly once the ppp iface has been created. This would then consolidate this PR and #342 into a single PR, presumably with improved structure. Got some time hopefully over the weekend early next week to cook this, and happy to do so. Just don't want to go down a path that will end up not getting the required support here. |
|
I won't consider my opinion an authoritative voice on the matter. It maybe good to solicit feedback from others here as well, like @paulusmack as he will eventually be the deciding factor if a PR gets merged. I didn't realize this earlier, but the ip-pre-up script provides the IPv4 parameters just like the ip-up script does, but before the adapter is set in state IFF_UP. I would imagine that an ipv6 version of the same script would do the same thing (or mplscp for that sake). A net-pre-up would not be able to pass protocol specific data indicating what the resulting negotiated IP-address would be. However, the net-up script could be made into querying the interface for those values. Also, the action of calling sifup, sifdown, or sif6up of sif6down is done directly from the ipcp.c or ip6cp.c which make it tricky as there is no central point to coordinate that. It's like the code needs to be refactored to indicate that the protocol came up and that when all network protocols has come up, there is a final commit message being sent to them where the sifup/sif6up/etc would set the interface in state up. Almost like pppd should have another state that says PHASE_NETWORK_UP after PHASE_NETWORK then "commit" the interface to "IFF_UP" This would 'violate' the original design, and likely require yet another callback into the ipcp/ip6cp (and other/future) protocols. It may very well be the right change, and right now; the interface will be in state IFF_UP when either ip6cp or ipcp transition to up. @pali as far as NetworkManager projects go, it is not using the ip-up/down scripts, but rather a plugin and dbus messages between it and the modem manager, vpn plugins, etc. This would impact maybe the ppp-scripts package on debian, and/or systemd integrations? |
ACK
Correct. The problem currently is the order in which various protocols calls sifup (which is a pppd function, which wraps the system specific mechanisms to up/down an interface), so the constraint that the "interface is configured but not brought up" is violated, in that if IPv6CP brings up the interface first, then once IPCP configures the interface it's not still down, so when ip-pre-up runs, the interface could already be up. One possible change might be to just move ip-pre-up to run before the interface is configured for IPv4 but pass the information that WILL BE USED to the ip-pre-up (or ipv6-pre-up script- although, given the way IPv6 works we really won't have useful information at this stage, link-local addresses only, once these are brought up the normal IPv6 configuration mechanisms (by way of RA and optionally DHCPv6) takes over. So from that perspective I'm not convinced keeping these are useful, and I'm in favour of deprecating the ip
OUCH. OK, so this should be reworked, if there is an up/down for each protocol that does the same thing (ditto for sys-solaris.c) then perhaps we should consolidate that logic into shared code for linux and solaris (in main.c?) which then calls simplified OS specific ifup and ifdown interfaces when the interfaces actually needs to be set up/down (as determined by the OS in use as per sys-*.c). I'll submit a separate PR for this quickly with what I recommend we do. This would also serve as a base then for net-pre-up and net-down as these hooks can then be run from same locations. So once we're happy with that, I can update this PR to be based off of that introducing: net-init (which runs post ppp unit creation, but prior to actually doing anything else) |
|
Scripts as discussed implemented in #367 |
This is required because either ipcp or ipv6cp can come up first, or as
the only protocol, and once IPv6 parameters has been established, there
are actions that may need to be executed prior to bringing the interface
up.
Prior to this we can have an execution order like:
pppd[26615]: Script /etc/ppp/ipv6-up started (pid 28183)
pppd[26615]: Script /etc/ppp/ip-pre-up started (pid 28186)
pppd[26615]: Script /etc/ppp/ip-pre-up finished (pid 28186), status = 0x0
pppd[26615]: Script /etc/ppp/ip-up started (pid 28238)
pppd[26615]: Script /etc/ppp/ipv6-up finished (pid 28183), status = 0x0
pppd[26615]: Script /etc/ppp/ip-up finished (pid 28238), status = 0x0
ip ad sh shows that during ip-pre-up the interface state was already up:
ppp-ip-pre-up(ppp1)[28208]: 423: ppp1: <POINTOPOINT,MULTICAST,NOARP,UP,LOWER_UP> mtu 1480 qdisc pfifo_fast state UNKNOWN group default qlen 3
ppp-ip-pre-up(ppp1)[28208]: link/ppp
ppp-ip-pre-up(ppp1)[28208]: inet 10.1.0.0 peer 192.168.50.0/32 scope global ppp1
ppp-ip-pre-up(ppp1)[28208]: valid_lft forever preferred_lft forever
ulsdns_monitor[28216]: 192.168.50.0 dev ppp1 proto kernel scope link src 10.1.0.0
ppp-ip-pre-up(ppp1)[28208]: inet6 fe80::b4a3:c896:22bc:151f peer fe80::6/128 scope link
ppp-ip-pre-up(ppp1)[28208]: valid_lft forever preferred_lft forever
In order to make this work properly, a system admin would need to take
the same action from ip-up and ipv6-up (probably in a locked manner) if
and only if the interface oper status is down.
As things stand one cannot depende on ip-up being executed whilst
interface is still in down state, and as such, the scription in the man
page is wrong too.