Adds PKCE Support to Conjur #2678

jvanderhoof · 2022-11-23T15:45:06Z

Desired Outcome

This PR attempts to accomplish two objectives:

Segregate the PKCE Support using the recently added Feature Flag functionality.
Provide a pattern for abstracting and testing workflows managed by a feature flag.

Implemented Changes

PKCE Support

Primary Changes:
- Adds a generated code challenge and code verify to provider API
- OIDC authentication endpoint now requires code verify, which is used in the provider token request
Secondary Changes:
- A unique nonce is generated on each Provider API request
- State is no longer generated on Provider API request. A client should manage its own state value without Conjur being aware.
- Adds required and optional fields to the Authenticator data object. This allows the repository to load only relevant variables.
- Moves the required parameters check up to the Strategy. This allows a handler to be unconcerned with authentication details.

Feature Flag Patterning

Primary Changes:
- Adds the ClimateControl gem and a small helper method (with_feature_enabled) to run RSpec tests with a feature toggled on (flags are off by default). This prevents us using environment variables on the container or test level.
  
  Note: we still need to develop an approach for testing Cucumber tests as they operate against an external container.

Reviewers should step through commits.

Connected Issue/Story

N/A

Definition of Done

Changelog

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a
CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code
changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR
A follow-up issue to update official docs has been filed here: ONYX-27159
This PR does not require updating any documentation

Behavior

This PR changes product behavior and has been reviewed by a PO, or
These changes are part of a larger initiative that will be reviewed later, or
No behavior was changed with this PR

Security

Security architect has reviewed the changes in this PR,
These changes are part of a larger initiative with a separate security review, or
There are no security aspects to these changes

codeclimate · 2022-11-23T18:18:59Z

app/domain/authentication/authn_oidc/pkce_support_feature/client.rb

+ end
+ end
+
+ def callback(code:, nonce:, code_verifier:)


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

codeclimate · 2022-11-23T18:18:59Z

app/domain/authentication/authn_oidc/pkce_support_feature/data_objects/authenticator.rb

+ module AuthnOidc
+ module PkceSupportFeature
+ module DataObjects
+ class Authenticator


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

codeclimate · 2022-11-23T18:19:00Z

app/domain/authentication/authn_oidc/pkce_support_feature/resolve_identity.rb

@@ -0,0 +1,19 @@
+module Authentication
+ module AuthnOidc


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

codeclimate · 2022-11-23T18:19:00Z

app/domain/authentication/authn_oidc/pkce_support_feature/client.rb

+ @logger = logger
+ end
+
+ def oidc_client


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

codeclimate · 2022-11-23T18:19:00Z

app/domain/authentication/authn_oidc/pkce_support_feature/client.rb

+ end
+ end
+
+ def callback(code:, nonce:, code_verifier:)


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

codeclimate · 2022-11-23T19:33:03Z

app/domain/authentication/authn_oidc/pkce_support_feature/client.rb

+ end
+ end
+
+ def callback(code:, nonce:, code_verifier:)


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

codeclimate · 2022-11-23T19:33:03Z

app/domain/errors.rb

+ code: "CONJ00133E"
+ )
+
+ # TODO - this is related to the feature flag 'pkce_support'. Once


Annotation keywords like TODO should be all upper case, followed by a colon, and a space, then a note describing the problem.

codeclimate · 2022-11-23T19:33:03Z

app/domain/authentication/authn_oidc/pkce_support_feature/data_objects/authenticator.rb

+ OPTIONAL_VARIABLES = %i[redirect_uri response_type provider_scope name].freeze
+
+ attr_reader :provider_uri, :client_id, :client_secret, :claim_mapping, :account
+ attr_reader :service_id, :redirect_uri, :response_type


Group together all attr_reader attributes.

codeclimate · 2022-11-23T19:33:03Z

app/domain/authentication/authn_oidc/pkce_support_feature/data_objects/authenticator.rb

+ 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


Group together all attr_reader attributes.

codeclimate · 2022-11-23T19:33:04Z

app/domain/authentication/authn_oidc/pkce_support_feature/client.rb

+ decoded_id_token
+ end
+
+ def discovery_information(invalidate: false)


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

micahlee · 2022-11-28T15:16:36Z

CHANGELOG.md

+### Added
+- Provides support for PKCE in the OIDC Authenticator code redirect workflow.
+ This is disabled by default, but is available under the
+ `CONJUR_FEATURE_POLICY_LOAD_EXTENSIONS` feature flag.


I think this should be CONJUR_FEATURE_PKCE_SUPPORT_ENABLED, right?

doh! good catch.

