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: Don't negcache entries for longer than their RRSIG validity #5773

Merged
merged 1 commit into from Oct 6, 2017

Conversation

Projects
None yet
4 participants
@rgacogne
Member

rgacogne commented Oct 5, 2017

Short description

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)

@rgacogne rgacogne added this to the rec-4.1.0 milestone Oct 5, 2017

@rgacogne rgacogne requested a review from pieterlexis Oct 5, 2017

@Habbie

One question, otherwise approved. Review by @pieterlexis would still be neat.

rec.d_place = records[recordsCount-1].d_place;
rec.d_name = records[recordsCount-1].d_name;
rec.d_type = QType::RRSIG;
rec.d_ttl = sigValidity;
rec.d_ttl = records[recordsCount-1].d_ttl;

This comment has been minimized.

@Habbie

Habbie Oct 6, 2017

Member

shouldn't we cap this to the max of d_ttl and sigValidity?

@Habbie

Habbie Oct 6, 2017

Member

shouldn't we cap this to the max of d_ttl and sigValidity?

This comment has been minimized.

@rgacogne

rgacogne Oct 6, 2017

Member

Section 3 of rfc4034 1 states:

The TTL value of an RRSIG RR MUST match the TTL value of the RRset it covers.

so this looks right to me?

@rgacogne

rgacogne Oct 6, 2017

Member

Section 3 of rfc4034 1 states:

The TTL value of an RRSIG RR MUST match the TTL value of the RRset it covers.

so this looks right to me?

This comment has been minimized.

@Habbie

Habbie Oct 6, 2017

Member

I may be confused, but say we receive an RRSIG with an 86400 TTL but expiry is in a few hours. We should only cache it for those few hours, and I get the impression this change would cache it for a day?

@Habbie

Habbie Oct 6, 2017

Member

I may be confused, but say we receive an RRSIG with an 86400 TTL but expiry is in a few hours. We should only cache it for those few hours, and I get the impression this change would cache it for a day?

This comment has been minimized.

@rgacogne

rgacogne Oct 6, 2017

Member

Well this code is only there to generate mockup RRSIGs records for our unit tests, so it is intentionally designed to allow the creation of weird records :-)

@rgacogne

rgacogne Oct 6, 2017

Member

Well this code is only there to generate mockup RRSIGs records for our unit tests, so it is intentionally designed to allow the creation of weird records :-)

This comment has been minimized.

@Habbie

Habbie Oct 6, 2017

Member

durrrr

@Habbie

Habbie Oct 6, 2017

Member

durrrr

@Habbie

Habbie approved these changes Oct 6, 2017

rec.d_place = records[recordsCount-1].d_place;
rec.d_name = records[recordsCount-1].d_name;
rec.d_type = QType::RRSIG;
rec.d_ttl = sigValidity;
rec.d_ttl = records[recordsCount-1].d_ttl;

This comment has been minimized.

@Habbie

Habbie Oct 6, 2017

Member

durrrr

@Habbie

Habbie Oct 6, 2017

Member

durrrr

@aerique aerique merged commit 3e05703 into PowerDNS:master Oct 6, 2017

1 check passed

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

@rgacogne rgacogne deleted the rgacogne:rec-check-negative-rrsig-validity branch Oct 6, 2017

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