git: allow signing commits with SSH key#1222
Draft
hiddeco wants to merge 44 commits into
Draft
Conversation
- 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 Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
…tureType' Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
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>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
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>
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>
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>
Re-exports the go-git Signer interface from the signature package so consumers can refer to it without importing go-git directly. Prepares for the upcoming NewOpenPGPSigner and NewSSHSigner constructors. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Wraps an *openpgp.Entity in a Signer so it can be used through the new generic signing path on CommitOptions. Produces the same ASCII-armored detached signature as go-git's internal gpgSigner, preserving existing commit-signing output bit-for-bit. The concrete OpenPGPSigner type is exported so callers can type-assert a Signer returned by NewOpenPGPSigner and distinguish it from other Signer implementations. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Wraps an SSH private key in a Signer that produces SSHSIG-armored signatures with namespace "git" and SHA-512 hash, matching Git's defaults for SSH-signed commits. The concrete SSHSigner type is exported so callers can type-assert a Signer returned by NewSSHSigner. This commit covers the unencrypted- key happy path; encrypted keys and the algorithm allowlist land in follow-up commits. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
OpenSSH-format private keys cannot be classified as encrypted before attempting to parse. Detect *gossh.PassphraseMissingError, require a passphrase, and retry through ParsePrivateKeyWithPassphrase. Wrong- passphrase parse failures surface with the generic "could not parse" wrap; missing-passphrase errors are explicit. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Allowlist ssh-ed25519, ecdsa-sha2-nistp256/384/521, and ssh-rsa with key size at least 2048 bits. DSA and undersized RSA produce signatures modern OpenSSH refuses to verify, so reject them at construction time instead of letting downstream verify silently fail. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
CommitOptions.Signer changes from *openpgp.Entity to the go-git Signer interface re-exported from the signature package, and WithSigner takes the same type. The gogit client forwards the value to go-git's generic Signer field, which takes precedence over the legacy typed SignKey. This is a breaking API change in pkg/git/repository; consumers must migrate to signature.NewOpenPGPSigner or signature.NewSSHSigner. Since pkg/git is still pre-1.0, the change lands in a coordinated minor. Refs #398[1]. [1]: #398 Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Calls gogit.Client.Commit with WithSigner(NewOpenPGPSigner(...)) and verifies the resulting commit's gpgsig header through signature.VerifyPGPSignature. The table-driven harness leaves room for SSH and unsigned cases to land as new rows in follow-up commits. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Adds an ed25519 row to TestCommit_WithSigner that exercises NewSSHSigner and verifies the SSHSIG-armored gpgsig header through signature.VerifySSHSignature. Asserts the signature uses the "git" namespace and round-trips against the corresponding authorized_keys public key. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Adds an ecdsa-sha2-nistp256 row to TestCommit_WithSigner so the non-ed25519 SSH-signing path is exercised explicitly. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Adds a nil-signer row to TestCommit_WithSigner that pins the nil- interface guard at gogit/client.go: a typed-nil reaching go-git's commit path would panic on signer.Sign. The row also documents the contract that WithSigner(nil) produces an empty gpgsig header. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Adds the supported-algorithms paragraph to the NewSSHSigner godoc so the public surface signals up front which keys it will accept. No behaviour change; validateSSHSigningKey already enforces this. Assisted-by: claude-code/claude-opus-4-7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
NewSSHSigner now returns a typed sentinel error instead of a bare errors.New for the encrypted-key-without-passphrase case, so consumers can branch on it via errors.Is rather than matching the literal error string. The wording is preserved verbatim so existing log scrapers and tests continue to match. Image-automation-controller currently string-matches this error to translate it into its user-facing "missing 'password' field" message. Once this lands and a new pkg/git minor is tagged, the consumer can switch to errors.Is.
This was referenced May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on top of #1141