AuthnOIDC V2: Customizable Token TTL #2683

john-odonnell · 2022-12-06T14:59:54Z

Desired Outcome

Allow users to extend the TTL of tokens distributed by a certain AuthnOIDC V2 instance.

Implemented Changes

When creating an AuthnOIDC V2 instance in policy, add a token-ttl variable in the parent policy to configure a custom token TTL. The value assigned to this variable must be a string representing an ISO 8601 Duration. By default, AuthnOIDC still distributes 8 minute tokens. This can be increased up to a 5 hour global maximum.

- !policy
  id conjur/authn-oidc/${service_id}
  body:
    - !webservice

    - !variable token-ttl
    ...

Includes token-ttl as an OPTIONAL_VARIABLE on the DataObjects::Authenticator class.

Includes MINIMUM_AUTHENTICATION_TOKEN_EXPIRATION constant on the TokenFactory class, in order to eliminate any negative duraitons.

Includes RSpec and Cucumber test cases.

Connected Issue/Story

CyberArk internal issue ID: ONYX-29302 & ONYX-29303

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-12-14T18:02:08Z

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

@@ -47,6 +69,26 @@ def name
          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


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

codeclimate · 2022-12-14T18:02:08Z

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

@@ -53,6 +75,26 @@ def name
          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


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

app/db/repository/authenticator_repository.rb

jvanderhoof · 2022-12-15T15:57:18Z

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

