Skip to content

Add CLI identity-token acquisition for keyless skill push - #6390

Merged
samuv merged 1 commit into
mainfrom
skills-keyless/03-cli-token-acquisition
Aug 21, 2026
Merged

samuv merged 1 commit into
mainfrom
skills-keyless/03-cli-token-acquisition

Conversation

@samuv

@samuv samuv commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

Stacked on #6385. #6307 (keyless push signing) makes keyless the default
signing path, not something to opt into per push — but until now nothing
gave thv skill push a way to actually obtain an identity token, so
PushOptions.IdentityToken (added in #6385) had no way to become non-empty.

  • Add --identity-token to thv skill push: accepts a raw OIDC token or a
    path to a file containing one (cosign parity with --key).
  • New pkg/skills/identitytoken package implements the acquisition ladder
    when the flag is absent and neither --key nor --no-sign was given:
    1. A GitHub Actions ambient OIDC token (ACTIONS_ID_TOKEN_REQUEST_URL /
      _TOKEN, scoped to the sigstore audience) — the CI case.
    2. Otherwise, only on an interactive terminal: a y/N prompt, then a
      browser sign-in against the public-good Sigstore OAuth instance
      (oauth2.sigstore.dev, via sigstore/sigstore/pkg/oauthflow — already
      a direct dependency).
    3. If neither yields a token: an actionable failure naming all three
      signing choices. Never silently unsigned.
  • An explicit --identity-token is always resolved and forwarded, even
    alongside --key — the conflict is the server's to reject
    (skillsvc.validateSigningInputs, from Plumb an identity token through skill push for keyless signing #6385), never something the
    client silently arbitrates.
  • All of this runs client-side before the push HTTP request, so a failed
    acquisition never leaves an unsigned artifact published.
  • docs/arch/12-skills-system.md: documents the three signing choices, the
    CLI-acquires/server-signs split, and the ladder, in the same PR that ships
    the UX.

Consequence worth flagging explicitly: after this merges, a bare
thv skill push with no flags at all stops being a hard 400 and starts
signing keylessly — in CI from the ambient token, or after a browser prompt
locally. That's the intended fix for the CI breakage from #6139 (the
workflow file itself is repaired in the next PR in this stack).

Type of change

  • New feature

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)
  • Manual testing (describe below)

task test passes for pkg/skills/identitytoken (new, thorough table-driven
coverage: Resolve's file/literal/error disambiguation, Ambient's
env-var-absent/success/error-response cases via httptest, Acquire's full
precedence table including the regression case where --key +
--identity-token are both set, and Interactive's wiring via
oauthflow.StaticTokenGetter with a structurally-valid unverified JWT
fixture — no real OAuth exchange). One unrelated failure appeared in
pkg/transport/proxy/streamable from a stray leftover process holding a
hardcoded test port in this local environment (same pre-existing flake noted
on the earlier PRs in this stack, untouched package).

Manual: built binary, ran thv skill push <ref> with stdin from /dev/null
(no --key/--no-sign/--identity-token) — fails immediately with the
documented text, no HTTP request attempted (no thv serve was even running).
--identity-token /no/such/file fails naming both possibilities rather than
being forwarded as a literal token. Did not exercise the real interactive
browser flow or a live Fulcio/Rekor round trip — that's covered by the
staging E2E job in the next PR in this stack, not here.

Changes

File Change
pkg/skills/identitytoken/{resolve,ambient,interactive,acquire}.go New: token resolution, ambient CI fetch, interactive browser flow, ladder orchestration
cmd/thv/app/skill_push.go --identity-token flag, TTY-gated confirm callback, one Acquire call before Push
docs/arch/12-skills-system.md Document the three signing choices, precedence, and the ladder
docs/cli/thv_skill_push.md Regenerated via task docs

Does this introduce a user-facing change?

Yes — thv skill push gains --identity-token, and a push with none of
--key / --identity-token / --no-sign now attempts keyless signing
automatically (ambient CI token, or an interactive browser prompt) instead
of unconditionally failing with "signing key required". If no credential can
be acquired, it still fails, with an error naming all three options.

Special notes for reviewers

Base branch is skills-keyless/02-push-token-plumbing (#6385), not main.

Two things flagged during design review worth a second look:

  • The sigstore OAuth client ID used for the interactive flow is cosign's
    well-known public value, but it isn't a constant in any vendored library
    (only the issuer URL, https://oauth2.sigstore.dev/auth, is), so it could
    not be confirmed from source alone — worth one real interactive run before
    merge to be certain.
  • oauthflow.OIDConnect takes no context.Context and hardcodes
    context.Background() internally, so Interactive has no cancellation
    path to plumb. The redirect wait self-limits to 120s, but provider
    discovery, the code exchange, and the out-of-band stdin fallback are
    unbounded. Accepted deliberately (documented in interactive.go): this
    only runs after an explicit y/N on a TTY, so a human is present and Ctrl-C
    is the exit — wrapping it in a goroutine would leak one blocked on stdin
    or an open socket with no way to cancel it.

Out of scope, flagged for a separate issue: thv ai-plugin push still
declares no --key / --no-sign / --identity-token at all, even though
plugins.PushOptions is a type alias of skills.PushOptions and already
carries IdentityToken. That issue has not been filed yet.

@github-actions github-actions Bot added the size/L Large PR: 600-999 lines changed label Aug 19, 2026
@codecov

codecov Bot commented Aug 19, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.32394% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.77%. Comparing base (d278b33) to head (381dd18).

Files with missing lines Patch % Lines
pkg/skills/identitytoken/acquire.go 81.25% 3 Missing ⚠️
pkg/skills/identitytoken/ambient.go 90.00% 3 Missing ⚠️
pkg/skills/identitytoken/resolve.go 85.71% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6390      +/-   ##
==========================================
+ Coverage   77.75%   77.77%   +0.02%     
==========================================
  Files         750      754       +4     
  Lines       72601    72672      +71     
==========================================
+ Hits        56448    56519      +71     
  Misses      16148    16148              
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JAORMX
JAORMX previously approved these changes Aug 19, 2026
@samuv samuv self-assigned this Aug 19, 2026
Base automatically changed from skills-keyless/02-push-token-plumbing to main August 20, 2026 13:36
Keyless push signing (#6307) is meant to be the default, not a flag
to opt into: a bare `thv skill push` should sign automatically in CI
and after a browser sign-in locally, never silently unsigned.

Add --identity-token (raw token or a file path, cosign parity) and a
pkg/skills/identitytoken acquisition ladder: an explicit flag value,
then a GitHub Actions ambient OIDC token, then a TTY-gated interactive
browser sign-in against the public-good Sigstore OAuth instance.
Exhausting the ladder fails with an actionable, unsigned-safe error
before any push request is made.

Signed-off-by: Samuele Verzi <samu@stacklok.com>
@samuv
samuv force-pushed the skills-keyless/03-cli-token-acquisition branch from 83b471b to 381dd18 Compare August 20, 2026 13:36
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Aug 20, 2026
@samuv
samuv merged commit 1f54577 into main Aug 21, 2026
119 of 120 checks passed
@samuv
samuv deleted the skills-keyless/03-cli-token-acquisition branch August 21, 2026 08:29
@github-actions github-actions Bot mentioned this pull request Aug 26, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large PR: 600-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants