[ech] APIs for ECH

Fri Feb 3 22:20:47 UTC 2023

Hi Rich,

Thanks for taking the time to comment...

On 03/02/2023 16:01, Salz, Rich wrote:
> 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.  

Sorry, not sure what you mean?

> I think you need to return -1 on error so that the client-side API's can say "I found no ECHConfig elements"

That's done via the ``int *num_echs`` output which'll be zero
valued on return in such cases. That can happen without
hitting an error if e.g. an HTTPS RR exists but contains no
ECHconfiglist. (I added a note to that effect.)

Or, did you mean something else?

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

You're welcome:-)

> 
>> 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.

Yep, mean culpa. I still think it better but I guess since
the HPKE stuff's not yet part of a release, that could be
revisited if everyone else hates it.

> 
>> 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.

I'd have no problem changing to that. Any other opinions?

> 
>> 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.

Good point. Will make that change shortly.

> 
>> 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?

It's caller allocated. I added a note (that was more or
less already in the POD file):

"The caller allocates the ``inner_ch`` buffer, on input
``inner_len`` should contain the size of the ``inner_ch``
buffer, on output the size of the actual inner CH. Note
that, when ECH decryption succeeds, the inner CH will
always be smaller than the outer CH."

> 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?

If there's no ECH present in the outer then this will
return 1/succeed but ``decrypted_ok`` will be zero. Same
if a GREASE'd ECH is found or decryption fails for some
other (indistinguishable) reason.

I added a note saying the above.

> 
>> 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?  

Fair point - one can argue these functions are doing a
little bit too much. I'd be fine with splitting things
as you suggest and actually now I think about it, it's
a good suggestion - it should take any heuristic format
guessing out of the mainstream and make for more useful
test code, so I'll look into doing that unless someone
thinks it's better as-is. (I need to chat to the person
who's looking at curl first to see what we think might
make longer term sense for that.)

And that'd also reduce the diff between these APIs and
boring too, which I guess is a positive.

> If not, seems like an SSL_CTX_set1_echconfig is missing

I guess you mean SSL_CTX_set1_scvb?

I used to have that and it'd be trivial to add back, but
turns out I didn't need it for any of my PoC implementations
so I took it out. But based on your suggestion above,
that'd go away too I guess.

> 
>> 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.

It could be better yes, but I based it on the existing API
for setting ALPNs. I'd be fine with changing both if that's
better but stayed away from changing existing APIs so far,
which is probably for the best?

> In OSSL_ECH_INFO there's no length on outer_alpns/inner_alpns, but here in last function has a protos_len parameter.

True. As OSSL_ECH_INFO is new, I figured it was ok to be
nicer to the coder with that and present a comma-sep list
of strings.

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

No, it's the same model where the caller allocates the
buffer, provides buffer size on input and gets actual
size on output. I figured that was ok as retry_config
sizes should almost always be <1k. I guess I could make
a change to fail and return the size-needed if the
input buffer is too small (or a NULL) though?

> 
>> 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?  

That's how you downselect from having e.g. 4 ECHConfig values
to the one that the caller prefers (e.g. that matches the
desired public_name or IP address). I added a note to that
effect.

> Th OSSL_ECH_INFO should be opaque, and there should be accessors and a free function.

There is a free function:-) But yeah, it's not opaque and so
I didn't add accessor functions. Could do that though if it's
better.

> 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?  

No particular reason, no, but not sure they'd be
needed

> And are both functions really needed?

I think yes, if we hit a scenario where clients want to
GREASE using an ECH extension type (and/or HPKE suite)
for which we don't have "real" ECH support, which I think
may be desirable in some transition scenarios.

> 
>> 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" ?

Not sure I get the question (similar to your first comment I
guess).

> 
>> 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.

Maybe I need to change the name? You'd get that if a server
supports ECH but has no keys or a client hasn't set any
ECHConfig values.

> 
>> 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?

It could probably be dropped. I populate a string that can be
logged for debugging purposes, which has been useful for me,
but maybe less useful when this stuff's done. The example
below is in the POD file:

"
An example string I<str> as seen on a client might be:

  ech_attempted=1
  ech_attempted_type=0xfe0d
  ech_atttempted_cid=0x5d
  ech_done=1
  ech_grease=0
  ech_returned_len=0
  ech_backend=0
  ech_success=1
  2 ECHConfig values loaded
  cfg(0): 
[fe0d,5d,cover.defo.ie,0020,[0001,0001],190984309c1a24cb944c005eb79d9c72ca9a4a979194b553dfd0bffc6b5c152d,00,00]
  cfg(1): 
[fe0d,fd,cover.defo.ie,0020,[0001,0001],46dd4e2c81bb15ef9d194c99b86983844e2a1387e4fb7e7d3b8d368c8e1b4d2a,00,00]
"

> 
>> void SSL_ech_set_callback(
>> void SSL_CTX_ech_set_callback(
> Seem okay.

Great! :-)

Thanks for the comments and suggestions. I made the small
changes to the markdown file already and will look at the
client API changes over the w/e.

Cheers,
S.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xE4D8E9F997A833DD.asc
Type: application/pgp-keys
Size: 1197 bytes
Desc: OpenPGP public key
URL: <https://mta.openssl.org/pipermail/ech/attachments/20230203/b73b60fd/attachment-0001.asc>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 236 bytes
Desc: OpenPGP digital signature
URL: <https://mta.openssl.org/pipermail/ech/attachments/20230203/b73b60fd/attachment-0001.sig>