@@ -28,7 +48,8 @@ def initialize(
            redirect_uri: nil,
            name: nil,
            response_type: 'code',
-            provider_scope: nil
+            provider_scope: nil,
+            token_ttl: nil


Instead of passing nil here, what about setting it to the default token TTL (PT8M). That'll allow you to avoid the nil check below.

One question on avoiding the nil check - could someone still explicitly pass in nil? Does that need to be caught?

jvanderhoof · 2022-12-15T16:28:40Z

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

+          # Currently, the TokenFactory class enforces default (same as below)
+          # and maximum (5 hours) TTL values. With these constants defined here,
+          # different authn flavors can be subject to unique restrictions.
+          TOKEN_TTL_MIN = 5.minutes


Instead of forcing the Annotation class to have an awareness of a Conjur token (and that token's TTL), let's push all this into the TokenFactory class. Something like this should allow us us to allow us to refactor the TokenFactory class to something like:

class TokenFactory < Dry::Struct # TODO: this error should be moved to the main `errors.rb` file NoSigningKey = ::Util::ErrorClass.new( "Signing key not found for account '{0}'") attribute :slosilo, ::Types::Any.default{ Slosilo } MAXIMUM_AUTHENTICATION_TOKEN_EXPIRATION = 5.hours MINIMUM_AUTHENTICATION_TOKEN_EXPIRATION = 2.minutes def signing_key(account) slosilo["authn:#{account}".to_sym] || raise(NoSigningKey, account) end def signed_token( account:, username:, host_ttl: Rails.application.config.conjur_config.host_authorization_token_ttl, user_ttl: Rails.application.config.conjur_config.user_authorization_token_ttl ) signing_key(account).issue_jwt( sub: username, exp: Time.now + offset( ttl: username.starts_with?('host/') ? host_ttl : user_ttl ) ) end def offset(ttl:) offset = parse_ttl(ttl: ttl) return MAXIMUM_AUTHENTICATION_TOKEN_EXPIRATION if offset > MAXIMUM_AUTHENTICATION_TOKEN_EXPIRATION return MINIMUM_AUTHENTICATION_TOKEN_EXPIRATION if offset < MINIMUM_AUTHENTICATION_TOKEN_EXPIRATION offset end def parse_ttl(ttl:) # If TTL is an integer return ttl.to_i if ttl.to_i.to_s == ttl.to_s # If TTL is ISO 8601 Duration begin ActiveSupport::Duration.parse(ttl).to_i rescue ActiveSupport::Duration::ISO8601Parser::ParsingError # Attempt to coerce string into integer ttl.to_s.to_i end end end

jvanderhoof · 2022-12-15T16:52:20Z

app/db/repository/authenticator_repository.rb

+                          @data_object.const_get(:OPTIONAL_VARIABLES) +
+                          @data_object.const_get(:OPTIONAL_ANNOTATIONS).map do |arg|
+                            arg.sub('/', '_').to_sym
+                          end


It might be worth considering annotations as an alternative way of defining variables. If we do something like the following:

args_list = {}.tap do |args| webservice.annotations.each do |annotation| args[annotation.name.gsub('\/', '_').to_sym] = annotation.value end args[:account] = account args[:service_id] = service_id variables.each do |variable| next unless variable.secret args[variable.resource_id.split('/')[-1].underscore.to_sym] = variable.secret.value end end

This allows variables to take precedence over annotations and prevents the Authenticator data object from needing to know where its values come from (variable vs annotation)

Using annotations for all configuration points might be a pleasant UX. In this case, should we disallow storing the client_secret as an annotation, and force it to be a variable? May not want to tie permissions over the client secret to permissions over the webservice.

@john-odonnell & I talked about this in our implementation security review. We ended up down several rabbit holes of impacts on UX that indicate there's likely a deeper discussion to be had here that's more than just the 2 of us, including things like additional complexity in mixed config (knowing which takes precedence and determining which is active when trying to debug malfunctioning policy, for example) and not encouraging/allowing customers to put secrets in policy (like here, if the OIDC client-secret variable was instead a hardcoded value in an annotation.)

We also spent some time talking about how some config pieces come from different people (an Okta admin vs. a Conjur admin, for example) who might have different degrees of comfort/familiarity and how some of those values might actually be better off with a little more friction in changing, while others could use less friction. For example, maybe changing the provider URL for an OIDC provider should be a little harder than rotating a client secret for that provider. We may want to make it so that some changes can get multiple people involved in a change (like we make PRs need a review from someone else) to avoid a single rogue person causing problems.

There's a lot to discuss here and a broader group needed for participating in this discussion, so for now we went with making TTL a variable as well for consistency. We'd like to have that bigger discussion though and after that it might make sense ot revisit this piece too.

jvanderhoof · 2022-12-15T16:56:19Z

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

+
+            begin
+              duration = ActiveSupport::Duration.parse(@token_ttl) if @token_ttl
+            rescue ActiveSupport::Duration::ISO8601Parser::ParsingError


I'm in the process of adding a "Status" check for the new OIDC. I think we should move the ISO8601 check into that verification.

Good idea, the Status check should definitely be able to verify the custom TTL config. Given that the Status and TokenFactory classes both need to decode ISO 8601 strings, does it make sense to keep that conversion in a method on the Authenticator class?

Related comment from above:

Instead of forcing the Annotation class to have an awareness of a Conjur token (and that token's TTL), let's push all this into the TokenFactory class.

The Authenticator class needs some concept of a custom TTL, even if only in its ISO 8601 form. Maybe it's best to move the max/min range into the TokenFactory, but keep ISO 8601 parsing as part of the Authenticator class.

Given the date/time edge cases we were talking about, @john-odonnell, if we do have to add code to handle those, having one place do the processing seems like a really good idea too.

andytinkham · 2022-12-16T21:43:17Z

John & I did an implementation review. I'm in agreement on this feaure's implementation.

codeclimate · 2022-12-19T16:03:06Z

app/domain/token_factory.rb

  end

+  def parse_ttl(ttl:)
+    return ttl.to_i if ttl.to_i.to_s == ttl.to_s


TokenFactory#parse_ttl calls 'ttl.to_i' 2 times

codeclimate · 2022-12-19T16:03:07Z

app/domain/token_factory.rb

  end

+  def parse_ttl(ttl:)
+    return ttl.to_i if ttl.to_i.to_s == ttl.to_s


TokenFactory#parse_ttl calls 'ttl.to_s' 2 times

codeclimate · 2022-12-19T16:03:07Z

app/domain/token_factory.rb

@@ -28,9 +29,16 @@
  end

  def offset(ttl:)
-    return ttl.to_i if ttl.to_i < MAXIMUM_AUTHENTICATION_TOKEN_EXPIRATION
+    offset = parse_ttl(ttl: ttl)
+    return MAXIMUM_AUTHENTICATION_TOKEN_EXPIRATION if offset > MAXIMUM_AUTHENTICATION_TOKEN_EXPIRATION


TokenFactory#offset refers to 'offset' more than self (maybe move it to another class?)

codeclimate · 2022-12-19T16:03:07Z

app/domain/token_factory.rb

  end

+  def parse_ttl(ttl:)


TokenFactory#parse_ttl doesn't depend on instance state (maybe move it to another class?)

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

gl-johnson

Dropped a couple minor questions but otherwise LGTM. Happy to approve when no longer a draft

gl-johnson · 2022-12-20T16:45:03Z

app/domain/token_factory.rb

  end

+  def parse_ttl(ttl:)
+    return ttl.to_i if ttl.to_i.to_s == ttl.to_s


Just curious about the logic here. Why does the Duration need to be validated against the to_s output, and why we can't just return the latter result: ttl.to_s.to_i if that's the case?

This is from Jason's review - I added some comments back in for clarity. Essentially it's an extra bit of input cleansing to force strings into integers if the signed_token method gets string input for a custom TTL.

def parse_ttl(ttl:) # If TTL is an integer, return it return ttl.to_i if ttl.to_i.to_s == ttl.to_s # Attempt to coerce a string into integer ttl.to_s.to_i end

gl-johnson · 2022-12-20T18:15:10Z

app/domain/authentication/handler/authentication_handler.rb

@@ -78,7 +78,8 @@ def call(parameters:, request_ip:)

        TokenFactory.new.signed_token(
          account: parameters[:account],
-          username: role.role_id.split(':').last
+          username: role.role_id.split(':').last,
+          user_ttl: authenticator.token_ttl


Is it safe to assume token_ttl will always be defined here? I understand this handler class currently only supports the OIDC/pkce authenticators which have been updated accordingly but wasn't sure if the intent was to generalize it for other cases

The intent is definitely to extend this pattern to other authenticators, so you're right - currently this is a safe assumption, but that may not always hold true. I think this may be acceptable - as long as we have one flavor of Authenticator that has to report a customized token_ttl, I'm not sure if this is avoidable.

AuthnOIDC definitions in policy now accept a 'token-ttl' variable as configuration. When this variable is set to a valid ISO 8601 Duration, the AuthnOIDC instance will distribute Conjur access tokens with a TTL matching that value.

codeclimate · 2022-12-22T19:33:49Z

app/domain/token_factory.rb

  end

+  def parse_ttl(ttl:)
+    # If TTL is an integer, return it
+    return ttl.to_i if ttl.to_i.to_s == ttl.to_s


Add empty line after guard clause.

codeclimate · 2022-12-22T19:33:51Z

Code Climate has analyzed commit 3bc07ed and detected 7 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	6
Style	1

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

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

View more on Code Climate.

gl-johnson

Thanks for the clarifications. LGTM!

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

Base automatically changed from pkce-support to master December 9, 2022 17:39

john-odonnell force-pushed the oidc-extend-token-ttl branch from e022d8c to f098e9a Compare December 14, 2022 01:19

codeclimate bot reviewed Dec 14, 2022

View reviewed changes

jvanderhoof reviewed Dec 15, 2022

View reviewed changes

john-odonnell force-pushed the oidc-extend-token-ttl branch from d86ad58 to 3d5cbaa Compare December 16, 2022 21:26

john-odonnell force-pushed the oidc-extend-token-ttl branch from 3d5cbaa to fd3a0e5 Compare December 19, 2022 15:36

codeclimate bot reviewed Dec 19, 2022

View reviewed changes

john-odonnell changed the title ~~OIDC V2 Extended Token TTL POC~~ OIDC V2: Customizable Token TTL Dec 19, 2022

john-odonnell changed the title ~~OIDC V2: Customizable Token TTL~~ AuthnOIDC V2: Customizable Token TTL Dec 19, 2022

john-odonnell force-pushed the oidc-extend-token-ttl branch from fd3a0e5 to 634b5a7 Compare December 20, 2022 17:42

gl-johnson reviewed Dec 20, 2022

View reviewed changes

john-odonnell force-pushed the oidc-extend-token-ttl branch from 634b5a7 to 513659e Compare December 22, 2022 19:00

AuthnOIDC V2: Allow custom TTLs on Conjur tokens

3bc07ed

AuthnOIDC definitions in policy now accept a 'token-ttl' variable as configuration. When this variable is set to a valid ISO 8601 Duration, the AuthnOIDC instance will distribute Conjur access tokens with a TTL matching that value.

john-odonnell force-pushed the oidc-extend-token-ttl branch from 513659e to 3bc07ed Compare December 22, 2022 19:08

john-odonnell marked this pull request as ready for review December 22, 2022 19:09

john-odonnell requested a review from a team as a code owner December 22, 2022 19:09

codeclimate bot reviewed Dec 22, 2022

View reviewed changes

gl-johnson approved these changes Dec 23, 2022

View reviewed changes

john-odonnell merged commit b4f1652 into master Dec 23, 2022

john-odonnell deleted the oidc-extend-token-ttl branch December 23, 2022 19:27

john-odonnell mentioned this pull request Mar 3, 2023

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

Merged

13 tasks

john-odonnell mentioned this pull request May 25, 2023

OIDC post endpoint cyberark/conjur-api-ruby#214

Closed

13 tasks

This was referenced Sep 11, 2023

Add token revocation and session management to AuthnOIDC #2674

Closed

Feature: OIDC refresh token support #2664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AuthnOIDC V2: Customizable Token TTL #2683

AuthnOIDC V2: Customizable Token TTL #2683

john-odonnell commented Dec 6, 2022 •

edited

Loading

codeclimate bot Dec 14, 2022

codeclimate bot Dec 14, 2022

jvanderhoof Dec 15, 2022

andytinkham Dec 16, 2022

jvanderhoof Dec 15, 2022

jvanderhoof Dec 15, 2022

john-odonnell Dec 15, 2022

andytinkham Dec 16, 2022 •

edited

Loading

jvanderhoof Dec 15, 2022

john-odonnell Dec 16, 2022

andytinkham Dec 16, 2022 •

edited

Loading

andytinkham commented Dec 16, 2022

codeclimate bot Dec 19, 2022

codeclimate bot Dec 19, 2022

codeclimate bot Dec 19, 2022

codeclimate bot Dec 19, 2022

gl-johnson left a comment

gl-johnson Dec 20, 2022

john-odonnell Dec 22, 2022

gl-johnson Dec 20, 2022

john-odonnell Dec 20, 2022

codeclimate bot Dec 22, 2022

codeclimate bot commented Dec 22, 2022

gl-johnson left a comment

AuthnOIDC V2: Customizable Token TTL #2683

AuthnOIDC V2: Customizable Token TTL #2683

Conversation

john-odonnell commented Dec 6, 2022 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

codeclimate bot Dec 14, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 14, 2022

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

andytinkham Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andytinkham Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

andytinkham commented Dec 16, 2022

codeclimate bot Dec 19, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 19, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 19, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 19, 2022

Choose a reason for hiding this comment

gl-johnson 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

codeclimate bot Dec 22, 2022

Choose a reason for hiding this comment

codeclimate bot commented Dec 22, 2022

gl-johnson left a comment

Choose a reason for hiding this comment

john-odonnell commented Dec 6, 2022 •

edited

Loading

andytinkham Dec 16, 2022 •

edited

Loading

andytinkham Dec 16, 2022 •

edited

Loading