add EchConfigListBytes for encrypted client hello configs #46

cpu · 2024-04-10T21:36:28Z

lib: DerInner -> BytesInner

The crate-internal DerInner type is really a Cow-like type for generic bytes. The nature of those bytes is indicated by the pub wrapper type holding the DerInner.

With that in mind we can re-use it for other non-DER types (e.g. ECH configs) but first need to rename the type to BytesInner since it is no longer DER in all cases.

EchConfigListBytes for encrypted client hello configs

Raw ECH config lists are likely to be shared between Rustls and other crates (e.g. they may be read from rustls-pemfile and .pem inputs for server configuration, or they may be fetched from DNS using DNS-over-HTTPS using hickory-dns for client config).

This commit introduces a EchConfigListBytes type, wrapping BytesInner, that allows representing a borrowed or owned raw TLS encoded ECH config list, corresponding to the ECHConfigList type specified in TLS presentation language in draft-ietf-tls-esni-18 §4. This type can be used between crates to maintain typed context for the bytes throughout.

src/lib.rs

cpu · 2024-04-11T14:06:32Z

Cargo: v1.4.1 -> v1.5.0

I tacked on a version bump.

djc

Is PEM encoding of EchConfig a common/described use case somewhere, or something you're hypothesizing might be useful?

Cargo.toml

cpu · 2024-04-11T14:44:36Z

Is PEM encoding of EchConfig a common/described use case somewhere, or something you're hypothesizing might be useful?

It's a real use case but there's not a consistent implementation across the ecosystem yet.

In BoringSSL land their helper tools (e.g. bssl generate-ech) spit out and consume binary files. I don't think they've implemented anything for PEM.
For OpenSSL, and in the standards space, there's a draft from the author of the WIP OpenSSL support for ECH (ECH-PR openssl/openssl#22938). The described .pem format is implemented and used there. It's specific to the server usecase and prescribes both a PKCS#8 private key and a single ECHConfig per .pem
For Cloudflare's Go fork they're using their own thing that isn't specified anywhere that I know of, -----BEGIN ECH KEYS----- for keys and configs (plural) and -----BEGIN ECH CONFIGS----- for ECH configs (plural), in separate inputs.

Of the set I think draft-farrell-tls-pemesni-06 looks useful but is a bit rough and early days. I'd rather not implement something with zero spec ala CF's impl. Not implementing anything and using binary formats is OK short term, but I think the proliferation of .pem for other things shows there's demand for a non-binary format. I have a branch of pemfile implementing it but I'm not sure its worth landing yet, except perhaps to try and move draft-farrell-tls-pemesni-06 forward and get more people to converge on that format.

cpu · 2024-04-11T14:53:18Z

I have a branch of pemfile implementing it but I'm not sure its worth landing yet,

I was thinking something like this: rustls/pemfile@3d3a9ef

cpu · 2024-04-11T18:39:36Z

cpu marked this pull request as draft now

I want to hold on to this a bit longer. I might want to add another type for a TLS encoded list of configs.

cpu · 2024-04-12T18:12:15Z

I want to hold on to this a bit longer. I might want to add another type for a TLS encoded list of configs.

yeah I think this needs some rework, the new type should be for representing the encoding of a list, specifically ECHConfig ECHConfigList<1..2^16-1> from the draft, not a ECHConfig as implied by this branch.

The encoding of a single item is equivalent to a list of size 1. The raw value read from DNS that we want to represent here is in fact a list, I've just been confused by the common case being a single item that doesn't appear to be a list due to the rules of TLS presentation syntax.

cpu · 2024-04-15T19:38:42Z

I've just been confused by the common case being a single item that doesn't appear to be a list due to the rules of TLS presentation syntax.

