Adds OIDC code workflow #2595

jvanderhoof · 2022-07-05T18:21:34Z

Desired Outcome

This PR adds support for OIDC's code-based authorization flow. This functionality allows Conjur to use an external
OIDC Provider (Okta, Keycloak), to authenticate users and redirect them to Conjur on success. Conjur will resolve the identified user to a Conjur User ID and return and Authorization Token.

Implemented Changes

The architecture of this feature is different from that of the other authenticators. The intention of this architecture is to simplify testing and reduce developer cognitive load.

Authenticator Specific Constructs:
- Authenticator Data Object - contains the configuration data specific to an authenticator type.
- Strategy - implements the lower level details (the "how") an authenticator verifies an external identity.
- Resolve Identity - functionality to an externally identified identity with an internal identity.
- Client - handles the low level network specifics of verifying an identity with an external identity provider.
Generic Authenticator Constructs:
- Authentication Handler - a generic handler which calls the authenticator specific objects to orchestrate the authentication flow.

Planned Future Work

Add tests for Authentication Handler: ONYX-22990
Document Authenticator Architecture: ONYX-22991

Connected Issue/Story

Jira Epic

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

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: insert issue ID
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-07-12T13:55:17Z

app/domain/authentication/handler/authentication_handler.rb

+        )
+      end
+
+      def log_audit_failure(account, service_id, client_ip, type, error)


Method log_audit_failure has 5 arguments (exceeds 4 allowed). Consider refactoring.

codeclimate · 2022-07-12T13:55:18Z

app/domain/authentication/handler/authentication_handler.rb

+        )
+      end
+
+      def call(parameters:, request_ip:)


Method call has a Cognitive Complexity of 15 (exceeds 5 allowed). Consider refactoring.

codeclimate · 2022-07-12T13:55:18Z

app/domain/authentication/handler/authentication_handler.rb

+        )
+      end
+
+      def call(parameters:, request_ip:)


Method call has 35 lines of code (exceeds 25 allowed). Consider refactoring.

codeclimate · 2022-07-12T13:55:18Z

app/domain/authentication/handler/authentication_handler.rb

+        handle_error(e)
+      end
+
+      def handle_error(err)


Method handle_error has 30 lines of code (exceeds 25 allowed). Consider refactoring.

codeclimate · 2022-07-12T13:55:18Z

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

+  module AuthnOidc
+    module V2
+      class ResolveIdentity
+        def call(identity:, account:, allowed_roles:)


Method call has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

mFelgate

lgtm (other then the error handling haha)

telday · 2022-07-12T16:15:22Z

app/controllers/authenticate_controller.rb

@@ -4,6 +4,25 @@ class AuthenticateController < ApplicationController
  include BasicAuthenticator
  include AuthorizeResource

+  def authenticate_okta


Should this be named something closer to authenticate_oidc_v2 as we use it for other services as well?

telday · 2022-07-12T16:18:10Z

app/db/repository/authenticator_repository.rb

+        webservice =  @resource_repository.where(
+          Sequel.like(
+            :resource_id,
+            "#{account}:webservice:conjur/#{type}/#{service_id}%"


It looks like this function is trying to find a specific webservice but wouldn't the % at the end allow it to pickup other rows from the db we don't necessarily want? Or am I missing something with this function?

good catch @telday!

telday · 2022-07-12T16:25:42Z

app/domain/authentication/handler/authentication_handler.rb

+          ).all
+        )
+
+        # TODO: Add an error message


Do we want to address this before merging or leave for later

telday · 2022-07-12T16:27:24Z

app/domain/authentication/handler/authentication_handler.rb

+        @logger.warn(@authentication_error.new(err))
+        case err
+        when Errors::Authentication::Security::RoleNotAuthorizedOnResource
+          puts "Errors::Authentication::Security::RoleNotAuthorizedOnResource"


Out of curiosity why are we printing the error class here?

We could we have a puts err.class before the switch statement to eliminate most of the print statements

codeclimate · 2022-07-12T22:45:18Z

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

+          end
+        end
+
+        def callback(code:)


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

codeclimate · 2022-07-12T22:45:18Z

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

+          end
+        end
+
+        def callback(code:)


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

codeclimate · 2022-07-12T22:45:18Z

app/db/repository/authenticator_repository.rb

+
+      private
+
+      def load_authenticator(type:, account:, id:)


Method load_authenticator has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

codeclimate · 2022-07-12T22:45:19Z

