Feature: OIDC refresh token support #2664

john-odonnell · 2022-10-14T00:47:57Z

Desired Outcome

This PR implements refresh token support for Conjur's OIDC V2 authenticator.

API Updates

Endpoint	Method	Input	Response Body	Optional Response Header	Notes
`/authenticate`	GET	`code` `nonce` `code_verifier`	Conjur access token	`X-OIDC-Refresh-Token`	Updated, Backwards-compatible with Refresh-less flows
`/authenticate`	POST	`code` `nonce` `code_verifier`	Conjur access token	`X-OIDC-Refresh-Token`	New
`/authenticate`	POST	`refresh_token` `nonce`	Conjur access token	`X-OIDC-Refresh-Token`	New
`/logout`	POST	`refresh_token` `nonce` `state` `redirect_uri`	URI to OIDC provider session management endpoint		New

Implemented Changes

Connected Issue/Story

CyberArk internal issue link: [ONYX-26608]

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

app/controllers/authenticate_controller.rb

john-odonnell · 2022-10-14T18:52:27Z

Maybe this shouldn't be merged yet. Probably shouldn't start distributing refresh tokens until we can either accept or revoke them.

- AuthenticationHandler class now expects a given Strategy to respond with the authenticating identity and a hash of response headers. These are passed through to the AuthenticationController. - AuthnOidc V2 Client and Strategy classes are updated to meet this expectation. - Includes RSpec and Cucumber tests validating the updated behavior.

codeclimate · 2022-10-19T23:09:52Z

app/controllers/authenticate_controller.rb

@@ -248,6 +249,12 @@ def render_authn_token(authn_token)
    render(content_type => authn_token)
  end

+  def set_headers(headers_h)
+    headers_h.each { |key, value|
+      response.set_header(key, value.to_s) unless key.blank? || value.blank?


AuthenticateController#set_headers refers to 'value' more than self (maybe move it to another class?)

codeclimate · 2022-10-19T23:09:53Z

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

          unless identity.present?
            raise Errors::Authentication::AuthnOidc::IdTokenClaimNotFoundOrEmpty,
                  @authenticator.claim_mapping
          end
-          identity
+
+          return [identity, {}] if refresh_token.nil?


Authentication::AuthnOidc::V2::Strategy#callback performs a nil-check

codeclimate · 2022-10-19T23:09:53Z

app/controllers/authenticate_controller.rb

@@ -248,6 +249,12 @@
    render(content_type => authn_token)
  end

+  def set_headers(headers_h)


Do not prefix writer method names with set_.

codeclimate · 2022-10-19T23:09:53Z

app/controllers/authenticate_controller.rb

@@ -248,6 +249,12 @@
    render(content_type => authn_token)
  end

+  def set_headers(headers_h)
+    headers_h.each { |key, value|


Avoid using {...} for multi-line blocks.

codeclimate · 2022-10-19T23:09:53Z

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

          unless identity.present?
            raise Errors::Authentication::AuthnOidc::IdTokenClaimNotFoundOrEmpty,
                  @authenticator.claim_mapping
          end
-          identity
+
+          return [identity, {}] if refresh_token.nil?


Add empty line after guard clause.

john-odonnell · 2022-10-25T17:25:46Z

app/controllers/authenticate_controller.rb

+    auth_token, headers_h = Authentication::Handler::AuthenticationHandler.new(
      authenticator_type: params[:authenticator]
    ).call(
      parameters: params.to_hash.symbolize_keys,
      request_ip: request.ip
    )


Reconsidering if this is the interface we want. Authn-OIDC V2 is our only auth scheme that needs to set a response header - this means each other auth scheme converted to use the AuthenticationHandler pattern will be returning an empty set of headers.

To avoid passing the response object everywhere, I experimented with passing a lambda as an optional parameter to the call method, so that while other auth schemes can do this:

auth_token = Authentication::Handler::AuthenticationHandler.new( authenticator_type: params[:authenticator] ).call( parameters: params.to_hash.symbolize_keys, request_ip: request.ip )

... Authn-OIDC V2 could do this:

auth_token = Authentication::Handler::AuthenticationHandler.new( authenticator_type: params[:authenticator] ).call( parameters: params.to_hash.symbolize_keys, request_ip: request.ip, set_header: lambda { |key, value| response.set_header(key, value) unless key.blank? or value.blank? } )

... and the set_header block could be used in the OIDC-V2-specific Authentication::AuthnOidc::V2::Strategy class.

Refactor OIDC client to include refresh token exchange

codeclimate · 2022-10-26T17:46:03Z

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

+          decoded_id_token
+        end
+
+        def verify_id_token(decoded_id_token, nonce, refresh: false)


Authentication::AuthnOidc::V2::Client#verify_id_token has boolean parameter 'refresh'

codeclimate · 2022-10-26T17:46:03Z

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

+
+        def verify_id_token(decoded_id_token, nonce, refresh: false)
+          expected_nonce = nonce
+          if refresh && decoded_id_token.raw_attributes['nonce'].nil?


Authentication::AuthnOidc::V2::Client#verify_id_token is controlled by argument 'refresh'

codeclimate · 2022-10-26T17:46:03Z

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

+
+        def verify_id_token(decoded_id_token, nonce, refresh: false)
+          expected_nonce = nonce
+          if refresh && decoded_id_token.raw_attributes['nonce'].nil?


Authentication::AuthnOidc::V2::Client#verify_id_token performs a nil-check

codeclimate · 2022-10-26T17:46:06Z

Code Climate has analyzed commit 127967c and detected 11 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	8
Style	3

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

This pull request will bring the total coverage in the repository to 89.9%.

View more on Code Climate.

Add refresh_token logic and tests to OIDC API

john-odonnell · 2023-09-11T13:08:45Z

Closing. Refresh token support was abandoned in favor of a configurable Conjur access token TTL - see #2683.

codeclimate bot reviewed Oct 14, 2022

View reviewed changes

app/controllers/authenticate_controller.rb Outdated Show resolved Hide resolved

john-odonnell changed the title ~~Authn OIDC V2: Enable refresh token response for auth code exchange~~ Authn OIDC V2: Update auth code exchange for refresh token response Oct 14, 2022

john-odonnell marked this pull request as ready for review October 14, 2022 17:30

john-odonnell requested a review from a team as a code owner October 14, 2022 17:30

john-odonnell marked this pull request as draft October 14, 2022 17:32

john-odonnell force-pushed the oidc-code-with-refresh branch from e8ab69d to d5fff30 Compare October 19, 2022 22:45

codeclimate bot reviewed Oct 19, 2022

View reviewed changes

Refactor OIDC client to include refresh token exchange

265cf02

john-odonnell commented Oct 25, 2022

View reviewed changes

Merge pull request #2667 from cyberark/johnodon-onyx-26609

127967c

Refactor OIDC client to include refresh token exchange

john-odonnell changed the title ~~Authn OIDC V2: Update auth code exchange for refresh token response~~ Feature: OIDC refresh token support Oct 26, 2022

codeclimate bot reviewed Oct 26, 2022

View reviewed changes

gl-johnson and others added 6 commits October 26, 2022 13:09

Add refresh token logic and tests to OIDC API

79eca30

Add decode_and_merge_request_body to auth controller

cce24bf

Avoid re-sending refresh token

c4400b9

Make nonce required

b8dde67

Change OIDC code cucumber scenario to POST

5479948

Merge pull request #2672 from cyberark/update-oidc-endpoint

f7ac424

Add refresh_token logic and tests to OIDC API

john-odonnell closed this Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: OIDC refresh token support #2664

Feature: OIDC refresh token support #2664

john-odonnell commented Oct 14, 2022 •

edited

Loading

john-odonnell commented Oct 14, 2022

codeclimate bot Oct 19, 2022

codeclimate bot Oct 19, 2022

codeclimate bot Oct 19, 2022

codeclimate bot Oct 19, 2022

codeclimate bot Oct 19, 2022

john-odonnell Oct 25, 2022 •

edited

Loading

codeclimate bot Oct 26, 2022

codeclimate bot Oct 26, 2022

codeclimate bot Oct 26, 2022

codeclimate bot commented Oct 26, 2022

john-odonnell commented Sep 11, 2023

Feature: OIDC refresh token support #2664

Feature: OIDC refresh token support #2664

Conversation

john-odonnell commented Oct 14, 2022 • edited Loading

Desired Outcome

API Updates

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

john-odonnell commented Oct 14, 2022

codeclimate bot Oct 19, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 19, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 19, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 19, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 19, 2022

Choose a reason for hiding this comment

john-odonnell Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

codeclimate bot Oct 26, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 26, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 26, 2022

Choose a reason for hiding this comment

codeclimate bot commented Oct 26, 2022

john-odonnell commented Sep 11, 2023

john-odonnell commented Oct 14, 2022 •

edited

Loading

john-odonnell Oct 25, 2022 •

edited

Loading