Skip to content

fix(keys-sync): require a compliant signing-capable key for the verdict - #1635

Merged
potiuk merged 2 commits into
apache:mainfrom
liwenjie200543:fix/keys-sync-signer-admission
Oct 16, 2026
Merged

fix(keys-sync): require a compliant signing-capable key for the verdict#1635
potiuk merged 2 commits into
apache:mainfrom
liwenjie200543:fix/keys-sync-signer-admission

Conversation

@liwenjie200543

@liwenjie200543 liwenjie200543 commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

What

keys-sync Step 1 (check_key.py) now requires a usable, strength-compliant signer, not just a strong primary key:

  • parse_colons() records subkeys and the gpg colons capability flags (field 12), and attaches each subkey's fingerprint.
  • The verdict additionally requires a signing-capable key: a primary key whose capabilities lack s (certify only) no longer counts as a signer on its own, and a signing subkey only counts when it independently meets the existing algorithm/size floor, validity and expiry rules.
  • The compliant signers are reported in a new signers field; listings without capability fields (older synthetic fixtures) keep the previous behaviour.

Why

Fixes #1628 (helper layer). Previously a certify-only Ed25519 primary with an RSA-1024 signing subkey got verdict: proceed with strength_check: pass, because only the primary's strength was assessed — the helper's own floor was bypassed by the actual signer.

