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

rec: Authority records in AA=1 CNAME answer are authoritative #6979

Merged
merged 1 commit into from Oct 8, 2018

Conversation

Projects
None yet
2 participants
@rgacogne
Member

rgacogne commented Sep 19, 2018

Short description

The records other than the CNAME for the initial target in ANSWER are not, nor are the ADDITIONAL ones, but AUTHORITY records are.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
rec: Authority records in AA=1 CNAME answer are authoritative
The records other than the CNAME for the initial target in ANSWER
are not, nor are the ADDITIONAL ones, but authority records are.
@rgacogne

This comment has been minimized.

Member

rgacogne commented Oct 4, 2018

@pieterlexis Ping? :-)

@@ -2117,7 +2117,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
set the TC bit solely because these RRSIG RRs didn't fit."
*/
bool isAA = lwr.d_aabit && i->first.place != DNSResourceRecord::ADDITIONAL;
if (isAA && isCNAMEAnswer && (i->first.place != DNSResourceRecord::ANSWER || i->first.type != QType::CNAME || i->first.name != qname)) {
if (isAA && isCNAMEAnswer && i->first.place == DNSResourceRecord::ANSWER && (i->first.type != QType::CNAME || i->first.name != qname)) {

This comment has been minimized.

@pieterlexis

pieterlexis Oct 8, 2018

Member

Code change looks good (as it seems to do what you describe). Perhaps a comment here and multi-lining the statement makes sense?

This comment has been minimized.

@rgacogne

rgacogne Oct 8, 2018

Member

Multi-lining might be a good idea indeed!
I briefly pondered adding a comment, but it would basically be "AUTHORITY records in AA=1 answers are authoritative", which is well, kind of obvious when you put it that way :) Also this test is already in the middle of 20 lines of comments, so... :)

@rgacogne rgacogne merged commit de2fe0e into PowerDNS:master Oct 8, 2018

4 checks passed

LGTM analysis: C/C++ No alert changes
Details
LGTM analysis: JavaScript No alert changes
Details
LGTM analysis: Python No alert changes
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rgacogne rgacogne deleted the rgacogne:rec-cname-authority branch Oct 8, 2018

rgacogne added a commit to rgacogne/pdns that referenced this pull request Oct 9, 2018

@rgacogne rgacogne referenced this pull request Oct 9, 2018

Merged

rec: Fix a compilation issue in the SyncRes unit tests #7044

4 of 7 tasks complete

@rgacogne rgacogne referenced this pull request Nov 9, 2018

Open

rec: Skip NS for the exact zone in CNAME answers #7178

5 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment