Fix by PR comments

cyberark · Aug 10, 2021 · 346f9d5 · 346f9d5
1 parent 7cebf4a
commit 346f9d5
Show file tree

Hide file tree

Showing 12 changed files with 136 additions and 200 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 
+### Fixed
+- Fix bug of cache not working in authn jwt. [ONYX-11330](https://ca-il-jira.il.cyber-ark.com:8443/browse/ONYX-11330)
+
 ## [1.13.0] - 2021-07-29
 
 ### Added

diff --git a/...signing_key/create_signing_key_factory.rb → ...igning_key/create_signing_key_provider.rb b/...signing_key/create_signing_key_factory.rb → ...igning_key/create_signing_key_provider.rb
@@ -2,7 +2,7 @@ module Authentication
   module AuthnJwt
     module SigningKey
       # Factory that returns the interface implementation of FetchSigningKey
-      CreateSigningKeyFactory ||= CommandClass.new(
+      CreateSigningKeyProvider ||= CommandClass.new(
         dependencies: {
           fetch_provider_uri_signing_key_class: Authentication::AuthnJwt::SigningKey::FetchProviderUriSigningKey,
           fetch_jwks_uri_signing_key_class: Authentication::AuthnJwt::SigningKey::FetchJwksUriSigningKey,
@@ -13,12 +13,12 @@ module SigningKey
         def call
           @logger.debug(LogMessages::Authentication::AuthnJwt::SelectingSigningKeyInterface.new)
           validate_key_configuration
-          create_signing_key
+          create_signing_key_provider
         end
 
         private
 
-        def create_signing_key
+        def create_signing_key_provider
           if provider_uri_has_valid_configuration?
             @logger.info(
               LogMessages::Authentication::AuthnJwt::SelectedSigningKeyInterface.new(PROVIDER_URI_INTERFACE_NAME)

diff --git a/app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb b/app/domain/authentication/authn_jwt/signing_key/fetch_cached_signing_key.rb
@@ -6,20 +6,18 @@ module SigningKey
       # fetch_signing_key it is extreme case that error need to be raised so it can be investigated so reek will ignore
       # this.
       # :reek:InstanceVariableAssumption
-      class FetchCachedSigningKey
-        def initialize(logger: Rails.logger)
-          @logger = logger
-        end
-
-        def call(signing_key_interface:)
-          @signing_key_interface = signing_key_interface
+      FetchCachedSigningKey = CommandClass.new(
+        dependencies: {},
+        inputs: %i[signing_key_provider]
+      ) do
+        def call
           fetch_signing_key
         end
 
         private
 
         def fetch_signing_key
-          @signing_key_interface.fetch_signing_key
+          @signing_key_provider.fetch_signing_key
         end
       end
     end

diff --git a/app/domain/authentication/authn_jwt/validate_and_decode/validate_and_decode_token.rb b/app/domain/authentication/authn_jwt/validate_and_decode/validate_and_decode_token.rb
@@ -7,7 +7,7 @@ module ValidateAndDecode
       # for the 2nd validation
       ValidateAndDecodeToken ||= CommandClass.new(
         dependencies: {
-          fetch_signing_key_from_cache: ::Util::ConcurrencyLimitedCache.new(
+          fetch_signing_key: ::Util::ConcurrencyLimitedCache.new(
             ::Util::RateLimitedCache.new(
               ::Authentication::AuthnJwt::SigningKey::FetchCachedSigningKey.new,
               refreshes_per_interval: CACHE_REFRESHES_PER_INTERVAL,
@@ -20,7 +20,7 @@ module ValidateAndDecode
           verify_and_decode_token: ::Authentication::Jwt::VerifyAndDecodeToken.new,
           fetch_jwt_claims_to_validate: ::Authentication::AuthnJwt::ValidateAndDecode::FetchJwtClaimsToValidate.new,
           get_verification_option_by_jwt_claim: ::Authentication::AuthnJwt::ValidateAndDecode::GetVerificationOptionByJwtClaim.new,
-          signing_key_interface_factory: ::Authentication::AuthnJwt::SigningKey::CreateSigningKeyFactory.new,
+          create_signing_key_provider: ::Authentication::AuthnJwt::SigningKey::CreateSigningKeyProvider.new,
           logger: Rails.logger
         },
         inputs: %i[authentication_parameters]
@@ -31,7 +31,6 @@ module ValidateAndDecode
         def call
           @logger.debug(LogMessages::Authentication::AuthnJwt::ValidatingToken.new)
           validate_token_exists
-          fetch_signing_key_interface
           fetch_signing_key
           validate_signature
           fetch_jwt_claims_to_validate
@@ -43,9 +42,8 @@ def call
 
         private
 
-        # Don't do memoization here because otherwise interface won't change between different requests
-        def fetch_signing_key_interface
-          @signing_key_interface = @signing_key_interface_factory.call(
+        def signing_key_provider
+          @signing_key_provider ||= @create_signing_key_provider.call(
             authentication_parameters: @authentication_parameters
           )
         end
@@ -55,10 +53,10 @@ def validate_token_exists
         end
 
         def fetch_signing_key(force_read: false)
-          @jwks = @fetch_signing_key_from_cache.call(
+          @jwks = @fetch_signing_key.call(
             refresh: force_read,
-            cache_key: @signing_key_interface.signing_key_uri,
-            signing_key_interface: @signing_key_interface
+            cache_key: signing_key_provider.signing_key_uri,
+            signing_key_provider: signing_key_provider
           )
           @logger.debug(LogMessages::Authentication::AuthnJwt::SigningKeysFetchedFromCache.new)
         end

diff --git a/app/domain/authentication/authn_jwt/validate_status.rb b/app/domain/authentication/authn_jwt/validate_status.rb
@@ -3,7 +3,7 @@ module AuthnJwt
 
     ValidateStatus = CommandClass.new(
       dependencies: {
-        fetch_signing_key_from_cache: ::Util::ConcurrencyLimitedCache.new(
+        fetch_signing_key: ::Util::ConcurrencyLimitedCache.new(
           ::Util::RateLimitedCache.new(
             ::Authentication::AuthnJwt::SigningKey::FetchCachedSigningKey.new,
             refreshes_per_interval: CACHE_REFRESHES_PER_INTERVAL,
@@ -13,7 +13,7 @@ module AuthnJwt
           max_concurrent_requests: CACHE_MAX_CONCURRENT_REQUESTS,
           logger: Rails.logger
         ),
-        create_signing_key_interface: Authentication::AuthnJwt::SigningKey::CreateSigningKeyFactory.new,
+        create_signing_key_provider: Authentication::AuthnJwt::SigningKey::CreateSigningKeyProvider.new,
         fetch_issuer_value: Authentication::AuthnJwt::ValidateAndDecode::FetchIssuerValue.new,
         fetch_audience_value: Authentication::AuthnJwt::ValidateAndDecode::FetchAudienceValue.new,
         fetch_enforced_claims: Authentication::AuthnJwt::RestrictionValidation::FetchEnforcedClaims.new,
@@ -144,15 +144,15 @@ def webservice
       end
 
       def validate_signing_key
-        @fetch_signing_key_from_cache.call(
-          cache_key: signing_key_interface.signing_key_uri,
-          signing_key_interface: signing_key_interface
+        @fetch_signing_key.call(
+          cache_key: signing_key_provider.signing_key_uri,
+          signing_key_provider: signing_key_provider
         )
         @logger.debug(LogMessages::Authentication::AuthnJwt::ValidatedSigningKeyConfiguration.new)
       end
 
-      def signing_key_interface
-        @signing_key_interface ||= @create_signing_key_interface.call(
+      def signing_key_provider
+        @signing_key_provider ||= @create_signing_key_provider.call(
           authentication_parameters: authentication_parameters
         )
       end

diff --git a/...domain/authentication/authn_jwt/vendor_configurations/configuration_jwt_generic_vendor.rb b/...domain/authentication/authn_jwt/vendor_configurations/configuration_jwt_generic_vendor.rb
@@ -82,29 +82,13 @@ def identity_provider
           )
         end
 
-        def fetch_singing_key_interface
-          @fetch_singing_key_interface ||= create_signing_key_interface.call(
-            authentication_parameters: @authentication_parameters
-          )
-        end
-
         def create_identity_provider
           @logger.debug(LogMessages::Authentication::AuthnJwt::CreateJwtIdentityProviderInstance.new)
           @create_identity_provider ||= @create_identity_provider
           @logger.debug(LogMessages::Authentication::AuthnJwt::CreatedJwtIdentityProviderInstance.new)
           @create_identity_provider
         end
 
-        def fetch_signing_key_interface
-          @fetch_signing_key_interface ||= create_signing_key_interface.call(
-            authentication_parameters: @authentication_parameters
-          )
-        end
-
-        def create_signing_key_interface
-          @create_signing_key_interface ||= Authentication::AuthnJwt::SigningKey::CreateSigningKeyFactory.new
-        end
-
         def restrictions_from_annotations
           @restrictions_from_annotations ||= Authentication::ResourceRestrictions::GetServiceSpecificRestrictionFromAnnotation.new
         end

diff --git a/app/domain/util/concurrency_limited_cache.rb b/app/domain/util/concurrency_limited_cache.rb
@@ -25,7 +25,7 @@ def initialize(
     # not in case before the flow runs and it can cause unexpected behaviour so reek will ignore this.
     # reek:DuplicateMethodCall
     def call(**args)
-      cache_key = cached_key(args)
+      cache_key = cache_key(args)
       @concurrency_mutex.synchronize do
         if @concurrent_requests >= @max_concurrent_requests
           @logger.debug(
@@ -72,13 +72,18 @@ def decrease_concurrent_requests
       end
     end
 
-    def cached_key(args)
-      cache_key = args.key?(:cache_key) ? args.fetch(:cache_key) : args
-      @logger.debug(
-        LogMessages::Util::ConcurrencyLimitedCacheKeyRetrieved.new(
-          cache_key
+    # Function returning cache key to store/retrieve in the cache
+    def cache_key(args)
+      if args.key?(:cache_key)
+        cache_key = args.fetch(:cache_key)
+        @logger.debug(
+          LogMessages::Util::ConcurrencyLimitedCacheKeyRetrieved.new(
+            cache_key
+          )
         )
-      )
+      else
+        cache_key = args
+      end
       cache_key
     end
   end

diff --git a/app/domain/util/rate_limited_cache.rb b/app/domain/util/rate_limited_cache.rb
@@ -72,14 +72,14 @@ def cached_key(args)
       if args.key?(:cache_key)
         cache_key = args.fetch(:cache_key)
         args.delete(:cache_key)
-        cache_key
-      end
-      cache_key = args
-      @logger.debug(
-        LogMessages::Util::RateLimitedCacheKeyRetrieved.new(
-          cache_key
+        @logger.debug(
+          LogMessages::Util::RateLimitedCacheKeyRetrieved.new(
+            cache_key
+          )
         )
-      )
+      else
+        cache_key = args
+      end
       cache_key
     end
 

diff --git a/cucumber/authenticators_jwt/features/authn_jwt_fetch_signing_key.feature b/cucumber/authenticators_jwt/features/authn_jwt_fetch_signing_key.feature
@@ -3,6 +3,7 @@ Feature: JWT Authenticator - Fetch signing key
   In this feature we define a JWT authenticator with various signing key
   configurations.
 
+  @sanity
   Scenario: ONYX-8702: provider-uri is configured with valid value
     Given I load a policy:
     """
@@ -88,59 +89,6 @@ Feature: JWT Authenticator - Fetch signing key
     CONJ00011E Failed to discover Identity Provider (Provider URI: 'unknown-host.com')
     """
 
-  Scenario: ONYX-8703: jwks uri configured with correct value
-    Given I load a policy:
-    """
-    - !policy
-      id: conjur/authn-jwt/raw
-      body:
-      - !webservice
-
-      - !variable
-        id: jwks-uri
-
-      - !variable
-        id: token-app-property
-
-      - !group hosts
-
-      - !permit
-        role: !group hosts
-        privilege: [ read, authenticate ]
-        resource: !webservice
-
-    - !host
-      id: myapp
-      annotations:
-        authn-jwt/raw/project-id: myproject
-
-    - !grant
-      role: !group conjur/authn-jwt/raw/hosts
-      member: !host myapp
-    """
-    And I am the super-user
-    And I initialize remote JWKS endpoint with file "authn-jwt-fetch-signing-key" and alg "RS256"
-    And I successfully set authn-jwt "jwks-uri" variable value to "http://jwks_py:8090/authn-jwt-fetch-signing-key/RS256" in service "raw"
-    And I have a "variable" resource called "test-variable"
-    And I successfully set authn-jwt "token-app-property" variable to value "host"
-    And I permit host "myapp" to "execute" it
-    And I add the secret value "test-secret" to the resource "cucumber:variable:test-variable"
-    And I am using file "authn-jwt-fetch-signing-key" and alg "RS256" for remotely issue token:
-    """
-    {
-      "host":"myapp",
-      "project-id": "myproject"
-    }
-    """
-    And I save my place in the log file
-    When I authenticate via authn-jwt with the JWT token
-    Then host "myapp" has been authorized by Conjur
-    And I successfully GET "/secrets/cucumber/variable/test-variable" with authorized user
-    And The following appears in the log after my savepoint:
-    """
-    cucumber:host:myapp successfully authenticated with authenticator authn-jwt service cucumber:webservice:conjur/authn-jwt/raw
-    """
-
   Scenario: ONYX-8705: jwks uri configured with bad value
     Given I load a policy:
     """

diff --git a/...ng_key/create_signing_key_factory_spec.rb → ...g_key/create_signing_key_provider_spec.rb b/...ng_key/create_signing_key_factory_spec.rb → ...g_key/create_signing_key_provider_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe('Authentication::AuthnJwt::SigningKey::CreateSigningKeyInterface') do
+RSpec.describe('Authentication::AuthnJwt::SigningKey::CreateSigningKeyProvider') do
 
   let(:authenticator_name) { 'authn-jwt' }
   let(:service_id) { "my-service" }
@@ -70,11 +70,11 @@
   #   )(   ) _ (  )__)     )(   )__) \__ \  )(  \__ \
   #  (__) (_) (_)(____)   (__) (____)(___/ (__) (___/
 
-  context "CreateSigningKeyInterface " do
+  context "CreateSigningKeyProvider " do
     context "'jwks-uri' and 'provider-uri' exist" do
 
       subject do
-        ::Authentication::AuthnJwt::SigningKey::CreateSigningKeyFactory.new(
+        ::Authentication::AuthnJwt::SigningKey::CreateSigningKeyProvider.new(
           fetch_provider_uri_signing_key_class: mocked_fetch_exists_provider_uri,
           fetch_jwks_uri_signing_key_class: mocked_fetch_exists_jwks_uri,
           logger: mocked_logger
@@ -91,7 +91,7 @@
     context "'jwks-uri' and 'provider-uri' does not exist" do
 
       subject do
-        ::Authentication::AuthnJwt::SigningKey::CreateSigningKeyFactory.new(
+        ::Authentication::AuthnJwt::SigningKey::CreateSigningKeyProvider.new(
           fetch_provider_uri_signing_key_class: mocked_fetch_non_exists_provider_uri,
           fetch_jwks_uri_signing_key_class: mocked_fetch_non_exists_jwks_uri,
           logger: mocked_logger
@@ -108,7 +108,7 @@
     context "'jwks-uri' exits and 'provider-uri' does not exists" do
 
       subject do
-        ::Authentication::AuthnJwt::SigningKey::CreateSigningKeyFactory.new(
+        ::Authentication::AuthnJwt::SigningKey::CreateSigningKeyProvider.new(
           fetch_provider_uri_signing_key_class: mocked_fetch_non_exists_provider_uri,
           fetch_jwks_uri_signing_key_class: mocked_fetch_exists_jwks_uri,
           logger: mocked_logger
@@ -125,7 +125,7 @@
     context "'jwks-uri' does not exists and 'provider-uri' exist" do
 
       subject do
-        ::Authentication::AuthnJwt::SigningKey::CreateSigningKeyFactory.new(
+        ::Authentication::AuthnJwt::SigningKey::CreateSigningKeyProvider.new(
           fetch_provider_uri_signing_key_class: mocked_fetch_exists_provider_uri,
           fetch_jwks_uri_signing_key_class: mocked_fetch_non_exists_jwks_uri,
           logger: mocked_logger