Add SSHFP and OPENPGPKEY support

[amesgen]

This PR (addresses #646) adds support for the SSHFP and OPENPGPKEY record types. It is WIP as tests are still missing.

• The implementation does not check if the OPENPGPKEY record data is a valid OpenPGP public key, even though there exist implementations of the TPK (Transferable Public Key) format in rust (such as in sequoia-openpgp).
• The Fingerprint enum of SSHFP does not include XMSS, which is included in OpenSSH. It does not seem to be standardized.
• As OPENPGPKEY records tend to be very large, they do not work well with UDP (they are also great for amplification attacks, just like DNSSEC in general). It could be reasonable to enforce TCP for (answering) OPENPGPKEY records. But I do not think this is too much of an issue.

The license headers are also missing in new files, as I was not sure which year to choose.

[amesgen]

Two small things that I noticed: I do not understand this test. And is this line intended to be there?

[codecov]

Codecov Report

Merging #647 into master will decrease coverage by 0.78%.
The diff coverage is 1.44%.

@@            Coverage Diff             @@
##           master     #647      +/-   ##
==========================================
- Coverage    86.3%   85.53%   -0.78%     
==========================================
  Files         142      146       +4     
  Lines       14951    15089     +138     
==========================================
+ Hits        12903    12905       +2     
- Misses       2048     2184     +136

[codecov]

Codecov Report

Merging #647 into master will increase coverage by 0.03%.
The diff coverage is 87.1%.

@@            Coverage Diff             @@
##           master     #647      +/-   ##
==========================================
+ Coverage    86.3%   86.33%   +0.03%     
==========================================
  Files         142      146       +4     
  Lines       14951    15196     +245     
==========================================
+ Hits        12903    13119     +216     
- Misses       2048     2077      +29

[bluejekyll]

This is looking good. I'm going to be on a plane today, so I can't promise a full review before tomorrow.

It is WIP as tests are still missing.

At a minimum, a parser test (can be the examples in the RFC's) would be good for the text parsing. For the binary, I'm fine with create the struct in Rust, then serialize/deserialize and validate that the result is expected. It should be a good enough sanity test.

The implementation does not check if the OPENPGPKEY record data is a valid OpenPGP public key, even though there exist implementations of the TPK (Transferable Public Key) format in rust (such as in sequoia-openpgp).

Yeah, let's not pull in a large dep. That can be the responsibility of high level libs validating the data.

The Fingerprint enum of SSHFP does not include XMSS, which is included in OpenSSH. It does not seem to be standardized.

Agreed, let's only go with what people are using. It can be added later.

As OPENPGPKEY records tend to be very large, they do not work well with UDP (they are also great for amplification attacks, just like DNSSEC in general). It could be reasonable to enforce TCP for (answering) OPENPGPKEY records. But I do not think this is too much of an issue.

This is an area I want to improve in general. At some point we'll probably start enforcing max packets sizes of 512 for UDP requests and truncating, but you shouldn't worry about that in this change. There's a bunch to clean up there.

Two small things that I noticed: I do not understand this test.

That test is to enforce canonical ordering of records: https://tools.ietf.org/html/rfc4034#section-6

Maybe put a comment in there that refers back to that rfc? That's less specific about the record types, and more about the name. You shouldn't need to modify that test as it's based off the RecordType assigned u16 value. Here's where that ordering is defined: https://github.com/bluejekyll/trust-dns/blob/master/crates/proto/src/rr/record_type.rs#L299-L309 (which is probably the better place for that comment).

And is this line intended to be there?

No! That should definitely be removed (must have come in after debugging something, I have a bad habit of that).

The license headers are also missing in new files, as I was not sure which year to choose.

I copy this into each file. You can update this to 2019, first patch of the year ;)
https://github.com/bluejekyll/trust-dns/blob/master/copyright.txt (oh, and feel free to put your name/company in any file you edit as an addendum, which is up to you, example). As you've noticed, this is out of date on some of the older files, I generally update those as I edit them.

[bluejekyll]

Also, if you don't mind, add this in the root CHANGELOG.md under the 0.16 release, in a new Added section if it doesn't exist (there are examples in that file)

[bluejekyll]

This looks really good. I don't see any major concerns. Thank you for taking care of putting in the RFC references in the comments.

[bluejekyll]

I'm assuming this means that this format isn't currently supported by the data_encoding library?

[amesgen]

Yes, it is not in data_encoding. It is copy-pasted from here. It could be shared, is there a canonical module in trust_dns for "utility things"?

[bluejekyll]

Not as such. I’m guessing there might be enough things around data encoding/base64 where this might be desirable.

Don’t worry about that in this pr though, that seems like something we could clean up later.

[bluejekyll]

This competes with a similar name, if this generally is only used in this context, it's probably ok, but an FYI: https://github.com/bluejekyll/trust-dns/blob/master/crates/proto/src/rr/dnssec/algorithm.rs#L104

[amesgen]

I have changed the representation format parsing so that the fingerprint must be in exactly one entry and not the rest is put together.

[bluejekyll]

Ok, this looks great! Thanks for the PR!