Opened 8 years ago

Closed 8 years ago

#1551 closed defect (fixed)

in-memory datasource allow RRSIG, NSEC, and CNAME at same time

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

Description

Error: Server configuration failed: CNAME and RRSIG can't coexist for ...

Allow RFC 4034 Section 3 for RRSIG and RFC 4034 Section 4 for NSEC. (Where is this said for NSEC3?)

Subtickets

Change History (14)

comment:1 Changed 8 years ago by jelte

this exception does not apply for NSEC3 because the NSEC3's owner name is hashed and should not match any other existing owner name

comment:2 Changed 8 years ago by jinmei

  • Milestone changed from New Tasks to Next-Sprint-Proposed

It's probably better to handle this issue after #1614.

comment:3 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 4

comment:4 Changed 8 years ago by jelte

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

comment:5 Changed 8 years ago by jinmei

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

comment:6 Changed 8 years ago by jinmei

trac1551 is ready for review.

This should be simple enough. (So I also added a couple of unrelated
cleanups).

One note if it's not obvious: I made this branch after merging
trac1574b. This branch doesn't depend on 1574b, but modifies the
same file, so (assuming 1574b is close to merge) I thought it's better
to do so we can reduce the risk of having conflict at merge time. The
first commit is for the merge and should be ignored.

Another note: the case for CNAME+RRSIG was already covered in #1614.
But I added an explicit test for that in this branch.

This is the proposed changelog entry:

373.?	[bug]		jinmei
	libdatasrc: the in-memory data source incorrectly rejected loading
	a zone containing a CNAME RR with RRSIG and/or NSEC.
	(Trac #1551, git TBD)

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 vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei

Hello

The code seems to do what it should. But I think the condition looks really awkward and complicated:

        // owner name except with NSEC, which is the only RR that can coexist
        // with CNAME (and also RRSIG, which is handled separately)
        if (rrset.getType() == RRType::CNAME()) {
            if (!domain.empty() &&
                (domain.size() > 1 ||
                 (domain.begin()->second->getType() != RRType::NSEC()))) {

Could I propose using a for loop, iterating through all the RRsets there and stopping on the first one that is not NSEC? This loop would, in fact, do 1 iteration at most, but it would be more obvious what is the intent ‒ that there's nothing except the NSEC record. It would also be easily extendable for more exceptions (we already allow DNAME and CNAME to coexist, don't we?).

Thank you

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

Replying to vorner:

The code seems to do what it should. But I think the condition looks really awkward and complicated:

        // owner name except with NSEC, which is the only RR that can coexist
        // with CNAME (and also RRSIG, which is handled separately)
        if (rrset.getType() == RRType::CNAME()) {
            if (!domain.empty() &&
                (domain.size() > 1 ||
                 (domain.begin()->second->getType() != RRType::NSEC()))) {

Could I propose using a for loop, iterating through all the RRsets there and stopping on the first one that is not NSEC? ...

I agree that the quoted original code wasn't intuitive. I didn't like
it either but thought it was probably more efficient as in the vast
majority of the cases the given zone should be valid and the check
would stop at empty(). But that's probably a premature (and probably
not so effective anyway) optimization, and especially because you also
saw it awkward, I now think it's better to prefer clarity.

I used find_if from <algorithm> instead of explicit loop:

            if (find_if(domain.begin(), domain.end(), IsNotNSEC())
                != domain.end()) {

because, IMO, explicit loop + check is also an indirect idiom from
what it actually means and is essentially counter intuitive (but the
idiom is so common for many experienced programmers that it could look
natural). Unfortunately, in C++ the algorithmic approach also
requires an indirection (we need to define a separate predicate
class), so, in the end, it may be a matter of taste.

So, if you strongly prefer a for loop in this case, I can live with
that.

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei
  • Total Hours changed from 0 to 0.75

Hello

Yes, you're right. This one is probably more to the point. How unfortunate C++ doesn't support closures.

I'd like to point out that a functor is maybe a little bit overkill in this case, a function could be enough. But it probably doesn't matter, so I'll leave it up to you if you want to change it to a functor.

Otherwise it would be ready to merge.

Thank you

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

Replying to vorner:

Yes, you're right. This one is probably more to the point. How unfortunate C++ doesn't support closures.

Boost.Lambda might make it look nicer, and I experimentally tried to
use it in fact, but many versions of it except very recent ones
seemed to have compilation issue (they caused an "unused function
parameter" warning), and I didn't want to weaken the compiler setting
just for that. Besides, it's probably too much anyway to introduce
another template bloat for this simple case.

Anyway,

I'd like to point out that a functor is maybe a little bit overkill in this case, a function could be enough. But it probably doesn't matter, so I'll leave it up to you if you want to change it to a functor.

ah, right. A function is better in terms of conciseness and of the
principle of preferring the least powerful tool that achieves the
goal. I changed it and merged the branch to master.

Now closing.

comment:14 Changed 8 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0.75 to 2.42
Note: See TracTickets for help on using tickets.