feat: expose fips140 tunable via GODEBUG env var - #110
Conversation
Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds a Helm chart toggle to enable Go’s FIPS 140 “only” mode by injecting GODEBUG=fips140=only into the zot container environment.
Changes:
- Introduces a new
fips140value in the chart to control FIPS 140 mode viaGODEBUG. - Updates Deployment/StatefulSet templates to conditionally inject the
GODEBUGenv var. - Adds/extends Helm unit tests to validate
GODEBUGinjection behavior for both Deployment and StatefulSet renders.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/zot/values.yaml | Adds fips140 chart value (and documents intent in values comments). |
| charts/zot/templates/statefulset.yaml | Conditionally injects GODEBUG=fips140=only into StatefulSet container env. |
| charts/zot/templates/deployment.yaml | Conditionally injects GODEBUG=fips140=only into Deployment container env. |
| charts/zot/unittests/statefulset_test.yaml | Adds tests covering GODEBUG behavior in StatefulSet renders. |
| charts/zot/unittests/deployment_test.yaml | Adds a new unit test suite for Deployment renders, including env ordering checks. |
| charts/zot/README.md | Documents the new fips140 value in the generated values table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- toYaml .Values.env | nindent 12 }} | ||
| {{- end }} |
There was a problem hiding this comment.
With fips140: true, this unconditionally injects a GODEBUG env var. If a user also supplies env: [{name: GODEBUG, ...}], the rendered manifest will contain duplicate GODEBUG entries, which is confusing and can lead to unpredictable precedence. Consider either preventing duplicates (filter user env) or clearly defining precedence (e.g., ensure the chart-provided value overrides).
| {{- toYaml .Values.env | nindent 12 }} | |
| {{- end }} | |
| {{- range .Values.env }} | |
| {{- if not (and $.Values.fips140 (eq .name "GODEBUG")) }} | |
| - {{- toYaml . | nindent 14 | trim }} | |
| {{- end }} | |
| {{- end }} | |
| {{- end }} |
There was a problem hiding this comment.
Actually why do we need this separate configuration, if the user can set it in the env vars?
There was a problem hiding this comment.
The separate toggle provides a convenient, well-documented configuration option that automatically handles FIPS 140 mode without requiring users to manually add environment variables. This is better UX than expecting users to know to add env: [{name: GODEBUG, value: "fips140=only"}] themselves.
There was a problem hiding this comment.
IMHO it gives a sense of false confidence that this should work out of the box. While this was and is ultimately something we provide as best-effort.
There was a problem hiding this comment.
IMHO if someone is responsible for implementing FIPS compliance in their company, they should know how it is implemented.
I don't want to encourages people to take this at face value, instead they should try this out do their own testing, see what works, what crashes with fips enabled, decide what they can live with, and open bugs for the bugs they can't live with.
| {{- toYaml .Values.env | nindent 12 }} | ||
| {{- end }} |
There was a problem hiding this comment.
When fips140 is enabled, the chart injects GODEBUG, but users can also set GODEBUG via .Values.env, resulting in duplicate env var entries. Consider de-duping or explicitly documenting/implementing precedence so users don’t accidentally disable/override the intended FIPS setting.
| {{- toYaml .Values.env | nindent 12 }} | |
| {{- end }} | |
| {{- range .Values.env }} | |
| {{- if and $.Values.fips140 (eq .name "GODEBUG") }} | |
| {{- else }} | |
| - {{- toYaml . | nindent 14 | trim }} | |
| {{- end }} | |
| {{- end }} | |
| {{- end }} |
| env: | ||
| {{- if .Values.fips140 }} | ||
| - name: GODEBUG | ||
| value: "fips140=only" | ||
| {{- end }} | ||
| {{- if .Values.env }} | ||
| {{- toYaml .Values.env | nindent 12 }} | ||
| {{- end }} |
There was a problem hiding this comment.
Same issue as in the StatefulSet template: env: is always present, but when fips140 is false and .Values.env is empty/nil, this renders env: null. It’s safer to omit the env: field entirely unless at least one env var is being set (either from fips140 or user-provided env).
…oyment and statefulset templates Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
What type of PR is this?
Which issue does this PR fix:
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades?
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.