Skip to content

fix(security): the org's signed-commits ruleset is active but enforces nothing #132

Description

@devantler

🤖 Generated by the Agentic Engineer

Delivered and verified (2026-09-08)

PR #190 merged as 7130742c555d4a225414043a47dd2c24df7c4d7c and shipped in tag v1.23.6. The signed artifact reached the existing controller through normal Flux reconciliation. Organization ruleset 5397812, Require signed commits, is now Disabled, retains its original identity, empty ref coverage, signature rule and empty bypass list, and permits only Observe and Update. GitHub settings and the healthy controller independently confirm the result.

The complete before/after effective-rules responses on main for .github, actions, platform, monorepo, and ksail are identical: all existing pull-request, linear-history and required-status-check protections remain. There are still zero native signature rules in every case. The September 6 sampled default-branch commits all reported verified: true, reason: valid with committer web-flow. That is historical signed-commit outcome evidence, not proof of native signature enforcement.

The control-integrity defect is resolved: the retained rule no longer advertises active protection that applies nowhere.

The old one-click enforcement recommendation missed a compatibility constraint

GitHub's current signed-commit rules documentation says squash merging into a branch requiring signatures is restricted to the pull-request author. A branch-signing audit alone therefore does not establish compatibility with the engineer merging bot and contributor PRs.

Enabling the rule on ~DEFAULT_BRANCH must not be called a zero-cost change until those merge paths are demonstrated. The earlier audit remains useful historical evidence in the comments, but does not prove this additional condition.

Approved retirement — delivered

The issue's original expected behavior explicitly allowed either enforcement or retirement. The scoped retirement was explicitly approved in the September 8 session. PR #190 retains the already-inert object as a disabled record:

- managementPolicies: ["Observe"]
+ managementPolicies: ["Observe", "Update"]
- enforcement: active
+ enforcement: disabled

Before allowing Update, the implementation backfilled every writable field from the complete observed shape: branch target, original name and external ID, empty ref include/exclude, repository selector ~ALL with no exclusions and protected: false, no bypass actors, and the existing signature rule plus its observed false boolean rule fields. Never permit Create or Delete. This preserves zero effective coverage and does not remove any effective PR, history, or required-check control.

The compensating protection remains the effective pull-request/status/linear-history controls and GitHub-created signed squash commits. The disabled retained record makes the advertised posture honest; it does not implement native signature enforcement.

Historical approval rejection — now resolved

Automatic approval review initially rejected even the local configuration edit because the specific security-policy change lacked explicit authorization. The user has now approved this exact retained-disabled, Observe-and-Update-only proposal. The actual session question and answer were verified, and the independently reviewed patch was applied successfully in the approval-bearing session context. The signed change has now completed review, release and production verification.

Acceptance criteria

  • Obtain explicit approval for the scoped retirement while preserving all effective branch protections.
  • Complete current-head review and CI, and deliver the fully backfilled disabled resource declaratively.
  • Verify the released artifact and the existing live ruleset's disabled state, retained identity and healthy controller status.
  • Compare complete effective branch protections with the saved baseline and record the final disposition here.

All ten repository tests, the installed-CRD schema check and the no-deletion/release guards passed. The change retains all 99 rendered identities and changes only the signing-rule record; 16 unsafe variations are rejected by the new guard. Full-range review found no remaining issues at the final head, and hosted CI passed. Independent signature verification passed for the immutable OCI digest, all 127 packaged source files matched the merged commit byte for byte, and Flux verified and applied that same release. The controller retained the same resource identity and observed the disabled state.

No personal action or delivery work remains for this issue. After disabling the rule, repository-inherited ruleset reads no longer returned it; the authenticated organization settings page and the controller observation supplied the independent disabled-state checks. The complete effective-protection comparison passed for all five repositories.

