[ech] APIs for ECH

[Salz, Rich]

(Removed things where we agree)

Sorry for the poor quoting/formatting.  We use outlook.
 
> Overall, I think this should use the openssl error stack and error codes. 

>Sorry, not sure what you mean?

The high-level point is that when an error happens, your code should all ERR_raise.  Being able to do that could mean either adding another error component or using an existing one.  See crypto/err/openssl.ec, perhaps, as a starting point. You'll have to ask the project folks which way they want to go.

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

Good.

>> int ossl_ech_make_echconfig(
> It's horrible that OSSL_HPKE_SUITE isn't a pointer, but that seems to be OpenSSL practice :(

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

On the one hand it seems silly to have a pointer, and ctor functions for three values that are specified in an RFC. On the other hand, future-proofing.  I guess I'm torn.

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

Cool.

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

You didn't mention the param order.

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

So if there isn't enough space, you call ERR_raise() to indicate "need more space" and return -1 ?  Or do you want to work like the d2i functions, allowing. NULL buffer points and the size needed is filled in?  That might be more work then desired.

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

ok

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

Yes.

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

Yes, better to be consistent.

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

I think it's better to be ...

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

If you can do that, like I mentioned above, sure.

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

Maybe SSL_ech_find_match ?

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

Did I explain it better?  The idea is that calling code all looks like the rest of openssl code
	If (ECH-func_fails() < 0) {
		Int error = ERR_get_status(....)
		Do something...
	}
Make sense?

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

Ah, maybe ECH_NOT_FOUND ?


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

Shrug. No strong feelings.  DDvO might tho :)