Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Integrate ca certificate flow #2462

Merged
merged 1 commit into from
Jan 16, 2022
Merged

Integrate ca certificate flow #2462

merged 1 commit into from
Jan 16, 2022

Conversation

sashaCher
Copy link
Contributor

@sashaCher sashaCher commented Jan 13, 2022 •
edited
Loading

Desired Outcome

Enable JWT Authenticator to work with new ca-cert variable.

Implemented Changes

  • CreateSigningKeyProvider class passes cert_store parameter into FetchJwksUriSigningKey instance
  • Enable relevant integration tests

Connected Issue/Story

ONYX-15872

Definition of Done

  • Desired outcome achieved
  • All integration tests are passed

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

@sashaCher sashaCher changed the base branch from master to authn-jwt-all-signing-key-settings January 13, 2022 11:29
@sashaCher sashaCher force-pushed the integrate-ca-certificate-flow branch 2 times, most recently from b2d4497 to 26cf81e Compare January 13, 2022 13:21
@sashaCher sashaCher requested a review from a team January 13, 2022 13:28
@sashaCher sashaCher force-pushed the integrate-ca-certificate-flow branch 4 times, most recently from 8ffc18e to a71fab3 Compare January 13, 2022 19:56
tzheleznyak
tzheleznyak previously approved these changes Jan 14, 2022
View reviewed changes
Copy link
Contributor

@tzheleznyak tzheleznyak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sashaCher sashaCher force-pushed the authn-jwt-all-signing-key-settings branch from 54d0c93 to 3d446df Compare January 14, 2022 20:32
@sashaCher sashaCher force-pushed the integrate-ca-certificate-flow branch from a71fab3 to a9f650a Compare January 14, 2022 20:38
@sashaCher sashaCher force-pushed the authn-jwt-all-signing-key-settings branch from 3d446df to ba79c6f Compare January 14, 2022 20:51
@sashaCher sashaCher force-pushed the integrate-ca-certificate-flow branch from a9f650a to e39e656 Compare January 14, 2022 20:54
@sashaCher sashaCher force-pushed the authn-jwt-all-signing-key-settings branch from ba79c6f to 5fabe9a Compare January 16, 2022 09:36
Base automatically changed from authn-jwt-all-signing-key-settings to master January 16, 2022 10:14
@sashaCher sashaCher force-pushed the integrate-ca-certificate-flow branch from e39e656 to 88b07eb Compare January 16, 2022 10:18
@sashaCher sashaCher marked this pull request as ready for review January 16, 2022 10:18
@sashaCher sashaCher requested a review from a team as a code owner January 16, 2022 10:18
tzheleznyak
tzheleznyak reviewed Jan 16, 2022
View reviewed changes
cucumber/authenticators_jwt/features/authn_jwt_ca_cert.feature Outdated
And the authenticator status check succeeds

@skip
# ONYX-15318 and ONYX-15317 are order sensitive tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a bug not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not. It's a proper work of cache.
If conjur already fetched keys from some url, keys remains in memory. So even there's no connectivity to jwks endpoint or like in example, I've added ca-cert variable with wrong cert chain; keys are already in memory - conjur does not invoke a request to bring them...

codeclimate[bot]
codeclimate bot reviewed Jan 16, 2022
View reviewed changes
CHANGELOG.md
@@ -6,6 +6,17 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

## [Unreleased]

### Added
- Added an ability to fetch signing keys from JWKS endpoints are using self-signed
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lists should be surrounded by blank lines

@sashaCher sashaCher changed the base branch from master to integrate-public-keys-flow January 16, 2022 11:20
@sashaCher sashaCher force-pushed the integrate-ca-certificate-flow branch 2 times, most recently from 9efd5e8 to fe2cc7f Compare January 16, 2022 11:37
Base automatically changed from integrate-public-keys-flow to master January 16, 2022 11:47
@sashaCher
Integrate ca-cert variable into full JWT Authentication flow
6509507 
The ca-cert value contains the X.509 public key certificate or certificate bundle. Each certificate in the bundle should be in PEM (RFC7468) format.
The certificate(s) from the variable is/are replacing default operating system CA certificates bundle during fetching JWK Set from remote URI.
Use the variable in order to establish TLS connection and validate server identity when the server is using self-signed certificate or certificate is signed by 3rd party CA.
@sashaCher sashaCher force-pushed the integrate-ca-certificate-flow branch from fe2cc7f to 6509507 Compare January 16, 2022 11:48
tzheleznyak
tzheleznyak approved these changes Jan 16, 2022
View reviewed changes
Copy link
Contributor

@tzheleznyak tzheleznyak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codeclimate
Copy link

codeclimate bot commented Jan 16, 2022

Code Climate has analyzed commit 6509507 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Style 1

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 sashaCher merged commit 3965075 into master Jan 16, 2022
@sashaCher sashaCher deleted the integrate-ca-certificate-flow branch January 16, 2022 12:19
@jtuttle jtuttle mentioned this pull request Jul 12, 2022
Merged
@gl-johnson gl-johnson mentioned this pull request Aug 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate[bot] left review comments

@tzheleznyak tzheleznyak tzheleznyak approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sashaCher @tzheleznyak