Skip to content
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

Add EDNS unknown version handling #7406

Merged
merged 7 commits into from Feb 21, 2019

Conversation

Projects
None yet
3 participants
@alenichev
Copy link
Contributor

commented Jan 22, 2019

Short description

Respond with RCODE=BADVERS on EDNS version >0 (as per rfc6891).
Turned out to be a rule to match on the EDNS version and a action to return ERCODE.

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)

@Habbie Habbie added the dnsdist label Jan 22, 2019

@Habbie

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Thank you for this work. Can you please update your message based on our pull request template at https://github.com/PowerDNS/pdns/blob/master/.github/PULL_REQUEST_TEMPLATE.md ?

Also, it appears your work breaks a bunch of our tests, as reported by Travis.

@rgacogne
Copy link
Member

left a comment

In my humble opinion, this is not something that a load-balancer like dnsdist should do, at least not by default. It might however make sense to have a rule to match on the EDNS version, along with an action to be able to return BADVERS if that rule matches.

Show resolved Hide resolved pdns/dnsdist-ecs.cc Outdated
Show resolved Hide resolved pdns/dnsdist.cc Outdated
Show resolved Hide resolved pdns/dnsdist.cc Outdated
replace RCODE=BADVERS response on version >0 with a rule to match on the
highest implementation level of EDNS version and action to return the
ERCode.
@alenichev

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

Thank you very much for comments, patch updated with a rule and action.
It is necessary to set rcode, because extended 12-bit RCODE is a 8 bits of EXTENDED-RCODEOPT field together with the 4 bits of RCODE.

@rgacogne rgacogne self-requested a review Jan 25, 2019

@rgacogne
Copy link
Member

left a comment

PR looks quite nice, thank you a lot! I'd really like to see some documentation and ideally a few regression tests before we merge this. Would you be willing to give it a try? Otherwise do you mind if push a few commits to this PR?

EDNS0Record edns0;
static_assert(sizeof(EDNS0Record) == sizeof(uint32_t), "sizeof(EDNS0Record) must match sizeof(uint32_t) AKA RR TTL size");
// copy out 4-byte "ttl" (really the EDNS0 record), after root label (1) + type (2) + class (2).
memcpy(&edns0, packet + optStart + 5, sizeof edns0);

This comment has been minimized.

Copy link
@rgacogne

rgacogne Feb 14, 2019

Member

There is a lot of code duplication with ERCodeRule, I think we need to turn that into a function that would be better off in dnsdist-ecs.cc. But we can do that refactoring after this PR has been merged if needed.

@rgacogne rgacogne added this to the dnsdist-1.3.x milestone Feb 14, 2019

rgacogne added some commits Feb 21, 2019

@rgacogne rgacogne merged commit 163e0cb into PowerDNS:master Feb 21, 2019

1 check passed

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

@rgacogne rgacogne referenced this pull request Mar 11, 2019

Merged

dnsdist: Properly initialize DNQuestion::ednsRCode #7564

3 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.