they did not previously allow any way to mock DNS query sequences
in a manner compatible with the RecursorPool, which prefers to create
new NameServers itself, rather than via anything injectable by the test.
The RFC 9460 presentation syntax allows escaping the ',' list separator
in a `SvcParamValue` by writing `\,`. This commit updates `parse_list`
to handle this case.
Once this is done, one of the RFC 9460 test vectors using this feature
can be added to the unit tests. We don't yet support the more
complicated escaped comma using an escaped backslash for delimiter
escape so one remaining test vector for this is left out.
Previously the `FromStr` impl for `SvcParamKey` had support for parsing
the "arbitrary key" presentation syntax where a key can be specified
"keyNNNNN", where NNNNN is the numeric value of the key type without
leading zeros. The existing code would pull out the numeric component
into a `u16` and then use the `TryFrom<u16>` impl for `SvcParamKey` to
get the key.
However, the `TryFrom<u16>` impl for `SvcParamKey` was using the IANA
service parameter keys registry to map from u16s to `SvcParamKey`.
Values 0..6 are mapped to the known key entries. The reserved range
(65280-65534) was mapped to `SvcParamKey::Key`, and 65535 was mapped to
`SvcParamKey::Key65535`. This makes sense when mapping an arbitrary u16,
but when we are parsing a "keyNNNNN" presentation syntax item, we want
to represent it as `Key(NNNNN)`, no matter if it is/isn't a registered
key.
This commit fixes this behaviour, constructing a `SvcParamKey::Key()`
entry when parsing the arbitrary key presentation syntax, avoiding
`TryFrom<u16>`.
With this change in place the two arbitrary key test vectors can be
included in the svcb test vector unit test.
[0] https://datatracker.ietf.org/doc/html/rfc9460#name-initial-contents
See https://datatracker.ietf.org/doc/html/rfc9460#appendix-D
Tests that we can parse the presentation format for each of the positive
test vectors.
Possible follow-up work:
* Add the negative failure tests
* Add coverage for wire format matching expected
The presentation format for RFC9460 SVCB/HTTPS RR types allows for
parameter values to be quoted. The code for processing parameter values
in hickory-dns had a comment indicating quotes should have been
stripped, but this wasn't occurring in practice.
This commit updates the parsing logic to perform the mentioned quote
stripping, and updates the unit tests with fresh data found from doing
`HTTPS` lookups for `google.com` and `crypto.cloudflare.com`. Notably
both of these show quoted strings in `dig`'s presentation format output
and so were sufficient to tickle the bug fixed above.
Previously the hickory-dns representation of ECH configs found in
SVCB/HTTPS records held and exposed its own non-standard representation
of the encoded ECH configs. Notably, it stripped the TLS-encoded list
length prefix from the remaining data. Similarly, it's presentation
format was the BASE64 encoding of this non-standard form.
Downstream consumers are likely to want the wire-encoding format
unmodified, because ECH is of most use to TLS libraries where they will
have already implemented a generic TLS-encoded list decoder that expects
the length prefix. In practice, popular tools like `dig`
are also encoding the presentation format BASE64 of the data in DNS for
some popular test servers with the prefix included.
This commit updates hickory-dns's representation to not do the
pre-processing it was before. This is trivial for a consumer to do if
they need it, and avoids having to restore it manually in order to use
other pre-existing TLS encoder/decoders with the value from hickory-dns.
Again, since ECH adoption is in very early days it doesn't seem
worthwhile to try and come up with a backwards compatible interface for
those that need the old behaviour. It should be straightforward to
remove the length prefix manually if required.
Previously the hickory-dns representation for ECH configurations in
SVCB/HTTPS records were named `EchConfig` and stored/exposed
a non-standard encoding of the config data, with the TLS-encoded length
prefix stripped.
In practice (and perhaps made clearer by draft-ietf-tls-svcb-ech-01[0]
vs earlier texts), the value in wire-encoded form is "an ECHConfigList"
as specified in Section 4 of draft-ietf-tls-esni-18[1] in TLS
presentation language as:
```
ECHConfig ECHConfigList<1..2^16-1>;
```
To make it clearer that it's a _list_ of `ECHConfig` values in the
`ech=` SVCB/HTTPS key, this commit renames the types to emphasize their
listy-ness.
[0]: https://datatracker.ietf.org/doc/html/draft-ietf-tls-svcb-ech-01
[1]: https://datatracker.ietf.org/doc/html/draft-ietf-tls-esni-18#section-4
Prev the ECH config parsing was placed in between parsing ipv4 and ipv6
hints. This commit reorders so that `parse_ech_config()` is after
`parse_ipv4_hint()` and `parse_ipv6_hint()`.
Previously `"echconfig"` was being used as the encrypted client hello
(ECH) service parameter key for SVCB/HTTPS RRs.
In RFC960 the parameter key is specified in the intial IANA registry
contents as `"ech"`[0].
This commit updates the two relevant parts of hickory (and corresponding
test data) to use the up-to-date parameter key.
This is a breaking change, however given the very low adoption of ECH,
and the use of the correct `"ech"` key in popular test servers, it
doesn't seem worth trying to maintain backwards compatibility with
earlier draft RFC values.
[0]: https://datatracker.ietf.org/doc/html/rfc9460#section-14.3.2
Since initial support for SVCB/HTTPS RRs landed in hickory-dns, RFC
9460[0] was published:
Service Binding and Parameter Specification via the DNS (SVCB and HTTPS
Resource Records)
This is the definitive reference for SVCB and HTTPS RRs and previous
references to `draft-ietf-dnsop-svcb-https-XX` need to be updated.
Thankfully, it seems as though the implementation did not change
meaningfully from draft-03 and so this commit can largely just update
documentation references and copied quotations to match RFC 9460.
One minor change is worth mentioning: the Encrypted Client Hello (ECH)
aspects of the draft were removed pre-publication and the RFC9460 IANA
registry includes a "reserved" allocation for the `"ech"` key, but no
details on its use. These details are now located in a separate draft,
draft-ietf-tls-svcb-ech-01[1].
Since the code in `svcb.rs` also concerned itself with ECH it now
references draft-ietf-tls-svcb-ech-01 where the ECH specific usage of
service parameter is under specification. Notably the new draft and RFC
9460 both use `"ech"` for the service parameter key for encrypted client
hello configs. Hickory-dns is currently using `"echconfig"`, but this
will be fixed in a follow-up commit to keep this one documentation only.
[0]: https://datatracker.ietf.org/doc/html/rfc9460
[1]: draft-ietf-tls-svcb-ech-01
Pushing branches named "$WHATEVER_dev" will result in CI being run. This
is helpful for those working on a fork that want a quick way to test CI
for their branch before opening a PR.
If we find that we've constructed a Rustls root cert store that has no
trust anchors, return an early error. This makes the problem obvious
and avoids surfacing some other less specific error cause when we first
try to validate a peer certificate with an empty root store.
In order for our new early error to be surfaced correctly the
`name_sever_pool.rs` `parallel_conn_loop` fn needs its error handling
adjusted. Previously it would always compare the new error produced by
trying to build the TLS config against the default error it starts its
loop with, `ProtoErrorKind::NoConnections`. Since the error being
returned is another `ProtoErrorKind`, and the error specificity
comparison considers two `ProtoErrorKinds` equivalent in the general
case, the default error was always returned and the new error thrown
away.
`ProtoErrorKind` is `Clone`, but the `Io` variant holding `io:Error`
runs into trouble with this: since the error can't be cloned we have to
reconstruct it and this is a lossy process: resulting in a "simple"
`io::Error` that only holds the error type from the parent it was cloned
from. This loses important details like the underlying error
source/message.
This commit changes `ProtoErrorKind::Io` to hold `Arc<io::Error>>`
instead. This makes implementing `Clone` trivial - we clone the arc
- and no error information is lost.
