ci: improve fips mode coverage #4970
Conversation
| foreach(test_case ${UNITTESTS_SRC}) | ||
| foreach(fips_mode ${TEST_FIPS_MODES}) |
There was a problem hiding this comment.
I know there's no indentation. I think it looks better than indentation. But I'm probably wrong xD
There was a problem hiding this comment.
Ehh, I'd vote for indentation. I agree that it looks ugly, but I think we need to be honest about ugly things we're doing, lol.
| # FIPS mode is optional for Openssl and must be deliberately configured | ||
| if ("$ENV{S2N_LIBCRYPTO}" MATCHES "openssl" AND "$ENV{S2N_LIBCRYPTO}" MATCHES "fips") | ||
| set(TEST_FIPS_MODES true false) | ||
| message(STATUS "Building against $ENV{S2N_LIBCRYPTO}: will test FIPS modes") |
| # NAME_WE: name without extension | ||
| get_filename_component(test_case_name ${test_case} NAME_WE) | ||
|
|
||
| if(fips_mode) | ||
| set(test_case_name ${test_case_name}+fips) | ||
| set(target_compile_definitions(${test_case_name} PRIVATE S2N_TEST_IN_FIPS_MODE)) |
There was a problem hiding this comment.
I can't figure out why this is wrapped in a set directive. Can we add a comment?
There was a problem hiding this comment.
Looking at the code, it looks like there is an ifdef in s2n_test.h, so I think this compile definition also needs to be added to the testlib.
Maybe it would be neater as a PUBLIC option on the testlib? But perhaps that's a bit too magical.
There was a problem hiding this comment.
s2n_test.h actually gets compiled into the individual tests, not testlib. That's how test_count works.
| foreach(test_case ${UNITTESTS_SRC}) | ||
| foreach(fips_mode ${TEST_FIPS_MODES}) |
There was a problem hiding this comment.
Ehh, I'd vote for indentation. I agree that it looks ugly, but I think we need to be honest about ugly things we're doing, lol.
Release Summary:
Description of changes:
Improve fips mode coverage. Unit tests enable fips mode when S2N_TEST_IN_FIPS_MODE is defined: see code.
When testing an openssl version that supports fips, this change causes the unit tests to test both fips and non-fips modes. Alternatively, we could just always test fips mode. Since we also test versions of openssl that don't support fips, that arguably wouldn't lose much coverage (although the exact version tested will always be different due to fips lagging behind). I'm curious for other opinions on what testing is sufficient.
Testing:
Existing tests pass. If you examine the output of one of the openssl-1.0.2-fips builds, you can now see both normal and "+fips" versions of each unit test.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.