From 327cf23e823ff04589c946304f6b89112947a70b Mon Sep 17 00:00:00 2001 From: Samuel Giddins Date: Fri, 20 Dec 2024 11:25:36 -0800 Subject: [PATCH] Guard against OIDC Provider configuration being stale (#5296) Fixes TOB-RGM-14 --- .../api/v1/oidc/api_key_roles_controller.rb | 29 ++++------- .../v1/oidc/trusted_publisher_controller.rb | 34 ++----------- app/controllers/concerns/jwt_validation.rb | 51 +++++++++++++++++++ app/jobs/refresh_oidc_provider_job.rb | 1 + ...figuration_updated_at_to_oidc_providers.rb | 5 ++ db/schema.rb | 1 + .../oidc/trusted_publisher_controller_test.rb | 14 +++++ 7 files changed, 87 insertions(+), 48 deletions(-) create mode 100644 app/controllers/concerns/jwt_validation.rb create mode 100644 db/migrate/20240910152643_add_configuration_updated_at_to_oidc_providers.rb diff --git a/app/controllers/api/v1/oidc/api_key_roles_controller.rb b/app/controllers/api/v1/oidc/api_key_roles_controller.rb index d3a9ae30cca..07a5a20e084 100644 --- a/app/controllers/api/v1/oidc/api_key_roles_controller.rb +++ b/app/controllers/api/v1/oidc/api_key_roles_controller.rb @@ -1,26 +1,20 @@ class Api::V1::OIDC::ApiKeyRolesController < Api::BaseController include ApiKeyable + include JwtValidation before_action :authenticate_with_api_key, except: :assume_role before_action :verify_user_api_key, except: :assume_role with_options only: :assume_role do before_action :set_api_key_role + before_action :validate_provider before_action :decode_jwt - before_action :verify_jwt + before_action :validate_jwt_format + before_action :verify_jwt_time + before_action :verify_jwt_issuer before_action :verify_access end - class UnverifiedJWT < StandardError - end - - rescue_from( - UnverifiedJWT, - JSON::JWT::VerificationFailed, JSON::JWK::Set::KidNotFound, - OIDC::AccessPolicy::AccessError, - with: :render_not_found - ) - rescue_from ActiveRecord::RecordInvalid do |err| render json: { errors: err.record.errors @@ -67,18 +61,15 @@ def assume_role def set_api_key_role @api_key_role = OIDC::ApiKeyRole.active.find_by!(token: params.permit(:token).require(:token)) + @provider = @api_key_role.provider end - def decode_jwt - raise UnverifiedJWT, "Provider missing JWKS" if @api_key_role.provider.jwks.blank? - @jwt = JSON::JWT.decode_compact_serialized(params.permit(:jwt).require(:jwt), @api_key_role.provider.jwks) - rescue JSON::ParserError - raise UnverifiedJWT, "Invalid JSON" + def jwt_key_or_secret + @provider.jwks end - def verify_jwt - raise UnverifiedJWT, "Issuer mismatch" unless @api_key_role.provider.issuer == @jwt["iss"] - raise UnverifiedJWT, "Invalid time" unless (@jwt["nbf"]..@jwt["exp"]).cover?(Time.now.to_i) + def verify_jwt_issuer + raise UnverifiedJWT, "Issuer mismatch" unless @provider.issuer == @jwt["iss"] end def verify_access diff --git a/app/controllers/api/v1/oidc/trusted_publisher_controller.rb b/app/controllers/api/v1/oidc/trusted_publisher_controller.rb index 3da6fe8d22e..f4aca943ed0 100644 --- a/app/controllers/api/v1/oidc/trusted_publisher_controller.rb +++ b/app/controllers/api/v1/oidc/trusted_publisher_controller.rb @@ -1,26 +1,16 @@ class Api::V1::OIDC::TrustedPublisherController < Api::BaseController include ApiKeyable + include JwtValidation before_action :decode_jwt before_action :validate_jwt_format + before_action :verify_jwt_time before_action :find_provider + before_action :validate_provider before_action :verify_signature before_action :find_trusted_publisher before_action :validate_claims - class UnsupportedIssuer < StandardError; end - class UnverifiedJWT < StandardError; end - class InvalidJWT < StandardError; end - - rescue_from InvalidJWT, with: :render_bad_request - - rescue_from( - UnsupportedIssuer, UnverifiedJWT, - JSON::JWT::VerificationFailed, JSON::JWK::Set::KidNotFound, - OIDC::AccessPolicy::AccessError, - with: :render_not_found - ) - def exchange_token key = generate_unique_rubygems_key iat = Time.at(@jwt[:iat].to_i, in: "UTC") @@ -42,20 +32,8 @@ def exchange_token private - def decode_jwt - @jwt = JSON::JWT.decode_compact_serialized(params.permit(:jwt).require(:jwt), :skip_verification) - rescue JSON::JWT::InvalidFormat, JSON::ParserError, ArgumentError => e - # invalid base64 raises ArgumentError - render_bad_request(e) - end - - def validate_jwt_format - %w[nbf iat exp].each do |claim| - raise InvalidJWT, "Missing/invalid #{claim}" unless @jwt[claim].is_a?(Integer) - end - %w[iss jti].each do |claim| - raise InvalidJWT, "Missing/invalid #{claim}" unless @jwt[claim].is_a?(String) - end + def jwt_key_or_secret + :skip_verification end def find_provider @@ -63,8 +41,6 @@ def find_provider end def verify_signature - raise UnsupportedIssuer, "Provider is missing jwks" if @provider.jwks.blank? - raise UnverifiedJWT, "Invalid time" unless (@jwt["nbf"]..@jwt["exp"]).cover?(Time.now.to_i) @jwt.verify!(@provider.jwks) end diff --git a/app/controllers/concerns/jwt_validation.rb b/app/controllers/concerns/jwt_validation.rb new file mode 100644 index 00000000000..aeac079fb8b --- /dev/null +++ b/app/controllers/concerns/jwt_validation.rb @@ -0,0 +1,51 @@ +module JwtValidation + extend ActiveSupport::Concern + + class UnsupportedIssuer < StandardError; end + class UnverifiedJWT < StandardError; end + class InvalidJWT < StandardError; end + + included do + rescue_from InvalidJWT, with: :render_bad_request + + rescue_from( + UnsupportedIssuer, UnverifiedJWT, + JSON::JWT::VerificationFailed, JSON::JWK::Set::KidNotFound, + OIDC::AccessPolicy::AccessError, + with: :render_not_found + ) + end + + def jwt_key_or_secret + raise NotImplementedError + end + + def decode_jwt + @jwt = JSON::JWT.decode_compact_serialized(params.permit(:jwt).require(:jwt), jwt_key_or_secret) + rescue JSON::JWT::InvalidFormat, JSON::ParserError, ArgumentError => e + # invalid base64 raises ArgumentError + render_bad_request(e) + end + + def validate_jwt_format + %w[nbf iat exp].each do |claim| + raise InvalidJWT, "Missing/invalid #{claim}" unless @jwt[claim].is_a?(Integer) + end + %w[iss jti].each do |claim| + raise InvalidJWT, "Missing/invalid #{claim}" unless @jwt[claim].is_a?(String) + end + end + + def validate_provider + raise UnsupportedIssuer, "Provider is missing jwks" if @provider.jwks.blank? + # TODO: delete &. after all providers have updated their configuration + return unless @provider.configuration_updated_at&.before?(1.day.ago) + raise UnsupportedIssuer, "Configuration last updated too long ago: #{@provider.configuration_updated_at}" + end + + def verify_jwt_time + now = Time.now.to_i + return if @jwt["nbf"] <= now && now < @jwt["exp"] + raise UnverifiedJWT, "Invalid time" + end +end diff --git a/app/jobs/refresh_oidc_provider_job.rb b/app/jobs/refresh_oidc_provider_job.rb index e7edcb433cd..e15946c0829 100644 --- a/app/jobs/refresh_oidc_provider_job.rb +++ b/app/jobs/refresh_oidc_provider_job.rb @@ -21,6 +21,7 @@ def perform(provider:) raise JWKSURIMismatchError, "Invalid JWKS URI in OpenID Connect configuration #{provider.configuration.jwks_uri.inspect}" end provider.jwks = connection.get(provider.configuration.jwks_uri).body + provider.configuration_updated_at = Time.current provider.save! end diff --git a/db/migrate/20240910152643_add_configuration_updated_at_to_oidc_providers.rb b/db/migrate/20240910152643_add_configuration_updated_at_to_oidc_providers.rb new file mode 100644 index 00000000000..96753beb392 --- /dev/null +++ b/db/migrate/20240910152643_add_configuration_updated_at_to_oidc_providers.rb @@ -0,0 +1,5 @@ +class AddConfigurationUpdatedAtToOIDCProviders < ActiveRecord::Migration[7.1] + def change + add_column :oidc_providers, :configuration_updated_at, :timestamp + end +end diff --git a/db/schema.rb b/db/schema.rb index 99235afcc75..8d0190f2dac 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -395,6 +395,7 @@ t.jsonb "jwks" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.datetime "configuration_updated_at", precision: nil t.index ["issuer"], name: "index_oidc_providers_on_issuer", unique: true end diff --git a/test/integration/api/v1/oidc/trusted_publisher_controller_test.rb b/test/integration/api/v1/oidc/trusted_publisher_controller_test.rb index 5d704d43818..aa392b43123 100644 --- a/test/integration/api/v1/oidc/trusted_publisher_controller_test.rb +++ b/test/integration/api/v1/oidc/trusted_publisher_controller_test.rb @@ -168,6 +168,20 @@ def jwt(claims = @claims, key: @pkey) assert_response :not_found end + should "return not found when provider configuration is too old" do + OIDC::Provider.github_actions.update!(configuration_updated_at: 2.days.ago) + trusted_publisher = build(:oidc_trusted_publisher_github_action, + repository_name: "oidc-test", + repository_owner_id: "1946610", + workflow_filename: "token.yml") + trusted_publisher.repository_owner = "segiddins" + trusted_publisher.save! + post api_v1_oidc_trusted_publisher_exchange_token_path, + params: { jwt: jwt.to_s } + + assert_response :not_found + end + should "return not found when signature validation fails" do @claims["exp"] -= 1_000_000 trusted_publisher = build(:oidc_trusted_publisher_github_action,