fipsccl: Add interfaces to expose FIPS-readiness status #114709
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 9 of 9 files at r2, 3 of 3 files at r3, 23 of 23 files at r4, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @bdarnell)
| // IsOpenSSLLoaded returns true if the OpenSSL library has been found and | ||
| // loaded. | ||
| func IsOpenSSLLoaded() bool { | ||
| return boring.Enabled() |
There was a problem hiding this comment.
What does this function do? I must be looking in the wrong place as what I find in the standard library suggests Enabled is a bool rather than a function. My suspicion is we don't actually need to create a dependency on crypto/boring and understanding what this function is doing will help us break the dependency.
There was a problem hiding this comment.
When golang-fips forked the boringcrypto branch, they left the crypto/boring package in a weird state. It is the only non-internal package to expose any information at all, but it's weirdly named in this context, it requires the boringcrypto build tag which is otherwise unused, and it's not even compatible with upstream because they changed this bool to a function. I'll file an issue with golang-fips to do something about this.
There was a problem hiding this comment.
IIUC Enabled() returns the value of the variable enabled which is set here.
There was a problem hiding this comment.
Yes, but that enabled variable is in a package that is in a vendor subdirectory of the (patched) standard library. There's no way for us to reach it except through the crypto/boring package. I've filed an upstream issue (golang-fips/go#147) asking for a cleaner way to do this, but for now the only way seems to be to go through this incompatible patched version of crypto/boring (which is fine because we don't support the actual boringcrypto experiment, only the golang-fips fork).
|
|
||
| // IsBoringBuild returns true if this binary was built with the boringcrypto | ||
| // build tag, which is a prerequisite for FIPS-ready mode. | ||
| func IsBoringBuild() bool { |
There was a problem hiding this comment.
Maybe IsFipsBuild? I don't like the IsBoringBuild name because we are not actually using boringcrypto and we risk confusing readers if we expose that information in public function names like this. If we must use crypto/boring for the check due to how the code is structured, then we have no choice about that, but we do have a choice to not propagate the confusion further.
There was a problem hiding this comment.
I actually had it that way at one point and confused myself in other ways :) The main function, the one you almost always want, is IsFIPSReady(). If the diagnostic function that reports on the build-time configuration also starts with IsFIPS, it's easy to use the wrong one. I settled on IsBoringBuild because it is technically precise in a way (that is the build tag it's looking for) and I'm not too worried about spreading confusion with a function that's only ever going to have 1 or 2 call sites. When golang-fips/go#147 is resolved and we have a different build tag to work with, I'll rename this function.
There was a problem hiding this comment.
At the very least I ask that the documentation comment for IsBoringBuild be updated to suggest that we are not actually using boringcrypto, and this function is checking for the presence of a build tag that does not actually imply boringcrypto usage. I still don't think the function name is good because it will always lie (we NEVER produce a boringcrypto build, regardless of what this function says). The least we can do is make the comment precise.
There was a problem hiding this comment.
OK, renamed to IsCompileTimeFIPSReady.
bdarnell
force-pushed
the
debug-fips2
branch
from
59f7b7d to
f6b60df
Compare
There was a problem hiding this comment.
Reviewed 29 of 29 files at r5, 3 of 3 files at r6, 23 of 23 files at r7, all commit messages.
Reviewable status:
The boringcrypto build tag/experiment is not necessary to enable FIPS functionality with this toolchain, but it is necessary to expose the crypto/boring.Enabled method, which is the application-visible way to confirm that FIPS mode is in use. Updates cockroachdb#114344 Release note: None
This command reports on the status of certain prerequisites for our fips-ready builds. Updates cockroachdb#114344 Release note (cli change): New command `cockroach debug enterprise-check-fips` diagnoses errors in FIPS deployments
Previously, misconfigurations of the FIPS environment would result in a silent fallback to non-FIPS-compliant Go cryptography. This flag permits users who require FIPS compliance to add some checks to CockroachDB startup to ensure that the Go crypto implementation will not be used. Updates cockroachdb#114344 Release note (cli change): New flag --enterprise-require-fips-ready can be added to any CRDB command to prevent startup if certain prerequisites for FIPS compliance are not met.
This function provides a way to verify FIPS readiness without modifying the deployment to add the --enterprise-require-fips-ready flag. Updates cockroachdb#114344 Release note (enterprise change): New SQL function fips_ready can be used to verify the FIPS readiness of the gateway node.
bdarnell
force-pushed
the
debug-fips2
branch
from
f6b60df to
62af449
Compare
|
bors r+ |
|
Build succeeded: |
|
blathers backport 23.2 |
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 883bc5d to blathers/backport-release-23.2-114709: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
cockroach debug enterprise-check-fipsfor detailed diagnostics--enterprise-require-fips-readyto abort if FIPS checks failcrdb_internal.fips_ready()to check at runtimeCloses #114344
Even though FIPS is security-related (and I added a
securitycclpackage to be owned by @cockroachdb/prodsec), this PR is really more build wrangling than anything significant from a security perspective.