Allow using OIDC to fetch api tokens #3716

segiddins · 2023-04-16T05:20:09Z

codecov · 2023-04-16T19:29:33Z

Codecov Report

Merging #3716 (e005ff2) into master (7e19c19) will decrease coverage by 0.01%.
The diff coverage is 98.97%.

@@            Coverage Diff             @@
##           master    #3716      +/-   ##
==========================================
- Coverage   98.92%   98.92%   -0.01%     
==========================================
  Files         229      260      +31     
  Lines        5570     6026     +456     
==========================================
+ Hits         5510     5961     +451     
- Misses         60       65       +5

Files Changed	Coverage Δ
...nts/avo/fields/json_viewer_field/edit_component.rb	`0.00% <0.00%> (ø)`
app/controllers/concerns/avo_auditable.rb	`89.47% <ø> (-0.53%)`	⬇️
lib/types/array_of.rb	`90.00% <90.00%> (ø)`
app/models/oidc/api_key_role.rb	`96.96% <96.96%> (ø)`
...ontrollers/api/v1/oidc/api_key_roles_controller.rb	`97.22% <97.22%> (ø)`
app/avo/actions/base_action.rb	`96.55% <100.00%> (+0.06%)`	⬆️
app/avo/actions/refresh_oidc_provider.rb	`100.00% <100.00%> (ø)`
app/avo/fields/array_of_field.rb	`100.00% <100.00%> (ø)`
app/avo/fields/json_viewer_field.rb	`100.00% <100.00%> (ø)`
app/avo/fields/nested_field.rb	`100.00% <100.00%> (ø)`
... and 38 more

indirect

Are there docs somewhere for how to use this, either as a maintainer or as a developer publishing gems? I'm ok merging this without, but I do think we should have docs to point people at when we ask them to help us test it!

See presidentbeef/brakeman#1765

jenshenny

Looks good! Put down a few initial questions.

jenshenny · 2023-08-03T20:07:22Z

app/avo/resources/oidc_provider_resource.rb

+  field :issuer, as: :text, link_to_resource: true
+  field :configuration, as: :nested do
+    visible_on = %i[edit new]
+    OIDC::Provider::Configuration.then { (_1.required_attributes + _1.optional_attributes) - fields.map(&:id) }.each do |k|


The create provider page is a bit overwhelming. I can see that this form won't be used often, however could we separate the common required attributes from the optional ones?

As an aside, continuing from the point that this form won't be used often... I feel like the provider model feels very abstract/confusing and felt like hardcoding the provider information would have been fine since the majority would use GH actions and there might a few additional ones. However, the model is created already so transitioning to something more hardcoded might not be worth it.

nit: _1 is nifty, though I would prefer to have it be named so it's more readable.

@jenshenny given this is the admin interface, I'm not too concerned

Agreed it's not blocking. Just raising that this can be iterated on so other admins are able to create/modify providers more easily when it comes to that point.

The modification is done automatically via an action that fetches from the well-known URLs

jenshenny · 2023-08-04T05:19:57Z

app/models/oidc/id_token.rb

@@ -0,0 +1,38 @@
+class OIDC::IdToken < ApplicationRecord
+  belongs_to :api_key_role, class_name: "OIDC::ApiKeyRole", foreign_key: "oidc_api_key_role_id", inverse_of: :id_tokens
+  belongs_to :provider, class_name: "OIDC::Provider", foreign_key: "oidc_provider_id", inverse_of: :id_tokens


To my knowledge, if api_key_role belongs to a provider, wouldn't it be redundant (and error prone) to have an id token be associated with a provider as you can access the provider through api key role.

so right now it's there because of the JTI uniqueness validation, but I can work around that

app/controllers/api/v1/oidc/api_key_roles_controller.rb

jenshenny · 2023-08-04T05:50:49Z

app/controllers/api_keys_controller.rb

@@ -7,7 +7,7 @@ class ApiKeysController < ApplicationController

  def index
    @api_key  = session.delete(:api_key)
-    @api_keys = current_user.api_keys
+    @api_keys = current_user.api_keys.unexpired


Haven't tested all the way so that the OIDC flow returns a API key yet but I'm guessing that index table will contain OIDC generated API keys and "manually" created keys. To avoid confusion between the two (they work and are created differently from each other), I believe it'll be better to have a section with just "manually" created keys and another section for api keys generated for each api key role (with the expiry date)?

they work identically to each other, the only difference is how they are created

for now, im avoiding touching the HTML pages, wanting to wait until we're satisfied with the bones of this and ready to roll it out to GA to do that work

