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
229 stars 58 forks source link

redundant length #486

Closed sftcd closed 3 years ago

sftcd commented 3 years ago

ECHConfig.length is either redundant or else it needs more explanation. This PR assumes the former. Either's fine by me (my draft-10 code supports >1 key per supplied ECHConfig for obscure API reasons anyway:-). This change notes that the field is currently redundant as there's only one ECHConfigContents per ECHConfig. If we want to allow >1 ECHConfigContents per ECHConfig then we should say that explicitly here and likely somewhere else too.

I'd have to look again at the SVCB spec but I think this being redundant also means that a deployment needs to have >1 HTTPS/SVCB RR value to publish more than one key at a time. If that's correct it may be worth noting here somewhere too.

davidben commented 3 years ago

No, the length prefix is necessary. It's not about having multiple ECHConfigContents per ECHConfig. It's about having multiple ECHConfigs in an ECHConfigList, and being able to skip over ECHConfigs from versions you don't understand. This property is a requirement for allowing us to experiment at all, without having to define a new SVCB/HTTPS key on every draft.

(TLS structures are not automatically delimited by length prefixes. You need to know their definition in order to skip over them. That means we need a version-independent ECHConfig structure. The length prefix with opaque body gives that by hiding the version-dependent ECHConfigContents bit.)

a deployment needs to have >1 HTTPS/SVCB RR value to publish more than one key at a time.

This is precisely what we want to avoid, and why we have the length prefix. Using separate HTTPS/SVCB RR values will result in either downgrade vulnerabilities on the client, or a lot of complexity. Keep in mind that DNS, connection setup, and TLS logic can come from different OS components or parts of the system. If we make the choice of HTTPS/SVCB RR depend on TLS capabilities, your DNS API needs to take in a description of your TLS stack's capabilities.

This would be excessive complexity in the way of deploying ECH, so we should keep the current, overall simpler design.

ekr commented 3 years ago

It's not redundant. It allows you to skip past ECHConfigs of unknown version, in, for instance ECHEncryptedExtensions.

sftcd commented 3 years ago

Ok, it's fine if this isn't redundant but then it being needed for ECHConflgList ought be explained IMO.

And maybe also mention that SVCB now contains an encoded ECHConfigList at the same time too.

ekr commented 3 years ago

I don't think we need to explain the rationale for every detail of how we define structures. It's not a security requirement someone needs to understand about how to implement this and the general need for this kind of length is just kind of community knowledge. If someone wanted to write that up in a TLS PDU style guide of some kind that would be fine, but it doesn't belong here.

sftcd commented 3 years ago

Fair enough that we disagree that this is unclear as-is and can usefully, simply and easily be improved, but that we don't have to do that:-)

The basic problem I think is that issues related to cardinality aren't clearly explained here nor in SVCB and I do think that'll bite some implementer sometime. I'm not fussed as to how exactly we improve that.

(And I'm not asking for every detail to be explained, I'm commenting while doing a sorta-fresh read through as per list-discussion yesterday.)

sftcd commented 3 years ago

Closed in favour of #496