refactor: remove legacy pkey impls #5241
Conversation
lrstewart
force-pushed
the
openssl3fips_evp_3c_3
branch
from
1ce54d2 to
fbf962c
Compare
lrstewart
force-pushed
the
openssl3fips_evp_3c_3
branch
from
fbf962c to
13689db
Compare
| #include "error/s2n_errno.h" | ||
| #include "utils/s2n_safety.h" |
There was a problem hiding this comment.
These headers were inherited from s2n_pkey.h before.
| server_conn->handshake_params.client_public_key.key.rsa_key.rsa = public_key.key.rsa_key.rsa; | ||
| server_conn->handshake_params.client_public_key.pkey = public_key.pkey; |
There was a problem hiding this comment.
This wouldn't have been working for a while, even if it compiled. The fuzz test would have always trivially failed. I'm also not sure it's not still trivially failing for another reason. I just got it compiling again.
We badly need to revisit our fuzz tests.
There was a problem hiding this comment.
Time to look at the fuzz test coverage report?
| #ifndef EVP_PKEY_RSA_PSS | ||
| #define EVP_PKEY_RSA_PSS EVP_PKEY_NONE | ||
| #endif |
There was a problem hiding this comment.
I moved this out of s2n_rsa_pss.h because rather alarmingly, EVP_PKEY_RSA_PSS is defined in openssl/evp.h and s2n_rsa_pss.h wasn't actually including openssl/evp.h. So EVP_PKEY_RSA_PSS only wasn't always EVP_PKEY_NONE because another included header happened to include openssl/evp.h.
This highlights the issue with #ifdefs in general, but in this particular case it's at least safer to do this in this file bc this file actually uses EVP_PKEY_RSA_PSS, so also requires other symbols from openssl/evp.h. We're less likely to just remove the openssl/evp.h include.
Release Summary:
Resolved issues:
resolves #5149
Description of changes:
After eca2a8e and ec05a02, the EVP pkey implementation has complete parity with the legacy pkey implementations. This PR therefore removes all the legacy pkey implementations and completely replaces them with the evp pkey implementation. It's mostly deletes, renames, fixing includes, and swapping s2n_pkey_evp_init in for s2n_rsa_pkey_init/s2n_ecdsa_pkey_init/s2n_rsa_pss_pkey_init.
Callouts
It's a lot of changed files, even if most of the changes are trivial (half the files are just include changes). Let me know if you don't feel confident reviewing it, and I'll figure out how to break it up into smaller chunks.
Testing:
In the previous PRs I added pkey tests for size/encrypt/decrypt/match that were implementation agnostic, and in this PR I swapped the sign/verify tests to be implementation agnostic too. We shouldn't have to write a new pkey test for every new implementation.
Previously both the encrypt/decrypt and sign/verify tests also included checks that the hard-coded known values worked with the legacy methods, so since I haven't changed the hard-coded known values we retain that guarantee even tho I've removed the legacy methods.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.