add ech-api.md

[sftcd]

Checklist

• [x] 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]

@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]

@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]

@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]

I'm happy to do whatever I'm told:-) Meanwhile, I can post to https://github.com/openssl/project/issues/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]

@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]

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]

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]

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]

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]

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]

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.

[sftcd]

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]

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

[sftcd]

commit handles today's comments from @FdaSilvaYY

[sftcd]

@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]

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:-)

[mattcaswell]

I think the concept of the ECHStore, broadly in line with what you are proposing, is the right way to go. I would call it OSSL_ECHSTORE instead though to avoid namespace issues.

I really think we need similar object for config and config lists, e.g. OSSL_ECHCONFIG, OSSL_ECHCONFIGLIST instead of the BIOs that you are currently using (there might be functions to create an OSSL_ECHCONFIG or OSSL_ECHCONFIGLIST from a BIO)

[sftcd]

I think the concept of the ECHStore, broadly in line with what you are proposing, is the right way to go. I would call it OSSL_ECHSTORE instead though to avoid namespace issues.

Ack, and thanks. Will proceed on that basis.

I really think we need similar object for config and config lists, e.g. OSSL_ECHCONFIG, OSSL_ECHCONFIGLIST instead of the BIOs that you are currently using (there might be functions to create an OSSL_ECHCONFIG or OSSL_ECHCONFIGLIST from a BIO)

I'd argue we can do without those being externally visible, and that making such fine-grained details visible to applications might later be a problem if/when new versions of ECH are developed. The idea with the OSSL_ECH_INFO was to provide just enough information for an application to use without the application having to know about how those are structured.

