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

Support JWK thumbprints as key ids #481

Merged
merged 1 commit into from Jun 24, 2022

Conversation

anakinj
Copy link
Member

@anakinj anakinj commented May 29, 2022

This addresses #474.

Allow the default and custom kid generation to be changed to a more "standard" way of generating JWK kids.

Also introduces a configuration concept via::JWK.configuration where the default values has been moved.

Would very much like to deprecate the current way of kid generation, but we would need to notify about the deprecation in some way before that.

module JWT
module JWK
# https://tools.ietf.org/html/rfc7638
class Thumbprint

Choose a reason for hiding this comment

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

JWT::JWK::Thumbprint has no descriptive comment

Read more about it here.

def initialize(options)
options ||= {}

if options.is_a?(String) # For backwards compatibility when kid was a String

Choose a reason for hiding this comment

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

JWT::JWK::KeyBase#initialize refers to 'options' more than self (maybe move it to another class?)

Read more about it here.


module JWT
module JWK
class CustomKidGenerator

Choose a reason for hiding this comment

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

JWT::JWK::CustomKidGenerator has no descriptive comment

Read more about it here.

require_relative 'configuration/container'

module JWT
module Configuration

Choose a reason for hiding this comment

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

JWT::Configuration has no descriptive comment

Read more about it here.


module JWT
module Configuration
class JwkConfiguration

Choose a reason for hiding this comment

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

JWT::Configuration::JwkConfiguration has no descriptive comment

Read more about it here.

end
end

attr_accessor :kid_generator

Choose a reason for hiding this comment

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

JWT::Configuration::JwkConfiguration#kid_generator is a writable attribute

Read more about it here.


module JWT
module Configuration
class DecodeConfiguration

Choose a reason for hiding this comment

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

JWT::Configuration::DecodeConfiguration has at least 10 instance variables

Read more about it here.


module JWT
module Configuration
class DecodeConfiguration

Choose a reason for hiding this comment

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

JWT::Configuration::DecodeConfiguration has no descriptive comment

Read more about it here.

:verify_iat,
:verify_jti,
:verify_aud,
:verify_sub,

Choose a reason for hiding this comment

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

JWT::Configuration::DecodeConfiguration#verify_sub is a writable attribute

Read more about it here.

module Configuration
class DecodeConfiguration
attr_accessor :verify_expiration,
:verify_not_before,

Choose a reason for hiding this comment

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

JWT::Configuration::DecodeConfiguration#verify_not_before is a writable attribute

Read more about it here.

:verify_not_before,
:verify_iss,
:verify_iat,
:verify_jti,

Choose a reason for hiding this comment

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

JWT::Configuration::DecodeConfiguration#verify_jti is a writable attribute

Read more about it here.

class DecodeConfiguration
attr_accessor :verify_expiration,
:verify_not_before,
:verify_iss,

Choose a reason for hiding this comment

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

JWT::Configuration::DecodeConfiguration#verify_iss is a writable attribute

Read more about it here.

attr_accessor :verify_expiration,
:verify_not_before,
:verify_iss,
:verify_iat,

Choose a reason for hiding this comment

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

JWT::Configuration::DecodeConfiguration#verify_iat is a writable attribute

Read more about it here.

module JWT
module Configuration
class DecodeConfiguration
attr_accessor :verify_expiration,

Choose a reason for hiding this comment

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

JWT::Configuration::DecodeConfiguration#verify_expiration is a writable attribute

Read more about it here.

:verify_iss,
:verify_iat,
:verify_jti,
:verify_aud,

Choose a reason for hiding this comment

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

JWT::Configuration::DecodeConfiguration#verify_aud is a writable attribute

Read more about it here.

:verify_sub,
:leeway,
:algorithms,
:required_claims

Choose a reason for hiding this comment

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

JWT::Configuration::DecodeConfiguration#required_claims is a writable attribute

Read more about it here.

:verify_jti,
:verify_aud,
:verify_sub,
:leeway,

Choose a reason for hiding this comment

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

JWT::Configuration::DecodeConfiguration#leeway is a writable attribute

Read more about it here.

:verify_aud,
:verify_sub,
:leeway,
:algorithms,

Choose a reason for hiding this comment

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

JWT::Configuration::DecodeConfiguration#algorithms is a writable attribute

Read more about it here.

reset!
end

def reset!

Choose a reason for hiding this comment

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

JWT::Configuration::Container has missing safe method 'reset!'


module JWT
module Configuration
class Container

Choose a reason for hiding this comment

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

JWT::Configuration::Container has no descriptive comment

Read more about it here.

@anakinj anakinj force-pushed the configuration-and-thumbprint-as-kid branch from 3306149 to 2f79667 Compare May 29, 2022 16:19
module JWT
module Configuration
class Container
attr_accessor :decode, :jwk

Choose a reason for hiding this comment

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

JWT::Configuration::Container#jwk is a writable attribute

Read more about it here.

module JWT
module Configuration
class Container
attr_accessor :decode, :jwk

Choose a reason for hiding this comment

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

JWT::Configuration::Container#decode is a writable attribute

Read more about it here.

Copy link
Member

@excpt excpt left a comment

Choose a reason for hiding this comment

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

Please update the version.rb to 3.0.0.dev

It's an API breaking change.

Great work! 👍

@anakinj
Copy link
Member Author

anakinj commented May 29, 2022

The API should not have changed in this case, the intention was that at least. What API do you have on your mind?

@anakinj anakinj force-pushed the configuration-and-thumbprint-as-kid branch from 2f79667 to d64ae02 Compare May 29, 2022 16:41
@sourcelevel-bot
Copy link

SourceLevel has finished reviewing this Pull Request and has found:

  • 15 possible new issues (including those that may have been commented here).
  • 1 fixed issue! 🎉

See more details about this review.

@anakinj
Copy link
Member Author

anakinj commented May 30, 2022

@excpt, I got very curious on what im missing:)

@excpt
Copy link
Member

excpt commented Jun 1, 2022

I see that I have chosen my word poorly. 😖

Clarification:

You want to depricate the current used way of kid generation.

This will break existing implementations while continuing using the 2.x versions of this gem.

I recommend bumping the version to 3.0 in that case. So this is will be clear for all the direct and mostly indirect users of this gem.

This allows us to continue the development and it's still possible to continue supporting the 2.x versions.

@anakinj
Copy link
Member Author

anakinj commented Jun 2, 2022

Thanks for the clarification 👍 . I will in my turn clarify my thoughts:

  • The changes are not changing the current behaviour at all so it's possible to ship in a 2.x version with optional support for this.
  • If someone would like to use the thumbprint as the kid the gem can be configured with JWT.configuration.jwk.kid_generator_type = :rfc7638_thumbprint. This will change the behaviour of the kid generation to generate the new kind of kids. The default value for this will be JWT.configuration.jwk.kid_generator_type = :custom
  • In a future 2.x version we could add a deprecation warning on load that the defaults will change and explain what it means.
  • Then in a even more future version the default will change

@anakinj
Copy link
Member Author

anakinj commented Jun 21, 2022

@excpt ok to merge? No changes to the current behaviour and deprecation will happen at a later stage with some kind of warnings to the user.

@excpt excpt self-requested a review June 23, 2022 10:57
Copy link
Member

@excpt excpt left a comment

Choose a reason for hiding this comment

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

Thank you! 👍

@anakinj anakinj force-pushed the configuration-and-thumbprint-as-kid branch from d64ae02 to 2e018fb Compare June 24, 2022 08:24
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.

None yet

2 participants