Ah I see that the key name is namespaced by the name of the API key role so a user can differentiate it by that. I would want to see these keys under a API key role section at some point where I can see all keys up to the last x days (user can only see keys that are active and can't see soft deleted keys right now, it'll be even better show the api key role to a specific gem push)

Curious if this is what you had in mind.

Eg.

#API Key roles ##<name> Scopes: Expiry: Gems: <list of keys>

honestly, I haven't thought about the HTML yet, my plan is to start on that once this ships

Gotcha, sounds great! I think this is necessary enough to be included in the first release and not for GA.

and here it is :)

app/controllers/passwords_controller.rb

jenshenny

Was able to test this out! Super cool to see this in action ✨ Thanks for putting in all this work

jenshenny · 2023-08-04T22:37:21Z

app/controllers/api_keys_controller.rb

@@ -7,7 +7,7 @@ class ApiKeysController < ApplicationController

  def index
    @api_key  = session.delete(:api_key)
-    @api_keys = current_user.api_keys
+    @api_keys = current_user.api_keys.unexpired


Ah I see that the key name is namespaced by the name of the API key role so a user can differentiate it by that. I would want to see these keys under a API key role section at some point where I can see all keys up to the last x days (user can only see keys that are active and can't see soft deleted keys right now, it'll be even better show the api key role to a specific gem push)

Curious if this is what you had in mind.

Eg.

#API Key roles ##<name> Scopes: Expiry: Gems: <list of keys>

Use a :through relationship instead

segiddins · 2023-07-07T18:15:03Z

test/policies/oidc/provider_policy_test.rb

@@ -0,0 +1,18 @@
+require "test_helper"
+
+class OIDC::ProviderPolicyTest < ActiveSupport::TestCase


fill out test methods

segiddins · 2023-07-07T18:15:08Z

test/policies/oidc/provider/configuration_policy_test.rb

@@ -0,0 +1,18 @@
+require "test_helper"
+
+class OIDC::Provider::ConfigurationPolicyTest < ActiveSupport::TestCase


fill out test methods

segiddins · 2023-07-07T18:15:13Z

test/policies/oidc/id_token_policy_test.rb

@@ -0,0 +1,18 @@
+require "test_helper"
+
+class OIDC::IdTokenPolicyTest < ActiveSupport::TestCase


fill out test methods

segiddins · 2023-07-07T18:15:17Z

test/policies/oidc/api_key_role_policy_test.rb

@@ -0,0 +1,18 @@
+require "test_helper"
+
+class OIDC::ApiKeyRolePolicyTest < ActiveSupport::TestCase


fill out test methods

segiddins · 2023-07-07T18:31:43Z

test/models/oidc/id_token_test.rb

@@ -0,0 +1,7 @@
+require "test_helper"
+
+class OIDC::IdTokenTest < ActiveSupport::TestCase


test the model

segiddins · 2023-08-04T21:57:12Z

app/models/oidc/id_token.rb

@@ -0,0 +1,38 @@
+class OIDC::IdToken < ApplicationRecord
+  belongs_to :api_key_role, class_name: "OIDC::ApiKeyRole", foreign_key: "oidc_api_key_role_id", inverse_of: :id_tokens
+  belongs_to :provider, class_name: "OIDC::Provider", foreign_key: "oidc_provider_id", inverse_of: :id_tokens


so right now it's there because of the JTI uniqueness validation, but I can work around that

app/controllers/api/v1/oidc/api_key_roles_controller.rb

segiddins · 2023-08-04T22:03:08Z

app/controllers/api_keys_controller.rb

@@ -7,7 +7,7 @@ class ApiKeysController < ApplicationController

  def index
    @api_key  = session.delete(:api_key)
-    @api_keys = current_user.api_keys
+    @api_keys = current_user.api_keys.unexpired


they work identically to each other, the only difference is how they are created

for now, im avoiding touching the HTML pages, wanting to wait until we're satisfied with the bones of this and ready to roll it out to GA to do that work

app/controllers/passwords_controller.rb

segiddins · 2023-08-04T22:52:29Z

app/controllers/api_keys_controller.rb

@@ -7,7 +7,7 @@ class ApiKeysController < ApplicationController

  def index
    @api_key  = session.delete(:api_key)
-    @api_keys = current_user.api_keys
+    @api_keys = current_user.api_keys.unexpired


honestly, I haven't thought about the HTML yet, my plan is to start on that once this ships

jenshenny

I have some comments regarding the UX of the basic views.

The PR is getting kind of large though. I would be 👍 in merging everything except 73bf7c9 and have another PR for the views. I would also be open to making the enhancements to the views too! Just let me know.

