Aman-Mittal opened a new issue, #428:
URL: https://github.com/apache/fineract-backoffice-ui/issues/428
`main` requires signed commits, and **Verify Commit Signatures** is what
enforces it. It currently answers a different question from the one GitHub
answers, so a PR can be green on the check and show **Unverified** on every
commit. A maintainer then has to notice by eye and ask the contributor to fix
it by hand, which is where this keeps landing.
## Evidence
#392 — the check is `success` on head `af3ef580`, and all three commits:
| commit | check | `verification.verified` | `reason` | signature present |
|---|---|---|---|---|
| `9d7f1793` | ✅ success | `false` | `no_user` | yes |
| `74f75234` | ✅ success | `false` | `no_user` | yes |
| `af3ef580` | ✅ success | `false` | `no_user` | yes |
Not a one-off:
| PR | commits | unverified | reason |
|---|---|---|---|
| #392 | 3 | 3 | `no_user` |
| #331 | 2 | 2 | `no_user` |
| #260 | 1 | 1 | `unsigned` |
```bash
gh api repos/apache/fineract-backoffice-ui/pulls/392/commits \
--jq '.[] | {sha: .sha[0:8], verified: .commit.verification.verified,
reason: .commit.verification.reason}'
```
## Three separate problems
**1. The check asks the wrong question.**
`scripts/verify-signed-commits.sh` treats every `%G?` except `N` as a pass,
and the comment says so plainly — a runner holds no contributor public keys, so
`E`/`U`/`B` are deliberately accepted. That reduces the check to *"is a
signature blob attached?"*. GitHub instead asks *"is this signature made by a
key registered to a GitHub account whose verified email matches the
committer?"* A commit passes the first and fails the second all day. `B` (bad
signature), `R` (revoked key) and `X` (expired signature) are also currently
passes.
**2. The gate is supplied by the branch it gates.**
The workflow triggers on `pull_request` and runs
`./scripts/verify-signed-commits.sh` from the checked-out merge ref — the PR's
copy. A PR can therefore weaken the check that is meant to gate it, and #392's
third commit does exactly that: it adds a `gpgsig` header fallback so that any
signature payload counts as signed, and points git at an empty
`allowed_signers` file.
That commit was itself written to get past the check rather than to satisfy
it (`Co-authored-by: Cursor <[email protected]>`). Whatever the intent, a
gate a contributor can edit from inside the PR is not a gate. **This is the
part worth fixing first**, and it should be resolved independently of whether
#392 lands.
**3. The contributor-side cause in #392 and #331 is not signing at all.**
```
author email : timichris45
committer email: timichris45
gh author acct : None
signature : -----BEGIN SSH SIGNATURE-----
```
`user.email` is a bare username, not an address. GitHub cannot map it to an
account, which is what `no_user` means — so the commits are not attributed to
the PR author either, and no key lookup ever happens. The SSH signing is set up
correctly; the identity is not. No change to signing configuration fixes this,
which is presumably why repeated asking has not converged.
Worth noting: `main`'s current script would *fail* these commits, because
git reports SSH signatures as `%G?=N` without `gpg.ssh.allowedSignersFile`. So
on `main` the check is accidentally right for SSH-signed commits and wrong for
GPG-signed ones — failing for a reason unrelated to whether GitHub can verify
them.
## Suggested fix
Ask GitHub, since GitHub is the authority and its answer cannot be forged
from inside the PR:
```
GET /repos/{owner}/{repo}/pulls/{number}/commits
→ .[].commit.verification.verified // must be true
→ .[].commit.verification.reason // for the failure message
```
Fail on any commit with `verified: false`, and put `reason` in the
annotation — the reasons are distinct and each implies a different remedy:
- `unsigned` — no signature; set up signing
- `no_user` — committer email is not on any GitHub account (**#392, #331**)
- `unverified_email` — email not *verified* on the account
- `unknown_key` / `unknown_signature_type` — key not uploaded to GitHub
- `expired_key`, `bad_signature`, `malformed_signature`
Move the job to `pull_request_target` so it runs the **base branch's**
workflow definition and carries a write token. It needs no repository content
at all — only the API — so it must **not** check out or execute PR code, which
also removes the usual `pull_request_target` hazard. `permissions:
pull-requests: write, contents: read`.
Under the current `pull_request` trigger, `GITHUB_TOKEN` is read-only for
fork PRs, so commenting is not possible without this change.
Keeping `scripts/verify-signed-commits.sh` is still worthwhile as a local
pre-push aid — it just should not be the CI gate.
## The manual step to remove
Have the job post one comment when it fails, naming the offending commits
and the specific `reason`, with the matching remedy and a link to
[CONTRIBUTING.md#commit-signing](../blob/main/CONTRIBUTING.md#commit-signing).
Update rather than append on re-runs, so a PR does not collect a wall of
identical comments.
For `no_user` in particular the message should say the committer email is
not an email address on their GitHub account and point at `git config
user.email` plus:
```bash
git rebase --exec 'git commit --amend --no-edit --reset-author -S'
origin/main
```
> I read *"and also commit if commits are not signed"* as **comment**. If
you did mean having CI amend and sign commits: it cannot for fork PRs (no write
access to the contributor's branch), and re-signing someone else's commit with
a CI key would attest to authorship the runner cannot vouch for. Say the word
and I will reopen that part.
## Also worth doing
- Enable branch protection **Require signed commits** on `main`. Branch
protection is enforced by GitHub itself, so it cannot be edited by a PR and
does not depend on any workflow — the check then becomes early feedback rather
than the only line of defence.
- Revert the CI change in #392 (`af3ef580`) regardless of that PR's fate;
the SSH-signature gap it describes is real, but the fix belongs on `main` on
its own merits, not inside a PR it makes pass.
- The PR template asks for signed commits and this still recurs, so the
message evidently is not landing where people read it. A failing check that
names the exact problem will do more than more template text.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
