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

Build the Go toolchain using allowcryptofallback #1505

Open
wants to merge 6 commits into
base: microsoft/main
Choose a base branch
from

Conversation

qmuntal
Copy link
Member

@qmuntal qmuntal commented Jan 17, 2025

allowcryptofallback can be used to allow executing the Go toolchain with GOFIPS=1, e.g. GOFIPS=1 go test ., even when the Go toolchain itself is not built with GOEXPERIMENT=systemcrypto.

Reusing allowcryptofallback allows us to drop patch 7, which was tackling the same issue with a more intrusive and less robust approach: unsetting GOFIPS when the runtime was initializing and setting it again every time a child process was spawned.

This will help migrating from GOFIPS to GODEBUG=fips140.

These changes uncovered a latent bug in the Go toolchain: GOFIPS was not being honored, for a reason that I still not understand, the Go toolchain didn't pass the GOFIPS env var to child processes. Note that this bug doesn't affect setting GOFIPS in production environments, only when running commands like go test, go run, or go tool dist test.

Now that GOFIPS actually does something when running go tool dist test (which we execute in CI to run all the toolchain tests), Mariner 1 and Mariner 2 builders started to fail (see logs) due to FIPS mode not being enabled (remember that we removed the openssl.SetFIPS(true) call in #1496, so we no longer enable FIPS mode on demand). To fix this, I've updated the builder scripts so that it enables FIPS mode before running the tests by setting the Mariner/AZL3 specific OPENSSL_FORCE_FIPS_MODE env var. Note that we already do something similar on Windows, where FIPS mode is enabled by modifying the system registry.

For #1445.

@qmuntal qmuntal force-pushed the dev/qmuntal/allowfips branch from da6b804 to 654eb65 Compare January 17, 2025 13:16
@qmuntal qmuntal marked this pull request as ready for review January 17, 2025 16:40
@qmuntal qmuntal requested review from dagood, mertakman and gdams January 17, 2025 16:40
+ // or "GOFIPS=1 go run .".
+ // Shadow goexperiment so that the global variable is not modified.
+ goexperiment := goexperiment
+ if !strings.Contains(goexperiment, "allowcryptofallback") {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, wonder if this patch would also let us remove this, to simplify:

// If the experiment enables a crypto backend, allow fallback to Go crypto. Go turns off cgo
// and/or cross-builds in various situations during the build/tests, so we need to allow for it.
if strings.Contains(experiment, "opensslcrypto") ||
strings.Contains(experiment, "cngcrypto") ||
strings.Contains(experiment, "boringcrypto") ||
strings.Contains(experiment, "darwincrypto") ||
strings.Contains(experiment, "systemcrypto") {
experiment += ",allowcryptofallback"

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