-
-
Notifications
You must be signed in to change notification settings - Fork 722
feat(mode): Add gofips140 mode #4449
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @blazingbbq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request integrates the GOFIPS140 environment variable into the Go build system, enabling users to manage the Go Cryptographic Module version directly through Bazel rules. This enhancement provides crucial control for applications requiring FIPS 140 compliance, ensuring that the correct cryptographic implementations are used during the build and test phases of Go projects.
Highlights
- GOFIPS140 Environment Variable Support: This pull request introduces comprehensive support for the
GOFIPS140environment variable within Bazel's Go rules, allowing users to specify the version of the Go Cryptographic Module to be linked into executables. - New gofips140 Attribute: A new
gofips140attribute has been added togo_binaryandgo_testrules, providing direct control over theGOFIPS140setting. This attribute accepts values like"off","latest", or specific versions such as"v1.0.0". - Configuration and Documentation Updates: The
go_configdefinition, build settings, and internal mode handling have been updated to recognize and propagate thegofips140setting. Corresponding documentation inrules.bzl,rules.md,core.rst, andmodes.rsthas been added or modified to reflect this new feature. - New Test Cases: New test cases have been added to
tests/core/go_binary/andtests/core/transition/to verify that thegofips140attribute and the--@io_bazel_rules_go//go/config:gofips140flag correctly control theGOFIPS140environment variable and its impact onfips140.Enabled().
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces support for the GOFIPS140 build mode, which is a valuable addition for users requiring FIPS 140 compliance in their Go applications. The changes are comprehensive, touching the build configuration, rule definitions, documentation, and adding new tests. While the overall approach is sound, I've identified a couple of critical issues in the implementation that could lead to incorrect build behavior and caching problems. Additionally, there are some minor documentation inconsistencies that could be improved for clarity. My review includes detailed suggestions to address these points.
fmeum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good, thanks for picking this up. Left a comment that may require some changes.
| if tags: | ||
| settings["//go/config:tags"] = _deduped_and_sorted(tags) | ||
|
|
||
| gofips140 = getattr(attr, "gofips140", "off") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this mean that it's impossible to disable fips for a binary if it is enabled globally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option just determines what gets built into the crypto module (see: https://go.dev/doc/security/fips140#the-gofips140-environment-variable). Essentially:
"off": Use the fips140 crypto from the standard library tree in use, but does not enable FIPS mode at runtime by default. You would need the GODEBUG runtime option to enable fips140 (e.g.GODEBUG=fips140=onorGODEBUG=fips140=only)"latest": Same as off, but enables FIPS mode without needing the runtime flag."vX.Y.Z": Uses a specific Go crypto module, and enabled FIPS mode by default.
|
Trying to get the tests working, will mark as Ready for Review once those are passing. |
jayconrod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up, this looks really good. I'm not really familiar with the GOFIPS140 functionality, but from reading the docs, it sounds like all we need to do is set the GOFIPS140 environment variable when building the standard library and when compiling and linking. The go tool itself may be doing some module magic, but setting the environment variable should cover it.
It seems like a build setting and a go_binary / go_test attribute is the correct way to express that, though I regret that go_binary has so many of these attributes now. Oh well, it's consistent.
|
Hi, a gentle ping on this PR -- I'm wondering how we can get this merged. It would be great to have support for FIPS mode in |
|
This generally looks good to me, we just have to get CI green. You may have to configure a more recent SDK foot the integration test. |
This captures code currently in the form of a draft PR at bazel-contrib#4449. When this PR is merged, this commit can be omitted from the cherry-picks onto our `crl-release-*` branches.
|
Having spent some time trying to get this working, it seems at least the following patches are required if you are using That doesn't seem to be entirely sufficient, and the build now fails for me with the error |
|
Still looking into the |
|
I'm no expert and am slightly out of my depth here, so it's very possible I'm missing something. But I think supporting
On the I hope I'm misreading the situation and it's much more straightforward than I think it is :) |
|
Could we limit support to |
|
I wonder instead if |
Unfortunately I also see that building with |
|
This gist captures my current version that I've been iterating on, derived from the original code in this PR. (It attempts to integrate jayconrod's advice to associate this configuration with the toolchain rather than the binary or test binary.) When attempting to build with
|
|
I found that the following diff is necessary to support the This ensures that the binary defaults to
The problem is that some of the built One option could be to use the gcexportdata package to read all the Edit: this gist captures my latest version of this, which supports |
The previous version of the code did not correctly build with `GOFIPS140=latest`, and namely did not link the binary such that FIPS mode was on-by-default, which it should. This fixes that. We also revert some misleading/incorrect changes from the prior in-progress commit `7c5c55d859d1518f963a79abec87af34876cc2ee`. See discussion [here](bazel-contrib#4449), and specifically [this comment](bazel-contrib#4449 (comment)).
Prior to this change, the FIPS build did not have FIPS mode enabled by default. This PR addresses this via a change to `rules_go`. We still need to fix `rules_go` so that `GOFIPS140=v1.0.0` will work, but that requires more tweaking (see [discussion](bazel-contrib/rules_go#4449 (comment))). Release note: none Epic: DEVINF-1477
Prior to this change, the FIPS build did not have FIPS mode enabled by default. This PR addresses this via a change to `rules_go`. We still need to fix `rules_go` so that `GOFIPS140=v1.0.0` will work, but that requires more tweaking (see [discussion](bazel-contrib/rules_go#4449 (comment))). Release note: none Epic: DEVINF-1477
Prior to this change, the FIPS build did not have FIPS mode enabled by default. This PR addresses this via a change to `rules_go`. We still need to fix `rules_go` so that `GOFIPS140=v1.0.0` will work, but that requires more tweaking (see [discussion](bazel-contrib/rules_go#4449 (comment))). Release note: none Epic: DEVINF-1477
156976: build: fix behavior of FIPS build r=rail a=rickystewart Prior to this change, the FIPS build did not have FIPS mode enabled by default. This PR addresses this via a change to `rules_go`. We still need to fix `rules_go` so that `GOFIPS140=v1.0.0` will work, but that requires more tweaking (see [discussion](bazel-contrib/rules_go#4449 (comment))). Release note: none Epic: DEVINF-1477 Co-authored-by: Ricky Stewart <[email protected]>
What type of PR is this?
Feature
What does this PR do? Why is it needed?
This PR adds support for the
GOFIPS140environment variable can be used with go build, go install, and go test to select the version of the Go Cryptographic Module to be linked into the executable program.See: https://go.dev/doc/security/fips140
Which issues(s) does this PR fix?
Fixes #4293
Other notes for review
None.