Opened 7 years ago

Closed 7 years ago

#2326 closed task (fixed)

Allocation engine v6: release

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20130103
Component: dhcp6 Version:
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

This ticket covers v6 address release.

Subtickets

Change History (9)

comment:1 Changed 7 years ago by stephen

  • Milestone changed from Sprint-DHCP-20121115 to DHCP 2012

comment:2 Changed 7 years ago by stephen

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20121213

comment:3 Changed 7 years ago by tomek

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

comment:4 Changed 7 years ago by tomek

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

Code has been implemented. Please review.

comment:5 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:6 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 6ce0831748423ab48611785f29ee999beeaf8322

src/bin/dhcp6/dhcp6_messages.mes
I've made some changes here and pushed the result. However:

DHCP6_DB_ERROR_LEASE6_WITHOUT_DUID: if the workaround is to remove the lease entry from the database, why does the software not do that regardless?

Also:

  • As this error is most likely not a problem with the database (but with the code using the database), I suggest taking out the "DB" from the symbolic name.
  • I also suggest removing "ERROR" from the name as well (no other messages indicate the severity in the message name).
  • I suggest removing "6" from "LEASE6". (It must be a Lease6 since the origin of the message is DHCP6: also, DHCP6_LEASE_ALLOC_FAIL uses "LEASE" and not "LEASE6".)

In summary, the name "DHCP6_LEASE_WITHOUT_DUID" is suggested.

src/bin/dhcp6/dhcp6_srv.cc
Line 715: "This should not happen. We have checked this before." This seems to indicate a software failure and I would suggest that a message be logged at this point (with at least WARNING severity).

Line 735: the "default" clause does not need a "break" - it could just drop through. (Leave the "default" clause there though with a comment to note that the remaining cases have been considered.)

Line 780: suggest that this be a DEBUG message - a malicious client could trigger a flood of warning messages by sending release messages for random addresses. (It is assumed that by default, WARNING messages are enabled and DEBUG messages disabled.)

Line 828: correction - s/necessary if/necessary to check if/

Line 833: looking at the code, a failure here is not a database error (as indicated by the message for DHCP6_RELEASE_FAIL). A database error would trigger an exception: deleteLease() returning false indicates that there was no lease with the given address in the database. As getLease6 has established that the address is present, the implication is that something else has deleted the lease in the meantime.

src/bin/dhcp6/dhcp6_srv.h
Header for releaseIA_NA does not quite parse. How about:

As RFC 3315 requires that a single status code be sent for the whole message,
this method may update the passed general_status: it is set to SUCCESS when
message processing begins, but may be updated to some error code if the
release process fails.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
!Dhcpv6SrvTest(s): instead of using a scope_ptr, why not declare !NamedDhcpv6Srv as an automatic variable?

Line 1245: "check that the packets originating from local addresses can be" what?

ReleaseReject: the last statement before case 1 adds a server ID to the renew packet with the comment that it is mandatory in a RENEW. Is there a test for the case where it is absent?

ChangeLog
Proposed entry is OK.

comment:7 in reply to: ↑ 6 Changed 7 years ago by tomek

  • Owner changed from tomek to stephen

Replying to stephen:

Reviewed commit 6ce0831748423ab48611785f29ee999beeaf8322

src/bin/dhcp6/dhcp6_messages.mes
I've made some changes here and pushed the result. However:

Thank you.

DHCP6_DB_ERROR_LEASE6_WITHOUT_DUID: if the workaround is to remove the lease entry from the database, why does the software not do that regardless?

Because the reason for this is unknown. Just wiping it out would potentially mask a problem. I think there could be 2 likely cases that could result to that condition:

  1. Someone modified the record to hold empty DUID (BTW do we have tests that try to read record with duid field set to NULL?)
  2. a backend has a bug that returns null DUIDs.

If we had removed the entry and the reason was 1, we would be essentially saying "that's ok that you're messing with the database, we can recover from that, carry on as you please". This is potentially dangerous situation. Logging a warning message is warranted here in my opinion. And I hope is it annoying to the user.

In summary, the name "DHCP6_LEASE_WITHOUT_DUID" is suggested.

Agree. Changed.


src/bin/dhcp6/dhcp6_srv.cc
Line 715: "This should not happen. We have checked this before." This seems to indicate a software failure and I would suggest that a message be logged at this point (with at least WARNING severity).

