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

WIP: Add Buildkite as a trusted publisher #5437

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yob
Copy link
Contributor

@yob yob commented Feb 7, 2025

I'm still learning my way around this codebase, so my approach for exploring trusted publishers and adding Buildkite has been to hack-and-slash my way through to a point where in my local environment I can exchange a Buildkite OIDC token for a trusted publisher API token via /api/v1/oidc/trusted_publisher/exchange_token 💪

Now that I'm here, I thought I'd come up for air and check in with y'all before I start to polish it up. Here's a few thoughts and questions I have:

  • this is built on top of Add integration test for assuming an API Key Role using a Buildkite OIDC token #5416
  • Now that I've done this, I realise why the rubygems team and docs are promoting Trusted Publishers over OIDC API Key Roles. Trusted Publishers is just a better system, particularly removing the need for the key role ID. I'd still advocate for fixing the issue in New system test creating an API Key Role for Buildkite #5434 though - While OIDC API Key Roles are a supported approach, it'd be nice for them to work with multiple providers
  • I assume it'd be OK to ship Buildkite Trusted Publisher support without Sigstore and attestation support, and that we could follow up with that in a later PR?
  • Thanks to the polymorphic association (and the different shape of the OIDC tokens), the forms for creating a new trusted publisher for GitHub Actions and Buildkite are quite different. The current "new trusted publisher" form has a provider select with a single item, but unless we want to do javascript things it might be easier to ask the user to pick the provider prior to loading the new form. What do you think?

yob added 2 commits February 3, 2025 10:03
…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
Copy link
Member

@segiddins segiddins left a comment

Choose a reason for hiding this comment

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

@yob you're a hero for getting this started!

This is generally what I'd expect.

I assume it'd be OK to ship Buildkite Trusted Publisher support without Sigstore and attestation support, and that we could follow up with that in a later PR?
Yes, that is OK, so long as it fails with a 400 with a clear message (and that code path is tested)
Thanks to the polymorphic association (and the different shape of the OIDC tokens), the forms for creating a new trusted publisher for GitHub Actions and Buildkite are quite different. The current "new trusted publisher" form has a provider select with a single item, but unless we want to do javascript things it might be easier to ask the user to pick the provider prior to loading the new form. What do you think?

If it's not too hard with a stimulus controller, I imagined using the selector to change -- it could even re-navigate the page, setting a query param. That way, it remains possible to link directly to a "create a trusted publisher" form


def change
create_table :oidc_trusted_publisher_buildkites do |t|
t.string :organization_slug, null: false
Copy link
Member

Choose a reason for hiding this comment

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

are these two slugs both immutable, or is it possible for someone to take over an organization name that was previously owned by someone else?

@@ -14,22 +14,31 @@ def view_template
title_content

div(class: "t-body") do
p do
"New Trusted Publisher: #{rubygem_trusted_publisher.trusted_publisher.class.publisher_name}"
Copy link
Member

Choose a reason for hiding this comment

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

i18n ?

@@ -18,7 +18,8 @@ def view_template
end

p do
button_to t(".create"), new_rubygem_trusted_publisher_path(rubygem.slug), class: "form__submit", method: :get
button_to t(".create") + " Buildkite", new_rubygem_trusted_publisher_path(rubygem.slug), params: {trusted_publisher: "buildkite"}, class: "form__submit", method: :get
Copy link
Member

Choose a reason for hiding this comment

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

instead of multiple buttons here, what if the trusted publisher new view reloaded the form when the selector for trusted publisher type was changed?


def to_access_policy(jwt)
# TODO what to do with jwt here?
# TODO should we be checking the audience claim?
Copy link
Member

Choose a reason for hiding this comment

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

yes, audience claim should be checked, same as is done for the GHA publisher

end

def to_access_policy(jwt)
# TODO what to do with jwt here?
Copy link
Member

Choose a reason for hiding this comment

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

I think we're OK to ignore it, since there are no cross-claim consistency checks we need to enforce, unlike with GitHub (where we make sure the workflow source matches up with the commit being built)

)
end

#class SigstorePolicy
Copy link
Member

Choose a reason for hiding this comment

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

can we add a push test that uses a build kite trusted publisher and includes an attestation?

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.

2 participants