Repository navigation
Conversation
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| curl --fail-with-body --silent --show-error \ | ||
| -H "DD-API-KEY: $DATADOG_API_KEY" -H "Content-Type: application/json" \ | ||
| --data-binary "@$RUNNER_TEMP/release-failure-event.json" \ | ||
| https://api.us3.datadoghq.com/api/v1/events || true |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): Neither Datadog curl call sets --connect-timeout or --max-time, and notify-release-failure has no timeout-minutes. A stalled Datadog endpoint could hold the failure job open until the 6-hour default. Add something like --connect-timeout 10 --max-time 30 (or a small job timeout), since || true already keeps the step best-effort.
| if name in PRIVILEGED_JOBS or oidc or "secrets." in json.dumps(job): | ||
| errors.extend(check_steps(job.get("steps", []), name)) | ||
| errors.extend(check_local_bootstrap(job.get("steps", []), name)) | ||
| return errors |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): The policy only looks at job["steps"]. A credential-bearing job that calls a reusable workflow (job-level uses:), or that sets container:/services: images, passes the check without review. The docs say unknown container actions "fail closed", but that only holds for step-level docker:// references. Consider rejecting job-level uses/container/services (or requiring a SHA/digest pin for them) for jobs in scope.
Superseded — see the current review report for commit
|
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
| oidc = permissions == "write-all" or ( | ||
| isinstance(permissions, dict) and permissions.get("id-token") == "write") | ||
| inherited_secret = "secrets." in json.dumps(workflow.get("env", {})) | ||
| if name in PRIVILEGED_JOBS or oidc or job.get("secrets") or inherited_secret or "secrets." in json.dumps(job): |
There was a problem hiding this comment.
🟡 Suggestion: Dynamic discovery only matches the substring secrets.. A new job that references a secret through index syntax (${{ secrets['TOKEN'] }}) or through toJSON(secrets) holds credentials but isn't detected, so its actions skip the allowlist check. Consider matching on \bsecrets\b (e.g. re.search(r"\bsecrets\b", json.dumps(job))) and adding a negative test. (confidence: medium)
Superseded — see the current review report for commit
|
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
| errors.append(f"{location}: checkout path must be a string") | ||
| ready = False | ||
| continue | ||
| if os.path.normpath(path) == "_workflows": |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): The overwrite guard only fires when normpath(path) == "_workflows". A later checkout into an ancestor directory is not caught. With no path, or with path: . or path: ${{ github.workspace }}, the checkout targets the workspace root, which is not a git repo, so actions/checkout clears it and fetches the new repo. If that repo contains _workflows/.github/actions/ca-trust-guard/action.yml, it replaces the verified local action while ready stays True. Consider treating any checkout whose normalized path is _workflows, an ancestor of it (., empty), or a ${{ }} expression as one that resets ready.
Superseded — see the current review report for commit
|
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
| rel = os.path.normpath(str(path or ".")) | ||
| if (rel in (".", "_workflows") or rel.startswith("_workflows/") | ||
| or rel.startswith("..") or os.path.isabs(rel) or "${{" in str(path)): |
There was a problem hiding this comment.
🟡 Suggestion (confidence: low): os.path.normpath uses POSIX rules here, but goreleaser-windows runs on windows-latest. On that runner a checkout path such as _workflows\.github, .\, or C:\... would still overwrite the verified tree, yet none of those values trip this guard. Consider normalizing with path.replace("\\", "/") before normpath, and also treating drive-letter paths as absolute.
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
| docker/setup-qemu-action | ||
| docker/setup-buildx-action | ||
| # Notification only; custody: DATADOG_API_KEY (Datadog org write key, non-signing). | ||
| # Re-pin review: diff the action's tag-to-SHA source; Renovate PRs are not auto-merged. |
There was a problem hiding this comment.
🟡 Suggestion: This push changed the docs to say re-pinning is manual and no Renovate tracking exists for workflow SHAs. That matches renovate.json, whose enabledManagers has no github-actions. This comment still says "Renovate PRs are not auto-merged", which suggests Renovate opens re-pin PRs. Reword it to match the docs, e.g. "re-pinning is manual (no Renovate tracking for workflow SHAs)".
Superseded — see the current review report for commit
|
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
gontzess
changed the title
|
|
||
| ### Pinned Release Action Custody | ||
|
|
||
| Review and organization Actions policy own action drift. Custody below describes |
There was a problem hiding this comment.
🟡 Suggestion (confidence: high): This push deleted scripts/check-release-actions.py, tools/privileged-actions.txt and scripts/test-release-actions.py, and dropped the test from make test-scripts. Repository CI no longer enforces the pins. The PR title ("Enforce pinned action allowlist") and body ("make action-policy drift fail repository CI", the evidence list, the negative controls) still describe that checker. Please update the title and description to match what this PR does now: one-time SHA pins plus documentation, with drift left to review and org Actions policy.
| | aws-actions/configure-aws-credentials | `61815dcd50bd041e203e49132bacad1fd04d2708` | goreleaser-binaries, goreleaser-windows, goreleaser-docker, publish-release-manifest, verify-release | Exchanges job OIDC for scoped AWS role credentials. | | ||
| | aws-actions/amazon-ecr-login | `03f1aad4c6c7ffd436567f42f9384779290529bd` | goreleaser-docker, verify-release | Job AWS credentials; registry authentication for publishing or pulling. | | ||
| | docker/setup-qemu-action | `c7c53464625b32c7a7e944ae62b3e17d2b600130` | goreleaser-docker | Digest-pinned binfmt setup in an OIDC-enabled job; no explicit secret input. | | ||
| | masci/datadog | `93a16378298f4e1980f13f6b412c0b8e66b96185` | notify-release-failure | `DATADOG_API_KEY` (Datadog org write key), only step `with: api-key`; non-signing, notification only. Best-effort (`continue-on-error: true`), five-minute job; event-only (added metric removed). Re-pin review: diff the action's tag-to-SHA source; re-pinning is manual (no Renovate tracking exists for workflow SHAs). | |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): "event-only (added metric removed)" describes an intermediate state of this PR. Neither main nor the merged workflow has a failure-count metric, so readers of the merged docs won't know what this refers to. Consider shortening it to "event-only".
Superseded — see the current review report for commit
|
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
General PR Review: Pin privileged release actions and document custodyBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe PR does three things. It swaps every remaining mutable The new commit only rewords the Datadog row of that table: it drops "(added metric removed)" and leaves "event-only". I scanned the full PR diff for security and correctness. Every Coverage gap: I checked that each pin is well-formed and placed where the table says. I did not independently confirm that each SHA matches the upstream tag in its trailing comment. Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. Resolved prior findings
Reviewed commit: |
1 participant
Why
Use immutable action revisions for release jobs and make credential custody explicit.
Scope and drift ownership
Review and organization Actions policy own action drift. This change adds no per-repository action lint. The final source diff is the workflow change plus a short custody table; the existing locked-tools integrity tests remain untouched.
What
job.workflow_sha.masci/datadog@93a16378298f4e1980f13f6b412c0b8e66b96185without a bump, with its exact event configuration and latest-attempt annotation plumbing.DATADOG_API_KEYstep-scoped throughwith: api-key, notification-stepcontinue-on-error: true, and the five-minute job timeout. Notification is event-only.Action custody
fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09GITHUB_TOKEN;persist-credentials: false.f28e40c7f34bde8b3046d885e986cb6290c5673b398d4b0eeef1380460a10c8013a76f728fb906ac253ddeeac23f2bdad1646faac5c8c2832e800071APPLE_SIGNING_KEY_P12andAPPLE_SIGNING_KEY_P12_PASSWORD.61815dcd50bd041e203e49132bacad1fd04d270803f1aad4c6c7ffd436567f42f9384779290529bdc7c53464625b32c7a7e944ae62b3e17d2b60013093a16378298f4e1980f13f6b412c0b8e66b96185DATADOG_API_KEY(Datadog org write key), only stepwith: api-key; non-signing, notification only. Best-effort (continue-on-error: true), five-minute job; event-only. Re-pin review: diff the action's tag-to-SHA source; re-pinning is manual (no Renovate tracking exists for workflow SHAs).Custody describes explicit inputs, not the absence of ambient job permissions. Re-pinning is manual: reviewers diff the action's tag-to-SHA source; no Renovate tracking exists for workflow SHAs.
Evidence
make test-scripts: PASS without the removed action-checking files or target hook (native PowerShell checks unavailable/skipped).GOTOOLCHAIN=local go test ./...andGOTOOLCHAIN=local go vet ./...: PASS.make workflow-validateandgit diff --check: PASS.release.yamlandscripts/test-tool-integrity.pybyte-for-byte unchanged from the preceding revision; all pins and notification controls are preserved.Rollout limits
Draft only. These local checks do not establish deployed organization Actions policy or live Datadog delivery. Review the immutable action revisions and evaluate live release/delivery evidence separately before rollout. No merge, deployment, secret creation or rotation is performed by this PR.