#5617 closed enhancement (complete)

Reselect subnet after RADIUS response

Reported by: tomek Owned by: fdupont
Priority: medium Milestone: Kea1.4
Component: hook-radius Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

A RADIUS hook user is dealing with the following issue:

PROBLEM:

He has two subnets. First with two pools ("gamers" and "fp"), second with different pools ("internet", "test"). Both subnets have the same relay ip-addresses specified. This is uncommon, but legal setup. One relay can handle multiple subnets.

The RADIUS response has Framed-Pool attribute that points to "internet". Unfortunately, Kea first selects the first subnet, then tries to select a pool from that subnet, but unfortunately there is no such pool, so pool and lease selection fails.

Here's the redacted config:

"subnet4": [
                {
                        "subnet": "1.2.3.16/28",
                        "id": 13100,
                        "pools": [
                                { "pool": "1.2.3.18 -  1.2.3.21" , "client-class": "gamers" } ,
                                { "pool": "1.2.3.22 -  1.2.3.25" , "client-class": "fp" }
                        ],
                        "relay": { "ip-addresses": ["1.2.5.20","1.2.5.21"] },
                        "option-data": [ { "name": "routers", "data": "1.2.3.17"  } ]
                },

                {    
                        "subnet": "1.2.3.20/28",
                        "id": 13103,
                        "pools": [
                                { "pool": "1.2.3.24 -  1.2.3.30" , "client-class": "internet" } ,
                                { "pool": "1.2.3.17 -  1.2.3.19" , "client-class": "test" }
                        ],
                        "relay": { "ip-addresses": ["1.2.5.20","1.2.5.21"] },
                        "option-data": [ { "name": "routers", "data": "1.2.5.20"  }  ]
                }
    ]

