Opened 9 years ago

Closed 9 years ago

#813 closed enhancement (complete)

TSIG: verifying messages

Reported by: stephen Owned by: jinmei
Priority: medium Milestone: Sprint-20110517
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: 4.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Note that TCP support also works for UDP, although we need access to the wire format.

Subtickets

Change History (11)

comment:1 Changed 9 years ago by shane

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

comment:2 Changed 9 years ago by shane

  • Defect Severity set to N/A
  • Feature Depending on Ticket set to tsig
  • Sub-Project set to DNS

comment:3 Changed 9 years ago by jinmei

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

comment:4 Changed 9 years ago by jinmei

Branch trac813 is ready for review. It's based on a recent version
of the trac871 branch, but is mostly independent from that branch
functionality wise. So this branch can be reviewed separately.

Like the signing case, I chose separating this task to two subtasks:
parsing incoming TSIG RR and verifying it. This branch implements the
first part. I plan to create a separate ticket for the latter.

Other notes:

  • the first commit (8320ec6) can be ignored. It simply incorporates trac871.
  • commit e22700f (the changes under lib/util/unittests) is not directly related this task but I introduced it for additional tests for Message::toText(). Also, some part of it has duplicate functionality with lib/dns/unittest_util. I plan to unify them and move generic utility to lib/util/unittests, which will be a separate task (and separate ticket)
  • the total diff is not small, but excluding the new files in unittests (they should be quite trivial) and test data, (I hope) it's essentially not that big.

comment:5 Changed 9 years ago by jinmei

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

comment:6 Changed 9 years ago by jinmei

  • Estimated Difficulty changed from 7.0 to 4.0

comment:7 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei

Hello

I made few typo and style fixes. Otherwise the code looks clear.

But I have a question ‒ you accept TSIG only at the end of the message. But there was something with signing of the stream when there's zone transfer. Just to make sure, the stream is split into multiple messages and the TSIGs are on some of the messages and they cover all the preceding messages, so TSIG will be at the end of message here as well, right? (quick google for „TSIG zone transfer“ isn't very helpful this morning)

So, if this is how the transfer works, I think it is safe to be merged (provided the branch this is based on is reviewed already).

Thanks

comment:9 in reply to: ↑ 8 ; follow-up: Changed 9 years ago by jinmei

Replying to vorner:

Thanks for the review.

I made few typo and style fixes. Otherwise the code looks clear.

The changes look good, thanks for cathing them. (And you taught me
another cool feature of gtest:-)

But I have a question ‒ you accept TSIG only at the end of the message. But there was something with signing of the stream when there's zone transfer. Just to make sure, the stream is split into multiple messages and the TSIGs are on some of the messages and they cover all the preceding messages, so TSIG will be at the end of message here as well, right? (quick google for „TSIG zone transfer“ isn't very helpful this morning)

Yes. To make sure we are on the same page, this is a specific example:

  • First DNS message in a TCP stream: it must have a TSIG. it must be placed at the end of that message.
  • Second DNS message in the same stream (a continuation of the first one for the entire zone transfer). It can skip including TSIG.
  • Third DNS message in the same stream. It can skip including TSIG, too.
  • Fourth DNS message in the same stream. This is the last one for the zone transfer. It must have a TSIG (which also covers both the second and third messages), and it must be placed at the end of the message.

So, if this is how the transfer works, I think it is safe to be merged (provided the branch this is based on is reviewed already).

Okay, so I think it's ready for merge. Once #871 is merged, I'll
merge this branch, too.

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

Replying to jinmei:

So, if this is how the transfer works, I think it is safe to be merged (provided the branch this is based on is reviewed already).

Okay, so I think it's ready for merge. Once #871 is merged, I'll
merge this branch, too.

Merge done. Closing ticket.

Thanks,

comment:11 Changed 9 years ago by jinmei

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