jenshenny · 2023-08-14T23:59:47Z

config/locales/en.yml

+        api_key_roles: OIDC API Key Roles
+      show:
+        api_key_role_name: "API Key Role %{name}"


Suggested change

api_key_roles: OIDC API Key Roles

show:

api_key_role_name: "API Key Role %{name}"

api_key_roles: OIDC API key roles

show:

api_key_role_name: "API key role %{name}"

Would users understand what an API key role actually is? It reveals too much of the implementation details. I would prefer provider or issuer even though it does conflict with some fields in the model.

Suggested change

api_key_roles: OIDC API Key Roles

show:

api_key_role_name: "API Key Role %{name}"

api_key_roles: API key provider (OIDC)

show:

api_key_role_name: "API key provider %{name}"

jenshenny · 2023-08-15T00:23:01Z

app/views/oidc/api_key_roles/index.html.erb

+            <code><%= api_key_role.token %></code>
+          </td>
+          <td class="owners__cell" data-title="Provider">
+            <%= link_to api_key_role.provider.issuer, api_key_role.provider.issuer %>


When this is setup on prod, could we name the issuer Github Actions? is provider.issuer the same as provider.configuration.issuer? If so, could we remove provider.issuer and add a name attribute and store it as Github Actions.

jenshenny · 2023-08-15T00:26:40Z

app/views/oidc/api_key_roles/index.html.erb

+      <% @api_key_roles.each do |api_key_role| %>
+        <tr class="owners__row">
+          <td class="owners__cell" data-title="Name">
+            <%= link_to api_key_role.name, profile_oidc_api_key_role_path(api_key_role.token) %>


It's a bit hard to tell that the link goes to the show page. Can we add a Details button on the side of each row like the edit button in API keys?

jenshenny · 2023-08-15T00:30:20Z

app/views/oidc/api_key_roles/show.html.erb

+  <div class="push--s">
+  <code><pre><%= JSON.pretty_generate @api_key_role.api_key_permissions.as_json %></pre></code>
+  </div>


Showing the raw json is a bit hard to read. Why not use the scopes, valid_for and gems from OIDC::ApiKeyPermissions?

jenshenny · 2023-08-15T00:34:05Z

app/views/api_keys/index.html.erb

+  <% if current_user.oidc_api_key_roles.any? %>
+    <p><%= button_to t("oidc.api_key_roles.index.api_key_roles"), profile_oidc_api_key_roles_path, method: "get", class: "form__submit" %></p>
+  <% end %>


nit: I would prefer to have the api_key_roles index view on the same page rather than having it separate. The button placement looks a bit off.

jenshenny · 2023-08-15T00:39:27Z

app/views/oidc/api_key_roles/show.html.erb

+        <tr class="owners__row owners__header">
+          <td class="owners__cell"><%= OIDC::IdToken.human_attribute_name(:created_at) %></td>
+          <td class="owners__cell"><%= ApiKey.human_attribute_name(:expires_at) %></td>
+          <td class="owners__cell"><%= OIDC::IdToken.human_attribute_name(:jti) %></td>
+          <td class="owners__cell"><%= OIDC::IdToken.human_attribute_name(:claims) %></td>
+          <td class="owners__cell"><%= OIDC::IdToken.human_attribute_name(:header) %></td>
+        </tr>
+      </thead>
+      <tbody class="t-body">
+        <% @api_key_role.id_tokens.each do |id_token| %>
+          <tr class="owners__row <%= 'owners__row__invalid' if id_token.api_key.expired? %>">
+            <td class="owners__cell"><%= id_token.created_at %></td>
+            <td class="owners__cell"><%= id_token.api_key.expires_at %></td>
+            <td class="owners__cell"><code><%= id_token.jti %></code></td>
+            <td class="owners__cell"><code><%= id_token.claims.to_json %></code></td>
+            <td class="owners__cell"><code><%= id_token.header.to_json %></code></td>
+          </tr>


This is pretty overwhelming. I think we can just get by with expired_at, created_at, and jti.

jenshenny · 2023-08-15T00:40:02Z

app/views/oidc/api_key_roles/show.html.erb

+        <tr class="owners__row owners__header">
+          <td class="owners__cell"><%= OIDC::IdToken.human_attribute_name(:created_at) %></td>
+          <td class="owners__cell"><%= ApiKey.human_attribute_name(:expires_at) %></td>
+          <td class="owners__cell"><%= OIDC::IdToken.human_attribute_name(:jti) %></td>


Instead of Jti for this, could we set it to JWT ID?

jenshenny · 2023-08-15T00:41:43Z

