Opened 6 years ago

Closed 5 years ago

#2512 closed enhancement (fixed)

RR type implementation: CAA

Reported by: shane Owned by: muks
Priority: high Milestone: bind10-1.2-release-freeze
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by shane)

Implement the CAA type. RFC 6844:

http://www.rfc-editor.org/rfc/rfc6844.txt

Subtickets

Change History (22)

comment:1 Changed 6 years ago by shane

  • Milestone New Tasks deleted

comment:2 Changed 6 years ago by shane

  • Description modified (diff)

comment:3 Changed 5 years ago by muks

  • Milestone set to Sprint-20131015
  • Status changed from new to reviewing

Up for review.

Empty value field is allowed as the RFC is not clear on this matter. So this is implemented on the permissive side.

The maximum length of a value field is 255 characters. This is not clearly specified in the RFC. Whereas the implicit size (based on RDLEN) would allow larger value fields, note that the presentation format advises a <character-string> encoding whose maximum size is 255 characters.

comment:4 Changed 5 years ago by shane

Ticket #3298 depends on this being complete.

comment:5 Changed 5 years ago by muks

ChangeLog entry:

XYZ.    [func]          muks
        Add support for the CAA RR type (RFC 6844).
        (Trac #2512, git ...)

comment:6 Changed 5 years ago by shane

  • Priority changed from medium to high

comment:7 follow-up: Changed 5 years ago by pselkirk

  • Owner changed from UnAssigned to muks
  • The CAA class provides getFlags and getTag, but not getValue. If there's a reason for this, it should be documented.
  • In CAAImpl, you say
        // The first byte of this vector contains the length of the rest of
        // the vector. This byte is actually unused and is skipped when
        // reading the vector.
    

Maybe what you mean is that detail::charStringToString() ignores the length byte (in favor of its own internal length field). But it still seems a strange thing to say here, when we do in fact store and render the length byte.

  • RFC 6844 describes the presentation format of the value field as
       Value:  Is the <character-string> encoding of the value field as
          specified in [RFC1035], Section 5.1.
    

And RFC 1035 describes escaped characters, multi-line data, and comments in the character-string format. So we should probably have test cases with escaped characters, multi-line data, and comments.

  • In CAA::constructFromLexer, you have this:
        if ((token.getType() != MasterToken::END_OF_FILE) &&
            (token.getType() != MasterToken::END_OF_LINE))
        {
            detail::stringToCharString(token.getStringRegion(), value);
        } else {
            // Convert it into a CharString.
            value.push_back(0);
        }
    

I think the 'else' comment belongs with the 'if' clause, in which case it would be documenting the obvious. It's not really clear what you meant here.

  • In the constructor from parameters, you do your own conversion of the value parameter from string to vector, but you don't do any conversion of escaped characters. Why not use stringToCharString here?
  • CAA::toWire() has a test for impl_->tag_.empty(), but the constructors all throw if tag is empty, so this seems redundant.
  • Rdata_CAA_Test constructor is over-indented.
  • There's a "missing value" test in Rdata_CAA_Test.fields that's redundant to Rdata_CAA_Test.createFromText. It's probably okay, but you may want to document it as such, as you do in Rdata_CAA_Test.badText.
  • Rdata_CAA_Test.compare should perhaps have an equality test, in addition to greater-than and less-than.

comment:8 Changed 5 years ago by muks

Before I start addressing the comments, let me first say that I like the quality of this review. +1. Keep it up.

comment:9 in reply to: ↑ 7 Changed 5 years ago by muks

Paul, I've addressed some of your comments on the branch but for the others I am trying to check again if I have understood the RFC correctly. The RFC has some conflicting comments on the "Value" field.

I'll answer back on this ticket soon.

comment:10 Changed 5 years ago by muks

  • Owner changed from muks to UnAssigned

Replying to pselkirk:

  • The CAA class provides getFlags and getTag, but
  • not getValue. If there's a reason for this, it should be
  • documented.

getValue() and a unittest for it have been added.

  • In CAAImpl, you say
        // The first byte of this vector contains the length of the rest of
        // the vector. This byte is actually unused and is skipped when
        // reading the vector.
    

Maybe what you mean is that detail::charStringToString() ignores the length byte (in favor of its own internal length field). But it still seems a strange thing to say here, when we do in fact store and render the length byte.

  • RFC 6844 describes the presentation format of the value field as
       Value:  Is the <character-string> encoding of the value field as
          specified in [RFC1035], Section 5.1.
    

And RFC 1035 describes escaped characters, multi-line data, and comments in the character-string format. So we should probably have test cases with escaped characters, multi-line data, and comments.

  • In CAA::constructFromLexer, you have this:
        if ((token.getType() != MasterToken::END_OF_FILE) &&
            (token.getType() != MasterToken::END_OF_LINE))
        {
            detail::stringToCharString(token.getStringRegion(), value);
        } else {
            // Convert it into a CharString.
            value.push_back(0);
        }
    

I think the 'else' comment belongs with the 'if' clause, in which case it would be documenting the obvious. It's not really clear what you meant here.

The code which the above describes was correctly written (with an unused
length byte in the vector to simulate a CharString), but it seems this
was confusing to a reader.

So I've now introduced a CharStringData type that does away with the
length byte prefix and the 255-byte limit (from <character-string> as
it does not apply to RR types like CAA). With this, the code should be
easier to follow.

Tests have been added for escaped characters, etc.

  • In the constructor from parameters, you do your own conversion of
  • the value parameter from string to vector, but you don't do
  • any conversion of escaped characters. Why not use
  • stringToCharString here?

This has been updated, and tests have been added for the
constructor-from-params that use escaping, etc.

  • CAA::toWire() has a test for impl_->tag_.empty(), but
  • the constructors all throw if tag is empty, so this seems
  • redundant.

This has been changed to an assertion now.

  • Rdata_CAA_Test constructor is over-indented.

This has been fixed.

  • There's a "missing value" test in Rdata_CAA_Test.fields that's
  • redundant to Rdata_CAA_Test.createFromText. It's probably
  • okay, but you may want to document it as such, as you do in
  • Rdata_CAA_Test.badText.

Such a comment has been added.

  • Rdata_CAA_Test.compare should perhaps have an equality test,
  • in addition to greater-than and less-than.

There is one in the branch in the .createFromWire test, but I've
copied it into the .compare test now with a comment.

Putting back to review.

comment:11 Changed 5 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:12 follow-up: Changed 5 years ago by stephen

  • Owner changed from stephen to muks

Reviewed commit a6a6cb2e33f807593e6d457bfc822a9c31ef211e.

I've looked at the differences from commit 11052a822a6c329a087e5590ac525ab3107926a8 which (going by the date) appears to be the commit that Paul reviewed and assessed whether they address Paul's comments.

I've also made some observations that did not appear to be picked up by the earlier review.

src/lib/dns/rdata/generic/caa_257.{cc,h}
All the specialised methods - including the trivial getFlags() etc - should have Doxygen headers. (These are usually in the .h file, but it's not worth changing it now.)

The documentation for getValue() should note that the reference returned is only valid for as long as the relevant CAA object is in existence.

Suggest declaring "CAA::impl_" as a "boost::scoped_ptr". Then there is no need to have an explicit destructor for the CAA class. Also, in the CAA constructor, there will be no need for the temporary "impl_ptr" variable - if the constructor fails, the impl_ destructor will automatically free any resources.

CAA::CAA(InputBuffer&, size_t): tag_vec is created then immediately resized to tag_length. Suggest that the vector be created with the correct size (by passing tag_length as the constructor argument).

src/lib/dns/rdata/generic/detail/char_string.{cc,h}
stringToCharStringData: I would prefix the assertion with a comment. The fact that there are three digits following an escape is checked in decimalToNumber(), so the only reason an assert() is needed is to catch the case where the check is removed from that function. (Also, it took a few seconds to work out why the assert was for n >=3 when the following line subtracts 2 from n.)

src/lib/dns/tests/rdata_char_string_data_unittest.cc
charStringDataToString test: rather than use an assert(), why not an ASSERT_GE? That way if the assertion does fail, the test fails in a graceful way rather than the the test program terminate.

comment:13 follow-up: Changed 5 years ago by stephen

Suggest declaring "CAA::impl_" as a "boost::scoped_ptr". Then there is no need to have an explicit destructor for the CAA class. Also, in the CAA constructor, there will be no need for the temporary "impl_ptr" variable - if the constructor fails, the impl_ destructor will automatically free any resources.

Ignore that suggestion - you can't declare a scoped_ptr with an incomplete type as the template argument.

comment:14 in reply to: ↑ 13 Changed 5 years ago by muks

Replying to stephen:

Suggest declaring "CAA::impl_" as a "boost::scoped_ptr". Then there is no need to have an explicit destructor for the CAA class. Also, in the CAA constructor, there will be no need for the temporary "impl_ptr" variable - if the constructor fails, the impl_ destructor will automatically free any resources.

Ignore that suggestion - you can't declare a scoped_ptr with an incomplete type as the template argument.

Nod :) #3298 was created to do this cleanup, but we realized it cannot be done as the Impl types are opaque there.

comment:15 in reply to: ↑ 12 Changed 5 years ago by muks

  • Owner changed from muks to stephen

Hi Stephen

Replying to stephen:

src/lib/dns/rdata/generic/caa_257.{cc,h}
All the specialised methods - including the trivial getFlags() etc -
should have Doxygen headers. (These are usually in the .h file, but
it's not worth changing it now.)

These are added now.

The documentation for getValue() should note that the reference
returned is only valid for as long as the relevant CAA object is in
existence.

Done.

CAA::CAA(InputBuffer&, size_t): tag_vec is created then immediately
resized to tag_length. Suggest that the vector be created with the
correct size (by passing tag_length as the constructor argument).

Done.

src/lib/dns/rdata/generic/detail/char_string.{cc,h}
stringToCharStringData: I would prefix the assertion with a
comment. The fact that there are three digits following an escape is
checked in decimalToNumber(), so the only reason an assert() is needed
is to catch the case where the check is removed from that
function. (Also, it took a few seconds to work out why the assert was
for n >=3 when the following line subtracts 2 from n.)

Comments were added for these cases.

src/lib/dns/tests/rdata_char_string_data_unittest.cc
charStringDataToString test: rather than use an assert(), why not an
ASSERT_GE? That way if the assertion does fail, the test fails in a
graceful way rather than the the test program terminate.

Done.

comment:16 Changed 5 years ago by stephen

  • Owner changed from stephen to muks

Reviewed commit 9b9a1771a28d416d7c5e954ef41799b6d9c02959

All points addressed, please merge.

comment:17 Changed 5 years ago by muks

  • Owner changed from muks to stephen

Hi Stephen

Please can you review commit 0c6ca2779a9152db8f4d43a88faf382380769961 on the branch? I had to merge from master and add this extra commit as master had moved on since the branch point and has removed API.

comment:18 Changed 5 years ago by stephen

  • Owner changed from stephen to muks

Please can you review commit 0c6ca2779a9152db8f4d43a88faf382380769961 on the branch? I had to merge from master and add this extra commit as master had moved on since the branch point and has removed API.

Reviewed. All OK, please merge.

comment:19 follow-up: Changed 5 years ago by muks

  • Owner changed from muks to stephen

Hi Stephen

Please can you review commit c1c4ea8d1b7629dac379f192d30cc79535214236 on the branch? I had to merge from master and add this extra commit as master had moved on since the branch point and has removed API.

comment:20 in reply to: ↑ 19 Changed 5 years ago by muks

Replying to muks:

Hi Stephen

Please can you review commit c1c4ea8d1b7629dac379f192d30cc79535214236 on the branch? I had to merge from master and add this extra commit as master had moved on since the branch point and has removed API.

Sorry, please ignore this. It is meant for a different ticket!

comment:21 Changed 5 years ago by muks

  • Owner changed from stephen to muks

comment:22 Changed 5 years ago by muks

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

Merged to master branch in commit 2d33f5b862022d599017430083fe814678258643:

* 0c6ca27 [2512] Stop using API that was removed
* 9b9a177 [2512] Add API doc comments for generic::CAA specialized methods
* 8d831e9 [2512] Add some comments
* 5b61be1 [2512] Update assert() to a gtest ASSERT
* 8c77a05 [2512] Combine resize() call with the constructor
* a6a6cb2 [2512] Use CharStringData when constructing from parameters too
* 3c09cb5 [2512] Add some <character-string> tests for Value field
* a5d2806 [2512] Add CAA::getValue()
* 7606f07 [2512] Simplify CAA implementation using CharStringData type
* cb1407f [2512] Add a CharStringData type (with no length byte prefix)
* 79defea [2512] Update check to an assertion
* 11052a8 [2512] Fix indent level
* 969a916 [2512] Add a comment about redundant test
* e48ae46 [2512] Add equality test in .compare unittest
* 2e80ea0 [2512] Add support for the CAA RR type

Resolving as fixed. Thank you for the reviews Stephen.

Note: See TracTickets for help on using tickets.