[ech] APIs for ECH

Salz, Rich rsalz at akamai.com
Fri Feb 3 16:01:25 UTC 2023

Here's my comments on https://github.com/sftcd/openssl/blob/ECH-draft-13c/doc/designs/ech-api.md

Overall, I think this should use the openssl error stack and error codes.  I think you need to return -1 on error so that the client-side API's can say "I found no ECHConfig elements"

Whether or not I agree with them, I like the IETF editorial comments sprinkled throughout :)

> int ossl_ech_make_echconfig(
It's horrible that OSSL_HPKE_SUITE isn't a pointer, but that seems to be OpenSSL practice :(
https://github.com/openssl/openssl/issues/20199  I blame you Stephen.

> int SSL_CTX_ech_server_enable_file(
> int SSL_CTX_ech_server_enable_dir(
> int SSL_CTX_ech_server_enable_buffer(
These seem fine.

> int SSL_CTX_ech_server_get_key_status(
I think "get_num_keys" is a better name.

> int SSL_CTX_ech_server_flush_keys(
Since "age" is number of seconds, I think making this a time_t is confusing. Either make it be an absolute time or change the type to an unsigned int.

>int SSL_CTX_ech_raw_decrypt(
The inner_sni/outer_sni params don't match the outer_ch/inner_ch params and probably should. What's the memory allocation, openssl_malloc?  How do you free the results?
Is decrypted_ok needed, or can it just be done via a failed return, and then use openssl error stack for details? I think the most common failure is going to be decryption, do you agree?

> int SSL_ech_set1_echconfig(
> int SSL_ech_set1_svcb(
> int SSL_CTX_ech_set1_echconfig(
Do we need both forms, echconfig and svcb?  Maybe a utility to parse SVCB into echconfig separately?  If not, seems like an SSL_CTX_set1_echconfig is missing.

> int SSL_ech_set_server_names(
> int SSL_ech_set_outer_server_name(
> int SSL_CTX_ech_set_outer_alpn_protos(
I wonder if the ALPN list can be more user-friendly, like a list of strings.
In OSSL_ECH_INFO there's no length on outer_alpns/inner_alpns, but here in last function has a protos_len parameter.

>int SSL_ech_get_retry_config(
I assume this is returning a pointer into internal data; should it be get0 ?

> void OSSL_ECH_INFO_free(
> int OSSL_ECH_INFO_print(
> int SSL_ech_get_info(
> int SSL_ech_reduce(
What's the reduce function do?  Th OSSL_ECH_INFO should be opaque, and there should be accessors and a free function. David will point out that the "new"/ctor function should take all the params at once, and not let you lazy-set each one (unlike RSA keys :)

> int SSL_ech_set_grease_suite(
> int SSL_ech_set_grease_type(
Any reason not to have SSL_CTX versions of these?  And are both functions really needed?

>int SSL_ech_get_status(
Does this function go away if you use the error stack?  Maybe not, but maybe then it's "get0_sni_names" ?

>define SSL_ECH_STATUS_NOT_CONFIGURED -103 /* ECH wasn't configured */

Is this needed?  If you config with no-ech than builds that call any of those functions should fail.

> typedef unsigned int (*SSL_ech_cb_func)(SSL *s, const char *str);
I don't understand what the 'str' looks like, who generates and why?

> void SSL_ech_set_callback(
> void SSL_CTX_ech_set_callback(
Seem okay.

More information about the ech mailing list