How checked

  • python -m unittest discover -s plugins/magpie-release-management/skills/keys-sync/tests — 40/40 pass (8 new SignerAdmissionTest cases cover the issue's reproduction shape: certify-only primary + weak/missing/expired/compliant signing subkey).
  • ruff check and ruff format --check pass on both files.
  • Fixtures use the real gpg colons field layout (capabilities at 0-based index 11, curve at 16) per GnuPG doc/DETAILS.

Scope note: this changes only the helper's admission rules; the broader workflow proposal (binding rc-cut signing to the validated key via --local-user) is left as the separate discussion the issue describes.

@liwenjie200543
liwenjie200543 force-pushed the fix/keys-sync-signer-admission branch from 03479c3 to f0f8279 Compare October 15, 2026 12:51
@github-actions github-actions Bot added capability:resolve Close-out: invalidate, dedupe, CVE allocate, announcement family:release-management release-* skills labels Oct 15, 2026
liwenjie200543 and others added 2 commits October 16, 2026 02:27
@liwenjie200543 @potiuk
fix(keys-sync): require a compliant signing-capable key for the verdict
5926d82 
The Step 1 helper validated only the primary key's strength, so a
certify-only primary with a weak (or missing) signing subkey still got
verdict: proceed. parse_colons now records subkeys and gpg capability
flags, and the verdict additionally requires a usable signing-capable
key: a primary without the sign capability needs a compliant signing
subkey, and a signing subkey must independently meet the existing
strength, validity and expiry rules. Compliant signers are reported in
the new signers field. Listings without capability fields keep the
previous behaviour. Fixes apache#1628 (helper layer).
@potiuk
fixup! fix(keys-sync): require a compliant signing-capable key for th…
d898932 
…e verdict

Generated-by: GitHub Copilot (Claude Sonnet 5.5)
@potiuk
potiuk force-pushed the fix/keys-sync-signer-admission branch from f0f8279 to d898932 Compare October 16, 2026 01:28
@potiuk
potiuk merged commit 3846714 into apache:main Oct 16, 2026
10 checks passed
@potiuk

potiuk commented Oct 16, 2026

Copy link
Copy Markdown
Member

Pushed a fixup (maintainer edit) before merge, after review and a roborev pass:

  • Capability letters keep their case. GnuPG reports uppercase aggregate capabilities for the primary key (for example cSC for a certify-only primary whose subkey signs), and lowercasing them made such a primary count as a signer. Only a lowercase s now admits the primary.
  • Admitted signing subkeys now carry the DSA and 90-day expiry advisories in strength_note, as the keys-sync skill's Step 1 describes.
  • Added regression tests for all three cases; the keys-sync tests pass and CI was green.

This was referenced Oct 16, 2026
Merged
Merged
potiuk added a commit to potiuk/magpie that referenced this pull request Oct 16, 2026
@liwenjie200543 @potiuk
fix(keys-sync): require a compliant signing-capable key for the verdi…
6083540 
…ct (apache#1635)

* fix(keys-sync): require a compliant signing-capable key for the verdict

The Step 1 helper validated only the primary key's strength, so a
certify-only primary with a weak (or missing) signing subkey still got
verdict: proceed. parse_colons now records subkeys and gpg capability
flags, and the verdict additionally requires a usable signing-capable
key: a primary without the sign capability needs a compliant signing
subkey, and a signing subkey must independently meet the existing
strength, validity and expiry rules. Compliant signers are reported in
the new signers field. Listings without capability fields keep the
previous behaviour. Fixes apache#1628 (helper layer).

* fixup! fix(keys-sync): require a compliant signing-capable key for the verdict

Generated-by: GitHub Copilot (Claude Sonnet 5.5)

---------

Co-authored-by: Jarek Potiuk <potiuk@apache.org>
potiuk added a commit to Kaap10/magpie that referenced this pull request Oct 16, 2026
@liwenjie200543 @potiuk
fix(keys-sync): require a compliant signing-capable key for the verdi…
0429718 
…ct (apache#1635)

* fix(keys-sync): require a compliant signing-capable key for the verdict

The Step 1 helper validated only the primary key's strength, so a
certify-only primary with a weak (or missing) signing subkey still got
verdict: proceed. parse_colons now records subkeys and gpg capability
flags, and the verdict additionally requires a usable signing-capable
key: a primary without the sign capability needs a compliant signing
subkey, and a signing subkey must independently meet the existing
strength, validity and expiry rules. Compliant signers are reported in
the new signers field. Listings without capability fields keep the
previous behaviour. Fixes apache#1628 (helper layer).

* fixup! fix(keys-sync): require a compliant signing-capable key for the verdict

Generated-by: GitHub Copilot (Claude Sonnet 5.5)

---------

Co-authored-by: Jarek Potiuk <potiuk@apache.org>
potiuk added a commit that referenced this pull request Oct 16, 2026
@potiuk
fix(release-keys-sync, release-archive-sweep): carry main's key-check…
8180f2e 
…er fixes into the tool and read S3 and version paths right

Port of the key-checker fixes main landed on the script this series moves
into tools/release-management (#1634, #1635, #1646):

- A key passes only with a signing-capable primary or subkey that itself
  meets the strength floor and is not revoked, invalid, disabled or
  expired; a certify-only primary with no such subkey is blocked
  (`signers` in the check output).
- GPG failures are reported, never read as "key not found": unreadable or
  non-file input, a failed import or listing, and gpg failing to start are
  errors naming the stage and exit code, with the "no key data" exception
  ported as is. release-verify list-keys, which runs gpg for keys-sync,
  now also reports each key's capabilities and subkeys.
- Every test of main's test_check_key.py has a ported equivalent.

Adversarial-review findings:

- archive-sweep reads S3 URLs with promote's whole-host path-style rule,
  so a virtual-hosted bucket whose name starts with "s3" keeps its key.
- A release_dist_url_template with a literal around <version> (v<version>,
  foo-<version>-bin) gives the right release directory and archive URLs.
- keys-sync reads a GitHub KEYS URL as branch (the first segment after
  blob/) plus a path that may include subdirectories, so KEYS can live
  in e.g. dist/; a new optional keys_file_branch names a branch that
  contains '/' (release/1.x). Without it, a subdirectory reading is
  warned; a URL whose file is not KEYS is refused.
- The keyserver's UIDs are compared with KEYS as a set, so a reordered
  keyserver listing is no longer reported as a rolled key.

Generated-by: Claude Opus 5
potiuk added a commit that referenced this pull request Oct 16, 2026
@potiuk
fix(release-keys-sync, release-archive-sweep): carry main's key-check…
897023c 
…er fixes into the tool and read S3 and version paths right

Port of the key-checker fixes main landed on the script this series moves
into tools/release-management (#1634, #1635, #1646):

- A key passes only with a signing-capable primary or subkey that itself
  meets the strength floor and is not revoked, invalid, disabled or
  expired; a certify-only primary with no such subkey is blocked
  (`signers` in the check output).
- GPG failures are reported, never read as "key not found": unreadable or
  non-file input, a failed import or listing, and gpg failing to start are
  errors naming the stage and exit code, with the "no key data" exception
  ported as is. release-verify list-keys, which runs gpg for keys-sync,
  now also reports each key's capabilities and subkeys.
- Every test of main's test_check_key.py has a ported equivalent.

Adversarial-review findings:

- archive-sweep reads S3 URLs with promote's whole-host path-style rule,
  so a virtual-hosted bucket whose name starts with "s3" keeps its key.
- A release_dist_url_template with a literal around <version> (v<version>,
  foo-<version>-bin) gives the right release directory and archive URLs.
- keys-sync reads a GitHub KEYS URL as branch (the first segment after
  blob/) plus a path that may include subdirectories, so KEYS can live
  in e.g. dist/; a new optional keys_file_branch names a branch that
  contains '/' (release/1.x). Without it, a subdirectory reading is
  warned; a URL whose file is not KEYS is refused.
- The keyserver's UIDs are compared with KEYS as a set, so a reordered
  keyserver listing is no longer reported as a rolled key.

Generated-by: Claude Opus 5
potiuk added a commit that referenced this pull request Oct 16, 2026
@potiuk
fix(release-keys-sync, release-archive-sweep): carry main's key-check…
02fcdf7 
…er fixes into the tool and read S3 and version paths right

Port of the key-checker fixes main landed on the script this series moves
into tools/release-management (#1634, #1635, #1646):

- A key passes only with a signing-capable primary or subkey that itself
  meets the strength floor and is not revoked, invalid, disabled or
  expired; a certify-only primary with no such subkey is blocked
  (`signers` in the check output).
- GPG failures are reported, never read as "key not found": unreadable or
  non-file input, a failed import or listing, and gpg failing to start are
  errors naming the stage and exit code, with the "no key data" exception
  ported as is. release-verify list-keys, which runs gpg for keys-sync,
  now also reports each key's capabilities and subkeys.
- Every test of main's test_check_key.py has a ported equivalent.

Adversarial-review findings:

- archive-sweep reads S3 URLs with promote's whole-host path-style rule,
  so a virtual-hosted bucket whose name starts with "s3" keeps its key.
- A release_dist_url_template with a literal around <version> (v<version>,
  foo-<version>-bin) gives the right release directory and archive URLs.
- keys-sync reads a GitHub KEYS URL as branch (the first segment after
  blob/) plus a path that may include subdirectories, so KEYS can live
  in e.g. dist/; a new optional keys_file_branch names a branch that
  contains '/' (release/1.x). Without it, a subdirectory reading is
  warned; a URL whose file is not KEYS is refused.
- The keyserver's UIDs are compared with KEYS as a set, so a reordered
  keyserver listing is no longer reported as a rolled key.

Generated-by: Claude Opus 5
potiuk added a commit that referenced this pull request Oct 16, 2026
@potiuk
fix(release-keys-sync, release-archive-sweep): carry main's key-check…
0b6fa2a 
…er fixes into the tool and read S3 and version paths right

Port of the key-checker fixes main landed on the script this series moves
into tools/release-management (#1634, #1635, #1646):

- A key passes only with a signing-capable primary or subkey that itself
  meets the strength floor and is not revoked, invalid, disabled or
  expired; a certify-only primary with no such subkey is blocked
  (`signers` in the check output).
- GPG failures are reported, never read as "key not found": unreadable or
  non-file input, a failed import or listing, and gpg failing to start are
  errors naming the stage and exit code, with the "no key data" exception
  ported as is. release-verify list-keys, which runs gpg for keys-sync,
  now also reports each key's capabilities and subkeys.
- Every test of main's test_check_key.py has a ported equivalent.

Adversarial-review findings:

- archive-sweep reads S3 URLs with promote's whole-host path-style rule,
  so a virtual-hosted bucket whose name starts with "s3" keeps its key.
- A release_dist_url_template with a literal around <version> (v<version>,
  foo-<version>-bin) gives the right release directory and archive URLs.
- keys-sync reads a GitHub KEYS URL as branch (the first segment after
  blob/) plus a path that may include subdirectories, so KEYS can live
  in e.g. dist/; a new optional keys_file_branch names a branch that
  contains '/' (release/1.x). Without it, a subdirectory reading is
  warned; a URL whose file is not KEYS is refused.
- The keyserver's UIDs are compared with KEYS as a set, so a reordered
  keyserver listing is no longer reported as a rolled key.

Generated-by: Claude Opus 5
potiuk added a commit that referenced this pull request Oct 16, 2026
@potiuk
fix(release-keys-sync, release-archive-sweep): carry main's key-check…
754a0be 
…er fixes into the tool and read S3 and version paths right

Port of the key-checker fixes main landed on the script this series moves
into tools/release-management (#1634, #1635, #1646):

- A key passes only with a signing-capable primary or subkey that itself
  meets the strength floor and is not revoked, invalid, disabled or
  expired; a certify-only primary with no such subkey is blocked
  (`signers` in the check output).
- GPG failures are reported, never read as "key not found": unreadable or
  non-file input, a failed import or listing, and gpg failing to start are
  errors naming the stage and exit code, with the "no key data" exception
  ported as is. release-verify list-keys, which runs gpg for keys-sync,
  now also reports each key's capabilities and subkeys.
- Every test of main's test_check_key.py has a ported equivalent.

Adversarial-review findings:

- archive-sweep reads S3 URLs with promote's whole-host path-style rule,
  so a virtual-hosted bucket whose name starts with "s3" keeps its key.
- A release_dist_url_template with a literal around <version> (v<version>,
  foo-<version>-bin) gives the right release directory and archive URLs.
- keys-sync reads a GitHub KEYS URL as branch (the first segment after
  blob/) plus a path that may include subdirectories, so KEYS can live
  in e.g. dist/; a new optional keys_file_branch names a branch that
  contains '/' (release/1.x). Without it, a subdirectory reading is
  warned; a URL whose file is not KEYS is refused.
- The keyserver's UIDs are compared with KEYS as a set, so a reordered
  keyserver listing is no longer reported as a rolled key.

Generated-by: Claude Opus 5
potiuk added a commit that referenced this pull request Oct 16, 2026
@potiuk
fix(release-keys-sync, release-archive-sweep): carry main's key-check…
f33ed9f 
…er fixes into the tool and read S3 and version paths right

Port of the key-checker fixes main landed on the script this series moves
into tools/release-management (#1634, #1635, #1646):

- A key passes only with a signing-capable primary or subkey that itself
  meets the strength floor and is not revoked, invalid, disabled or
  expired; a certify-only primary with no such subkey is blocked
  (`signers` in the check output).
- GPG failures are reported, never read as "key not found": unreadable or
  non-file input, a failed import or listing, and gpg failing to start are
  errors naming the stage and exit code, with the "no key data" exception
  ported as is. release-verify list-keys, which runs gpg for keys-sync,
  now also reports each key's capabilities and subkeys.
- Every test of main's test_check_key.py has a ported equivalent.

Adversarial-review findings:

- archive-sweep reads S3 URLs with promote's whole-host path-style rule,
  so a virtual-hosted bucket whose name starts with "s3" keeps its key.
- A release_dist_url_template with a literal around <version> (v<version>,
  foo-<version>-bin) gives the right release directory and archive URLs.
- keys-sync reads a GitHub KEYS URL as branch (the first segment after
  blob/) plus a path that may include subdirectories, so KEYS can live
  in e.g. dist/; a new optional keys_file_branch names a branch that
  contains '/' (release/1.x). Without it, a subdirectory reading is
  warned; a URL whose file is not KEYS is refused.
- The keyserver's UIDs are compared with KEYS as a set, so a reordered
  keyserver listing is no longer reported as a rolled key.

Generated-by: Claude Opus 5
potiuk added a commit that referenced this pull request Oct 16, 2026
@potiuk
fix(release-keys-sync, release-archive-sweep): carry main's key-check…
7a45dd7 
…er fixes into the tool and read S3 and version paths right

Port of the key-checker fixes main landed on the script this series moves
into tools/release-management (#1634, #1635, #1646):

- A key passes only with a signing-capable primary or subkey that itself
  meets the strength floor and is not revoked, invalid, disabled or
  expired; a certify-only primary with no such subkey is blocked
  (`signers` in the check output).
- GPG failures are reported, never read as "key not found": unreadable or
  non-file input, a failed import or listing, and gpg failing to start are
  errors naming the stage and exit code, with the "no key data" exception
  ported as is. release-verify list-keys, which runs gpg for keys-sync,
  now also reports each key's capabilities and subkeys.
- Every test of main's test_check_key.py has a ported equivalent.

Adversarial-review findings:

- archive-sweep reads S3 URLs with promote's whole-host path-style rule,
  so a virtual-hosted bucket whose name starts with "s3" keeps its key.
- A release_dist_url_template with a literal around <version> (v<version>,
  foo-<version>-bin) gives the right release directory and archive URLs.
- keys-sync reads a GitHub KEYS URL as branch (the first segment after
  blob/) plus a path that may include subdirectories, so KEYS can live
  in e.g. dist/; a new optional keys_file_branch names a branch that
  contains '/' (release/1.x). Without it, a subdirectory reading is
  warned; a URL whose file is not KEYS is refused.
- The keyserver's UIDs are compared with KEYS as a set, so a reordered
  keyserver listing is no longer reported as a rolled key.

Generated-by: Claude Opus 5
potiuk added a commit that referenced this pull request Oct 16, 2026
@potiuk
perf(release-audit-report, release-archive-sweep, release-keys-sync):…
5fc6a32 
… decide and render as code (#1625)

* perf(release-audit-report, release-archive-sweep, release-keys-sync): decide and render as code

The last three release skills each had one rule in a skill-local script
and the rest in prose. The scripts move unchanged into
tools/release-management (render_record → audit_report.record,
retention → archive_sweep, check_key → keys_sync), and new commands do
what the agent did by hand, printing JSON and running nothing:

- audit-report: `verdict` (planning issue found, `org/repo#n`
  normalised to the canonical URL), `gather` (PonyMail subjects and
  queries, the `svn info` revision command, the required fields; the
  search itself stays with the agent), `voters` (the [RESULT] voters
  mapped to roster handles with vote-tally's binding resolution,
  unresolved ones listed, never cited), `check` (Step 1 missing /
  redacted fields, schema violations, the thread-permalink shape),
  `record`, and `pr`.
- archive-sweep: `verdict`, `listing`, `trains` (strict `N.x` lines
  become patterns; a free-text retention rule leaves `keep` to the
  agent), `retention`, and `render` for svn mv, gh release delete (with
  the permanent-deletion warning), aws s3 mv, self-hosted, and atr
  (no command).
- keys-sync: whether the fingerprint is in KEYS and its UID matches the
  keyserver's is no longer a text scan. A new read-only
  `release-verify list-keys` lists fingerprints and UIDs in the
  existing throwaway GNUPGHOME (no agent, no keyring, no network,
  private-key material refused), and `keys-sync verdict` classifies it
  as proceed / noop / blocked, failing closed when it cannot compare.
  `render` draws the `# <uid>` KEYS block, the svn / git / generic
  sequence and the upload reminder with the expiry advisory.

Every rendered command quotes its values with the shared shell helpers.
Each backend and outcome gets its own file; audit-report's PR step
moves, byte-identical with its generated adversarial block, to
audit-log-pr.md at the skill root. Golden rules, Hard rules and the
eval headings are unchanged. audit-report 6,657 → 5,136 tokens,
archive-sweep 4,573 → 4,165, keys-sync 4,871 → 4,584.

Evals: the scripted cases become pytest cases named after each former
case (expected records byte-identical); the judgement and injection
cases stay, their fixtures now carrying the real tool output.

Generated-by: Claude Opus 5
Claude-Session: https://claude.ai/code/session_01GTPsZ5aEE5bUVYqp47Dpvv

* fix(release-archive-sweep, release-keys-sync, release-audit-report): refuse what the rules forbid instead of defaulting

Review findings on the three tools:

- archive-sweep: `retention` refuses a trains file whose retention rule
  was not decided (keep_decided false) unless every train names its keep,
  so a free-text rule no longer falls back to keep=1 and archives what
  the rule keeps.
- archive-sweep: `render` refuses a retention output without
  mapping_complete, and any latest-of-line, orphan or pre-release in
  past_retention (Golden rule 2).
- archive-sweep: a trailing -incubating qualifier is a release, not a
  pre-release, so podlings archive their older releases.
- keys-sync: the Step 0 verdict normalises the fingerprint as Step 1
  does (a 0x prefix no longer turns the no-op into a duplicate append).
- keys-sync: Step 2 requires --key-listing and appends only an armour
  holding exactly the key Step 1 checked.
- audit-report: `pr --base` defaults to <default-branch>, not main.

Generated-by: Claude Opus 5

* fix(release-keys-sync, release-archive-sweep): carry main's key-checker fixes into the tool and read S3 and version paths right

Port of the key-checker fixes main landed on the script this series moves
into tools/release-management (#1634, #1635, #1646):

- A key passes only with a signing-capable primary or subkey that itself
  meets the strength floor and is not revoked, invalid, disabled or
  expired; a certify-only primary with no such subkey is blocked
  (`signers` in the check output).
- GPG failures are reported, never read as "key not found": unreadable or
  non-file input, a failed import or listing, and gpg failing to start are
  errors naming the stage and exit code, with the "no key data" exception
  ported as is. release-verify list-keys, which runs gpg for keys-sync,
  now also reports each key's capabilities and subkeys.
- Every test of main's test_check_key.py has a ported equivalent.

Adversarial-review findings:

- archive-sweep reads S3 URLs with promote's whole-host path-style rule,
  so a virtual-hosted bucket whose name starts with "s3" keeps its key.
- A release_dist_url_template with a literal around <version> (v<version>,
  foo-<version>-bin) gives the right release directory and archive URLs.
- keys-sync reads a GitHub KEYS URL as branch (the first segment after
  blob/) plus a path that may include subdirectories, so KEYS can live
  in e.g. dist/; a new optional keys_file_branch names a branch that
  contains '/' (release/1.x). Without it, a subdirectory reading is
  warned; a URL whose file is not KEYS is refused.
- The keyserver's UIDs are compared with KEYS as a set, so a reordered
  keyserver listing is no longer reported as a rolled key.

Generated-by: Claude Opus 5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiuk potiuk potiuk approved these changes

Assignees

No one assigned

Labels

capability:resolve Close-out: invalidate, dedupe, CVE allocate, announcement family:release-management release-* skills

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

keys-sync can approve key groups without a compliant signer; validation is not bound to release signing

2 participants

@liwenjie200543 @potiuk