Added.

Line 735: the "default" clause does not need a "break" - it could just drop through. (Leave the "default" clause there though with a comment to note that the remaining cases have been considered.)

But it needs something after the label. Added semicolon.

Line 780: suggest that this be a DEBUG message - a malicious client could trigger a flood of warning messages by sending release messages for random addresses. (It is assumed that by default, WARNING messages are enabled and DEBUG messages disabled.)

I disagree. This is a (serious) violation of RFC and we should log it. Nobody would notice a log on DEBUG level. I've lowered message severity to INFO. Does it look acceptable?

Line 828: correction - s/necessary if/necessary to check if/

Done.

Line 833: looking at the code, a failure here is not a database error (as indicated by the message for DHCP6_RELEASE_FAIL). A database error would trigger an exception: deleteLease() returning false indicates that there was no lease with the given address in the database. As getLease6 has established that the address is present, the implication is that something else has deleted the lease in the meantime.

Ok. Updated explanation of the message. So what would the mysql backend do, if it has access to the database in read-only mode? Return false or throw?

src/bin/dhcp6/dhcp6_srv.h
Header for releaseIA_NA does not quite parse. How about:

As RFC 3315 requires that a single status code be sent for the whole message,
this method may update the passed general_status: it is set to SUCCESS when
message processing begins, but may be updated to some error code if the
release process fails.

Updated.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
!Dhcpv6SrvTest(s): instead of using a scope_ptr, why not declare !NamedDhcpv6Srv as an automatic variable?

I don't like it much, because I can't control creation/destruction moment with automatic variable. If I try:

ASSERT_NO_THROW(Dhcpv6Srv srv(0));

The srv variable is valid only within ASSERT_NO_THROW scope and is destroyed immediately afterwards. But ASSERT_NO_THROW() macro does not have much sense anyway. Any unexpected throws will abort the test anyway.

Line 1245: "check that the packets originating from local addresses can be" what?

Comment rewritten.

ReleaseReject: the last statement before case 1 adds a server ID to the renew packet with the comment that it is mandatory in a RENEW. Is there a test for the case where it is absent?

It is tested indirectly. First call in processRelease() (or any other processX method) is to call sanityCheck(). This method is checked. But I agree that dedicated tests could be developed for that. Added @todo in the code as this is a broad topic, not just a single test. If you multiply absent/present/than one instance by forbidden/optional/mandatory by number of messages, you'll get quite a number of cases to cover. And such negative cases could be more subtle than that, e.g. renew with empty IA, renew without IA at all.

Changes pushed. Please confirm that all is ok (or point out new issues).

comment:8 Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 72beaf8964363734d4f5e7b4b11ae7b8b4b5557c.

I've pushed another change to dhcp6_messages.mes.

Some comments:

Because the reason for this is unknown. Just wiping it out would potentially mask a problem.

Fair enough

Line 780: suggest that this be a DEBUG message - a malicious client could trigger a flood of warning messages by sending release messages for random addresses. (It is assumed that by default, WARNING messages are enabled and DEBUG messages disabled.)

I disagree. This is a (serious) violation of RFC and we should log it. Nobody would notice a log on DEBUG level. I've lowered message severity to INFO. Does it look acceptable?

Given that we log so few conditions, it's probably not a major issue. The general concern for all BIND 10 is that any message that can be generated by a packet received from a user can be used as a DoS vector. Although messages can be disabled, disabling all warning messages could mask other warning conditions related to the server. Logging such messages with DEBUG severity is one solution:the other is to use a different logger (see src/lib/log/README - and before you ask, ticket #2566 has been created to convert this to Doxygen format for inclusion into the developer's guide).

I've posted a message to bind10-dev to see which of the two solutions (if either) has been used for the DNS. I suggest reverting it back to a warning (so that it is easier to find again) and we'll revisit this once there has been some discussion of the post.

So what would the mysql backend do, if it has access to the database in read-only mode?

Read-only access to a database is set up by only restricting the user under which the connection is made to SELECT privilege only. As a result, a delete operation will fail, even if the WHERE clause is such that no rows are selected for deletion.

All OK, please merge after changing the message back to a warning.

comment:9 Changed 7 years ago by tomek

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

Log level changed to INFO. The code is merged to master.

I will post a question about logging levels to bind10-dev.

Note: See TracTickets for help on using tickets.