SOLUTION:

  1. Add configuration parameter reselect-subnet. If enabled, it will enable the following mechanism. The default should be false.
  1. extend the RADIUS callout installed on subnet4_select with this code (which is modified version of CfgSubnets4::selectSubnet(selector)
    • go over each subnet:
      • if relay ip-address dont' match, go to the next subnet
      • go over all pools if there is one that can be used based on Framed-Pool, use this subnet
      • if not, go to the next subnet

If Access-Reject is returned, the callout should return NULL (telling kea to use no subnets, effectively causing Kea to not respond).

Subtickets

Change History (10)

comment:1 Changed 18 months ago by fdupont

  • Owner set to fdupont
  • Priority changed from medium to high
  • Status changed from new to accepted

To be addressed as soon as #5605 (premium trac5605a branch) is merged.

comment:2 Changed 18 months ago by fdupont

Split the flag into pool (check available pool with client class / Framed-Pool), address (check reserved address is in subnet range) and (provision, i.e. not fully defined nor implemented) attribute (subnet ID from a RADIUS attribute).
Adopted the idea to reuse subnet code so moving the selector filling to library.

comment:3 Changed 18 months ago by fdupont

Code done and debugged for reject, subnet reselected by pool/client-class and by range/reserved address (for the last one I even got a conflict because the lease file kept previous subnet). Of course unit tests are still to be done (same than #5612 but have a small case matrix).
BTW I had to replace getBySubnetId by getSubnet to match exact type in handle set/get argument. A comment about the second in the header file warns about bad performance (full scan) and in the code about it should be updated to use the multi-index both for cfg_subnets[46].{h,cc}. This should be addressed (took 5 mn to be fixed) but not now.

comment:4 Changed 18 months ago by fdupont

  • Priority changed from high to medium

comment:5 Changed 18 months ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Finished unit tests: ready for review. Don't forget the Kea trac5617 branch too.

comment:6 Changed 18 months ago by tomek

  • Owner changed from UnAssigned to tomek

comment:7 follow-up: Changed 18 months ago by tomek

  • Owner changed from tomek to fdupont

KEA CODE

Please update doc/guide/hooks-radius.xml with the description of new parameters.
The text you provided in radius.dox will be ok, but please make sure you omit
reselect-subnet-attribute and format everything properly.

Also please update doc/examples/kea4/hooks-radius.json to mention two
new parameters.

PREMIUM CODE

I think you made the configuration too complex. I would simply go with a single switch and called it reselect-subnet.
But since the code is already written, let it stay the way it is done. But please remove
reselect-subnet-attribute. It doesn't work (not implemented) and as far as I can tell, the
customer didn't request it. Adding just the knob, without the code behind it, is just confusing.

radius_access.h
RadiusAuthEnv? constructor: The comment should say what values are allowed for uint32_t family
(these are AF_INET or AF_INET6, right?)

buildAuth has been changed to use reference for subnet_id. If this value may change,
it has to be mentioned in the description of that method and also in RadiusAuthEnv ctor.

New reselectSubnet methods require some description. Stating their name is not a sufficient
description. Added. Please pull and review.

radius_access.cc
reselectSubnet(query, subnet_id, addr): I'm not sure it's reasonable to continue going through
the list of subnets once the first one was found. The subnets are not supposed to overlap.
But to be on the safe side, it's better to keep it going. Perhaps adding a @todo in the code pointing potential optimization?

radius.h
As requested earlier, please remove reselect_subnet_attribute_.

radius.dox
Please remove reselect-subnet-attribute.

access_unittests.cc
There should be some comments in AccessTest.hostReselect4 and hostReselect6.

I've added comments for hostReselect4. Please add similar for hostReselect6.

A question about hostReselect4. Why the second host cache entry is also negative?
It contains radius context, so it is a real reservation, not a negative cache.
I marked the place in the code (line 1560). Please either explain why it's
true or change it to false.


The code builds fine and unit-tests pass on Mac OS.

This change requires a Premium Changelog. Please consider the following:

48.	[func]		fdupont
	An ability to reselect subnet based on RADIUS response has been
	added to the RADIUS hook. The new parameters -
	reselect-subnet-address and reselect-subnet-pool - are
	now supported.
	(Trac #5617, git tbd)

comment:8 in reply to: ↑ 7 Changed 18 months ago by fdupont

  • Owner changed from fdupont to tomek

Replying to tomek:

KEA CODE

Please update doc/guide/hooks-radius.xml with the description of new parameters.
The text you provided in radius.dox will be ok, but please make sure you omit
reselect-subnet-attribute and format everything properly.

=> done.

Also please update doc/examples/kea4/hooks-radius.json to mention two
new parameters.

=> done

I believe the Kea changes are ready to be merged, aren't they?

PREMIUM CODE

I think you made the configuration too complex.

=> pool mismatch is not enough because with enough addresses people
will use reserved addresses. Now even in standard Kea nowhere it is written
or enforced that a reserved address should match the parent subnet range...
So I went to describe all possible ways to add another criterion in subnet
selection.

But please remove reselect-subnet-attribute.

=> no problem. It can be added in future (code is easy, the only hairy point
is the choice of the attribute where to find the subnet ID...).

radius_access.h
buildAuth has been changed to use reference for subnet_id. If this value may change,
it has to be mentioned in the description of that method and also in RadiusAuthEnv ctor.

=> done (with different comments so the intention should be clear).

New reselectSubnet methods require some description. Stating their name is not a sufficient
description. Added.

=> nothing pulled. I added my own comment explaining what these methods do.

radius_access.cc
reselectSubnet(query, subnet_id, addr): I'm not sure it's reasonable to continue going through
the list of subnets once the first one was found. The subnets are not supposed to overlap.

=> not only there can overlap but they can be the same as soon as the range text is different.
And it is easier to just use the same algorithm (opportunity to factor it later).

radius.h
As requested earlier, please remove reselect_subnet_attribute_.

=> done.

radius.dox
Please remove reselect-subnet-attribute.

=> done (and in other files given by grep too).

access_unittests.cc
There should be some comments in AccessTest.hostReselect4 and hostReselect6.

=> pull and completed.

I've added comments for hostReselect4. Please add similar for hostReselect6.

=> cut and pasted.

A question about hostReselect4. Why the second host cache entry is also negative?

=> because it has no address reservation nor hostname so it is not a legal host entry.
To mark it as negative make sure the existing host code won't be disturbed.

It contains radius context, so it is a real reservation, not a negative cache.

=> it is not a question of user context (cf the code setting the negative cache
flag at the end of terminate / radius_access.cc.

comment:9 Changed 18 months ago by fdupont

  • Owner changed from tomek to fdupont

comment:10 Changed 18 months ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged. Closing.

Note: See TracTickets for help on using tickets.