Fix and improve SetFIPS #219
Merged
Conversation
This file contains 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
gdams
approved these changes
Nov 13, 2024
mertakman
approved these changes
Nov 13, 2024
dagood
reviewed
Nov 13, 2024
| @@ -103,6 +103,7 @@ var ( | |||
| providerNameFips = C.CString("fips") | |||
| providerNameDefault = C.CString("default") | |||
| propFIPS = C.CString("fips=yes") | |||
| propNoFIPS = C.CString("-fips") | |||
There was a problem hiding this comment.
I'm not able to find any docs on -fips, and the lack of symmetry vs. fips=yes is a bit strange. Where does this come from?
There was a problem hiding this comment.
Not sure why it didn't show up in my initial searches, but found the meaning here: https://docs.openssl.org/3.0/man7/property/
There was a problem hiding this comment.
Arguably the name could be propIgnoreGlobalFIPS to match the docs, but I don't know if that's meaningfully different.
This was referenced Nov 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
#206 introduced a regression where
openssl.SetFIPS(false)would always fail if there wasn't any FIPS-capable provider.This PR fix that issue, adds some tests for
SetFIPS, which was previously untested.A benefit of the new implementation is that
OSSL_PROVIDER_try_loadis only called if the already loaded providers can't support the desired FIPS mode. It was previously unconditionally called, which was potentially unnecessary and could have undesired side-effects. For example, if the SymCrypt provider was already loaded and the built-in FIPS provider was not loaded but was installed in the system, then it would be loaded when callingSetFIPS(true), ending with two conflicting FIPS providers available.