Opened 6 years ago

Closed 6 years ago

#3210 closed defect (complete)

Kea4 must not echo back client-id

Reported by: tomek Owned by: tomek
Priority: very high Milestone: Kea0.8
Component: dhcp4 Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Low
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

That's a bug. DHCPv6 requires to echo back client-id. DHCPv4 forbids it.

Subtickets

Change History (7)

comment:1 Changed 6 years ago by tomek

  • Owner set to UnAssigned
  • Status changed from new to reviewing

Again, the issues became more complex after a closer look. There's RFC6842 that changes the behaviour:

pre-RFC6842 (or vanilla RFC2131): MUST NOT send client-id
updated RFC2131 (RFC682 changes): MUST send client-id

Since we need both behaviours, a new flag to control this was needed :( Yay for parser updates.

Oh well. So be it. The code is ready for review.

comment:2 follow-ups: Changed 6 years ago by marcin

  • Owner changed from UnAssigned to tomek

Reviewed commit: 4a08e3662e7720ac6c96b7d585084bbe4033c29b

doc/guide/bind10-guide.xml
Apparently, the RFC6842 says that the client id must be echoed back to the client IF client id has been included in the client's message. I suggest that the bind10-guide is updated accordingly. Otherwise, it gives an impression that client-id must be always included.

src/bin/dhcp4/config_parser.cc
commitGlobalOptions: why is this function called like this? It merely stores the new ''echo-client-id'' option in the CfgMgr.

Question: do you think it makes sense to have a per-subnet configuration of this new option? For the current use cases I am sure it is ok to have the global setting but I can imagine that in the future we may want to support a number of devices which are not RFC6842 conformant for which the client-id echoing should be disabled while for other devices it should be still enabled.

src/bin/dhcp4/tests/config_parser_unittest.cc
The echoClientId test is insufficient because it doesn't check the value of ''echo-client-id'' flag before applying the configuration. It may be the case that the parser doesn't parse this new option at all and thus this check:

    EXPECT_FALSE(CfgMgr::instance().echoClientId());

will always pass because the parser simply wouldn't modify this value. I think you should either set this value in the CfgMgr to true before applying first configuration or you should swap the configurations (so as the one which sets it goes first) and prior to applying this configuration perform ASSERT_FALSE.

src/bin/dhcp4/dhcp4_test_utils.h
checkClientId: After recent changes to this function you should extend the description of this function to say that the function checks presence of the client-id if specified so in the CfgMgr and checks the absence in other case. Otherwise it looks like you always expect that client-id is present.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
Dhcpv4SrvTest::discoverEchoClientId: The test description is a bit ambiguous. It may be unclear what it means that the ''echoing back client-id'' is controllable.

The checks in line 705 and 713 look like duplicates of what the ''checkClientId'' function is already doing.

The same comments apply to Dhcpv4SrvTest::requestEchoClientId

src/lib/dhcpsrv/cfgmgr.h
I fixed a few trivial issues and pushed changes to the branch.

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
CfgMgrTest::echoClientId: Just a thought. The test could also check that the flag can be set to TRUE after setting it to FALSE. However, I admit that it is not very likely to fail :-)

We need a ChangeLog for this because it has a user impact.

comment:3 in reply to: ↑ 2 Changed 6 years ago by marcin

Replying to marcin:

Reviewed commit: 4a08e3662e7720ac6c96b7d585084bbe4033c29b

doc/guide/bind10-guide.xml
Apparently, the RFC6842 says that the client id must be echoed back to the client IF client id has been included in the client's message. I suggest that the bind10-guide is updated accordingly. Otherwise, it gives an impression that client-id must be always included.

src/bin/dhcp4/config_parser.cc
commitGlobalOptions: why is this function called like this? It merely stores the new ''echo-client-id'' option in the CfgMgr.

Question: do you think it makes sense to have a per-subnet configuration of this new option? For the current use cases I am sure it is ok to have the global setting but I can imagine that in the future we may want to support a number of devices which are not RFC6842 conformant for which the client-id echoing should be disabled while for other devices it should be still enabled.

