Fix tests to work with FIPS enabled #4047
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test suite currently failing with FIPS enabled due to hard-coded MD5 tests.
Still failing on: SCons/UtilTests.py test/Configure/ConfigureDryRunError.py test/Configure/implicit-cache.py test/Configure/option--config.py test/option/hash-format.py test/option/option-n.py test/question/Configure.py These tests all have hardcoded md5 sums or use md5 directly Next commit should fix it so the md5-specific tests are skipped if fips mode detected. Also will figure out a way around the hardcoded m5sums.
bdbaddog
reviewed
Nov 1, 2021
SCons/Node/NodeTests.py
Outdated
| node._func_get_contents = 4 | ||
| result = node.get_csig() | ||
| assert result == '550a141f12de6341fba65b0ad0433500', result | ||
| assert result == '550a141f12de6341fba65b0ad0433500' or result == '9a3e61b6bcc8abec08f195526c3132d5a4a98cc0' or result == '3538a1ef2e113da64249eea7bd068b585ec7ce5df73b2d1e319d8c9bf47eb314', result |
There was a problem hiding this comment.
might be cleaner as result in ( hash1, hash2, hash3) ? (where hash# is the quoted hashes above)
There was a problem hiding this comment.
I will change it to the result-in syntax.
bdbaddog
reviewed
Nov 1, 2021
doc/man/scons.xml
Outdated
| <para>If this option is not specified, a hash format of | ||
| md5 is used, and the SConsign database is | ||
| <para>If this option is not specified, a the first supported hash format found | ||
| is selected, and the SConsign database is |
There was a problem hiding this comment.
maybe add
typically this is md5, however if you are on a FIPS compliant system it will likely be sha256
bdbaddog
reviewed
Nov 1, 2021
CHANGES.txt
Outdated
| RELEASE VERSION/DATE TO BE FILLED IN LATER | ||
|
|
||
| From Jacob Cassagnol: | ||
| - Default hash algorithm check updated for Scons FIPS compliance. Now checks for hash viability |
There was a problem hiding this comment.
minor nit should be "SCons", not "Scons"
There was a problem hiding this comment.
Whoops. I keep doing that, I will get to it tomorrow 😁
bdbaddog
changed the title
Refactored code to match PR requirements Changed result ==... result== to result in(...) Updated the man page to reference the defaulting behavior for FIPS machines. Updated a typo and design of change in the CHANGES.txt file Made an ugly bit of code in Util.py that fixes it so python3.9 has MD5 support enabled in FIPS mode. The code is much more ugly than before, with the tradeoff being it's testable and usable. In python >= 3.9, SCons will now always default to MD5. Next commit will work on getting the tests to auto-skip the md5-required steps on versions <= python 3.8.
|
hmmm looks like the changes in 188d6f8 broke python 3.8 builds at least on Windows. |
The original check was causing issues where 3.8 in Windows was throwing an unexpected error. This should hopefully fix 3.8 in Windows while maintaining the same support in Linux.
Modified failing tests to use the new defaulted .sconsign database based on the hash algorithm For MD5, default database will be .sconsign.dblite For other algorithms the default will be .sconsign_<hashname>.dblite. For all cases where the user changes the hash algorithm used, the database will be .sconsign_<hashname>.dblite (including md5) For sub-scons directories it remains as .sconsign Also added unit-tests for Util.py for the new hash default changes. It's difficult to setup a fips-compliant platform using containers, and instead we mock that. option--config uses multiple types of hash algorithms so was skipped. Removed one f-string (python 3.5 doesn't support those) Corrupt.py is using an explicit .sconsign so that was left as-is, and only the parent default .sconsign was changed for work test 1. A fetch-database name option was added to the testing framework. The unlink_sconsignfile was not updated as no usages of it were found.
|
I'm going to let the tests run, and if it passes the tests this PR should be ready for review. |
The linter identified that warnings was unused in hash-format.py The linter found that the skip_test function call was using 2 space indentation (not sure how I managed to do that). Warning was originally added as hash-format gave a warning on ALLOWED_HASH_FORMATS != default That was changed to instead test the different cases directly. option--config.py skips the test if the default algorithm is not MD5 as the hashes used are OS-specific, and supporting it in FIPS mode would be difficult. It's better to just skip this testcase in FIPS mode instead as this specific test is different in that regard to the other FIPS-enabled tests.
…defaulted to md5. Dir search now excludes all types of sconsfiles that are now created. Environment now defaults to the current scons filename instead of .sconsfile Sconsign now has a function used by a lot of code that gets the default sconsign filename Any tests referring to .sconsfile have now been changed, including one old legacy test.
is none vs == None. this check is to ensure that the default None passthrough works correctly
Accidentally added two unused format strings. Also referenced a variable from a copy-paste error.
These lines setup the sconsign program to use the correct filename for the test.
the mocks have all attrs by default, had to remove the nonexistent ones. The test was missing the correct name and wasn't running.
jcassagnol-public
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelog
Fixes #2949 (FIPS breaking default build, even with hash-format changes).
Fixes test suite with FIPS mode enabled.
Fixes all non-defaulted-to-md5 .sconsign files to all store to .sconsign_algorthm files, including sub-sconsign files.
Default algorithms other than md5 also change the sconsign database name correctly (example if SCons defaults to sha1, the database now correctly is named .sconsign_sha1.dblite instead of .sconsign.dblite)
Purpose
Builds using FIPS mode were broken prior to the changes made in this PR, as despite the --hash-format being added as an option, several code paths existed in the primary codebase that would default to MD5 and would crash out with a message about the MD5 algorithm was not allowed to initialize in FIPS mode.
This PR seeks to change the defaulting behavior of SCons such that the first algorithm it defaults to is one it can call successfully rather than hard-setting the default to MD5 as it currently is setup.
It also seeks to fix some minor edge conditions which currently occur when using the --hash-format flag, namely updates the naming of all sconsign files to match the parent database name, rather than only the root database having the name in the extension.
Long-term Python supports use of MD5 with FIPS mode turned on through the use of a special flag passed to hashlib, this PR will also include defaulting behavior to enable that, meaning Python users with version >= 3.9 will always see default behavior of MD5. This is a mid-term fix for older/slower distributions of Linux such as CentOS/Rocky Linux 7, 8, which remain on python <= 3.6 for the foreseeable future as the out-of-box platform distribution of python.
Contributor Checklist:
CHANGES.txt(and read theREADME.rst)