micahlee

Hey @jvanderhoof,

I left some thoughts for you regarding the testing approach for feature flags. This isn't something I feel super strongly about, so you can tell me if you rather keep it as it is.

Thanks!

(Also, I'm assuming all of the logic itself is previously reviewed, and we're only reviewing the feature flag part, is that correct?)

micahlee · 2022-11-28T15:27:46Z

spec/spec_helper.rb

@@ -150,3 +150,11 @@ def conjur_server_dir
 # Navigate from its directory (/bin) to the root Conjur server directory
 Pathname.new(File.join(File.dirname(conjurctl_path), '..')).cleanpath
 end
+
+# Toggle a feature flag on for a particular set of tests.
+def with_feature_enabled(feature, &block)


I'm not opposed to this, but I am concerned it:

A) Adds more "magic"/black box logic our tests, when we've been trending to more "obvious face value" specs.

B) Encourages the use of integration-style specs that depend on the actual environment, rather than injecting dependencies.

I'll add some more specific comments in the use of this in the tests.

Handling the flag toggle with dependency injection means this can be removed. It's MUCH cleaner and prevents us from needing to mutate the environment variable during testing. I'm removing this and the gem.

micahlee · 2022-11-28T17:56:16Z

spec/app/db/repository/authenticator_repository_spec.rb

- let(:arguments) { %i[provider_uri client_id client_secret claim_mapping nonce state] }
+ %w[true false].each do |enabled|
+ context "with flag #{enabled}" do
+ with_feature_enabled(:pkce_support) do


It doesn't look like enabled is actually used here to turn it on or off.

micahlee · 2022-11-28T17:57:28Z

spec/app/db/repository/authenticator_repository_spec.rb

+ if Rails.configuration.feature_flags.enabled?(:pkce_support)
+ Authentication::AuthnOidc::PkceSupportFeature::DataObjects::Authenticator
+ else
+ Authentication::AuthnOidc::V2::DataObjects::Authenticator
+ end


Here's an example of where I'd rather see this driven by test inputs, the enabled option above, rather than indirectly through the feature flag environment variable.

micahlee · 2022-11-28T17:58:40Z

app/db/repository/authenticator_repository.rb

 begin
- @data_object.new(**args_list)
+ if Rails.configuration.feature_flags.enabled?(:pkce_support)


Again, with testing in mind. I'd rather see either configuration, feature_flags, or even pkce_support passed in as a dependency to the class, and use that when unit testing this, rather than setting the environment.

Brillant. I didn't even think about that as an option 😢. That may also allow us to remove the toggle in testing...

jvanderhoof · 2022-12-06T17:17:08Z

Hey @jvanderhoof,

I left some thoughts for you regarding the testing approach for feature flags. This isn't something I feel super strongly about, so you can tell me if you rather keep it as it is.

Thanks!

(Also, I'm assuming all of the logic itself is previously reviewed, and we're only reviewing the feature flag part, is that correct?)

The flagged functionality was reviewed, but if you have any concerns, I'd love the feedback. The original code was merged in more of a rush than I felt was ideal.

micahlee · 2022-12-06T17:56:48Z

Hey @jvanderhoof, I think you meant to rebase this PR, rather than merge master in:

micahlee · 2022-12-06T18:05:09Z

Gemfile

@@ -80,7 +80,6 @@ gem 'i18n', '~> 1.8.11'
 group :development, :test do
 gem 'aruba'
 gem 'ci_reporter_rspec'
- gem 'climate_control'


Can we squash these changes down so that the gemfile changes don't manifest in this PR at all (added in an earlier commit and deleted here in the same PR).

micahlee

Thanks @jvanderhoof, I left a few more comments on the PR as a whole. Feel free to take them or leave them! 😃

micahlee · 2022-12-06T18:32:09Z

app/db/repository/authenticator_repository.rb

@@ -14,7 +20,10 @@ 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_id(webservice.id)


Could this be service_id_from_resource_id instead?

micahlee · 2022-12-06T18:33:04Z

app/db/repository/authenticator_repository.rb

@@ -14,7 +20,10 @@ 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_id(webservice.id)
+ next unless webservice.id.split(':').last == "conjur/#{type}/#{service_id}"


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

micahlee · 2022-12-06T18:35:12Z

app/domain/authentication/authn_oidc/pkce_support_feature/client.rb

+ id_token,
+ discovery_information.jwks
+ )
+ rescue Exception => e


Code climate also flags this. Do we need to rescue Exception here, or can it be just StandardError? If Exception is intentional, mind adding a comment why that is?

micahlee · 2022-12-06T18:36:46Z

