Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement SVCB and HTTPS RRs #83

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from
Draft

Conversation

cormacrelf
Copy link

@cormacrelf cormacrelf commented Sep 27, 2021

Fixes #76

$ cargo run -q -p dog -- HTTPS crypto.cloudflare.com # line wrapped for github
HTTPS crypto.cloudflare.com. 4m07s   1 . alpn=h2 ipv4hint=162.159.135.79,162.159.136.79
    ech=AEb+DQBCigAgACDsxSq7stP1EM9FNA8OgbEid4L0EMilddE8PIc6s8xYCwAEAAEAAQATY2xvdWRmbGFyZS1lc25pLmNvbQAA
    ipv6hint=2606:4700:7::a29f:874f,2606:4700:7::a29f:884f

And piping the --json output through my other little tool to decode the ECH configuration:

cargo run -q -p dog -- HTTPS crypto.cloudflare.com --json \
    | jq '.responses[] .answers[].data.params.ech' -r \
    | cargo run -q -p ech-config \
    | jq
[
  {
    "version": 65037,
    "contents": {
      "key_config": {
        "config_id": 138,
        "kem_id": "DHKEM_X25519_HKDF_SHA512",
        "public_key": "7MUqu7LT9RDPRTQPDoGxIneC9BDIpXXRPDyHOrPMWAs=",
        "cipher_suites": [
          {
            "kdf_id": "HKDF_SHA256",
            "aead_id": "AES_128_GCM"
          }
        ]
      },
      "maximum_name_length": 0,
      "public_name": "cloudflare-esni.com",
      "extensions": []
    }
  }
]

@cormacrelf
Copy link
Author

Some notes:

  1. I have fuzz tested this for an hour on the default settings. There was an initial hit due to some stuff that's no longer an issue because it doesn't do a hacky rewind-double-parse of the ech config any more (moved to another crate), but after that, nothing.

  2. There's an implementation of #[feature(cursor_remaining)] that enables much less copying in the parsing routines.

  3. There's a bit too much code in the value_list module. The parsing of presentation-formatted values seems a bit out of scope, if there is a particular goal for the dns crate I'm not sure that would be part of it. OTOH the parsing is very similar to the escaping.

  4. The ECH decoding is probably out of scope, hence another crate. I can remove this entirely if that's better.

  5. Misc changes affecting other parts of dog:

  • strings are only given quotes if they need them. This affects e.g. TXT records, it's a very standard approach.
  • Labels now writes a dot if it's empty
  • There's a u16_enum! macro that seems like it might be useful elsewhere. It has a spill-over/catch-all field.

// let contains_spaces = true;
if contains_spaces {
write!(f, "\"")?;
}

for byte in self.0.iter().copied() {
if byte < 32 || byte >= 128 {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reminds me, I think dog's existing ascii escaping here is not quite correct, it's meant to be three digits unconditionally so fails for the byte < 32 case. If you slap this output in a zone file it may not parse. (Scroll down in the actual file)

"mandatory": mandatory.iter().map(|x| x.to_string()).collect::<Vec<String>>(),
"alpn": alpn.as_ref().map(|x| x.ids.iter().map(|id| id.to_string()).collect::<Vec<String>>()),
"port": *port,
"no-default-alpn": alpn.as_ref().map(|x| x.no_default_alpn).unwrap_or(false),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we prefer some kind of underscores convention or matching the presentation format in the RFC (hyphens as shown here)?

@@ -65,6 +66,7 @@ impl Colours {
sshfp: Cyan.normal(),
soa: Purple.normal(),
srv: Cyan.normal(),
svcb: Cyan.normal(),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Picked to match SRV records which these new ones are somewhat similar to.

#[serde(untagged)]
pub enum ECHConfigContents {
// if version == 0xfe0d
Version0xfe0d {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call this Draft13 because that's what 0xfe0d means

let inner = self.get_ref();
let len = inner.len() as u64;
let start = self.position().min(len);
let end = (start + to_length).min(len);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. This should obviously just error if the remaining slice is not big enough to hold the requested length. And return a wire error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SVCB and HTTPS query type support
1 participant