Prioritize FIPS-enabled versions when automatically selecting a version #53
Conversation
| provName = providerNameDefault | ||
| } | ||
| // Check if there is any provider that matches props. | ||
| if !providerAvailable(props) { | ||
| if C.go_openssl_OSSL_PROVIDER_available(nil, provName) == 0 { | ||
| // If not, fallback to provName provider. |
There was a problem hiding this comment.
This doesn't seem to line up with the behavior mentioned in the SetFIPS func comment--it seems like there isn't room for a config-file-based provider to sneak in anymore.
There was a problem hiding this comment.
Did that on purpose, but forgot to remove the comment. I now think that supporting FIPS providers not named fips (as the official one) is overthinking, and the implications of that are unknown to me, I haven't seen that support elsewhere. For example, nodejs and python checks if the fips provider can be loaded and if is enabled by default, and ruby only checks if it is enabled by default.
There was a problem hiding this comment.
Notice that I've implemented things a bit different in golang-fips/openssl#66. That's because I don't want to break the go-crypto-openssl API, in case we decide to backport this.
Co-authored-by: Davis Goodin <dagood@users.noreply.github.com>
| int (*FIPS_mode)(void); | ||
| FIPS_mode = (int (*)(void))dlsym(handle, "FIPS_mode"); | ||
| if (FIPS_mode != NULL) | ||
| return FIPS_mode(); |
There was a problem hiding this comment.
Hmm, on second look, I think this might be going too deep? If systemwide/opensslwide FIPS isn't enabled, it seems like this means that even if FIPS mode is supported, this will return 0, so this version of OpenSSL won't get picked. So if we e.g. have 3.0 that doesn't support FIPS and 1.0.2 that supports FIPS but FIPS isn't enabled by default, we would pick 3.0 as the "fallback", not have FIPS, and (e.g.) fail to turn on FIPS mode later when we really should have been able to.
There was a problem hiding this comment.
Yep, that was may intention, pick the systemwide enabled OpenSSL version, not just one that is FIPS-capable but not enabled (should have documented it better in the PR description). IMO it makes more sense this way, production environments that require FIPS must have it enable systemwide, as not all tooling and framework provides something like GOFIPS=1.
There was a problem hiding this comment.
Interesting, yeah, that makes sense.
My next thought would be "what if you want to test FIPS mode on a non-FIPS machine with multiple OpenSSL versions etc." but in that case, setting GO_OPENSSL_VERSION_OVERRIDE is perfectly fine if the fallback isn't the right version--and it's probably something you would end up doing anyway to test each version intentionally. 😄
This PR updates the
Init("")behavior so it first try to select a FIPS-enabled OpenSSL version from the list of well-known versions. If non is found, then fallback to the previous behavior.While here, simplify
SetFipsby using functions provided by OpenSSL 3 instead of trying to do everything ourselves.