Authn-JWT Refactor #2734

jvanderhoof · 2023-03-03T18:29:30Z

This work is an effort to port the Authn-JWT functionality to the new Authenticator architecture.

Desired Outcome

Refactor Authn-JWT to use the new authenticator architecture defined in the new OIDC authenticator.

Implemented Changes

The authenticator has been completely ported, with all previous integration tests passing. I've added a Readme which overviews the architecture and interfaces.

Connected Issue/Story

N/A

Definition of Done

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

rafis3

Thanks for this design @jvanderhoof.
It's really visible in your implementation, of how much the different steps of the authenticator logic is organized and easier to understand. I added a few comments and questions, would love for you to take a look.

rafis3 · 2023-03-19T07:38:19Z

app/domain/authentication/Readme.md

+end
+```
+
+which defines the required and optional data as well as the type. As Conjur Variables store values as strings, they type will always be `String`.


Question: while all variables are strings, some of them hold a JSON structure. For example, in the JWT authenticator, the public-keys variable expects a structure like this:

{ "type": "jwks", "value": <JWKS VALUE AS DEFINED BY THE IDENTITY PROVIDER> }

Do you think that the schema should somehow define that as well?

I really like this idea because the public-keys implementation was one of the hardest bits to reverse engineer.

Dry Validations use Dry RB's Schema library, which supports custom types: https://dry-rb.org/gems/dry-schema/1.10/advanced/custom-types/. Schema also supports nested validation: https://dry-rb.org/gems/dry-schema/1.10/nested-data/. I'll need to dig in some more, but it looks likely that we could perform some validation on the provided JSON (if defined). This would give us the ability to guide the end user on the format of their public keys data.

rafis3 · 2023-03-19T07:45:41Z

app/domain/authentication/Readme.md

+#
+# @param [Hash/String] identifier - the role identifier established by the Strategy
+# @param [String] account - request account
+# @param [Array] allowed_roles  - an array of roles with permission to authenticate using this authenticator


I assume that the allowed_roles are given so that the Identity resolver will not need to access the DB directly?
I have a concern that if this authenticator has 30K roles that are allowed to it, such array will be quite a memory consumer and also the copying process of such amount of data will probably not be efficient for the Ruby runtime.

WDYT?

I went back and forth a LOT on this during the design of the code redirect OIDC authentication flow. Ultimately I decided the simplicity of the approach (and critically, removing the DB awareness from the scope of the Strategy) was more important in the short term.

This question hints at a ResolveIdentity improvement I'm trying to figure out how best to implement (and it would solve the above concern). Ultimately, I'd like the ResolveIdentity.call method to be called for each role allowed to authenticate. The call method would return either True or False depending on if the match is successful or not.

This solves two problems. The first, as noted above, is that it allows us to stream a set of roles (resolving the memory "bloat" potential. The second, is it allows us to use annotations for re-mapping email address to users (and enable OIDC users to be kept outside the root policy. Andy's security concern is that multiple matches need to be identified and flagged (we can't exit on the first identified match). "Streaming" the role match allows us to guarantee only one match exists.

I need to find a good way to allow us to perform setup activity (example) before transforming call into a method which takes a block.

rafis3 · 2023-03-19T07:55:15Z

app/domain/authentication/Readme.md

+# @param [String] request_body - authentication request body
+# @param [Hash] parameters - authentication request parameters
+#
+# @return something suitable for identifying a Conjur Role (usually a String or Hash)


If the Strategy is the code that has the authentication logic itself, maybe it needs to respond a decision? Like true if the authentication was successful, false if not. And additionally an optional string, with a reason to why an authentication failed?
I am trying to think if it makes sense for the strategy to be more strict in what it returns, so that the correct logic (authentication decision making) will be implemented into it. I see that in your implementation, it returns the JWT, right? Can you please explain why is that needed?

In the case of the JWT Authenticator, we return the relevant part of the JWT token. Initially I was thinking is is because we perform some extraction in the Identity Resolution phase. In the OIDC Authenticator, we extract the desired lookup attribute from the claim (and return that).

Until you asked the question, I didn't think about the fact that the "pre-filter" logic probably should live in the Strategy (and Strategies need to return a single identity to be mapped to a Conjur Role). This allows us to keep the implementation details (JWT/OIDC/AWS IAM, etc.) in the Strategy.

This change also enables the above refactor (streaming role match) to be significantly simpler.

Scratch part of this above statement. There is key information in the JWT that needs to be available in the Identity Resolution phase (ex annotation matching). Returning the relevant bits of the validated JWT ensures the secondary check that at least one mapping annotation exists on a host can occur.

codeclimate · 2023-03-31T00:48:26Z

app/domain/authentication/authn_jwt/v2/resolve_identity.rb

+          @logger = logger
+        end
+
+        def call(identifier:, allowed_roles:, id: nil)


Method call has 67 lines of code (exceeds 25 allowed). Consider refactoring.

codeclimate · 2023-03-31T00:48:26Z

app/domain/authentication/authn_jwt/v2/data_objects/authenticator_contract.rb

+          end
+
+          # Verify that `ca_cert` has a secret value set if the variable is present
+          rule(:ca_cert, :account, :service_id) do


Similar blocks of code found in 9 locations. Consider refactoring.

codeclimate · 2023-03-31T00:48:26Z

app/domain/authentication/authn_jwt/v2/data_objects/authenticator_contract.rb

+          end
+
+          # Verify that `enforced_claims` has a secret value set if variable is present
+          rule(:enforced_claims, :service_id, :account) do


Similar blocks of code found in 9 locations. Consider refactoring.

codeclimate · 2023-03-31T00:48:26Z

app/domain/authentication/authn_jwt/v2/data_objects/authenticator_contract.rb

+          end
+
+          # Verify that `identity_path` has a secret value set if variable is present
+          rule(:identity_path, :service_id, :account) do


Similar blocks of code found in 9 locations. Consider refactoring.

codeclimate · 2023-03-31T00:48:27Z

app/domain/authentication/authn_jwt/v2/data_objects/authenticator_contract.rb

+          end
+
+          # Verify that `audience` has a secret value set if variable is present
+          rule(:audience, :service_id, :account) do


Similar blocks of code found in 9 locations. Consider refactoring.

codeclimate

The PR diff size of 12507 lines exceeds the maximum allowed for the inline comments feature.

codeclimate

The PR diff size of 12513 lines exceeds the maximum allowed for the inline comments feature.

codeclimate · 2023-04-24T19:37:42Z

Code Climate has analyzed commit 29dcd27 and detected 204 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	114
Duplication	17
Bug Risk	6
Style	66
Security	1

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

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

View more on Code Climate.

This commit migrates the existing authn-jwt functionality to the new Strategy/ResolveIdentity architecture.

This commit updates the previously implemented authn-oidc workflow to adhere to the small changes in interface defined in the authn-jwt refactor

This commit removes the previous authn-jwt implementation and unit tests.

This commit: - Adds support for regional STS endpoints. - Improves log messaging for failed requests. - Adds unit tests for all authenticator functionality.

Conjur is currently the only project which uses the conjur-rack gem. This PR brings it into Conjur to simplify the authorization process.

This commit: - Replaces a call to `git` with the equivalent Ruby call. This prevents a Git permission error. - Addresses all Rubocop warnings.

szh · 2025-02-07T18:58:21Z

@jvanderhoof Can this PR be closed?

jvanderhoof force-pushed the authn-jwt-refactor branch 3 times, most recently from 69a6f88 to 34f63f7 Compare March 10, 2023 18:17

rafis3 reviewed Mar 19, 2023

View reviewed changes

jvanderhoof force-pushed the authn-jwt-refactor branch from 5d0826f to 972a3c5 Compare March 21, 2023 18:27

jvanderhoof force-pushed the authn-jwt-refactor branch from 2413c6e to cae1e05 Compare March 29, 2023 15:26

codeclimate bot reviewed Mar 31, 2023

View reviewed changes

jvanderhoof force-pushed the authn-jwt-refactor branch 2 times, most recently from b8aa76a to 7652fef Compare March 31, 2023 02:31

jvanderhoof force-pushed the authn-jwt-refactor branch from 975e82d to b563eb9 Compare April 13, 2023 14:42

codeclimate bot reviewed Apr 13, 2023

View reviewed changes

jvanderhoof marked this pull request as ready for review April 24, 2023 16:58

jvanderhoof requested a review from a team as a code owner April 24, 2023 16:58

codeclimate bot reviewed Apr 24, 2023

View reviewed changes

jvanderhoof force-pushed the authn-jwt-refactor branch from 29dcd27 to 31a8f22 Compare April 25, 2023 16:11

jvanderhoof added 14 commits May 24, 2023 06:41

Authn-JWT refactor

6cc87ff

This commit migrates the existing authn-jwt functionality to the new Strategy/ResolveIdentity architecture.

Interface updates to Authn-OIDC code redirect authentication flow

f89d381

This commit updates the previously implemented authn-oidc workflow to adhere to the small changes in interface defined in the authn-jwt refactor

Removes previous authn-jwt code

7dcbc92

This commit removes the previous authn-jwt implementation and unit tests.

Fixes Authenticator list endpoint

cdc8895

Refactored for code climate

0baa525

Refactor resolver based on CC

42a48d1

Additional refactoring

6fbf1bd

Additional refactoring

04bbc03

Refactor on status handler

a785b89

Refactor method length

5f03552

Refactor

301b9ce

More refactors

c5bd01c

Refactoring

6a82e51

Refactor

a8cb667

jvanderhoof added 3 commits May 24, 2023 06:49

Refactor

c96a3df

Refactoring

063bcff

CC refactors

e2584f9

jvanderhoof force-pushed the authn-jwt-refactor branch from 5c83479 to e2584f9 Compare May 24, 2023 12:50

jvanderhoof and others added 8 commits May 24, 2023 13:41

CC fixes

986b739

Adds support for authenticating with the AWS STS regional endpoint

2713a2b

This commit: - Adds support for regional STS endpoints. - Improves log messaging for failed requests. - Adds unit tests for all authenticator functionality.

Pulls conjur-rack gem directly into Conjur

d250768

Conjur is currently the only project which uses the conjur-rack gem. This PR brings it into Conjur to simplify the authorization process.

Removes a Git call from gemspec

c515508

This commit: - Replaces a call to `git` with the equivalent Ruby call. This prevents a Git permission error. - Addresses all Rubocop warnings.

additional cc cleanup

c6c5ddd

More fixes

3b55d20

More cc fixes

e600464

Merge branch 'master' into authn-jwt-refactor

314bbc6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authn-JWT Refactor #2734

Authn-JWT Refactor #2734

jvanderhoof commented Mar 3, 2023 •

edited

Loading

rafis3 left a comment

rafis3 Mar 19, 2023

jvanderhoof Mar 20, 2023

rafis3 Mar 19, 2023

jvanderhoof Mar 20, 2023 •

edited

Loading

rafis3 Mar 19, 2023

jvanderhoof Mar 20, 2023

jvanderhoof Mar 20, 2023 •

edited

Loading

codeclimate bot Mar 31, 2023

codeclimate bot Mar 31, 2023

codeclimate bot Mar 31, 2023

codeclimate bot Mar 31, 2023

codeclimate bot Mar 31, 2023

codeclimate bot left a comment

codeclimate bot left a comment

codeclimate bot commented Apr 24, 2023

szh commented Feb 7, 2025

Authn-JWT Refactor #2734

Are you sure you want to change the base?

Authn-JWT Refactor #2734

Conversation

jvanderhoof commented Mar 3, 2023 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

rafis3 left a comment

Choose a reason for hiding this comment

rafis3 Mar 19, 2023

Choose a reason for hiding this comment

jvanderhoof Mar 20, 2023

Choose a reason for hiding this comment

rafis3 Mar 19, 2023

Choose a reason for hiding this comment

jvanderhoof Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

rafis3 Mar 19, 2023

Choose a reason for hiding this comment

jvanderhoof Mar 20, 2023

Choose a reason for hiding this comment

jvanderhoof Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

codeclimate bot Mar 31, 2023

Choose a reason for hiding this comment

codeclimate bot Mar 31, 2023

Choose a reason for hiding this comment

codeclimate bot Mar 31, 2023

Choose a reason for hiding this comment

codeclimate bot Mar 31, 2023

Choose a reason for hiding this comment

codeclimate bot Mar 31, 2023

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot commented Apr 24, 2023

szh commented Feb 7, 2025

jvanderhoof commented Mar 3, 2023 •

edited

Loading

jvanderhoof Mar 20, 2023 •

edited

Loading

jvanderhoof Mar 20, 2023 •

edited

Loading