openssl backends for all remaining KDFs

[frozencemetery]

Starting in 3.0, openssl will support a new KDF interface. We have four KDF families implemented within krb5 currently:

1. RFC 8636's id-pkinit-kdf is a specific case of SSKDF, which has been implemented already in openssl. This PR adds support for its use; Add SSKDF test vectors from RFC 8636 openssl/openssl#9957 adds the RFC's vectors to openssl.

2. RFC 8009's unnamed KDF (for aes-sha2) has been proposed for inclusion as [KDF] Add KBKDF implementation for counter-mode HMAC openssl/openssl#9924 and been given the name kbkdf (by NIST); I have a proof-of-concept branch at https://github.com/frozencemetery/krb5/tree/800-108test Update: this has merged and been added on to this pull request.

3. RFC 3961's unnamed KDF (for 3DES and aes-sha1) has been proposed for inclusion as Add KRB5KDF from RFC 3961 openssl/openssl#9949 tentatively named krb5kdf; I have a proof-of-concept branch at https://github.com/frozencemetery/krb5/tree/3961test Update: this has merged and been added on to this pull request.

4. RFC 3962's KDF (for camellia) has not yet been proposed in openssl. It would be another flavor of kbkdf. has been proposed for inclusion as part of kbkdf as KBKDF Feedback mode + CMAC support openssl/openssl#10143 . I have reused the proof-of-concept branch at https://github.com/frozencemetery/krb5/tree/800-108test Update: this has merged and been added on to this pull request.

I'm proposing this PR before openssl 3.0 releases in order to get early feedback. Also out of laziness: if krb5 doesn't want to merge this, then the patches I use downstream are just the proofs-of-concept above. I definitely don't expect krb5 to merge it before openssl 3.0 releases. When the other KDFs are merged / released, I can open separate PRs for them (or just tack them on here).

[frozencemetery]

Added support for OpenSSL's KBKDF (SP800-108) and rebased.

[frozencemetery]

Added Camellia and rebased onto master.

[frozencemetery]

Added krb5kdf and rebased onto master.

[frozencemetery]

This is now complete and presentable; review would be appreciated.

I wanted to have the KDFs follow the builtin/openssl distinction, but that's not clearly possible since we still have to contend with "pre-3.0 openssl". Of course, we could pull up the ladder on that, but given we just patched for glibc < 2.24, I don't imagine you want to go that case :)

It would be still possible to move the KDFs into their own file within the krb directory, but I'm not sure that gains us anything.

[greghudson]

This is a really surface-level review; I only flagged one issue and two style nits so far.

[frozencemetery]

Switched to AC_CHECK_FUNCS and rebased on master.

[greghudson]

The EVP KDF interface seems cumbersome. I feel like they could have just made a function per KDF type (like EVP_sha1(), but probably with a destructor) and given each function the necessary parameters for the KDF. The length of "OSSL_PARAM_construct_octet_string" and similar leads to a lot of line breaking. This is of course not in our control.

Sam considers the pattern ret = SOME_ERROR; do a bunch of stuff and maybe goto cleanup; ret = 0; cleanup: free stuff; return ret to be dangerous (see https://krbdev.mit.edu/rt/Ticket/Display.html?id=6959 ) because it would be easy to insert code which sets ret to 0 on success. openssl_sskdf() uses this pattern. Better would be ret = oerr(..., KRB5_CRYPTO_INTERNAL, ...);" even though that requires an extra line in most exception cases.

Looking up the digest name by output length in openssl_sskdf() is inelegant and not especially future-proof (e.g. there could be SHA-3 variants with the same lengths as SHA-256 and SHA-512). Perhaps it would be better to return the digest name in pkinit_alg_values() and supply it as a parameter to openssl_sskdf().

The previous two problems also apply to openssl_kbkdf_feedback_cmac() and openssl_krb5kdf(), but the solutions are less clear.

I would probably use "ret" instead of "retval" in pkinit_alg_agility_kdf() because the code is being churned anyway. (The counterargument is that the whole file uses "retval" aside from the new openssl_kdf(), but I think function-by-function migration towards "ret" is okay.)

Why put a k5_ prefix on the static derive_random_() function names?

[frozencemetery]

Agreed about the interface. OpenSSL's style allows for a line break before the first argument to a function, which means the pain is felt slightly less. Additionally, OSSL_PARAM_construct_utf8_string() requires that the string not be constant (i.e., OSSL_PARAM OSSL_PARAM_construct_utf8_string(const char *key, char *buf, size_t bsize);, so that gets gross fast too.

Rebased onto master. Removed problematic pattern. Moved digest string generation into pkinit_alg_values(). Migrated to ret.

I believe the reason for the k5_ prefix was consistency with k5_sp800_108_counter_hmac(); I've dropped it in this update though.

Without modifying the struct krb5_enc_provider, I don't have a good solution for the enc name lookup issue. Updated with a different approach using memcmp(), but this relies on the assumption that all instances of const struct krb5_enc_provider * are those found in crypto_int.h - this seems safe but also raises future-proofing concerns to me.

[kaduk]

Additionally, OSSL_PARAM_construct_utf8_string() requires that the string not be constant (i.e., OSSL_PARAM OSSL_PARAM_construct_utf8_string(const char *key, char *buf, size_t bsize);, so that gets gross fast too

I think it's not too late to complain to openssl about that (I don't think adding const would cause problems, off the top of my head)

[frozencemetery]

It's beyond what I can fix, but reported as openssl/openssl#10658

[frozencemetery]

Rebased on master and papered over a coverity issue around dereferencing hash in counter_hmac.

[frozencemetery]

Updated for the EVP_KDF_derive() interface change. Will check with openssl folks about the RSA change.

[greghudson]

I think it would be good to split commit "Fix softpkcs11 build issues with openssl 3.0" out into a separate PR. Merging it would allow OpenSSL to re-enable PKINIT in the krb5 tests part of its CI.

[frozencemetery]

Agreed. The softpkcs11 part doesn't work yet - I pushed it to share and forgot that would update the PR.

[frozencemetery]

softpkcs11-only: #1187

[frozencemetery]

(Probably should have done this a while ago, but finally made a separate branch for the non-KDF OpenSSL-3 relevant things: https://github.com/frozencemetery/krb5/tree/ossl3)