ONYX-11330: Fix cache of signing key #2353

tzheleznyak · 2021-08-03T18:06:16Z

What does this PR do?

Bugfix for cache not working at all for saving signing key.

Return Of FetchCachedSigningKey command class wrapper for signing keys fetchers
Add hash function to authentication_parameters. Algorithem and placemant of this function need to be discussed @sashaCher @nessiLahav
Check for hash of *args in rate_limited_cache and concurrency_limited_cache in a way that don't need to hurt other authenticators. ( If hash function exists use it otherwise keep normal)

What ticket does this PR close?

ONYX-11330

Checklists

Change log

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, and/or there is a follow-on issue to update docs, or
This PR does not require updating any documentation

API Changes

The OpenAPI spec has been updated to meet new API changes (or an issue has been opened), or
The changes in this PR do not affect the Conjur API

app/domain/authentication/authn_jwt/authentication_parameters.rb

app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb

app/domain/authentication/authn_jwt/validate_and_decode/validate_and_decode_token.rb

app/domain/authentication/authn_jwt/vendor_configurations/configuration_jwt_generic_vendor.rb

app/domain/util/concurrency_limited_cache.rb

app/domain/util/rate_limited_cache.rb

nessiLahav

left some comments

app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb

app/domain/authentication/authn_jwt/signing_key/fetch_jwks_uri_signing_key.rb

app/domain/authentication/authn_jwt/signing_key/fetch_provider_uri_signing_key.rb

app/domain/util/rate_limited_cache.rb

app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb

app/domain/authentication/authn_jwt/validate_and_decode/validate_and_decode_token.rb

orenbm · 2021-08-09T09:39:35Z

app/domain/util/concurrency_limited_cache.rb

@@ -66,5 +71,15 @@ def decrease_concurrent_requests
        )
      end
    end
+
+    def cached_key(args)


can you please add a description to this method? what is this cached key? is it cached key or cache key?

Renamed to cache_key and added comment

app/domain/util/concurrency_limited_cache.rb

* ONYX-11330: Fix cache of signing key * Fix cache bug in authn-jwt status flow * Fixes according to PR comments Co-authored-by: Tamir Zheleznyak <[email protected]>

codeclimate · 2021-08-09T12:39:57Z

app/domain/authentication/authn_jwt/validate_and_decode/validate_and_decode_token.rb

