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

Prevent unexpected Go crypto fallback: fail instead #965

Merged
merged 4 commits into from
Jun 29, 2023

Conversation

dagood
Copy link
Member

@dagood dagood commented Jun 27, 2023

Once this is merged, something like GOEXPERIMENT=opensslcrypto GOOS=windows go build . will fail with:

# runtime
../go/go/src/runtime/backenderr_gen_nofallback_openssl.go:12:2: `
        The goexperiment.opensslcrypto tag is specified, but other tags required to enable that backend were not met.
        Required build tags:
          goexperiment.opensslcrypto && linux && cgo && !android && !cmd_go_bootstrap && !msan
        Please check your build environment and build command for a reason one or more of these tags weren't specified.
        For more information, visit https://github.com/microsoft/go/tree/microsoft/main/eng/doc/fips
        ` (untyped string constant "\n\tThe goexperiment.opensslcrypto tag is specified, but other tags ...) is not used

Adding -tags=allow_missing_crypto_backend_fallback removes the error, allowing fallback. This is only intended for specific cases in our CI job where we e.g. enable GOEXPERIMENT=opensslcrypto globally and then run the whole test suite. A number of tests have inner go build commands that either disable cgo or use a different GOOS and needed the tag.

If someone needs to use Go crypto in their own app, it makes more sense to remove the GOEXPERIMENT value. Then, it's clear what the intent is by looking at the build command, and our toolset then makes sure the intent is followed.

This PR also changes the cngcrypto/opensslcrypto/boringcrypto CI jobs to build normally and run the tests with the target GOEXPERIMENT. The toolchain (at various levels of bootstrapping) builds without cgo, so the "accidental fallback" error would show up. Since the toolchain is using fallback Go crypto, we aren't meaningfully testing the goexperiment in these cases, so it makes more sense disable the goexperiment for the initial build rather than feed allow_missing_crypto_backend_fallback into many places.

(The build of Go we ship is built with no GOEXPERIMENT, so it isn't necessarily important to test that building Go with a crypto experiment works, only that tests built with our Go toolset and the crypto experiments work.)

The bootstrap builds fail because they always fall back to Go standard crypto. Instead of adding code in various places of the bootstrap process to allow the fallback, don't enable the experiment at this point in the build process. Enable the experiment for the test run specifically.
@dagood dagood marked this pull request as ready for review June 28, 2023 22:21
// build is clean. The problem is that the build doesn't include cgo, which is
// required for some crypto backends. Set this variable to promise that our build is
// fresh and avoid the non-cgo build.
os.Setenv("GO_BUILD_FRESH", "1")
Copy link
Contributor

Choose a reason for hiding this comment

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

GO_BUILD_FRESH seems like an upstream env var and it keeps me wondering if we are reusing some already existing functionality. I'll prefer to name is something like GO_MSFT_BUILD_FRESH so the scope is clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ended up as GO_MSFT_SCRIPTED_BUILD--realized that "BUILD_FRESH" sounds more like a request to run a fresh build, not something that indicates current status.

@dagood dagood merged commit 2eb34d1 into microsoft/main Jun 29, 2023
@dagood dagood deleted the dev/dagood/crypto-intent branch June 29, 2023 21:05
dagood added a commit that referenced this pull request Jul 14, 2023
…ith patch revert performed manually

This reverts commit 2eb34d1.
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.

Make nobackend.go build constraint an inverse of the other backends (and add test)
3 participants