Wrap discovery and callback calls #2933

gl-johnson · 2023-08-31T14:45:31Z

Desired Outcome

Implement the wrapper methods introduced in #2919 and the optional ca-cert config in #2920 to allow custom certs to be provided in the OIDC authenticator config.

Implemented Changes

Added wrapper methods around:
- OAuth discovery + JWKS retrieval
- Identity resolution callback
- OIDC configuration discovery
Updated keycloak OIDC environment to use ca-cert variable instead of container truststore (includes integration tests)

Connected Issue/Story

CNJR-2476

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: CNJR-2309
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

gl-johnson · 2023-09-01T16:06:49Z

app/domain/authentication/o_auth/fetch_provider_keys.rb

        jwks = {
-          keys: @discovered_provider.jwks


@john-odonnell This is still a WIP, but I'm curious if you have thoughts about this: turns out that .jwks makes a separate call to the provider's JWKS uri so the cert needs to be available for that as well.

As a workaround I added an optional jwks flag to the custom Client.discover which updates the lambda to include the .jwks call accordingly. Feels a bit hacky but better than the alternative of adding yet another wrapper imo.

I was going to say:

I think the only downside to this is that we wouldn't be able to take advantage of previously cached discovery responses.

but as it turns out, the DiscoverIdentityProvider class was never using the discovery cache anyways. Maybe this is a future low-hanging advancement.

Otherwise, you're right - feels hacky, but I think this works for now. Makes me think that ditching the underlying OIDC client gem might make all this a bit cleaner, though.

codeclimate · 2023-09-01T16:43:34Z

app/domain/authentication/authn_oidc/v2/client.rb

-          d = -> { discovery_configuration.discover!(provider_uri) }
+          case jwks
+          when false
+            d = -> { discovery_configuration.discover!(provider_uri) }


Authentication::AuthnOidc::V2::Client#self.discover calls 'discovery_configuration.discover!(provider_uri)' 2 times

codeclimate · 2023-09-01T16:43:34Z

app/domain/authentication/authn_oidc/v2/client.rb

        )
-          d = -> { discovery_configuration.discover!(provider_uri) }
+          case jwks


Authentication::AuthnOidc::V2::Client#self.discover is controlled by argument 'jwks'

CHANGELOG.md

john-odonnell · 2023-09-05T18:22:28Z

CHANGELOG.md

@@ -18,6 +18,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
  [cyberark/conjur#2901](https://github.com/cyberark/conjur/pull/2901)

 ### Added
+- Support an optional`ca-cert` variable for providing custom certs/chains for to verify


Suggested change

- Support an optional`ca-cert` variable for providing custom certs/chains for to verify

- Support an optional`ca-cert` variable for providing custom certs/chains to verify

john-odonnell · 2023-09-05T18:29:31Z

app/domain/authentication/o_auth/fetch_provider_keys.rb

        jwks = {
-          keys: @discovered_provider.jwks


I was going to say:

I think the only downside to this is that we wouldn't be able to take advantage of previously cached discovery responses.

but as it turns out, the DiscoverIdentityProvider class was never using the discovery cache anyways. Maybe this is a future low-hanging advancement.

john-odonnell · 2023-09-05T19:07:01Z

app/domain/authentication/o_auth/fetch_provider_keys.rb

        jwks = {
-          keys: @discovered_provider.jwks


Otherwise, you're right - feels hacky, but I think this works for now. Makes me think that ditching the underlying OIDC client gem might make all this a bit cleaner, though.

CHANGELOG.md

codeclimate · 2023-09-05T22:50:36Z

Code Climate has analyzed commit 932bb49 and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Style	1
Complexity	4

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

View more on Code Climate.

john-odonnell

LGTM 👍

gl-johnson force-pushed the oidc-configurable-truststore branch 3 times, most recently from 38701a4 to 362d7cf Compare September 1, 2023 15:53

gl-johnson commented Sep 1, 2023

View reviewed changes

codeclimate bot reviewed Sep 1, 2023

View reviewed changes

john-odonnell reviewed Sep 5, 2023

View reviewed changes

gl-johnson force-pushed the oidc-configurable-truststore branch from 560c526 to 89dd2af Compare September 5, 2023 20:41

codeclimate bot reviewed Sep 5, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

gl-johnson added 2 commits September 5, 2023 16:02

Wrap discovery and callback calls

14a9fc6

Update cukes and wrap jwks call

932bb49

gl-johnson force-pushed the oidc-configurable-truststore branch from 89dd2af to 932bb49 Compare September 5, 2023 22:03

gl-johnson marked this pull request as ready for review September 5, 2023 22:57

gl-johnson requested a review from a team as a code owner September 5, 2023 22:57

john-odonnell approved these changes Sep 6, 2023

View reviewed changes

gl-johnson merged commit b9130ad into master Sep 6, 2023

gl-johnson deleted the oidc-configurable-truststore branch September 6, 2023 14:57

gl-johnson mentioned this pull request Oct 2, 2023

Suite release v1.20.0+suite.1 cyberark/conjur-oss-suite-release#270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap discovery and callback calls #2933

Wrap discovery and callback calls #2933

gl-johnson commented Aug 31, 2023 •

edited

Loading

gl-johnson Sep 1, 2023 •

edited

Loading

john-odonnell Sep 5, 2023

john-odonnell Sep 5, 2023

codeclimate bot Sep 1, 2023

codeclimate bot Sep 1, 2023

john-odonnell Sep 5, 2023

john-odonnell Sep 5, 2023

john-odonnell Sep 5, 2023

codeclimate bot commented Sep 5, 2023

john-odonnell left a comment

	- Support an optional`ca-cert` variable for providing custom certs/chains for to verify
	- Support an optional`ca-cert` variable for providing custom certs/chains to verify

Wrap discovery and callback calls #2933

Wrap discovery and callback calls #2933

Conversation

gl-johnson commented Aug 31, 2023 • edited Loading

Desired Outcome

Implemented Changes

Connected Issue/Story

Definition of Done

Changelog

Test coverage

Documentation

Behavior

Security

gl-johnson Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

john-odonnell Sep 5, 2023

Choose a reason for hiding this comment

john-odonnell Sep 5, 2023

Choose a reason for hiding this comment

codeclimate bot Sep 1, 2023

Choose a reason for hiding this comment

codeclimate bot Sep 1, 2023

Choose a reason for hiding this comment

john-odonnell Sep 5, 2023

Choose a reason for hiding this comment

john-odonnell Sep 5, 2023

Choose a reason for hiding this comment

john-odonnell Sep 5, 2023

Choose a reason for hiding this comment

codeclimate bot commented Sep 5, 2023

john-odonnell left a comment

Choose a reason for hiding this comment

gl-johnson commented Aug 31, 2023 •

edited

Loading

gl-johnson Sep 1, 2023 •

edited

Loading