app/views/oidc/api_key_roles/show.html.erb

+
+  <h3 class="t-list__heading"><%= OIDC::ApiKeyRole.human_attribute_name(:access_policy) %></h3>
+  <div class="push--s">
+    <code><pre><%= JSON.pretty_generate @api_key_role.access_policy.as_json %></pre></code>


This is fine for now, but for GA, I don't want to show raw json.

segiddins · 2023-08-15T00:54:02Z

I mean, i didnt want to do the HTML in this PR but it seemed like it was a blocker

jenshenny · 2023-08-15T02:46:54Z

Apologies if it sounded like the HTML was a blocker for this PR specifically 😅 To rephrase my original comment, I agree with your plan to work on it after this PR was merged, as long as the views are going to be created before any announcements of the alpha release are sent out.

This reverts commit 73bf7c9.

segiddins · 2023-08-16T18:09:07Z

@jenshenny reverted the HTML, will revisit it as soon as this merges (will keep going on the same branch so we can use the same shipit env for it).

I don't plan on announcing the alpha -- the goal is to get a handful of folks (e.g. those on the rubygems / rubygems.org teams) using the feature before announcing it at all / opening it up for wider adoption. That way, we can make any adjustments as needed before committing to the API in any way.

jenshenny

I don't plan on announcing the alpha -- the goal is to get a handful of folks (e.g. those on the rubygems / rubygems.org teams) using the feature before announcing it at all / opening it up for wider adoption. That way, we can make any adjustments as needed before committing to the API in any way.

Oh ok, I misunderstood what alpha entails - that makes sense, thanks for clarifying!

segiddins force-pushed the oidc-api-tokens branch from 297b4de to 5439eaf Compare April 16, 2023 19:15

segiddins force-pushed the oidc-api-tokens branch from 473fa53 to 44807ef Compare April 18, 2023 11:09

rubygems-org-shipit bot had a problem deploying to oidc-api-token April 19, 2023 21:07 Failure

rubygems-org-shipit bot had a problem deploying to oidc-api-token April 19, 2023 21:13 Failure

rubygems-org-shipit bot had a problem deploying to oidc-api-token April 19, 2023 21:16 Failure

rubygems-org-shipit bot temporarily deployed to oidc-api-token April 19, 2023 21:21 Inactive

rubygems-org-shipit bot had a problem deploying to oidc-api-token April 19, 2023 21:40 Failure

rubygems-org-shipit bot temporarily deployed to oidc-api-token April 19, 2023 22:00 Inactive

rubygems-org-shipit bot had a problem deploying to oidc-api-token April 20, 2023 05:24 Failure

rubygems-org-shipit bot temporarily deployed to oidc-api-token April 20, 2023 07:51 Inactive

rubygems-org-shipit bot temporarily deployed to oidc-api-token April 20, 2023 19:05 Inactive

rubygems-org-shipit bot temporarily deployed to oidc-api-token April 20, 2023 19:27 Inactive

rubygems-org-shipit bot temporarily deployed to oidc-api-token April 20, 2023 20:55 Inactive

segiddins force-pushed the oidc-api-tokens branch from 68a4e97 to ce1b7f3 Compare April 26, 2023 20:17

rubygems-org-shipit bot temporarily deployed to oidc-api-token April 27, 2023 14:13 Inactive

segiddins force-pushed the oidc-api-tokens branch from ce1b7f3 to 0c37bf0 Compare June 22, 2023 17:53

segiddins mentioned this pull request Jul 7, 2023

OIDC API Key Role RFC rubygems/rfcs#49

Merged

segiddins force-pushed the oidc-api-tokens branch from 0c37bf0 to 0768e2e Compare July 7, 2023 03:06

rubygems-org-shipit bot temporarily deployed to oidc-api-token July 7, 2023 03:22 Inactive

segiddins force-pushed the oidc-api-tokens branch from 0768e2e to 6a66c70 Compare July 19, 2023 02:17

segiddins changed the title ~~WIP: Allow using OIDC to fetch api tokens~~ Allow using OIDC to fetch api tokens Jul 19, 2023

rubygems-org-shipit bot had a problem deploying to oidc-api-token July 19, 2023 05:36 Failure

segiddins force-pushed the oidc-api-tokens branch from ffbff8e to 9fb8546 Compare July 19, 2023 19:27

rubygems-org-shipit bot temporarily deployed to oidc-api-token July 19, 2023 19:44 Inactive

rubygems-org-shipit bot had a problem deploying to oidc-api-token July 19, 2023 20:41 Failure

