Opened 5 years ago

Closed 5 years ago

#3705 closed enhancement (complete)

RFC6422 support: Relay-Supplied Options

Reported by: fltemplin Owned by: marcin
Priority: medium Milestone: Kea0.9.1
Component: Unclassified 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

Under ISC DHC Release 4.3.1, I added support for RFC6422 to the DHCPv6 server
code. Please see below for the diffs.

The code currently does not do any sanity checking of the RSOO option, so the
server basically accepts anything the relay sends it. This is intentional, as I want
to be able to deliver Vendor-Specific Information Options. I am also not sure
how this is supposed to work if there are multiple relays in the path, so
someone should probably investigate that.

Please consider adding RFC6422 to an upcoming kea distribution.

Thanks - Fred
fred.l.templin@…

---
--- dhcpv6.c.orig 2015-02-05 12:06:12.365455203 -0800
+++ dhcpv6.c 2015-02-06 13:09:05.075764075 -0800
@@ -1422,6 +1422,9 @@

static struct reply_state reply;
struct option_cache *oc;
struct data_string packet_oro;

+#if 1 /* #ifdef CONFIG_AERO */
+ struct packet *cpacket;
+#endif

int i;

memset(&packet_oro, 0, sizeof(packet_oro));

@@ -1625,6 +1628,36 @@

required_opts_solicit,
&packet_oro);

+#if 1 /* #ifdef CONFIG_AERO */
+ /*
+ * RFC6422 - store any options inserted by the relay in an RSOO
+ * option. This might only work when there is a single relay in
+ * the path. Don't know for sure, so someone else who knows more
+ * about this code should investigate.
+ */
+ if ((cpacket = packet->dhcpv6_container_packet)) {
+ struct data_string enc_rsoo_data;
+
+ oc = lookup_option(&dhcpv6_universe, cpacket->options, D6O_RSOO);
+ if (oc == NULL)
+ goto no_rsoo;
+
+ if (!evaluate_option_cache(&enc_rsoo_data, NULL, NULL, NULL,
+ NULL, NULL, &global_scope, oc, MDL))
+ goto exit;
+
+ if (!packet6_len_okay((char *)enc_rsoo_data.data,
+ enc_rsoo_data.len))
+ goto exit;
+
+ if ((sizeof(reply.buf) - reply.cursor) >= enc_rsoo_data.len) {
+ memcpy((char *)(reply.buf.data + reply.cursor),
+ (char *)(enc_rsoo_data.data), enc_rsoo_data.len);
+ reply.cursor += enc_rsoo_data.len;
+ }
+ }
+no_rsoo:
+#endif

/* Return our reply to the caller. */
reply_ret->len = reply.cursor;
reply_ret->buffer = NULL;

Subtickets

Change History (10)

comment:1 Changed 5 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea0.9.1
  • Priority changed from high to low

comment:2 Changed 5 years ago by marcin

  • Priority changed from low to medium

comment:3 Changed 5 years ago by tomek

  • Owner set to tomek
  • Status changed from new to assigned

comment:4 Changed 5 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from assigned to reviewing
  • Summary changed from RFC6422 support to RFC6422 support: Relay-Supplied Options

The code for Kea is now ready for review. See trac3705 branch for details.

Implemented functionalities:

  • the server is able to process RSOO, extract options and after certain checks, send them back to the client
  • currently only option 65 is marked by IANA as RSOO-enabled
  • it is possible to mark other options as RSOO-enabled (new parameter was added, see section 8.2.15 Relay Supplied Options).

Proposed ChangeLog?:

9xx.	[func]		tomek
	The DHCPv6 server now supports Relay-Supplied Options option,
	defined RFC6422. The relay can insert options that will be
	sent back to the client if certain criteria are met.
	(Trac #3705, git tbd)

Note to Fred: this code is now ready for our internal review. I strongly recommend
that you wait till the review process is complete and the code appears on our master
branch, but If you need it immediately, you can checkout branch trac3705.

comment:5 Changed 5 years ago by sar

  • Owner changed from UnAssigned to sar

comment:6 follow-up: Changed 5 years ago by sar

  • Owner changed from sar to tomek

I have fixed several typos and committed those changes, please do a pull before making more modifications.

doc/guide/dhcp6-srv.xml

Table 8.1 "List of standard DHCPv6 options" should be updated to include:
D6O_ERP_LOCAL_DOMAIN_NAME, D6O_RSOO and D6O_CLIENT_LINKLAYER_ADDR

src/bin/dhcp6/tests/config_parser_unittest.cc

Should there also be a unit test to check what happens if the administrator includes an inappropriate value such as a negative numbers, misspelled or truncated option names or option names that don't exist at all?

src/lib/dhcp/std_options_defs.h

copyright (update done)

In the defintion for rsoo the encapsulation field is set to "dhcp4" which doesn't seem correct.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

In the rsoo test the client never asks for the 3 options the relay is attempting to supply. I take it that the server sends all options it can even if the client doesn't request them?

In the rsoo2relays test it would be good to use different values for the two relays for hop count, linkaddr and peeraddr. As an aside, shoudln't the server be verifying that the hop count is 0 for relay1 and 1 for relay2? (or perhaps such a validity check is skipped for this testing?)

Shouldn't there be a test to verify that if the server has been configured for an option and the relay supplies it that the server chooses its own version? I think this can be added to the two relays check by adding a fourth option 140 which one or both of the relays try to supply but for which the server has a configuration.

src/lib/dhcp/tests/pkt_captures6.cc

Part of the comment for captureRelayed2xRSOO seems wrong to me (or maybe I'm just reading it wrong.) It seems to me that it should be something like this:

/// RELAY-FORW
///  - relay message option
///      - RELAY-FORW
///          - rsoo (66)
///              - option 255 (len 4)
///              - option 256 (len 9)
///          - remote-id option (37) (invalid)
///          - relay message option
///             - SOLICIT
///                  - client-id option
///                  - ia_na option
///                  - elapsed time
///                  - ORO
///  - interface-id option (18)
///  - remote-id option (37)

src/lib/dhcpsrv/cfg_option.cc

copyright (update done)

I wonder if it might be better to add a function addDefaultRSOO() to cfg_option.cc and cfg_option.h and move the knowledge of the default list out of json_config_parser.cc. The discussion of what is the proper default seems to be in cfg_option.h.

src/lib/dhcpsrv/cfg_option.h

copyright (update done)

Line 364 - The comment states "...the user may add extra options here." But the comment is describing clearRSOO() so it doesn't quite fit there. It would either go with addRSOO() indicating the user could add them there, or in the parsing code if the want it to be a default (or see above comment about adding a function to set the defaults.)

src/lib/dhcpsrv/tests/cfg_option_unittest.cc

copyright (update done)

Is it worth checking any of the following?

  • Adding an option twice and verifying that it was added and that the second add didn't fail.
  • Adding an option to the list and then removing it.
  • Removing the default and then re-adding it

General thoughts

These are some extra thoughts I had during the review. I don't think they need to be handled as part of this ticket. If they seem useful we can create new tickets for them.

Should we supply a way to disable this option even for the defaults? If I understand the code correctly currently there is no way to eliminate the default options.

Should there be any way to disable the option for only some relays? (The RFC suggests having an a relay closer to the server drop the packet if an untrusted external relay is adding these options but would it be useful for users to have more control at the server?)

Slightly updated suggestion for ChangeLog? Note

9xx. [func] tomek

The DHCPv6 server now supports the Relay-Supplied Options option,
as defined in RFC6422. The relay can insert options in the relay forward
message that the server will send back to the client if certain criteria are met.
(Trac #3705, git tbd)

comment:7 Changed 5 years ago by marcin

  • Owner changed from tomek to marcin

comment:8 in reply to: ↑ 6 ; follow-up: Changed 5 years ago by marcin

  • Owner changed from marcin to sar

Replying to sar:

I have fixed several typos and committed those changes, please do a pull before making more modifications.

doc/guide/dhcp6-srv.xml

Table 8.1 "List of standard DHCPv6 options" should be updated to include:
D6O_ERP_LOCAL_DOMAIN_NAME, D6O_RSOO and D6O_CLIENT_LINKLAYER_ADDR

Added them to the list.

src/bin/dhcp6/tests/config_parser_unittest.cc

Should there also be a unit test to check what happens if the administrator includes an inappropriate value such as a negative numbers, misspelled or truncated option names or option names that don't exist at all?

I added the test for negative values and for the non-existing option names. The test also verifies that the error message contains the config line number.

src/lib/dhcp/std_options_defs.h

copyright (update done)

In the defintion for rsoo the encapsulation field is set to "dhcp4" which doesn't seem correct.

I changed it. But, it strikes me that those option spaces are not really used for anything right now. All those options that encapsulate some option space have special processing in the code.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

In the rsoo test the client never asks for the 3 options the relay is attempting to supply. I take it that the server sends all options it can even if the client doesn't request them?

Yes, this is how it works. I think we also need to finally implement the configuration switch which will configure some options to be always sent, regardless if requested or not. Do you think it is useful? If yes, I will open a ticket.

In the rsoo2relays test it would be good to use different values for the two relays for hop count, linkaddr and peeraddr. As an aside, shoudln't the server be verifying that the hop count is 0 for relay1 and 1 for relay2? (or perhaps such a validity check is skipped for this testing?)

Perhaps it should verify. Maybe add another ticket for this?

Shouldn't there be a test to verify that if the server has been configured for an option and the relay supplies it that the server chooses its own version? I think this can be added to the two relays check by adding a fourth option 140 which one or both of the relays try to supply but for which the server has a configuration.

I added a test to exercise this scenario.

src/lib/dhcp/tests/pkt_captures6.cc

Part of the comment for captureRelayed2xRSOO seems wrong to me (or maybe I'm just reading it wrong.) It seems to me that it should be something like this:

/// RELAY-FORW
///  - relay message option
///      - RELAY-FORW
///          - rsoo (66)
///              - option 255 (len 4)
///              - option 256 (len 9)
///          - remote-id option (37) (invalid)
///          - relay message option
///             - SOLICIT
///                  - client-id option
///                  - ia_na option
///                  - elapsed time
///                  - ORO
///  - interface-id option (18)
///  - remote-id option (37)

Corrected.

src/lib/dhcpsrv/cfg_option.cc

copyright (update done)

I wonder if it might be better to add a function addDefaultRSOO() to cfg_option.cc and cfg_option.h and move the knowledge of the default list out of json_config_parser.cc. The discussion of what is the proper default seems to be in cfg_option.h.

I agree. I moved this whole logic to the new class !CfgRSOO and the constructor adds a default options. We may extend it in the future if we need the addDefault.

src/lib/dhcpsrv/cfg_option.h

copyright (update done)

Line 364 - The comment states "...the user may add extra options here." But the comment is describing clearRSOO() so it doesn't quite fit there. It would either go with addRSOO() indicating the user could add them there, or in the parsing code if the want it to be a default (or see above comment about adding a function to set the defaults.)

Fixed.

src/lib/dhcpsrv/tests/cfg_option_unittest.cc

copyright (update done)

Is it worth checking any of the following?

  • Adding an option twice and verifying that it was added and that the second add didn't fail.
  • Adding an option to the list and then removing it.
  • Removing the default and then re-adding it

Added some unit tests to cover those.

General thoughts

These are some extra thoughts I had during the review. I don't think they need to be handled as part of this ticket. If they seem useful we can create new tickets for them.

Should we supply a way to disable this option even for the defaults? If I understand the code correctly currently there is no way to eliminate the default options.

Right, but they will be only sent if the relay puts them. I'd wait for the feedback from users, but it may also be good to check with Tomek.

Should there be any way to disable the option for only some relays? (The RFC suggests having an a relay closer to the server drop the packet if an untrusted external relay is adding these options but would it be useful for users to have more control at the server?)

Possibly, but the relay closest to the client has the best knowledge of the client, no? Again, I think we should wait for the feedback from the users.

Slightly updated suggestion for ChangeLog? Note

9xx. [func] tomek

The DHCPv6 server now supports the Relay-Supplied Options option,
as defined in RFC6422. The relay can insert options in the relay forward
message that the server will send back to the client if certain criteria are met.
(Trac #3705, git tbd)

comment:9 in reply to: ↑ 8 Changed 5 years ago by sar

  • Owner changed from sar to marcin

I fixed a couple more typos.

There are a couple more items below, I don't think I need to do another review.

We can talk some more about if we need tickets for some of the items.

Replying to marcin:

Replying to sar:

src/lib/dhcp/std_options_defs.h

copyright (update done)

In the defintion for rsoo the encapsulation field is set to "dhcp4" which doesn't seem correct.

I changed it. But, it strikes me that those option spaces are not really used for anything right now. All those options that encapsulate some option space have special processing in the code.

We should probably add a definition for ERP_LOCAL_DOMAIN_NAME.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

In the rsoo test the client never asks for the 3 options the relay is attempting to supply. I take it that the server sends all options it can even if the client doesn't request them?

Yes, this is how it works. I think we also need to finally implement the configuration switch which will configure some options to be always sent, regardless if requested or not. Do you think it is useful? If yes, I will open a ticket.

I think there are some clients that don't do a good job asking for objects (probably more for v4 than v6) It would be good to have such a configure switch, but I'm not sure about its priority.

It appears as if we have an inconsistent method of processing the options. If the server has information for an option it only inserts it into the response if the client requested it via ORO. If the relay has information we always insert it even if it isn't in the ORO. If my understanding is correct we will get odd results when the server has information to override the relay. If the client doesn't request it then the relay info is used and if the client does request it the server info is used.

If this is an issue we can create a new ticket for it. I don't think it is necessary for 0.9.1 and don't even know how often the admins will want to do an override of the relay info.

In the rsoo2relays test it would be good to use different values for the two relays for hop count, linkaddr and peeraddr. As an aside, shoudln't the server be verifying that the hop count is 0 for relay1 and 1 for relay2? (or perhaps such a validity check is skipped for this testing?)

Perhaps it should verify. Maybe add another ticket for this?

The link and peer adds were updated but the hop count is the same for both relays.

I think they do get verified elsewhere if a packet is normally processed. But it would be useful to confirm that.

src/lib/dhcp/tests/pkt_captures6.cc

Part of the comment for captureRelayed2xRSOO seems wrong to me (or maybe I'm just reading it wrong.) It seems to me that it should be something like this:

/// RELAY-FORW
///  - relay message option
///      - RELAY-FORW
///          - rsoo (66)
///              - option 255 (len 4)
///              - option 256 (len 9)
///          - remote-id option (37) (invalid)
///          - relay message option
///             - SOLICIT
///                  - client-id option
///                  - ia_na option
///                  - elapsed time
///                  - ORO
///  - interface-id option (18)
///  - remote-id option (37)

Corrected.

There were two other items that weren't updated - the last RELAY-FORW was converted to a relay-message option and the last two options had their indentation changed to be in the outermost scope.

General thoughts

These are some extra thoughts I had during the review. I don't think they need to be handled as part of this ticket. If they seem useful we can create new tickets for them.

Should we supply a way to disable this option even for the defaults? If I understand the code correctly currently there is no way to eliminate the default options.

Right, but they will be only sent if the relay puts them. I'd wait for the feedback from users, but it may also be good to check with Tomek.

Should there be any way to disable the option for only some relays? (The RFC suggests having an a relay closer to the server drop the packet if an untrusted external relay is adding these options but would it be useful for users to have more control at the server?)

Possibly, but the relay closest to the client has the best knowledge of the client, no? Again, I think we should wait for the feedback from the users.

Yes we can wait and see how this gets used.

comment:10 Changed 5 years ago by marcin

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

Merged with commit 4772ee589712f5359ecbd79ebf71fbc7bb68741b.

Note: See TracTickets for help on using tickets.