Add SSH signature verification for git commits by bb-Ricardo · Pull Request #1141 · fluxcd/pkg

bb-Ricardo · 2026-02-27T12:04:07Z

This PR adds support of SSH signature validation.

resolves: fluxcd/flux2#4145

adds new package git/signatures
adds validation of SSH signed commits to ssh_signature.go
moves GPG signature validation to gpg_signature.go
adds text fixtures for all SSH and GPG key types including commits and signatures
adds tests for all key/signature combinations
adds wrapper for "Verify(keyRings ...string)" function

stefanprodan · 2026-02-28T08:04:40Z

@bb-Ricardo please run make tidy and force push to unblock the test.

bb-Ricardo · 2026-03-10T07:40:56Z

Hi, was just wondering if this PR is sufficient or if anything else is needed?

stefanprodan · 2026-03-10T08:30:24Z

Hey @hiddeco and @pjbgf could you please review?

stefanprodan

Tests are failing with open signatures/... no such file or directory please run make test-git before pushing commits.

bb-Ricardo · 2026-03-10T08:54:15Z

sorry for that, now the tests passed locally.

bb-Ricardo · 2026-03-17T19:52:09Z

@hiddeco - would you mind to have a look at this PR again? Thank you.

pjbgf

@bb-Ricardo thanks for working on this. Overall LGTM, however I'd remove any references to DSA as per thread below.

stefanprodan · 2026-03-19T10:35:46Z

@bb-Ricardo please use rebase not merge. Undo the last merge, and properly rebase your fork, GH has a button for this if you go to your forked repo.

bb-Ricardo · 2026-03-19T11:32:46Z

@bb-Ricardo please use rebase not merge. Undo the last merge, and properly rebase your fork, GH has a button for this if you go to your forked repo.

Yes, sorry. I used the seemingly convenient button GitHub offered in this PR. I removed all occurrences of GPG DSA keys in the tests and test fixtures.
Also rebased onto the current main.

bb-Ricardo · 2026-04-20T08:31:00Z

Hi,

was wondering if anything else is needed. Thank you

Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com> Signed-off-by: Ricardo <ricardo@bitchbrothers.com> Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

…alfored key is included Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

…EADME Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

…ommits and tags Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

hiddeco · 2026-05-28T08:39:59Z

 // with, or an error.
