Opened 9 years ago

Closed 9 years ago

#955 closed defect (fixed)

xfrin should check TSIG before other part of incoming message

Reported by: jinmei Owned by: zzchen_pku
Priority: medium Milestone: Sprint-20110614
Component: xfrin 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

The current implementation of xfrin checks some DNS message values
before checking TSIG. This should be reversed. (not a big deal,
though, because bogus responses would be discarded either way and it's
in the context of receiving responses so no compliance issue on how to
respond to them exists).

Subtickets

Attachments (1)

xfrin-test.diff (1.6 KB) - added by jinmei 9 years ago.

Download all attachments as: .zip

Change History (20)

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 stephen

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

comment:4 Changed 9 years ago by zzchen_pku

  • Owner set to zzchen_pku
  • Status changed from new to assigned

comment:5 follow-up: Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to UnAssigned
  • Status changed from assigned to reviewing

Ready for review, let xfrin check tsig status first.

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

Replying to zzchen_pku:

Ready for review, let xfrin check tsig status first.

Unless I miss something, there's no test for this.

Also, before picking up a new development task (after finishing one),
could you first consider purging open review requests? If everyone
only takes on new development tasks and keeps asking for review, the
size of the review queue will be monotonically increased and will
never be empty.

comment:7 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to zzchen_pku

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

Replying to jinmei:

Replying to zzchen_pku:

Ready for review, let xfrin check tsig status first.

Unless I miss something, there's no test for this.

Because I just reversed the checking order, both of the checking will raise XfrinException? and discard bogus responses, so the function of the interface has not been changed, the original test should be enough to handle it.

Also, before picking up a new development task (after finishing one),
could you first consider purging open review requests? If everyone
only takes on new development tasks and keeps asking for review, the
size of the review queue will be monotonically increased and will
never be empty.

oh, currently, almost all review tickets have been assigned to somebody, except #767 and #755.
(#767 require discussion and #755 was done after I picked this task.)

comment:9 Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to UnAssigned

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

Replying to zzchen_pku:

Unless I miss something, there's no test for this.

Because I just reversed the checking order, both of the checking will raise XfrinException? and discard bogus responses, so the function of the interface has not been changed, the original test should be enough to handle it.

Not really. We can still check whether the exception error message is
the expected one (and I think we should). In general, when we wanted
to say "we don't need a test for this case", we should re-think at
least three times if it's really, really true. IMO in many cases it's
not (and, often, embarrassingly, there's a bug that could be
identifiable via tests).

And, this remind me of another issue I've been having with xfrin and
xfrout: they use the same single exception in so many places,
obscuring the accuracy of test results. When we do
self.assertRaises(XfrinException?, xxx), we cannot really be sure if
the exception is really triggered at where it should be triggered. We
could check the exception error message as I said above, but since the
error message tends to be modified it will be unstable. I guess we'll
need a higher granularity of exceptions (or sub types of them) for
these apps. But I understand that's beyond the scope of this ticket.

Also, before picking up a new development task (after finishing one),
could you first consider purging open review requests? If everyone
only takes on new development tasks and keeps asking for review, the
size of the review queue will be monotonically increased and will
never be empty.

oh, currently, almost all review tickets have been assigned to somebody, except #767 and #755.
(#767 require discussion and #755 was done after I picked this task.)

When this ticket was picked up, #957 was already in the review queue
(though I'm not sure if there are others). Maybe you checked the
queue first and simply overlooked it and then picked up this one. If
so, that's fine; I've been a bit concerned about the size of review
queue for a while, and just wanted to make sure reviews are generally
given a higher priority.

comment:11 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to zzchen_pku

comment:12 in reply to: ↑ 10 ; follow-up: Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to jinmei

Replying to jinmei:

Not really. We can still check whether the exception error message is
the expected one (and I think we should). In general, when we wanted
to say "we don't need a test for this case", we should re-think at
least three times if it's really, really true. IMO in many cases it's
not (and, often, embarrassingly, there's a bug that could be
identifiable via tests).

Okay, I have added check for error message.

And, this remind me of another issue I've been having with xfrin and
xfrout: they use the same single exception in so many places,
obscuring the accuracy of test results. When we do
self.assertRaises(XfrinException?, xxx), we cannot really be sure if
the exception is really triggered at where it should be triggered. We
could check the exception error message as I said above, but since the
error message tends to be modified it will be unstable. I guess we'll
need a higher granularity of exceptions (or sub types of them) for
these apps. But I understand that's beyond the scope of this ticket.

Agree, can you create ticket for it?

comment:13 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 2

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

Replying to zzchen_pku:

Replying to jinmei:

Not really. We can still check whether the exception error message is
the expected one (and I think we should). In general, when we wanted
to say "we don't need a test for this case", we should re-think at
least three times if it's really, really true. IMO in many cases it's
not (and, often, embarrassingly, there's a bug that could be
identifiable via tests).

Okay, I have added check for error message.

Your solution is impressive:-) but I have a concern. With this approach
the original stdout is closed after a test, which may cause unexpectedly
bad side effect (even if not, it's generally not good to leave a global
side effect after a test case). I'd suggest catching and examining
the exception and the error message explicitly. See the attached
sample diff.

We may want to generalize it and move it somewhere else so that other
tests can use it; or this may just have to be a short term workaround
until we solve the other generic issue (below). In any case that will
be beyond the scope of this ticket, so I'm okay with having it within
xfrin for now.

Note also that with this approach you won't need to tweak the body of
xfrin as you did in the previous commit.

Agree, can you create ticket for it?

Yes, please.

Changed 9 years ago by jinmei

comment:15 Changed 9 years ago by jinmei

  • Owner changed from jinmei to zzchen_pku

comment:16 in reply to: ↑ 14 ; follow-up: Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to jinmei

Replying to jinmei:

Your solution is impressive:-) but I have a concern. With this approach
the original stdout is closed after a test, which may cause unexpectedly
bad side effect (even if not, it's generally not good to leave a global
side effect after a test case). I'd suggest catching and examining
the exception and the error message explicitly. See the attached
sample diff.

We may want to generalize it and move it somewhere else so that other
tests can use it; or this may just have to be a short term workaround
until we solve the other generic issue (below). In any case that will
be beyond the scope of this ticket, so I'm okay with having it within
xfrin for now.

Note also that with this approach you won't need to tweak the body of
xfrin as you did in the previous commit.

Good suggestion, applied:-)

Agree, can you create ticket for it?

Yes, please.

#1007 has been created for it.

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

Replying to zzchen_pku:

We may want to generalize it and move it somewhere else so that other
tests can use it; or this may just have to be a short term workaround
until we solve the other generic issue (below). In any case that will
be beyond the scope of this ticket, so I'm okay with having it within
xfrin for now.

Note also that with this approach you won't need to tweak the body of
xfrin as you did in the previous commit.

Good suggestion, applied:-)

I've made a copule of additional changes. (If they are okay) with these
I'm okay with the branch. Please merge.

Agree, can you create ticket for it?

Yes, please.

#1007 has been created for it.

Ack, thanks.

comment:18 Changed 9 years ago by jinmei

  • Owner changed from jinmei to zzchen_pku

comment:19 Changed 9 years ago by zzchen_pku

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

Okay, merged, thanks.

Note: See TracTickets for help on using tickets.