WIP: Add Buildkite as a trusted publisher #5437

yob · 2025-02-07T02:23:41Z

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?

segiddins

@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

segiddins · 2025-02-10T16:47:51Z

db/migrate/20250204190405_create_oidc_trusted_publisher_buildkite.rb

+
+  def change
+    create_table :oidc_trusted_publisher_buildkites do |t|
+      t.string :organization_slug, null: false


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?

Pipeline slug is not very immutable. Similar to a github repository in an account, they can be deleted, renamed and re-use slugs

Organization slug is unique for as long as the account exists, including if the customer cancels their account. However, if they opt into deleting their account then the slug does become available for others

in that case, can we do what we do for GitHub and also store an organization_id that gets fetched on trusted publisher creation to prevent cross-organization name takeovers?

Unfortunately we don't have a public API that would make this as easy as it is for the GitHub provider.

We do have a unique and immutable organization_id for each organization and it's available as a claim in our OIDC tokens. The value is a UUID. It's less readable to the user, but I can add it as a requirement here to avoid the risk of slug reuse attacks.

If we do that to prevent cross-organization risk, are you comfortable leaving pipeline_slug as a readable scope within an organization context?

I think that's the right tradeoff on both counts, but also curious what @woodruffw & @sethmlarson as my collaborators on the trusted publishing guides from the openssf

segiddins · 2025-02-10T16:49:50Z

app/views/oidc/rubygem_trusted_publishers/new_view.rb

@@ -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}"


segiddins · 2025-02-10T16:50:45Z

app/views/oidc/rubygem_trusted_publishers/index_view.rb

@@ -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


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

I'll have to brush up on how stimulus works, but I'll give it a go!

segiddins · 2025-02-10T16:52:16Z

app/models/oidc/trusted_publisher/buildkite.rb

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


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

segiddins · 2025-02-10T16:54:09Z

app/models/oidc/trusted_publisher/buildkite.rb

+  end
+
+  def to_access_policy(jwt)
+    # TODO what to do with jwt here?


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)

segiddins · 2025-02-10T16:55:24Z

app/models/oidc/trusted_publisher/buildkite.rb

+    )
+  end
+
+  #class SigstorePolicy


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

I'll start by adding one and asserting that the push is rejected, just to keep this PR a manageable size. I'm keen to come back and fill in that functionality though, we have a few gems to get green on https://segiddins.github.io/are-we-attested-yet/ ;)

The form and POSTing to create works, but the resulting API Key Role has a condition that expects a principal of "https://token.actions.githubusercontent.com"

When a gem has a source doe URI for GitHub, `add_default_params` defaults the form to common settings for GitHub OIDC tokens. That's a reasonable thing to do. However, that includes setting the principal to a Github Actions shaped value. That value is rendered on the form in hidden elements so the user doesn't have a chance to edit it. After saving the created role has a provider of Buildkite with an expected principal for GitHub Actions. Its safe to remove the statement.principal assignment completely. It's not required - when the form is submitted the OIDC::ApiKeyRole#set_statement_principals callback will set the correct principal for both GitHub Actions *and* Buildkite.

codecov · 2025-02-17T12:31:20Z

Codecov Report

Attention: Patch coverage is 42.52874% with 50 lines in your changes missing coverage. Please review.

Project coverage is 93.78%. Comparing base (968beaf) to head (fb8d33b).

Files with missing lines	Patch %	Lines
...oidc/trusted_publisher/buildkite/form_component.rb	0.00%	18 Missing ⚠️
app/models/oidc/trusted_publisher/buildkite.rb	57.89%	16 Missing ⚠️
...idc/trusted_publisher/buildkite/table_component.rb	0.00%	11 Missing ⚠️
...lers/oidc/rubygem_trusted_publishers_controller.rb	60.00%	2 Missing ⚠️
.../views/oidc/rubygem_trusted_publishers/new_view.rb	80.00%	2 Missing ⚠️
app/models/oidc/provider.rb	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5437      +/-   ##
==========================================
- Coverage   97.06%   93.78%   -3.28%     
==========================================
  Files         451      454       +3     
  Lines        9392     9531     +139     
==========================================
- Hits         9116     8939     -177     
- Misses        276      592     +316

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

yob mentioned this pull request Feb 7, 2025

Add support for trusted publishers buildkite-plugins/rubygems-oidc-buildkite-plugin#12

Open

segiddins reviewed Feb 10, 2025

View reviewed changes

yob added 3 commits February 17, 2025 23:18

Add a failing system test for creating an API Key Role for Buildkite

ab4aac5

The form and POSTing to create works, but the resulting API Key Role has a condition that expects a principal of "https://token.actions.githubusercontent.com"

WIP: hacking and slashing my way to a Buildkite Trusted Publisher

fb8d33b

yob force-pushed the buildkite-trusted-publisher branch from 50c8f01 to fb8d33b Compare February 17, 2025 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add Buildkite as a trusted publisher #5437

WIP: Add Buildkite as a trusted publisher #5437

yob commented Feb 7, 2025

segiddins left a comment

segiddins Feb 10, 2025

yob Feb 17, 2025

segiddins Feb 17, 2025

yob Feb 19, 2025

segiddins Feb 19, 2025

segiddins Feb 10, 2025

segiddins Feb 10, 2025

yob Feb 17, 2025

segiddins Feb 10, 2025

segiddins Feb 10, 2025

segiddins Feb 10, 2025

yob Feb 17, 2025

codecov bot commented Feb 17, 2025

WIP: Add Buildkite as a trusted publisher #5437

Are you sure you want to change the base?

WIP: Add Buildkite as a trusted publisher #5437

Conversation

yob commented Feb 7, 2025

segiddins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 17, 2025

Codecov Report