sys/crypto/openssl/ossl.c
180–183	It seems like we have an extra nest level here. csp_auth_key != NULL should be true IFF csp_auth_klen != 0, and vice versa? (During newsession.) Nevermind, I guess we could be creating a keyed-hash session without any per-session key.
185–194	The implication is that hash functions with a Setkey operation cannot be HMACs? That's probably fine.
231–232	Why move opad initialization below instead of just doing it here? Avoid a copy?
sys/crypto/openssl/ossl.h
41	this feels brittle, although it's not a regression in this diff
sys/crypto/openssl/ossl_poly1305.c
7	What portions of this file are direct from openssl and what portions are novel?

jhb marked 2 inline comments as done.Mar 2 2021, 10:47 PM

jhb added inline comments.

sys/crypto/openssl/ossl.c
185–194	The implication is that hash functions with a Setkey operation cannot be HMACs? That's probably fine. Correct.
231–232	Why move opad initialization below instead of just doing it here? Avoid a copy? It overwrites the single `ctx`. Basically, we have a single auth `ctx` on the stack. We either copy it from a saved context when using session keys, or we generate the context on the fly when using per-op keys. Moving opad here would mean having to store two copies on the stack.
sys/crypto/openssl/ossl.h
41	this feels brittle, although it's not a regression in this diff There are static assertions in each of the auth hash files that the context is big enough. The alternative of trying to include the relevant headers is a bit messy.
sys/crypto/openssl/ossl_poly1305.c
144	From here down is new, the #includes are new, and the explicit #define is new, the rest is from OpenSSL.

cem accepted this revision.Mar 3 2021, 5:11 AM

cem added inline comments.

sys/crypto/openssl/ossl.c
231–232	Ok
sys/crypto/openssl/ossl.h
41	Yeah, that's reasonable. Is uint32 aligned enough for all algorithms and architectures?
sys/crypto/openssl/ossl_poly1305.c
144	Thanks!
150–151	Would it make sense to assert klen matches openssl's expectations?

This revision is now accepted and ready to land.Mar 3 2021, 5:11 AM

jhb marked 4 inline comments as done.Mar 3 2021, 10:51 PM

jhb added inline comments.

sys/crypto/openssl/ossl.h
41	Yeah, that's reasonable. Is uint32 aligned enough for all algorithms and architectures? It's actually 32 byte alignment for AVX and is sufficient for everything currently.
sys/crypto/openssl/ossl_poly1305.c
150–151	Would it make sense to assert klen matches openssl's expectations? I can add that.