There was another aspect to my confusion I figured out over the weekend. The Hickory DNS representation of EchConfig Svc param values strips the list length prefix, storing only the raw bytes after that in its wrapped Vec. The docs don't describe what format this Vec should be considered as holding, but it isn't the wire format (it's missing the length prefix required) and it isn't the presentation format (it's not the base64 of the wire format). You can get the value with the list prefix by calling EchConfig.emit() with a BinEncoder. I will do that in my client code as a temporary workaround to be able to treat everything consistently.

I think this is the wrong way for Hickory to present this data to a consumer. I believe most users of the data are going to be expecting the wire format described in the draft-ietf-tls-svcb-ech-01 and draft-ietf-tls-esni-18. It's easier to deserialize the list when you know can treat it the same as any other TLS encoded list, but this requires the list length prefix be in place. I'll propose an upstream change shortly to rework this interface.

In the meantime I think my conclusion is that we should not have a pki-types representation for a single ECHConfig, but only for ECHConfigList. That's what draft-ietf-tls-svcb-ech-01 describes as going in the DNS. That's what our client facing code wants to load to pick a compatible ECH config. That's what servers will want to load to specify the configs it understands/offers for retries.

cpu · 2024-04-15T23:02:28Z

I'll propose an upstream change shortly to rework this interface.

hickory-dns/hickory-dns#2183

The crate-internal `DerInner` type is really a `Cow`-like type for generic bytes. The nature of those bytes is indicated by the pub wrapper type holding the `DerInner`. With that in mind we can re-use it for other non-DER types (e.g. ECH configs) but first need to rename the type to `BytesInner` since it is no longer DER in all cases.

Raw ECH config lists are likely to be shared between Rustls and other crates (e.g. they may be read from `rustls-pemfile` and `.pem` inputs for server configuration, or they may be fetched from DNS using DNS-over-HTTPS using `hickory-dns` for client config). This commit introduces a `EchConfigListBytes` type, wrapping `BytesInner`, that allows representing a borrowed or owned raw TLS encoded ECH config list, corresponding to the `ECHConfigList` type specified in TLS presentation language in draft-ietf-tls-esni-18 §4[0]. This type can be used between crates to maintain typed context for the bytes throughout. [0]: https://datatracker.ietf.org/doc/html/draft-ietf-tls-esni-18#section-4

This matches our preference in the main rustls repo.

cpu · 2024-04-17T21:36:34Z

In the meantime I think my conclusion is that we should not have a pki-types representation for a single ECHConfig, but only for ECHConfigList.

I've reworked the branch around this conclusion and updated the commit/PR description accordingly. I think this is ready for re-review & I'll have an update to the WIP Rustls ECH PR to take a git patch on this branch shortly.

While I was here I also tacked on the ubuntu-20.04 -> ubuntu-latest change I'm proposing in rustls/webpki#248. Like over there this branch is now showing "Required statuses must pass before merging" and will need an admin merge so we can retroactively fix the branch protection rules.

src/lib.rs

djc · 2024-04-18T14:49:14Z

Can we derive them for BytesInner then, instead of manually implementing?

cpu · 2024-04-18T14:53:27Z

Can we derive them for BytesInner then, instead of manually implementing?

Hmmm. I don't think so because we want BytesInner::Owned(vec![0xFF]) to match BytesInner::Borrowed(&[0xFF]) right? Wouldn't a derived impl consider the enum variants first, and then their contents?

djc · 2024-04-18T14:55:26Z

Ah, yes!

cpu · 2024-04-18T14:56:39Z

Wouldn't a derived impl consider the enum variants first, and then their contents?

Yeah, I think so:

    #[test]
    fn bytes_inner_equality() {
        let owned = BytesInner::Owned(vec![1, 2, 3]);
        let borrowed = BytesInner::Borrowed(&[1, 2, 3]);
        assert_eq!(owned, borrowed);
    }

With the derived impl:

thread 'tests::bytes_inner_equality' panicked at src/lib.rs:825:9:
assertion `left == right` failed
  left: Owned([1, 2, 3])
 right: Borrowed([1, 2, 3])

With my manual impl:

test tests::bytes_inner_equality ... ok

I'll add some test coverage for this while I'm here. The derived version passed without error before I added a test.

djc · 2024-04-18T15:55:23Z

Can we do self.as_ref() == other.as_ref() in the PartialEq impl?

This allows deriving `PartialEq` and `Eq` automatically for the two wrapper types, `Der` and `EchConfigListBytes`. The `BytesInner` type is crate-internal and so we don't have to be worried about users accidentally comparing `BytesInner` from a `EchConfigListBytes` with a `Der` `BytesInner` without the broader type context catching the err. Similarly by dragging the meat of the `AsRef` impls down onto `BytesInner` we can simplify the `PartialEq` impl and wrapper type `AsRef` impls.

cpu · 2024-04-18T16:02:08Z

Can we do self.as_ref() == other.as_ref() in the PartialEq impl?

Ah! good idea. It was a similar situation here: AsRef was being implemented for the wrapping types manually, not for BytesInner. I dragged that trait impl down, used it in the partial eq, and updated the manual impls on the wrappers to defer to the wrapped type impl.

cpu · 2024-04-18T18:11:34Z

src/lib.rs

-
-#[derive(Clone)]
-enum DerInner<'a> {
+#[derive(Debug, Clone)]


note: BytesInner also gains a derived Debug in this changeset to be usable with assert_eq! in the added unit test.

cpu · 2024-04-23T14:36:16Z

@ctz Do you have interest in reviewing this branch? I think its in a stable state now and I don't expect to need any other ECH related changes in this repo.

ctz

👍🏽

cpu · 2024-04-23T16:11:38Z

admin merged
fixed the branch protection rules for ubuntu-latest.
pushed v/1.5.0 tag
Published rustls-pki-types v1.5.0 at registry crates-io
Created v1.5.0 GitHub release

cpu self-assigned this Apr 10, 2024

djc reviewed Apr 11, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

cpu force-pushed the cpu-client-ech branch from ceea7d9 to 65427f2 Compare April 11, 2024 14:03

cpu changed the title ~~add TlsBytes, EchConfigBytes~~ add EchConfigBytes for encrypted client hello configs Apr 11, 2024

djc reviewed Apr 11, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

djc approved these changes Apr 11, 2024

View reviewed changes

cpu force-pushed the cpu-client-ech branch from 8449b10 to 2b39806 Compare April 11, 2024 14:27

cpu marked this pull request as draft April 11, 2024 18:38

cpu mentioned this pull request Apr 12, 2024

Encrypted Client Hello support (client only) rustls/rustls#1718

Merged

cpu added 3 commits April 17, 2024 17:24

ci: ubuntu-20.04 -> ubuntu-latest

00df307

This matches our preference in the main rustls repo.

cpu force-pushed the cpu-client-ech branch from 2b39806 to ee8e8ef Compare April 17, 2024 21:33

cpu changed the title ~~add EchConfigBytes for encrypted client hello configs~~ add EchConfigListBytes for encrypted client hello configs Apr 17, 2024

cpu marked this pull request as ready for review April 17, 2024 21:34

djc approved these changes Apr 18, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

cpu force-pushed the cpu-client-ech branch from ee8e8ef to 7533867 Compare April 18, 2024 14:47

cpu force-pushed the cpu-client-ech branch from 7533867 to 6ecd054 Compare April 18, 2024 15:00

cpu added 2 commits April 18, 2024 12:01

Cargo: v1.4.1 -> v1.5.0

65ed67d

cpu force-pushed the cpu-client-ech branch from 6ecd054 to 65ed67d Compare April 18, 2024 16:01

cpu commented Apr 18, 2024

View reviewed changes

djc approved these changes Apr 18, 2024

View reviewed changes

cpu requested a review from ctz April 19, 2024 18:09

ctz approved these changes Apr 23, 2024

View reviewed changes

cpu merged commit e341f9c into rustls:main Apr 23, 2024
13 checks passed

cpu deleted the cpu-client-ech branch April 23, 2024 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add EchConfigListBytes for encrypted client hello configs #46

add EchConfigListBytes for encrypted client hello configs #46

cpu commented Apr 10, 2024 •

edited

Loading

cpu commented Apr 11, 2024 •

edited

Loading

djc left a comment

cpu commented Apr 11, 2024 •

edited

Loading

cpu commented Apr 11, 2024

cpu commented Apr 11, 2024

cpu commented Apr 12, 2024

cpu commented Apr 15, 2024 •

edited

Loading

cpu commented Apr 15, 2024

cpu commented Apr 17, 2024 •

edited

Loading

djc commented Apr 18, 2024

cpu commented Apr 18, 2024

djc commented Apr 18, 2024

cpu commented Apr 18, 2024

djc commented Apr 18, 2024

cpu commented Apr 18, 2024 •

edited

Loading

cpu Apr 18, 2024

cpu commented Apr 23, 2024

ctz left a comment

cpu commented Apr 23, 2024

add EchConfigListBytes for encrypted client hello configs #46

add EchConfigListBytes for encrypted client hello configs #46

Conversation

cpu commented Apr 10, 2024 • edited Loading

lib: DerInner -> BytesInner

EchConfigListBytes for encrypted client hello configs

cpu commented Apr 11, 2024 • edited Loading

djc left a comment

Choose a reason for hiding this comment

cpu commented Apr 11, 2024 • edited Loading

cpu commented Apr 11, 2024

cpu commented Apr 11, 2024

cpu commented Apr 12, 2024

cpu commented Apr 15, 2024 • edited Loading

cpu commented Apr 15, 2024

cpu commented Apr 17, 2024 • edited Loading

djc commented Apr 18, 2024

cpu commented Apr 18, 2024

djc commented Apr 18, 2024

cpu commented Apr 18, 2024

djc commented Apr 18, 2024

cpu commented Apr 18, 2024 • edited Loading

cpu Apr 18, 2024

Choose a reason for hiding this comment

cpu commented Apr 23, 2024

ctz left a comment

Choose a reason for hiding this comment

cpu commented Apr 23, 2024

cpu commented Apr 10, 2024 •

edited

Loading

cpu commented Apr 11, 2024 •

edited

Loading

cpu commented Apr 11, 2024 •

edited

Loading

cpu commented Apr 15, 2024 •

edited

Loading

cpu commented Apr 17, 2024 •

edited

Loading

cpu commented Apr 18, 2024 •

edited

Loading