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

Parser for public-keys value #2450

Merged
merged 3 commits into from
Jan 3, 2022
Merged

Parser for public-keys value #2450

merged 3 commits into from
Jan 3, 2022

Conversation

sashaCher
Copy link
Contributor

Desired Outcome

There's a new Fetch*SigningKey family class FetchPublicKeysSigningKey is responsible for parsing public-keys value.

Implemented Changes

  • Added new FetchPublicKeysSigningKey class parses the public-keys value
  • Added new POJO class PublicSigningKey

Connected Issue/Story

ONYX-15144

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 December 29, 2021 12:51
tzheleznyak
tzheleznyak reviewed Dec 29, 2021
View reviewed changes
spec/app/domain/authentication/authn-jwt/signing_key/public_signing_keys_spec.rb Outdated
"CONJ00120E Failed to parse 'public-keys': the value is not in valid JSON format"],
"When public-keys value is empty object":
[{},
"CONJ00120E Failed to parse 'public-keys': Type can't be blank, Type '' is not a valid public-keys type, and Value can't be blank"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this string to const or to part of the loop down there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll consider to move it to the loop once will finish all use cases...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tzheleznyak
tzheleznyak reviewed Dec 29, 2021
View reviewed changes
config/locales/en.yml Outdated
@@ -18,3 +18,4 @@
#
# To learn more, please read the Rails Internationalization guide
# available at http://guides.rubyonrails.org/i18n.html.
en:
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without it all ActiveModel validation exceptions will be masked by i18n exception that the file is invalid.
Adding en: to he file makes it valid and allows to receive original validation exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I've found that removing the file has the same effect.
Initially all lines in file are commented out #....
So I'm removing it instead of adding en: to it...

tzheleznyak
tzheleznyak reviewed Dec 29, 2021
View reviewed changes
spec/app/domain/authentication/authn-jwt/signing_key/fetch_public_keys_signing_key_spec.rb Outdated
context "FetchPublicKeysSigningKey call " do
context "propagates false refresh value" do
subject do
jwks = Net::HTTP.get_response(URI("https://www.googleapis.com/oauth2/v3/certs")).body
Copy link
Contributor

Choose a reason for hiding this comment

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

It means that our automation will be dependent on the existence of this endpoint?
Its not better this thing be in integration test and with our own endpoint for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that there're billions things depends this endpoint in the real world :-).
Let's consider UTs as not finished yet but I'm almost sure that I'll keep it because it will be a great indicator of Conjur's compatibility to the real world...

@sashaCher sashaCher marked this pull request as ready for review January 1, 2022 19:57
@sashaCher sashaCher requested a review from a team as a code owner January 1, 2022 19:57
codeclimate[bot]
codeclimate bot reviewed Jan 1, 2022
View reviewed changes
app/domain/authentication/authn_jwt/signing_key/fetch_public_keys_signing_key.rb Outdated Show resolved Hide resolved
app/domain/authentication/authn_jwt/signing_key/public_signing_keys.rb
errors.add(:value, "is not a valid JWKS (RFC7517)") unless
@value.is_a?(Hash) &&
@value.key?(:keys) &&
@value[:keys].is_a?(Array) &&
Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnJwt::SigningKey::PublicSigningKeys#validate_value_is_jwks calls '@value[:keys]' 2 times

app/domain/authentication/authn_jwt/signing_key/public_signing_keys.rb
module AuthnJwt
module SigningKey
# This class is a POJO class presents public-keys structure
class PublicSigningKeys
Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnJwt::SigningKey::PublicSigningKeys assumes too much for instance variable '@value'

app/domain/authentication/authn_jwt/signing_key/public_signing_keys.rb
end
end

def validate!
Copy link

Choose a reason for hiding this comment

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

Authentication::AuthnJwt::SigningKey::PublicSigningKeys has missing safe method 'validate!'

app/domain/authentication/authn_jwt/signing_key/fetch_public_keys_signing_key.rb Outdated Show resolved Hide resolved
@cyberark cyberark deleted a comment from sashaCher Jan 2, 2022
eladkug
eladkug reviewed Jan 2, 2022
View reviewed changes
app/domain/authentication/authn_jwt/signing_key/public_signing_keys.rb Outdated
private

def validate_value_is_jwks
errors.add(:value, "is not a valid JWKS (RFC7517)") unless
Copy link
Contributor

Choose a reason for hiding this comment

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

We have several error messages for key validation , this one is an addition ? the RFC is not to global (JSON Web Key (JWK))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The validations only for public-keys use case where the value is came from customer and is not fetched directly from keys provider

eladkug
eladkug reviewed Jan 2, 2022
View reviewed changes
app/domain/logs.rb Outdated Show resolved Hide resolved
@sashaCher sashaCher force-pushed the fetch-public-keys branch 3 times, most recently from 0673393 to e97ca9c Compare January 2, 2022 17:59
tzheleznyak
tzheleznyak reviewed Jan 3, 2022
View reviewed changes
app/domain/authentication/authn_jwt/signing_key/public_signing_keys.rb Outdated
validate(:validate_value_is_jwks, if: -> { @type == "jwks" })

def initialize(hash)
raise Errors::Authentication::AuthnJwt::InvalidPublicKeys.new("the value is not in valid JSON format") unless
Copy link
Contributor

Choose a reason for hiding this comment

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

Can "the value is not in valid JSON format" be moved to errors.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Errors at the moment contains only exceptions
It's a variable part of general InvalidPublicKeys exception text...

tzheleznyak
tzheleznyak reviewed Jan 3, 2022
View reviewed changes
app/domain/logs.rb Outdated Show resolved Hide resolved
tzheleznyak
tzheleznyak previously approved these changes Jan 3, 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
Remove commented out config/locales/en.yml
c4ff24f 
ActiveModule validation is expecting to see a dictionary in the file that masquerading original validation errors
@sashaCher
PublicSigningKeys class represents public-keys structure and validations
d3f478f
@sashaCher
FetchPublicKeysSigningKey class
22cd7a2 
Parses public-keys value from JSON and return a valid JWKS structure
tzheleznyak
tzheleznyak approved these changes Jan 3, 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 3, 2022

Code Climate has analyzed commit 22cd7a2 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 90.9% (0.0% change).

View more on Code Climate.

@sashaCher sashaCher merged commit b9b8ed7 into master Jan 3, 2022
@sashaCher sashaCher deleted the fetch-public-keys branch January 3, 2022 09:54
@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

@eladkug eladkug eladkug left review comments

@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.
3 participants
@sashaCher @eladkug @tzheleznyak