Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add integration test for assuming an API Key Role using a Buildkite OIDC token #5416

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yob
Copy link

@yob yob commented Jan 30, 2025

Until recently, Buildkite OIDC tokens did not contain a jti claim. At some point in early 2024 it was possible to assume an API Key Role using Buildkite OIDC tokens, but when testing in January 2025 we found the assume role request was failing with an error:

Missing/invalid jti

Buildkite has addressed that by adding a jti claim to tokens - it's a good claim to include. However, to reduce the risk of regressions in the future, this PR proposes adding an integration test with a Buildkite-shaped OIDC token.

The trait added to the OIDC::Provider factory is based on a real token that I generated then anonymized. I only test the happy path with this token - there's a buncha existing tests for various unhappy paths (expired token, etc) using the Github Actions shaped OIDC token and there's little value in replicating them.

Most of the added test is copy-pasted from the happy-path Github Actions test further up the file.

Fixes #5412

Copy link
Author

@yob yob left a comment

Choose a reason for hiding this comment

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

This is my first contribution to rubygems.org, so I'm very happy to be guided on style and approach ❤️

test/integration/api/v1/oidc/api_key_roles_test.rb Outdated Show resolved Hide resolved
runner_environment
]
}
end
Copy link
Author

Choose a reason for hiding this comment

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

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.26%. Comparing base (55c07ac) to head (7e2c2ef).
Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5416      +/-   ##
==========================================
- Coverage   97.06%   94.26%   -2.80%     
==========================================
  Files         451      451              
  Lines        9391     9450      +59     
==========================================
- Hits         9115     8908     -207     
- Misses        276      542     +266     

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

@yob yob force-pushed the test-api-key-roles-with-buildkite branch from 262e057 to 0098d67 Compare February 2, 2025 22:49
…IDC token

Until recently, Buildkite OIDC tokens did not contain a `jti` claim. At
some point in early 2024 it was possible to assume an API Key Role using
Buildkite OIDC tokens, but when testing in January 2025 we found the
assume role request was failing with an error:

> Missing/invalid jti

Buildkite has addressed that by adding a `jti` claim to tokens - it's a
good claim to include. However, to reduce the risk of regressions in the
future, this proposes adding an integration test with a Buildkite-shaped
OIDC token.

The trait added to the OIDC::Provider factory is based on a real token
that I generated then anonymized. I only test the happy path with this
token - there's a buncha existing tests for various unhappy paths
(expired token, etc) using the Github Actions shaped OIDC token and
there's little value in replicating them.

Most of the added test is copy-pasted from the happy-path Github Actions
test further up the file.

Fixes rubygems#5412
@yob yob force-pushed the test-api-key-roles-with-buildkite branch from 0098d67 to 7e2c2ef Compare February 2, 2025 23:03
@yob
Copy link
Author

yob commented Feb 2, 2025

@simi I think this is ready for another look. I've tidied the test setup, and there's a single test failure in CI but it seems unrelated

@simi simi requested a review from segiddins February 4, 2025 10:51
@simi
Copy link
Member

simi commented Feb 4, 2025

@simi I think this is ready for another look. I've tidied the test setup, and there's a single test failure in CI but it seems unrelated

Thanks for the info, looks good to me. 💪 codecov/project CI fail is unrelated. Let's leave a time for Sam to review also before merge.

@yob
Copy link
Author

yob commented Feb 4, 2025

Sounds good.

Since opening this PR, we've also published a Buildkite plugin for pushing to rubygems.org using OIDC API Key Roles, and started using it for one of our own open source gems. Hopefully that dogfooding will also help us build familiarity with the feature and notice if any issues arise (from either end) ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Example Buildkite OIDC token for regression tests
2 participants