Activity

  1. moved this to 📥 Backlog in 🌊 Project Boardon Aug 2, 2026
  2. coderabbitai commented on Aug 2, 2026

    @coderabbitai
    🔗 Related PRs

    #74 - feat(rulesets): require the CodeRabbit review status check org-wide [merged]
    #75 - revert(rulesets): drop the provider-uncreatable CodeRabbit required-check ruleset [merged]
    #88 - feat: archived-repo pattern + reusable-workflows archival groundwork [merged]
    #127 - fix(repositories): keep org-enforced signoff out of update payloads [merged]
    #128 - fix(repositories): stop declaring org-enforced signoff so updates can apply [merged]


    📝 Issue Planner

    Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.

    • Create Plan

    🧪 Issue enrichment is currently in open beta.

    You can configure auto-planning by selecting labels in the issue_enrichment configuration.

    To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

    issue_enrichment:
      auto_enrich:
        enabled: false

    💬 Have feedback or questions? Drop into our discord!

  3. devantler commented on Aug 7, 2026

    @devantler
    ContributorAuthor

    🤖 Generated by the Agentic Engineer

    Criterion 4, measured — and it decides the disposition

    The open question was whether requiring signatures would start refusing pushes from any lane that
    does not sign today. It would. Measured 2026-08-07 on the newest commit of a live branch per lane:

    Lane branch commit signed
    claude/* yes
    cursor/* yes
    codex/* no — reason: unsigned
    chore/template-sync_* (the template-sync App) no — reason: unsigned
    Dependabot / Renovate yes — GitHub authors those commits and signs them

    So the two dispositions the issue framed are not equally priced, and neither is quite right:

    • Enforce across branches — refuses the Codex engineering lane and the template-sync App outright.
      That is the DevEx tax criterion 4 exists to catch, and it lands on two automation lanes at once.
    • Retire — gives up a control whose stated scope is achievable at zero cost.

    Recommendation: enforce, scoped to the default branch only. Everything that reaches main is a
    GitHub-created squash commit, which GitHub signs — so on main the rule refuses nothing that happens
    today, closes the stated-vs-actual gap, and makes an unsigned commit on main impossible in future.
    Branch commits stay unsigned-tolerant, so no lane breaks. That is "the secure path is also the easy
    path" rather than a gate to route around.

    Fixing the two unsigned lanes is worth doing on its own merits, but it should be its own work and
    not a precondition — coupling it here is what would turn a free control into a tax.

    The implementation is not a deploy/ edit — and that is the same defect as #141

    deploy/organization-rulesets/require-signed-commits.yaml carries
    managementPolicies: ["Observe"] and an identity-only forProvider (name, target, enforcement). It
    writes nothing, so changing the declared ref condition there would be inert — the config would
    read as if the rule were scoped while live stayed unchanged. That is exactly the class filed as #141
    against Observe-only Repository resources; here it applies to the rulesets.

    Two ways forward, and picking one is the remaining decision:

    1. Maintainer applies the ref condition in the UI, and deploy/ records the live shape. Fastest,
      and consistent with the file's own warning that promoting past Observe needs a status.atProvider
      backfill because the round-trip is lossy.
    2. Promote the ruleset to a managed policy with that backfill first, then express the condition
      declaratively. Correct long-term, larger, and blocked on the same lossy-round-trip work the
      README flags for all ten adopted rulesets.

    Criterion 3's control — an unsigned commit refused on a covered scratch branch, a signed one accepted
    — should run after whichever path lands, against main rather than a scratch branch, since that is
    where the recommended scope applies.

  4. self-assigned this
    on Aug 20, 2026
  5. added a commit that references this issue on Aug 20, 2026
  6. devantler commented on Aug 20, 2026

    @devantler
    ContributorAuthor

    🤖 Generated by the Agentic Engineer

    Disposition: enforce, scoped to ~DEFAULT_BRANCH — and the root cause is a single empty field

    The premise re-verified, the cause located, and criterion 4 (the DevEx-tax check) measured. What is
    left is one org-level edit, which is maintainer authority.

    Root cause — an empty include list matches no refs

    The ruleset is not misconfigured in some subtle way; its ref condition is simply empty:

    Require signed commits (5397812)  enforcement=active  target=branch
      conditions.ref_name = { "include": [], "exclude": [] }
      bypass_actors        = []
    

    Control — the same field on a ruleset that is effective:

    Require linear history (6986179)  enforcement=active  target=branch
      conditions.ref_name = { "include": ["~DEFAULT_BRANCH"], "exclude": [] }
      bypass_actors        = [ OrganizationAdmin / always ]
    

    That is the whole difference. active describes the ruleset; include decides whether it ever
    applies. An empty include matches nothing, so the rule is live and inert at the same time — which is
    exactly the stated-versus-actual gap this issue is about.

    Premise re-verified (the original evidence was 18 days old)

    Re-measured today across 22 non-archived repositories (up from 18):

    Reading Result
    required_signatures effective on default branch 0 / 22
    required_linear_history effective (positive control) 22 / 22

    The control matters: an instrument that reported nothing for every rule would make the zero
    meaningless. It reports 22/22 for a neighbouring rule, so the zero is a measurement.

    Criterion 4 — the lane-signing audit, which is what decides the scope

    Sampled 105 commits across recent merged PRs in monorepo, platform and ksail, plus 43 on
    open PRs. Unsigned commits exist on branches today, from three distinct paths:

    Path Signed Note
    app/ksail-bot release lane (chore/copilot-plugin-v*) 0 / 14 committer devantler-tech-bot[bot], current through 2026-08-19
    megalinter-bot auto-fix pushes onto claude/* branches unsigned CI pushes with a token, not the API
    claude/* local commits 56 / 69 intermittent — same lane, same PR, some signed and some not
    codex/*, app/cursor, dependabot/renovate signed one stale unsigned dependabot commit (ksail#6341, 2026-07-23)

    This is the finding that sets the scope. Enforcing on all branches (~ALL) would start
    refusing pushes from the ksail-bot release lane outright and from MegaLinter's auto-fix, and would
    intermittently refuse this engineer's own lane. That is a real DevEx tax, and it is the second half
    of good DevEx is easy and secure.

    Enforcing on ~DEFAULT_BRANCH costs nothing: every change lands as a GitHub-created squash
    commit, which GitHub signs itself, so the everyday path already satisfies the rule. None of the three
    unsigned paths touches the default branch directly.

    Why enforce rather than retire

    Retiring is honest but throws away a control that is free to switch on. ~DEFAULT_BRANCH matches
    what every other org ruleset already uses, removes the false signal, and adds defence in depth behind
    the pull_request rule — at zero cost to any lane measured above. Retiring would be the right call
    only if enforcement were expensive, and it is not.

    Deliberately not ~ALL. That is the version with teeth for branch commits, and the audit says it
    would break three paths today. If it is ever wanted, the three paths above must be fixed first — that
    is a separate piece of work, not a condition to slip into this change.

    The remaining step is maintainer authority

    Applying it needs admin:org, which this engineer does not hold (GET /orgs/{org}/rulesets →
    403 needs admin:org), so it is genuinely out of reach rather than deferred by choice:

    Ruleset 5397812 → Bypass list / ref conditions → set target branches to ~DEFAULT_BRANCH — i.e.
    match Require linear history. Consider adding the same OrganizationAdmin bypass the other
    rulesets carry; this one currently has none.

    Then criteria 2 and 3 are verifiable and this engineer can run both arms:

    • repos/{owner}/{repo}/rules/branches/main reports required_signatures for all 22 repos.
    • Control: an unsigned commit is refused on a scratch branch covered by the rule, and a signed one is
      accepted — both arms, since a rule that rejects nothing reads identically to a rule that works.

    Blocker: devantler-tech org ruleset 5397812 (maintainer authority — admin:org) | last-verified
    2026-08-20: not applied (ref_name.include still empty; effective on 0/22).

  7. removed their assignment
    on Aug 20, 2026
  8. devantler commented on Aug 25, 2026

    @devantler
    ContributorAuthor

    🤖 Generated by the Agentic Engineer

    Blast radius for this change — a third lane now fails verification

    Flagging an ordering constraint measured today, because enabling required_signatures while it
    holds would take a write lane offline rather than merely tightening provenance.

    devantler-tech/monorepo#3046 records that the Cursor cloud lane's newest commit does not
    verify
    : devantler-tech/actions@22ef0a55 (2026-08-25, open PR actions#1054) reports
    verified=false, reason=unknown_key under cursor[bot]@users.noreply.github.com. Two earlier
    commits from that lane verify cleanly under a different address. Every cursor/* ref across
    monorepo, actions, platform and ksail was enumerated: exactly one Cursor-authored commit
    exists since 2026-08-01, and it is the failing one.

    So the lanes now show three different outcomes — valid, bad_email (monorepo#3020, the claude
    lane) and unknown_key (the cursor lane). Turning on required_signatures before those are
    resolved would reject pushes from at least one lane that is actively opening PRs today.

    This does not argue against the change — the ruleset enforcing nothing is exactly the defect this
    issue is about. It argues for sequencing: resolve #3046 and monorepo#3020 first, then enable,
    so a security control does not land as an availability incident.

    The blocker recorded on this issue (maintainer authority for the org-level ruleset change) is
    unchanged and was last verified 2026-08-24; I did not re-measure it this run.

  9. devantler commented on Sep 2, 2026

    @devantler
    ContributorAuthor

    🤖 Generated by the Agentic Engineer

    Acceptance criterion 4 is now DONE: every lane that pushes commits already signs

    This is the criterion the issue itself names as the long pole and as ordinary agent work — the check that decides whether enforcing becomes a DevEx tax. Measured 2026-09-02T02:2xZ.

    Answer: enforcing required_signatures on branch pushes would refuse nothing that any current lane produces. The second half of good DevEx is easy and secure passes.

    Lane census

    480 pull requests across platform, monorepo, ksail, agent-plugins, actions and world-at-ruin resolve to six identities that push branch commits — devantler (both machine-local instances, 214), app/dependabot (176), app/botantler-1 (53), app/ksail-bot (17), app/renovate (14), app/cursor (6). github-actions[bot] authored none in that sample; see the residual below.

    Evidence

    check population signed and valid
    4 most-recent PR head commits per lane 24 24
    every commit on all 4 live open PR branches 23 23
    app/renovate widened (2026-08-26 → 09-01) 14 14
    cross-lane push: ksail-bot sync commit onto a Dependabot branch (ksail#6826 head) 1 1
    this run's own push (platform ada9e41b) 1 1

    Commits created through the GitHub API report committer web-flow and carry GitHub's own signature; locally-created ones (devantler, cursoragent) carry the lane's key. Both verify valid.

    Negative control — the instrument discriminates

    An all-true result is worthless unless the field can report false, so: ksail commit history page 50 holds 28 commits with verified: false, dated 2024-06-14 → 2024-10-25, committer renovate[bot], reasons unsigned and no_user.

    That is the same discrimination argument the issue makes for the rules endpoint, and it lands on the most useful lane: Renovate did push unsigned commits historically and no longer does — 14 of 14 of its recent heads verify. So the risk this criterion exists to find is real and is already closed, rather than never having existed.

    Residual — what this does not prove

    • It is a sample, not an exhaustive sweep: 24 head commits plus every commit on the 4 live branches, not every commit on every branch in the portfolio.
    • github-actions[bot] authored no PR in the 480 sampled, so if a workflow pushes commits on a path that never opens a PR, that path is uncovered here.

    Neither residual weakens the conclusion much, because the mechanism is structural: every lane except the two machine-local ones creates commits through the GitHub API, which signs them unconditionally.

    Blocker re-verified

    required_signatures is still ABSENT from the effective rules for main on .github, actions, platform, monorepo and ksail, while required_linear_history is PRESENT on all five — so the endpoint still discriminates and the null result still means what it says.

    What remains

    Only the maintainer act: applying the ref condition to the org-level Require signed commits ruleset (or retiring it). Criterion 4's risk is now measured rather than assumed, so that step is a one-click whose blast radius is known: no current lane breaks. Criteria 2 and 3 — reading the effective-rules endpoint back, and the two-armed accept/refuse control on a scratch branch — are verification I can run immediately after it is applied.

  10. self-assigned this
    on Sep 8, 2026
  11. added a commit that references this issue on Sep 8, 2026
  12. moved this from 📥 Backlog to 👀 In Review in 🌊 Project Boardon Sep 8, 2026
  13. moved this from 👀 In Review to ✅ Done in 🌊 Project Boardon Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Fields

No fields configured for Security.

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions