Opened 9 years ago

Closed 9 years ago

#871 closed enhancement (complete)

TSIG signing, part 2

Reported by: jinmei Owned by: jinmei
Priority: high Milestone: Sprint-20110517
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 2.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

See #812. Conceptually this ticket belongs to #812, but it would be
easier to have a separate ticket for review process. So I created a
new one (and intentionally left estimation 0).

Subtickets

Change History (12)

comment:1 Changed 9 years ago by jinmei

Branch trac871 is ready for review.

This is a continuation of trac812, but (assuming the basic design has
been agreed) could be reviewed separetely.

Notes for the reviewer:

  • the fisrt commit is a merge of dependencies and can/should be ignored.
  • the second commit can also be very trivial. it just moves some portion of trac812 code to new files. So, essentially, this can be the start point for review.
  • commit 29170e2 is irrelevant cleanup for the main subject of this ticket, but I proposed this to be done in this (sub)task
  • commit 28143be is also mostly irrelevant (so the review could be separated before this commit ant this one). And, although the diff for commit is a bit large, it should be quite straightforward.

comment:2 Changed 9 years ago by jinmei

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

comment:3 Changed 9 years ago by jinmei

Oops, I forgot to add one important note.

This version doesn't implement the truncation case described in Section
3.1 of RFC2845. This case should be very rare in our initial targets
(xfrs use TCP from the beginning), so I propose deferring it to a
separate (later) ticket.

comment:4 Changed 9 years ago by jinmei

  • Estimated Difficulty changed from 0.0 to 2.0

comment:5 Changed 9 years ago by jinmei

another note to reviewer:
Now that trac812 has been merged to master, I've merged the latest
master (containing trac812) back to this branch. The merge commit is
364be72653933235d362a57c9342bae1c6e17b3a
(The latest commit as of this writing).

For review purposes this commit should better be ignored. That is,
important part of the diff can be retrieved by:
git diff cc788af 28143be

comment:6 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to jinmei

I've got a bit confused with the various branches and what commits to review/not reviews (and the git inheritance tree is also complicated). To summarise: at fd45c40 (the commit that was the subject of review on ticket #812), trac812 was branched:

  • one child was merged into master (starting at commit cc788af) has a set of commits with log messages starting [trac812next]. The final commit in this branch was 28143be.
  • the other branch (with commits labelled ![trac812]) addresses changes made in the review of #812. This was subsequently merged into master.

The two branches (trac812next and master) were merged at commit 364be72; at this point, branch trac871 starts.

Accordingly, the review is divided into two parts:

  • Review of [trac812next] changes - differences between cc788af and 28143be.
  • Review of ![trac871] - differences between 364be72 and the latest version (31a6f34).

trac812next

src/lib/dns/message.{cc, h}
Although it's only been moved within the .cc file, some documentation about the RenderSection struct would be useful (the two terse one-line comments don't really give a lot of information).

Comment: a matter of taste, but as RenderSection is now only used within MessageImpl::toWire() (and MessageImpl is defined within the .cc file), it could be declared as part of MessageImpl instead of being inside an anonymous namespace.

Suggest that the "TBD" comments in MessageImpl::toWire() be renamed "TODO" in common with most "to do" comments.

It took me a moment to realise why the assert(arcount != 0) is there and what condition could trigger it. The condition is very unlikely I agree, but congratulations on foreseeing the possibility!

src/lib/dns/tests/testdata/gen-wiredata.py.in
There appears to be very little documentation on this program or on the format of its input files - yet a lot of tests appear to be based on it. I would suggest that a task be added to write such documentation. Having said that, the changes appear to be OK.

trac871

src/lib/dns/tests/tsigrecord_unittest.cc
Comment: Given that the "getLength" test has verified that TSIGRecord::getLength() works and gives the correct size, in the "recordTooLongToWire" test instead of setting the render's length limit to a hard-coded 84, it could be set to "test_record.getLength() - 1" instead. (Don't bother changing it unless you anticipate future changes - it was only made because it was noticed that a change to the TSIG record length resulted in two changes in the file.)

comment:8 in reply to: ↑ 7 Changed 9 years ago by jinmei

Replying to stephen:

I've got a bit confused with the various branches and what commits to review/not reviews (and the git inheritance tree is also complicated). To summarise: at fd45c40 (the commit that was the subject of review on ticket #812), trac812 was branched:

Thanks for the review.

Sorry for the confusion and inconvenience. I should have created a
separate ticket for the second subtask from the beginning (I took that
approach for the verify tasks).

trac812next

src/lib/dns/message.{cc, h}

Although it's only been moved within the .cc file, some documentation about the RenderSection struct would be useful (the two terse one-line comments don't really give a lot of information).

Okay, I've added some more generic comments (not specific to the
subject of this ticket, but the message class still generally misses
documentation and tests, it would be a good idea to improve it
gradually).

Comment: a matter of taste, but as RenderSection is now only used within MessageImpl::toWire() (and MessageImpl is defined within the .cc file), it could be declared as part of MessageImpl instead of being inside an anonymous namespace.

I'd rather keep hiding it in the unnamed namespace. First, since
the existence of MessageImpl is still publicly visible (even though
the class definition is hidden), it's less safer in terms of
encapsulating a totally private thing. Second, RenderSection doesn't
need any internal information about MessageImpl (or !Message) and
doesn't have to be a part of it. I basically prefer keeping a class
as concise as possible even for a private pimpl class.

Suggest that the "TBD" comments in MessageImpl::toWire() be renamed "TODO" in common with most "to do" comments.

Okay, done.

It took me a moment to realise why the assert(arcount != 0) is there and what condition could trigger it. The condition is very unlikely I agree, but congratulations on foreseeing the possibility!

Hmm, on thinking about it more, I've removed the assert. I gave more
detailed reason in the commit log (commit 28c50de).

src/lib/dns/tests/testdata/gen-wiredata.py.in
There appears to be very little documentation on this program or on the format of its input files - yet a lot of tests appear to be based on it. I would suggest that a task be added to write such documentation. Having said that, the changes appear to be OK.

Yeah I know. This is part of long standing hidden agenda. I've been
trying to improve it gradually, but it makes more sense to create a
dedicated task. Will do.

trac871

src/lib/dns/tests/tsigrecord_unittest.cc
Comment: Given that the "getLength" test has verified that TSIGRecord::getLength() works and gives the correct size, in the "recordTooLongToWire" test instead of setting the render's length limit to a hard-coded 84, it could be set to "test_record.getLength() - 1" instead. (Don't bother changing it unless you anticipate future changes - it was only made because it was noticed that a change to the TSIG record length resulted in two changes in the file.)

Applied the suggestion. Actually, in some cases I intentionally use
hardcoded magic numbers in tests so that we can detect it when the
testee is accidentally modified in an unintended way. But in this
case such consideration doesn't seem to be crucial, so I modified it
to centralize the hardcode constant.

comment:9 Changed 9 years ago by jinmei

  • Owner changed from jinmei to stephen

comment:10 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to jinmei

All OK - please merge.

comment:11 in reply to: ↑ 10 Changed 9 years ago by jinmei

Replying to stephen:

All OK - please merge.

Thanks, merged. Closing ticket.

comment:12 Changed 9 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.