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

plugin/sign: fix signing of authoritative data #3479

Merged
merged 1 commit into from Dec 6, 2019
Merged

plugin/sign: fix signing of authoritative data #3479

merged 1 commit into from Dec 6, 2019

Conversation

@miekg
Copy link
Member

miekg commented Nov 24, 2019

Don't sign data we are not authoritative for. This adds an AuthWalk
which skips names we should not authoritative for. Adds a few tests to
check this is the case. Generates zones have been compared to
dnssec-signzone.

A number of changes have been made:

  • don't add DS records to the apex
  • NSEC TTL is the SOA's minttl value (copying bind9)
  • Various cleanups

Fixes: #3354

Bulk of the code is new tests cases. The gist is file/tree/authwalk.go and sign/signer.go

Signed-off-by: Miek Gieben miek@miek.nl

@corbot corbot bot requested a review from chrisohaver Nov 24, 2019
@corbot

This comment has been minimized.

Copy link

corbot bot commented Nov 24, 2019

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked chrisohaver (via /OWNERS) for a review.
Note this is not an exclusive request. Anyone is free to provide a review of this pull request.

If you have questions or suggestions for this bot, please file an issue against the miekg/dreck repository.

The bot understands the commands that are listed here.

@miekg miekg force-pushed the auth10x branch from da99484 to 3267339 Nov 24, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 24, 2019

Codecov Report

Merging #3479 into master will decrease coverage by 0.02%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3479      +/-   ##
==========================================
- Coverage   56.66%   56.64%   -0.03%     
==========================================
  Files         221      222       +1     
  Lines       11027    11041      +14     
==========================================
+ Hits         6249     6254       +5     
- Misses       4294     4308      +14     
+ Partials      484      479       -5
Impacted Files Coverage Δ
plugin/file/tree/auth_walk.go 0% <0%> (ø)
plugin/file/tree/walk.go 0% <0%> (ø) ⬆️
plugin/sign/signer.go 55.14% <100%> (+3.83%) ⬆️
plugin/sign/nsec.go 100% <100%> (+27.77%) ⬆️
plugin/file/reload.go 69.44% <0%> (-5.56%) ⬇️
plugin/sign/file.go 69.38% <0%> (+12.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45c4355...5a76e53. Read the comment docs.

@miekg miekg force-pushed the auth10x branch from caebc71 to 6aed527 Nov 25, 2019
plugin/file/tree/walk.go Outdated Show resolved Hide resolved
@chrisohaver

This comment has been minimized.

Copy link
Member

chrisohaver commented Dec 4, 2019

@miekg, I'm finding I need to familiarize my self with DNSSEC before I can understand the PR and do a full/fair review.
This will slow my review. If you'd prefer a faster code review, feel free to rope in others.


// Check if the current name is a subdomain of *any* of the delegated names we've seen, if so, skip this name.
// The ordering of the tree and how we walk if guarantees we see parents first.
if n.Elem.Type(dns.TypeNS) != nil {

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Dec 5, 2019

Member

Will this tag the origin/apex as delegated? Does this need check to see if n.Elem.Name() == origin?
maybe by checking for an SOA? e.g. n.Elem.Type(dns.TypeNS) != nil && n.Elem.Type(dns.TypeSOA) == nil

This comment has been minimized.

Copy link
@miekg

miekg Dec 6, 2019

Author Member

Yes, this is a bit icky; i will not by virtue for zones as these apex records somewhere else: https://github.com/coredns/coredns/blob/master/plugin/file/zone.go#L24

Don't sign data we are not authoritative for. This adds an AuthWalk
which skips names we should not authoritative for. Adds a few tests to
check this is the case. Generates zones have been compared to
dnssec-signzone.

A number of changes have been made:

* don't add DS records to the apex
* NSEC TTL is the SOA's minttl value (copying bind9)
* Various cleanups
* signer struct was cleaned up: doesn't need ttl, nor expiration or
  inception.
* plugin/sign: remove apex stuff from names()
  This is never used because we will always have other types in the
  apex, because we *ADD* them ourselves, before we sign (DNSKEY, CDS and
  CDNSKEY).

Signed-off-by: Miek Gieben <miek@miek.nl>
Co-Authored-By: Chris O'Haver <cohaver@infoblox.com>
@miekg miekg force-pushed the auth10x branch from acbc606 to 5a76e53 Dec 6, 2019
Copy link
Member

chrisohaver left a comment

LGTM - Although I don't yet fully understand NSEC type bitmaps, and therefore can't verify that change. But otherwise LGTM.

@miekg

This comment has been minimized.

Copy link
Member Author

miekg commented Dec 6, 2019

@miekg miekg merged commit a53321d into master Dec 6, 2019
5 checks passed
5 checks passed
DCO DCO
Details
ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
codecov/project 56.64% (target 50%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
@miekg miekg deleted the auth10x branch Dec 6, 2019
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.