Add SSH commit signing for HEAD commits - #2789
Conversation
|
Thanks for the contribution @kausters. I'm out for some time so I probably won't review it this week. I'm also looking to push out the revamped release I've had in the continuous channel for quite some time to stable. So ballpark 2+ weeks from now we'll get this in a build if all looks good. |
|
@greptile Can you review this PR? |
|
| Filename | Overview |
|---|---|
| GitUpKit/Core/GCCommitSigning.m | New file implementing SSH commit signing. The libgit2 buffer/sign/create flow is correct, but _CommitSigningPATH uses an un-synchronized process-global static cache that is never invalidated. |
| GitUpKit/Core/GCRepository+HEAD.m | Routes all three user-facing HEAD commit operations (create, merge, amend) through the new signing-aware helpers. Parents array and count logic is preserved correctly. |
| GitUpKit/Core/GCCommitSigning-Tests.m | Good coverage of signing scenarios including disabled, unsupported format, inline key, defaultKeyCommand, path key, and signer failure. Duplicates _CommitSignature and setup helpers that also appear in the HEAD test file. |
| GitUpKit/Core/GCRepository+HEAD-Tests.m | Adds integration test that verifies normal, merge, and amend commits are all SSH-signed and verifiable by the Git CLI. Contains duplicated _CommitSignature and _ConfigureSSHSigningWithKeyPath helpers. |
| GitUpKit/Core/GCPrivate.h | Adds declarations for the two new public signing helpers and the getPATHUsingShell: method. Changes are minimal and correct. |
| GitUpKit/GitUpKit.xcodeproj/project.pbxproj | Adds GCCommitSigning.m and GCCommitSigning-Tests.m to all three relevant build targets (framework, unit test, and iOS framework). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User triggers commit\n(create / merge / amend)"] --> B["createCommitFromHEAD…\n(GCRepository+HEAD.m)"]
B --> C["_CreateCommitFromIndex /\nGCCreateCommitFromCommitWithIndexAndOptionalSignature"]
C --> D["GCCreateCommitFromTreeWithOptionalSignature"]
D --> E{"_ShouldSSHSignCommit"}
E -- "commit.gpgsign=false\nor gpg.format ≠ ssh" --> F["git_commit_create\n(unsigned)"]
E -- "gpg.format = ssh" --> G["git_commit_create_buffer\n(build commit content)"]
G --> H["_SSHSignatureForCommitBuffer"]
H --> I["_SSHSigningKeyPath\nresolve key from user.signingkey\nor gpg.ssh.defaultKeyCommand"]
I --> J["_CommitSigningPATH\nresolve shell PATH (cached)"]
J --> K["/usr/bin/env gpg.ssh.program\nor ssh-keygen -Y sign -n git -f key"]
K -- "non-zero exit" --> L["Error: signer failed"]
K -- "signature on stdout" --> M["git_commit_create_with_signature\n(write signed object)"]
M --> N["git_commit_lookup → GCCommit"]
F --> N
N --> O["Update HEAD reference\n(reflog)"]
Comments Outside Diff (1)
-
GitUpKit/Core/GCCommitSigning.m, line 317-325 (link)Process-global PATH cache is never invalidated
cachedPATHis astaticlocal that is populated on the first signing call and then reused for the entire process lifetime — across all repositories, users, and sessions in the same GitUp process. Two consequences: (1) if the first PATH resolution succeeds via the fallback/bin/shbut the primary$SHELLwas temporarily unavailable, every subsequent signing will silently use the narrower/bin/shPATH even after the shell recovers; (2) in tests, if one test initialises the cache with a stripped PATH, later tests inherit it.This is the same one-shot caching pattern used by
runHookWithName:inGCRepository.m, so it is consistent with the existing codebase. Still worth flagging because signing is more security-sensitive than hook execution: an unexpected tool being resolved from a cached PATH could silently produce commits signed by the wrong key program.
Reviews (1): Last reviewed commit: "Hide amend parent handling in signing he..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| NSString* signature = [[NSString alloc] initWithBytes:buffer.ptr length:buffer.size encoding:NSUTF8StringEncoding]; | ||
| git_buf_free(&buffer); | ||
| return signature; | ||
| } | ||
|
|
||
| static BOOL _WriteLocalConfigOption(GCRepository* repository, NSString* variable, NSString* value) { | ||
| return [repository writeConfigOptionForLevel:kGCConfigLevel_Local variable:variable withValue:value error:NULL]; | ||
| } | ||
|
|
There was a problem hiding this comment.
_CommitSignature() is copy-pasted character-for-character into GCRepository+HEAD-Tests.m (lines 581–591 of that file). _ConfigureSSHSigningWithKeyPath also appears in both files, with slightly different implementations (one uses a _WriteLocalConfigOption wrapper, the other calls writeConfigOptionForLevel:variable:withValue:error: directly). If either helper needs to change — for example, to read a different signature header field for a future signing format — the two copies will diverge silently.
Both helpers should be extracted into GCTestCase (or a dedicated signing test utility category) so they are shared by both test files.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
@lucasderraugh not sure what the project convention is for shared test helpers across files, happy to adjust or leave it as-is, up to you
There was a problem hiding this comment.
Yes, I think reusing this same call would be ideal. A small file with helpers in the test target would be fine.
| CALL_LIBGIT2_FUNCTION_GOTO(cleanup, git_tree_lookup, &tree, repository.private, &oid); | ||
|
|
||
| parentCount = git_commit_parentcount(amendedCommit); | ||
| if (parentCount) { | ||
| parentCommits = (git_commit**)calloc(parentCount, sizeof(git_commit*)); | ||
| if (!parentCommits) { | ||
| GC_SET_GENERIC_ERROR(@"Unable to allocate commit parent list"); | ||
| goto cleanup; | ||
| } | ||
| for (unsigned int i = 0; i < parentCount; ++i) { | ||
| CALL_LIBGIT2_FUNCTION_GOTO(cleanup, git_commit_parent, &parentCommits[i], amendedCommit, i); | ||
| } | ||
| } | ||
| parents = (const git_commit**)parentCommits; | ||
|
|
||
| commit = GCCreateCommitFromTreeWithOptionalSignature(repository, tree, parents, parentCount, git_commit_author(amendedCommit), message, error); | ||
|
|
||
| cleanup: | ||
| if (parentCommits) { | ||
| for (unsigned int i = 0; i < parentCount; ++i) { | ||
| git_commit_free(parentCommits[i]); |
There was a problem hiding this comment.
commit.gpgsign=true but format is not SSH
When gpg.format is set to openpgp or x509 (or any non-SSH value), _ShouldSSHSignCommit sets *shouldSign = NO and returns YES, causing GCCreateCommitFromTreeWithOptionalSignature to silently produce an unsigned commit. The test testCommitSigningLeavesCommitsUnsignedWhenDisabledOrUnsupported documents this behaviour, and the PR description explicitly acknowledges the tradeoff.
The concern is that a repository administrator who enforces commit.gpgsign=true with OpenPGP will silently receive unsigned commits from GitUp with no warning. Would it be worth surfacing a non-fatal console log (XLOG_WARNING) so that developers using GitUp in such repositories at least see a diagnostic message, even if the commit still succeeds?
There was a problem hiding this comment.
I think this can be ignored.
2 participants
Summary
Hi! This PR adds SSH commit signing support for GitUpKit user-facing HEAD commit flows. Repositories configured like the Git CLI with
commit.gpgsign=trueandgpg.format=sshnow create commits with agpgsigSSH signature for normal commits, merge commits, amend, and conflict-resolution commits.To limit changes scope, the change intentionally supports only SSH signing. Non-SSH signing formats, including OpenPGP and X.509, continue to produce unsigned GitUp commits rather than blocking existing workflows. History replay/reorder paths also remain unsigned; signing those GitUp-created rewrite commits can be considered separately from signing commits users make directly.
What changed
GCCommitSigning.mhelper that creates the unsigned commit buffer, signs it with SSH, and writes the signed object through libgit2.user.signingkey, inline SSH public keys, andgpg.ssh.defaultKeyCommand.gpg.ssh.programorssh-keygen, launched with GitUp login-shell PATH handling so GUI-launched GitUp can find shell-installed signers.Tests
verify-commitverification with an allowed signers file.Notes