Repository navigation
Say which key to unlock when commit signing fails #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,12 @@ package app | |
| import ( | ||
| "context" | ||
| "errors" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
||
| "github.com/DomBlack/git-stack/pkg/exec" | ||
| "github.com/DomBlack/git-stack/pkg/git" | ||
| "github.com/DomBlack/git-stack/pkg/shell" | ||
| "github.com/DomBlack/git-stack/pkg/stack" | ||
| ) | ||
|
|
||
|
|
@@ -86,11 +89,56 @@ func (a *App) commitInfo(ctx context.Context, repo git.Repo, sha string) CommitI | |
| return CommitInfo{SHA: sha, Subject: subject} | ||
| } | ||
|
|
||
| // editorError maps a missing TTY for an editor-backed commit. | ||
| func editorError(err error, hint string) error { | ||
| // commitError maps the ways git commit fails that the user can act on: a | ||
| // missing TTY for an editor-backed commit (editorHint says how to pass the | ||
| // message instead) and a signing failure. | ||
| func commitError(err error, editorHint string) error { | ||
| if errors.Is(err, exec.ErrTTYUnavailable) { | ||
| return stack.New(stack.KindInteractionRequired, "a commit message editor is needed but no terminal is available"). | ||
| WithSteps(hint) | ||
| WithSteps(editorHint) | ||
| } | ||
| if se, ok := errors.AsType[*git.SigningError](err); ok { | ||
| return signingError(se) | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| // signingError explains a signing failure. The common cause with an SSH key | ||
| // is a passphrase protected key that isn't in ssh-agent: without a terminal | ||
| // (the MCP server, a script) nothing can answer the passphrase prompt, so | ||
| // git fails instead. With gpg it is usually an agent whose pinentry can't | ||
| // prompt either. | ||
| func signingError(se *git.SigningError) error { | ||
| var e *stack.Error | ||
| switch se.Format { | ||
| case "ssh": | ||
| if se.Key == "" { | ||
| e = stack.New(stack.KindSigningFailed, "commit signing failed: gpg.format is ssh but user.signingkey is not set"). | ||
| WithSteps("set user.signingkey to the public key file to sign with") | ||
| } else { | ||
| // The private key sits next to the configured .pub; quote it | ||
| // so the line can be pasted (a path a shell can't take, one | ||
| // with a control character, gets described instead). | ||
| // git resolved a key file to an absolute path (or ~); anything | ||
| // else is a literal key, which can't be ssh-added by name. | ||
| step := "load the signing key into ssh-agent with ssh-add" | ||
| if filepath.IsAbs(se.Key) || strings.HasPrefix(se.Key, "~") { | ||
| if q, ok := shell.Path(strings.TrimSuffix(se.Key, ".pub")); ok { | ||
| step = "load the key into ssh-agent: ssh-add " + q | ||
| } else { | ||
| step = "load the private key next to " + se.Key + " into ssh-agent with ssh-add" | ||
| } | ||
| } | ||
| e = stack.Newf(stack.KindSigningFailed, "commit signing failed: the SSH key %s could not be used (is it unlocked in ssh-agent?)", se.Key). | ||
| WithSteps(step) | ||
|
Comment on lines
+124
to
+133
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A user.signingkey with a control character in it is a config I'm happy to print badly; the Detail already carries whatever git printed, which includes the same path. Leaving this one. |
||
| } | ||
| case "x509": | ||
| e = stack.New(stack.KindSigningFailed, "commit signing failed: the X.509 signer could not sign the commit (it may need a terminal to ask for the passphrase)"). | ||
| WithSteps("check that the signer (gpg.x509.program, gpgsm by default) can sign without a prompt") | ||
| default: | ||
| e = stack.New(stack.KindSigningFailed, "commit signing failed: gpg could not sign the commit (its agent may need a terminal to ask for the passphrase)"). | ||
| WithSteps("check that gpg can sign without a prompt: echo test | gpg --batch --clearsign") | ||
|
Copilot marked this conversation as resolved.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair that it's not definitive, but it's a hint, not a check we act on; the real test is running the command again, which the next steps say. Leaving the wording. |
||
| } | ||
| return e.WithSteps("or turn signing off for this repository: git config commit.gpgsign false", "then run the command again"). | ||
| WithDetail(se.Detail).WithCause(se) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| package git_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/DomBlack/git-stack/pkg/exec" | ||
| "github.com/DomBlack/git-stack/pkg/git" | ||
| "github.com/DomBlack/git-stack/pkg/git/gittest" | ||
| ) | ||
|
|
||
| // TestCommitWithLockedSSHKeyFailsFast signs with a passphrase protected key | ||
| // that is not in any agent, the setup behind the MCP hang: ssh-keygen must | ||
| // fail at once rather than wait for a passphrase, and the failure must come | ||
| // back as a SigningError naming the key. | ||
| func TestCommitWithLockedSSHKeyFailsFast(t *testing.T) { | ||
| gittest.Isolate(t) | ||
| t.Setenv("SSH_AUTH_SOCK", "") | ||
| dir := gittest.InitRepo(t) | ||
| key := filepath.Join(t.TempDir(), "id_ed25519") | ||
| res, err := exec.New().Run(context.Background(), exec.Cmd{ | ||
| Name: "ssh-keygen", Args: []string{"-q", "-t", "ed25519", "-N", "secret", "-C", "test", "-f", key}, | ||
| }) | ||
| if _, missing := errors.AsType[*exec.NotFoundError](err); missing { | ||
| t.Skip("ssh-keygen not installed") | ||
| } | ||
| if err != nil { | ||
| t.Fatalf("ssh-keygen: %v\n%s", err, res.Err()) | ||
| } | ||
| gittest.Run(t, dir, "config", "gpg.format", "ssh") | ||
| gittest.Run(t, dir, "config", "user.signingkey", key+".pub") | ||
| gittest.Run(t, dir, "config", "commit.gpgsign", "true") | ||
| gittest.WriteFile(t, dir, "a.txt", "a\n") | ||
| gittest.Run(t, dir, "add", "a.txt") | ||
|
|
||
| c := newClient() | ||
| ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) | ||
| defer cancel() | ||
| repo, _ := c.Discover(ctx, dir) | ||
| start := time.Now() | ||
| _, err = c.Commit(ctx, repo, git.CommitOptions{Message: []string{"signed"}}) | ||
| if errors.Is(err, context.DeadlineExceeded) { | ||
| t.Fatal("git commit waited on the passphrase prompt instead of failing") | ||
| } | ||
| se, ok := errors.AsType[*git.SigningError](err) | ||
| if !ok { | ||
| t.Fatalf("err = %v (%T), want *git.SigningError", err, err) | ||
| } | ||
| if se.Format != "ssh" || se.Key != key+".pub" { | ||
| t.Errorf("SigningError = %+v, want format ssh and key %s.pub", se, key) | ||
| } | ||
| if !strings.Contains(se.Detail, "failed to write commit object") { | ||
| t.Errorf("Detail = %q, want git's stderr", se.Detail) | ||
| } | ||
| if time.Since(start) > 10*time.Second { | ||
| t.Errorf("commit took %s", time.Since(start)) | ||
| } | ||
| if staged, _ := c.HasStagedChanges(ctx, repo); !staged { | ||
| t.Error("the staged changes should still be staged") | ||
| } | ||
| } | ||
|
|
||
| // TestCommitObjectWriteFailureIsNotSigning makes git print "failed to write | ||
| // commit object" for a reason that has nothing to do with signing (a read | ||
| // only object store); that must stay an ordinary error. | ||
| func TestCommitObjectWriteFailureIsNotSigning(t *testing.T) { | ||
| if os.Geteuid() == 0 { | ||
| t.Skip("root can write to a read only directory") | ||
| } | ||
| gittest.Isolate(t) | ||
| dir := gittest.InitRepo(t) | ||
| gittest.WriteFile(t, dir, "a.txt", "a\n") | ||
| gittest.Run(t, dir, "add", "a.txt") | ||
| objects := filepath.Join(dir, ".git", "objects") | ||
| if err := os.Chmod(objects, 0o555); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| t.Cleanup(func() { _ = os.Chmod(objects, 0o755) }) | ||
|
|
||
| c := newClient() | ||
| ctx := context.Background() | ||
| repo, _ := c.Discover(ctx, dir) | ||
| _, err := c.Commit(ctx, repo, git.CommitOptions{Message: []string{"x"}}) | ||
| if err == nil { | ||
| t.Fatal("the commit should fail with a read only object store") | ||
| } | ||
| if _, ok := errors.AsType[*git.SigningError](err); ok { | ||
| t.Fatalf("err = %v, classified as a signing failure", err) | ||
| } | ||
| if !strings.Contains(err.Error(), "git commit:") { | ||
| t.Errorf("err = %v, want git's message", err) | ||
| } | ||
| } | ||
|
|
||
| // TestCommitBareGpgsignKeyIsSigning: git reads `[commit] gpgsign` with no | ||
| // value as true, so a signing failure under it must still be recognised. | ||
| func TestCommitBareGpgsignKeyIsSigning(t *testing.T) { | ||
| gittest.Isolate(t) | ||
| dir := gittest.InitRepo(t) | ||
| f, err := os.OpenFile(filepath.Join(dir, ".git", "config"), os.O_APPEND|os.O_WRONLY, 0o644) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if _, err := f.WriteString("[commit]\n\tgpgsign\n"); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| _ = f.Close() | ||
| gittest.Run(t, dir, "config", "gpg.format", "ssh") | ||
| gittest.Run(t, dir, "config", "gpg.ssh.program", "false") | ||
| gittest.Run(t, dir, "config", "user.signingkey", "keys/id_ed25519.pub") | ||
| gittest.WriteFile(t, dir, "keys/id_ed25519.pub", "ssh-ed25519 AAAAC3Nza test\n") | ||
| gittest.WriteFile(t, dir, "a.txt", "a\n") | ||
| gittest.Run(t, dir, "add", "a.txt") | ||
|
|
||
| c := newClient() | ||
| ctx := context.Background() | ||
| repo, _ := c.Discover(ctx, dir) | ||
| _, err = c.Commit(ctx, repo, git.CommitOptions{Message: []string{"x"}}) | ||
| se, ok := errors.AsType[*git.SigningError](err) | ||
| if !ok { | ||
| t.Fatalf("err = %v (%T), want *git.SigningError", err, err) | ||
| } | ||
| // A relative key file is resolved against the repository. | ||
| if want := filepath.Join(repo.TopLevel, "keys", "id_ed25519.pub"); se.Key != want { | ||
| t.Errorf("Key = %q, want %q", se.Key, want) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what #29 in the same stack is for: create undoes the branch and its registration when the commit fails, so running it again is the right recovery there. On its own this PR only improves the message, and I'd rather not grow a second recovery path that #29 then deletes.