Fix OpenPGP key-fingerprint panic under GODEBUG=fips140=only - #458
Fix OpenPGP key-fingerprint panic under GODEBUG=fips140=only#458sameerforge wants to merge 3 commits into
Conversation
47a44b0 to
6273fcb
Compare
|
@joaopapereira Please review the PR. |
joaopapereira
left a comment
joaopapereira
left a comment
There was a problem hiding this comment.
I do have some doubts about the implementation here and the removal of the FIPS check
| keys, err := ReadArmoredKeys(testPublicKey) | ||
| if err != nil { | ||
| t.Fatalf("ReadArmoredKeys: %s", err) | ||
| } | ||
| if len(keys) != 1 { | ||
| t.Fatalf("expected 1 key, got %d", len(keys)) | ||
| } |
There was a problem hiding this comment.
we should be using require.Error() and require.Len() in here instead of t.Fatalf because that is the pattern we have been using throughout carvel
There was a problem hiding this comment.
updated to use require.NoError and require.Len to match the pattern used elsewhere in carvel. Thanks!
| // as a key identifier for later signature lookups (see | ||
| // fetch/git/verification.go); it is not used to verify or trust | ||
| // any signature, so relaxing FIPS enforcement for just this parse | ||
| // is safe. The signature check itself is unaffected and stays enforced. |
There was a problem hiding this comment.
That is not true given the companion change we are wrapping the signature check in WithoutEnforcement too. The comment should be corrected so it doesn't mislead a future reader into thinking verification remains FIPS-enforced.
There was a problem hiding this comment.
You're right, fixed the comment so it accurately reflects that enforcement is relaxed around both operations, scoped narrowly rather than implying verification stays fully enforced.
| // Verifying a signature made with non-FIPS-approved | ||
| // algorithms panics under GODEBUG=fips140=only. | ||
| // Wrapping this call in WithoutEnforcement allows signature | ||
| // verification of legacy signed commits/tags without panicking. | ||
| fips140.WithoutEnforcement(func() { | ||
| _, err = openpgp.CheckArmoredDetachedSignature(publicKeys, target, sig) | ||
| }) |
There was a problem hiding this comment.
This change in itself is a big complicated, because here we are saying that we will no longer verify, if in a FIPS environment, the keys are good or not. Also not sure if we can consider this backwards compatible.
We could do something like parsing the signature packet, read its hash algorithm, and if it's non-FIPS (SHA-1/MD5) return a clear error; otherwise verify normally under enforcement?
Another option would be for us to try to recover the panic and just get an error out, not the greatest option but that would be an option as well
There was a problem hiding this comment.
I think erroring out isn't viable — it would break vendir sync for existing repos with older SHA-1/DSA-signed commits/tags, which is a real production concern for consumers.
So we added a pre-check (hash + public-key algorithm), but instead of erroring, we log a warning naming the non-approved algorithm and let verification proceed. Verification itself is unchanged — the signature/key is still checked and a bad match still fails; we only wrap the call in WithoutEnforcement so it doesn't panic on the non-approved primitive. Outside fips140=only this is a no-op, so it's backwards compatible.
There was a problem hiding this comment.
Thanks for explaining the production concern, that's legitimate and I don't want to make life harder for people with legacy signed history for no reason.
But I don't think warn-and-proceed should be the default here. The algorithm pre-check you already added is doing the hard part, so let's just use it to return an error instead of a log warning, and skip the CheckArmoredDetachedSignature call entirely when the algorithm isn't FIPS-approved. That means we don't need WithoutEnforcement around the actual verify call at all, only around the key-parsing/fingerprint step in armor.go, which I'm fine with since that's just a lookup id and not a trust decision.
That gets us a clean, actionable error under fips140=only instead of a panic or a silent pass that's easy to miss in CI. It also matches what someone turning that flag on is actually asking for. I'd rather fail loudly by default than have vendir quietly trust a SHA-1/DSA signature in a mode whose whole point is to disallow that.
For your production concern, let's add an explicit opt-out, something like --allow-legacy-signatures or a config field, so consumers who know they need to keep syncing repos with older signatures can do that on purpose instead of us deciding it for everyone.
Can you take a pass at reworking it this way? Should be a small change from what's already there, happy to re-review quickly once it's up.
There was a problem hiding this comment.
Done — reworked as suggested. The algorithm pre-check now returns an error by default and short-circuits before CheckArmoredDetachedSignature is ever called, so non-approved signatures never reach the verify call at all. WithoutEnforcement is gone from the default path entirely; it's now only used inside the new opt-in (when a caller explicitly sets it), since that's the one case where we do need to let a non-approved algorithm through to verify. Went with a config field (verification.allowLegacySignatures) over a CLI flag for the opt-out, since it composes better with how some of our consumers drive vendir purely through YAML config rather than extra CLI args.
There was a problem hiding this comment.
Note : kapp-controller (a primary consumer of vendir) invokes vendir as a subprocess, driving it entirely through a generated YAML configuration piped over stdin (vendir sync -f - --lock-file /dev/null) — it has no mechanism for passing additional CLI flags, which is why allowLegacySignatures is exposed as a configuration field here rather than a flag.
We also confirmed that kapp-controller does not currently set verification at all when building the git configuration it passes to vendir. Its AppFetchGit API type has no signature-verification concept today, so the code path this PR changes — including the previously-panicking behavior — is not reachable through any App or PackageInstall resource in existing kapp-controller deployments. Wiring up verification support (and, by extension, allowLegacySignatures) in kapp-controller would be net-new work, tracked separately from this PR.
5d136b7 to
1b7539c
Compare
Parsing public keys via openpgp.ReadArmoredKeyRing unconditionally computes an RFC 4880 V4 key fingerprint using SHA-1, which panics under GODEBUG=fips140=only before any signature is ever checked. The fingerprint is used only as a key identifier for later signature lookups; it plays no part in verifying or trusting a signature. Wrap the parsing call in crypto/fips140.WithoutEnforcement so key parsing succeeds under strict FIPS enforcement without weakening signature verification, which continues to enforce FIPS independently. Adds a unit test covering key parsing under GODEBUG=fips140=only. Signed-off-by: Sameer <sameer.khan@broadcom.com>
1b7539c to
8f90ad1
Compare
Verifying a signature made with a non-FIPS-approved hash or public-key algorithm (SHA-1, MD5, DSA) panics under GODEBUG=fips140=only. Under strict enforcement, reject such signatures outright by default, with an error naming the exact non-approved algorithm(s) and pointing at the new verification.allowLegacySignatures configuration field. Add verification.allowLegacySignatures to DirectoryContentsGitVerification for consumers who need to keep syncing repositories with legacy-signed commits/tags under FIPS enforcement: when set, a non-approved algorithm only logs a warning instead of failing, and the actual CheckArmoredDetachedSignature call is wrapped in crypto/fips140.WithoutEnforcement (only reached in this opt-in case, since it would otherwise panic on the same non-approved algorithm). Outside of strict enforcement, or when the signature already uses an approved algorithm, behavior is unchanged and WithoutEnforcement is never invoked. This is exposed as a configuration field rather than a CLI flag so that systems driving vendir purely through a YAML configuration document (rather than additional CLI arguments) can set it. Adds unit tests covering the hash/pubkey-algorithm pre-check and its reject/warn behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Sameer <sameer.khan@broadcom.com>
- Update Go version to 1.26.5 in go.mod - Bump carvel.dev/imgpkg dependency to v0.48.1 - Update golangci-lint to align with Go version - Run `go mod tidy` and `go mod vendor` to update go.sum and vendor/ Signed-off-by: Sameer <sameer.khan@broadcom.com>
8f90ad1 to
2517fc5
Compare
|
@joaopapereira Updated the PR as per you review comments. PTAL! |
|
@joaopapereira Please review. |
3 participants
Summary
Fixes two runtime panics in vendir's OpenPGP git-signature verification when running under
GODEBUG=fips140=only(Go native FIPS 140-3 strict enforcement):Changes
1. Key-fingerprint panic (
pkg/vendir/openpgparmor/armor.go)Parsing an OpenPGP key via
openpgp.ReadArmoredKeyRingcomputes an RFC 4880 v4 fingerprint using SHA-1 as part of key identification, not as a trust decision. This call is now wrapped incrypto/fips140.WithoutEnforcement, since the fingerprint is only used as a lookup key for later signature verification — it plays no part in verifying or trusting a signature, which continues to be enforced independently.2. Non-FIPS-approved signature algorithms (
pkg/vendir/fetch/git/verification.go)Before verifying a signature, vendir now inspects its declared hash and public-key algorithm. Under
GODEBUG=fips140=only:CheckArmoredDetachedSignatureis never called in this case, so nothing reaches the code path that would otherwise panic.verification.allowLegacySignatures: a new configuration field onDirectoryContentsGitVerification. When set totruefor a given git source, a non-approved algorithm is downgraded from an error to a logged warning, and verification proceeds — the actualCheckArmoredDetachedSignaturecall is wrapped incrypto/fips140.WithoutEnforcementonly in this opt-in case, since that's the only scenario where a non-approved algorithm reaches it.This addresses the production concern that outright failing on legacy-signed history (SHA-1/DSA-signed commits or tags) would break existing consumers with such history in their tree — they can now opt in deliberately via config, rather than vendir silently trusting or unconditionally rejecting legacy signatures for everyone.
allowLegacySignaturesis exposed as a configuration field rather than a CLI flag, so that systems which drive vendir purely through a YAML configuration document (rather than additional CLI arguments) are able to set it.Testing
pkg/vendir/fetch/git/verification_test.go).pkg/vendir/openpgparmor/armor_test.go).go build,go vet,go test) validated both with and withoutGODEBUG=fips140=only, confirming parity for non-FIPS builds and correct reject/warn behavior under strict enforcement.Backwards compatibility
No behavior change for any existing configuration:
Note on kapp-controller usage
kapp-controller (a primary downstream consumer of vendir) invokes
vendiras a subprocess, driving it entirely through a generated YAML configuration piped over stdin (vendir sync -f - --lock-file /dev/null) — it has no mechanism for passing additional CLI flags, which is whyallowLegacySignaturesis exposed as a configuration field here rather than a flag.We also confirmed that kapp-controller does not currently set
verificationat all when building the git configuration it passes to vendir. ItsAppFetchGitAPI type has no signature-verification concept today, so the code path this PR changes — including the previously-panicking behavior — is not reachable through anyApporPackageInstallresource in existing kapp-controller deployments. Wiring up verification support (and, by extension,allowLegacySignatures) in kapp-controller would be net-new work, tracked separately from this PR.