Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor openssl.FIPS to support third-party FIPS providers #184

Open
wants to merge 4 commits into
base: v2
Choose a base branch
from

Conversation

qmuntal
Copy link
Collaborator

@qmuntal qmuntal commented Sep 13, 2024

Our current openssl.FIPS implementation only works with the built-in FIPS provider, as it checks whether it is available or not to determine if FIPS is enabled, which precludes using other FIPS providers.

This PR removes the OSSL_PROVIDER_available(nil, "fips") check from openssl.FIPS so it is not tied to the built-in provider.

The new implementation should be as good as the previous: if EVP_default_properties_is_fips_enabled returns 1, we can be sure that only providers matching the fips=1 property will be used, so we can say that FIPS mode is enabled. To avoid breacking backwards compatibility, I've added an additional check that tried to fetch a SHA256 digets using the default properties (which should contain fips=1 at that point). If it succeeds, then it means that there is a provider that matches the fips=1 query.

Copy link
Collaborator

@dagood dagood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside is that if FIPS mode is configured (aka fips=1 in the default properties) but the FIPS provider is missing, then openssl.FIPS will return true and the user won't notice that FIPS mode is unusable until it tries to use an algorithm, e.g. calling sha256.New.

It looks like if a Go fork wanted to keep this init-time check working and only expects to ever run with the built in OpenSSL FIPS provider, there would just need to be a new Go-side func added to let that Go fork run the old check. (But if nobody actually cares, there's no need to add a func like that.)

@qmuntal qmuntal changed the title Simplify openssl.FIPS to support SymCrypt Simplify openssl.FIPS to support third-party FIPS providers Sep 13, 2024
@qmuntal qmuntal changed the title Simplify openssl.FIPS to support third-party FIPS providers Refactor openssl.FIPS to support third-party FIPS providers Sep 16, 2024
@qmuntal qmuntal requested a review from dagood September 16, 2024 06:16
@qmuntal
Copy link
Collaborator Author

qmuntal commented Sep 16, 2024

@dagood I made up my mind a bit with this this PR. I was too much of a breaking change. I've added a provider-agnostic check so the new behavior is less disruptive. Please take another look.

openssl.go Outdated Show resolved Hide resolved
return 0;
if (EVP_default_properties_is_fips_enabled == NULL || EVP_MD_fetch == NULL || EVP_MD_free == NULL) {
// Shouldn't happen, but if it does, we can't determine if FIPS mode is enabled.
return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A false negative for FIPS mode does seem nearly harmless, but it also seems like it would be pretty simple to return and handle -1 or 2 or something just to be sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing with the current openssl.CheckVersion API is to log the error or panic, as it doesn't return any error value. I wouldn't say it is a false negative, if those functions are not available then we won't be able to use the shared library properly, even less in FIPS mode.

goopenssl.c Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants