Skip to content

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.

}

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)?

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