From e7a15efa57866903c0680137630c0f26fa226626 Mon Sep 17 00:00:00 2001 From: Jason Vanderhoof Date: Thu, 2 Feb 2023 11:48:05 -0700 Subject: [PATCH] Remove support for disabling the non-PKCE OIDC authentication flow --- CHANGELOG.md | 18 +- app/db/repository/authenticator_repository.rb | 17 +- .../authn_oidc/authenticator.rb | 7 +- .../authn_oidc/pkce_support_feature/client.rb | 115 ------ .../data_objects/authenticator.rb | 69 ---- .../pkce_support_feature/resolve_identity.rb | 19 - .../pkce_support_feature/strategy.rb | 42 --- .../views/provider_context.rb | 66 ---- .../authentication/authn_oidc/v2/client.rb | 53 ++- .../v2/data_objects/authenticator.rb | 22 +- .../authentication/authn_oidc/v2/status.rb | 13 - .../authentication/authn_oidc/v2/strategy.rb | 13 +- .../authn_oidc/v2/views/provider_context.rb | 52 ++- .../handler/authentication_handler.rb | 13 +- .../authentication/util/namespace_selector.rb | 12 +- .../authenticator_repository_spec.rb | 353 +++++++++--------- .../pkce_support_feature/client_spec.rb | 195 ---------- .../data_objects/authenticator.rb | 92 ----- .../resolve_identity_spec.rb | 96 ----- .../pkce_support_feature/strategy_rspec.rb | 80 ---- .../views/providor_context_spec.rb | 126 ------- .../authn-oidc/v2/client_spec.rb | 117 +++--- .../v2/data_objects/authenticator.rb | 11 +- .../authn-oidc/v2/strategy_rspec.rb | 40 +- .../v2/views/providor_context_spec.rb | 117 ++++-- .../util/namespace_selector_spec.rb | 27 +- 26 files changed, 474 insertions(+), 1311 deletions(-) delete mode 100644 app/domain/authentication/authn_oidc/pkce_support_feature/client.rb delete mode 100644 app/domain/authentication/authn_oidc/pkce_support_feature/data_objects/authenticator.rb delete mode 100644 app/domain/authentication/authn_oidc/pkce_support_feature/resolve_identity.rb delete mode 100644 app/domain/authentication/authn_oidc/pkce_support_feature/strategy.rb delete mode 100644 app/domain/authentication/authn_oidc/pkce_support_feature/views/provider_context.rb delete mode 100644 app/domain/authentication/authn_oidc/v2/status.rb delete mode 100644 spec/app/domain/authentication/authn-oidc/pkce_support_feature/client_spec.rb delete mode 100644 spec/app/domain/authentication/authn-oidc/pkce_support_feature/data_objects/authenticator.rb delete mode 100644 spec/app/domain/authentication/authn-oidc/pkce_support_feature/resolve_identity_spec.rb delete mode 100644 spec/app/domain/authentication/authn-oidc/pkce_support_feature/strategy_rspec.rb delete mode 100644 spec/app/domain/authentication/authn-oidc/pkce_support_feature/views/providor_context_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 1954a8b5c2..8c449d9a8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,19 +9,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Nothing should go in this section, please add to the latest unreleased version (and update the corresponding date), or add a new version. -## [1.19.3] - 2023-01-26 +## [1.19.2] - 2023-2-1 -## [1.19.2] - 2022-01-13 +### Changed -### Fixed -- Previously, including `limit` or `offset` parameters to a resource list request - resulted in the returned list being unexpectedly sorted. Now, all resource list - request results are sorted by resource ID. - [cyberark/conjur#2702](https://github.com/cyberark/conjur/pull/2702) - -### Security -- Upgraded Rails to 6.1.7.1 to resolve CVE-2023-22794 (not vulnerable) - [cyberark/conjur#2703](https://github.com/cyberark/conjur/pull/2703) +- Removes support for disabling the `CONJUR_FEATURE_PKCE_SUPPORT_ENABLED` flag. + [cyberark/conjur#2713](https://github.com/cyberark/conjur/pull/2713) ## [1.19.1] - 2022-12-08 @@ -75,6 +68,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Updated nokogiri in root and docs Gemfile.lock files to resolve GHSA-2qc6-mcvw-92cw [cyberark/conjur#2670](https://github.com/cyberark/conjur/pull/2670) +### Changed +- Changes the Conjur Auth token TTL for OIDC authentication to 60 minutes. + [] ## [1.18.5] - 2022-09-14 ### Added diff --git a/app/db/repository/authenticator_repository.rb b/app/db/repository/authenticator_repository.rb index 5e8c7e3c45..45694dce96 100644 --- a/app/db/repository/authenticator_repository.rb +++ b/app/db/repository/authenticator_repository.rb @@ -4,13 +4,14 @@ class AuthenticatorRepository def initialize( data_object:, resource_repository: ::Resource, - logger: Rails.logger, - pkce_support_enabled: Rails.configuration.feature_flags.enabled?(:pkce_support) + logger: Rails.logger + # , + # pkce_support_enabled: Rails.configuration.feature_flags.enabled?(:pkce_support) ) @resource_repository = resource_repository @data_object = data_object @logger = logger - @pkce_support_enabled = pkce_support_enabled + # @pkce_support_enabled = pkce_support_enabled end def find_all(type:, account:) @@ -73,12 +74,10 @@ def load_authenticator(type:, account:, service_id:) end begin - if @pkce_support_enabled - allowed_args = %i[account service_id] + - @data_object.const_get(:REQUIRED_VARIABLES) + - @data_object.const_get(:OPTIONAL_VARIABLES) - args_list = args_list.select{ |key, value| allowed_args.include?(key) && value.present? } - end + allowed_args = %i[account service_id] + + @data_object.const_get(:REQUIRED_VARIABLES) + + @data_object.const_get(:OPTIONAL_VARIABLES) + args_list = args_list.select { |key, value| allowed_args.include?(key) && value.present? } @data_object.new(**args_list) rescue ArgumentError => e @logger.debug("DB::Repository::AuthenticatorRepository.load_authenticator - exception: #{e}") diff --git a/app/domain/authentication/authn_oidc/authenticator.rb b/app/domain/authentication/authn_oidc/authenticator.rb index ddf165f989..9f765b6ee4 100644 --- a/app/domain/authentication/authn_oidc/authenticator.rb +++ b/app/domain/authentication/authn_oidc/authenticator.rb @@ -29,13 +29,8 @@ def status(authenticator_status_input:) # is done, the following check can be removed. # Attempt to load the V2 version of the OIDC Authenticator - data_object = if Rails.configuration.feature_flags.enabled?(:pkce_support) - Authentication::AuthnOidc::PkceSupportFeature::DataObjects::Authenticator - else - Authentication::AuthnOidc::V2::DataObjects::Authenticator - end authenticator = DB::Repository::AuthenticatorRepository.new( - data_object: data_object + data_object: Authentication::AuthnOidc::V2::DataObjects::Authenticator ).find( type: authenticator_status_input.authenticator_name, account: authenticator_status_input.account, diff --git a/app/domain/authentication/authn_oidc/pkce_support_feature/client.rb b/app/domain/authentication/authn_oidc/pkce_support_feature/client.rb deleted file mode 100644 index b4ae42b172..0000000000 --- a/app/domain/authentication/authn_oidc/pkce_support_feature/client.rb +++ /dev/null @@ -1,115 +0,0 @@ -module Authentication - module AuthnOidc - module PkceSupportFeature - class Client - def initialize( - authenticator:, - client: ::OpenIDConnect::Client, - oidc_id_token: ::OpenIDConnect::ResponseObject::IdToken, - discovery_configuration: ::OpenIDConnect::Discovery::Provider::Config, - cache: Rails.cache, - logger: Rails.logger - ) - @authenticator = authenticator - @client = client - @oidc_id_token = oidc_id_token - @discovery_configuration = discovery_configuration - @cache = cache - @logger = logger - end - - def oidc_client - @oidc_client ||= begin - issuer_uri = URI(@authenticator.provider_uri) - @client.new( - identifier: @authenticator.client_id, - secret: @authenticator.client_secret, - redirect_uri: @authenticator.redirect_uri, - scheme: issuer_uri.scheme, - host: issuer_uri.host, - port: issuer_uri.port, - authorization_endpoint: URI(discovery_information.authorization_endpoint).path, - token_endpoint: URI(discovery_information.token_endpoint).path, - userinfo_endpoint: URI(discovery_information.userinfo_endpoint).path, - jwks_uri: URI(discovery_information.jwks_uri).path, - end_session_endpoint: URI(discovery_information.end_session_endpoint).path - ) - end - end - - def callback(code:, nonce:, code_verifier: nil) - oidc_client.authorization_code = code - access_token_args = { scope: true, client_auth_method: :basic } - access_token_args[:code_verifier] = code_verifier if code_verifier.present? - begin - bearer_token = oidc_client.access_token!(**access_token_args) - rescue Rack::OAuth2::Client::Error => e - # Only handle the expected errors related to access token retrieval. - case e.message - when /PKCE verification failed/ - raise Errors::Authentication::AuthnOidc::TokenRetrievalFailed, - 'PKCE verification failed' - when /The authorization code is invalid or has expired/ - raise Errors::Authentication::AuthnOidc::TokenRetrievalFailed, - 'Authorization code is invalid or has expired' - when /Code not valid/ - raise Errors::Authentication::AuthnOidc::TokenRetrievalFailed, - 'Authorization code is invalid' - end - raise e - end - id_token = bearer_token.id_token || bearer_token.access_token - - begin - attempts ||= 0 - decoded_id_token = @oidc_id_token.decode( - id_token, - discovery_information.jwks - ) - rescue StandardError => e - attempts += 1 - raise e if attempts > 1 - - # If the JWKS verification fails, blow away the existing cache and - # try again. This is intended to handle the case where the OIDC certificate - # changes, and we want to cache the new certificate without decode failing. - discovery_information(invalidate: true) - retry - end - - begin - decoded_id_token.verify!( - issuer: @authenticator.provider_uri, - client_id: @authenticator.client_id, - nonce: nonce - ) - rescue OpenIDConnect::ResponseObject::IdToken::InvalidNonce - raise Errors::Authentication::AuthnOidc::TokenVerificationFailed, - 'Provided nonce does not match the nonce in the JWT' - rescue OpenIDConnect::ResponseObject::IdToken::ExpiredToken - raise Errors::Authentication::AuthnOidc::TokenVerificationFailed, - 'JWT has expired' - rescue OpenIDConnect::ValidationFailed => e - raise Errors::Authentication::AuthnOidc::TokenVerificationFailed, - e.message - end - decoded_id_token - end - - def discovery_information(invalidate: false) - @cache.fetch( - "#{@authenticator.account}/#{@authenticator.service_id}/#{URI::Parser.new.escape(@authenticator.provider_uri)}", - force: invalidate, - skip_nil: true - ) do - @discovery_configuration.discover!(@authenticator.provider_uri) - rescue HTTPClient::ConnectTimeoutError, Errno::ETIMEDOUT => e - raise Errors::Authentication::OAuth::ProviderDiscoveryTimeout.new(@authenticator.provider_uri, e.message) - rescue => e - raise Errors::Authentication::OAuth::ProviderDiscoveryFailed.new(@authenticator.provider_uri, e.message) - end - end - end - end - end -end diff --git a/app/domain/authentication/authn_oidc/pkce_support_feature/data_objects/authenticator.rb b/app/domain/authentication/authn_oidc/pkce_support_feature/data_objects/authenticator.rb deleted file mode 100644 index 98e3f5556a..0000000000 --- a/app/domain/authentication/authn_oidc/pkce_support_feature/data_objects/authenticator.rb +++ /dev/null @@ -1,69 +0,0 @@ -module Authentication - module AuthnOidc - module PkceSupportFeature - module DataObjects - class Authenticator - - REQUIRED_VARIABLES = %i[provider_uri client_id client_secret claim_mapping].freeze - OPTIONAL_VARIABLES = %i[redirect_uri response_type provider_scope name token_ttl].freeze - - attr_reader( - :provider_uri, - :client_id, - :client_secret, - :claim_mapping, - :account, - :service_id, - :redirect_uri, - :response_type - ) - - def initialize( - provider_uri:, - client_id:, - client_secret:, - claim_mapping:, - account:, - service_id:, - redirect_uri: nil, - name: nil, - response_type: 'code', - provider_scope: nil, - token_ttl: 'PT8M' - ) - @account = account - @provider_uri = provider_uri - @client_id = client_id - @client_secret = client_secret - @claim_mapping = claim_mapping - @response_type = response_type - @service_id = service_id - @name = name - @provider_scope = provider_scope - @redirect_uri = redirect_uri - @token_ttl = token_ttl - end - - def scope - (%w[openid email profile] + [*@provider_scope.to_s.split(' ')]).uniq.join(' ') - end - - def name - @name || @service_id.titleize - end - - def resource_id - "#{account}:webservice:conjur/authn-oidc/#{service_id}" - end - - # Returns the validity duration, in seconds, of an instance's access tokens. - def token_ttl - ActiveSupport::Duration.parse(@token_ttl) - rescue ActiveSupport::Duration::ISO8601Parser::ParsingError - raise Errors::Authentication::DataObjects::InvalidTokenTTL.new(resource_id, @token_ttl) - end - end - end - end - end -end diff --git a/app/domain/authentication/authn_oidc/pkce_support_feature/resolve_identity.rb b/app/domain/authentication/authn_oidc/pkce_support_feature/resolve_identity.rb deleted file mode 100644 index 4ac21cb8b2..0000000000 --- a/app/domain/authentication/authn_oidc/pkce_support_feature/resolve_identity.rb +++ /dev/null @@ -1,19 +0,0 @@ -module Authentication - module AuthnOidc - module PkceSupportFeature - class ResolveIdentity - def call(identity:, account:, allowed_roles:) - # make sure role has a resource (ex. user, host) - roles = allowed_roles.select(&:resource?) - - roles.each do |role| - role_account, _, role_id = role.id.split(':') - return role if role_account == account && identity == role_id - end - - raise(Errors::Authentication::Security::RoleNotFound, identity) - end - end - end - end -end diff --git a/app/domain/authentication/authn_oidc/pkce_support_feature/strategy.rb b/app/domain/authentication/authn_oidc/pkce_support_feature/strategy.rb deleted file mode 100644 index c8f92bf993..0000000000 --- a/app/domain/authentication/authn_oidc/pkce_support_feature/strategy.rb +++ /dev/null @@ -1,42 +0,0 @@ -module Authentication - module AuthnOidc - module PkceSupportFeature - class Strategy - def initialize( - authenticator:, - client: Authentication::AuthnOidc::PkceSupportFeature::Client, - logger: Rails.logger - ) - @authenticator = authenticator - @client = client.new(authenticator: authenticator) - @logger = logger - end - - def callback(args) - %i[code nonce].each do |param| - unless args[param].present? - raise Errors::Authentication::RequestBody::MissingRequestParam, param.to_s - end - end - - identity = resolve_identity( - jwt: @client.callback( - code: args[:code], - nonce: args[:nonce], - code_verifier: args[:code_verifier] - ) - ) - unless identity.present? - raise Errors::Authentication::AuthnOidc::IdTokenClaimNotFoundOrEmpty, - @authenticator.claim_mapping - end - identity - end - - def resolve_identity(jwt:) - jwt.raw_attributes.with_indifferent_access[@authenticator.claim_mapping] - end - end - end - end -end diff --git a/app/domain/authentication/authn_oidc/pkce_support_feature/views/provider_context.rb b/app/domain/authentication/authn_oidc/pkce_support_feature/views/provider_context.rb deleted file mode 100644 index adb4ba3ee8..0000000000 --- a/app/domain/authentication/authn_oidc/pkce_support_feature/views/provider_context.rb +++ /dev/null @@ -1,66 +0,0 @@ -require 'securerandom' -require 'digest' - -module Authentication - module AuthnOidc - module PkceSupportFeature - module Views - class ProviderContext - def initialize( - client: Authentication::AuthnOidc::PkceSupportFeature::Client, - digest: Digest::SHA256, - random: SecureRandom, - logger: Rails.logger - ) - @client = client - @logger = logger - @digest = digest - @random = random - end - - def call(authenticators:) - authenticators.map do |authenticator| - begin - nonce = @random.hex(25) - code_verifier = @random.hex(25) - code_challenge = @digest.base64digest(code_verifier).tr("+/", "-_").tr("=", "") - { - service_id: authenticator.service_id, - type: 'authn-oidc', - name: authenticator.name, - nonce: nonce, - code_verifier: code_verifier, - redirect_uri: generate_redirect_url( - client: @client.new(authenticator: authenticator), - authenticator: authenticator, - nonce: nonce, - code_challenge: code_challenge - ) - } - rescue Errors::Authentication::OAuth::ProviderDiscoveryFailed, - Errors::Authentication::OAuth::ProviderDiscoveryTimeout - @logger.warn("Authn-OIDC '#{authenticator.service_id}' provider-uri: '#{authenticator.provider_uri}' is unreachable") - nil - end - end.compact - end - - def generate_redirect_url(client:, authenticator:, nonce:, code_challenge:) - params = { - client_id: authenticator.client_id, - response_type: authenticator.response_type, - scope: ERB::Util.url_encode(authenticator.scope), - nonce: nonce, - code_challenge: code_challenge, - code_challenge_method: 'S256', - redirect_uri: ERB::Util.url_encode(authenticator.redirect_uri) - } - formatted_params = params.map { |key, value| "#{key}=#{value}" }.join("&") - - "#{client.discovery_information.authorization_endpoint}?#{formatted_params}" - end - end - end - end - end -end diff --git a/app/domain/authentication/authn_oidc/v2/client.rb b/app/domain/authentication/authn_oidc/v2/client.rb index cc62b873f3..9bf58541c1 100644 --- a/app/domain/authentication/authn_oidc/v2/client.rb +++ b/app/domain/authentication/authn_oidc/v2/client.rb @@ -37,17 +37,27 @@ def oidc_client end end - def callback(code:) - unless code.present? - raise Errors::Authentication::RequestBody::MissingRequestParam, 'code' - end - + def callback(code:, nonce:, code_verifier: nil) oidc_client.authorization_code = code - bearer_token = oidc_client.access_token!( - scope: true, - client_auth_method: :basic, - nonce: @authenticator.nonce - ) + access_token_args = { scope: true, client_auth_method: :basic } + access_token_args[:code_verifier] = code_verifier if code_verifier.present? + begin + bearer_token = oidc_client.access_token!(**access_token_args) + rescue Rack::OAuth2::Client::Error => e + # Only handle the expected errors related to access token retrieval. + case e.message + when /PKCE verification failed/ + raise Errors::Authentication::AuthnOidc::TokenRetrievalFailed, + 'PKCE verification failed' + when /The authorization code is invalid or has expired/ + raise Errors::Authentication::AuthnOidc::TokenRetrievalFailed, + 'Authorization code is invalid or has expired' + when /Code not valid/ + raise Errors::Authentication::AuthnOidc::TokenRetrievalFailed, + 'Authorization code is invalid' + end + raise e + end id_token = bearer_token.id_token || bearer_token.access_token begin @@ -67,14 +77,23 @@ def callback(code:) retry end - decoded_id_token.verify!( - issuer: @authenticator.provider_uri, - client_id: @authenticator.client_id, - nonce: @authenticator.nonce - ) + begin + decoded_id_token.verify!( + issuer: @authenticator.provider_uri, + client_id: @authenticator.client_id, + nonce: nonce + ) + rescue OpenIDConnect::ResponseObject::IdToken::InvalidNonce + raise Errors::Authentication::AuthnOidc::TokenVerificationFailed, + 'Provided nonce does not match the nonce in the JWT' + rescue OpenIDConnect::ResponseObject::IdToken::ExpiredToken + raise Errors::Authentication::AuthnOidc::TokenVerificationFailed, + 'JWT has expired' + rescue OpenIDConnect::ValidationFailed => e + raise Errors::Authentication::AuthnOidc::TokenVerificationFailed, + e.message + end decoded_id_token - rescue OpenIDConnect::ValidationFailed => e - raise Errors::Authentication::AuthnOidc::TokenVerificationFailed, e.message end def discovery_information(invalidate: false) diff --git a/app/domain/authentication/authn_oidc/v2/data_objects/authenticator.rb b/app/domain/authentication/authn_oidc/v2/data_objects/authenticator.rb index 3e42aa173e..a5c3d661e2 100644 --- a/app/domain/authentication/authn_oidc/v2/data_objects/authenticator.rb +++ b/app/domain/authentication/authn_oidc/v2/data_objects/authenticator.rb @@ -4,17 +4,25 @@ module V2 module DataObjects class Authenticator - # required - attr_reader :provider_uri, :client_id, :client_secret, :claim_mapping, :nonce, :state, :account - attr_reader :service_id, :redirect_uri, :response_type + REQUIRED_VARIABLES = %i[provider_uri client_id client_secret claim_mapping].freeze + OPTIONAL_VARIABLES = %i[redirect_uri response_type provider_scope name token_ttl].freeze + + attr_reader( + :provider_uri, + :client_id, + :client_secret, + :claim_mapping, + :account, + :service_id, + :redirect_uri, + :response_type + ) def initialize( provider_uri:, client_id:, client_secret:, claim_mapping:, - nonce:, - state:, account:, service_id:, redirect_uri: nil, @@ -28,9 +36,7 @@ def initialize( @client_id = client_id @client_secret = client_secret @claim_mapping = claim_mapping - @nonce = nonce @response_type = response_type - @state = state @service_id = service_id @name = name @provider_scope = provider_scope @@ -39,7 +45,7 @@ def initialize( end def scope - (%w[openid email profile] + [*@provider_scope]).uniq.join(' ') + (%w[openid email profile] + [*@provider_scope.to_s.split(' ')]).uniq.join(' ') end def name diff --git a/app/domain/authentication/authn_oidc/v2/status.rb b/app/domain/authentication/authn_oidc/v2/status.rb deleted file mode 100644 index 27de1a96f1..0000000000 --- a/app/domain/authentication/authn_oidc/v2/status.rb +++ /dev/null @@ -1,13 +0,0 @@ -module Authentication - module AuthnOidc - module V2 - class Status - def call(authenticator:) - { - - } - end - end - end - end -end diff --git a/app/domain/authentication/authn_oidc/v2/strategy.rb b/app/domain/authentication/authn_oidc/v2/strategy.rb index 003c9bf83d..cabce019f7 100644 --- a/app/domain/authentication/authn_oidc/v2/strategy.rb +++ b/app/domain/authentication/authn_oidc/v2/strategy.rb @@ -1,4 +1,3 @@ - module Authentication module AuthnOidc module V2 @@ -14,12 +13,18 @@ def initialize( end def callback(args) - # TODO: Check that `code` and `state` attributes are present - raise Errors::Authentication::AuthnOidc::StateMismatch unless args[:state] == @authenticator.state + # Note: `code_verifier` param is optional + %i[code nonce].each do |param| + unless args[param].present? + raise Errors::Authentication::RequestBody::MissingRequestParam, param.to_s + end + end identity = resolve_identity( jwt: @client.callback( - code: args[:code] + code: args[:code], + nonce: args[:nonce], + code_verifier: args[:code_verifier] ) ) unless identity.present? diff --git a/app/domain/authentication/authn_oidc/v2/views/provider_context.rb b/app/domain/authentication/authn_oidc/v2/views/provider_context.rb index 3f7eff5d34..1e54b06e2b 100644 --- a/app/domain/authentication/authn_oidc/v2/views/provider_context.rb +++ b/app/domain/authentication/authn_oidc/v2/views/provider_context.rb @@ -1,3 +1,6 @@ +require 'securerandom' +require 'digest' + module Authentication module AuthnOidc module V2 @@ -5,37 +8,56 @@ module Views class ProviderContext def initialize( client: Authentication::AuthnOidc::V2::Client, + digest: Digest::SHA256, + random: SecureRandom, logger: Rails.logger ) @client = client @logger = logger + @digest = digest + @random = random end def call(authenticators:) authenticators.map do |authenticator| - { - service_id: authenticator.service_id, - type: 'authn-oidc', - name: authenticator.name, - redirect_uri: generate_redirect_url( - client: @client.new(authenticator: authenticator), - authenticator: authenticator - ) - } - end + begin + nonce = @random.hex(25) + code_verifier = @random.hex(25) + code_challenge = @digest.base64digest(code_verifier).tr("+/", "-_").tr("=", "") + { + service_id: authenticator.service_id, + type: 'authn-oidc', + name: authenticator.name, + nonce: nonce, + code_verifier: code_verifier, + redirect_uri: generate_redirect_url( + client: @client.new(authenticator: authenticator), + authenticator: authenticator, + nonce: nonce, + code_challenge: code_challenge + ) + } + rescue Errors::Authentication::OAuth::ProviderDiscoveryFailed, + Errors::Authentication::OAuth::ProviderDiscoveryTimeout + @logger.warn("Authn-OIDC '#{authenticator.service_id}' provider-uri: '#{authenticator.provider_uri}' is unreachable") + nil + end + end.compact end - def generate_redirect_url(client:, authenticator:) + def generate_redirect_url(client:, authenticator:, nonce:, code_challenge:) params = { client_id: authenticator.client_id, response_type: authenticator.response_type, scope: ERB::Util.url_encode(authenticator.scope), - state: authenticator.state, - nonce: authenticator.nonce, + nonce: nonce, + code_challenge: code_challenge, + code_challenge_method: 'S256', redirect_uri: ERB::Util.url_encode(authenticator.redirect_uri) - }.map { |key, value| "#{key}=#{value}" }.join("&") + } + formatted_params = params.map { |key, value| "#{key}=#{value}" }.join("&") - "#{client.discovery_information.authorization_endpoint}?#{params}" + "#{client.discovery_information.authorization_endpoint}?#{formatted_params}" end end end diff --git a/app/domain/authentication/handler/authentication_handler.rb b/app/domain/authentication/handler/authentication_handler.rb index b5efaf801b..3978c7e65a 100644 --- a/app/domain/authentication/handler/authentication_handler.rb +++ b/app/domain/authentication/handler/authentication_handler.rb @@ -10,15 +10,13 @@ def initialize( authn_repo: DB::Repository::AuthenticatorRepository, namespace_selector: Authentication::Util::NamespaceSelector, logger: Rails.logger, - authentication_error: LogMessages::Authentication::AuthenticationError, - pkce_support_enabled: Rails.configuration.feature_flags.enabled?(:pkce_support) + authentication_error: LogMessages::Authentication::AuthenticationError ) @role = role @resource = resource @authenticator_type = authenticator_type @logger = logger @authentication_error = authentication_error - @pkce_support_enabled = pkce_support_enabled # Dynamically load authenticator specific classes namespace = namespace_selector.select( @@ -33,15 +31,6 @@ def initialize( end def call(parameters:, request_ip:) - unless @pkce_support_enabled - required_parameters = %i[state code] - required_parameters.each do |parameter| - if !parameters.key?(parameter) || parameters[parameter].strip.empty? - raise Errors::Authentication::RequestBody::MissingRequestParam, parameter - end - end - end - # Load Authenticator policy and values (validates data stored as variables) authenticator = @authn_repo.find( type: @authenticator_type, diff --git a/app/domain/authentication/util/namespace_selector.rb b/app/domain/authentication/util/namespace_selector.rb index a877269b34..fe86aa859e 100644 --- a/app/domain/authentication/util/namespace_selector.rb +++ b/app/domain/authentication/util/namespace_selector.rb @@ -6,14 +6,10 @@ class NamespaceSelector def self.select(authenticator_type:, pkce_support_enabled: Rails.configuration.feature_flags.enabled?(:pkce_support)) case authenticator_type when 'authn-oidc' - if pkce_support_enabled - 'Authentication::AuthnOidc::PkceSupportFeature' - else - # 'V2' is a bit of a hack to handle the fact that - # the original OIDC authenticator is really a - # glorified JWT authenticator. - 'Authentication::AuthnOidc::V2' - end + # 'V2' is a bit of a hack to handle the fact that + # the original OIDC authenticator is really a + # glorified JWT authenticator. + 'Authentication::AuthnOidc::V2' else raise "'#{authenticator_type}' is not a supported authenticator type" # TODO: make this dynamic based on authenticator type. diff --git a/spec/app/db/repository/authenticator_repository_spec.rb b/spec/app/db/repository/authenticator_repository_spec.rb index ac347c3e83..2da4441232 100644 --- a/spec/app/db/repository/authenticator_repository_spec.rb +++ b/spec/app/db/repository/authenticator_repository_spec.rb @@ -3,241 +3,228 @@ require 'spec_helper' RSpec.describe('DB::Repository::AuthenticatorRepository') do - # Verify tests pass with flag enabled/disabled - [true, false].each do |pkce_flag_enabled| - context "with flag #{pkce_flag_enabled}" do - let(:data_object) do - if pkce_flag_enabled - Authentication::AuthnOidc::PkceSupportFeature::DataObjects::Authenticator - else - Authentication::AuthnOidc::V2::DataObjects::Authenticator - end - end - - let(:resource_repository) { ::Resource } + let(:data_object) { Authentication::AuthnOidc::V2::DataObjects::Authenticator } + let(:resource_repository) { ::Resource } + + let(:repo) do + DB::Repository::AuthenticatorRepository.new( + resource_repository: resource_repository, + data_object: data_object + ) + end - let(:repo) do - DB::Repository::AuthenticatorRepository.new( - resource_repository: resource_repository, - data_object: data_object, - pkce_support_enabled: pkce_flag_enabled - ) - end + let(:arguments) { %i[provider_uri client_id client_secret claim_mapping nonce state] } - let(:arguments) { %i[provider_uri client_id client_secret claim_mapping nonce state] } + describe('#find_all') do + context 'when webservice is not present' do + it { expect(repo.find_all(type: 'authn-oidc', account: 'rspec')).to eq([]) } + end - describe('#find_all') do - context 'when webservice is not present' do - it { expect(repo.find_all(type: 'authn-oidc', account: 'rspec')).to eq([]) } + context 'when webservices are presents' do + let(:services) { %w[foo bar] } + before(:each) do + services.each do |service| + ::Role.create( + role_id: "rspec:policy:conjur/authn-oidc/#{service}" + ) + # Webservice for authenticator + ::Resource.create( + resource_id: "rspec:webservice:conjur/authn-oidc/#{service}", + owner_id: "rspec:policy:conjur/authn-oidc/#{service}" + ) + # Webservice for authenticator status + ::Resource.create( + resource_id: "rspec:webservice:conjur/authn-oidc/#{service}/status", + owner_id: "rspec:policy:conjur/authn-oidc/#{service}" + ) end + end - context 'when webservices are presents' do - let(:services) { %w[foo bar] } - before(:each) do - services.each do |service| - ::Role.create( - role_id: "rspec:policy:conjur/authn-oidc/#{service}" - ) - # Webservice for authenticator - ::Resource.create( - resource_id: "rspec:webservice:conjur/authn-oidc/#{service}", - owner_id: "rspec:policy:conjur/authn-oidc/#{service}" - ) - # Webservice for authenticator status + context 'variables are not loaded' do + it { expect(repo.find_all(type: 'authn-oidc', account: 'rspec')).to eq([]) } + end + + context 'variables are loaded' do + before(:each) do + services.each do |service| + arguments.each do |variable| ::Resource.create( - resource_id: "rspec:webservice:conjur/authn-oidc/#{service}/status", + resource_id: "rspec:variable:conjur/authn-oidc/#{service}/#{variable}", owner_id: "rspec:policy:conjur/authn-oidc/#{service}" ) end end + end - context 'variables are not loaded' do - it { expect(repo.find_all(type: 'authn-oidc', account: 'rspec')).to eq([]) } - end + context 'secrets are not set' do + it { expect(repo.find_all(type: 'authn-oidc', account: 'rspec')).to eq([]) } + end - context 'variables are loaded' do - before(:each) do - services.each do |service| - arguments.each do |variable| - ::Resource.create( - resource_id: "rspec:variable:conjur/authn-oidc/#{service}/#{variable}", - owner_id: "rspec:policy:conjur/authn-oidc/#{service}" - ) - end + context 'secrets are set' do + before(:each) do + services.each do |service| + arguments.each do |variable| + ::Secret.create( + resource_id: "rspec:variable:conjur/authn-oidc/#{service}/#{variable}", + value: "#{variable}" + ) end end + end - context 'secrets are not set' do - it { expect(repo.find_all(type: 'authn-oidc', account: 'rspec')).to eq([]) } - end + let(:authenticators) { repo.find_all(type: 'authn-oidc', account: 'rspec') } - context 'secrets are set' do - before(:each) do - services.each do |service| - arguments.each do |variable| - ::Secret.create( - resource_id: "rspec:variable:conjur/authn-oidc/#{service}/#{variable}", - value: "#{variable}" - ) - end - end - end + it { expect(authenticators.length).to eq(2) } + it { expect(authenticators.first).to be_kind_of(data_object) } + it { expect(authenticators.last).to be_kind_of(data_object) } - let(:authenticators) { repo.find_all(type: 'authn-oidc', account: 'rspec') } - - it { expect(authenticators.length).to eq(2) } - it { expect(authenticators.first).to be_kind_of(data_object) } - it { expect(authenticators.last).to be_kind_of(data_object) } - - context 'filters invalid authenticators' do - before(:each) do - ::Role.create( - role_id: "rspec:policy:conjur/authn-oidc/baz" - ) - ::Resource.create( - resource_id: "rspec:webservice:conjur/authn-oidc/baz", - owner_id: "rspec:policy:conjur/authn-oidc/baz" - ) - end - - it { expect(repo.find_all(type: 'authn-oidc', account: 'rspec').length).to eq(2) } - - after(:each) do - ::Resource['rspec:webservice:conjur/authn-oidc/baz'].destroy - ::Role['rspec:policy:conjur/authn-oidc/baz'].destroy - end - end + context 'filters invalid authenticators' do + before(:each) do + ::Role.create( + role_id: "rspec:policy:conjur/authn-oidc/baz" + ) + ::Resource.create( + resource_id: "rspec:webservice:conjur/authn-oidc/baz", + owner_id: "rspec:policy:conjur/authn-oidc/baz" + ) end + it { expect(repo.find_all(type: 'authn-oidc', account: 'rspec').length).to eq(2) } + after(:each) do - services.each do |service| - arguments.each do |variable| - ::Resource["rspec:variable:conjur/authn-oidc/#{service}/#{variable}"].destroy - end - end + ::Resource['rspec:webservice:conjur/authn-oidc/baz'].destroy + ::Role['rspec:policy:conjur/authn-oidc/baz'].destroy end end + end - after(:each) do - services.each do |service| - ::Resource["rspec:webservice:conjur/authn-oidc/#{service}"].destroy - ::Role["rspec:policy:conjur/authn-oidc/#{service}"].destroy + after(:each) do + services.each do |service| + arguments.each do |variable| + ::Resource["rspec:variable:conjur/authn-oidc/#{service}/#{variable}"].destroy end end end end - describe('#find') do - context 'when webservice is not present' do - it { expect(repo.find(type: 'authn-oidc', account: 'rspec', service_id: 'abc123')).to be(nil) } + after(:each) do + services.each do |service| + ::Resource["rspec:webservice:conjur/authn-oidc/#{service}"].destroy + ::Role["rspec:policy:conjur/authn-oidc/#{service}"].destroy end + end + end + end - context 'when webservice is present' do - before(:each) do - ::Role.create( - role_id: "rspec:policy:conjur/authn-oidc/abc123" - ) + describe('#find') do + context 'when webservice is not present' do + it { expect(repo.find(type: 'authn-oidc', account: 'rspec', service_id: 'abc123')).to be(nil) } + end + + context 'when webservice is present' do + before(:each) do + ::Role.create( + role_id: "rspec:policy:conjur/authn-oidc/abc123" + ) + ::Resource.create( + resource_id: "rspec:webservice:conjur/authn-oidc/abc123", + owner_id: "rspec:policy:conjur/authn-oidc/abc123" + ) + end + + context 'when no variables are set' do + it { expect(repo.find(type: 'authn-oidc', account: 'rspec', service_id: 'abc123')).to be(nil) } + end + + context 'when all variables are present' do + before(:each) do + arguments.each do |variable| ::Resource.create( - resource_id: "rspec:webservice:conjur/authn-oidc/abc123", + resource_id: "rspec:variable:conjur/authn-oidc/abc123/#{variable}", owner_id: "rspec:policy:conjur/authn-oidc/abc123" ) end + end - context 'when no variables are set' do - it { expect(repo.find(type: 'authn-oidc', account: 'rspec', service_id: 'abc123')).to be(nil) } - end + context 'are empty' do + it { expect(repo.find(type: 'authn-oidc', account: 'rspec', service_id: 'abc123')).to be(nil) } + end - context 'when all variables are present' do - before(:each) do - arguments.each do |variable| - ::Resource.create( - resource_id: "rspec:variable:conjur/authn-oidc/abc123/#{variable}", - owner_id: "rspec:policy:conjur/authn-oidc/abc123" - ) - end + context 'are set' do + before(:each) do + arguments.each do |variable| + ::Secret.create( + resource_id: "rspec:variable:conjur/authn-oidc/abc123/#{variable}", + value: "#{variable}" + ) end + end - context 'are empty' do - it { expect(repo.find(type: 'authn-oidc', account: 'rspec', service_id: 'abc123')).to be(nil) } - end + let(:authenticator) { repo.find(type: 'authn-oidc', account: 'rspec', service_id: 'abc123') } - context 'are set' do - before(:each) do - arguments.each do |variable| - ::Secret.create( - resource_id: "rspec:variable:conjur/authn-oidc/abc123/#{variable}", - value: "#{variable}" - ) - end - end + it { expect(authenticator).to be_truthy } + it { expect(authenticator).to be_kind_of(data_object) } + it { expect(authenticator.account).to eq('rspec') } + it { expect(authenticator.service_id).to eq('abc123') } - let(:authenticator) { repo.find(type: 'authn-oidc', account: 'rspec', service_id: 'abc123') } - - it { expect(authenticator).to be_truthy } - it { expect(authenticator).to be_kind_of(data_object) } - it { expect(authenticator.account).to eq('rspec') } - it { expect(authenticator.service_id).to eq('abc123') } - - context 'custom token TTL' do - before(:each) do - ::Resource.create( - resource_id: "rspec:variable:conjur/authn-oidc/abc123/token_ttl", - owner_id: "rspec:policy:conjur/authn-oidc/abc123" - ) - ::Secret.create( - resource_id: "rspec:variable:conjur/authn-oidc/abc123/token_ttl", - value: "PT2H" - ) - end - - it { expect(authenticator.token_ttl).to eq(2.hours) } - end + context 'custom token TTL' do + before(:each) do + ::Resource.create( + resource_id: "rspec:variable:conjur/authn-oidc/abc123/token_ttl", + owner_id: "rspec:policy:conjur/authn-oidc/abc123" + ) + ::Secret.create( + resource_id: "rspec:variable:conjur/authn-oidc/abc123/token_ttl", + value: "PT2H" + ) end - after(:each) do - arguments.each do |variable| - ::Resource["rspec:variable:conjur/authn-oidc/abc123/#{variable}"].destroy - end - end + it { expect(authenticator.token_ttl).to eq(2.hours) } end + end - after(:each) do - ::Resource['rspec:webservice:conjur/authn-oidc/abc123'].destroy - ::Role['rspec:policy:conjur/authn-oidc/abc123'].destroy + after(:each) do + arguments.each do |variable| + ::Resource["rspec:variable:conjur/authn-oidc/abc123/#{variable}"].destroy end end end - describe('#exists?') do - context 'when webservice is present' do - before(:context) do - ::Role.create( - role_id: "rspec:policy:conjur/authn-oidc/abc123" - ) - ::Resource.create( - resource_id: "rspec:webservice:conjur/authn-oidc/abc123", - owner_id: "rspec:policy:conjur/authn-oidc/abc123" - ) - end + after(:each) do + ::Resource['rspec:webservice:conjur/authn-oidc/abc123'].destroy + ::Role['rspec:policy:conjur/authn-oidc/abc123'].destroy + end + end + end - it { expect(repo.exists?(type: 'authn-oidc', account: 'rspec', service_id: 'abc123')).to be_truthy } - it { expect(repo.exists?(type: nil, account: 'rspec', service_id: 'abc123')).to be_falsey } - it { expect(repo.exists?(type: 'authn-oidc', account: nil, service_id: 'abc123')).to be_falsey } - it { expect(repo.exists?(type: 'authn-oidc', account: 'rspec', service_id: nil)).to be_falsey } + describe('#exists?') do + context 'when webservice is present' do + before(:context) do + ::Role.create( + role_id: "rspec:policy:conjur/authn-oidc/abc123" + ) + ::Resource.create( + resource_id: "rspec:webservice:conjur/authn-oidc/abc123", + owner_id: "rspec:policy:conjur/authn-oidc/abc123" + ) + end - after(:context) do - ::Resource['rspec:webservice:conjur/authn-oidc/abc123'].destroy - ::Role['rspec:policy:conjur/authn-oidc/abc123'].destroy - end - end + it { expect(repo.exists?(type: 'authn-oidc', account: 'rspec', service_id: 'abc123')).to be_truthy } + it { expect(repo.exists?(type: nil, account: 'rspec', service_id: 'abc123')).to be_falsey } + it { expect(repo.exists?(type: 'authn-oidc', account: nil, service_id: 'abc123')).to be_falsey } + it { expect(repo.exists?(type: 'authn-oidc', account: 'rspec', service_id: nil)).to be_falsey } - context 'when webservice is not present' do - it { expect(repo.exists?(type: 'authn-oidc', account: 'rspec', service_id: 'abc123')).to be_falsey } - it { expect(repo.exists?(type: nil, account: 'rspec', service_id: 'abc123')).to be_falsey } - it { expect(repo.exists?(type: 'authn-oidc', account: nil, service_id: 'abc123')).to be_falsey } - it { expect(repo.exists?(type: 'authn-oidc', account: 'rspec', service_id: nil)).to be_falsey } - end + after(:context) do + ::Resource['rspec:webservice:conjur/authn-oidc/abc123'].destroy + ::Role['rspec:policy:conjur/authn-oidc/abc123'].destroy end end + + context 'when webservice is not present' do + it { expect(repo.exists?(type: 'authn-oidc', account: 'rspec', service_id: 'abc123')).to be_falsey } + it { expect(repo.exists?(type: nil, account: 'rspec', service_id: 'abc123')).to be_falsey } + it { expect(repo.exists?(type: 'authn-oidc', account: nil, service_id: 'abc123')).to be_falsey } + it { expect(repo.exists?(type: 'authn-oidc', account: 'rspec', service_id: nil)).to be_falsey } + end end end diff --git a/spec/app/domain/authentication/authn-oidc/pkce_support_feature/client_spec.rb b/spec/app/domain/authentication/authn-oidc/pkce_support_feature/client_spec.rb deleted file mode 100644 index 4b3d528cfb..0000000000 --- a/spec/app/domain/authentication/authn-oidc/pkce_support_feature/client_spec.rb +++ /dev/null @@ -1,195 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe(Authentication::AuthnOidc::PkceSupportFeature::Client) do - let(:authenticator) do - Authentication::AuthnOidc::PkceSupportFeature::DataObjects::Authenticator.new( - provider_uri: 'https://dev-92899796.okta.com/oauth2/default', - redirect_uri: 'http://localhost:3000/authn-oidc/okta-2/cucumber/authenticate', - client_id: '0oa3w3xig6rHiu9yT5d7', - client_secret: 'e349BMTTIpLO-rPuPqLLkLyH_pO-loUzhIVJCrHj', - claim_mapping: 'foo', - account: 'bar', - service_id: 'baz' - ) - end - - let(:client) do - VCR.use_cassette("authenticators/authn-oidc/pkce_support_feature/client_load") do - client = Authentication::AuthnOidc::PkceSupportFeature::Client.new( - authenticator: authenticator - ) - # The call `oidc_client` queries the OIDC endpoint. As such, - # we need to wrap this in a VCR call. Calling this before - # returning the client to allow this call to be more effectively - # mocked. - client.oidc_client - client - end - end - - describe '.callback' do - context 'when credentials are valid' do - it 'returns a valid JWT token', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_callback-valid_oidc_credentials' do - # Because JWT tokens have an expiration timeframe, we need to hold - # time constant after caching the request. - travel_to(Time.parse("2022-09-30 17:02:17 +0000")) do - token = client.callback( - code: '-QGREc_SONbbJIKdbpyYudA13c9PZlgqdxowkf45LOw', - code_verifier: 'c1de7f1251849accd99d4839d79a637561b1181b909ed7dc1d', - nonce: '7efcbba36a9b96fdb5285a159665c3d382abd8b6b3288fcc8d' - ) - expect(token).to be_a_kind_of(OpenIDConnect::ResponseObject::IdToken) - expect(token.raw_attributes['nonce']).to eq('7efcbba36a9b96fdb5285a159665c3d382abd8b6b3288fcc8d') - expect(token.raw_attributes['preferred_username']).to eq('test.user3@mycompany.com') - expect(token.aud).to eq('0oa3w3xig6rHiu9yT5d7') - end - end - end - - context 'when code verifier does not match' do - it 'raises an error', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_callback-invalid_code_verifier' do - travel_to(Time.parse("2022-10-17 17:23:30 +0000")) do - expect do - client.callback( - code: 'GV48_SF4a19ghvBhVbbSG3Lr8BuFl8PhWVPZSbokV2o', - code_verifier: 'bad-code-verifier', - nonce: '3e6bd5235e4692b37ca1f04cb01b6e0cb177aa20dcef19e89f' - ) - end.to raise_error( - Errors::Authentication::AuthnOidc::TokenRetrievalFailed, - "CONJ00133E Access Token retrieval failure: 'PKCE verification failed'" - ) - end - end - end - - context 'when nonce does not match' do - it 'raises an error', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_callback-valid_oidc_credentials' do - travel_to(Time.parse("2022-09-30 17:02:17 +0000")) do - expect do - client.callback( - code: '-QGREc_SONbbJIKdbpyYudA13c9PZlgqdxowkf45LOw', - code_verifier: 'c1de7f1251849accd99d4839d79a637561b1181b909ed7dc1d', - nonce: 'bad-nonce' - ) - end.to raise_error( - Errors::Authentication::AuthnOidc::TokenVerificationFailed, - "CONJ00128E JWT Token validation failed: 'Provided nonce does not match the nonce in the JWT'" - ) - end - end - end - - context 'when JWT has expired' do - it 'raises an error', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_callback-valid_oidc_credentials' do - travel_to(Time.parse("2022-10-01 17:02:17 +0000")) do - expect do - client.callback( - code: '-QGREc_SONbbJIKdbpyYudA13c9PZlgqdxowkf45LOw', - code_verifier: 'c1de7f1251849accd99d4839d79a637561b1181b909ed7dc1d', - nonce: '7efcbba36a9b96fdb5285a159665c3d382abd8b6b3288fcc8d' - ) - end.to raise_error( - Errors::Authentication::AuthnOidc::TokenVerificationFailed, - "CONJ00128E JWT Token validation failed: 'JWT has expired'" - ) - end - end - end - - context 'when code has previously been used' do - it 'raise an exception', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_callback-used_code-valid_oidc_credentials' do - expect do - client.callback( - code: '-QGREc_SONbbJIKdbpyYudA13c9PZlgqdxowkf45LOw', - code_verifier: 'c1de7f1251849accd99d4839d79a637561b1181b909ed7dc1d', - nonce: '7efcbba36a9b96fdb5285a159665c3d382abd8b6b3288fcc8d' - ) - end.to raise_error( - Errors::Authentication::AuthnOidc::TokenRetrievalFailed, - "CONJ00133E Access Token retrieval failure: 'Authorization code is invalid or has expired'" - ) - end - end - - context 'when code has expired', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_callback-expired_code-valid_oidc_credentials' do - it 'raise an exception' do - expect do - client.callback( - code: 'SNSPeiQJ0-D6nUHTg-Ht9ZoDxIaaWBB80pnYuXY2VxU', - code_verifier: 'c1de7f1251849accd99d4839d79a637561b1181b909ed7dc1d', - nonce: '7efcbba36a9b96fdb5285a159665c3d382abd8b6b3288fcc8d' - ) - end.to raise_error( - Errors::Authentication::AuthnOidc::TokenRetrievalFailed, - "CONJ00133E Access Token retrieval failure: 'Authorization code is invalid or has expired'" - ) - end - end - end - - describe '.oidc_client' do - context 'when credentials are valid' do - it 'returns a valid oidc client', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_initialization' do - oidc_client = client.oidc_client - - expect(oidc_client).to be_a_kind_of(OpenIDConnect::Client) - expect(oidc_client.identifier).to eq('0oa3w3xig6rHiu9yT5d7') - expect(oidc_client.secret).to eq('e349BMTTIpLO-rPuPqLLkLyH_pO-loUzhIVJCrHj') - expect(oidc_client.redirect_uri).to eq('http://localhost:3000/authn-oidc/okta-2/cucumber/authenticate') - expect(oidc_client.scheme).to eq('https') - expect(oidc_client.host).to eq('dev-92899796.okta.com') - expect(oidc_client.port).to eq(443) - expect(oidc_client.authorization_endpoint).to eq('/oauth2/default/v1/authorize') - expect(oidc_client.token_endpoint).to eq('/oauth2/default/v1/token') - expect(oidc_client.userinfo_endpoint).to eq('/oauth2/default/v1/userinfo') - end - end - end - - describe '.discovery_information', vcr: 'authenticators/authn-oidc/pkce_support_feature/discovery_endpoint-valid_oidc_credentials' do - context 'when credentials are valid' do - it 'endpoint returns valid data' do - discovery_information = client.discovery_information(invalidate: true) - - expect(discovery_information.authorization_endpoint).to eq( - 'https://dev-92899796.okta.com/oauth2/default/v1/authorize' - ) - expect(discovery_information.token_endpoint).to eq( - 'https://dev-92899796.okta.com/oauth2/default/v1/token' - ) - expect(discovery_information.userinfo_endpoint).to eq( - 'https://dev-92899796.okta.com/oauth2/default/v1/userinfo' - ) - expect(discovery_information.jwks_uri).to eq( - 'https://dev-92899796.okta.com/oauth2/default/v1/keys' - ) - expect(discovery_information.end_session_endpoint).to eq( - 'https://dev-92899796.okta.com/oauth2/default/v1/logout' - ) - end - end - - context 'when provider URI is invalid', vcr: 'authenticators/authn-oidc/pkce_support_feature/discovery_endpoint-invalid_oidc_provider' do - it 'returns an timeout error' do - client = Authentication::AuthnOidc::PkceSupportFeature::Client.new( - authenticator: Authentication::AuthnOidc::PkceSupportFeature::DataObjects::Authenticator.new( - provider_uri: 'https://foo.bar1234321.com', - redirect_uri: 'http://localhost:3000/authn-oidc/okta-2/cucumber/authenticate', - client_id: '0oa3w3xig6rHiu9yT5d7', - client_secret: 'e349BMTTIpLO-rPuPqLLkLyH_pO-loUzhIVJCrHj', - claim_mapping: 'foo', - account: 'bar', - service_id: 'baz' - ) - ) - - expect{client.discovery_information(invalidate: true)}.to raise_error( - Errors::Authentication::OAuth::ProviderDiscoveryFailed - ) - end - end - end -end diff --git a/spec/app/domain/authentication/authn-oidc/pkce_support_feature/data_objects/authenticator.rb b/spec/app/domain/authentication/authn-oidc/pkce_support_feature/data_objects/authenticator.rb deleted file mode 100644 index 47043a57fa..0000000000 --- a/spec/app/domain/authentication/authn-oidc/pkce_support_feature/data_objects/authenticator.rb +++ /dev/null @@ -1,92 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe(Authentication::AuthnOidc::PkceSupportFeature::DataObjects::Authenticator) do - let(:default_args) do - { - provider_uri: 'https://foo.bar.com/baz', - client_id: 'client-id-123', - client_secret: 'client-secret-123', - claim_mapping: 'email', - account: 'default', - service_id: 'my-authenticator' - } - end - let(:args) { default_args } - - let(:authenticator) { described_class.new(**args) } - - describe '.scope' do - context 'with default initializer' do - it { expect(authenticator.scope).to eq('openid email profile') } - end - - context 'when initialized with a string argument' do - let(:args) { default_args.merge({ provider_scope: 'foo' }) } - it { expect(authenticator.scope).to eq('openid email profile foo') } - end - - context 'when initialized with a non-string argument' do - let(:args) { default_args.merge({ provider_scope: 1 }) } - it { expect(authenticator.scope).to eq('openid email profile 1') } - end - - context 'when initialized with a duplicated argument' do - let(:args) { default_args.merge({ provider_scope: 'profile' }) } - it { expect(authenticator.scope).to eq('openid email profile') } - end - - context 'when initialized with an array argument' do - context 'single value array' do - let(:args) { default_args.merge({ provider_scope: 'foo' }) } - it { expect(authenticator.scope).to eq('openid email profile foo') } - end - - context 'multi-value array' do - let(:args) { default_args.merge({ provider_scope: 'foo bar' }) } - it { expect(authenticator.scope).to eq('openid email profile foo bar') } - end - end - end - - describe '.name' do - context 'when name is missing' do - it { expect(authenticator.name).to eq('My Authenticator') } - end - context 'when name is present' do - let(:args) { default_args.merge(name: 'foo') } - it { expect(authenticator.name).to eq('foo') } - end - end - - describe '.resource_id' do - context 'correctly renders' do - it { expect(authenticator.resource_id).to eq('default:webservice:conjur/authn-oidc/my-authenticator') } - end - end - - describe '.response_type' do - context 'with default initializer' do - it { expect(authenticator.response_type).to eq('code') } - end - end - - describe '.token_ttl' do - context 'with default initializer' do - it { expect(authenticator.token_ttl).to eq(8.minutes) } - end - - context 'when initialized with a valid duration' do - let (:args) { default_args.merge({ token_ttl: 'PT1H'}) } - it { expect(authenticator.token_ttl).to eq(1.hour)} - end - - context 'when initialized with an invalid duration' do - let (:args) { default_args.merge({ token_ttl: 'PTinvalidH' }) } - it { expect { - authenticator.token_ttl - }.to raise_error(Errors::Authentication::DataObjects::InvalidTokenTTL) } - end - end -end diff --git a/spec/app/domain/authentication/authn-oidc/pkce_support_feature/resolve_identity_spec.rb b/spec/app/domain/authentication/authn-oidc/pkce_support_feature/resolve_identity_spec.rb deleted file mode 100644 index 83943542fb..0000000000 --- a/spec/app/domain/authentication/authn-oidc/pkce_support_feature/resolve_identity_spec.rb +++ /dev/null @@ -1,96 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe(' Authentication::AuthnOidc::PkceSupportFeature::ResolveIdentity') do - let(:resolve_identity) do - Authentication::AuthnOidc::PkceSupportFeature::ResolveIdentity.new - end - - describe('#call') do - let(:valid_role) do - instance_double(::Role).tap do |double| - allow(double).to receive(:id).and_return('rspec:user:alice') - allow(double).to receive(:resource?).and_return(true) - end - end - - context 'when identity matches a role ID' do - it 'returns the matching role' do - expect( - resolve_identity.call( - account: 'rspec', - identity: 'alice', - allowed_roles: [ valid_role ] - ).id - ).to eq('rspec:user:alice') - end - - context 'when it includes roles without resources' do - it 'returns the matching role' do - expect( - resolve_identity.call( - account: 'rspec', - identity: 'alice', - allowed_roles: [ - instance_double(::Role).tap do |double| - allow(double).to receive(:id).and_return('rspec:user:alice') - allow(double).to receive(:resource?).and_return(false) - end, - valid_role - ] - ).id - ).to eq('rspec:user:alice') - end - end - - context 'when the accounts are different' do - it 'returns the matching role' do - expect( - resolve_identity.call( - account: 'rspec', - identity: 'alice', - allowed_roles: [ - instance_double(::Role).tap do |double| - allow(double).to receive(:id).and_return('foo:user:alice') - allow(double).to receive(:resource?).and_return(true) - end, - valid_role - ] - ).id - ).to eq('rspec:user:alice') - end - end - end - - context 'when the provided identity does not match a role or annotation' do - it 'raises the error RoleNotFound' do - expect { - resolve_identity.call( - account: 'rspec', - identity: 'alice', - allowed_roles: [ - instance_double(::Role).tap do |double| - allow(double).to receive(:id).and_return('rspec:user:bob') - allow(double).to receive(:resource?).and_return(true) - end, - instance_double(::Role).tap do |double| - allow(double).to receive(:id).and_return('rspec:user:chad') - allow(double).to receive(:resource?).and_return(true) - allow(double).to receive(:resource).and_return( - instance_double(::Resource).tap do |resource| - allow(resource).to receive(:annotation).with('authn-oidc/identity').and_return('chad.example') - end - ) - end - ] - ) - }.to raise_error( - Errors::Authentication::Security::RoleNotFound, - /CONJ00007E 'alice' not found/ - ) - end - end - - end -end diff --git a/spec/app/domain/authentication/authn-oidc/pkce_support_feature/strategy_rspec.rb b/spec/app/domain/authentication/authn-oidc/pkce_support_feature/strategy_rspec.rb deleted file mode 100644 index 62040a41bf..0000000000 --- a/spec/app/domain/authentication/authn-oidc/pkce_support_feature/strategy_rspec.rb +++ /dev/null @@ -1,80 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe(' Authentication::AuthnOidc::PkceSupportFeature::Strategy') do - - let(:jwt) { double(raw_attributes: { claim_mapping: "alice" }) } - - let(:authenticator) do - Authentication::AuthnOidc::PkceSupportFeature::DataObjects::Authenticator.new( - account: "cucumber", - service_id: "foo", - redirect_uri: "http://conjur/authn-oidc/cucumber/authenticate", - provider_uri: "http://test", - name: "foo", - client_id: "ConjurClient", - client_secret: 'client_secret', - claim_mapping: mapping - ) - end - - let(:client) do - class_double(::Authentication::AuthnOidc::PkceSupportFeature::Client).tap do |double| - allow(double).to receive(:new).and_return(current_client) - end - end - - let(:current_client) do - instance_double(::Authentication::AuthnOidc::PkceSupportFeature::Client).tap do |double| - allow(double).to receive(:callback).and_return(jwt) - end - end - - let(:strategy) do - Authentication::AuthnOidc::PkceSupportFeature::Strategy.new( - authenticator: authenticator, - client: client - ) - end - - describe('#callback') do - context 'when a role_id matches the identity exist' do - let(:mapping) { "claim_mapping" } - it 'returns the role' do - expect(strategy.callback(nonce: 'nonce', code: 'code', code_verifier: 'foo')) - .to eq('alice') - end - end - - context "when the claiming matching in the token doesn't match the jwt" do - let(:mapping) { 'wrong_mapping' } - it 'raises a `IdTokenClaimNotFoundOrEmpty` error' do - expect { strategy.callback(code: 'code', nonce: 'nonce', code_verifier: 'foo') } - .to raise_error(Errors::Authentication::AuthnOidc::IdTokenClaimNotFoundOrEmpty) - end - end - - context 'when required parameters are missing' do - let(:mapping) { '' } - context 'when code is missing' do - it 'raises a `MissingRequestParam` error' do - expect { strategy.callback(nonce: 'nonce', code_verifier: 'foo') } - .to raise_error(Errors::Authentication::RequestBody::MissingRequestParam) - end - end - context 'when nonce is missing' do - it 'raises a `MissingRequestParam` error' do - expect { strategy.callback(code: 'code', code_verifier: 'foo') } - .to raise_error(Errors::Authentication::RequestBody::MissingRequestParam) - end - end - context 'when code_verifier is missing' do - it 'raises a `MissingRequestParam` error' do - expect { strategy.callback(nonce: 'nonce', code: 'code') } - .to raise_error(Errors::Authentication::RequestBody::MissingRequestParam) - end - end - end - end -end diff --git a/spec/app/domain/authentication/authn-oidc/pkce_support_feature/views/providor_context_spec.rb b/spec/app/domain/authentication/authn-oidc/pkce_support_feature/views/providor_context_spec.rb deleted file mode 100644 index 056055561f..0000000000 --- a/spec/app/domain/authentication/authn-oidc/pkce_support_feature/views/providor_context_spec.rb +++ /dev/null @@ -1,126 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe('Authentication::AuthnOidc::PkceSupportFeature::Views::ProviderContext') do - let(:client) do - class_double(::Authentication::AuthnOidc::PkceSupportFeature::Client).tap do |double| - allow(double).to receive(:new).and_return(current_client) - end - end - - let(:current_client) do - instance_double(::Authentication::AuthnOidc::PkceSupportFeature::Client).tap do |double| - allow(double).to receive(:discovery_information).and_return(endpoint) - end - end - - let(:endpoint) { double(authorization_endpoint: '"http://test"') } - - let(:foo) do - Authentication::AuthnOidc::PkceSupportFeature::DataObjects::Authenticator.new( - account: "cucumber", - service_id: "foo", - redirect_uri: "http://conjur/authn-oidc/cucumber/authenticate", - provider_uri: "http://foo", - name: "foo", - client_id: "ConjurClient", - client_secret: 'client_secret', - claim_mapping: 'claim_mapping' - ) - end - - let(:bar) do - Authentication::AuthnOidc::PkceSupportFeature::DataObjects::Authenticator.new( - account: "cucumber", - service_id: "bar", - provider_uri: "http://bar", - redirect_uri: "http://conjur/authn-oidc/cucumber/authenticate", - name: "bar", - client_id: "ConjurClient", - client_secret: 'client_secret', - claim_mapping: 'claim_mapping' - ) - end - - let(:authenticators) {[foo, bar]} - - let(:response) do - [ - { - name: "foo", - redirect_uri: "\"http://test\"?client_id=ConjurClient&response_type=code&scope=openid%20email%20profile" \ - "&nonce=random-string&code_challenge=random-sha&code_challenge_method=S256&redirect_uri=http%3A%2F%2Fconjur%2Fauthn-oidc%2Fcucumber%2Fauthenticate", - service_id: "foo", - type: "authn-oidc", - nonce: 'random-string', - code_verifier: 'random-string' - }, { - name: "bar", - redirect_uri: "\"http://test\"?client_id=ConjurClient&response_type=code&scope=openid%20email%20profile" \ - "&nonce=random-string&code_challenge=random-sha&code_challenge_method=S256&redirect_uri=http%3A%2F%2Fconjur%2Fauthn-oidc%2Fcucumber%2Fauthenticate", - service_id: "bar", - type: "authn-oidc", - nonce: 'random-string', - code_verifier: 'random-string' - } - ] - end - - let(:provider_context) do - ::Authentication::AuthnOidc::PkceSupportFeature::Views::ProviderContext.new( - client: client, - random: random, - digest: digest, - logger: logger - ) - end - - let(:log_output) { StringIO.new } - let(:logger) { Logger.new(log_output) } - - let(:digest) do - class_double(Digest::SHA256).tap do |double| - allow(double).to receive(:base64digest).and_return('random-sha') - end - end - - let(:random) do - class_double(SecureRandom).tap do |double| - allow(double).to receive(:hex).and_return('random-string') - end - end - - describe('#call') do - context 'when provider context is given multiple authenticators' do - it 'returns the providers object with the redirect urls' do - expect(provider_context.call(authenticators: authenticators)) - .to eq(response) - end - end - - context 'when provider context is given no authenticators ' do - it 'returns an empty array' do - expect(provider_context.call(authenticators: [])) - .to eq([]) - end - end - - context 'when authenticator discovery endpoint is unreachable' do - let(:current_client) do - instance_double(::Authentication::AuthnOidc::PkceSupportFeature::Client).tap do |double| - allow(double).to receive(:discovery_information).and_raise( - Errors::Authentication::OAuth::ProviderDiscoveryFailed - ) - end - end - it 'does not cause an exception' do - expect(provider_context.call(authenticators: authenticators)).to eq([]) - expect(log_output.string).to include('WARN') - %w[foo bar].each do |authenticator| - expect(log_output.string).to include("Authn-OIDC '#{authenticator}' provider-uri: 'http://#{authenticator}' is unreachable") - end - end - end - end -end diff --git a/spec/app/domain/authentication/authn-oidc/v2/client_spec.rb b/spec/app/domain/authentication/authn-oidc/v2/client_spec.rb index 8074da1d01..07bc70d623 100644 --- a/spec/app/domain/authentication/authn-oidc/v2/client_spec.rb +++ b/spec/app/domain/authentication/authn-oidc/v2/client_spec.rb @@ -10,98 +10,129 @@ client_id: '0oa3w3xig6rHiu9yT5d7', client_secret: 'e349BMTTIpLO-rPuPqLLkLyH_pO-loUzhIVJCrHj', claim_mapping: 'foo', - nonce: '1656b4264b60af659fce', - state: 'state', account: 'bar', service_id: 'baz' ) end let(:client) do - Authentication::AuthnOidc::V2::Client.new( - authenticator: authenticator - ) + VCR.use_cassette("authenticators/authn-oidc/pkce_support_feature/client_load") do + client = Authentication::AuthnOidc::V2::Client.new( + authenticator: authenticator + ) + # The call `oidc_client` queries the OIDC endpoint. As such, + # we need to wrap this in a VCR call. Calling this before + # returning the client to allow this call to be more effectively + # mocked. + client.oidc_client + client + end end describe '.callback' do context 'when credentials are valid' do - it 'returns a valid JWT token', vcr: 'authenticators/authn-oidc/v2/client_callback-valid_oidc_credentials' do + it 'returns a valid JWT token', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_callback-valid_oidc_credentials' do # Because JWT tokens have an expiration timeframe, we need to hold # time constant after caching the request. - travel_to(Time.parse("2022-06-30 16:42:17 +0000")) do + travel_to(Time.parse("2022-09-30 17:02:17 +0000")) do token = client.callback( - code: 'qdDm7On1dEEzNmMlk2bF7IcOF8gCgfvgMCMXXXDlYEE' + code: '-QGREc_SONbbJIKdbpyYudA13c9PZlgqdxowkf45LOw', + code_verifier: 'c1de7f1251849accd99d4839d79a637561b1181b909ed7dc1d', + nonce: '7efcbba36a9b96fdb5285a159665c3d382abd8b6b3288fcc8d' ) expect(token).to be_a_kind_of(OpenIDConnect::ResponseObject::IdToken) - expect(token.raw_attributes['nonce']).to eq('1656b4264b60af659fce') + expect(token.raw_attributes['nonce']).to eq('7efcbba36a9b96fdb5285a159665c3d382abd8b6b3288fcc8d') expect(token.raw_attributes['preferred_username']).to eq('test.user3@mycompany.com') expect(token.aud).to eq('0oa3w3xig6rHiu9yT5d7') end end end + context 'when code verifier does not match' do + it 'raises an error', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_callback-invalid_code_verifier' do + travel_to(Time.parse("2022-10-17 17:23:30 +0000")) do + expect do + client.callback( + code: 'GV48_SF4a19ghvBhVbbSG3Lr8BuFl8PhWVPZSbokV2o', + code_verifier: 'bad-code-verifier', + nonce: '3e6bd5235e4692b37ca1f04cb01b6e0cb177aa20dcef19e89f' + ) + end.to raise_error( + Errors::Authentication::AuthnOidc::TokenRetrievalFailed, + "CONJ00133E Access Token retrieval failure: 'PKCE verification failed'" + ) + end + end + end + + context 'when nonce does not match' do + it 'raises an error', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_callback-valid_oidc_credentials' do + travel_to(Time.parse("2022-09-30 17:02:17 +0000")) do + expect do + client.callback( + code: '-QGREc_SONbbJIKdbpyYudA13c9PZlgqdxowkf45LOw', + code_verifier: 'c1de7f1251849accd99d4839d79a637561b1181b909ed7dc1d', + nonce: 'bad-nonce' + ) + end.to raise_error( + Errors::Authentication::AuthnOidc::TokenVerificationFailed, + "CONJ00128E JWT Token validation failed: 'Provided nonce does not match the nonce in the JWT'" + ) + end + end + end + context 'when JWT has expired' do - it 'raises an error', vcr: 'authenticators/authn-oidc/v2/client_callback-valid_oidc_credentials' do - travel_to(Time.parse("2022-06-30 20:42:17 +0000")) do + it 'raises an error', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_callback-valid_oidc_credentials' do + travel_to(Time.parse("2022-10-01 17:02:17 +0000")) do expect do client.callback( - code: 'qdDm7On1dEEzNmMlk2bF7IcOF8gCgfvgMCMXXXDlYEE' + code: '-QGREc_SONbbJIKdbpyYudA13c9PZlgqdxowkf45LOw', + code_verifier: 'c1de7f1251849accd99d4839d79a637561b1181b909ed7dc1d', + nonce: '7efcbba36a9b96fdb5285a159665c3d382abd8b6b3288fcc8d' ) end.to raise_error( - OpenIDConnect::ResponseObject::IdToken::ExpiredToken, - 'Invalid ID token: Expired token' + Errors::Authentication::AuthnOidc::TokenVerificationFailed, + "CONJ00128E JWT Token validation failed: 'JWT has expired'" ) end end end context 'when code has previously been used' do - it 'raise an exception', vcr: 'authenticators/authn-oidc/v2/client_callback-used_code-valid_oidc_credentials' do + it 'raise an exception', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_callback-used_code-valid_oidc_credentials' do expect do client.callback( - code: '7wKEGhsN9UEL5MG9EfDJ8KWMToKINzvV29uyPsQZYpo' + code: '-QGREc_SONbbJIKdbpyYudA13c9PZlgqdxowkf45LOw', + code_verifier: 'c1de7f1251849accd99d4839d79a637561b1181b909ed7dc1d', + nonce: '7efcbba36a9b96fdb5285a159665c3d382abd8b6b3288fcc8d' ) end.to raise_error( - Rack::OAuth2::Client::Error, - 'invalid_grant :: The authorization code is invalid or has expired.' + Errors::Authentication::AuthnOidc::TokenRetrievalFailed, + "CONJ00133E Access Token retrieval failure: 'Authorization code is invalid or has expired'" ) end end - context 'when code has expired', vcr: 'authenticators/authn-oidc/v2/client_callback-expired_code-valid_oidc_credentials' do + context 'when code has expired', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_callback-expired_code-valid_oidc_credentials' do it 'raise an exception' do expect do client.callback( - code: 'SNSPeiQJ0-D6nUHTg-Ht9ZoDxIaaWBB80pnYuXY2VxU' + code: 'SNSPeiQJ0-D6nUHTg-Ht9ZoDxIaaWBB80pnYuXY2VxU', + code_verifier: 'c1de7f1251849accd99d4839d79a637561b1181b909ed7dc1d', + nonce: '7efcbba36a9b96fdb5285a159665c3d382abd8b6b3288fcc8d' ) end.to raise_error( - Rack::OAuth2::Client::Error, - 'invalid_grant :: The authorization code is invalid or has expired.' + Errors::Authentication::AuthnOidc::TokenRetrievalFailed, + "CONJ00133E Access Token retrieval failure: 'Authorization code is invalid or has expired'" ) end end - - context 'when code is invalid' do - context 'raise an error when' do - it 'code is nil' do - expect { client.callback(code: nil) }.to raise_error( - Errors::Authentication::RequestBody::MissingRequestParam, - "CONJ00009E Field 'code' is missing or empty in request body" - ) - end - it 'code is an empty string' do - expect { client.callback(code: '') }.to raise_error( - Errors::Authentication::RequestBody::MissingRequestParam, - "CONJ00009E Field 'code' is missing or empty in request body" - ) - end - end - end end describe '.oidc_client' do context 'when credentials are valid' do - it 'returns a valid oidc client', vcr: 'authenticators/authn-oidc/v2/client_initialization' do + it 'returns a valid oidc client', vcr: 'authenticators/authn-oidc/pkce_support_feature/client_initialization' do oidc_client = client.oidc_client expect(oidc_client).to be_a_kind_of(OpenIDConnect::Client) @@ -118,7 +149,7 @@ end end - describe '.discovery_information', vcr: 'authenticators/authn-oidc/v2/discovery_endpoint-valid_oidc_credentials' do + describe '.discovery_information', vcr: 'authenticators/authn-oidc/pkce_support_feature/discovery_endpoint-valid_oidc_credentials' do context 'when credentials are valid' do it 'endpoint returns valid data' do discovery_information = client.discovery_information(invalidate: true) @@ -141,7 +172,7 @@ end end - context 'when provider URI is invalid', vcr: 'authenticators/authn-oidc/v2/discovery_endpoint-invalid_oidc_provider' do + context 'when provider URI is invalid', vcr: 'authenticators/authn-oidc/pkce_support_feature/discovery_endpoint-invalid_oidc_provider' do it 'returns an timeout error' do client = Authentication::AuthnOidc::V2::Client.new( authenticator: Authentication::AuthnOidc::V2::DataObjects::Authenticator.new( @@ -150,8 +181,6 @@ client_id: '0oa3w3xig6rHiu9yT5d7', client_secret: 'e349BMTTIpLO-rPuPqLLkLyH_pO-loUzhIVJCrHj', claim_mapping: 'foo', - nonce: '1656b4264b60af659fce', - state: 'state', account: 'bar', service_id: 'baz' ) diff --git a/spec/app/domain/authentication/authn-oidc/v2/data_objects/authenticator.rb b/spec/app/domain/authentication/authn-oidc/v2/data_objects/authenticator.rb index 01e239b3d1..a34b491f62 100644 --- a/spec/app/domain/authentication/authn-oidc/v2/data_objects/authenticator.rb +++ b/spec/app/domain/authentication/authn-oidc/v2/data_objects/authenticator.rb @@ -9,8 +9,6 @@ client_id: 'client-id-123', client_secret: 'client-secret-123', claim_mapping: 'email', - nonce: 'nonce456', - state: 'state123', account: 'default', service_id: 'my-authenticator' } @@ -34,14 +32,19 @@ it { expect(authenticator.scope).to eq('openid email profile 1') } end + context 'when initialized with a duplicated argument' do + let(:args) { default_args.merge({ provider_scope: 'profile' }) } + it { expect(authenticator.scope).to eq('openid email profile') } + end + context 'when initialized with an array argument' do context 'single value array' do - let(:args) { default_args.merge({ provider_scope: ['foo'] }) } + let(:args) { default_args.merge({ provider_scope: 'foo' }) } it { expect(authenticator.scope).to eq('openid email profile foo') } end context 'multi-value array' do - let(:args) { default_args.merge({ provider_scope: ['foo', 'bar'] }) } + let(:args) { default_args.merge({ provider_scope: 'foo bar' }) } it { expect(authenticator.scope).to eq('openid email profile foo bar') } end end diff --git a/spec/app/domain/authentication/authn-oidc/v2/strategy_rspec.rb b/spec/app/domain/authentication/authn-oidc/v2/strategy_rspec.rb index 7ce0cb9d48..557ab8ebb2 100644 --- a/spec/app/domain/authentication/authn-oidc/v2/strategy_rspec.rb +++ b/spec/app/domain/authentication/authn-oidc/v2/strategy_rspec.rb @@ -3,7 +3,6 @@ require 'spec_helper' RSpec.describe(' Authentication::AuthnOidc::V2::Strategy') do - let(:jwt) { double(raw_attributes: { claim_mapping: "alice" }) } let(:authenticator) do @@ -13,18 +12,15 @@ redirect_uri: "http://conjur/authn-oidc/cucumber/authenticate", provider_uri: "http://test", name: "foo", - state: 'foostate', client_id: "ConjurClient", client_secret: 'client_secret', - claim_mapping: mapping, - nonce: 'secret' + claim_mapping: mapping ) end let(:client) do class_double(::Authentication::AuthnOidc::V2::Client).tap do |double| - allow(double).to receive(:new).and_return( - current_client) + allow(double).to receive(:new).and_return(current_client) end end @@ -45,24 +41,32 @@ context 'when a role_id matches the identity exist' do let(:mapping) { "claim_mapping" } it 'returns the role' do - expect(strategy.callback({ state: "foostate", code: "code" })) - .to eq("alice") + expect(strategy.callback(nonce: 'nonce', code: 'code', code_verifier: 'foo')) + .to eq('alice') end end - context 'When the authn state doesnt match the arguments' do - let(:mapping) { "claim_mapping" } - it 'raises an error' do - expect { strategy.callback({ state: "barstate", code: "code" }) } - .to raise_error(Errors::Authentication::AuthnOidc::StateMismatch) + context "when the claiming matching in the token doesn't match the jwt" do + let(:mapping) { 'wrong_mapping' } + it 'raises a `IdTokenClaimNotFoundOrEmpty` error' do + expect { strategy.callback(code: 'code', nonce: 'nonce', code_verifier: 'foo') } + .to raise_error(Errors::Authentication::AuthnOidc::IdTokenClaimNotFoundOrEmpty) end end - context 'When the claiming matching in the token doesnt match the jwt' do - let(:mapping) { "wrong_mapping" } - it 'raises an error' do - expect { strategy.callback({ state: "foostate", code: "code" }) } - .to raise_error(Errors::Authentication::AuthnOidc::IdTokenClaimNotFoundOrEmpty) + context 'when required parameters are missing' do + let(:mapping) { '' } + context 'when code is missing' do + it 'raises a `MissingRequestParam` error' do + expect { strategy.callback(nonce: 'nonce', code_verifier: 'foo') } + .to raise_error(Errors::Authentication::RequestBody::MissingRequestParam) + end + end + context 'when nonce is missing' do + it 'raises a `MissingRequestParam` error' do + expect { strategy.callback(code: 'code', code_verifier: 'foo') } + .to raise_error(Errors::Authentication::RequestBody::MissingRequestParam) + end end end end diff --git a/spec/app/domain/authentication/authn-oidc/v2/views/providor_context_spec.rb b/spec/app/domain/authentication/authn-oidc/v2/views/providor_context_spec.rb index 79aafb9436..70e0886cf0 100644 --- a/spec/app/domain/authentication/authn-oidc/v2/views/providor_context_spec.rb +++ b/spec/app/domain/authentication/authn-oidc/v2/views/providor_context_spec.rb @@ -5,8 +5,7 @@ RSpec.describe('Authentication::AuthnOidc::V2::Views::ProviderContext') do let(:client) do class_double(::Authentication::AuthnOidc::V2::Client).tap do |double| - allow(double).to receive(:new).and_return( - current_client) + allow(double).to receive(:new).and_return(current_client) end end @@ -19,60 +18,84 @@ let(:endpoint) { double(authorization_endpoint: '"http://test"') } let(:foo) do - Authentication::AuthnOidc::V2::DataObjects::Authenticator - .new(account: "cucumber", - service_id: "foo", - redirect_uri: "http://conjur/authn-oidc/cucumber/authenticate", - provider_uri: "http://test", - name: "foo", - state: 'foostate', - client_id: "ConjurClient", - client_secret: 'client_secret', - claim_mapping: 'claim_mapping', - nonce: 'secret', - ) + Authentication::AuthnOidc::V2::DataObjects::Authenticator.new( + account: "cucumber", + service_id: "foo", + redirect_uri: "http://conjur/authn-oidc/cucumber/authenticate", + provider_uri: "http://foo", + name: "foo", + client_id: "ConjurClient", + client_secret: 'client_secret', + claim_mapping: 'claim_mapping' + ) end let(:bar) do - Authentication::AuthnOidc::V2::DataObjects::Authenticator - .new(account: "cucumber", - service_id: "bar", - provider_uri: "http://test", - redirect_uri: "http://conjur/authn-oidc/cucumber/authenticate", - name: "bar", - state: 'barstate', - client_id: "ConjurClient", - client_secret: 'client_secret', - claim_mapping: 'claim_mapping', - nonce: 'secret') + Authentication::AuthnOidc::V2::DataObjects::Authenticator.new( + account: "cucumber", + service_id: "bar", + provider_uri: "http://bar", + redirect_uri: "http://conjur/authn-oidc/cucumber/authenticate", + name: "bar", + client_id: "ConjurClient", + client_secret: 'client_secret', + claim_mapping: 'claim_mapping' + ) end let(:authenticators) {[foo, bar]} - let(:res) do - [{ name: "foo", - redirect_uri: "\"http://test\"?client_id=ConjurClient&response_type=code&scope=openid%20email%20profile" \ - "&state=foostate&nonce=secret&redirect_uri=http%3A%2F%2Fconjur%2Fauthn-oidc%2Fcucumber%2Fauthenticate", - service_id: "foo", - type: "authn-oidc" }, - { name: "bar", - redirect_uri: "\"http://test\"?client_id=ConjurClient&response_type=code&scope=openid%20email%20profile" \ - "&state=barstate&nonce=secret&redirect_uri=http%3A%2F%2Fconjur%2Fauthn-oidc%2Fcucumber%2Fauthenticate", - service_id: "bar", - type: "authn-oidc" }] + let(:response) do + [ + { + name: "foo", + redirect_uri: "\"http://test\"?client_id=ConjurClient&response_type=code&scope=openid%20email%20profile" \ + "&nonce=random-string&code_challenge=random-sha&code_challenge_method=S256&redirect_uri=http%3A%2F%2Fconjur%2Fauthn-oidc%2Fcucumber%2Fauthenticate", + service_id: "foo", + type: "authn-oidc", + nonce: 'random-string', + code_verifier: 'random-string' + }, { + name: "bar", + redirect_uri: "\"http://test\"?client_id=ConjurClient&response_type=code&scope=openid%20email%20profile" \ + "&nonce=random-string&code_challenge=random-sha&code_challenge_method=S256&redirect_uri=http%3A%2F%2Fconjur%2Fauthn-oidc%2Fcucumber%2Fauthenticate", + service_id: "bar", + type: "authn-oidc", + nonce: 'random-string', + code_verifier: 'random-string' + } + ] end let(:provider_context) do ::Authentication::AuthnOidc::V2::Views::ProviderContext.new( - client: client + client: client, + random: random, + digest: digest, + logger: logger ) end + let(:log_output) { StringIO.new } + let(:logger) { Logger.new(log_output) } + + let(:digest) do + class_double(Digest::SHA256).tap do |double| + allow(double).to receive(:base64digest).and_return('random-sha') + end + end + + let(:random) do + class_double(SecureRandom).tap do |double| + allow(double).to receive(:hex).and_return('random-string') + end + end + describe('#call') do context 'when provider context is given multiple authenticators' do it 'returns the providers object with the redirect urls' do expect(provider_context.call(authenticators: authenticators)) - .to eq(res) + .to eq(response) end end @@ -82,6 +105,22 @@ .to eq([]) end end + + context 'when authenticator discovery endpoint is unreachable' do + let(:current_client) do + instance_double(::Authentication::AuthnOidc::V2::Client).tap do |double| + allow(double).to receive(:discovery_information).and_raise( + Errors::Authentication::OAuth::ProviderDiscoveryFailed + ) + end + end + it 'does not cause an exception' do + expect(provider_context.call(authenticators: authenticators)).to eq([]) + expect(log_output.string).to include('WARN') + %w[foo bar].each do |authenticator| + expect(log_output.string).to include("Authn-OIDC '#{authenticator}' provider-uri: 'http://#{authenticator}' is unreachable") + end + end + end end end - diff --git a/spec/app/domain/authentication/util/namespace_selector_spec.rb b/spec/app/domain/authentication/util/namespace_selector_spec.rb index a16c2b0e77..d9f37fb6bb 100644 --- a/spec/app/domain/authentication/util/namespace_selector_spec.rb +++ b/spec/app/domain/authentication/util/namespace_selector_spec.rb @@ -6,26 +6,13 @@ describe '#select' do context 'when type is a supported authenticator type' do context 'when type is `authn-oidc`' do - context 'when pkce support feature flag is enabled' do - it 'returns the valid namespace' do - expect( - Authentication::Util::NamespaceSelector.select( - authenticator_type: 'authn-oidc', - pkce_support_enabled: true - ) - ).to eq('Authentication::AuthnOidc::PkceSupportFeature') - end - end - - context 'when pkce support feature flag is disabled' do - it 'returns the valid namespace' do - expect( - Authentication::Util::NamespaceSelector.select( - authenticator_type: 'authn-oidc', - pkce_support_enabled: false - ) - ).to eq('Authentication::AuthnOidc::V2') - end + it 'returns the valid namespace' do + expect( + Authentication::Util::NamespaceSelector.select( + authenticator_type: 'authn-oidc', + pkce_support_enabled: false + ) + ).to eq('Authentication::AuthnOidc::V2') end end end