Opened 9 years ago

Closed 9 years ago

#910 closed enhancement (complete)

TSIG + TC bit support

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

Description

There's one TODO item in the TSIG signing task: adding TSIG with
truncated DNS message. To do this we need to have the ability of
knowing the length of expected TSIG RR (which also requires the
ability of knowning the expected signature size), and if the message
has to be truncated we need special processing of removing all
inserted RRs and re-construct a message only containing the Question
section and TSIG.

Subtickets

Change History (12)

comment:1 Changed 9 years ago by shane

  • Milestone changed from New Tasks to Year 3 Task Backlog

comment:2 Changed 9 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:3 Changed 9 years ago by jinmei

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

comment:4 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 4

comment:5 Changed 9 years ago by stephen

  • Milestone changed from Next-Sprint-Proposed to Sprint-20110712

comment:6 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jinmei
  • Status changed from assigned to accepted

comment:7 Changed 9 years ago by jinmei

trac910 is ready for review.

This branch consists of several not-directly-related sets of changes,
which unfortunately makes the total diff a bit big.

  • the first two commits extend TSIGContext class so that the Message class can detect the expected length of TSIG RR.
  • most of the remaining changes are for the Message::toWire() so that the truncation case will be handled correctly. Python test cases are straightforward mapping from the corresponding C++ tests, so hopefully this fact reduces review load.
  • commit 146c48 is an exception. It's not directly related to TSIG+TC, but is necessary to handle a corner case scenario. I considered an option of differing this change and the corner case handling, but that would open up a possibility of making the toWire() logic cause an unexpected exception, which would be a remotely exploitable DoS to the auth server. So I decided it to include the entire set.

The proposed change entry is as follows:

269.?	[func]		jinmei
	libdns++/pydnspp: TSIG signing now handles truncated DNS messages
	(i.e. with TC bit on) with TSIG correctly.
	(Trac #910, git TBD)

comment:8 Changed 9 years ago by jinmei

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

comment:9 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to jinmei

Reviewed commit dfd8332b1a958ed9aeb6ae423ea937b5e08024f8

Only two points:

src/lib/dns/tests/testdata/gen-wiredata.py.in
I assume this is OK, but as I've mentioned before there is little documentation. As we are about to start on a programme of increasing the support for RR types in BIND 10, can I suggest that we create a ticket to properly document this utility?

src/lib/dns/tsig.cc
getHMAC(): The description is "create an HMAC object", whereas the method name is getHAMC() (implying it returns the class attribute, regardless of what it is). Also, the getHMAC() has the side-effect of deleting the stored HMAC if one exists. Suggest renaming the method to something like createHMAC() or constructHMAC(), and making clear that the hmac_ created in the constructor is not expected to be retained.

ChangeLog entry is OK

I don't need to see this again - please merge after you updated the getHMAC name/documentation.

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

Replying to stephen:

src/lib/dns/tests/testdata/gen-wiredata.py.in
I assume this is OK, but as I've mentioned before there is little documentation. As we are about to start on a programme of increasing the support for RR types in BIND 10, can I suggest that we create a ticket to properly document this utility?

Ah, nice reminder. And there's already a ticket for that, #904:-)
I've moved it to next-sprint-proposed.

src/lib/dns/tsig.cc
getHMAC(): The description is "create an HMAC object", whereas the method name is getHAMC() (implying it returns the class attribute, regardless of what it is). Also, the getHMAC() has the side-effect of deleting the stored HMAC if one exists. Suggest renaming the method to something like createHMAC() or constructHMAC(), and making clear that the hmac_ created in the constructor is not expected to be retained.

Actually createHMAC() is not really 100% accurate in that it may
simply return a stored object. But it's not a public method, so the
naming wouldn't matter much anyway. I've simply adopted the name
"createHMAC()" with some additional comments (not really sure if the
comments are something you wanted. Please check).

I've merged the code to master. Will close ticket.

Thanks for the prompt review.

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.