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

Support executing the Go toolchain with GOFIPS set #1141

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

qmuntal
Copy link
Member

@qmuntal qmuntal commented Feb 14, 2024

It is currently not possible to execute the Go toolchain with most combinations of GOFIPS+GOEXPERIMENT. For example, this command fails: GOFIPS=1 GOEXPERIMENT=systemcrypto go test ./...:

$ GOFIPS=1 GOEXPERIMENT=systemcrypto ~git/go/go/bin/go run .
panic: FIPS mode requested (environment variable GOFIPS=1) but no supported crypto backend is enabled

goroutine 1 [running]:
crypto/internal/backend.init.0()
        /home/dagood/git/go/go/src/crypto/internal/backend/common.go:21 +0x65

The GOFIPS env is intended to be used in the temporary binary generated by go test (and go run), but it also affects the Go toolchain itself, as it imports crypto/sha256, which triggers the GOFIPS check even before building the binary.

This PR updates the Go toolchain initialization to unset GOFIPS for it's own processes, while keeping it set for the user binaries. There is no need to add new tests, as go test and go run are already heavily tested. We haven't seen any test failure in our fips builders yet because we were setting GOFIPS=true instead of GOFIPS=1, which caused the tests to ignore it.

Notice that this change doesn't preclude the Go toolchain to run in FIPS mode. When GOFIPS is unset, the default behavior is to try to use FIPS mode whenever possible. The only change is that the process won't panic when FIPS mode can't be enabled. If someone wants to make sure that the Go toolchain is executed in FIPS mode, then the only solution is to build it with -tags requirefips.

Fixes #1106.

@qmuntal qmuntal requested review from dagood and gdams February 14, 2024 10:43
@qmuntal qmuntal marked this pull request as ready for review February 14, 2024 10:44
Copy link
Member

@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.

Looks good! At first glance the dependency on init order seems fragile, but because packages are basically initialized breadth-first, it's fine. If we mess it up in the future, the error may still be confusing, but the fixed test should at least keep it confined to the PR that breaks it.

I tried to think of a way to make the error clearer in case the ordering does somehow get mixed up, but I couldn't come up with anything because communication between cmd/ and crypto/ is limited (can't use each others' internal packages).

@qmuntal qmuntal merged commit c000bd6 into microsoft/main Feb 19, 2024
19 checks passed
@qmuntal qmuntal deleted the dev/qmuntal/gofips branch February 19, 2024 15:51
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.

Improve behavior of GOFIPS=1 GOEXPERIMENT=systemcrypto go run .
3 participants