Enable fipstls when running in FIPS mode #1127
Conversation
| @@ -281,6 +282,10 @@ index 00000000000000..d929b3e516bdb0 | |||
| + } | |||
| + } | |||
| + } | |||
| + if openssl.FIPS() { | |||
There was a problem hiding this comment.
Maybe call this once before the switch to avoid potentially calling it twice?
There was a problem hiding this comment.
The code in the switch potentially call openssl.SetFIPS, so it can't be directly reused. We could add some logic to workaround that, but this code is already quite tricky and I would prefer not to add more complexity just to save a couple of cgo calls that are only ever called once.
| @@ -281,6 +282,10 @@ index 00000000000000..d929b3e516bdb0 | |||
| + } | |||
| + } | |||
| + } | |||
| + if openssl.FIPS() { | |||
| + fipstls.Force() | |||
| + sig.FIPSOnly() | |||
There was a problem hiding this comment.
This could be misleading... since openssl.FIPS() isn't a const, my understanding is this will always be linked in whether FIPS mode is on or off, then a static scanner (coded with boringcrypto in mind) would claim that the whole binary uses FIPS-only TLS.
The sig file's comment is pretty specific:
// FIPSOnly indicates that package crypto/tls/fipsonly is present.
func FIPSOnly()
However I do think that in spirit, linking in sig.FIPSOnly() makes sense.
As far as the clarity of the patch goes, I think it's worth a comment to clarify the intent and move it out of the if so there can't be any misimpression that it'll actually be conditionally included.
That all said, https://pkg.go.dev/rsc.io/goversion already doesn't seem to work on our binaries (at least on Windows?): ... X:cngcrypto (standard crypto). So, actual interactions with scanners is probably something to figure out later. The go tool nm foo.exe output looks correct to me, including both BoringCrypto and FIPSOnly.
There was a problem hiding this comment.
Hmm, interesting. Given that we can't really make sig.FIPSOnly work correctly, I would rather not add it. If someone is using an scanner looking for sig.FIPSOnly as a matter of compliance, I prefer to have a false negative (aka binary reported as not compliant when it is compliant) that can be fixed with an exception rather than a false positive (aka binary reported as compliant when it is not compliant).
dd7586b to
a755653
Compare
Automatically enforces that
crypto/tlsandcrypto/x509only use FIPS-approved settings when running in FIPS mode. This differs from upstream's BoringCrypto backend, which requires you to importcrypto/tls/fipsonlyto apply the FIPS-mandated restrictions.Do this automatically to reduce the source code changes necessary to produce a FIPS-compliant Go application.
Code taken from #964.