@@ -7,19 +7,31 @@ module ValidateAndDecode
      # for the 2nd validation
      ValidateAndDecodeToken ||= CommandClass.new(
        dependencies: {
+          fetch_signing_key_from_cache: ::Util::ConcurrencyLimitedCache.new(


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

codeclimate · 2021-08-09T12:39:57Z

app/domain/authentication/authn_jwt/validate_status.rb

@@ -3,6 +3,16 @@ module AuthnJwt

    ValidateStatus = CommandClass.new(
      dependencies: {
+        fetch_signing_key_from_cache: ::Util::ConcurrencyLimitedCache.new(


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

codeclimate · 2021-08-09T12:39:57Z

app/domain/util/concurrency_limited_cache.rb


-          return @cache[args]
+          return @cache[cache_key]


Util::ConcurrencyLimitedCache#call calls '@cache[cache_key]' 2 times

codeclimate · 2021-08-09T12:39:57Z

app/domain/util/concurrency_limited_cache.rb

@@ -66,5 +71,15 @@ def decrease_concurrent_requests
        )
      end
    end
+
+    def cached_key(args)
+      cache_key = args.key?(:cache_key) ? args.fetch(:cache_key) : args


Util::ConcurrencyLimitedCache#cached_key refers to 'args' more than self (maybe move it to another class?)

codeclimate · 2021-08-09T12:39:57Z

app/domain/util/rate_limited_cache.rb

+    end
+
+    def cached_key(args)
+      if args.key?(:cache_key)


Util::RateLimitedCache#cached_key refers to 'args' more than self (maybe move it to another class?)

codeclimate · 2021-08-09T15:01:06Z

app/domain/authentication/authn_jwt/validate_and_decode/validate_and_decode_token.rb

@@ -7,12 +7,23 @@ module ValidateAndDecode
      # for the 2nd validation
      ValidateAndDecodeToken ||= CommandClass.new(
        dependencies: {
+          fetch_signing_key: ::Util::ConcurrencyLimitedCache.new(


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

codeclimate · 2021-08-09T15:01:06Z

app/domain/authentication/authn_jwt/validate_status.rb

@@ -3,7 +3,17 @@ module AuthnJwt

    ValidateStatus = CommandClass.new(
      dependencies: {
-        create_signing_key_interface: Authentication::AuthnJwt::SigningKey::CreateSigningKeyFactory.new,
+        fetch_signing_key: ::Util::ConcurrencyLimitedCache.new(


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

codeclimate · 2021-08-09T15:01:06Z

app/domain/util/concurrency_limited_cache.rb


-          return @cache[args]
+          return @cache[cache_key]


Util::ConcurrencyLimitedCache#call calls '@cache[cache_key]' 2 times

codeclimate · 2021-08-09T15:01:06Z

app/domain/util/concurrency_limited_cache.rb

+
+    # Function returning cache key to store/retrieve in the cache
+    def cache_key(args)
+      if args.key?(:cache_key)


Util::ConcurrencyLimitedCache#cache_key refers to 'args' more than self (maybe move it to another class?)

codeclimate · 2021-08-09T15:01:06Z

app/domain/util/rate_limited_cache.rb

+    end
+
+    def cached_key(args)
+      if args.key?(:cache_key)


Util::RateLimitedCache#cached_key refers to 'args' more than self (maybe move it to another class?)

codeclimate · 2021-08-09T15:46:44Z

app/domain/authentication/authn_jwt/validate_and_decode/validate_and_decode_token.rb

@@ -7,12 +7,23 @@ module ValidateAndDecode
      # for the 2nd validation
      ValidateAndDecodeToken ||= CommandClass.new(
        dependencies: {
+          fetch_signing_key: ::Util::ConcurrencyLimitedCache.new(


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

codeclimate · 2021-08-09T15:46:44Z

app/domain/authentication/authn_jwt/validate_status.rb

@@ -3,7 +3,17 @@ module AuthnJwt

    ValidateStatus = CommandClass.new(
      dependencies: {
-        create_signing_key_interface: Authentication::AuthnJwt::SigningKey::CreateSigningKeyFactory.new,
+        fetch_signing_key: ::Util::ConcurrencyLimitedCache.new(


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

codeclimate · 2021-08-09T15:46:44Z

app/domain/util/concurrency_limited_cache.rb


-          return @cache[args]
+          return @cache[cache_key]


Util::ConcurrencyLimitedCache#call calls '@cache[cache_key]' 2 times

codeclimate · 2021-08-09T15:46:44Z

app/domain/util/concurrency_limited_cache.rb

+
+    # Function returning cache key to store/retrieve in the cache
+    def cache_key(args)
+      if args.key?(:cache_key)


Util::ConcurrencyLimitedCache#cache_key refers to 'args' more than self (maybe move it to another class?)

codeclimate · 2021-08-09T15:46:45Z

app/domain/util/rate_limited_cache.rb

+    end
+
+    def cached_key(args)
+      if args.key?(:cache_key)


Util::RateLimitedCache#cached_key refers to 'args' more than self (maybe move it to another class?)

tzheleznyak · 2021-08-10T06:59:20Z

CHANGELOG.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

 ## [Unreleased]

+### Fixed


@hilagross @shulifink
Please review changelog message

orenbm · 2021-08-10T07:00:31Z

app/domain/authentication/authn_jwt/signing_key/create_signing_key_factory.rb

@@ -2,7 +2,7 @@ module Authentication
  module AuthnJwt
    module SigningKey
      # Factory that returns the interface implementation of FetchSigningKey
-      CreateSigningKeyFactory ||= CommandClass.new(
+      CreateSigningKeyInterface ||= CommandClass.new(


please match the class name to the file name (or the other way)

What does Interface stand for? does this class create a Signing key object? if so it should be named CreateSigningKey

if you do change this to CreateSigningKey, please make the corresponding changes in the rest of the PR

Its creating a Provider object for signing key so i will rename to CreateSigningKeyProvider

app/domain/authentication/authn_jwt/signing_key/fetch_signing_key_interface.rb

app/domain/authentication/authn_jwt/signing_key/create_signing_key_provider.rb

orenbm · 2021-08-10T07:40:02Z

app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb

+      # :reek:InstanceVariableAssumption
+      FetchCachedSigningKey = CommandClass.new(
+        dependencies: {},
+        inputs: %i[signing_key_interface]


please change signing_key_interface to signing_key_provider in the whole PR

and fix the description above

codeclimate · 2021-08-10T10:21:36Z

app/domain/authentication/authn_jwt/validate_and_decode/validate_and_decode_token.rb

@@ -7,12 +7,23 @@ module ValidateAndDecode
      # for the 2nd validation
      ValidateAndDecodeToken ||= CommandClass.new(
        dependencies: {
+          fetch_signing_key: ::Util::ConcurrencyLimitedCache.new(


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

codeclimate · 2021-08-10T10:21:37Z

app/domain/authentication/authn_jwt/validate_status.rb

@@ -3,7 +3,17 @@ module AuthnJwt

    ValidateStatus = CommandClass.new(
      dependencies: {
-        create_signing_key_interface: Authentication::AuthnJwt::SigningKey::CreateSigningKeyFactory.new,
+        fetch_signing_key: ::Util::ConcurrencyLimitedCache.new(


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

codeclimate · 2021-08-10T10:21:37Z

app/domain/util/rate_limited_cache.rb

+    end
+
+    def cached_key(args)
+      if args.key?(:cache_key)


Util::RateLimitedCache#cached_key refers to 'args' more than self (maybe move it to another class?)

codeclimate · 2021-08-10T10:21:37Z

app/domain/util/concurrency_limited_cache.rb


-          return @cache[args]
+          return @cache[cache_key]


Util::ConcurrencyLimitedCache#call calls '@cache[cache_key]' 2 times

codeclimate · 2021-08-10T10:21:37Z

app/domain/util/concurrency_limited_cache.rb

+
+    # Function returning cache key to store/retrieve in the cache
+    def cache_key(args)
+      if args.key?(:cache_key)


Util::ConcurrencyLimitedCache#cache_key refers to 'args' more than self (maybe move it to another class?)

codeclimate · 2021-08-10T10:21:39Z

Code Climate has analyzed commit 346f9d5 and detected 12 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	2
Style	1
Complexity	9

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 91.0% (0.0% change).

View more on Code Climate.

nessiLahav

LGTM

tzheleznyak requested review from nessiLahav and a team August 3, 2021 18:06

tzheleznyak force-pushed the bugfix-jwt-signing-key-chache branch from 72728a0 to 1a46ded Compare August 4, 2021 10:06

nessiLahav reviewed Aug 5, 2021

View reviewed changes

app/domain/authentication/authn_jwt/authentication_parameters.rb Outdated Show resolved Hide resolved

nessiLahav reviewed Aug 5, 2021

View reviewed changes

app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb Outdated Show resolved Hide resolved

nessiLahav reviewed Aug 5, 2021

View reviewed changes

app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb Outdated Show resolved Hide resolved

nessiLahav reviewed Aug 5, 2021

View reviewed changes

app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb Outdated Show resolved Hide resolved

nessiLahav reviewed Aug 5, 2021

View reviewed changes

app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb Outdated Show resolved Hide resolved

nessiLahav reviewed Aug 5, 2021

View reviewed changes

app/domain/authentication/authn_jwt/validate_and_decode/validate_and_decode_token.rb Show resolved Hide resolved

nessiLahav reviewed Aug 5, 2021

View reviewed changes

app/domain/authentication/authn_jwt/vendor_configurations/configuration_jwt_generic_vendor.rb Outdated Show resolved Hide resolved

nessiLahav reviewed Aug 5, 2021

View reviewed changes

app/domain/authentication/authn_jwt/vendor_configurations/configuration_jwt_generic_vendor.rb Outdated Show resolved Hide resolved

nessiLahav reviewed Aug 5, 2021

View reviewed changes

app/domain/util/concurrency_limited_cache.rb Outdated Show resolved Hide resolved

nessiLahav reviewed Aug 5, 2021

View reviewed changes

app/domain/util/concurrency_limited_cache.rb Outdated Show resolved Hide resolved

nessiLahav reviewed Aug 5, 2021

View reviewed changes

app/domain/util/rate_limited_cache.rb Show resolved Hide resolved

nessiLahav suggested changes Aug 5, 2021

View reviewed changes

tzheleznyak force-pushed the bugfix-jwt-signing-key-chache branch 2 times, most recently from d6a1987 to 951b371 Compare August 8, 2021 09:38

sashaCher reviewed Aug 8, 2021

View reviewed changes

app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb Outdated Show resolved Hide resolved

sashaCher reviewed Aug 8, 2021

View reviewed changes

app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb Outdated Show resolved Hide resolved

tzheleznyak force-pushed the bugfix-jwt-signing-key-chache branch from 951b371 to dcc49eb Compare August 8, 2021 12:33

sashaCher reviewed Aug 8, 2021

View reviewed changes

app/domain/authentication/authn_jwt/signing_key/fetch_jwks_uri_signing_key.rb Show resolved Hide resolved

sashaCher reviewed Aug 8, 2021

View reviewed changes

app/domain/authentication/authn_jwt/signing_key/fetch_provider_uri_signing_key.rb Show resolved Hide resolved

nessiLahav reviewed Aug 8, 2021

View reviewed changes

app/domain/util/rate_limited_cache.rb Outdated Show resolved Hide resolved

tzheleznyak force-pushed the bugfix-jwt-signing-key-chache branch 2 times, most recently from 281a1fa to 64ad246 Compare August 9, 2021 07:22

ONYX-11330: Fix cache of signing key

bdd7a81

tzheleznyak force-pushed the bugfix-jwt-signing-key-chache branch from 64ad246 to bdd7a81 Compare August 9, 2021 08:29

Adjust ValidateAndDecodeToken UTs

c4eedc7

sashaCher force-pushed the bugfix-jwt-signing-key-chache branch from 4a72688 to c4eedc7 Compare August 9, 2021 09:29

orenbm reviewed Aug 9, 2021

View reviewed changes

app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb Outdated Show resolved Hide resolved

orenbm reviewed Aug 9, 2021

View reviewed changes

app/domain/authentication/authn_jwt/validate_and_decode/validate_and_decode_token.rb Outdated Show resolved Hide resolved

orenbm reviewed Aug 9, 2021

View reviewed changes

app/domain/util/concurrency_limited_cache.rb Outdated Show resolved Hide resolved

nessiLahav and others added 2 commits August 9, 2021 15:07

Bugfix status cache (#2356)

2049cee

* ONYX-11330: Fix cache of signing key * Fix cache bug in authn-jwt status flow * Fixes according to PR comments Co-authored-by: Tamir Zheleznyak <[email protected]>

Fix signing_key UTs

7cebf4a

codeclimate bot reviewed Aug 9, 2021

View reviewed changes

tzheleznyak marked this pull request as ready for review August 9, 2021 14:15

tzheleznyak requested a review from a team as a code owner August 9, 2021 14:15

tzheleznyak force-pushed the bugfix-jwt-signing-key-chache branch from c0119ec to fd5c93a Compare August 9, 2021 14:35

codeclimate bot reviewed Aug 9, 2021

View reviewed changes

tzheleznyak force-pushed the bugfix-jwt-signing-key-chache branch from fd5c93a to 8247b0d Compare August 9, 2021 15:21

codeclimate bot reviewed Aug 9, 2021

View reviewed changes

tzheleznyak force-pushed the bugfix-jwt-signing-key-chache branch from 8247b0d to 3ed3fd9 Compare August 10, 2021 06:47

tzheleznyak commented Aug 10, 2021

View reviewed changes

orenbm reviewed Aug 10, 2021

View reviewed changes

tzheleznyak force-pushed the bugfix-jwt-signing-key-chache branch from 3ed3fd9 to f40ffe5 Compare August 10, 2021 07:16

nessiLahav reviewed Aug 10, 2021

View reviewed changes

app/domain/authentication/authn_jwt/signing_key/fetch_signing_key_interface.rb Show resolved Hide resolved

tzheleznyak force-pushed the bugfix-jwt-signing-key-chache branch from f40ffe5 to 673317e Compare August 10, 2021 07:22

orenbm reviewed Aug 10, 2021

View reviewed changes

app/domain/authentication/authn_jwt/signing_key/create_signing_key_provider.rb Show resolved Hide resolved

orenbm reviewed Aug 10, 2021

View reviewed changes

Fix by PR comments

346f9d5

tzheleznyak force-pushed the bugfix-jwt-signing-key-chache branch from 673317e to 346f9d5 Compare August 10, 2021 08:17

orenbm approved these changes Aug 10, 2021

View reviewed changes

codeclimate bot reviewed Aug 10, 2021

View reviewed changes

nessiLahav approved these changes Aug 10, 2021

View reviewed changes

tzheleznyak merged commit 7b5cffd into master Aug 10, 2021

tzheleznyak deleted the bugfix-jwt-signing-key-chache branch August 10, 2021 11:57

jtuttle mentioned this pull request Sep 14, 2021

Update suite.yml for v1.13.1+suite.1 release cyberark/conjur-oss-suite-release#233

Merged

cyberark deleted a comment from orenbm Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ONYX-11330: Fix cache of signing key #2353

ONYX-11330: Fix cache of signing key #2353

tzheleznyak commented Aug 3, 2021 •

edited

Loading

nessiLahav left a comment

orenbm Aug 9, 2021

tzheleznyak Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

codeclimate bot Aug 9, 2021

tzheleznyak Aug 10, 2021

orenbm Aug 10, 2021

orenbm Aug 10, 2021

tzheleznyak Aug 10, 2021

orenbm Aug 10, 2021

orenbm Aug 10, 2021

codeclimate bot Aug 10, 2021

codeclimate bot Aug 10, 2021

codeclimate bot Aug 10, 2021

codeclimate bot Aug 10, 2021

codeclimate bot Aug 10, 2021

codeclimate bot commented Aug 10, 2021

nessiLahav left a comment

		@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

		## [Unreleased]

		### Fixed

ONYX-11330: Fix cache of signing key #2353

ONYX-11330: Fix cache of signing key #2353

Conversation

tzheleznyak commented Aug 3, 2021 • edited Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

API Changes

nessiLahav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 9, 2021

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 Aug 10, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 10, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 10, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 10, 2021

Choose a reason for hiding this comment

codeclimate bot Aug 10, 2021

Choose a reason for hiding this comment

codeclimate bot commented Aug 10, 2021

nessiLahav left a comment

Choose a reason for hiding this comment

tzheleznyak commented Aug 3, 2021 •

edited

Loading