Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds PKCE Support to Conjur #2678

Merged
merged 2 commits into from
Dec 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,18 @@ 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-qv4q-mr5r-qprj
[cyberark/conjur#2684](https://github.com/cyberark/conjur/pull/2684)

### Fixed
- Previously, if an OIDC authenticator was configured with a `Status` webservice,
the OIDC provider endpoint would include duplicate OIDC authenticators. This
change resolves ONYX-25530.
[cyberark/conjur#2678](https://github.com/cyberark/conjur/pull/2678)

### Added
- Provides support for PKCE in the OIDC Authenticator code redirect workflow.
This is disabled by default, but is available under the
`CONJUR_FEATURE_PKCE_SUPPORT_ENABLED` feature flag.
[cyberark/conjur#2678](https://github.com/cyberark/conjur/pull/2678)

## [1.19.0] - 2022-11-29

### Added
Expand Down
34 changes: 29 additions & 5 deletions app/db/repository/authenticator_repository.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
module DB
module Repository
class AuthenticatorRepository
def initialize(data_object:, resource_repository: ::Resource, logger: Rails.logger)
def initialize(
data_object:,
resource_repository: ::Resource,
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
end

def find_all(type:, account:)
Expand All @@ -14,7 +20,14 @@ def find_all(type:, account:)
"#{account}:webservice:conjur/#{type}/%"
)
).all.map do |webservice|
load_authenticator(account: account, id: webservice.id.split(':').last, type: type)
service_id = service_id_from_resource_id(webservice.id)
Copy link

Choose a reason for hiding this comment

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

DB::Repository::AuthenticatorRepository#find_all calls 'webservice.id' 2 times


# Querying for the authenticator webservice above includes the webservices
# for the authenticator status. The filter below removes webservices that
# don't match the authenticator policy.
next unless webservice.id.split(':').last == "conjur/#{type}/#{service_id}"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear what business logic this is performing. Can you add a comment elaborating what the outcome of this filtering is?


load_authenticator(account: account, service_id: service_id, type: type)
end.compact
end

Expand All @@ -27,7 +40,7 @@ def find(type:, account:, service_id:)
).first
return unless webservice

load_authenticator(account: account, id: webservice.id.split(':').last, type: type)
load_authenticator(account: account, service_id: service_id, type: type)
end

def exists?(type:, account:, service_id:)
Expand All @@ -36,8 +49,12 @@ def exists?(type:, account:, service_id:)

private

def load_authenticator(type:, account:, id:)
service_id = id.split('/')[2]
def service_id_from_resource_id(id)
Copy link

Choose a reason for hiding this comment

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

DB::Repository::AuthenticatorRepository#service_id_from_resource_id doesn't depend on instance state (maybe move it to another class?)

full_id = id.split(':').last
full_id.split('/')[2]
end

def load_authenticator(type:, account:, service_id:)
Copy link

Choose a reason for hiding this comment

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

Method load_authenticator has 26 lines of code (exceeds 25 allowed). Consider refactoring.

variables = @resource_repository.where(
Sequel.like(
:resource_id,
Expand All @@ -54,7 +71,14 @@ def load_authenticator(type:, account:, id:)
args[variable.resource_id.split('/')[-1].underscore.to_sym] = variable.secret.value
end
end

begin
if @pkce_support_enabled
allowed_args = %i[account service_id] +
@data_object.const_get(:REQUIRED_VARIABLES) +
Copy link

Choose a reason for hiding this comment

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

Use 2 (not 14) spaces for indenting an expression in an assignment spanning multiple lines.

@data_object.const_get(:OPTIONAL_VARIABLES)
Copy link

Choose a reason for hiding this comment

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

Use 2 (not 14) spaces for indenting an expression in an assignment spanning multiple lines.

args_list = args_list.select{ |key, _| allowed_args.include?(key) }
end
@data_object.new(**args_list)
rescue ArgumentError => e
@logger.debug("DB::Repository::AuthenticatorRepository.load_authenticator - exception: #{e}")
Expand Down
117 changes: 117 additions & 0 deletions app/domain/authentication/authn_oidc/pkce_support_feature/client.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
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
Copy link

Choose a reason for hiding this comment

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

Complex method Authentication::AuthnOidc::PkceSupportFeature::Client#oidc_client (43.4)

@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:)
Copy link

Choose a reason for hiding this comment

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

Method callback has 51 lines of code (exceeds 25 allowed). Consider refactoring.

Copy link

Choose a reason for hiding this comment

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

Complex method Authentication::AuthnOidc::PkceSupportFeature::Client#callback (32.5)

Copy link

Choose a reason for hiding this comment

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

Method callback has a Cognitive Complexity of 10 (exceeds 5 allowed). Consider refactoring.

oidc_client.authorization_code = code
begin
bearer_token = oidc_client.access_token!(
scope: true,
client_auth_method: :basic,
code_verifier: code_verifier
)
rescue Rack::OAuth2::Client::Error => e
# Only handle the expected errors related to access token retrieval.
case e.message
Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnOidc::PkceSupportFeature::Client#callback calls 'e.message' 2 times

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
Copy link

Choose a reason for hiding this comment

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

Omit the error class when rescuing StandardError by itself.

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)
Copy link

Choose a reason for hiding this comment

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

Complex method Authentication::AuthnOidc::PkceSupportFeature::Client#discovery_information (21.8)

Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnOidc::PkceSupportFeature::Client#discovery_information has boolean parameter 'invalidate'

@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
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
module Authentication
module AuthnOidc
module PkceSupportFeature
module DataObjects
class Authenticator
Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnOidc::PkceSupportFeature::DataObjects::Authenticator has no descriptive comment


REQUIRED_VARIABLES = %i[provider_uri client_id client_secret claim_mapping].freeze
OPTIONAL_VARIABLES = %i[redirect_uri response_type provider_scope name].freeze

attr_reader(
:provider_uri,
:client_id,
:client_secret,
:claim_mapping,
:account,
:service_id,
:redirect_uri,
:response_type
)

def initialize(
Copy link

Choose a reason for hiding this comment

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

Avoid parameter lists longer than 7 parameters. [10/7]

provider_uri:,
client_id:,
client_secret:,
claim_mapping:,
account:,
service_id:,
redirect_uri: nil,
name: nil,
response_type: 'code',
provider_scope: nil
)
@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
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
end
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
module Authentication
module AuthnOidc
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

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
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
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 code_verifier].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
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
require 'securerandom'
require 'digest'

module Authentication
module AuthnOidc
module PkceSupportFeature
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|
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
)
}
end
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
Loading