refactor: use EVP_MD_fetch() if available #5116
Conversation
lrstewart
force-pushed
the
openssl3fips_evp_2a
branch
5 times, most recently
from
16dfb2a to
0433ba4
Compare
| const EVP_MD *md = s2n_hash_alg_to_evp_md(alg); | ||
| POSIX_ENSURE(md, S2N_ERR_HASH_INVALID_ALGORITHM); | ||
| POSIX_GUARD_OSSL(EVP_DigestInit_ex(state->digest.high_level.evp.ctx, md, NULL), | ||
| S2N_ERR_HASH_INIT_FAILED); |
There was a problem hiding this comment.
This doesn't cause more/less failures, it just makes the reason for failures more explicit. It seemed worth adding the S2N_ERR_HASH_INVALID_ALGORITHM here since hash_init is the place we actually kind of expect to discover a hash isn't allowed.
tests/unit/s2n_openssl_test.c
Outdated
| @@ -61,5 +62,10 @@ int main(int argc, char** argv) | |||
| EXPECT_FALSE(s2n_supports_custom_rand()); | |||
| } | |||
|
|
|||
| /* We expect openssl-3.0 to support providers */ | |||
| if (strstr(env_libcrypto, "openssl") && strstr(env_libcrypto, "3")) { | |||
There was a problem hiding this comment.
nit: Is there a reason we can't just do openssl-3?
There was a problem hiding this comment.
Eh not really? I guess I'm just paranoid after seeing all the different ways people format "OpenSSL 3". And openssl-1.0.2 and openssl-1.1.1 don't have "3"s, and it's not like we're going to add another pre-3.0 version.
lrstewart
force-pushed
the
openssl3fips_evp_2a
branch
from
0433ba4 to
9fc8672
Compare
lrstewart
force-pushed
the
openssl3fips_evp_2a
branch
from
2bd347b to
b79264f
Compare
| /* We expect openssl-3.0 to support providers */ | ||
| if (strstr(env_libcrypto, "openssl") && strstr(env_libcrypto, "3")) { | ||
| EXPECT_TRUE(s2n_libcrypto_supports_providers()); | ||
| } |
There was a problem hiding this comment.
Should buildspec_openssl3fips.yml set S2N_LIBCRYPTO?
There was a problem hiding this comment.
Yes it should D: I must have forgotten a negative CI test when I added the spec!
Here's a negative CI test now: https://us-west-2.console.aws.amazon.com/codesuite/codebuild/024603541914/projects/Openssl3fipsWIP/build/Openssl3fipsWIP%3Ac4c1670b-9b51-40a6-96e5-2328ad81a548?region=us-west-2 I ran against a change that broke s2n_build_test only for openssl-3.0-fips: 9319bb3
lrstewart
force-pushed
the
openssl3fips_evp_2a
branch
from
70b4042 to
2a4c4ab
Compare
Co-authored-by: Sam Clark <[email protected]>
Release Summary:
Resolved issues:
related to #5105
Description of changes:
openssl-3.0 "fetches" implementations of algorithms from "providers". To make that more efficient, you're encouraged to "pre-fetch" and store the implementations you use frequently. Additionally, if we want to influence which provider is chosen for the algorithm, we have to provide a "property query string". Here, we're using a string that tells openssl to ignore any default query for fips that was set for MD5.
Basically, this PR should just be a performance improvement for openssl-3.0 (although I didn't actually test that) and is required for openssl-3.0-fips.
I also felt very silly writing EVP_MD *evp_mds[S2N_HASH_SENTINEL], so I renamed S2N_HASH_SENTINEL to S2N_HASH_ALGS_COUNT. That's how it's used almost everywhere in the code, except for like one very old test (s2n_connection_test.c) that really treats it as a sentinel. If that complicates the PR too much, I can revert the rename.
Testing:
We have existing tests for s2n_hash. I added them to the openssl build job.
I also added s2n_openssl_test and s2n_init_test to the openssl build job. I think that's all the really relevant tests.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.