src/bin/dhcp4/tests/config_parser_unittest.cc
The echoClientId test is insufficient because it doesn't check the value of ''echo-client-id'' flag before applying the configuration. It may be the case that the parser doesn't parse this new option at all and thus this check:

    EXPECT_FALSE(CfgMgr::instance().echoClientId());

will always pass because the parser simply wouldn't modify this value. I think you should either set this value in the CfgMgr to true before applying first configuration or you should swap the configurations (so as the one which sets it goes first) and prior to applying this configuration perform ASSERT_FALSE.

One correction. The default value is true, so there is no reason to set it to true explicitly. However, it should be checked that the value IS true before applying configuration.

src/bin/dhcp4/dhcp4_test_utils.h
checkClientId: After recent changes to this function you should extend the description of this function to say that the function checks presence of the client-id if specified so in the CfgMgr and checks the absence in other case. Otherwise it looks like you always expect that client-id is present.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
Dhcpv4SrvTest::discoverEchoClientId: The test description is a bit ambiguous. It may be unclear what it means that the ''echoing back client-id'' is controllable.

The checks in line 705 and 713 look like duplicates of what the ''checkClientId'' function is already doing.

The same comments apply to Dhcpv4SrvTest::requestEchoClientId

src/lib/dhcpsrv/cfgmgr.h
I fixed a few trivial issues and pushed changes to the branch.

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
CfgMgrTest::echoClientId: Just a thought. The test could also check that the flag can be set to TRUE after setting it to FALSE. However, I admit that it is not very likely to fail :-)

We need a ChangeLog for this because it has a user impact.

comment:4 Changed 6 years ago by tomek

  • Milestone changed from Sprint-DHCP-20131016 to Sprint-DHCP-20131204

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

  • Owner changed from tomek to marcin

Replying to marcin:

Reviewed commit: 4a08e3662e7720ac6c96b7d585084bbe4033c29b

doc/guide/bind10-guide.xml
Apparently, the RFC6842 says that the client id must be echoed back to the client IF client id has been included in the client's message. I suggest that the bind10-guide is updated accordingly. Otherwise, it gives an impression that client-id must be always included.

Clarified.

src/bin/dhcp4/config_parser.cc
commitGlobalOptions: why is this function called like this? It merely stores the new ''echo-client-id'' option in the CfgMgr.

Because it is the first global parameter. The first, but definitely not the last one. For example, dibbler-server has at least 24 global scoped parameters (work-dir, log parameters, auth info, cache sizes, performance-mode and tons of other stuff). Added a comment that clarifies the name.

Question: do you think it makes sense to have a per-subnet configuration of this new option?

No. Let's try to follow the latest spec. If users complain about that, there's the (global) switch. If users complain about that, then we'll ask for detailed explanation why that is not sufficient. If they convince us, then we will invest time in fancy migration procedures with per-subnet information.

src/bin/dhcp4/tests/config_parser_unittest.cc
The echoClientId test is insufficient because it doesn't check the value of ''echo-client-id'' flag before applying the configuration.

The test has been extended. It checks the default (before any configuration tricks), then "echo-client-id false" configuration and finally a reverse back to the default.

src/bin/dhcp4/dhcp4_test_utils.h
checkClientId: After recent changes to this function you should extend the description of this function to say that the function checks presence of the client-id if specified so in the CfgMgr and checks the absence in other case. Otherwise it looks like you always expect that client-id is present.

Done.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
Dhcpv4SrvTest::discoverEchoClientId: The test description is a bit ambiguous. It may be unclear what it means that the ''echoing back client-id'' is controllable.

Clarified.

The checks in line 705 and 713 look like duplicates of what the ''checkClientId'' function is already doing.

Removed.

The same comments apply to Dhcpv4SrvTest::requestEchoClientId

Removed.

src/lib/dhcpsrv/cfgmgr.h
I fixed a few trivial issues and pushed changes to the branch.