cucumber/authenticators_oidc/features/step_definitions/authn_list_steps.rb

+require 'cucumber/policy/features/support/client'
+require 'json'
+
+Then('the list of authenticators contains the service-id {string}') do |service_id|


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

codeclimate · 2022-07-12T22:45:19Z

cucumber/authenticators_oidc/features/step_definitions/authn_list_steps.rb

+end
+
+
+Then('the list of authenticators does not contain the service-id {string}') do |service_id|


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

codeclimate · 2022-07-14T17:44:29Z

app/controllers/authenticate_controller.rb

+  def oidc_authenticate_code_redirect
+    # TODO: need a mechanism for an authenticator strategy to define the required
+    # params. This will likely need to be done via the Handler.
+    params.permit!


Parameters should be whitelisted for mass assignment

codeclimate · 2022-07-14T17:44:29Z

Gemfile

  gem 'json_spec', '~> 1.1'
+  gem 'faye-websocket'


Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem faye-websocket should appear before json_spec.

codeclimate · 2022-07-14T17:44:29Z

app/domain/authentication/handler/authentication_handler.rb

+        ::Authentication::LogAuditEvent.new.call(
+          authentication_params:
+            Authentication::AuthenticatorInput.new(
+              authenticator_name: "#{type}",


Prefer to_s over string interpolation.

codeclimate · 2022-07-14T17:44:29Z

app/domain/authentication/handler/authentication_handler.rb

+        ::Authentication::LogAuditEvent.new.call(
+          authentication_params:
+            Authentication::AuthenticatorInput.new(
+              authenticator_name: "#{type}",


Prefer to_s over string interpolation.

codeclimate · 2022-07-14T17:44:29Z

app/domain/authentication/handler/authentication_handler.rb

+          raise ApplicationController::BadRequest
+
+        when Errors::Conjur::RequestedResourceNotFound
+          raise ApplicationController::RecordNotFound.new(err.message)


Provide an exception class and message as arguments to raise.

This commit includes all work related to support for the code-based OIDC authentication workflow.

codeclimate · 2022-07-14T20:12:45Z

Gemfile

@@ -88,6 +88,7 @@ group :development, :test do
  gem 'debase', '~> 0.2.5.beta2'
  gem 'faye-websocket'
  gem 'json_spec', '~> 1.1'
+  gem 'faye-websocket'


Gem faye-websocket requirements already given on line 89 of the Gemfile.

codeclimate · 2022-07-14T20:12:45Z

app/domain/authentication/handler/authentication_handler.rb

+
+      def call(parameters:, request_ip:)
+        raise Errors::Authentication::RequestBody::MissingRequestParam, parameters[:code] unless parameters[:code]
+        raise Errors::Authentication::RequestBody::MissingRequestParam, parameters[:state] unless parameters[:state]


Add empty line after guard clause.

codeclimate · 2022-07-14T20:12:45Z

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

@@ -0,0 +1,41 @@
+
+module Authentication


Unnecessary blank line at the beginning of the source.

codeclimate · 2022-07-14T20:12:46Z

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

+          attr_reader :provider_uri, :client_id, :client_secret, :claim_mapping, :nonce, :state, :account
+          attr_reader :service_id, :redirect_uri, :response_type
+
+          def initialize(


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

codeclimate · 2022-07-14T20:12:46Z

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

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


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

codeclimate · 2022-07-14T20:12:48Z

Code Climate has analyzed commit e0d8366 and detected 124 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	76
Duplication	5
Bug Risk	4
Style	38
Security	1

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

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

View more on Code Climate.

mFelgate · 2022-07-14T20:38:35Z

cucumber/authenticators_oidc/features/list_authenticators.feature

+  @smoke
+  Scenario: List readable authenticators
+
+    Given I load a policy:


this, and the variable setting should be the background.

mFelgate · 2022-07-14T20:38:40Z

cucumber/authenticators_oidc/features/list_authenticators.feature

+    Then I can add a secret to variable resource "conjur/authn-oidc/okta/claim-mapping"
+    Then I can add a secret to variable resource "conjur/authn-oidc/okta/nonce"
+    Then I can add a secret to variable resource "conjur/authn-oidc/okta/state"
+    When I log in as user "admin"


these should be separate Scenario

jvanderhoof force-pushed the adds-oidc-code-workflow branch from f18eec8 to 0553203 Compare July 11, 2022 19:29

codeclimate bot reviewed Jul 12, 2022

View reviewed changes

jvanderhoof force-pushed the adds-oidc-code-workflow branch 2 times, most recently from edab389 to e2c0ecc Compare July 12, 2022 15:46

jvanderhoof marked this pull request as ready for review July 12, 2022 15:47

jvanderhoof requested a review from a team as a code owner July 12, 2022 15:47

mFelgate previously approved these changes Jul 12, 2022

View reviewed changes

telday reviewed Jul 12, 2022

View reviewed changes

jvanderhoof dismissed mFelgate’s stale review via aee117f July 12, 2022 17:44

jvanderhoof force-pushed the adds-oidc-code-workflow branch from 4e85584 to dc77aa3 Compare July 12, 2022 19:20

codeclimate bot reviewed Jul 12, 2022

View reviewed changes

jvanderhoof force-pushed the adds-oidc-code-workflow branch 5 times, most recently from 2c99ef1 to 171eb1f Compare July 14, 2022 16:34

codeclimate bot reviewed Jul 14, 2022

View reviewed changes

jvanderhoof added 3 commits July 14, 2022 12:53

Add support for OIDC Authenticator code-based authorization

fadfb4f

This commit includes all work related to support for the code-based OIDC authentication workflow.

Adds a OIDC Provider endpoint

20aa4f8

Fixes failing tests related to OIDC changes

e0d8366

jvanderhoof force-pushed the adds-oidc-code-workflow branch from 5c3d401 to e0d8366 Compare July 14, 2022 18:54

codeclimate bot reviewed Jul 14, 2022

View reviewed changes

jvanderhoof requested review from mFelgate and telday July 14, 2022 20:48

mFelgate approved these changes Jul 14, 2022

View reviewed changes

jvanderhoof merged commit 1f0ebfe into master Jul 14, 2022

jvanderhoof deleted the adds-oidc-code-workflow branch July 14, 2022 21:33

gl-johnson mentioned this pull request Aug 23, 2022

Update suite.yml for v18.0.0+suite1 release cyberark/conjur-oss-suite-release#250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds OIDC code workflow #2595

Adds OIDC code workflow #2595

jvanderhoof commented Jul 5, 2022 •

edited

Loading

codeclimate bot Jul 12, 2022

codeclimate bot Jul 12, 2022

codeclimate bot Jul 12, 2022

codeclimate bot Jul 12, 2022

codeclimate bot Jul 12, 2022

mFelgate left a comment

telday Jul 12, 2022

telday Jul 12, 2022

jvanderhoof Jul 12, 2022

telday Jul 12, 2022

telday Jul 12, 2022

codeclimate bot Jul 12, 2022

codeclimate bot Jul 12, 2022

codeclimate bot Jul 12, 2022

codeclimate bot Jul 12, 2022

codeclimate bot Jul 12, 2022

codeclimate bot Jul 14, 2022

codeclimate bot Jul 14, 2022

codeclimate bot Jul 14, 2022

codeclimate bot Jul 14, 2022

codeclimate bot Jul 14, 2022

codeclimate bot Jul 14, 2022

codeclimate bot Jul 14, 2022

codeclimate bot Jul 14, 2022

codeclimate bot Jul 14, 2022

codeclimate bot Jul 14, 2022

codeclimate bot commented Jul 14, 2022

mFelgate Jul 14, 2022

mFelgate Jul 14, 2022

		end


		Then('the list of authenticators does not contain the service-id {string}') do \|service_id\|

Adds OIDC code workflow #2595

Adds OIDC code workflow #2595

Conversation

jvanderhoof commented Jul 5, 2022 • edited Loading

Desired Outcome

Implemented Changes

Planned Future Work

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

codeclimate bot Jul 12, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 12, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 12, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 12, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 12, 2022

Choose a reason for hiding this comment

mFelgate left a comment

Choose a reason for hiding this comment

telday Jul 12, 2022

Choose a reason for hiding this comment

telday Jul 12, 2022

Choose a reason for hiding this comment

jvanderhoof Jul 12, 2022

Choose a reason for hiding this comment

telday Jul 12, 2022

Choose a reason for hiding this comment

telday Jul 12, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 12, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 12, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 12, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 12, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 12, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2022

Choose a reason for hiding this comment

codeclimate bot Jul 14, 2022

Choose a reason for hiding this comment

codeclimate bot commented Jul 14, 2022

mFelgate Jul 14, 2022

Choose a reason for hiding this comment

mFelgate Jul 14, 2022

Choose a reason for hiding this comment

jvanderhoof commented Jul 5, 2022 •

edited

Loading