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

rec: add bogus ring to make it more easy to detect high profile domains with broken dnssec #6713

Merged
merged 4 commits into from Jun 21, 2018

Conversation

mind04
Copy link
Contributor

@mind04 mind04 commented Jun 5, 2018

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)

@rgacogne rgacogne added this to the rec-4.2.0 milestone Jun 5, 2018
@rgacogne
Copy link
Member

rgacogne commented Jun 5, 2018

The packet cache unit tests do not compile anymore :'(

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature in itself is great, code looks very good. I'm not a big fan of adding this logic in the cache code, though.

t_bogusremotes->push_back(source);
if(t_bogusqueryring)
t_bogusqueryring->push_back(make_pair(qname, qtype));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit wrong to have this code here, IMHO the packet cache should not have any knowledge of the rings or the requestor's source address. Perhaps this logic could be moved in doProcessUDPQuestion()?

@mind04 mind04 force-pushed the bogus-ring branch 2 times, most recently from ddcf6ae to 88694a6 Compare June 18, 2018 18:23
@rgacogne rgacogne changed the title rec: add bugus ring to make it more easy to detect high profile domains with broken dnssec rec: add bogus ring to make it more easy to detect high profile domains with broken dnssec Jun 20, 2018
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, looks very good otherwise. Thanks a lot for this PR!

@@ -174,6 +178,7 @@ void RecursorPacketCache::insertResponsePacket(unsigned int tag, uint32_t qhash,
e.d_ttd = now+ttl;
e.d_creation = now;
e.d_tag = tag;
e.d_vstate = valState;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should likely also update the validation state when we replace an existing entry, just above.

@rgacogne rgacogne merged commit c5c9ae7 into PowerDNS:master Jun 21, 2018
@mind04 mind04 deleted the bogus-ring branch June 21, 2018 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants