Authn jwt all signing key settings #2461

sashaCher · 2022-01-12T20:04:49Z

Desired Outcome

JWT Authenticator is able to relate to two new variables ca-cert and public-keys. Both variables are related to signing keys settings area. Authenticator validates validity of variables permutations.
It's not full variables integration yet, so this PR does not change the behavior in this matter.

Implemented Changes

Responsibility for fetching variables values and validating and building signing key settings object is now split between two classes: FetchSigningKeyParametersFromVariables and SigningKeySettingsBuilder.
Both classes are dependency of the CreateSigningKeyProvider class, signing key providers are using signing key settings.

General and non informative InvalidUriConfiguration (CONJ00086E) error is replaced by informative InvalidSigningKeySettings (CONJ00122E) one, see spec/app/domain/authentication/authn-jwt/signing_key/signing_key_settings_builder_spec.rb file for the list of inputs and output error messages.

Connected Issue/Story

ONYX-15140

Definition of Done

Desired outcome is achieved
No integration tests are broken

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/domain/authentication/authn_jwt/signing_key/signing_key_settings_builder.rb

codeclimate · 2022-01-12T22:23:50Z

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

+          return provider_uri if provider_uri
+        end
+
+        def signing_key_settings_type


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

codeclimate · 2022-01-12T22:23:50Z

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

@@ -0,0 +1,52 @@
+module Authentication
+  module AuthnJwt
+    module SigningKey


Authentication::AuthnJwt::SigningKey has no descriptive comment

app/domain/errors.rb

szamir1 · 2022-01-13T11:58:38Z

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

+          return unless !jwks_uri && !provider_uri && !public_keys
+
+          raise Errors::Authentication::AuthnJwt::InvalidSigningKeySettings,
+                "One of #{JWKS_URI_RESOURCE_NAME}, #{PUBLIC_KEYS_RESOURCE_NAME}, and #{PROVIDER_URI_RESOURCE_NAME} have to be defined"


actually this raises a good question if to leave the provider uri, as the customer will see it in the error, and will wonder what it is...
maybe we can remove it from the error?
will verify with Sharon.

I think that we cannot remove it because it may turn troubleshooting to a nightmare...

szamir1 · 2022-01-13T11:59:27Z

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

+          return unless jwks_uri && public_keys &&  provider_uri
+
+          raise Errors::Authentication::AuthnJwt::InvalidSigningKeySettings,
+                "#{JWKS_URI_RESOURCE_NAME}, #{PUBLIC_KEYS_RESOURCE_NAME}, and #{PROVIDER_URI_RESOURCE_NAME} cannot be define simultaneously"


@shulifink can you pls review the text?
these variables cannot be defined togethe.

tzheleznyak · 2022-01-14T12:28:43Z

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

+          return unless jwks_uri && provider_uri
+
+          raise Errors::Authentication::AuthnJwt::InvalidSigningKeySettings,
+                "#{JWKS_URI_RESOURCE_NAME} and #{PROVIDER_URI_RESOURCE_NAME} cannot be define simultaneously"


Cant we move this template to errors.rb?

Actually errors.rb contains only TrackableErrorClass instances so moving plain text messages to it feels me not right.
But I'll move those texts to consts of the same class

The class validates signing key parameters and creates a valid SigningKeySettings object from them Two new parameters cert_store and signing_keys are added to the SigningKeySettings class

codeclimate · 2022-01-14T21:33:29Z

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

@@ -0,0 +1,133 @@
+module Authentication
+  module AuthnJwt
+    module SigningKey


Authentication::AuthnJwt::SigningKey has 8 constants

tzheleznyak · 2022-01-16T08:23:09Z

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

+          check_authenticator_secret_exists: Authentication::Util::CheckAuthenticatorSecretExists.new,
+          fetch_authenticator_secrets: Authentication::Util::FetchAuthenticatorSecrets.new
+        },
+        inputs: %i[authenticator_input]


Maybe consider using delgators for
accout, service_id, authenticator_name

From one point of view let it be.
From other do we have some rule of thumb when to use delegators?

tzheleznyak · 2022-01-16T08:54:12Z

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

