New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lookups one level (or more) below apex did confuse getAuth() for qytpe DS #5519

Merged
merged 4 commits into from Aug 14, 2017

Conversation

Projects
None yet
4 participants
@mind04
Contributor

mind04 commented Jul 13, 2017

Short description

lookups one level (or more) below apex did confuse getAuth() for qytpe DS

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@Habbie Habbie added this to the auth-4.1.0 milestone Aug 14, 2017

@Habbie Habbie merged commit 76b530e into PowerDNS:master Aug 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mind04 mind04 deleted the mind04:ds branch Aug 14, 2017

@mind04 mind04 referenced this pull request Sep 15, 2017

Closed

Fix finding parent zone for DS queries #5693

3 of 6 tasks complete
@klaus3000

This comment has been minimized.

Show comment
Hide comment
@klaus3000

klaus3000 Sep 15, 2017

I still think this fix is incomplete (only 90% sure as this logic is not easy to reverse engineer and the documentation of the code is sparse).

Consider the scenario where there is only 1 backend and this backend immediately responds with the best match. E.g. target="a.b.c.example.com" and the backend responses with "com".

In this case, shorter=target=a.b.c.example.com. If the query is for DS, PowerDNS would still "chasing next" as the if condition "target != shorter" is wrong. The proper comparison would be target against sd->qname (compare with the zone returned from the backend, not with the zone the backend was asked).

Further, in case of DS queries, I think getAuth() should return false if there is no parent zone found, instead of using the zone which is identical to the queried label:

Hence, I think the proper fix would be:

found:
+    // DS records must be answered from the parent zone (the zone
+    // which has the delegation). Hence, if we found Authority, we
+    // start again and chase the next zone.
+    // There is one exception: if the first found Authority is not
+    // identical to the queried target, we already have the parent
+    // zone found.
-    if(found == (qtype == QType::DS) || target != shorter) {
+    if(found == (qtype == QType::DS) || target != sd->qname) {
      DLOG(L<<Logger::Error<<"found: "<<sd->qname<<endl);
      return true;
    } else {
      DLOG(L<<Logger::Error<<"chasing next: "<<sd->qname<<endl);
      found = true;
    }

  } while(shorter.chopOff());
+  // DS queries must be answered from the parent zone. If we do not
+  // find a parent zone, return false
+  if (found && (qtype == QType::DS) && (target == sd->qname)) {
+    return false;
+  }
  return found;
}

klaus3000 commented Sep 15, 2017

I still think this fix is incomplete (only 90% sure as this logic is not easy to reverse engineer and the documentation of the code is sparse).

Consider the scenario where there is only 1 backend and this backend immediately responds with the best match. E.g. target="a.b.c.example.com" and the backend responses with "com".

In this case, shorter=target=a.b.c.example.com. If the query is for DS, PowerDNS would still "chasing next" as the if condition "target != shorter" is wrong. The proper comparison would be target against sd->qname (compare with the zone returned from the backend, not with the zone the backend was asked).

Further, in case of DS queries, I think getAuth() should return false if there is no parent zone found, instead of using the zone which is identical to the queried label:

Hence, I think the proper fix would be:

found:
+    // DS records must be answered from the parent zone (the zone
+    // which has the delegation). Hence, if we found Authority, we
+    // start again and chase the next zone.
+    // There is one exception: if the first found Authority is not
+    // identical to the queried target, we already have the parent
+    // zone found.
-    if(found == (qtype == QType::DS) || target != shorter) {
+    if(found == (qtype == QType::DS) || target != sd->qname) {
      DLOG(L<<Logger::Error<<"found: "<<sd->qname<<endl);
      return true;
    } else {
      DLOG(L<<Logger::Error<<"chasing next: "<<sd->qname<<endl);
      found = true;
    }

  } while(shorter.chopOff());
+  // DS queries must be answered from the parent zone. If we do not
+  // find a parent zone, return false
+  if (found && (qtype == QType::DS) && (target == sd->qname)) {
+    return false;
+  }
  return found;
}
@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Sep 15, 2017

Member

Hello,

comments on closed tickets tend to get forgotten. If you believe more work needs to be done, can you please open a ticket?

Member

Habbie commented Sep 15, 2017

Hello,

comments on closed tickets tend to get forgotten. If you believe more work needs to be done, can you please open a ticket?

@klaus3000

This comment has been minimized.

Show comment
Hide comment
@klaus3000

klaus3000 commented Sep 15, 2017

Done: #5697

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment