SVCB/HTTPS RFC updates, ECH config tweaks #2183
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arkxfly
approved these changes
Apr 16, 2024
I had wanted to update the There are still a couple of parser bugs in there w.r.t how escaped characters are handled but it seems both very niche and complicated to fix. I've left one test vector commented out that demonstrates the issue in case someone else wants to follow-up with a fix. |
djc
reviewed
Apr 17, 2024
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.
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
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
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 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
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.
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.
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
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
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.
cpu
force-pushed
the
cpu-svcb-updates_dev
branch
from
fdf246f
to
1f846a1
Compare
djc
approved these changes
Apr 18, 2024
|
Nice! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch brings in some changes I've found helpful while working on client ECH support downstream in Rustls.
"ech", where as hickory was using"echconfig".EchConfigtoEchConfigListto better emphasize that the returned value's wire-encoding is a TLS-encodedECHConfigList, as specified in draft-ietf-tls-esni-18 §4.EchConfigListto be the wire-encoding, removing pre-processing that stripped the TLS encoded list length prefix. Downstream users will have an easier time with this based on existing TLS list decoding capabilities inherit to a TLS implementation. Similarly, the BASE64 presentation format is updated to encode the wire-encoded value with the prefixed length. This matches what tools likedigrender for what some public implementations have published in DNS.I haven't attempted to offer backwards compatible options based on the earlier draft content. I think that it's probably not worthwhile based on the very early state of ECH support broadly.