From 274066905f12d3892a4ff828318acfc5629f0eda Mon Sep 17 00:00:00 2001 From: Bas Nijholt Date: Tue, 30 Jun 2026 10:19:29 -0700 Subject: [PATCH 1/4] feat: support SSH commit signature verification --- docs/generated-module-options.md | 23 +- docs/howtos.md | 16 +- go.mod | 2 +- internal/config/config.go | 11 +- internal/repository/git.go | 359 +++++++++++++++++++++++++ internal/repository/git_test.go | 264 ++++++++++++++++++ internal/repository/repository.go | 37 ++- internal/repository/repository_test.go | 51 ++++ internal/types/types.go | 10 +- nix/comin-config.nix | 3 + nix/module-options-doc.nix | 2 +- nix/module-options.nix | 5 + 12 files changed, 756 insertions(+), 27 deletions(-) diff --git a/docs/generated-module-options.md b/docs/generated-module-options.md index ae2bf298..ee77e35f 100644 --- a/docs/generated-module-options.md +++ b/docs/generated-module-options.md @@ -889,6 +889,27 @@ signed integer +## services\.comin\.sshAllowedSignersPath + + + +An OpenSSH allowed signers file path used to verify SSH-signed Git commits\. + + + +*Type:* +null or string + + + +*Default:* + +```nix +null +``` + + + ## services\.comin\.submodules @@ -932,5 +953,3 @@ null or string ```nix null ``` - - diff --git a/docs/howtos.md b/docs/howtos.md index 1033b82b..5101b027 100644 --- a/docs/howtos.md +++ b/docs/howtos.md @@ -65,11 +65,23 @@ configuration to the new machine. ## Check Git commit signatures The option `services.comin.gpgPublicKeyPaths` allows to declare a list -of GPG public keys. If `services.comin.gpgPublicKeyPaths != []`, comin **only** evaluates commits signed -by one of these GPG keys. Note only the last commit needs to be signed. +of GPG public keys. If `services.comin.gpgPublicKeyPaths != []` or +`services.comin.sshAllowedSignersPath` is set, comin only evaluates commits +signed by a configured commit-signing trust source. Note only the last commit +needs to be signed. The file containing a GPG public key has to be created with `gpg --armor --export alice@cyb.org`. +The option `services.comin.sshAllowedSignersPath` can also be used to verify +SSH-signed commits with an OpenSSH allowed signers file. For example: + +```text +alice@example.org ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAA... +``` + +comin supports public key entries and the optional `namespaces=` restriction; +other allowed signers options are rejected. + ## How to deploy a nix-darwin configuration diff --git a/go.mod b/go.mod index a405b6c5..4f45f403 100644 --- a/go.mod +++ b/go.mod @@ -15,6 +15,7 @@ require ( github.com/sirupsen/logrus v1.9.3 github.com/spf13/cobra v1.8.0 github.com/stretchr/testify v1.9.0 + golang.org/x/crypto v0.39.0 google.golang.org/grpc v1.75.0 google.golang.org/protobuf v1.36.9 gopkg.in/yaml.v2 v2.4.0 @@ -77,7 +78,6 @@ require ( github.com/tadvi/systray v0.0.0-20190226123456-11a2b8fa57af // indirect github.com/xanzy/ssh-agent v0.3.3 // indirect github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect - golang.org/x/crypto v0.39.0 // indirect golang.org/x/exp v0.0.0-20240613232115-7f521ea00fb8 // indirect golang.org/x/mod v0.25.0 // indirect golang.org/x/net v0.41.0 // indirect diff --git a/internal/config/config.go b/internal/config/config.go index 8d0a4752..9d38a0da 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -78,10 +78,11 @@ func Read(path string) (config types.Configuration, err error) { func MkGitConfig(config types.Configuration) types.GitConfig { return types.GitConfig{ - Path: filepath.Join(config.StateDir, "repository"), - Dir: config.RepositorySubdir, - Remotes: config.Remotes, - GpgPublicKeyPaths: config.GpgPublicKeyPaths, - Submodules: config.Submodules, + Path: filepath.Join(config.StateDir, "repository"), + Dir: config.RepositorySubdir, + Remotes: config.Remotes, + GpgPublicKeyPaths: config.GpgPublicKeyPaths, + SshAllowedSignersPath: config.SshAllowedSignersPath, + Submodules: config.Submodules, } } diff --git a/internal/repository/git.go b/internal/repository/git.go index 4443ca65..dabd3ae0 100644 --- a/internal/repository/git.go +++ b/internal/repository/git.go @@ -1,10 +1,17 @@ package repository import ( + "bytes" "context" + "crypto/sha256" + "crypto/sha512" + "encoding/pem" "fmt" + "io" "os" + "strings" "time" + "unicode" "github.com/ProtonMail/go-crypto/openpgp" "github.com/go-git/go-git/v5" @@ -14,6 +21,7 @@ import ( "github.com/go-git/go-git/v5/plumbing/transport/http" "github.com/nlewo/comin/internal/types" "github.com/sirupsen/logrus" + "golang.org/x/crypto/ssh" ) func getRemoteCommitHash(r repository, remote, branch string) *plumbing.Hash { @@ -208,3 +216,354 @@ func commitSignedBy(r *git.Repository, commitId string, publicKeys []string) (si } return nil, fmt.Errorf("commit %s is not signed", commitId) } + +func commitSignedByTrustedKey(r *git.Repository, commitId string, gpgPublicKeys []string, sshAllowedSigners string) (string, error) { + var verifyErr error + if len(gpgPublicKeys) > 0 { + entity, err := commitSignedBy(r, commitId, gpgPublicKeys) + if err == nil { + return entity.PrimaryIdentity().Name, nil + } + verifyErr = err + } + if sshAllowedSigners != "" { + signedBy, err := commitSignedBySSH(r, commitId, sshAllowedSigners) + if err == nil { + return signedBy, nil + } + verifyErr = err + } + if verifyErr != nil { + return "", verifyErr + } + return "", fmt.Errorf("commit %s is not signed", commitId) +} + +type sshAllowedSigner struct { + principal string + key ssh.PublicKey + namespaces []string +} + +func commitSignedBySSH(r *git.Repository, commitId string, allowedSigners string) (string, error) { + commit, err := r.CommitObject(plumbing.NewHash(commitId)) + if err != nil { + return "", err + } + if strings.TrimSpace(allowedSigners) == "" { + return "", fmt.Errorf("commit %s is not signed", commitId) + } + if !strings.HasPrefix(strings.TrimSpace(commit.PGPSignature), "-----BEGIN SSH SIGNATURE-----") { + return "", fmt.Errorf("commit %s is not signed", commitId) + } + + signature, err := parseSSHSignature(commit.PGPSignature) + if err != nil { + return "", fmt.Errorf("commit %s has an invalid SSH signature: %w", commitId, err) + } + if signature.namespace != "git" { + return "", fmt.Errorf("commit %s has SSH signature namespace %q instead of git", commitId, signature.namespace) + } + + encoded := &plumbing.MemoryObject{} + if err := commit.EncodeWithoutSignature(encoded); err != nil { + return "", err + } + reader, err := encoded.Reader() + if err != nil { + return "", err + } + defer reader.Close() // nolint:errcheck + payload, err := io.ReadAll(reader) + if err != nil { + return "", err + } + signedData, err := sshSignedData(signature.namespace, signature.reserved, signature.hashAlgorithm, payload) + if err != nil { + return "", err + } + + signers, err := parseSSHAllowedSigners(allowedSigners) + if err != nil { + return "", err + } + for _, signer := range signers { + if !signer.allowsNamespace(signature.namespace) { + continue + } + if !bytes.Equal(signer.key.Marshal(), signature.publicKey) { + continue + } + if err := signer.key.Verify(signedData, signature.signature); err == nil { + logrus.Debugf("Commit %s signed by %s", commitId, signer.principal) + return signer.principal, nil + } + } + return "", fmt.Errorf("commit %s is not signed", commitId) +} + +func parseSSHAllowedSigners(allowedSigners string) ([]sshAllowedSigner, error) { + signers := []sshAllowedSigner{} + for lineNumber, line := range strings.Split(allowedSigners, "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + principalField, rest, ok := splitFirstField(line) + if !ok { + return nil, fmt.Errorf("failed to read the SSH allowed signer on line %d", lineNumber+1) + } + principal, err := sshPrincipalFromPatternList(principalField) + if err != nil { + return nil, fmt.Errorf("failed to read the SSH allowed signer on line %d: %w", lineNumber+1, err) + } + options, keyInput, err := splitSSHAllowedSignersRest(rest) + if err != nil { + return nil, fmt.Errorf("failed to read the SSH allowed signer on line %d: %w", lineNumber+1, err) + } + namespaces, err := sshNamespacesFromOptions(options) + if err != nil { + return nil, fmt.Errorf("failed to read the SSH allowed signer on line %d: %w", lineNumber+1, err) + } + key, _, keyOptions, keyRest, err := ssh.ParseAuthorizedKey([]byte(keyInput)) + if err != nil { + return nil, fmt.Errorf("failed to read the SSH allowed signer on line %d: %w", lineNumber+1, err) + } + if len(keyOptions) > 0 { + return nil, fmt.Errorf("failed to read the SSH allowed signer on line %d: unsupported SSH allowed signer option %q", lineNumber+1, keyOptions[0]) + } + if strings.TrimSpace(string(keyRest)) != "" { + return nil, fmt.Errorf("failed to read the SSH allowed signer on line %d: unsupported trailing data", lineNumber+1) + } + if _, ok := key.(*ssh.Certificate); ok { + return nil, fmt.Errorf("failed to read the SSH allowed signer on line %d: SSH certificates are not supported", lineNumber+1) + } + signers = append(signers, sshAllowedSigner{ + principal: principal, + key: key, + namespaces: namespaces, + }) + } + if len(signers) == 0 { + return nil, fmt.Errorf("no SSH allowed signers found") + } + return signers, nil +} + +func splitFirstField(line string) (string, string, bool) { + i := strings.IndexFunc(line, unicode.IsSpace) + if i < 0 { + return "", "", false + } + return line[:i], strings.TrimLeftFunc(line[i:], unicode.IsSpace), true +} + +func splitSSHAllowedSignersRest(rest string) (string, string, error) { + first, remaining, err := splitSSHToken(rest) + if err != nil { + return "", "", err + } + if first == "" { + return "", "", fmt.Errorf("missing SSH public key") + } + if isSSHPublicKeyType(first) { + return "", rest, nil + } + if remaining == "" { + return "", "", fmt.Errorf("missing SSH public key") + } + return first, remaining, nil +} + +func splitSSHToken(s string) (string, string, error) { + inQuote := false + for i, r := range s { + if r == '"' { + inQuote = !inQuote + continue + } + if unicode.IsSpace(r) && !inQuote { + return s[:i], strings.TrimLeftFunc(s[i:], unicode.IsSpace), nil + } + } + if inQuote { + return "", "", fmt.Errorf("unterminated quote") + } + return s, "", nil +} + +func isSSHPublicKeyType(token string) bool { + return strings.HasPrefix(token, "ssh-") || + strings.HasPrefix(token, "ecdsa-") || + strings.HasPrefix(token, "sk-") +} + +func sshPrincipalFromPatternList(patternList string) (string, error) { + principals := strings.Split(patternList, ",") + for _, principal := range principals { + if !isSupportedSSHPattern(principal) { + return "", fmt.Errorf("unsupported SSH principal pattern %q", principal) + } + } + return principals[0], nil +} + +func sshNamespacesFromOptions(options string) ([]string, error) { + var namespaces []string + if options == "" { + return nil, nil + } + seenNamespaces := false + optionTokens, err := splitSSHOptionTokens(options) + if err != nil { + return nil, err + } + for _, option := range optionTokens { + if strings.HasPrefix(option, "namespaces=") { + if seenNamespaces { + return nil, fmt.Errorf("duplicate SSH namespaces option") + } + seenNamespaces = true + value, err := parseSSHOptionValue(strings.TrimPrefix(option, "namespaces=")) + if err != nil { + return nil, err + } + namespaces = []string{} + for _, namespace := range strings.Split(value, ",") { + if !isSupportedSSHPattern(namespace) { + return nil, fmt.Errorf("unsupported SSH namespace pattern %q", namespace) + } + namespaces = append(namespaces, namespace) + } + continue + } + return nil, fmt.Errorf("unsupported SSH allowed signer option %q", option) + } + return namespaces, nil +} + +func splitSSHOptionTokens(options string) ([]string, error) { + tokens := []string{} + start := 0 + inQuote := false + for i, r := range options { + if r == '"' { + inQuote = !inQuote + continue + } + if r == ',' && !inQuote { + token := options[start:i] + if token == "" { + return nil, fmt.Errorf("unsupported SSH allowed signer option %q", token) + } + tokens = append(tokens, token) + start = i + 1 + } + } + if inQuote { + return nil, fmt.Errorf("unterminated quote") + } + token := options[start:] + if token == "" { + return nil, fmt.Errorf("unsupported SSH allowed signer option %q", token) + } + return append(tokens, token), nil +} + +func parseSSHOptionValue(value string) (string, error) { + if !strings.HasPrefix(value, "\"") || !strings.HasSuffix(value, "\"") || strings.Contains(value, "\\") { + return "", fmt.Errorf("unsupported SSH namespace pattern %q", value) + } + return strings.TrimSuffix(strings.TrimPrefix(value, "\""), "\""), nil +} + +func isSupportedSSHPattern(pattern string) bool { + return pattern != "" && pattern == strings.TrimSpace(pattern) && (pattern == "*" || !strings.ContainsAny(pattern, "*?!\"'")) +} + +func (s sshAllowedSigner) allowsNamespace(namespace string) bool { + if len(s.namespaces) == 0 { + return true + } + for _, allowed := range s.namespaces { + if allowed == namespace || allowed == "*" { + return true + } + } + return false +} + +type sshCommitSignature struct { + publicKey []byte + namespace string + reserved string + hashAlgorithm string + signature *ssh.Signature +} + +func parseSSHSignature(signature string) (*sshCommitSignature, error) { + block, _ := pem.Decode([]byte(strings.TrimSpace(signature))) + if block == nil || block.Type != "SSH SIGNATURE" { + return nil, fmt.Errorf("not an SSH signature") + } + if !bytes.HasPrefix(block.Bytes, []byte("SSHSIG")) { + return nil, fmt.Errorf("missing SSH signature magic") + } + + var wire struct { + Version uint32 + PublicKey []byte + Namespace string + Reserved string + HashAlgorithm string + Signature []byte + } + if err := ssh.Unmarshal(block.Bytes[len("SSHSIG"):], &wire); err != nil { + return nil, err + } + if wire.Version != 1 { + return nil, fmt.Errorf("unsupported SSH signature version %d", wire.Version) + } + if _, err := ssh.ParsePublicKey(wire.PublicKey); err != nil { + return nil, err + } + + sshSignature := &ssh.Signature{} + if err := ssh.Unmarshal(wire.Signature, sshSignature); err != nil { + return nil, err + } + + return &sshCommitSignature{ + publicKey: wire.PublicKey, + namespace: wire.Namespace, + reserved: wire.Reserved, + hashAlgorithm: wire.HashAlgorithm, + signature: sshSignature, + }, nil +} + +func sshSignedData(namespace, reserved, hashAlgorithm string, payload []byte) ([]byte, error) { + var digest []byte + switch hashAlgorithm { + case "sha256": + sum := sha256.Sum256(payload) + digest = sum[:] + case "sha512": + sum := sha512.Sum512(payload) + digest = sum[:] + default: + return nil, fmt.Errorf("unsupported SSH signature hash algorithm %q", hashAlgorithm) + } + + return append([]byte("SSHSIG"), ssh.Marshal(struct { + Namespace string + Reserved string + HashAlgorithm string + Hash []byte + }{ + Namespace: namespace, + Reserved: reserved, + HashAlgorithm: hashAlgorithm, + Hash: digest, + })...), nil +} diff --git a/internal/repository/git_test.go b/internal/repository/git_test.go index b294b744..4952cde8 100644 --- a/internal/repository/git_test.go +++ b/internal/repository/git_test.go @@ -2,7 +2,9 @@ package repository import ( "os" + "os/exec" "path/filepath" + "strings" "testing" "time" @@ -13,6 +15,8 @@ import ( "github.com/stretchr/testify/assert" ) +const testSSHPublicKey = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDL18Zw2FkReAMqtgjNAvTr0il/FmljJnOtEGApGqZcp ssh@example.com" + func commitFile(remoteRepository *git.Repository, dir, branch, content string) (commitId string, err error) { return commitFileAndSign(remoteRepository, dir, branch, content, nil) } @@ -96,6 +100,69 @@ func HeadCommitId(r *git.Repository) string { return ref.Hash().String() } +func initSSHSignedRemoteRepository(t *testing.T) (dir, allowedSignersPath, commitId string) { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git is required for SSH signed commit tests") + } + if _, err := exec.LookPath("ssh-keygen"); err != nil { + t.Skip("ssh-keygen is required for SSH signed commit tests") + } + + dir = t.TempDir() + runTestCommand(t, dir, "git", "init", "-q") + runTestCommand(t, dir, "git", "checkout", "-q", "-b", "main") + runTestCommand(t, dir, "git", "config", "user.name", "SSH Test") + runTestCommand(t, dir, "git", "config", "user.email", "ssh@example.com") + runTestCommand(t, dir, "git", "config", "gpg.format", "ssh") + + signingKeyPath := filepath.Join(dir, "signing_key") + runTestCommand(t, dir, "ssh-keygen", "-q", "-t", "ed25519", "-N", "", "-C", "ssh@example.com", "-f", signingKeyPath) + runTestCommand(t, dir, "git", "config", "user.signingkey", signingKeyPath+".pub") + + filename := filepath.Join(dir, "file-1") + assert.Nil(t, os.WriteFile(filename, []byte("file-1"), 0644)) + runTestCommand(t, dir, "git", "add", "file-1") + runTestCommand(t, dir, "git", "commit", "-q", "-S", "-m", "file-1") + commitId = runTestCommand(t, dir, "git", "rev-parse", "HEAD") + + publicKey, err := os.ReadFile(signingKeyPath + ".pub") + assert.Nil(t, err) + allowedSignersPath = filepath.Join(dir, "allowed_signers") + assert.Nil(t, os.WriteFile(allowedSignersPath, []byte("ssh@example.com "+string(publicKey)), 0644)) + + return dir, allowedSignersPath, commitId +} + +func runTestCommand(t *testing.T, dir, command string, args ...string) string { + t.Helper() + cmd := exec.Command(command, args...) + cmd.Dir = dir + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("%s %s failed: %s\n%s", command, strings.Join(args, " "), err, string(output)) + } + return strings.TrimSpace(string(output)) +} + +func testSSHCertificatePublicKey(t *testing.T) string { + t.Helper() + if _, err := exec.LookPath("ssh-keygen"); err != nil { + t.Skip("ssh-keygen is required for SSH certificate tests") + } + + dir := t.TempDir() + caKeyPath := filepath.Join(dir, "ca_key") + userKeyPath := filepath.Join(dir, "user_key") + runTestCommand(t, dir, "ssh-keygen", "-q", "-t", "ed25519", "-N", "", "-C", "ca@example.com", "-f", caKeyPath) + runTestCommand(t, dir, "ssh-keygen", "-q", "-t", "ed25519", "-N", "", "-C", "ssh@example.com", "-f", userKeyPath) + runTestCommand(t, dir, "ssh-keygen", "-q", "-s", caKeyPath, "-I", "test-cert", "-n", "ssh@example.com", userKeyPath+".pub") + + cert, err := os.ReadFile(userKeyPath + "-cert.pub") + assert.Nil(t, err) + return strings.TrimSpace(string(cert)) +} + func TestIsAncestor(t *testing.T) { remoteRepositoryDir := t.TempDir() repository, err := initRemoteRepostiory(remoteRepositoryDir, true) @@ -151,3 +218,200 @@ func TestHeadSignedBy(t *testing.T) { assert.Nil(t, signedBy) } + +func TestHeadSignedBySSH(t *testing.T) { + dir, allowedSignersPath, commitId := initSSHSignedRemoteRepository(t) + remoteRepository, err := git.PlainOpen(dir) + assert.Nil(t, err) + + allowedSigners, err := os.ReadFile(allowedSignersPath) + assert.Nil(t, err) + + signedBy, err := commitSignedBySSH(remoteRepository, commitId, string(allowedSigners)) + assert.Nil(t, err) + assert.Equal(t, "ssh@example.com", signedBy) + + signedBy, err = commitSignedBySSH(remoteRepository, commitId, "") + assert.ErrorContains(t, err, "is not signed") + assert.Equal(t, "", signedBy) +} + +func TestSSHAllowedSignersRejectUnsupportedOptions(t *testing.T) { + tests := []struct { + name string + line string + }{ + { + name: "unsupported option before namespace", + line: `ssh@example.com valid-before="20240101",namespaces="git" ` + testSSHPublicKey, + }, + { + name: "unsupported option after namespace", + line: `ssh@example.com namespaces="git",valid-before="20240101" ` + testSSHPublicKey, + }, + { + name: "empty option after namespace", + line: `ssh@example.com namespaces="git", ` + testSSHPublicKey, + }, + { + name: "unsupported option in key input", + line: `ssh@example.com namespaces="git" valid-before="20240101" ` + testSSHPublicKey, + }, + { + name: "cert authority option in key input", + line: `ssh@example.com namespaces="git" cert-authority ` + testSSHPublicKey, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := parseSSHAllowedSigners(tt.line) + assert.ErrorContains(t, err, "unsupported SSH allowed signer option") + }) + } + + _, err := parseSSHAllowedSigners(`ssh@example.com namespaces="git" ` + testSSHPublicKey) + assert.Nil(t, err) +} + +func TestSSHAllowedSignersRejectCertificates(t *testing.T) { + publicCert := testSSHCertificatePublicKey(t) + + _, err := parseSSHAllowedSigners(`ssh@example.com ` + publicCert) + assert.ErrorContains(t, err, "SSH certificates are not supported") +} + +func TestSSHAllowedSignersRejectUnsupportedPrincipals(t *testing.T) { + tests := []struct { + name string + line string + }{ + { + name: "negated principal", + line: `!ssh@example.com ` + testSSHPublicKey, + }, + { + name: "partial wildcard principal", + line: `*@example.com ` + testSSHPublicKey, + }, + { + name: "question wildcard principal", + line: `ssh?example.com ` + testSSHPublicKey, + }, + { + name: "empty principal", + line: `ssh@example.com, ` + testSSHPublicKey, + }, + { + name: "quoted empty principal", + line: `"" ` + testSSHPublicKey, + }, + { + name: "malformed quoted principal", + line: `""ssh@example.com"" ` + testSSHPublicKey, + }, + { + name: "whitespace padded quoted principal", + line: `" ssh@example.com " ` + testSSHPublicKey, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := parseSSHAllowedSigners(tt.line) + assert.ErrorContains(t, err, "unsupported SSH principal pattern") + }) + } + + _, err := parseSSHAllowedSigners(`* ` + testSSHPublicKey) + assert.Nil(t, err) +} + +func TestSSHAllowedSignersNamespaces(t *testing.T) { + tests := []struct { + name string + line string + allowed bool + wantError string + }{ + { + name: "exact git namespace", + line: `ssh@example.com namespaces="git" ` + testSSHPublicKey, + allowed: true, + }, + { + name: "non git namespace", + line: `ssh@example.com namespaces="deploy" ` + testSSHPublicKey, + allowed: false, + }, + { + name: "wildcard namespace", + line: `ssh@example.com namespaces="*" ` + testSSHPublicKey, + allowed: true, + }, + { + name: "unquoted namespace", + line: `ssh@example.com namespaces=git ` + testSSHPublicKey, + wantError: "unsupported SSH namespace pattern", + }, + { + name: "unquoted wildcard namespace", + line: `ssh@example.com namespaces=* ` + testSSHPublicKey, + wantError: "unsupported SSH namespace pattern", + }, + { + name: "negated git after wildcard", + line: `ssh@example.com namespaces="*,!git" ` + testSSHPublicKey, + wantError: "unsupported SSH namespace pattern", + }, + { + name: "negated git after exact namespace", + line: `ssh@example.com namespaces="git,!git" ` + testSSHPublicKey, + wantError: "unsupported SSH namespace pattern", + }, + { + name: "partial wildcard", + line: `ssh@example.com namespaces="gi*" ` + testSSHPublicKey, + wantError: "unsupported SSH namespace pattern", + }, + { + name: "question wildcard", + line: `ssh@example.com namespaces="g?t" ` + testSSHPublicKey, + wantError: "unsupported SSH namespace pattern", + }, + { + name: "leading namespace whitespace", + line: `ssh@example.com namespaces=" git" ` + testSSHPublicKey, + wantError: "unsupported SSH namespace pattern", + }, + { + name: "malformed quoted namespace", + line: `ssh@example.com namespaces=""git"" ` + testSSHPublicKey, + wantError: "unsupported SSH namespace pattern", + }, + { + name: "duplicate namespace clauses", + line: `ssh@example.com namespaces="deploy",namespaces="git" ` + testSSHPublicKey, + wantError: "duplicate SSH namespaces option", + }, + { + name: "escaped wildcard namespace", + line: `ssh@example.com namespaces="\x2a" ` + testSSHPublicKey, + wantError: "unsupported SSH namespace pattern", + }, + { + name: "escaped git namespace", + line: `ssh@example.com namespaces="\x67it" ` + testSSHPublicKey, + wantError: "unsupported SSH namespace pattern", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + signers, err := parseSSHAllowedSigners(tt.line) + if tt.wantError != "" { + assert.ErrorContains(t, err, tt.wantError) + return + } + assert.Nil(t, err) + assert.Equal(t, tt.allowed, signers[0].allowsNamespace("git")) + }) + } +} diff --git a/internal/repository/repository.go b/internal/repository/repository.go index 6d94a93a..39feb09a 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -11,8 +11,8 @@ import ( "github.com/ProtonMail/go-crypto/openpgp" "github.com/go-git/go-git/v5" "github.com/nlewo/comin/internal/prometheus" - pb "github.com/nlewo/comin/pkg/protobuf" "github.com/nlewo/comin/internal/types" + pb "github.com/nlewo/comin/pkg/protobuf" "github.com/sirupsen/logrus" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/timestamppb" @@ -20,11 +20,12 @@ import ( ) type repository struct { - Repository *git.Repository - GitConfig types.GitConfig - RepositoryStatus *pb.RepositoryStatus - prometheus prometheus.Prometheus - gpgPubliKeys []string + Repository *git.Repository + GitConfig types.GitConfig + RepositoryStatus *pb.RepositoryStatus + prometheus prometheus.Prometheus + gpgPubliKeys []string + sshAllowedSigners string } type Repository interface { @@ -47,10 +48,22 @@ func New(config types.GitConfig, mainCommitId string, prometheus prometheus.Prom } gpgPublicKeys[i] = string(k) } + sshAllowedSigners := "" + if config.SshAllowedSignersPath != "" { + k, err := os.ReadFile(config.SshAllowedSignersPath) + if err != nil { + return nil, fmt.Errorf("failed to open the SSH allowed signers file %s: %w", config.SshAllowedSignersPath, err) + } + sshAllowedSigners = string(k) + if _, err := parseSSHAllowedSigners(sshAllowedSigners); err != nil { + return nil, fmt.Errorf("failed to read the SSH allowed signers file %s: %w", config.SshAllowedSignersPath, err) + } + } r = &repository{ - prometheus: prometheus, - gpgPubliKeys: gpgPublicKeys, + prometheus: prometheus, + gpgPubliKeys: gpgPublicKeys, + sshAllowedSigners: sshAllowedSigners, } r.GitConfig = config @@ -202,18 +215,18 @@ func (r *repository) Update() error { r.RepositoryStatus.SelectedCommitId = selectedCommitId } - if len(r.gpgPubliKeys) > 0 { + if len(r.gpgPubliKeys) > 0 || r.sshAllowedSigners != "" { r.RepositoryStatus.SelectedCommitShouldBeSigned = wrapperspb.Bool(true) - signedBy, err := commitSignedBy(r.Repository, selectedCommitId, r.gpgPubliKeys) + signedBy, err := commitSignedByTrustedKey(r.Repository, selectedCommitId, r.gpgPubliKeys, r.sshAllowedSigners) if err != nil { r.RepositoryStatus.ErrorMsg = err.Error() } - if signedBy == nil { + if signedBy == "" { r.RepositoryStatus.SelectedCommitSigned = wrapperspb.Bool(false) r.RepositoryStatus.SelectedCommitSignedBy = "" } else { r.RepositoryStatus.SelectedCommitSigned = wrapperspb.Bool(true) - r.RepositoryStatus.SelectedCommitSignedBy = signedBy.PrimaryIdentity().Name + r.RepositoryStatus.SelectedCommitSignedBy = signedBy } } else { r.RepositoryStatus.SelectedCommitShouldBeSigned = wrapperspb.Bool(false) diff --git a/internal/repository/repository_test.go b/internal/repository/repository_test.go index 0aa53ebd..4d725ef3 100644 --- a/internal/repository/repository_test.go +++ b/internal/repository/repository_test.go @@ -41,6 +41,7 @@ func TestNew(t *testing.T) { func TestNewGpg(t *testing.T) { gitConfig := types.GitConfig{ + Path: t.TempDir(), GpgPublicKeyPaths: []string{"./fail.public", "./test.public"}, } r, err := New(gitConfig, "", prometheus.New()) @@ -48,12 +49,31 @@ func TestNewGpg(t *testing.T) { assert.Equal(t, 2, len(r.gpgPubliKeys)) gitConfig = types.GitConfig{ + Path: t.TempDir(), GpgPublicKeyPaths: []string{"./fail.public", "./test.public", "./invalid.public"}, } _, err = New(gitConfig, "", prometheus.New()) assert.ErrorContains(t, err, "failed to read the GPG public key") } +func TestNewSSHAllowedSigners(t *testing.T) { + dir := t.TempDir() + allowedSignersPath := dir + "/allowed_signers" + assert.Nil(t, os.WriteFile(allowedSignersPath, []byte(""), 0644)) + + gitConfig := types.GitConfig{ + Path: t.TempDir(), + SshAllowedSignersPath: allowedSignersPath, + } + _, err := New(gitConfig, "", prometheus.New()) + assert.ErrorContains(t, err, "failed to read the SSH allowed signers file") + + assert.Nil(t, os.WriteFile(allowedSignersPath, []byte("ssh@example.com "+testSSHPublicKey), 0644)) + r, err := New(gitConfig, "", prometheus.New()) + assert.Nil(t, err) + assert.NotEmpty(t, r.sshAllowedSigners) +} + func TestPreferMain(t *testing.T) { var err error r1Dir := t.TempDir() @@ -786,3 +806,34 @@ func TestUpdateGpg(t *testing.T) { assert.Equal(t, "", r.RepositoryStatus.SelectedCommitSignedBy) assert.False(t, r.RepositoryStatus.SelectedCommitShouldBeSigned.GetValue()) } + +func TestUpdateSSHSigning(t *testing.T) { + dir, allowedSignersPath, cMain := initSSHSignedRemoteRepository(t) + cominRepositoryDir := t.TempDir() + + gitConfig := types.GitConfig{ + Path: cominRepositoryDir, + SshAllowedSignersPath: allowedSignersPath, + Remotes: []types.Remote{ + { + Name: "r1", + URL: dir, + Branches: types.Branches{ + Main: types.Branch{ + Name: "main", + }, + }, + Timeout: 30, + }, + }, + } + r, err := New(gitConfig, "", prometheus.New()) + assert.Nil(t, err) + r.Fetch([]string{"r1"}) + err = r.Update() + assert.Nil(t, err) + assert.Equal(t, cMain, r.RepositoryStatus.SelectedCommitId) + assert.True(t, r.RepositoryStatus.SelectedCommitSigned.GetValue()) + assert.Equal(t, "ssh@example.com", r.RepositoryStatus.SelectedCommitSignedBy) + assert.True(t, r.RepositoryStatus.SelectedCommitShouldBeSigned.GetValue()) +} diff --git a/internal/types/types.go b/internal/types/types.go index 9ab15fad..b3e12a24 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -23,10 +23,11 @@ type GitConfig struct { // The repository Path Path string // The directory in the repository - Dir string - Remotes []Remote - GpgPublicKeyPaths []string - Submodules bool + Dir string + Remotes []Remote + GpgPublicKeyPaths []string + SshAllowedSignersPath string + Submodules bool } type Auth struct { @@ -81,6 +82,7 @@ type Configuration struct { Grpc Grpc `yaml:"grpc"` Exporter HttpServer `yaml:"exporter"` GpgPublicKeyPaths []string `yaml:"gpg_public_key_paths"` + SshAllowedSignersPath string `yaml:"ssh_allowed_signers_path"` PostDeploymentCommand string `yaml:"post_deployment_command"` BuildConfirmer Confirmer `yaml:"build_confirmer"` DeployConfirmer Confirmer `yaml:"deploy_confirmer"` diff --git a/nix/comin-config.nix b/nix/comin-config.nix index 5df95585..7418a098 100644 --- a/nix/comin-config.nix +++ b/nix/comin-config.nix @@ -26,6 +26,9 @@ rec { deploy_confirmer = cfg.services.comin.deployConfirmer; retention = cfg.services.comin.retention; } + // (lib.optionalAttrs (cfg.services.comin.sshAllowedSignersPath != null) { + ssh_allowed_signers_path = cfg.services.comin.sshAllowedSignersPath; + }) // (lib.optionalAttrs (cfg.services.comin.postDeploymentCommand != null) { post_deployment_command = cfg.services.comin.postDeploymentCommand; }); diff --git a/nix/module-options-doc.nix b/nix/module-options-doc.nix index d246a3f0..1c2ba90a 100644 --- a/nix/module-options-doc.nix +++ b/nix/module-options-doc.nix @@ -28,7 +28,7 @@ pkgs: rec { }; in pkgs.runCommand "options-doc.md" { } '' - cat ${optionsDoc.optionsCommonMark} >> $out + awk 'NF { last = NR } { lines[NR] = $0 } END { for (i = 1; i <= last; i++) print lines[i] }' ${optionsDoc.optionsCommonMark} > $out ''; optionsDocCommonMarkGenerator = pkgs.writers.writeBashBin "optionsDocCommonMarkGenerator" '' cp -v ${optionsDocCommonMark} ./docs/generated-module-options.md diff --git a/nix/module-options.nix b/nix/module-options.nix index c41b4fea..9cc34fa7 100644 --- a/nix/module-options.nix +++ b/nix/module-options.nix @@ -266,6 +266,11 @@ in type = listOf str; default = [ ]; }; + sshAllowedSignersPath = mkOption { + description = "An OpenSSH allowed signers file path used to verify SSH-signed Git commits."; + type = nullOr str; + default = null; + }; postDeploymentCommand = mkOption { description = "A path to a script executed after each deployment. comin provides to the script the following From 1233dacd00f8fea5843d0b1ddef3ecb6f89a16f7 Mon Sep 17 00:00:00 2001 From: Bas Nijholt Date: Wed, 1 Jul 2026 11:48:50 -0700 Subject: [PATCH 2/4] fix: address SSH signing review feedback --- docs/howtos.md | 1 + internal/repository/git.go | 43 +++++----- internal/repository/git_test.go | 131 +++++++++++++++++++----------- internal/repository/repository.go | 2 +- 4 files changed, 108 insertions(+), 69 deletions(-) diff --git a/docs/howtos.md b/docs/howtos.md index 5101b027..90fe2c1a 100644 --- a/docs/howtos.md +++ b/docs/howtos.md @@ -77,6 +77,7 @@ SSH-signed commits with an OpenSSH allowed signers file. For example: ```text alice@example.org ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAA... +bob@example.org ssh-ed25519 AAAAC3NzaC1lZDI1NTE5BBBB... ``` comin supports public key entries and the optional `namespaces=` restriction; diff --git a/internal/repository/git.go b/internal/repository/git.go index dabd3ae0..8b425edb 100644 --- a/internal/repository/git.go +++ b/internal/repository/git.go @@ -202,7 +202,7 @@ func manageRemote(r *git.Repository, remote types.Remote) error { return nil } -func commitSignedBy(r *git.Repository, commitId string, publicKeys []string) (signedBy *openpgp.Entity, err error) { +func commitSignedByGPG(r *git.Repository, commitId string, publicKeys []string) (signedBy *openpgp.Entity, err error) { commit, err := r.CommitObject(plumbing.NewHash(commitId)) if err != nil { return nil, err @@ -217,10 +217,10 @@ func commitSignedBy(r *git.Repository, commitId string, publicKeys []string) (si return nil, fmt.Errorf("commit %s is not signed", commitId) } -func commitSignedByTrustedKey(r *git.Repository, commitId string, gpgPublicKeys []string, sshAllowedSigners string) (string, error) { +func commitSignedBy(r *git.Repository, commitId string, gpgPublicKeys []string, sshAllowedSigners string) (string, error) { var verifyErr error if len(gpgPublicKeys) > 0 { - entity, err := commitSignedBy(r, commitId, gpgPublicKeys) + entity, err := commitSignedByGPG(r, commitId, gpgPublicKeys) if err == nil { return entity.PrimaryIdentity().Name, nil } @@ -250,11 +250,8 @@ func commitSignedBySSH(r *git.Repository, commitId string, allowedSigners string if err != nil { return "", err } - if strings.TrimSpace(allowedSigners) == "" { - return "", fmt.Errorf("commit %s is not signed", commitId) - } if !strings.HasPrefix(strings.TrimSpace(commit.PGPSignature), "-----BEGIN SSH SIGNATURE-----") { - return "", fmt.Errorf("commit %s is not signed", commitId) + return "", fmt.Errorf("commit %s is not signed with SSH", commitId) } signature, err := parseSSHSignature(commit.PGPSignature) @@ -501,6 +498,22 @@ type sshCommitSignature struct { signature *ssh.Signature } +type sshSignatureWire struct { + Version uint32 + PublicKey []byte + Namespace string + Reserved string + HashAlgorithm string + Signature []byte +} + +type sshSignedDataWire struct { + Namespace string + Reserved string + HashAlgorithm string + Hash []byte +} + func parseSSHSignature(signature string) (*sshCommitSignature, error) { block, _ := pem.Decode([]byte(strings.TrimSpace(signature))) if block == nil || block.Type != "SSH SIGNATURE" { @@ -510,14 +523,7 @@ func parseSSHSignature(signature string) (*sshCommitSignature, error) { return nil, fmt.Errorf("missing SSH signature magic") } - var wire struct { - Version uint32 - PublicKey []byte - Namespace string - Reserved string - HashAlgorithm string - Signature []byte - } + var wire sshSignatureWire if err := ssh.Unmarshal(block.Bytes[len("SSHSIG"):], &wire); err != nil { return nil, err } @@ -555,12 +561,7 @@ func sshSignedData(namespace, reserved, hashAlgorithm string, payload []byte) ([ return nil, fmt.Errorf("unsupported SSH signature hash algorithm %q", hashAlgorithm) } - return append([]byte("SSHSIG"), ssh.Marshal(struct { - Namespace string - Reserved string - HashAlgorithm string - Hash []byte - }{ + return append([]byte("SSHSIG"), ssh.Marshal(sshSignedDataWire{ Namespace: namespace, Reserved: reserved, HashAlgorithm: hashAlgorithm, diff --git a/internal/repository/git_test.go b/internal/repository/git_test.go index 4952cde8..30888be5 100644 --- a/internal/repository/git_test.go +++ b/internal/repository/git_test.go @@ -1,10 +1,12 @@ package repository import ( + "crypto/ed25519" + "crypto/rand" + "encoding/pem" + "io" "os" - "os/exec" "path/filepath" - "strings" "testing" "time" @@ -13,6 +15,7 @@ import ( "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/object" "github.com/stretchr/testify/assert" + "golang.org/x/crypto/ssh" ) const testSSHPublicKey = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDL18Zw2FkReAMqtgjNAvTr0il/FmljJnOtEGApGqZcp ssh@example.com" @@ -102,65 +105,99 @@ func HeadCommitId(r *git.Repository) string { func initSSHSignedRemoteRepository(t *testing.T) (dir, allowedSignersPath, commitId string) { t.Helper() - if _, err := exec.LookPath("git"); err != nil { - t.Skip("git is required for SSH signed commit tests") - } - if _, err := exec.LookPath("ssh-keygen"); err != nil { - t.Skip("ssh-keygen is required for SSH signed commit tests") - } dir = t.TempDir() - runTestCommand(t, dir, "git", "init", "-q") - runTestCommand(t, dir, "git", "checkout", "-q", "-b", "main") - runTestCommand(t, dir, "git", "config", "user.name", "SSH Test") - runTestCommand(t, dir, "git", "config", "user.email", "ssh@example.com") - runTestCommand(t, dir, "git", "config", "gpg.format", "ssh") - - signingKeyPath := filepath.Join(dir, "signing_key") - runTestCommand(t, dir, "ssh-keygen", "-q", "-t", "ed25519", "-N", "", "-C", "ssh@example.com", "-f", signingKeyPath) - runTestCommand(t, dir, "git", "config", "user.signingkey", signingKeyPath+".pub") + remoteRepository, err := git.PlainInit(dir, false) + assert.Nil(t, err) + worktree, err := remoteRepository.Worktree() + assert.Nil(t, err) filename := filepath.Join(dir, "file-1") assert.Nil(t, os.WriteFile(filename, []byte("file-1"), 0644)) - runTestCommand(t, dir, "git", "add", "file-1") - runTestCommand(t, dir, "git", "commit", "-q", "-S", "-m", "file-1") - commitId = runTestCommand(t, dir, "git", "rev-parse", "HEAD") + _, err = worktree.Add("file-1") + assert.Nil(t, err) + hash, err := worktree.Commit("file-1", &git.CommitOptions{ + Author: &object.Signature{ + Name: "SSH Test", + Email: "ssh@example.com", + When: time.Unix(0, 0), + }, + }) + assert.Nil(t, err) - publicKey, err := os.ReadFile(signingKeyPath + ".pub") + commit, err := remoteRepository.CommitObject(hash) assert.Nil(t, err) + signer := testSSHSigner(t) + signedHash := signCommitWithSSH(t, remoteRepository, commit, signer) + assert.Nil(t, remoteRepository.Storer.SetReference( + plumbing.NewHashReference(plumbing.NewBranchReferenceName("main"), signedHash), + )) + assert.Nil(t, remoteRepository.Storer.SetReference( + plumbing.NewSymbolicReference(plumbing.HEAD, plumbing.NewBranchReferenceName("main")), + )) + commitId = signedHash.String() + allowedSignersPath = filepath.Join(dir, "allowed_signers") - assert.Nil(t, os.WriteFile(allowedSignersPath, []byte("ssh@example.com "+string(publicKey)), 0644)) + assert.Nil(t, os.WriteFile(allowedSignersPath, []byte("ssh@example.com "+string(ssh.MarshalAuthorizedKey(signer.PublicKey()))), 0644)) return dir, allowedSignersPath, commitId } -func runTestCommand(t *testing.T, dir, command string, args ...string) string { +func testSSHSigner(t *testing.T) ssh.Signer { t.Helper() - cmd := exec.Command(command, args...) - cmd.Dir = dir - output, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("%s %s failed: %s\n%s", command, strings.Join(args, " "), err, string(output)) - } - return strings.TrimSpace(string(output)) + _, privateKey, err := ed25519.GenerateKey(rand.Reader) + assert.Nil(t, err) + signer, err := ssh.NewSignerFromKey(privateKey) + assert.Nil(t, err) + return signer } -func testSSHCertificatePublicKey(t *testing.T) string { +func signCommitWithSSH(t *testing.T, repository *git.Repository, commit *object.Commit, signer ssh.Signer) plumbing.Hash { t.Helper() - if _, err := exec.LookPath("ssh-keygen"); err != nil { - t.Skip("ssh-keygen is required for SSH certificate tests") - } - - dir := t.TempDir() - caKeyPath := filepath.Join(dir, "ca_key") - userKeyPath := filepath.Join(dir, "user_key") - runTestCommand(t, dir, "ssh-keygen", "-q", "-t", "ed25519", "-N", "", "-C", "ca@example.com", "-f", caKeyPath) - runTestCommand(t, dir, "ssh-keygen", "-q", "-t", "ed25519", "-N", "", "-C", "ssh@example.com", "-f", userKeyPath) - runTestCommand(t, dir, "ssh-keygen", "-q", "-s", caKeyPath, "-I", "test-cert", "-n", "ssh@example.com", userKeyPath+".pub") + encoded := &plumbing.MemoryObject{} + assert.Nil(t, commit.EncodeWithoutSignature(encoded)) + reader, err := encoded.Reader() + assert.Nil(t, err) + defer reader.Close() // nolint:errcheck + payload, err := io.ReadAll(reader) + assert.Nil(t, err) - cert, err := os.ReadFile(userKeyPath + "-cert.pub") + signedData, err := sshSignedData("git", "", "sha512", payload) + assert.Nil(t, err) + signature, err := signer.Sign(rand.Reader, signedData) assert.Nil(t, err) - return strings.TrimSpace(string(cert)) + commit.PGPSignature = string(pem.EncodeToMemory(&pem.Block{ + Type: "SSH SIGNATURE", + Bytes: append([]byte("SSHSIG"), ssh.Marshal(sshSignatureWire{ + Version: 1, + PublicKey: signer.PublicKey().Marshal(), + Namespace: "git", + Reserved: "", + HashAlgorithm: "sha512", + Signature: ssh.Marshal(signature), + })...), + })) + + obj := repository.Storer.NewEncodedObject() + assert.Nil(t, commit.Encode(obj)) + hash, err := repository.Storer.SetEncodedObject(obj) + assert.Nil(t, err) + return hash +} + +func testSSHCertificatePublicKey(t *testing.T) string { + t.Helper() + caSigner := testSSHSigner(t) + cert := &ssh.Certificate{ + Key: testSSHSigner(t).PublicKey(), + Serial: 1, + CertType: ssh.UserCert, + KeyId: "test-cert", + ValidPrincipals: []string{"ssh@example.com"}, + ValidBefore: ssh.CertTimeInfinity, + } + assert.Nil(t, cert.SignCert(rand.Reader, caSigner)) + return string(ssh.MarshalAuthorizedKey(cert)) } func TestIsAncestor(t *testing.T) { @@ -204,16 +241,16 @@ func TestHeadSignedBy(t *testing.T) { failPublic, _ := os.ReadFile("./fail.public") testPublic, _ := os.ReadFile("./test.public") - signedBy, err := commitSignedBy(remoteRepository, commitId, []string{string(failPublic), string(testPublic)}) + signedBy, err := commitSignedByGPG(remoteRepository, commitId, []string{string(failPublic), string(testPublic)}) assert.Nil(t, err) assert.Equal(t, "test ", signedBy.PrimaryIdentity().Name) - signedBy, err = commitSignedBy(remoteRepository, commitId, []string{string(failPublic)}) + signedBy, err = commitSignedByGPG(remoteRepository, commitId, []string{string(failPublic)}) assert.ErrorContains(t, err, "is not signed") assert.Nil(t, signedBy) commitId, _ = commitFileAndSign(remoteRepository, dir, "main", "file-2", nil) - signedBy, err = commitSignedBy(remoteRepository, commitId, []string{string(failPublic), string(testPublic)}) + signedBy, err = commitSignedByGPG(remoteRepository, commitId, []string{string(failPublic), string(testPublic)}) assert.ErrorContains(t, err, "is not signed") assert.Nil(t, signedBy) @@ -232,7 +269,7 @@ func TestHeadSignedBySSH(t *testing.T) { assert.Equal(t, "ssh@example.com", signedBy) signedBy, err = commitSignedBySSH(remoteRepository, commitId, "") - assert.ErrorContains(t, err, "is not signed") + assert.ErrorContains(t, err, "no SSH allowed signers found") assert.Equal(t, "", signedBy) } diff --git a/internal/repository/repository.go b/internal/repository/repository.go index 39feb09a..3f1156a9 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -217,7 +217,7 @@ func (r *repository) Update() error { if len(r.gpgPubliKeys) > 0 || r.sshAllowedSigners != "" { r.RepositoryStatus.SelectedCommitShouldBeSigned = wrapperspb.Bool(true) - signedBy, err := commitSignedByTrustedKey(r.Repository, selectedCommitId, r.gpgPubliKeys, r.sshAllowedSigners) + signedBy, err := commitSignedBy(r.Repository, selectedCommitId, r.gpgPubliKeys, r.sshAllowedSigners) if err != nil { r.RepositoryStatus.ErrorMsg = err.Error() } From 31b3f478e69a06ef6f2642f790dbef2b1fe5b41c Mon Sep 17 00:00:00 2001 From: Bas Nijholt Date: Wed, 1 Jul 2026 16:36:30 -0700 Subject: [PATCH 3/4] fix: reject SHA-1 SSH signature algorithms and simplify signer handling - Refuse ssh-rsa and ssh-dss signature algorithms in SSH signatures, matching OpenSSH's sshsig behavior, since x/crypto otherwise accepts SHA-1 based signatures for RSA keys. - Parse the allowed signers file once in New() and store the parsed signers instead of re-parsing on every update. - Report both GPG and SSH verification errors when both trust sources are configured, instead of only the last one. --- internal/repository/git.go | 19 ++++++++++--------- internal/repository/git_test.go | 26 +++++++++++++++++++++++--- internal/repository/repository.go | 10 +++++----- 3 files changed, 38 insertions(+), 17 deletions(-) diff --git a/internal/repository/git.go b/internal/repository/git.go index 8b425edb..b7ce1ba8 100644 --- a/internal/repository/git.go +++ b/internal/repository/git.go @@ -6,6 +6,7 @@ import ( "crypto/sha256" "crypto/sha512" "encoding/pem" + "errors" "fmt" "io" "os" @@ -217,21 +218,21 @@ func commitSignedByGPG(r *git.Repository, commitId string, publicKeys []string) return nil, fmt.Errorf("commit %s is not signed", commitId) } -func commitSignedBy(r *git.Repository, commitId string, gpgPublicKeys []string, sshAllowedSigners string) (string, error) { +func commitSignedBy(r *git.Repository, commitId string, gpgPublicKeys []string, sshAllowedSigners []sshAllowedSigner) (string, error) { var verifyErr error if len(gpgPublicKeys) > 0 { entity, err := commitSignedByGPG(r, commitId, gpgPublicKeys) if err == nil { return entity.PrimaryIdentity().Name, nil } - verifyErr = err + verifyErr = errors.Join(verifyErr, err) } - if sshAllowedSigners != "" { + if len(sshAllowedSigners) > 0 { signedBy, err := commitSignedBySSH(r, commitId, sshAllowedSigners) if err == nil { return signedBy, nil } - verifyErr = err + verifyErr = errors.Join(verifyErr, err) } if verifyErr != nil { return "", verifyErr @@ -245,7 +246,7 @@ type sshAllowedSigner struct { namespaces []string } -func commitSignedBySSH(r *git.Repository, commitId string, allowedSigners string) (string, error) { +func commitSignedBySSH(r *git.Repository, commitId string, signers []sshAllowedSigner) (string, error) { commit, err := r.CommitObject(plumbing.NewHash(commitId)) if err != nil { return "", err @@ -280,10 +281,6 @@ func commitSignedBySSH(r *git.Repository, commitId string, allowedSigners string return "", err } - signers, err := parseSSHAllowedSigners(allowedSigners) - if err != nil { - return "", err - } for _, signer := range signers { if !signer.allowsNamespace(signature.namespace) { continue @@ -538,6 +535,10 @@ func parseSSHSignature(signature string) (*sshCommitSignature, error) { if err := ssh.Unmarshal(wire.Signature, sshSignature); err != nil { return nil, err } + // OpenSSH refuses SHA-1 based signature algorithms for SSH signatures. + if sshSignature.Format == ssh.KeyAlgoRSA || sshSignature.Format == ssh.KeyAlgoDSA { + return nil, fmt.Errorf("unsupported SSH signature algorithm %q", sshSignature.Format) + } return &sshCommitSignature{ publicKey: wire.PublicKey, diff --git a/internal/repository/git_test.go b/internal/repository/git_test.go index 30888be5..45ae6987 100644 --- a/internal/repository/git_test.go +++ b/internal/repository/git_test.go @@ -264,13 +264,33 @@ func TestHeadSignedBySSH(t *testing.T) { allowedSigners, err := os.ReadFile(allowedSignersPath) assert.Nil(t, err) - signedBy, err := commitSignedBySSH(remoteRepository, commitId, string(allowedSigners)) + signers, err := parseSSHAllowedSigners(string(allowedSigners)) + assert.Nil(t, err) + + signedBy, err := commitSignedBySSH(remoteRepository, commitId, signers) assert.Nil(t, err) assert.Equal(t, "ssh@example.com", signedBy) - signedBy, err = commitSignedBySSH(remoteRepository, commitId, "") + _, err = parseSSHAllowedSigners("") assert.ErrorContains(t, err, "no SSH allowed signers found") - assert.Equal(t, "", signedBy) +} + +func TestSSHSignatureRejectsSHA1Algorithms(t *testing.T) { + publicKey := testSSHSigner(t).PublicKey() + for _, format := range []string{"ssh-rsa", "ssh-dss"} { + signature := pem.EncodeToMemory(&pem.Block{ + Type: "SSH SIGNATURE", + Bytes: append([]byte("SSHSIG"), ssh.Marshal(sshSignatureWire{ + Version: 1, + PublicKey: publicKey.Marshal(), + Namespace: "git", + HashAlgorithm: "sha512", + Signature: ssh.Marshal(ssh.Signature{Format: format, Blob: []byte("signature")}), + })...), + }) + _, err := parseSSHSignature(string(signature)) + assert.ErrorContains(t, err, "unsupported SSH signature algorithm") + } } func TestSSHAllowedSignersRejectUnsupportedOptions(t *testing.T) { diff --git a/internal/repository/repository.go b/internal/repository/repository.go index 3f1156a9..19f7ce53 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -25,7 +25,7 @@ type repository struct { RepositoryStatus *pb.RepositoryStatus prometheus prometheus.Prometheus gpgPubliKeys []string - sshAllowedSigners string + sshAllowedSigners []sshAllowedSigner } type Repository interface { @@ -48,14 +48,14 @@ func New(config types.GitConfig, mainCommitId string, prometheus prometheus.Prom } gpgPublicKeys[i] = string(k) } - sshAllowedSigners := "" + var sshAllowedSigners []sshAllowedSigner if config.SshAllowedSignersPath != "" { k, err := os.ReadFile(config.SshAllowedSignersPath) if err != nil { return nil, fmt.Errorf("failed to open the SSH allowed signers file %s: %w", config.SshAllowedSignersPath, err) } - sshAllowedSigners = string(k) - if _, err := parseSSHAllowedSigners(sshAllowedSigners); err != nil { + sshAllowedSigners, err = parseSSHAllowedSigners(string(k)) + if err != nil { return nil, fmt.Errorf("failed to read the SSH allowed signers file %s: %w", config.SshAllowedSignersPath, err) } } @@ -215,7 +215,7 @@ func (r *repository) Update() error { r.RepositoryStatus.SelectedCommitId = selectedCommitId } - if len(r.gpgPubliKeys) > 0 || r.sshAllowedSigners != "" { + if len(r.gpgPubliKeys) > 0 || len(r.sshAllowedSigners) > 0 { r.RepositoryStatus.SelectedCommitShouldBeSigned = wrapperspb.Bool(true) signedBy, err := commitSignedBy(r.Repository, selectedCommitId, r.gpgPubliKeys, r.sshAllowedSigners) if err != nil { From ded5429aa5c7ad7b695e9e33a5d22d76350bef66 Mon Sep 17 00:00:00 2001 From: Bas Nijholt Date: Sun, 5 Jul 2026 22:52:07 -0700 Subject: [PATCH 4/4] fix: avoid deprecated ssh.KeyAlgoDSA constant in signature check --- internal/repository/git.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/repository/git.go b/internal/repository/git.go index b7ce1ba8..23c60122 100644 --- a/internal/repository/git.go +++ b/internal/repository/git.go @@ -536,7 +536,8 @@ func parseSSHSignature(signature string) (*sshCommitSignature, error) { return nil, err } // OpenSSH refuses SHA-1 based signature algorithms for SSH signatures. - if sshSignature.Format == ssh.KeyAlgoRSA || sshSignature.Format == ssh.KeyAlgoDSA { + // "ssh-dss" spelled out to avoid referencing the deprecated ssh.KeyAlgoDSA constant. + if sshSignature.Format == ssh.KeyAlgoRSA || sshSignature.Format == "ssh-dss" { return nil, fmt.Errorf("unsupported SSH signature algorithm %q", sshSignature.Format) }