Skip to content

fix(cli): prefer user/api audience for mixed-aud JWTs#2828

Open
SergioChan wants to merge 2 commits intochainloop-dev:mainfrom
SergioChan:fix-2812-token-audience-priority
Open

fix(cli): prefer user/api audience for mixed-aud JWTs#2828
SergioChan wants to merge 2 commits intochainloop-dev:mainfrom
SergioChan:fix-2812-token-audience-priority

Conversation

@SergioChan
Copy link
Copy Markdown

Summary

  • handle JWT aud claims as an ordered set instead of trusting the first item in an array
  • prioritize api-token-auth.chainloop and user-auth.chainloop ahead of generic chainloop when multiple audiences are present
  • add a regression test for mixed-audience user tokens (["chainloop", "user-auth.chainloop"]) so user identity is selected instead of issuer fallback

Testing

  • go test ./app/cli/internal/token

Related

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 2 files

@SergioChan SergioChan force-pushed the fix-2812-token-audience-priority branch from b3a0ca9 to 5885882 Compare March 9, 2026 20:26
@SergioChan
Copy link
Copy Markdown
Author

Thanks for the checks — I force-pushed an amended commit with DCO sign-off so the DCO status should now pass.

No functional code changes were introduced in this push.

@migmartri
Copy link
Copy Markdown
Member

Thanks for the contribution

Could you elaborate on what was the root cause of the problem?

Thanks

@SergioChan
Copy link
Copy Markdown
Author

Thanks for the detailed review feedback 鈥?I鈥檓 working through the requested updates and will post a focused patch (or direct answers) shortly.

1 similar comment
@SergioChan
Copy link
Copy Markdown
Author

Thanks for the detailed review feedback 鈥?I鈥檓 working through the requested updates and will post a focused patch (or direct answers) shortly.

@SergioChan
Copy link
Copy Markdown
Author

Thanks for asking — root cause was in the CLI audience selection logic, not tmux itself.

Before this patch, we effectively trusted the first value when aud was an array. For affected user tokens the audience list can be mixed, e.g. ["chainloop", "user-auth.chainloop"], where the generic chainloop value may appear before the specific auth audience.

That caused the CLI to classify the token via the generic audience path and then fall back to issuer resolution (cp.chainloop), which is where the issuer not found: cp.chainloop error came from.

The fix changes audience handling to treat aud as a set and apply explicit priority (api-token-auth.chainloop / user-auth.chainloop before generic chainloop) so mixed-audience user tokens are classified correctly.

@migmartri
Copy link
Copy Markdown
Member

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@Piskoo
Copy link
Copy Markdown
Collaborator

Piskoo commented Mar 18, 2026

The issue mentions a possible race condition between tmux sessions affecting the local config. Does this fix address the root cause? Consider adding a negative case for a multi-audience token where none match the known audiences (["unknown1", "unknown2"] → returns nil).

@SergioChan
Copy link
Copy Markdown
Author

Good callout — thank you.

Short answer: this PR addresses the audience-selection bug in mixed-aud tokens (which surfaced as the issuer not found: cp.chainloop behavior), not tmux session coordination itself.

I’ll add the negative multi-audience case you suggested ([unknown1, unknown2] -> nil) and push an update.

@migmartri migmartri requested a review from Piskoo March 23, 2026 11:31
SergioChan and others added 2 commits March 23, 2026 16:09
Signed-off-by: sergiochan <yuheng9211@qq.com>
@SergioChan SergioChan force-pushed the fix-2812-token-audience-priority branch from 5885882 to 2eef292 Compare March 23, 2026 23:11
@SergioChan
Copy link
Copy Markdown
Author

Thanks for the suggestion — I’ve pushed an update that adds the requested negative multi-audience case:

  • Added test case: aud = ["unknown1", "unknown2"] now asserts Parse(...) returns nil
  • This complements the existing mixed-audience regression case and verifies unknown audience arrays do not get misclassified

Validation run:

  • go test ./app/cli/internal/token

@migmartri
Copy link
Copy Markdown
Member

Thanks for the contribution. But I'm still struggling to identify the root cause, because today it is not possible to have two audiences in the checkloop token. So the problem that we are solving here is non-existent.

@migmartri
Copy link
Copy Markdown
Member

Some findings after investigating the root cause of #2812:

The user token audience (user-auth.chainloop) and API token audience (api-token-auth.chainloop) have never changed in the codebase. The control plane JWT builder (app/controlplane/pkg/jwt/user/user.go) always emits exactly one audience per token (jwt.ClaimStrings{aud}), so a token with aud: ["chainloop", "user-auth.chainloop"] cannot be produced by any version of the control plane.

The likely root cause of #2812 is a stale token + newer control plane scenario:

  1. User has a cached JWT from an old control plane deployment (different HMAC secret)
  2. Current control plane middleware fails HMAC validation for all standard providers (RobotAccount, APIToken, UserToken)
  3. Falls through to the federated auth provider as a last resort
  4. Federated provider sees iss: cp.chainloop and returns issuer not found: cp.chainloop

The fix in this PR (smarter audience selection via chooseAudience()) is a reasonable defensive improvement for hypothetical mixed-audience tokens, but likely doesn't address the actual issue reported in #2812, which is a server-side auth middleware fallthrough triggered by an invalid/stale token. The real fix for the reporter would be chainloop auth login.

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.

ERR failed to load issuer: authorization error: issuer not found: cp.chainloop

3 participants