Skip to content

Audience fixes#443

Merged
di merged 9 commits intodi:mainfrom
jku:audience-fixes
Feb 3, 2026
Merged

Audience fixes#443
di merged 9 commits intodi:mainfrom
jku:audience-fixes

Conversation

@jku
Copy link
Contributor

@jku jku commented Feb 1, 2026

This is all the improvements around audience that made sense to me: I can also try to split these up if there's disagreement.
Fixes #441

  • Add internal _request() helper to do what @di suggested in the bug: build the URL manually to avoid the query parameter mangling that currently happens to GitHub token request URL
  • Start doing light validation on the token: We can check that the audience is what caller wants. It's a little annoying since it means running json and base64 but failing here seems preferable to giving out incorrect tokens
  • tests: Start running a test using actual Github token (the unit tests are all mocked). This does not run on PRs from forks
  • tests: fix tests that broke because of token validation
  • tests: fix tests that broke because we now manually build URL
  • tests: Add several tests for token validation and audience validation. This requires adding an actual token as a test asset

jku added 5 commits February 1, 2026 10:34
* unit tests do not use GitHub OIDC
* Also enable workflow dispatch for easier workflow testing

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Unit tests are all mocked: makes sense to do an actual test as well.

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Mainly make sure the audience is the requested one.

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Add a _request() helper method.

The helper ensures query params are added to the url correctly,
avoiding the urllib3 pitfall where

  urllib3.request("GET", "http://localhost:8000/?arg2=val2", fields={"arg": "val"}`
  # server sees "GET /?arg2=val2?arg=val"

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* Add tests to check audience
* Also fix the existing tests so the new url building
  does not break them
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Copy link
Owner

@di di left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Just one nit

Copy link
Owner

@di di left a comment

Choose a reason for hiding this comment

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

forgot to add the comment

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku jku requested a review from di February 3, 2026 07:53
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@woodruffw
Copy link
Collaborator

/gcbrun

@woodruffw
Copy link
Collaborator

Thanks @jku!

Copy link
Owner

@di di left a comment

Choose a reason for hiding this comment

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

Thanks so much @jku!

@di
Copy link
Owner

di commented Feb 3, 2026

/gcbrun

@di di merged commit 90fa386 into di:main Feb 3, 2026
16 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.

sigstore-python fails to sign on GitHub Actions with id 1.6.0

3 participants