Thanks.

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
CfgMgrTest::echoClientId: Just a thought. The test could also check that the flag can be set to TRUE after setting it to FALSE. However, I admit that it is not very likely to fail :-)

Implemented anyway. Somewhat not surprisingly, the test is passing.


We need a ChangeLog for this because it has a user impact.

Added proposed ChangeLog?.

comment:6 in reply to: ↑ 5 Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

Replying to tomek:

Replying to marcin:

Reviewed commit: 4a08e3662e7720ac6c96b7d585084bbe4033c29b

Reviewed commit 9d3cf130a0c92c715b8ff1b7ce8f1743ab9c2bc7

doc/guide/bind10-guide.xml
Apparently, the RFC6842 says that the client id must be echoed back to the client IF client id has been included in the client's message. I suggest that the bind10-guide is updated accordingly. Otherwise, it gives an impression that client-id must be always included.

Clarified.

Similar to the comment to changelog (below)... It is worth to clarify what kind of ''flag'' has been introduced to control whether the client id is echoed back or not. It would be clearer to call it ''configuration parameter''.


src/bin/dhcp4/config_parser.cc
commitGlobalOptions: why is this function called like this? It merely stores the new ''echo-client-id'' option in the CfgMgr.

Because it is the first global parameter. The first, but definitely not the last one. For example, dibbler-server has at least 24 global scoped parameters (work-dir, log parameters, auth info, cache sizes, performance-mode and tons of other stuff). Added a comment that clarifies the name.

ok

Question: do you think it makes sense to have a per-subnet configuration of this new option?

No. Let's try to follow the latest spec. If users complain about that, there's the (global) switch. If users complain about that, then we'll ask for detailed explanation why that is not sufficient. If they convince us, then we will invest time in fancy migration procedures with per-subnet information.

ok. I didn't mean to complicate things if we feel it is unnecessary.

src/bin/dhcp4/tests/config_parser_unittest.cc
The echoClientId test is insufficient because it doesn't check the value of ''echo-client-id'' flag before applying the configuration.

The test has been extended. It checks the default (before any configuration tricks), then "echo-client-id false" configuration and finally a reverse back to the default.

Ok. Except that you could do assert instead of expect, because if the first check is wrong, the further part of the test doesn't make sense. This applies to the other expect too.

src/bin/dhcp4/dhcp4_test_utils.h
checkClientId: After recent changes to this function you should extend the description of this function to say that the function checks presence of the client-id if specified so in the CfgMgr and checks the absence in other case. Otherwise it looks like you always expect that client-id is present.

Done.

ok.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
Dhcpv4SrvTest::discoverEchoClientId: The test description is a bit ambiguous. It may be unclear what it means that the ''echoing back client-id'' is controllable.

Clarified.

ok.

The checks in line 705 and 713 look like duplicates of what the ''checkClientId'' function is already doing.

Removed.

ok


The same comments apply to Dhcpv4SrvTest::requestEchoClientId

Removed.

ok


src/lib/dhcpsrv/cfgmgr.h
I fixed a few trivial issues and pushed changes to the branch.

Thanks.

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
CfgMgrTest::echoClientId: Just a thought. The test could also check that the flag can be set to TRUE after setting it to FALSE. However, I admit that it is not very likely to fail :-)

Implemented anyway. Somewhat not surprisingly, the test is passing.


ok. Except, that you could have used assert for the first check.

We need a ChangeLog for this because it has a user impact.

Added proposed ChangeLog?.

I think it may be worth to clarify what ''flag'' has been added, i.e. it is a ''configuration parameter'' or something like this.

I am fine with the changes. If you concur to replace expects to asserts were indicated, please do so. Also, please clarify what the ''flag'' is as it sounds a bit ambiguous. I don't need to see this ticket again.

comment:7 Changed 6 years ago by tomek

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

Thanks for the review. Merged, pushed, closed.

Note: See TracTickets for help on using tickets.