add ech-api.md by sftcd · Pull Request #24738 · openssl/openssl

sftcd · 2024-06-26T11:57:27Z

Checklist

documentation is added or updated
tests are added or updated

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 · 2024-06-26T12:24:56Z

@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

doc/designs/ech-api.md

t8m · 2024-06-26T12:49:35Z

@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 · 2024-06-26T13:51:44Z

@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 · 2024-06-26T14:01:36Z

I'm happy to do whatever I'm told:-) Meanwhile, I can post to openssl/project#892 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 · 2024-06-26T15:05:13Z

@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 · 2024-06-27T15:47:54Z

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:

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,
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 · 2024-06-27T16:44:59Z

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.

t8m

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

doc/designs/ech-api.md

t8m · 2024-06-27T17:06:40Z

doc/designs/ech-api.md

+```c
+int SSL_CTX_ech_server_enable_file(SSL_CTX *ctx, const char *file,
+                                   int for_retry);
+int SSL_CTX_ech_server_enable_dir(SSL_CTX *ctx, int *loaded,
+                                  const char *echdir, int for_retry);
+int SSL_CTX_ech_server_enable_buffer(SSL_CTX *ctx, const unsigned char *buf,
+                                     const size_t blen, int for_retry);
+```


We have the concept of BIOs. Maybe (except the _dir variant) we could just have SSL_CTX_ech_server_enable_BIO?

That could certainly work (modulo the _dir() one differing as you say). It'd mean a bit more code in applications that use a char *filename configuration setting, or unsigned char* buffer parameter though so I guess the trade-off is a bit less library code vs. a bit more application code.

And for completeness (and truth in advertising:-), while I used both the _enable_dir() and _enable_buffer() flavours in the haproxy integration, in nginx, lighttpd and apache, even though the configuration setting can be a directory name, I ended up changing to use the __enable_file() variant (and doing the directory handling in application layer code) so that I could do better application layer logging. (Happy to provide pointers to those code fragments if useful.)

So, perhaps the choice is between changing to only a single _enable_BIO() flavour and perhaps adding another demo that shows how to use that to load a directory's worth of files? The simplification then does start to seem attractive tbh.

I've not done that yet btw as it'd take a bit of time to get the openssl and other builds back into whack, but happy to make the change if we conclude it's right. (And I'm leaning towards making the change now.)

Let's see what other reviewers think about this. @openssl/otc ?

I think we should support a BIO approach. This is consistent with how we work elsewhere and is the "OpenSSL" way to do things.

t8m · 2024-06-27T17:10:44Z

doc/designs/ech-api.md

+Note that ``SSL_CTX_ech_raw_decrypt()`` only takes a ClientHello as input. If
+the flight containing the ClientHello contains other messages (e.g. a
+ChangeCipherSuite or Early data), then the caller is responsible for
+disentangling those, and for assembling a new flight containing the inner
+ClientHello.


I think we should provide some more convenient API, i.e. a special TLS_method() that would do all this for the application. But I assume it could be something for future implementation.

Yes to that being for future study:-)

Implementing ECH split-mode is pretty odd tbh - it took quite a while to find the right place in the haproxy and nginx code-bases where that could be done. In both cases, the existing application code was essentially acting as a TCP-proxy or similar and wasn't using any TLS APIs so the above seemed to me to make sense. Happy to provide more detail now or later.

doc/designs/ech-api.md

sftcd · 2024-06-27T19:18:44Z

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 · 2024-06-27T19:46:41Z

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 · 2024-06-28T08:08:47Z

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.

mattcaswell

I really don't like all the passing around of raw binary data...

doc/designs/ech-api.md

demos/sslecho/echecho.c

sftcd · 2024-06-28T12:16:14Z

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.

doc/designs/ech-api.md

sftcd · 2024-07-02T23:21:29Z

I made a branch for the padding stuff mentioned above, i.e. `SSL_CTX_set_block_padding_ex()`` et al. I'll turn that into a PR (aimed at master) tomorrow (or so) but in case someone has comments meantime the commit is here

sftcd · 2024-07-04T17:15:32Z

Probably obvious, but the PR above is the padding one discussed here before:-)

FdaSilvaYY

👍

demos/sslecho/echecho.c

sftcd · 2024-07-04T18:30:40Z

commit handles today's comments from @FdaSilvaYY

sftcd · 2024-07-05T19:32:41Z

@mattcaswell - I think you had two main problems with the current APIs, those being making changes to SSL_CTX values as ECH keys are updated and use of raw buffers in APIs. Does the following look like it'd address your comments?

/* New ECHStore APIs */
typedef struct ech_store_st ECHStore;

ECHStore *ECHStore_init(OSSL_LIB_CTX *libctx, const char *propq);
void ECHStore_free(ECHStore *es);
int ECHStore_new_config(ECHStore *es, uint16_t echversion, uint16_t max_name_length,
                        const char *public_name, OSSL_HPKE_SUITE suite);
int ECHStore_make_pemech(ECHStore *es, BIO *out);

int ECHStore_set1_echconfiglist(ECHStore *es, BIO *in);

int ECHStore_set1_key_and_list(ECHStore *es, EVP_PKEY *priv, BIO *in,
                               int for_retry);
int ECHStore_set1_pemech(ECHStore *es, BIO *in, int for_retry);

int ECHStore_get_info(ECHStore *es, OSSL_ECH_INFO **info, int *count);
int ECHStore_downselect(ECHStore *es, int index);

int SSL_CTX_ech_server_enable(SSL_CTX *ctx, ECHStore *es);

int SSL_CTX_set1_echstore(SSL_CTX *ctx, ECHStore *es);
int SSL_set1_echstore(SSL *s, ECHStore *es);

You'd have to read that in conjunction with the current design doc, but if needed I can add more description.

If that looks in the ballpark, I can re-do the ech-api.md and this PR along those lines as I try get those APIs working on top of my code.

(Edited to reflect a 2nd pass at those, still not yet started to code 'em up.)

sftcd · 2024-07-10T10:37:24Z

I did a bit of prototyping of the ECHStore stuff above - I have a branch where the _init() _free(), _new_config() and _make_pemech() functions work, and it looks quite feasible, but a pile of work, to head that direction, if it's the right direction. (The relevant snippet of code for the openssl ech command line is here.)

So, opinions on whether that's a good direction welcome! If it is, I can update the ech-api.md file accordingly, then we can move on from there. If something else is better, it'd be very good to know that now:-)

and includes a minimal demo for some of those APIs. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#24738)