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

Remove openssl.SetFIPS(true) call #1513

Merged
merged 10 commits into from
Jan 28, 2025
Merged

Conversation

qmuntal
Copy link
Member

@qmuntal qmuntal commented Jan 24, 2025

As agreed in https://github.com/microsoft/go-lab/blob/main/docs/adr/0012-remove-gofips.md, we shouldn't try to modify the OpenSSL FIPS mode.

This PR removes the openssl.SetFIPS(true) call and update our build scripts to enable FIPS mode system-wide.

Our CI Mariner 2 image is not FIPS-enabled by default, so we need to force FIPS mode by setting OPENSSL_FORCE_FIPS_MODE. That flag should be passes to the TestScript child processes as they only inherit a filtered set of environment variables, which includes GODEBUG.

Note that since we switched from GOFIPS to GODEBUG=fips140, our test FIPS test coverage has increased, as GOFIPS was not being passed to TestScript child processes, making them not aware of the required FIPS mode.

Also, this is unlikely that users need to update their code to also pass OPENSSL_FORCE_FIPS_MODE to child processes that don't inherit all environment variables. Mainly because they should be running a FIPS-enabled Mariner image on production. If they don't, possible for testing purposes, then child processes won't inherit the GODEBUG env var neither.

For #1445.

@qmuntal
Copy link
Member Author

qmuntal commented Jan 24, 2025

Looks like Mariner 2 hasn't ported forward-ported the code to enable FIPS mode from the config file. OpenSSL only officially supports FIPS mode in v1.0.2, so the relevant code was removed in OpenSSL 1.1. Will have to find another way.

@qmuntal qmuntal closed this Jan 24, 2025
@qmuntal qmuntal reopened this Jan 24, 2025
@qmuntal qmuntal marked this pull request as ready for review January 24, 2025 19:33
@qmuntal qmuntal requested review from dagood, mertakman and gdams January 24, 2025 19:34
patches/0004-Use-crypto-backends.patch Outdated Show resolved Hide resolved
eng/_util/cmd/run-builder/systemfips_linux.go Outdated Show resolved Hide resolved
eng/_util/cmd/run-builder/systemfips_linux.go Outdated Show resolved Hide resolved
eng/_util/cmd/run-builder/systemfips_linux.go Outdated Show resolved Hide resolved
@dagood
Copy link
Member

dagood commented Jan 24, 2025

Also, this is unlikely that users need to update their code to also pass OPENSSL_FORCE_FIPS_MODE to child processes that don't inherit all environment variables. Mainly because they should be running a FIPS-enabled Mariner image on production. If they don't, possible for testing purposes, then child processes won't inherit the GODEBUG env var neither.

To make sure I have this right: for a user to run a test with forced FIPS mode, this will work:

GODEBUG=fips140=on OPENSSL_FORCE_FIPS_MODE=1 go test .

For the user to have an issue with passthrough, they'd have to be using yet another custom test runner that passes through GODEBUG but not OPENSSL_FORCE_FIPS_MODE.

@qmuntal qmuntal requested a review from dagood January 28, 2025 08:32
@qmuntal
Copy link
Member Author

qmuntal commented Jan 28, 2025

Also, this is unlikely that users need to update their code to also pass OPENSSL_FORCE_FIPS_MODE to child processes that don't inherit all environment variables. Mainly because they should be running a FIPS-enabled Mariner image on production. If they don't, possible for testing purposes, then child processes won't inherit the GODEBUG env var neither.

To make sure I have this right: for a user to run a test with forced FIPS mode, this will work:

GODEBUG=fips140=on OPENSSL_FORCE_FIPS_MODE=1 go test .

For the user to have an issue with passthrough, they'd have to be using yet another custom test runner that passes through GODEBUG but not OPENSSL_FORCE_FIPS_MODE.

Yep, that's accurate.

@qmuntal qmuntal merged commit cefe8e6 into microsoft/main Jan 28, 2025
35 checks passed
@qmuntal qmuntal deleted the dev/qmuntal/nosetfipss branch January 28, 2025 09:40
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.

3 participants