-func (t *Tag) Verify(keyRings ...string) (string, error) {
-	fingerprint, err := verifySignature(t.Signature, t.Encoded, keyRings...)
+func (t *Tag) VerifyGPG(keyRings ...string) (string, error) {


It's slightly strange for this to be called VerifyGPG while it is paired with a IsPGPSigned method.

the original function name was verify and I added the signature type to the name. But you are right, I missed the different naming.

I changed the naming.

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

When a key ring in VerifyPGPSignature or VerifySSHSignature failed to parse, the read error was recorded and the iteration skipped. The post-loop return then surfaced that error unconditionally, masking a later parseable key ring whose keys did not match the signer with an "unable to read armored key ring" message. Track whether any verification was attempted. Fall back to the recorded parse error only when no key ring could be parsed; otherwise the no-match error takes precedence. Apply the same fix on both the openPGP and SSH paths and add regression tests for the malformed-then-valid-non-matching sequence. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

Verification previously returned all error conditions as opaque fmt.Errorf strings, forcing callers to compare on substrings. The per-key sshsig sentinels (ErrPublicKeyMismatch, ErrNamespaceMismatch, ErrUnsupportedHashAlgorithm) were swallowed once the loop completed without a match. Introduce ErrSignatureEmpty, ErrPayloadEmpty, ErrSignatureFormat and ErrNoMatchingKey in the signature package. The early-return paths wrap these with %w so callers can branch via errors.Is. For the SSH path, each per-key sshsig error is deduplicated and joined into the final ErrNoMatchingKey-wrapping error so the underlying sentinels remain visible in the chain. The user-visible error message becomes "unable to verify payload: signature is empty" instead of "...as the provided signature is empty"; tests asserting on the long form are updated. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

Git's gpgsig field carries only detached signatures, which use PGP SIGNATURE armor. PGP MESSAGE armor covers encrypted bodies and inline cleartext signatures, none of which openpgp's CheckArmoredDetachedSignature can verify. Detecting it at the signature-type layer therefore produced a misleading "no matching key" error downstream rather than an honest "this is not a detached signature". Drop the PGP MESSAGE entry from the prefix list. GetSignatureType now classifies MESSAGE-armored input as unknown, and VerifyPGPSignature returns ErrSignatureFormat for it. Tests covering MESSAGE detection are flipped to assert the new expectation rather than removed, so the deliberate exclusion stays under coverage. While here, trim leading and trailing whitespace once at the top of VerifyPGPSignature and VerifySSHSignature so the format-detection helpers (which already TrimSpace internally) and the underlying armor decoders operate on identical input. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

The doc comments around the verification API drifted during review and now contradict either the code or each other in several places. Pure documentation plus two mechanical identifier renames: * Deprecation notices on Commit.Verify and Tag.Verify are reformatted into the conventional two-paragraph shape (description, blank line, Deprecated: paragraph) that staticcheck SA1019 and gopls look for. * The PGP-path methods describe their return value as the openPGP key ID, not the fingerprint. Under RFC 4880 these differ -- the function literally calls KeyIdString() -- and a consumer indexing by "fingerprint" otherwise got a 16-hex key ID for PGP and a SHA256: full fingerprint for SSH. * The doc previously attached to var X509SignaturePrefix actually described the IsX509Signature function. Move it onto the function and give the variable a proper description. * Commit.SignatureType and Tag.SignatureType enumerate all five values GetSignatureType can return (openpgp, ssh, x509, empty, unknown). * The Signature field doc on Commit and Tag no longer pretends it only stores PGP signatures. * ParseAuthorizedKeys documents its fail-fast semantics. * The build.go package-private signature helper is renamed to toGitSignature so it does not shadow the signature package name. * The test variable keyRingFingerprintFixture is renamed to keyRingKeyIDFixture. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

PGPSignaturePrefix, SSHSignaturePrefix and X509SignaturePrefix were exported `var []string` values used only by the IsXxxSignature helpers in the same package. Being exported and mutable they constituted a forever-API hazard: any consumer could prepend or replace entries and globally break signature detection across every importer. The slices carry no information a caller would ever need beyond what IsXxxSignature already exposes. Lowercase the three vars. No behavioural change; the detection helpers remain exported and continue to be the supported entry point. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

The two helpers in git/testutils (ParseCommitFromFixture, ParseTagFromFixture) are used only by tests inside the git module yet they lived under a non-internal path and were therefore part of the module's public API. Move the package to git/internal/testutil and drop the trailing `s` to match the singular form already used elsewhere under internal/. The helpers themselves are unchanged; only the import path and the package name move. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

The review surfaced a handful of scenarios that lived in spirit in the existing tables but were not actually exercised: * Tampered-payload coverage for both VerifyPGPSignature and VerifySSHSignature: prepend a byte to the encoded payload and assert the verification refuses (ErrNoMatchingKey). * Wrong SSH namespace: generate an ed25519 key in-test, sign the payload with namespace "file" via sshsig.Sign, and confirm the error chain contains both ErrNoMatchingKey and sshsig.ErrNamespaceMismatch. * Deprecated Verify on SSH input: route SSH-signed fixtures through Commit.Verify and Tag.Verify to lock in the documented refusal. * SignatureType / fixture parity: every PGP commit fixture must report "openpgp", every SSH fixture "ssh", unsigned ones "empty". * ParseAuthorizedKeys mixed-validity: the new "valid then invalid then valid" case pins the documented fail-fast contract. Also fix the tautological ContainSubstring assertion in TestCommit_VerifyPGP -- both sides are key-ID strings, so the relation should be Equal. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

Both generate_*.sh scripts wrote directly into the test-data directory next to the script. A half-failed run could leave that directory in a partial state, and adding a new key type silently left orphan fixtures from the old set unless the caller knew the right rm incantation. Restructure the scripts to write into a fresh $(mktemp -d) staging directory and atomically swap that into the output directory -- everything except the script itself, the README, and any dotfiles -- on successful completion. This makes the run idempotent without an artefact-name allow-list: anything no longer produced is implicitly deleted, and adding a new key type just adds a new file to staging. A failed run leaves the prior good fixtures untouched. While here, expand the EXIT trap to also catch INT/TERM, trim the dependency lists to the load-bearing tools (gpg/git for GPG; ssh-keygen/git for SSH), rename create_verified_signers to create_temp_allowed_signers to reflect its actual lifetime, and update the SSH README to drop the misleading "Verified Signers Format" section. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

hiddeco

This LGTM, thanks for the patience @bb-Ricardo. I'd preferably have one other person sign-off, just to ensure I didn't miss anything.

bb-Ricardo · 2026-05-28T17:13:30Z

Thank you for this extensive review. You pay attention to get all details correct, highly appreciate that.

Hope I can deliver better quality for the source-controller implementation.

Would you mind having a look at the proposed implementation and share your opinion?: fluxcd/source-controller#1996

Thank you

bb-Ricardo requested review from aryan9600 and hiddeco as code owners February 27, 2026 12:04

bb-Ricardo force-pushed the rb-feature/adds-ssh-signature-validation branch 2 times, most recently from 96523af to 1561774 Compare February 28, 2026 00:35

bb-Ricardo requested a review from a team as a code owner February 28, 2026 08:48

bb-Ricardo force-pushed the rb-feature/adds-ssh-signature-validation branch from eedb46c to 048e862 Compare February 28, 2026 09:07

bb-Ricardo mentioned this pull request Feb 28, 2026

Improve UX for verifying commits and Tags go-git/go-git#1869

Open

2 tasks

stefanprodan reviewed Feb 28, 2026

View reviewed changes

Comment thread git/signatures/ssh_signature.go Outdated

Comment thread git/signatures/signature.go Outdated

Comment thread git/signature/ssh_signature.go

bb-Ricardo mentioned this pull request Feb 28, 2026

Feature Request: support validation of additional git commit signature types fluxcd/source-controller#1996

Open

stefanprodan reviewed Feb 28, 2026

View reviewed changes

Comment thread git/signatures/ssh_signature_test.go Outdated

Comment thread git/signatures/ssh_signature_test.go Outdated

Comment thread git/signatures/ssh_signature.go Outdated

bb-Ricardo force-pushed the rb-feature/adds-ssh-signature-validation branch from e247a34 to 5e6ab4d Compare March 2, 2026 23:16

stefanprodan added enhancement New feature or request area/git Git and SSH related issues and pull requests labels Mar 10, 2026

stefanprodan reviewed Mar 10, 2026

View reviewed changes

hiddeco reviewed Mar 10, 2026

View reviewed changes

Comment thread git/signatures/ssh_signature.go Outdated

Comment thread git/signatures/ssh_signature.go Outdated

Comment thread git/signatures/signature.go Outdated

bb-Ricardo force-pushed the rb-feature/adds-ssh-signature-validation branch from 83338dc to 51ceefd Compare March 10, 2026 14:30

pjbgf requested changes Mar 19, 2026

View reviewed changes

bb-Ricardo force-pushed the rb-feature/adds-ssh-signature-validation branch from b6f0ece to bd9abd8 Compare March 19, 2026 11:30

bb-Ricardo requested a review from pjbgf March 23, 2026 15:00

bb-Ricardo force-pushed the rb-feature/adds-ssh-signature-validation branch from bd9abd8 to fa037d1 Compare April 9, 2026 04:59

bb-Ricardo requested a review from stefanprodan April 9, 2026 05:00

bb-Ricardo force-pushed the rb-feature/adds-ssh-signature-validation branch from fa037d1 to 0811c24 Compare April 20, 2026 08:30

bb-Ricardo and others added 13 commits May 28, 2026 08:35

Update git/signatures/testdata/gpg_signatures/generate_gpg_fixtures.sh

8e1ae7c

Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com> Signed-off-by: Ricardo <ricardo@bitchbrothers.com> Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

removes insecure DSA keys from signature test

b78fdf1

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

updates git go.mod dependencies

54214d5

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

fixes tests for unsigned tags

36fc961

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

renames package to signature and fixes requested changes

5e870f6

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

switches signarture type from public to private

2dfb2d1

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

moves buildTag and buildCommitWithRef to internal/build package

1661b58

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

improves signature validation to test all provided keys/keyrings if m…

fc4b507

…alfored key is included Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

removes gnupg ed448 generated key test fixtures and adds mention to R…

71d7b3f

…EADME Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

removes verfied signers from ssh signature test fixtures

8322227

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

updates test fixtures generation scripts and regenerated test fixtures

5a4d56c

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

fixes text fixtures creation description and adds test for unsigned c…

44f9b66

…ommits and tags Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

adds more test for malfromed signatures

7495ab3

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

bb-Ricardo force-pushed the rb-feature/adds-ssh-signature-validation branch from 8ef0280 to 7495ab3 Compare May 28, 2026 06:35

hiddeco reviewed May 28, 2026

View reviewed changes

bb-Ricardo and others added 10 commits May 28, 2026 11:54

renames VerifyGPG to VerifyPGP

cf11371

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

git: fixes issue with order of imports

656ea35

Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>

hiddeco approved these changes May 28, 2026

View reviewed changes

This was referenced May 29, 2026

git: allow signing commits with SSH key #1222

Draft

Support signing commits with SSH key fluxcd/image-automation-controller#1035

Draft

Allow signing commits using SSH key fluxcd/flux2#5920

Draft

Conversation

bb-Ricardo commented Feb 27, 2026

Uh oh!

stefanprodan commented Feb 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bb-Ricardo commented Mar 10, 2026

Uh oh!

stefanprodan commented Mar 10, 2026

Uh oh!

stefanprodan left a comment

Choose a reason for hiding this comment

Uh oh!

bb-Ricardo commented Mar 10, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bb-Ricardo commented Mar 17, 2026

Uh oh!

pjbgf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefanprodan commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bb-Ricardo commented Mar 19, 2026

Uh oh!

bb-Ricardo commented Apr 20, 2026

Uh oh!

hiddeco May 28, 2026

Choose a reason for hiding this comment

Uh oh!

bb-Ricardo May 28, 2026

Choose a reason for hiding this comment

Uh oh!

hiddeco left a comment

Choose a reason for hiding this comment

Uh oh!

bb-Ricardo commented May 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stefanprodan commented Mar 19, 2026 •

edited

Loading