search

openssl / openssl

TLS/SSL and crypto library
https://www.openssl.org
Apache License 2.0
24.84k stars 9.92k forks source link

add ech-api.md #24738

Open sftcd opened 3 days ago

sftcd commented 3 days ago
Checklist

This is an initial PR to a) check I'm following the right process, and b) include the ECH-API design doc as a possibly useful 1st thing to add to the feature branch.

nhorman commented 3 days ago

@sftcd Thank you for working on this, I've created a milestone for you (Encrypted Client Hello). Please add any PR/Issues you create in pursuit of this feature to that Milestone

t8m commented 2 days ago

@nhorman External contributors AFAIK cannot change Milestone on their issues/PRs. However we could create a new group with Triage rights for the main contributors on external feature branches.

nhorman commented 2 days ago

@t8m thanks for the note. Github is so frustrating sometimes. I like the idea of elevating external contributors such that they can modify issues, they are working on. I'll create a task for that

sftcd commented 2 days ago

I'm happy to do whatever I'm told:-) Meanwhile, I can post to https://github.com/openssl/private/issues/528 whenever I make a new PR for the feature branch. (Probably best to fully process this one first though, before starting a 2nd, so I'll hold off on making new PRs for a wee while.)

t8m commented 2 days ago

@t8m thanks for the note. Github is so frustrating sometimes. I like the idea of elevating external contributors such that they can modify issues, they are working on. I'll create a task for that

Yeah, unfortunately the GH privileges are not so fine grained so we could assign them only such privilege that they could change labels/milestones on any issue in the repo. IMO this is not a serious problem as we would give this privilege only to more involved contributors which we would trust not to misuse it and if they misused it the damage would not be so serious and the privileges could be easily revoked from them.

sftcd commented 1 day ago

So... back to the content of this PR: what's our modus-operandi for merging it into the feature branch? I can see two reasonable possibilities:

1) we treat the API description as a work-in-progress that describes the current implementation and that will change as that gets reviewed in subsequent PRs to the feature branch, or,

2) we aim to get sufficient review so that people are confident that the APIs etc. described are (close to) where we want to end up.

(1) probably means a lower barrier to merging this PR, and seems (to me, but it would, wouldn't it:-) good enough for now, but I'd be happy if people had time to do more substantive review of the text.

I'm also not quite clear on how many approvers are needed for a merge - IIUC, for the master branch that needs approval from two maintainers but is the same true for the feature branch?

(Meanwhile, after this PR is merged to the feature branch, I think the next one would likely be build artefacts and some stub functions so also not very substantive - I assume it makes sense to keep 'em small and simple for as long as we can:-)

t8m commented 1 day ago

My preference would be to get some rough reviews of the API by multiple people to see if the basic concepts are sound. We can fine-tune the design as the implementation goes in.

I'm also not quite clear on how many approvers are needed for a merge - IIUC, for the master branch that needs approval from two maintainers but is the same true for the feature branch?

Yes, the rules are the same on feature branches as on any other branches.

sftcd commented 1 day ago

I do not see any major issues with the proposal. There are some nits.

Thanks! I've made the obvious changes, pushed those and marked the comments resolved (if you prefer to click that "resolved" button that's fine by me). Have some questions on others above though might take another round of chat...

sftcd commented 1 day ago

Sorry, there's a thing implicit in the above that might be worth calling out now for reviewers. (We can change minds later of course but it nearly came up in @t8m's comment...)

The input format for e.g. SSL_CTX_ech_server_enable_file() and co. is the PEM format defined in this internet-draft. That's a personal I-D and is not a TLS WG work item. And neither boringssl nor NSS make use of that format - they require the ECHConfigList and private value to be handled separately. (I think, but not sure, that wolfssl and the recent ECH work on rusttls do simlarly.)

I'd argue the PEM format with both public and private values in one structure is a significantly better option for OpenSSL though as it's much better suited for servers where handling the ECHConfigList and private value as one thing make much better sense. In contrast, I think all the other implementations mentioned above are much more focused on TLS clients.

Just wanted to call that out, as while it's mentioned in the ECH API doc, not all of the above might be apparent.

t8m commented 1 day ago

The input format for e.g. SSL_CTX_ech_server_enable_file() and co.

I do not think this format must be standardized (at least for now). And yeah, it makes sense to include both public and private counterparts for server configuration. In fact, if we provided a BIO based function you could easily make the application to provide both parts from separate files if it needs so. Of course the "from memory" function achieves that as well but the BIO one would be more convenient.

sftcd commented 1 day ago

I made the obvious changes in the most recent commit, will give it a few hours before marking anything as resolved and then await feedback on the non-obvious stuff.

I think making the change to a BIO approach for loading server keys is probably right, so I plan to do that but it may take a day or two as I want to test corresponding changes in the server integrations we've done.