segiddins force-pushed the oidc-api-tokens branch from f70c4c1 to 473fe23 Compare July 20, 2023 19:10

rubygems-org-shipit bot temporarily deployed to oidc-api-token July 20, 2023 19:57 Inactive

segiddins marked this pull request as ready for review July 20, 2023 19:59

indirect approved these changes Jul 20, 2023

View reviewed changes

segiddins added 5 commits August 3, 2023 16:27

Add expired/unexpired filter to admin api key resource

4d1ca00

RuboCop

a2f3bb1

Use token as title for Api key roles

e2c0b14

Name splat args to unbreak breakman

bd73cd4

See presidentbeef/brakeman#1765

Add tests for invalid times in #verify_jwt

d491702

segiddins force-pushed the oidc-api-tokens branch from 8001606 to d491702 Compare August 3, 2023 23:56

jenshenny reviewed Aug 4, 2023

View reviewed changes

Remove provider_id from oidc_id_tokens

f984bb4

Use a :through relationship instead

segiddins commented Aug 4, 2023

View reviewed changes

rubygems-org-shipit bot temporarily deployed to oidc-api-token August 4, 2023 23:17 Inactive

dentarg mentioned this pull request Aug 7, 2023

Improve release workflow sinatra/sinatra#1920

Closed

Add basic index/show pages for OIDC::ApiKeyRole

73bf7c9

rubygems-org-shipit bot had a problem deploying to oidc-api-token August 14, 2023 20:20 Failure

gen_erd

f844933

rubygems-org-shipit bot temporarily deployed to oidc-api-token August 14, 2023 23:34 Inactive

jenshenny reviewed Aug 15, 2023

View reviewed changes

Revert "Add basic index/show pages for OIDC::ApiKeyRole"

760ddb5

This reverts commit 73bf7c9.

rubygems-org-shipit bot had a problem deploying to oidc-api-token August 16, 2023 18:08 Failure

jenshenny approved these changes Aug 17, 2023

View reviewed changes

Merge branch 'master' into oidc-api-tokens

e005ff2

rubygems-org-shipit bot temporarily deployed to oidc-api-token August 17, 2023 23:11 Inactive

segiddins merged commit a2ae029 into master Aug 17, 2023
14 of 15 checks passed

segiddins deleted the oidc-api-tokens branch August 17, 2023 23:19

segiddins restored the oidc-api-tokens branch August 17, 2023 23:19

segiddins mentioned this pull request Sep 11, 2023

Publish August 2023 RubyGems Monthly Update rubygems/rubygems.github.io#169

Merged

indirect mentioned this pull request Oct 11, 2023

OIDC support for short-lived API keys #4126

Open

3 tasks

		@@ -0,0 +1,18 @@
		require "test_helper"

		class OIDC::ProviderPolicyTest < ActiveSupport::TestCase

		@@ -0,0 +1,18 @@
		require "test_helper"

		class OIDC::Provider::ConfigurationPolicyTest < ActiveSupport::TestCase

		@@ -0,0 +1,18 @@
		require "test_helper"

		class OIDC::IdTokenPolicyTest < ActiveSupport::TestCase

		@@ -0,0 +1,18 @@
		require "test_helper"

		class OIDC::ApiKeyRolePolicyTest < ActiveSupport::TestCase

		@@ -0,0 +1,7 @@
		require "test_helper"

		class OIDC::IdTokenTest < ActiveSupport::TestCase

Allow using OIDC to fetch api tokens #3716

Allow using OIDC to fetch api tokens #3716

Conversation

segiddins commented Apr 16, 2023 • edited Loading

codecov bot commented Apr 16, 2023 • edited Loading

Codecov Report

indirect left a comment

Choose a reason for hiding this comment

jenshenny left a comment

Choose a reason for hiding this comment

jenshenny Aug 3, 2023 • edited Loading

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

jenshenny Aug 4, 2023 • edited Loading

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

jenshenny left a comment

Choose a reason for hiding this comment

jenshenny Aug 4, 2023 • edited Loading

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

jenshenny 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

segiddins commented Aug 15, 2023

jenshenny commented Aug 15, 2023 • edited Loading

segiddins commented Aug 16, 2023

jenshenny left a comment

Choose a reason for hiding this comment

segiddins commented Apr 16, 2023 •

edited

Loading

codecov bot commented Apr 16, 2023 •

edited

Loading

jenshenny Aug 3, 2023 •

edited

Loading

jenshenny Aug 4, 2023 •

edited

Loading

jenshenny Aug 4, 2023 •

edited

Loading

jenshenny commented Aug 15, 2023 •

edited

Loading