I also found that it's useful to internally normalise the set of ECHConfigList values e.g. a client might present (from separate HTTPS RR values) into a set of "singleton" ECHConfig values, that'd maybe become harder if the application has a view of which public keys/names are in which structures. (And I don't think applications need to care.)

How about going with OSSL_ECHSTORE for now as an opaque to the application structure and seeing if we really need to expose more internal details later?

[mattcaswell]

We don't need to expose any internal structure or fields of an ECHConfig or ECHConfigList. They can be entirely opaque with constructors for various sources (e.g. mem buffer or BIO) and a destructor. Possibly also functions for adding/removing ECHConfig from an ECHConfigList if we feel it is necessary.

[sftcd]

We don't need to expose any internal structure or fields of an ECHConfig or ECHConfigList.

Ah, good.

They can be entirely opaque with constructors for various sources (e.g. mem buffer or BIO) and a destructor. Possibly also functions for adding/removing ECHConfig from an ECHConfigList if we feel it is necessary.

So I don't see that any application ever needs to know about an ECHConfig - only ECHConfigList values are processed by applications, and pretty much always in base64 encoded form.

I'm not understanding why it'd be better for the application to have an opaque type for ECHConfigList - what kind of thing did you have in mind?

[sftcd]

So I don't see that any application ever needs to know about an ECHConfig

I guess, to be fair, if one wanted to offer a rich API for handling extensions then one might want that. I'd argue that it'd be better to not try offer a rich API for ECHConfig extensions though. There are none defined as of now, and afaik, no other code base supports adding them. (Can check if that's changed though, been a while since I looked.) Personally, I regard ECHConfig extensions as a bit of bad design and the most I'd suggest doing for 'em is being able to fail because of or skip over 'em, if present (which the library implementation handles without needing an API) - and enabling some way to create test cases.

[mattcaswell]

By passing the raw bytes around you have to parse it every time. Using an object you can parse it once, and just pass that object around from then on. It also gives us type safety and extensibility. See my earlier comment on this:

https://github.com/openssl/openssl/pull/24738#discussion_r1658751555

[sftcd]

By passing the raw bytes around you have to parse it every time. Using an object you can parse it once, and just pass that object around from then on. It also gives us type safety and extensibility. See my earlier comment on this:

#24738 (comment)

Sure, I get the general point. OTOH, I can't think of any case where it'd be necessary to parse an encoded ECHConfigList more than once. Those are just ingested by clients. For servers, those only need to ingest a key+ECHConfigList (in the PEM format, or separately).

So ISTM to come down to whether or not we want to provide rich APIs for ECHConfig extension handling.

But maybe I'm missing something? (Could well be:-)

Also - not saying "I won't do it" btw, just want to understand better what benefit accrues.

[sftcd]

Just checking in case we're talking past one another: an ECHConfigList is not like an X.509 certification path - there's no chaining and the list is just a set of independent ECHConfig values. (Sorry if that's obvious.)

[mattcaswell]

I don't think we can know how applications will use these APIs, nor how they will evolve over time, whether we will want to enable access to the initially opaque data within these objects at some point in the future, or whether the extensions feature will turn out to be useful after all. It just seems to me that having these things as their own independent objects gives us benefits with very little in the way of downsides.

I'd be interested in the views of other reviewers on this point.

Does an ECConfigList have any other data associated with it, besides the list of constituent ECHConfig items? If not we could possibly get away with just an OSSL_ECHCONFIG object and pass around a STACK_OF(OSSL_ECHCONFIG) in contexts where a list is needed (meaning that we would only need one extra object instead of two).

[sftcd]

I don't think we can know how applications will use these APIs, nor how they will evolve over time, whether we will want to enable access to the initially opaque data within these objects at some point in the future, or whether the extensions feature will turn out to be useful after all. It just seems to me that having these things as their own independent objects gives us benefits with very little in the way of downsides.

The main downside I'd see is added code to enable ECH that' s just overhead and that's harder for developers to understand. I'm also wary of adding code for handling ECHConfig extensions that nobody uses - always seems to be where bugs end up:-)

In terms of what applications want/need, I can say that the PoC integrations we've done with curl, apache, nginx, lighttpd and haproxy did work fine with the APIs proposed. (And will also be fine with OSSL_ECHSTORE as proposed.) There are no cases of parsing an ECHConfigList twice in any of those, nor can I see why that'd be needed.

I double-checked a few mins ago and neither boringssl nor wolfssl have any API support for ECHConfig extensions. (Both also take buffers as inputs for clients:-) The boringssl server stuff does look more like where we'll end up with OSSL_ECHSTORE though.

I'd be interested in the views of other reviewers on this point.

Yep, more eyeballs are always good.

Does an ECConfigList have any other data associated with it, besides the list of constituent ECHConfig items? If not we could possibly get away with just an OSSL_ECHCONFIG object and pass around a STACK_OF(OSSL_ECHCONFIG) in contexts where a list is needed (meaning that we would only need one extra object instead of two).

There is nothing in an ECHConfigList except an overall length and a set of ECHConfig values. The definition is here. Protocol-wise, a bare ECHConfig is never passed around, only encoded ECHConfigList values. (When one has only one key, then a "singleton" ECHConfigList is what's used.)

Internally, (in my last night's prototype), I do have a STACK_OF(OSSL_ECHSTORE_entry) - OSSL_ECHSTORE_entry is currently just internal but does have more than the fields from an ECHConfig though, as we e.g. want to keep the associated private value (as an EVP_PKEY) for when the server has that. It's also useful to keep the binary encoded ECHConfig value for use when actually doing ECH (it's fed into the HPKE crypto operations). There's a bit more meta-data too, e.g. whether to send in a retry-config, the relevant filename (if any) and the time loaded, which are useful when updating keys, esp. in haproxy. So far, the only things in an OSSL_ECHSTORE aside from the entries are the libctx and propq values. (You can see that here - bear in mind that's a bit of a mish-mash for now, so don't look elsewhere in the file yet:-)

Maybe the best next step is I'll update ech-api.md with what I have now and that might make the discussion easier. I'll also ponder this discussion while doing that. Probably tomorrow before that's done.

[sftcd]

So, based on yesterday's discussions I've pushed a new version of the API design and the demo code.

I'd say maybe best to read the API doc from scratch as I've re-ordered things some and made a bunch of changes. The new demo code compiles (in another branch) but hasn't been tested of course as actually getting the OSSL_ECHSTORE based stuff to work will be... work:-) And better to do that work after we seem happy with the API doc.

[sftcd]

ping! I've travel next week so could make progress on that OSSL_ECHSTORE stuff if it looks like the the right direction. Is it?

Also - I hope we're not expecting the result of this discussion to be the exact final design doc text (I bet a beverage something will change as we go:-) so hopefully getting this merged and moving on shouldn't be too big a barrier if we're heading the right way.

(BTW - while I slightly prefer the original APIs, I'm fine with doing the OSSL_ECHSTORE thing partly as it'll force me to refactor the internal data structures some - that's likely a good thing given those evolved as the protocol was changing over the last few years.)

[sftcd]

ping again:-) feedback welcome!

[sftcd]

Thanks for the comments, will process those in a few hours...