Skip to content

Prefer GODEBUG=fips140 over GOFIPS #1503

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

Merged
merged 6 commits into from
Jan 20, 2025
Merged

Conversation

qmuntal
Copy link
Member

@qmuntal qmuntal commented Jan 17, 2025

This PR makes our backends to fail if GODEBUG=fips140=1 is set but FIPS mode is not enabled, as agreed in https://github.com/microsoft/go-lab/blob/main/docs/adr/0012-remove-gofips.md.

⚠️ This partially reverts #1496: it reintroduces openssl.SetFIPS(true) 😢 . Removing it will make life more difficult for applications running on non-FIPS distros (like Mariner 2 when not running on FIPS mode) which have properly configured OpenSSL to be FIPS-compliant, as most times one still need to call FIPS_mode_set(true). On the upside, since #1496 we use openssl.FIPSCapable instead of openssl.FIPS, which makes it less likely to end up calling openssl.SetFIPS. Most importantly, we no longer call that function when running on Azure Linux 3, which was my main goal with #1496.

For #1445.

@qmuntal qmuntal requested review from dagood, mertakman and gdams January 20, 2025 12:11
@qmuntal qmuntal marked this pull request as ready for review January 20, 2025 12:11
@qmuntal qmuntal merged commit 386505a into microsoft/main Jan 20, 2025
35 checks passed
@qmuntal qmuntal deleted the dev/qmuntal/fips149deb branch January 20, 2025 12:49
@dagood
Copy link
Member

dagood commented Jan 22, 2025

This partially reverts #1496: it reintroduces openssl.SetFIPS(true) 😢 . Removing it will make life more difficult for applications running on non-FIPS distros (like Mariner 2 when not running on FIPS mode) which have properly configured OpenSSL to be FIPS-compliant, as most times one still need to call FIPS_mode_set(true).

(I assume "Removing it will make life more difficult" should be "Removing it would have made life more difficult" or similar, because this PR adds it back in?)

Why are we supporting this situation? My reading of the ADR is that we will intentionally make this situation harder--there is no provision for this halfway point. In the PR thread, my sense was that it only makes sense in a testing scenario, and we should instead document how the user can properly enable OpenSSL FIPS "locally-ish" on the test machine.

+
+// Enabled reports whether FIPS 140 mode is enabled by using GOFIPS=1, GOLANG_FIPS=1,
+var fips140GODEBUG = godebug.New("#fips140")
Copy link

Choose a reason for hiding this comment

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

@qmuntal I am reading this code and I am not sure if this is a typo, or if it correct. Everywhere else I see calls to godebug.New("fips140") what is the meaning of a leading #? I also don't see anywhere for GODEBUG=#fips140=on to be set.

Copy link
Member

@dagood dagood Apr 23, 2025

Choose a reason for hiding this comment

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

# is consumed by New: https://pkg.go.dev/internal/godebug#New.

GODEBUGs meant for use by end users must be listed in ../godebugs/table.go, which is used for generating and checking various documentation. If the name is not listed in that table, New will succeed but calling Value on the returned Setting will panic. To disable that panic for access to an undocumented setting, prefix the name with a #, as in godebug.New("#gofsystrace"). The # is a signal to New but not part of the key used in $GODEBUG.

Copy link
Member

Choose a reason for hiding this comment

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

Related: upstream changed theirs from #fips140 to fips140 post-1.24, in golang/go@10cef81. (I don't think still having # should hurt, though.)

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.

4 participants