Better DNSSEC proofs

[bluejekyll]

I've taken an initial stab at cleaning up the DNSSEC validation and move to associating a Proof to each record. This differs a little bit from what was discussed in #1708 as I think this might be the best option.

This adds a Proof enum that has the Secure, Bogus, Insecure, and Indeterminate as members.
Once validated it associates that proof to the Record, there's more cleanup here as we only support Secure records being returned at the moment, and even though we can detect Bogus, it's returned as an Error right now, which I think we decided we want to move away from.

I still need to add the Insecure path, but before I do that I wanted to see if anyone wanted to offer early feedback. Also, I want to reduce more cloning. I was able to remove quite a bit that was still there from before async/await was finalized. I think there's more yet that can be done.

Fixes: #1708
Fixes: #2095
Fixes: #1650

[bluejekyll]

Ok, I think this is ready. The big change here is that records are always returned on lookups now, rather than removing them from the response if they were unable to be verified. Record now has an associated Proof which matches the RFC, Secure, Insecure, Bogus and Indeterminate.

I haven't implemented this across all Lookup types in the resolver interface yet, if folks like this interface I can add it to all the Lookup types, so feedback is desired here. I adde these interfaces to Lookup:

pub fn dnssec_iter(&self) -> DnssecIter<'_>;
pub fn dnssec_record_iter(&self) -> DnssecLookupRecordIter<'_>;

This dnssec_iter returns an iterator over the RData of the response records, with the interface of:

impl<'a> Iterator for DnssecIter<'a> {
    type Item = Proven<&'a RData>;
    ...
}

And then for a full Record if desired, the dnssec_record_iter will return a similar iterator but with Record. The Proven type has these two methods:

pub fn require_as_ref<I: Into<ProofFlags>>(&self, flags: I) -> Result<&T, Proof>;
pub fn require<I: Into<ProofFlags>>(self, flags: I) -> Result<T, Self>;

The idea is that one would use the resolver as normal (switching on the configs for performing DNSSEC validation) and then use the dnssec iterators in order to ensure that they evaluate the DNSSEC status of the record before use, for example:

let lookup = resolver.lookup("www.example.com", RecordType::A).unwrap();
for data in lookup.dnssec_iter() {
     // Only evaluate the data if it was either proven to be `Secure` or `Insecure`, 
     //    i.e. signed or a zone that's known to not be signed 
     let a = data.try_borrow(Proof::Secure | Proof::Insecure).unwrap();

     // do something with the A record
     ...
}

Now, a note based on my testing. It seems that upstream resolvers may not return NSEC records for names that don't exist. This matters because the DS record is used as "proof" that the zone is unsigned. At some level in the DNS chain, there will always be a DS record, for example .org. is a zone known to be signed. That means we should get an NSEC record for the DS of a sub zone, like not-signed.org. but without the NSEC records being returned, we can't prove that the DS doesn't exist, which puts the logic into a Bogus state, i.e. .org is signed, but there is no signed non-existence record for the sub zone. This should be an insecure state. I considered trying to determine if we should just say Indeterminate in this case, but that doesn't "feel" correct.

On a similar line, we still don't have NSEC3 validation, and some zones are not returning NSEC, only NSEC3, which means we can't evaluate non-existence properly, so you might get Bogus on non-existence.

Overall, this was a big change, so I'm guessing there are other gaps.

Fixes: #1708, #2095, #1650,

[bluejekyll]

FYI, @kevincox, @teythoon, @moparisthebest, @wez, I think you've all expressed interest here.

[wez]

I'm excited to see this!
I not familiar enough with most of the implementation to comment on that, but I do have some feedback on the proof/proven stuff.

[bluejekyll]

Waiting on one interface discussion, otherwise, I think this is finally ready to land.

[bluejekyll]

@djc, I think I'd like to land this. We could squash the commit if you want to clean up the number of changes here...

[djc]

I think this merits a round of review before landing? Might take me a bit to get too, though.

[bluejekyll]

I wish there was a simple way to go back and clean up the PRs. There's some interfaces that ended up in there that I removed later, like TypedRecordData.

[djc]

I wish there was a simple way to go back and clean up the PRs.

You're aware of git rebase --interactive, right?

[bluejekyll]

Yes, but it can be a lot of work to remove code, I can take a look and see.

[wez]

re: tidying up, if it proves to be too difficult to fixup the code in a commit without dealing with 20+ merge conflicts, you could just comment in the commit description to explain it.

eg: if a commit was:

Introduce TypedRecordData to blah blah blah.

you could amend the description:

Introduce TypedRecordData. Note that in a later commit in this stack, it is removed and replaced by XXX, so
don't sweat the details too much.

As an aside/slight plug: https://sapling-scm.com/ (to which I used to be a contributor) has some nice UX around dealing with a stack of commits. You can directly check out a commit from the stack, then amend it and it will automatically rebase the rest of the stack. Perhaps too late to help with this stack now, I wanted to mention that it has a killer absorb command that can intelligently amend code changes to appropriate commits (yes, plural!) in the stack in a single invocation.

While sapling was built for FB's fork of Mercurial, it also works on Git repositories. If you find yourself regularly dealing with big stacks like this, I think it is worth evaluating. I think my personal record was ~100 commits in a stack and refactoring that thing after review feedback was surprisingly manageable.

If you like the idea of absorb, but don't or can't use sapling for whatever reason, it looks like someone has ported that concept to git over in https://github.com/tummychow/git-absorb. I don't know how well this particular implementation works.

[djc]

I often use git-absorb, works well in all the straightforward cases!

(From the Google side of the fence there's also jujutsu which I think fits in the same general category as sapling.)

[bluejekyll]

I'm not going to have time to go back through the commits. I'd love to try and get this landed though, so let me know what I can do to help.

[bluejekyll]

ok, I finally had a moment to clean up that unnecessary stuff. I think this is ready to go, sorry for the largish change, let me know if you want me to simplify this by squashing some of the commits.

[bluejekyll]

@djc any comments on this PR? I'd really like this to be landed before more DNSSEC changes start coming in.

[djc]

(This is on my radar -- it's been very busy -- sorry I haven't gotten to it.)

[bluejekyll]

@djc, I'm going to merge this as we have other work coming into the project in this area now that this should really land in support of. If you can come back to this and review, I can put up a PR to resolve those issues.