Opened 9 years ago

Closed 8 years ago

#812 closed enhancement (complete)

TSIG: Signing messages

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

Description (last modified by stephen)

Note that TCP support also works for UDP.

  • Don't TSIG sign notify. :)
  • Don't make TSIG key generator (for now).
  • Support HMAC MD5. Don't support SHA*, GSS-TSIG for now.

Subtickets

Change History (14)

comment:1 Changed 9 years ago by stephen

  • Description modified (diff)

comment:2 Changed 9 years ago by shane

  • Estimated Difficulty changed from 0.0 to 8
  • Milestone changed from Year 3 Task Backlog to Sprint-20110419

comment:3 Changed 9 years ago by jinmei

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

comment:4 Changed 8 years ago by shane

  • Defect Severity set to N/A
  • Feature Depending on Ticket set to tsig
  • Priority changed from major to critical
  • Sub-Project set to DNS

comment:5 Changed 8 years ago by shane

  • Priority changed from critical to blocker

comment:6 Changed 8 years ago by jinmei

Branch trac812 is ready for review.

This branch still misses some final parts of TSIG signing (which I'll
soon complete), but the branch is already quite big, so I decided to
get this part reviewed first.

The entire diff is actually VERY big, so I guess some guidance and
notes are necessary.

  • first two commits can (or even should) be ignored. it simply incorporates a dependent branch (781) and makes minimal change to make the tree buildable.
  • next three commits (adfd101..ad13479) are not directly related to TSIG signing, but make some adjustments to the existing TSIGKey class for the use of crypto API with TSIG. These are a separate chunk of changes.
  • changes to Makefiles should be trivial: they are addition of new files and addition of path to libcryptolink for tests/apps that implicitly rely on it.
  • other changes are new files: lib/dns/{tsig,tsigerror}.{cc,h} and their tests, and lib/util/unittests/newhook.{cc,h}.
    • tsigerror (ant its tests) should be mostly trivial. so, if the entire diff is deemed to be too big, one possible breakdown is to complete the tsigkey changes and tsigerror first, and discuss the rest in the next step.
    • tsig.{h,cc} and its tests are the main subject of this ticket. the API design is detailed in tsig.h. if the API design is okay, I believe the code itself is quite straightforward. test cases are complicated, but I'm quite confident they worked correctly because I took the test data from actual packet generated by BIND 9 and most of the tests check the integrity via HMAC.
    • newhook is a utility for a test in tsig_unittest. See the description in newhook.h. This may be too tricky. If so, I'm okay with removing it and corresponding test.

A couple of other notes:

  • TSIG is time-sensitive protocol, so I used the same trick to generate "fake current time" as that was used in lib/util/time_utilities. It would be better to (internally) publish the wrapper trick and unify the implementation, but I didn't do this in this branch because the branch was already quite big. I plan to do it in the next phase.
  • I'm not intending to create a changelog entry for this specific task. It would make more sense to have a single entry when we provide a complete set of sign/verify.

comment:7 Changed 8 years ago by jinmei

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

comment:8 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to jinmei

first two commits can (or even should) be ignored. it simply incorporates a dependent branch (781) and makes minimal change to make the tree buildable.

Ignored :-)

next three commits (adfd101..ad13479) are not directly related to TSIG signing, but make some adjustments to the existing TSIGKey class for the use of crypto API with TSIG. These are a separate chunk of changes.

src/lib/dns/tsigkey.{cc,h}
The method that returns the hash algorithm name is called getAlgorithmName(), but the (new) method that returns the hash algorithm itself is called getCryptoAlgorithm(). Wouldn't getAlgorithm() be more consistent?

(Review of differences between ad13479 and fd45c40)

src/lib/dns/tsig.{cc,h}
In TSIGContext::sign(), suggest adding a comment to the effect that TSIG uses a 48-bit time before the statement masking gettimeofdayWrapper with a constant. (It's not clear why the value is masked unless you really study RFC 2845.)

In TSIGContext::sign(), the 48-bit time is written using writeUint16(). followed by writeUint32(). For completeness (and for possible extensions in the future), is there a case for adding writeUint48() and writeUint64() (and the corresponding read functions) to the library?

src/lib/dns/tsigerror.{cc,h}
Inconsistent layout in the header file: the body of some methods is on the line after the function signature; for other methods it is on the same line.

src/lib/util/unittests/Makefile.am
Need to add src/lib/util/io/libutil_io.la to the link command line (else the dns tests fail to link). As an aside (and not related to this change), is there any reason why there is a separate util/io library rather than the "io" code being included in the main util library?

src/lib/util/unittests/newhook.{cc,h}
I think that the ability to do a memory allocation failure test is a useful addition to our test suite. In the comments in the code, I suggest emphasising that "throw_size_on_new" is the exact size that causes an exception to be thrown rather than the minimum size at which an exception is thrown. The comments mention memory allocation failure, which suggests that the number is a limit above which an allocation request fails.

Comments on the Design
TSIGContext: the decision to make this class independent from Message seems sound.

TSIGRecord: I admit that my first impulse would have been to derive the class from AbstractRRSet and implement restrictions on setTTL(). However, I accept the argument that TSIG is a very special RR type and so will only be used in special circumstances. If circumstances change, we should be prepared to re-visit this decision.

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

Thanks for the prompt review! I really didn't expect it to be that fast:-)

Replying to stephen:

src/lib/dns/tsigkey.{cc,h}
The method that returns the hash algorithm name is called getAlgorithmName(), but the (new) method that returns the hash algorithm itself is called getCryptoAlgorithm(). Wouldn't getAlgorithm() be more consistent?

Yeah, this is one of the points I was not sure. I chose
getCryptoAlgorithm() to make it clear that it would be used for
libcryptolink stuff. But since that might also be clear from the
return type, I'm fine with changing it to getAlgorithm() if you prefer
it.

(Review of differences between ad13479 and fd45c40)

src/lib/dns/tsig.{cc,h}
In TSIGContext::sign(), suggest adding a comment to the effect that TSIG uses a 48-bit time before the statement masking gettimeofdayWrapper with a constant. (It's not clear why the value is masked unless you really study RFC 2845.)

I've tried to clarify that with added comments (commit daf48e8),
please check.

In TSIGContext::sign(), the 48-bit time is written using writeUint16(). followed by writeUint32(). For completeness (and for possible extensions in the future), is there a case for adding writeUint48() and writeUint64() (and the corresponding read functions) to the library?

This is another thing I was not sure. I had checked the BIND 9
implementation (it has its own "buffer" module for 48-bit in/out), and
had found that TSIG is the only use case for the 48-bit interface. So
I hesitated to make the generic class more complicated only for one
single use case. I'm okay with adding it, but I'd personally hold off
until we see at least one other use case. And, in any case, I'd defer
it to a separate ticket.

src/lib/dns/tsigerror.{cc,h}
Inconsistent layout in the header file: the body of some methods is on the line after the function signature; for other methods it is on the same line.

You mean the following two, for example?

    uint16_t getCode() const { return (code_); }

    bool equals(const TSIGError& other) const
    { return (code_ == other.code_); }

Basically, I (personally) follow the rule that

  • if everything can fit in a single line (up to 79 columns), do it
  • otherwise, create a new line

(which I think is consistent with the BIND 9 coding guideline:
http://bind10.isc.org/wiki/BIND9CodingGuidelines)

But at the same time I love consistency:-) So, if you strongly want to
see consistent line-break rule in this file, I don't mind changing it.

src/lib/util/unittests/Makefile.am

Need to add src/lib/util/io/libutil_io.la to the link command line (else the dns tests fail to link). As an aside (and not related to this change), is there any reason why there is a separate util/io library rather than the "io" code being included in the main util library?

Hmm...it didn't happen for me and I'm not sure why (I guess we both
use Mac. Maybe this is because I mainly use clang++?), but anyway,
that would break building lib/util/unittests itself because in
util/Makefile.am unittests is built before io. I don't have an answer
to your "unrelated" question (you should ask Jerry I guess), but
that's probably related to this point.

My suggestion at the moment is to have dns/tests link libutil_io.la
and create a separate ticket to resolve the dependency problem in a
cleaner way. The current structure seems to be so fragile and I think
we need some fundamental cleanup. (This suggestion is not yet
committed).

src/lib/util/unittests/newhook.{cc,h}
I think that the ability to do a memory allocation failure test is a useful addition to our test suite. In the comments in the code, I suggest emphasising that "throw_size_on_new" is the exact size that causes an exception to be thrown rather than the minimum size at which an exception is thrown. The comments mention memory allocation failure, which suggests that the number is a limit above which an allocation request fails.

Okay, I've added some comments (commit 78bd2b7)

Comments on the Design

TSIGContext: the decision to make this class independent from Message seems sound.

TSIGRecord: I admit that my first impulse would have been to derive the class from AbstractRRSet and implement restrictions on setTTL(). However, I accept the argument that TSIG is a very special RR type and so will only be used in special circumstances. If circumstances change, we should be prepared to re-visit this decision.

Fair enough, and that (about TSIGRecord) was actually what I thought.

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to stephen

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

  • Owner changed from stephen to jinmei

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

Yeah, this is one of the points I was not sure. I chose getCryptoAlgorithm() to make it clear that it would be used for libcryptolink stuff. But since that might also be clear from the return type, I'm fine with changing it to getAlgorithm() if you prefer it.

I think it would be worth it (either that or change getAlgorithmName() to getCryptoAlgorithmName()). It was just that two names seemed to imply that there were two algorithms present.

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

I've tried to clarify that with added comments (commit daf48e8), please check.

The new comments provide suitable clarification.

I'd personally hold off until we see at least one other use case. And, in any case, I'd defer it to a separate ticket.

That seems sensible.

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

But at the same time I love consistency:-) So, if you strongly want to see consistent line-break rule in this file, I don't mind changing it.

I think it would look better, but I'll leave it up to you.

src/lib/util/unittests/Makefile.am

My suggestion at the moment is to have dns/tests link libutil_io.la and create a separate ticket to resolve the dependency problem in a cleaner way.

That is OK.

src/lib/util/unittests/newhook.{cc,h}

Okay, I've added some comments (commit 78bd2b7)

They are OK

All seems OK, so you can continue with the rest of the work on this ticket.

comment:13 in reply to: ↑ 12 Changed 8 years ago by jinmei

Replying to stephen:

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

But at the same time I love consistency:-) So, if you strongly want to see consistent line-break rule in this file, I don't mind changing it.

I think it would look better, but I'll leave it up to you.

For now I didn't change this part. This is not the only case of
such "inconsistency", so unless we have a unified policy as part of
coding guideline the inconsistency will be easily reintroduced anyway.

If you think it's worth having as a common guideline, please raise it
at the dev list.

All seems OK, so you can continue with the rest of the work on this ticket.

Okay, thanks, merged. I've opened a separate ticket for the 2nd part (#871),
so I'm closing this one. I'll also divide the total estimation to 6 and 2
and re-assign them to this ticket and #871, respectively.

comment:14 Changed 8 years ago by jinmei

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