Skip to content

allowlist: add carabiner-dev install/{ampel,bnd} transitive deps#831

Merged
ppkarwasz merged 1 commit into
apache:mainfrom
potiuk:allowlist-add-carabiner-install-transitive
May 18, 2026
Merged

allowlist: add carabiner-dev install/{ampel,bnd} transitive deps#831
ppkarwasz merged 1 commit into
apache:mainfrom
potiuk:allowlist-add-carabiner-install-transitive

Conversation

@potiuk
Copy link
Copy Markdown
Member

@potiuk potiuk commented May 11, 2026

Summary

  • Add carabiner-dev/actions/install/ampel@v1.1.7 and install/bnd@v1.1.7 to the allowlist. Both are transitive deps that carabiner-dev/actions/ampel/verify@v1.2.0 calls.
  • Fixes the recurring check-for-transitive-failures failures (e.g. run 25643163039): GitHub Actions rejects the transitive install/* refs because they aren't on the org allowlist.

Test plan

  • After merge, update_actions.yml regenerates approved_patterns.yml + the composite action.yml from actions.yml.
  • ASF Infra's allowlist sync picks up the new SHAs; the next hourly check-for-transitive-failures run passes.

carabiner-dev/actions/ampel/verify@v1.2.0 (already approved) calls
carabiner-dev/actions/install/ampel and install/bnd at v1.1.7 SHA
2a11d59a135c5e291f305f249a92ad7903e3ee0f. GitHub Actions validates
every transitive `uses:` against the org allowlist, so these two
need to be approved too — without them, check-for-transitive-failures
fails every hour with "action ... is not allowed".

Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@potiuk potiuk requested a review from dfoulks1 as a code owner May 11, 2026 01:17
@potiuk
Copy link
Copy Markdown
Member Author

potiuk commented May 14, 2026

Needs approve :)

@potiuk potiuk requested review from dave2wave, ppkarwasz and raboof May 17, 2026 13:17
@ppkarwasz
Copy link
Copy Markdown
Member

Do we need to pin those dependencies by SHA1?

Until https://github.com/orgs/community/discussions/26245#discussioncomment-15601440 is solved, sibling actions will need to either be pinned by SHA1 and refer to the previous version or be pinned by tag.

We could ask @puerco to:

  • Enable immutable releases, so his version tags can not be moved.
  • Pin sibling actions by tag, instead of SHA1.
  • Add a verification step in our script to accept both pinning by SHA1 or pinning by version tag, if the version tag is immutable.

@potiuk
Copy link
Copy Markdown
Member Author

potiuk commented May 17, 2026

Do we need to pin those dependencies by SHA1?

Until https://github.com/orgs/community/discussions/26245#discussioncomment-15601440 is solved, sibling actions will need to either be pinned by SHA1 and refer to the previous version or be pinned by tag.

We could ask @puerco to:

  • Enable immutable releases, so his version tags can not be moved.
  • Pin sibling actions by tag, instead of SHA1.
  • Add a verification step in our script to accept both pinning by SHA1 or pinning by version tag, if the version tag is immutable.

That all seems much more complex than just approving those SHAs now - why would that be better?

@ppkarwasz
Copy link
Copy Markdown
Member

With the current setup we always need to trust two SHA1 at each given moment:

  • The one of the current version (v1.2.0),
  • The one of the previous version (v1.1.7) for the level one nested calls,
  • Possibly the version before that, if another level of nesting appears.

This is a lot of patterns and we already use up around one third (332) of the 1000 pattern limit imposed by GitHub. It would be better to have a single pattern for a single version of the actions (e.g. carabiner-dev/actions/*@v1.2.0).

If a tag is immutable, it is as good as a SHA1, but sibling actions can reference each other.

@potiuk
Copy link
Copy Markdown
Member Author

potiuk commented May 18, 2026

Thanks @ppkarwasz — the diagnosis is right and the proposals have merit, but I think they belong in a separate piece of work rather than gating this PR. Splitting the response in two:

On the immediate fix (this PR):
Right now check-for-transitive-failures is failing hourly on the missing transitive SHAs. The 2 new entries here unblock that. Even if we agree to move toward tag-based pinning, the migration path requires upstream cooperation (puerco enabling immutable releases, switching sibling refs from SHA to tag) and a verifier change on our side — none of which lands today. Rolling this back leaves the hourly job red while we negotiate that.

The pattern-count pressure also isn't acute: we're at 332/1000 with the soft-fail at 800 (README), so we have years of headroom at current growth.

On the broader proposal:
I think (1) and (2) are cheap to ask of puerco and worth pursuing. (3) is the one with real trade-offs:

  • "Immutable" is a per-release repo setting that can be reversed on future releases. Our verifier would need to re-check the immutability flag at audit time, or we accept silent erosion of the guarantee between releases.
  • It's a policy departure from the README's "Always pin to exact commit SHAs" line. The current @* wildcards are version-side only and reserved for low-risk allowlisted-by-name actions — accepting tag-pinning for arbitrary actions is a different primitive.
  • Your carabiner-dev/actions/*@v1.2.0 pattern assumes GitHub allowlist syntax supports path-segment wildcards. The existing wildcards are all version-side; I'd want to verify path wildcards actually expand before pricing the saving.

I'll open a tracking issue to capture this so we can design it properly (immutability check semantics, path-wildcard verification, README/policy update) without holding up the immediate fix. Sound reasonable?

@potiuk
Copy link
Copy Markdown
Member Author

potiuk commented May 18, 2026

Issues opened - including one on carabiner

Copy link
Copy Markdown
Member

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ppkarwasz ppkarwasz merged commit ba7238d into apache:main May 18, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants