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.

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

authn-jwt jwks-uri with ca-cert integration tests #2452

Merged
merged 2 commits into from
Jan 4, 2022

Conversation

sashaCher
Copy link
Contributor

@sashaCher sashaCher commented Jan 2, 2022

Desired Outcome

Test recently added ca-cert variable of authn-jwt

Implemented Changes

  • Generated static certificate chain
  • Added new server to jwks (nginx) service configuration in ci and dev environments mycompany.local that's using the certificate chain
  • The "old" jwks server is already using self signed
  • Implement tests according test plan

Connected Issue/Story

ONYX-15511

Definition of Done

  • Desired outcome is achieved

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 requested a review from a team January 2, 2022 19:55
ci/oauth/jwks/cert-chain/README.md Outdated Show resolved Hide resolved
ci/oauth/jwks/cert-chain/README.md Outdated Show resolved Hide resolved
ci/oauth/jwks/cert-chain/README.md Outdated Show resolved Hide resolved
ci/oauth/jwks/cert-chain/README.md Outdated Show resolved Hide resolved
ci/oauth/jwks/cert-chain/README.md Outdated Show resolved Hide resolved
And the HTTP response content type is "application/json"
And the authenticator status check succeeds

Scenario Outline: ONYX-15313/6: Self-signed jwks-uri with ca-cert contains bundle includes the valid certificate
Copy link
Contributor

Choose a reason for hiding this comment

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

why ONYX-15313/6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This outline combines two similar tests one for self-signed use case and one for chained

tzheleznyak
tzheleznyak previously approved these changes Jan 3, 2022
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-ca-cert-infra branch from 2fa00fd to 092548a Compare January 3, 2022 13:58
@sashaCher sashaCher changed the title Infra for authn-jwt jwks-uri with ca-cert integration tests authn-jwt jwks-uri with ca-cert integration tests Jan 3, 2022
@sashaCher sashaCher requested a review from a team January 3, 2022 14:00
@sashaCher sashaCher force-pushed the authn-jwt-ca-cert-infra branch from 092548a to ce063c0 Compare January 4, 2022 08:59
@sashaCher sashaCher marked this pull request as ready for review January 4, 2022 08:59
@sashaCher sashaCher requested a review from a team as a code owner January 4, 2022 08:59
The server name exposes HTTPS endpoint with static chained certificate
Skip tests that's waiting for feature implementation
@sashaCher sashaCher force-pushed the authn-jwt-ca-cert-infra branch from ce063c0 to 87f41ea Compare January 4, 2022 10:30
@@ -0,0 +1,14 @@
Given(/^I fetch root certificate from https:\/\/([^"]*) endpoint as "([^"]*)"$/) do |hostname, key|
Copy link

Choose a reason for hiding this comment

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

Use %r around regular expression.

fetch_and_store_root_certificate(hostname: hostname, key: key)
end

Given(/^I successfully set authn\-jwt "([^"]*)" variable value to the "([^"]*)" certificate$/) do |variable, key|
Copy link

Choose a reason for hiding this comment

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

Redundant escape inside regexp literal

@certs ||= {}
end

def get_certificate_chain(connect_hostname)
Copy link

Choose a reason for hiding this comment

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

CertsHelper#get_certificate_chain doesn't depend on instance state (maybe move it to another class?)


```bash
cp ./certificates/nodes/chained.mycompany.local/chained.mycompany.local.cert.pem ./
cp ./certificates/nodes/chained.mycompany.local/chained.mycompany.local.key.pem ./
Copy link

Choose a reason for hiding this comment

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

Line length

full chain bundle

```bash
cp ./certificates/nodes/chained.mycompany.local/chained.mycompany.local.cert.pem ./
Copy link

Choose a reason for hiding this comment

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

Line length

@codeclimate
Copy link

codeclimate bot commented Jan 4, 2022

Code Climate has analyzed commit 87f41ea and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category Count
Style 4
Complexity 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 90.9% (0.0% change).

View more on Code Climate.

Copy link
Contributor

@eladkug eladkug left a comment

Choose a reason for hiding this comment

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

Tests approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants