fix(operator): set GODEBUG explicitly even when fipsMode=off - #349
fix(operator): set GODEBUG explicitly even when fipsMode=off#349nshidlansik wants to merge 2 commits into
Conversation
GOFIPS140=v1.0.0 at build time makes the operator binary's own
compiled-in GODEBUG default fips140=on (confirmed via go version -m:
DefaultGODEBUG=fips140=on, and per Go's own docs: "[GOFIPS140] enable[s]
FIPS 140-3 mode by default" and the fips140 GODEBUG option "defaults to
off unless GOFIPS140 is set at build time").
deployment.yaml wrapped the GODEBUG env var in
{{- if ne .Values.fipsMode "off" }} to avoid setting it unnecessarily,
per review on #335. That meant fipsMode=off (the chart default) set no
GODEBUG at all, so the binary fell through to its compiled-in default
of fips140=on instead of actually running off: off and on were
identical, and every default install silently ran FIPS self-tests and
the FIPS-restricted TLS cipher/curve list.
Always render GODEBUG explicitly on the operator container so fipsMode
maps to the runtime mode it names. crd-upgrader-job.yaml is unaffected:
its default image (registry.k8s.io/kubectl) is not built with
GOFIPS140, so omitting GODEBUG there genuinely means off already.
Found by Aviad Hayumi in review after #335 merged.
Signed-off-by: Nir Shidlansik <nshidlansik@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe operator Go module adds a ChangesFIPS configuration
Priority: ⬇️ Low — Defer this narrow operator configuration fix because it only corrects the default FIPS mode behavior without supplied evidence of broader product impact. Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The operator now defaults FIPS mode to off while runtime configuration can still enable it. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
A rabbit found a setting bright Comment |
yuval-gr
left a comment
There was a problem hiding this comment.
The current fix works but leaves an unnecessary env var in place (my original comment). Please change it to GODEBUG=fips140=off as Aviad wrote above, and keep the if condition on the env var.
Revert the previous approach (always render GODEBUG on the operator
container) and fix the actual root cause instead: GOFIPS140=v1.0.0
overrides the fips140 GODEBUG default to on, so add a "godebug
fips140=off" directive to operator/go.mod to override that back to
off. Restores the {{- if ne .Values.fipsMode "off" }} guard in
deployment.yaml (skip the env var entirely when off, since off is now
the binary's own real default again).
Verified with crypto/fips140.Enabled() run inside the operator module:
GOFIPS140=v1.0.0, no GODEBUG at runtime -> Enabled() == false (was
true before this fix)
GOFIPS140=v1.0.0, GODEBUG=fips140=on -> Enabled() == true
GOFIPS140=v1.0.0, GODEBUG=fips140=only -> Enabled() == true
Runtime GODEBUG still overrides the go.mod default, as expected.
Signed-off-by: Nir Shidlansik <nshidlansik@nvidia.com>
What does this PR do?
Fixes a bug in the FIPS 140-3 support added by #335:
fipsMode: off(thechart default) silently ran the operator with FIPS mode enabled instead of
disabled.
GOFIPS140=v1.0.0at build time makes the operator binary's own compiled-inGODEBUGdefaultfips140=on(per Go's own docs:GOFIPS140"enable[s]FIPS 140-3 mode by default", and the
fips140GODEBUG option "defaults tooff unless GOFIPS140 is set at build time").
deployment.yamlwrapped theGODEBUGenv var in{{- if ne .Values.fipsMode "off" }}to avoid settingit unnecessarily, per review on #335. That meant
fipsMode=offset noGODEBUGat all, so the binary fell through to its compiled-in default offips140=oninstead of actually running off.Found by Aviad Hayumi in review after #335 merged.
Fix: add a
godebug fips140=offdirective tooperator/go.mod, whichoverrides
GOFIPS140's default back to off at compile time. The chart's{{- if ne .Values.fipsMode "off" }}guard is kept as-is (an earlier versionof this PR removed it and always rendered
GODEBUGinstead, but fixing thebinary's own default is more direct and keeps the chart's original,
reviewed-and-requested shape).
Verified with
crypto/fips140.Enabled()run inside the operator module:Runtime
GODEBUGstill overrides thego.moddefault, as expected.Related issue(s)
Relates to #334
Checklist
git commit -s)make check)Summary by CodeRabbit