[ech] APIs for ECH
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.
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.
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?
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(
More information about the ech