app/domain/authentication/authn_oidc/pkce_support_feature/strategy.rb

+ @logger = logger
+ end
+
+ # Don't love this name...


Mind either removing this comment, or elaborate on it such that someone other than you (or future you!) could continue the train of thought in an actionable way?

micahlee · 2022-12-06T18:38:22Z

app/domain/errors.rb

+ # TODO - this is related to the feature flag 'pkce_support'. Once
+ # this feature is permanently added, this error should be removed.
+ StateMismatch = ::Util::TrackableErrorClass.new(
+ msg: "Conjur internal state doesn't match given state",
+ code: "CONJ00127E"
+ )


It's not clear to me why this is related to the feature flag itself. Mind elaborating a little more as to why this error won't be relevant once the feature is graduated from a flag?

micahlee · 2022-12-06T21:39:20Z

CHANGELOG.md

@@ -9,7 +9,19 @@ 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.0] - 2022-11-29
+## [1.19.0] - 2022-12-06


It looks like this should be bumped to 1.19.1 (or greater).

codeclimate · 2022-12-07T17:51:09Z

app/db/repository/authenticator_repository.rb

+ full_id.split('/')[2]
+ end
+
+ def load_authenticator(type:, account:, service_id:)


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

codeclimate · 2022-12-07T17:51:09Z

app/domain/errors.rb

+ code: "CONJ00133E"
+ )
+
+ # TODO - this error comes from the current version of the OIDC


Annotation keywords like TODO should be all upper case, followed by a colon, and a space, then a note describing the problem.

codeclimate · 2022-12-07T17:51:09Z

app/domain/authentication/authn_oidc/pkce_support_feature/data_objects/authenticator.rb

+ :response_type
+ )
+
+ def initialize(


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

codeclimate · 2022-12-07T17:51:09Z

app/domain/authentication/authn_oidc/pkce_support_feature/client.rb

+ id_token,
+ discovery_information.jwks
+ )
+ rescue Exception => e


Avoid rescuing the Exception class. Perhaps you meant to rescue StandardError?

codeclimate · 2022-12-07T17:51:10Z

app/db/repository/authenticator_repository.rb

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


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

szh · 2022-12-08T19:18:35Z

@jvanderhoof Is there an ETA for when this will be merged?

codeclimate · 2022-12-08T20:43:11Z

app/domain/authentication/authn_oidc/v2/status.rb

+ module AuthnOidc
+ module V2
+ class Status
+ def call(authenticator:)


Unused method argument - authenticator. You can also write as call(*) if you want the method to accept any arguments but don't care about them.

codeclimate · 2022-12-08T20:43:12Z

app/domain/authentication/authn_oidc/v2/client.rb

@@ -56,7 +56,7 @@ def callback(code:)
 id_token,
 discovery_information.jwks
 )
- rescue Exception => e
+ rescue StandardError => e


Omit the error class when rescuing StandardError by itself.

codeclimate · 2022-12-08T20:43:12Z

app/domain/authentication/authn_oidc/pkce_support_feature/client.rb

+ id_token,
+ discovery_information.jwks
+ )
+ rescue StandardError => e


Omit the error class when rescuing StandardError by itself.

codeclimate · 2022-12-08T20:43:12Z

app/db/repository/authenticator_repository.rb

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


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

codeclimate · 2022-12-08T20:43:12Z

app/db/repository/authenticator_repository.rb

@@ -36,8 +49,12 @@

 private

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


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

This commit fixes the issue where an OIDC authenticator with a status webservice causes the providers endpoint to display the authenticator twice.

This commit adds support for PKCE to improve the security posture of OIDC Authentication. It also includes small improvements to make the Authentication Handler and Authenticator Repository more generic and durable. This feature is behind a feature flag as the UI requires changes to support this improved workflow.

micahlee

LGTM! Thanks, @jvanderhoof

codeclimate · 2022-12-08T21:36:07Z

app/db/repository/authenticator_repository.rb

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


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

codeclimate · 2022-12-08T21:36:07Z

app/domain/authentication/util/namespace_selector.rb

- # the original OIDC authenticator is really a
- # glorified JWT authenticator.
- 'Authentication::AuthnOidc::V2'
+ if pkce_support_enabled


Authentication::Util::NamespaceSelector#self.select is controlled by argument 'pkce_support_enabled'

codeclimate · 2022-12-08T21:36:07Z

app/domain/authentication/authn_oidc/v2/status.rb

+ module AuthnOidc
+ module V2
+ class Status
+ def call(authenticator:)


Authentication::AuthnOidc::V2::Status#call has unused parameter 'authenticator'

