search

tlswg / draft-ietf-tls-esni

TLS Encrypted Client Hello
https://tlswg.github.io/draft-ietf-tls-esni/#go.draft-ietf-tls-esni.html
Other
231 stars 57 forks source link

Preserve length prefixes in ClientHelloOuterAAD #442

Closed davidben closed 3 years ago

davidben commented 3 years ago

After having written so many variations of ClientHelloOuterAAD construction (client and server, test and production), I think this may be simpler. A little bit less ClientHello rewriting. The cost is predicting the payload length on the client (same as PSK binders), but that prediction is just adding the tag length to len(EncodedClientHelloInner). The server, in particular, needs to parse and then re-encode an arbitrary incoming ClientHelloOuter with transforms.

Thoughts?

davidben commented 3 years ago

Did I miss the rationale for not including the HPKE parameters? I know that some of these are in HPKE and so they are redundant here, but they are encoded in the e_c_h extension, so maybe there is a different spelling of this that only drops/zeros-out the encrypted portion.

That's exactly what this PR does. payload is the encrypted portion. (The whole extension body is extension_data in RFC8446. Guessing that was the disconnect?)

martinthomson commented 3 years ago

Ah, precise terminology is useful up until the point that you aren't intimately familiar with each and every term. That would be OK, but as I said, it's not an improvement for us.

davidben commented 3 years ago

Hehe. I'm mostly hoping it'd tidy up the server side. When you decide to do ECH, you've already got to have parsed the ClientHello and found the ECH extension. Instead of going back, trimming out the ECH extension, preserving the surrounding extensions, and then updating the extensions length prefix, you can just take the offset of the payload field you just found, zero that and leave the rest alone.

martinthomson commented 3 years ago

Yeah, if it is a server-side improvement (we don't heavily optimize any of this stuff, so we don't care much), then I won't stand in the way.

briansmith commented 3 years ago

The benefit of this change, IIRC, is that you don't need to reserialize anything. But the new proposed text still talks about reserializing. It would be good to find a way to reword it so it doesn't imply that reserialization is needed.

davidben commented 3 years ago

@briansmith Rephrased.

briansmith commented 3 years ago

@davidben LGTM! Thanks for making that change.

chris-wood commented 3 years ago

@davidben would you mind resolving conflicts here?

davidben commented 3 years ago

would you mind resolving conflicts here?

Rebased.

chris-wood commented 3 years ago

Thanks! Paging @cjpatton, @sftcd, and @cbartle891 for additional reviews.

cbartle891 commented 3 years ago

Apart from my question about clarification, makes sense to me.

sftcd commented 3 years ago

On 08/06/2021 00:57, Christopher Wood wrote:

Thanks! Paging @cjpatton, @sftcd, and @cbartle891 for additional reviews.

LGTM

S.

davidben commented 3 years ago

I'm kind of thrown off by the use of the word "predict." It suggests to me that there's a chance that the prediction might be wrong. Should it not be "compute" instead? Or is there in fact a chance it might be wrong?

Good idea. I went to make that more precise and, in the process, realized a whole lot of the client procedure was specified funny so e5ddeb9af5f4f142e436e399c1496faceaeb7e83 fixes that up. (Those issues apply to the existing text too, so if we end up not merging this PR, I'll write an equivalent PR with just those bits.)

[edit: changed commit hash. old one had a typo.] [edit: so did the second one. bah, okay, going to stop force-pushing now...]