pppoe-server: Ability to ignore the client service name and respond a… #27
Conversation
…nyway. Signed-off-by: Jaco Kroon <jaco@uls.co.za>
| static InterfaceConfig globalconfig = { | ||
| .draining = DRAIN_OFF, | ||
| .ignore_service_name = 0, | ||
| }; | ||
|
|
There was a problem hiding this comment.
This is aimed towards being able to set configuration on a per-interface basis rather than only globally. So unfortunately there's a mix and match here.
| @@ -623,7 +626,7 @@ processPADI(Interface *ethif, PPPoEPacket *packet, int len) | |||
| unsigned char *myAddr = ethif->mac; | |||
|
|
|||
| /* Ignore PADI's if we're draining the server */ | |||
| if (draining != DRAIN_OFF) { | |||
| if (interface_get_config_int(*ethif, draining) != DRAIN_OFF) { | |||
There was a problem hiding this comment.
This will if interface config is <0 retrieve global config instead.
| @@ -687,7 +690,7 @@ processPADI(Interface *ethif, PPPoEPacket *packet, int len) | |||
| ok = 1; /* No Service-Name tag in PADI */ | |||
| } | |||
|
|
|||
| if (!ok) { | |||
| if (!ok && !interface_get_config_int(*ethif, ignore_service_name)) { | |||
There was a problem hiding this comment.
Here we would normally return, but if ignore_service_name is true, we will respond.
| @@ -687,7 +690,7 @@ processPADI(Interface *ethif, PPPoEPacket *packet, int len) | |||
| ok = 1; /* No Service-Name tag in PADI */ | |||
| } | |||
|
|
|||
| if (!ok) { | |||
| if (!ok && !interface_get_config_int(*ethif, ignore_service_name)) { | |||
There was a problem hiding this comment.
My problem is lower down, when we add service names to the PADO, the code currently either responds with a blank service name, or ALL service names.
Obviously, in the case where we respond to 'any' (Juniper describes it this way: https://www.juniper.net/documentation/us/en/software/junos/subscriber-mgmt-vlan/topics/concept/pppoe-service-name-tables-understanding.html) it's unclear what we should respond with. For that matter:
If the PADI contains at a service name, does it make sense to respond with service names other than
Unfortunately I can't vouch for other PPPoE servers, but it seems that at least whatever Openserve (I suspect Huawei and possibly CISCO previously) simply reflects back whatever session name was requested, so I suspect this is the way to go here. As such, I'm inclined to rework this code in it's entirety a bit to (if ignore is set) not even check the OK, but simply copy back the requested service name.
current variations (pre-patch):
client sets service name:
matches: we respond with full list of locally configured names (or empty name)
no match: we ignore (drop).
client doesn't set service name:
we respond with full list (or empty name).
What I think is the right approach:
if client client sends service name:
if ignoring: reflect requested name back
if matches: reflect requested name back (but I suggest we keep existing behaviour)
if no matches: ignore (drop)
if client did not service service name (even though this is against RFC behaviour which states "The PADI packet MUST contain exactly one TAG of TAG_TYPE Service-Name"), keep current behaviour.
The RFC states the following:
The PADO packet MUST contain one AC-Name TAG containing the Access
Concentrator's name, a Service-Name TAG identical to the one in the
PADI, and any number of other Service-Name TAGs indicating other
services that the Access Concentrator offers. If the Access
Concentrator can not serve the PADI it MUST NOT respond with a PADO.
The way I read this, and what I really would suggest we do:
If we're filtering service names and we don't match - drop the PADI.
If the client put a service-name (non-empty), reflect it back.
If we don't have Service-Names configured and the client didn't send a Service-Name, put the empty Service-Name.
If we have service names configured and the client didn't provide a non-empty service name, list them all
Does this make sense?
There was a problem hiding this comment.
I don't think it makes sense. If a client asks for service ABC in its PADI and we don't have that service name, then IMO we should not respond.
There was a problem hiding this comment.
This shouldn't be an unconditional ignore, it should be an explicitly configured thing.
We're seeing some routers that has to have a service name and people are setting it randomly and it's impossible to set it on pppoe-server side to all possible values, even just considering a few characters.
Functionality is required, the question is what's the best approach.
Juniper docs is the clearest on what other parties are doing. And what I've sniffed re what Openserve does probably speaks towards Huawei and/or CISCO.
There was a problem hiding this comment.
IMO, those routers are broken and breaking rp-pppoe to support them means we'll be broken.
But OK. I understand your dilemma. I'd agree to merge this if it were enabled only by a command-line option like -Z meaning "Allow any service-name at all and simply reflect it back to the client."
There was a problem hiding this comment.
I was thinking of doing a -c "set ignore_service_name on", such that -c (already in use so something else) basically ends up executing a control command during startup, or even -l filename that reads one line at a time and executes it as if it was a control connection.
There was a problem hiding this comment.
Sure, as long as there's a way to do it and it's off by default, I don't really have an opinion on exactly how it's to be implemented.
There was a problem hiding this comment.
@jkroonza, sorry, I forgot about this. Is it ready to merge in your opinion?
There was a problem hiding this comment.
I think the bits that are there is operational, but either a way to parse and load a config file that executes configuration commands, or to specify commands from the CLI is still needed I reckon.
There was a problem hiding this comment.
Hmm. OK. Then I don't think it's ready to merge.
…nyway.