codeclimate · 2022-12-08T21:36:07Z

app/domain/authentication/authn_oidc/pkce_support_feature/client.rb

+ decoded_id_token
+ end
+
+ def discovery_information(invalidate: false)


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

codeclimate · 2022-12-08T21:36:07Z

app/domain/authentication/authn_oidc/pkce_support_feature/client.rb

+ )
+ rescue Rack::OAuth2::Client::Error => e
+ # Only handle the expected errors related to access token retrieval.
+ case e.message


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

codeclimate · 2022-12-08T21:36:10Z

Code Climate has analyzed commit 0951249 and detected 46 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	33
Duplication	4
Bug Risk	1
Style	8

The test coverage on the diff in this pull request is 77.5% (50% is the threshold).

This pull request will bring the total coverage in the repository to 90.0% (-1.7% change).

View more on Code Climate.

codeclimate bot reviewed Nov 23, 2022

View reviewed changes

jvanderhoof force-pushed the pkce-support branch 2 times, most recently from c188f13 to adece22 Compare November 23, 2022 19:06

codeclimate bot reviewed Nov 23, 2022

View reviewed changes

jvanderhoof marked this pull request as ready for review November 23, 2022 20:32

jvanderhoof requested a review from a team as a code owner November 23, 2022 20:32

micahlee reviewed Nov 28, 2022

View reviewed changes

jvanderhoof force-pushed the pkce-support branch from adece22 to fd6155b Compare November 28, 2022 17:33

micahlee reviewed Nov 28, 2022

View reviewed changes

micahlee reviewed Dec 6, 2022

View reviewed changes

jvanderhoof force-pushed the pkce-support branch 3 times, most recently from 0ebf0ea to 38d9d4e Compare December 6, 2022 20:27

micahlee reviewed Dec 6, 2022

View reviewed changes

codeclimate bot reviewed Dec 7, 2022

View reviewed changes

jvanderhoof force-pushed the pkce-support branch from 6ca3b52 to 235ce06 Compare December 8, 2022 20:18

codeclimate bot reviewed Dec 8, 2022

View reviewed changes

jvanderhoof force-pushed the pkce-support branch from 235ce06 to c620d14 Compare December 8, 2022 21:08

jvanderhoof added 2 commits December 8, 2022 14:10

Fixes "Duplicate Providers" Bug

89c7501

This commit fixes the issue where an OIDC authenticator with a status webservice causes the providers endpoint to display the authenticator twice.

jvanderhoof force-pushed the pkce-support branch from c620d14 to 0951249 Compare December 8, 2022 21:10

micahlee approved these changes Dec 8, 2022

View reviewed changes

codeclimate bot reviewed Dec 8, 2022

View reviewed changes

jvanderhoof merged commit 94aa6e9 into master Dec 9, 2022

jvanderhoof deleted the pkce-support branch December 9, 2022 17:39

john-odonnell mentioned this pull request Mar 3, 2023

Release v1.19.2+suite.1 cyberark/conjur-oss-suite-release#262

Merged

13 tasks

Adds PKCE Support to Conjur #2678

Adds PKCE Support to Conjur #2678

Conversation

jvanderhoof commented Nov 23, 2022 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

codeclimate bot Nov 23, 2022

Choose a reason for hiding this comment

codeclimate bot Nov 23, 2022

Choose a reason for hiding this comment

codeclimate bot Nov 23, 2022

Choose a reason for hiding this comment

codeclimate bot Nov 23, 2022

Choose a reason for hiding this comment

codeclimate bot Nov 23, 2022

Choose a reason for hiding this comment

codeclimate bot Nov 23, 2022

Choose a reason for hiding this comment

codeclimate bot Nov 23, 2022

Choose a reason for hiding this comment

codeclimate bot Nov 23, 2022

Choose a reason for hiding this comment

codeclimate bot Nov 23, 2022

Choose a reason for hiding this comment

codeclimate bot Nov 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micahlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvanderhoof commented Dec 6, 2022

micahlee commented Dec 6, 2022

Choose a reason for hiding this comment

micahlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot Dec 7, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 7, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 7, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 7, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 7, 2022

Choose a reason for hiding this comment

szh commented Dec 8, 2022 • edited Loading

codeclimate bot Dec 8, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 8, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 8, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 8, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 8, 2022

Choose a reason for hiding this comment

micahlee left a comment

Choose a reason for hiding this comment

codeclimate bot Dec 8, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 8, 2022

jvanderhoof commented Nov 23, 2022 •

edited

Loading

szh commented Dec 8, 2022 •

edited

Loading