Conversation
folbricht
left a comment
folbricht
left a comment
There was a problem hiding this comment.
What happens when a domain isn't signed? Will this fail? I'm wondering if there should be a flag or so to switch into a more permissive mode where DNSSEC responses are validated, but responses for unsigned domains are passed through.
dnssec-backend.go
Outdated
| MsgHdr: dns.MsgHdr{ | ||
| RecursionDesired: true, | ||
| }, |
There was a problem hiding this comment.
Technically you don't need this as SetQuestion() below does it already (see https://github.com/miekg/dns/blob/v1.1.63/defaults.go#L35).
| return err | ||
| } | ||
|
|
||
| for _, rr := range signedZone.Dnskey.RrSet { |
There was a problem hiding this comment.
getRRSet() can return nil, nil which would trigger a panic here
dnssec-backend.go
Outdated
| } | ||
|
|
||
| func extractRRset(r *dns.Msg) (*RRSet, error) { | ||
| result := &RRSet{RrSet: make([]dns.RR, 0)} |
There was a problem hiding this comment.
| result := &RRSet{RrSet: make([]dns.RR, 0)} | |
| result := &RRSet{} |
No need to initialize an empty list, you can read from and append to a nil list.
dnssec-backend.go
Outdated
| return nil, ErrNoResult | ||
| } | ||
| if r == nil || r.Rcode == dns.RcodeSuccess { | ||
| return r, err |
There was a problem hiding this comment.
| return r, err | |
| return r, nil |
err has already been checked and must be nil here.
dnssec-backend.go
Outdated
| defer func() { | ||
| if r := recover(); r != nil { | ||
| Log.Warn("panic occurred", | ||
| slog.String("domainName", domainName), | ||
| slog.Any("panic", r), | ||
| ) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Do we really need this? Seems more like we don't trust our own code to handle all nil pointers correctly.
dnssec-backend.go
Outdated
| Log.Error("DS query failed", | ||
| slog.String("domainName", domainName), | ||
| "error", err, | ||
| ) | ||
|
|
||
| return err |
There was a problem hiding this comment.
Rather than logging from within, might be better to extend the error and pass it back to the caller to handle. Something like
return fmt.Errorf("DS query failed for %q: %w", domainName, err)
dnssec-backend.go
Outdated
| if err := g.Wait(); err != nil { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
| if err := g.Wait(); err != nil { | |
| return err | |
| } | |
| return nil | |
| return g.Wait() |
dnssec-backend.go
Outdated
| defer func() { | ||
| if err := recover(); err != nil { | ||
| Log.Error("AuthChain panic occurred", "error", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Again, there should be a better way to deal with panics, i.e. avoid them at all.
|
Is there anything else you're planning to add to this PR? Overall my main concern is around calling it DNSSEC when it really is just a partial implementation. There are several more safeguards in the spec that aren't really handled by this implementation. It gets complex fast, which is also why I haven't tried it myself yet. Including, but probably not limited to:
Perhaps we should call out that this is just a partial/experimental implementation? There are quite a few weaknesses in it and users shouldn't fully rely on it. |
|
I am currently on vacation, so I won't be able to work on this until the end of the month. I should have some time then to dive deeper into DNSSEC and extend this PR. You can decide if you want to merge it already, but I agree with your point that some additional information should be included. |
|
I added support for loading in trust-anchor root-keys from a XML file. (Currently it only supports loading in the root zone "." ) And in the latest commit I added support for NSEC and NSEC3 Proof of Non-Existence. I am not confident about my code here, but it seemed to work for all domains I tested so far. I couldn't find anything on how to prove unsigned domains are to be unsigned. |
This reverts commit a4fa6b3.
2 participants
This is to start the process of integrating a DNSSEC validator to routedns.
This implementation is inspired by uw-ictd/DNSSEC-Validator and peterzen/goresolver, but adds concurrency and is adapted to work with the other routedns modules.
Currently, only validated DNS responses are forwarded. DNS responses that are not singed or have an invalid signature are dropped.
Example config file: