Add script to require signed commits on protected branches - #14
Conversation
Prow branchprotector does not support required_signatures, so this script fills the gap by enabling/disabling it per branch using a YAML config file (signed-commits.yaml). RHWA-788 Assisted-by: Claude claude-opus-4-6
40ad9f8 to
8a27902
Compare
| @@ -0,0 +1,66 @@ | |||
| org: medik8s | |||
There was a problem hiding this comment.
IIUC will need to keep updating this table.
Any chance we can take this information from an existing location ? (somehow keep a single source of truth for that)
[edit] probably low priority issue as IIUC the script is manually triggered
There was a problem hiding this comment.
I think would be better to use regexes here. No need to update the file anymore
There was a problem hiding this comment.
The branches listed here correspond to the supported OCP operator versions per https://access.redhat.com/support/policy/updates/openshift_operators#platform-aligned. So the source of truth is the Red Hat support lifecycle. The list only changes when versions go EOL or new releases ship, and since the script is manually triggered, the maintenance cost is low.
Added a comment at the top of the YAML documenting this in d989e71.
There was a problem hiding this comment.
What about skipping the config file and just use all main & release-* branches on all repos in the org?
There was a problem hiding this comment.
I think would be better to use regexes here
I agree 😁
|
Since this is different from the Signed-off-by required by many OpenShift org repos - do we have (or plan to have) a short doc or link explaining the signed commits requirement and setup ? Would be good to have it ready before rolling this out, maybe linked from CONTRIBUTING.md somewhere ? |
| branches=$(yq ".repos.\"${repo}\".branches[]" "$CONFIG") | ||
|
|
||
| for branch in $branches; do | ||
| api_response=$(gh api "repos/${ORG}/${repo}/branches/${branch}/protection/required_signatures" 2>&1) && \ |
There was a problem hiding this comment.
Nit: any failure will result in "no-protection" response which can shadow other unrelated errors (permission denied, Github API error, network etc...)
| info "$branch — disabled" | ||
| ((changed++)) || true | ||
| else | ||
| warn "$repo/$branch — failed to disable" |
There was a problem hiding this comment.
Nit: would be a good idea to capture and wrap the actual error
| info "$branch — enabled" | ||
| ((changed++)) || true | ||
| else | ||
| warn "$repo/$branch — failed to enable (need admin access?)" |
There was a problem hiding this comment.
Nit: would be a good idea to capture and wrap the actual error
|
Nit: some code duplication in the script that can be avoided |
mshitrit
left a comment
mshitrit
left a comment
There was a problem hiding this comment.
/hold
lgtm, left some non blocking nits.
Giving other chance to review
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mshitrit, razo7 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Distinguish API errors from missing branch protection rules instead of treating all failures as "no-protection". Capture and include actual error messages in warnings for enable/disable operations. Add a comment in signed-commits.yaml documenting that the branch list corresponds to supported OCP operator versions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
New changes are detected. LGTM label has been removed. |
|
Warning Review limit reached
More reviews will be available in 19 minutes and 18 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Bash utility that discovers repositories and branches, checks GitHub required-signatures protection, and enables or disables it with dry-run, confirmation, and authentication checks. It tracks changed, skipped, and failed branches and exits non-zero when failures occur. ChangesSigned commits enforcement
Sequence Diagram(s)sequenceDiagram
participant Script as require-signed-commits.sh
participant gh as gh CLI
participant GitHub as GitHub API
Script->>gh: list repos or branches
gh->>GitHub: GET org/repo/branch data
GitHub-->>gh: repos or branches
gh-->>Script: filtered targets
loop each branch
Script->>gh: read required_signatures
gh->>GitHub: GET required_signatures
GitHub-->>gh: enabled or missing protection
gh-->>Script: current state
alt disable
Script->>gh: DELETE required_signatures
else enable
Script->>gh: POST required_signatures
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/require-signed-commits.sh (1)
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the repeated endpoint URL.
The
repos/${ORG}/${repo}/branches/${branch}/protection/required_signaturespath is built three times (query, DELETE, POST). Extracting it into a per-branch local variable reduces duplication and the chance of the three copies drifting.Also applies to: 78-78, 100-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/require-signed-commits.sh` at line 56, The required signatures API path is duplicated across the branch protection flow, which makes the query, DELETE, and POST calls easy to drift apart. In require-signed-commits.sh, introduce a per-branch local variable for the repos/${ORG}/${repo}/branches/${branch}/protection/required_signatures endpoint and reuse it in the logic around api_response, the DELETE call, and the POST call so all three references stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/require-signed-commits.sh`:
- Around line 56-66: The `gh api` call inside the `api_response` assignment in
`require-signed-commits.sh` is being executed under `set -e`, so a non-zero exit
aborts the script before the `api_exit` handling can run. Update the
branch-protection check around `api_response`, `api_exit`, and the
`current`/`warn` logic so the command failure is captured inline and does not
trigger `set -e`, allowing the existing `no-protection` and API-error paths to
execute as intended.
---
Nitpick comments:
In `@scripts/require-signed-commits.sh`:
- Line 56: The required signatures API path is duplicated across the branch
protection flow, which makes the query, DELETE, and POST calls easy to drift
apart. In require-signed-commits.sh, introduce a per-branch local variable for
the repos/${ORG}/${repo}/branches/${branch}/protection/required_signatures
endpoint and reuse it in the logic around api_response, the DELETE call, and the
POST call so all three references stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af88db1f-a6eb-449e-beac-31e980b8785b
📒 Files selected for processing (2)
scripts/require-signed-commits.shscripts/signed-commits.yaml
The `api_response=$(gh api ...) + api_exit=$?` pattern silently aborts under `set -e` when gh returns non-zero — the assignment fails before `api_exit=$?` executes. Use `|| api_exit=$?` to capture the exit code inline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Require --confirm with --disable to prevent accidental protection removal - Replace unquoted for-loops with while-read to prevent word splitting - Validate org/repo/branch names against [a-zA-Z0-9._-] to block path traversal - Detect wrong yq variant (kislyuk vs mikefarah) at startup - Add timestamps to info/warn output for audit trail - Exit non-zero when any operations fail Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/require-signed-commits.sh`:
- Around line 96-100: The branch discovery logic in the matched_branches
population block is swallowing failures from gh api by discarding stderr and
forcing the pipeline to succeed, so update this flow to surface API errors
instead of treating them as an empty result. Keep the branch filtering around
branch_pattern and the read loop that fills matched_branches, but remove the
suppression that masks gh api failures and add explicit handling so permission,
rate-limit, or network issues cause the script to fail loudly rather than
continuing with no branches.
- Around line 107-111: The branch protection check in the loop over matched
branches rejects valid branch names like release/1.2 because valid_name is too
strict and the gh api path uses the raw branch string. Update valid_name to
allow forward slashes for branch names that should be supported, and make sure
the branch value used in the required_signatures request is URL-encoded before
interpolating it into the gh api path. Keep the existing behavior for invalid
names and continue using matched_branches, valid_name, and the gh api call site
as the main points to adjust.
- Around line 70-75: Validate the constructed branch regex before it is used in
the branch-checking flow, since invalid patterns from --branch can be hidden
later by the grep ... || true fallback. Add a single preflight validation step
in require-signed-commits.sh right after building branch_pattern from BRANCHES
or DEFAULT_BRANCH_PATTERN, and fail fast with a clear error if the regex is
invalid so the later matching logic in the script does not mask it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7dbe33b0-544e-443f-a24d-47a3bef1cb5e
📒 Files selected for processing (1)
scripts/require-signed-commits.sh
Drop signed-commits.yaml and its yq dependency. The script now discovers repos via the GitHub API (non-archived, non-fork) and filters branches with a regex pattern (default: main|release-.+). New flags: --org, --repo (repeatable), --branch (repeatable). Removed: --config. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7e32acc to
390abe4
Compare
Thanks @razo7. I'm happy to help with the CONTRIBUTING.md as part of RHWA-1171. That said, I do think we should settle the scope of RHWA-1171 before merging this - requiring cryptographic signatures from all contributors is unusual in open source and could discourage community participation. A CONTRIBUTING.md explaining the requirement would be the bare minimum, but ideally we'd also discuss whether there's a less restrictive approach (e.g., signatures required only from maintainers, at least two approvals, ok-to-test required from contributors that are not in OWNERS or only on release branches). One other consideration is about OpenShift What I'd propose:
I'd suggest we document these decisions in CONTRIBUTING.md for all repos first, then implement the tooling to match. What do you think? |
I agree that mentioning the need for signing commits in CONTRIBUTING.md won't be enough without something like the Developer Certificate of Origin (DCO) check test to enforce and guide that.
Good point, and I wonder what is best for tackling that 🤔
I like your motivation and the above sounds like a good plan for now. |
5 participants
Why we need this PR:
Prow branchprotector does not support
required_signatures. This script uses the GitHub API to enable (or disable) the signed commits requirement on protected branches.Changes made:
scripts/require-signed-commits.sh— enables/disablesrequired_signaturesper branch viagh apimain|release-.+)--dry-run,--disable --confirm,--org,--repo(repeatable),--branch(repeatable)ghCLI (authenticated with admin scope)[a-zA-Z0-9._-]+to prevent injectionWhich issue(s) this PR fixes:
Test plan:
Summary by CodeRabbit