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

JWK Common Parameters #520

Merged
merged 16 commits into from
Oct 14, 2022
Merged

JWK Common Parameters #520

merged 16 commits into from
Oct 14, 2022

Conversation

bellebaum
Copy link
Contributor

@bellebaum bellebaum commented Oct 7, 2022

This PR Draft addresses #518

It adds a new member common_parameters to JWKs, representing values in the IANA JSON Web Key Parameters Registry which are not specific to any particular value of kty, as well as any custom values to be associated with a JWK.

There is currently no validation according to the semantics of any of these parameters.
Common Parameters (as they were called in RFC 7517) are automatically imported and exported, and can be explicitly specified for initialize using the common_parameters option.
During import, specified common parameter keys are converted to symbols.

This PR is mostly API compatible to main, with one exception:

When trying to import a JWK using the wrong class (as indicated by the kty value), the lib now raises a JWT::JWKError before passing the invalid key to OpenSSL and having an error thrown there.
The same happens for unspecified kty, which actually makes the import candidate a non-JWK according to RFC 7517:

The "kty" value is a case-sensitive string. This member MUST be present in a JWK.

Never ending list of things to do:

  • Add proper documentation
  • Decide whether to reject registered kty specific parameters in the common_parameters member
  • Provide an API-compatible way for future validation of parameters (e.g. use would be a good candidate to enforce for the key_finder)

representing values from the IANA JSON Web Key Parameters Registry
which are not specific to any key type
Copy link
Member

@anakinj anakinj left a comment

Choose a reason for hiding this comment

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

Looks great so far. Left some comments, please do not take them as the truth just comments and my opinions and my opinions change daily :)

lib/jwt/jwk/key_base.rb Outdated Show resolved Hide resolved
lib/jwt/jwk/key_base.rb Outdated Show resolved Hide resolved
lib/jwt/jwk/rsa.rb Outdated Show resolved Hide resolved
lib/jwt/jwk/rsa.rb Outdated Show resolved Hide resolved
lib/jwt/jwk/rsa.rb Outdated Show resolved Hide resolved
lib/jwt/jwk/ec.rb Show resolved Hide resolved
lib/jwt/jwk/key_base.rb Outdated Show resolved Hide resolved
@bellebaum bellebaum mentioned this pull request Oct 11, 2022
Copy link
Member

@anakinj anakinj left a comment

Choose a reason for hiding this comment

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

I added one big comment about the JWK initializer, automatically becomes a little messy when backwards compatibility needs to be preserved.

The tests are also failing for older Rubies because of the Hash#except method is missing, the exclusion of the private parts needs to be done manually I guess.

Im really liking the improvements, super work! and apologies for my nitpicking:)


super(options)
raise ArgumentError, 'keypair must be of type OpenSSL::PKey::EC' unless keypair.is_a?(Hash)
Copy link
Member

Choose a reason for hiding this comment

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

I have a few suggestions for the initializer (the suggestion applies for all the different types).

  • The keypair parameter could be renamed to something more describing.
  • The given OpenSSL object could be kept to avoid recreation from JWK parameters
  • The ArgumentError could tell that the parameter also can be a Hash
  • Transforming the keys to symbols is only needed when first parameter is a Hash.

one idea would be to handle the different variants of the first parameters in a case-block:

def initialize(keypair_or_params, params = nil, options = {})
  params ||= {}

  # For backwards compatibility when kid was a String
  params = { kid: params } if params.is_a?(String)

  key_params = case keypair_or_params
               when OpenSSL::PKey::EC  # Accept OpenSSL key as input
                 @keypair = keypair_or_params # Preserve the object to avoid recreation
                 parse_ec_key(keypair_or_params)
               when Hash
                 keypair_or_params.transform_keys(&:to_sym)
               else
                 raise ArgumentError, 'keypair_or_params must be of type OpenSSL::PKey::EC or Hash with key parameters'
               end

  params = params.transform_keys(&:to_sym)

  check_jwk(keypair, params)

  super(options, key_params.merge(params))
end

Copy link
Member

@anakinj anakinj Oct 12, 2022

Choose a reason for hiding this comment

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

Suggestion 2: What do you think, would an example for the usage without the OpenSSL object in the readme be valuable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think, would an example for the usage without the OpenSSL object in the readme be valuable?

Yes. In fact, the section Importing and exporting JSON Web Keys should probably demonstrate this instead of converting an OpenSSL key as in the example above.

The keypair parameter could be renamed to something more describing.

I agree, though keypair_or_params runs the risk of suggesting that you can specify everything from the first argument in the second, which is not the case (e.g. the first argument needs to be the one describing the kty).

In some other places I think I have simply used key, which could be an OpenSSL Key or a JSON Web Key.

The given OpenSSL object could be kept to avoid recreation from JWK parameters

I like this 👍

lib/jwt/jwk/key_base.rb Show resolved Hide resolved
@bellebaum
Copy link
Contributor Author

I am having a bit of a struggle with RuboCop.
Locally, when I execute bundle exec rubocop, the code only passes all checks if I explicitly disable the class length check for JWT::JWK::RSA (which is in large parts due to the different versions of create_rsa_key), but on GitHub, it seems to complain about me doing this.
The version seems to be 1.31.2 in both cases.

@anakinj
Copy link
Member

anakinj commented Oct 13, 2022

I think the reason is the runtime you are using. RuboCop is executed on Ruby 3.1 on the CI

@bellebaum bellebaum marked this pull request as ready for review October 13, 2022 13:41
@anakinj
Copy link
Member

anakinj commented Oct 13, 2022

This is so great! Still that one RuboCop issue and we are good to go.

@bellebaum
Copy link
Contributor Author

There is one more feature I have been thinking about adding:

When initializing a JWK with another JWK of the same type as key, it should make a copy of this key.

This would allow stuff such as

jwk1 = JWT::JWK.new(OpenSSL::PKey::RSA.new 2048)
jwk2 = JWT::JWK.new(OpenSSL::PKey::RSA.new 2048)
jwks = { keys: [jwk1, jwk2] } # Note: no explicit export

# Uses key accessor to find key with right `kid` and calls `initialize` and `keypair`
JWT.decode my_token, nil, true, jwks: jwks

which is useful if the JWKS is not created as above but fetched from some URL, but I guess even more useful would be some JWKS handling, which is best handled in a future PR :)

@anakinj
Copy link
Member

anakinj commented Oct 14, 2022

I like the suggestion. But maybe not in this PR.

One tiny thing still related to this PR, a little note in the changelog would be great.

@anakinj
Copy link
Member

anakinj commented Oct 14, 2022

We are happy with this, right? No more changes coming @bellebaum?

@bellebaum
Copy link
Contributor Author

Not in this PR. I think it should be possible to extend this in the future, given that options parameters are now separate from params. So I am ok with the current state being merged unless there are further suggestions

@anakinj
Copy link
Member

anakinj commented Oct 14, 2022

Great. Was thinking we could make some plan for the 3.0 version of this gem to get rid of the older behaviour of the interfaces that clutter the code.

@anakinj anakinj merged commit 1ef5247 into jwt:main Oct 14, 2022
@anakinj
Copy link
Member

anakinj commented Oct 14, 2022

Super work here, highly appreciated. Also the valuable suggestions and feedback on these JWK features is great!

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

Successfully merging this pull request may close these issues.

2 participants