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

Including the client's ephemeral public key in the AAD precludes use of a single-shot HPKE API #397

Closed
sftcd opened this issue Mar 10, 2021 · 13 comments
Labels
design

Comments

@sftcd
Copy link
Collaborator

@sftcd sftcd commented Mar 10, 2021

This isn't critical but I think it might be better to allow use of a single-shot HPKE API that doesn't need to first and separately generate a public/private pair before calling Seal(). The downside of forcing two HPKE calls is perhaps mostly that someone will be tempted to re-use that key pair for something else or a 2nd attempt with maybe bad effects. If someone ever did HPKE in h/w there might also be a security difference.

I'm also not entirely sure what we're protecting by including that public value ("enc") in the AAD as well so that could be better documented at least, if we keep things as-is, but perhaps including that field is really a bit of overkill?

@davidben
Copy link
Collaborator

@davidben davidben commented Mar 11, 2021

A single-shot HPKE already isn't viable because of HRR. You need to keep the second HPKE context around in case you need to encrypt second ClientHello.

That said, I'm perfectly happy with omitting it. (Or including it.) This dates to #362 and #326. Of the added fields, cipher_suite and enc are already incorporated into HPKE itself anyway, so we shouldn't need to redo it in ECH.

That leaves config_id, which is... almost redundant. It's part of ECHConfig which is already authenticated, except in the optional-config_id mode, where it is otherwise unauthenticated. But it's also ignored in that mode so... ¯\_(ツ)_/¯

@sftcd
Copy link
Collaborator Author

@sftcd sftcd commented Mar 11, 2021

@chris-wood
Copy link
Collaborator

@chris-wood chris-wood commented Mar 23, 2021

That said, I'm perfectly happy with omitting it. (Or including it.)

@davidben what is "it" here? The AAD? That seems orthogonal to whether or not the HPKE context (and shared KEM secret) are reused for CH2.

@davidben
Copy link
Collaborator

@davidben davidben commented Mar 23, 2021

Sorry, yeah, that was unclear. By "it" I mean enc in AAD.

@cjpatton
Copy link
Contributor

@cjpatton cjpatton commented Mar 23, 2021

The reason to authenticate these things was to be as conservative as possible. Of course, we don't have any formal analysis that tells us whether or not it's strictly necessary. But given that single-shot HPKE isn't viable in this application, I'd suggest we keep it as-is. (Unless another reason to change this comes along :)

@chris-wood
Copy link
Collaborator

@chris-wood chris-wood commented Mar 30, 2021

@sftcd given that this is needed for HRR to avoid two KEM operations, are you comfortable closing this?

@sftcd
Copy link
Collaborator Author

@sftcd sftcd commented Mar 30, 2021

@davidben
Copy link
Collaborator

@davidben davidben commented Mar 30, 2021

Reusing the context is not just about performance. It also avoids a host of complexity and weird corner cases when ECH decryptions are RPC calls. It also reduces server complexity by not needing to check for the client picking a different ECHConfig the second time, etc. I would oppose regressing those complexity improvements.

@sftcd
Copy link
Collaborator Author

@sftcd sftcd commented Mar 30, 2021

@davidben
Copy link
Collaborator

@davidben davidben commented Mar 31, 2021

#352 is the PR that switched it, by the way. Also some discussion in #349.

@chris-wood chris-wood added the design label Apr 21, 2021
@chris-wood
Copy link
Collaborator

@chris-wood chris-wood commented Jun 11, 2021

@sftcd, now that #423 is merged, which reuses the HPKE context for HRR, which accepts that HRR must be dealt with in the protocol, along with @davidben's comment, I'd like to propose closing this. Any objections?

@sftcd
Copy link
Collaborator Author

@sftcd sftcd commented Jun 11, 2021

@chris-wood
Copy link
Collaborator

@chris-wood chris-wood commented Jun 11, 2021

Sounds good -- thanks!

@chris-wood chris-wood closed this Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design
Projects
None yet
Development

No branches or pull requests

4 participants