+  module AuthnJwt
+    module SigningKey
+
+      NO_SIGNING_KEYS_SOURCE = "One of #{JWKS_URI_RESOURCE_NAME}, #{PUBLIC_KEYS_RESOURCE_NAME}, and #{PROVIDER_URI_RESOURCE_NAME} have to be defined".freeze


Can it sit in consts or errors.rb?

It's too specific from my point of view.
Nobody else is using the same consts so there's no reason to split it to other files.
As I mentioned before errors.rb is a container for exceptions, not for error messages.

tzheleznyak

LGTM

Variables values related to signing keys fetching from one side and values validation and the settings object creation are split to two classes (parameters fetcher and settings builder) both classes are used by CreateSigningKeyProvider class

tzheleznyak

LGTM

codeclimate · 2022-01-16T10:08:28Z

Code Climate has analyzed commit 5fabe9a and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	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 91.2% (0.1% change).

View more on Code Climate.

sashaCher requested a review from a team January 12, 2022 20:04

sashaCher requested a review from a team as a code owner January 12, 2022 20:04

tzheleznyak reviewed Jan 12, 2022

View reviewed changes

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

Base automatically changed from authn-jwt-integrate-signing-key-settings to master January 12, 2022 20:49

codeclimate bot reviewed Jan 12, 2022

View reviewed changes

eladkug reviewed Jan 13, 2022

View reviewed changes

app/domain/errors.rb Show resolved Hide resolved

sashaCher force-pushed the authn-jwt-all-signing-key-settings branch from 8354e5e to 54d0c93 Compare January 13, 2022 08:18

szamir1 reviewed Jan 13, 2022

View reviewed changes

tzheleznyak reviewed Jan 14, 2022

View reviewed changes

sashaCher force-pushed the authn-jwt-all-signing-key-settings branch from 54d0c93 to 3d446df Compare January 14, 2022 20:32

Signing key settings builder class

50d952b

The class validates signing key parameters and creates a valid SigningKeySettings object from them Two new parameters cert_store and signing_keys are added to the SigningKeySettings class

sashaCher force-pushed the authn-jwt-all-signing-key-settings branch from 3d446df to ba79c6f Compare January 14, 2022 20:51

codeclimate bot reviewed Jan 14, 2022

View reviewed changes

tzheleznyak reviewed Jan 16, 2022

View reviewed changes

tzheleznyak previously approved these changes Jan 16, 2022

View reviewed changes

sashaCher dismissed tzheleznyak’s stale review via 5fabe9a January 16, 2022 09:36

sashaCher force-pushed the authn-jwt-all-signing-key-settings branch from ba79c6f to 5fabe9a Compare January 16, 2022 09:36

tzheleznyak approved these changes Jan 16, 2022

View reviewed changes

sashaCher merged commit 7801d79 into master Jan 16, 2022

sashaCher deleted the authn-jwt-all-signing-key-settings branch January 16, 2022 10:14

jtuttle mentioned this pull request Jul 12, 2022

Update suite.yml for v1.17.6+suite.1 release cyberark/conjur-oss-suite-release#247

Merged

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

Authn jwt all signing key settings #2461

Authn jwt all signing key settings #2461

sashaCher commented Jan 12, 2022

codeclimate bot Jan 12, 2022

codeclimate bot Jan 12, 2022

szamir1 Jan 13, 2022

sashaCher Jan 13, 2022

szamir1 Jan 13, 2022

tzheleznyak Jan 14, 2022

sashaCher Jan 14, 2022

codeclimate bot Jan 14, 2022

tzheleznyak Jan 16, 2022

sashaCher Jan 16, 2022

tzheleznyak Jan 16, 2022

sashaCher Jan 16, 2022 •

edited

Loading

tzheleznyak left a comment

tzheleznyak left a comment

codeclimate bot commented Jan 16, 2022

Authn jwt all signing key settings #2461

Authn jwt all signing key settings #2461

Conversation

sashaCher commented Jan 12, 2022

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

codeclimate bot Jan 12, 2022

Choose a reason for hiding this comment

codeclimate bot Jan 12, 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

codeclimate bot Jan 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

sashaCher Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

tzheleznyak left a comment

Choose a reason for hiding this comment

tzheleznyak left a comment

Choose a reason for hiding this comment

codeclimate bot commented Jan 16, 2022

sashaCher Jan 16, 2022 •

edited

Loading