eclipse / microprofile-jwt-auth

Apache License 2.0
108 stars 59 forks source link

Update minimum key size to 2048 #142

Open darranl opened 4 years ago

darranl commented 4 years ago

Section 9.2 of the specification states the required supported key sizes as: -

Support for RSA Public Keys of 1024 or 2048 bits in length is required. Other key sizes are allowed, but should be considered vendor-specific.

Section 4.1 states "alg" must be "RS256": -

This JOSE header parameter identifies the cryptographic algorithm used to secure the JWT. MPJWT requires the use of the RSASSA-PKCS1-v1_5 SHA-256 algorithm and must be specified as "RS256", RFC7515, Section 4.1.1

However RFC7518 "JSON Web Algorithms (JWA)" specifically mandates a minimum key size of 2048: -

A key of size 2048 bits or larger MUST be used with these algorithms.

https://tools.ietf.org/html/rfc7518#section-3.3

I am raising this issue as I don't believe it would have been intentional to override the minimum supported key size.

sberyozkin commented 4 years ago

@darranl Can you please provide a PR to the spec text dropping a requirement to support 1024 bits ? This change will not be a breaking one so would be good to go in the next release IMHO

darranl commented 4 years ago

Sure will do.

rdebusscher commented 4 years ago

@sberyozkin To my understanding, this is a breaking change. Since a JWT which was signed with a 1024 bit key would no longer be accepted. Thus non backwards compatible.

darranl commented 4 years ago

The specification still allows an implementation to support other key sizes so should an implementation wish to retain support for 1024 bit key sizes would still work.

However RFC7518 sets the minimum key size to 2048 bits so there are implementations already with this as a minimum size so 1024 bit key sizes don't guarantee an interoperability.

rdebusscher commented 4 years ago

Verification of the signature doesn't check on the size of the key.

Does your statement means that JWT spec also implicitly takes into account all requirements of the RFC? Because there are no checks for that at the moment and no mentioning in the spec that it is the case today.

darranl commented 4 years ago

No my statement is just that the JWT mandates the algorithm that should be used and the definition for the algorithm mandates the minimum key size for use with the algorithm. There are implementations of that algorithm that can and do check the size of the key they are provided with meet the minimum size of the key size.

The opposite approach could be to add a TCK test using a 1024 bit key size and force all implementations to turn off the minimum size validation if they are using it.

sberyozkin commented 4 years ago

@rdebusscher a 2048 requirement would be about simply enforcing the best practice which in this case has been strongly proposed in the JWA spec based on the RSA security experts' appreciation of the risks involved with the lower key sizes. Same as we all think the exp claim is important, etc. IMHO we should not be too concerned about dropping a requirement for 1024.

rdebusscher commented 4 years ago

I agree that 2048 is almost used all the time and should be used. But defining that 1024 is no longer allowed is according to the rules a breaking change.

And switching to 2.0 because a 2014 key is no longer allowed is a bit harsh (but required in that case)

RFC7518 indeed defines the minimum key length as 2048, but RSASSA-PKCS1-v1_5 SHA-256 can also be used with keys of length 1024.

So we have to refer to RFC7518 explicitly then (which is now not the case) , but as said, that will be a breaking change.

darranl commented 4 years ago

This issue is not saying 1024 would be defined as no longer allowed, this issue is only to remove it from the list of key sizes an implementation is told it must support.

The text in the specification that states an implementation can also optionally support other key sizes would also not be removed by this issue so implementations could still choose to support 1024 bit key sizes.

But there is a useful point here which should probably be addressed at the same time, where the JWT specification is referencing algorithms defined elsewhere it would probably be better to reduce ambiguity and ensure the RFCs / specifications with the details are cross referenced.

sberyozkin commented 4 years ago

@rdebusscher but MP JWT refers to the RS256 definition which is part of the JWA spec and that same spec requires that the key length of the keys must be 2048. I don't think it is correct just to get some algorithm definition from the JWA spec and ignore everything that is advised about it. In this case not requiring the implementations to support 1024 will be actually a bug fix. And indeed, as Darran suggested, any implementation which will think it is important to keep maintaining 1024 support can continue doing so. I can see there is some grey area here indeed because RFC7518 is not referenced at the moment, but I'm not sure this technicality matters, lets just keep things moving forward and show the spec is appreciative of the best security practices. At the very least the requirement to support 1024 should be deprecated now.

sberyozkin commented 4 years ago

@darranl Hi Darran, would it be OK if we just updated the spec with the link to the JWA spec and said that support for 1024 is now deprecated and will be removed in the next major revision of the spec, something along these lines to address the concern of @rdebusscher ? Hi @rdebusscher, just to double check, you'd be opposed to dropping 1024 for 1.2 ?

darranl commented 4 years ago

@sberyozkin The problem I see with deprecation is I am aware of a few servers supporting JWT do not presently support 1024 bit key sizes so if the end result is to keep it as mandatory in the spec then maybe that should be addressed instead. As it stands today the 1024 bit key sizes are not providing a level of interoperability.

rdebusscher commented 4 years ago

@sberyozkin If we put the 1024 bit as no longer mandatory (but still possible to support) this is ok for me in the next version.

I don't think it is correct just to get some algorithm definition from the JWA spec and ignore everything that is advised about it.

It depends on what the goals of the spec are. Since they are referring to RS256 but explicitly say that 1024 bit length MUST be supported, there was some reason behind it (although I do not know them). So bug-fix is not the correct word when you remove something intended features from the past.

In the longer run, say 2.0, we need to base the text better on the RFCs we reference and explicitly mention that they need to be followed (or what points are required to be supported from those RFCs) and create TCK tests for it of course so that we enforce it.

sberyozkin commented 4 years ago

@darranl I believe the goal is to remove it asap, in 2.0, given that it appears technically impossible to do so for the 1.2. And since TCK does not enforce that a 1024 key size is accepted, then so be it, 1024 requirement will remain for 1.2 and I thought a deprecation message would prepare the users who might be using these keys (if any at all) that in 2.0 they won't be supported. The deprecation message would not make it worse for the servers which do not accept 1024 size keys and while it would also be technically correct to go and TCK enforce it, IMHO there are other issues which we can try to tackle for 1.2 :-), due to the other competing commitments and a rather tight schedule. What do you think ?

darranl commented 4 years ago

I think this has gone round in circles a little bit.

The ONLY intent for this issue to remove the requirement from the specification that says a minimum key size of 1024 bits MUST be supported as it is contradictory.

IMO it is either mandatory or it is optional and if it is mandatory ALL implementations MUST support it or they are not compliant so I believe we should add to the TCK.

On the opposite side, if what we are saying is it a mistake for it to have been specified then we can remove it from the specification and implementations are still free to support other key sizes so can still choose to use 1024 bit key sizes.

As a specification I don't believe it is a good practice to contain text that is both mandatory and optional at the same time.

sberyozkin commented 4 years ago

@darranl I'm not suggesting to have it both optional and mandatory. I agree it is a mistake but I don't see a way to drop it for 1.2. So that still makes it mandatory. Just because there is no TCK test to enforce 1024 it does not make it less mandatory or optional. If you'd like, please provide a PR with a TCK test (single test would be enough which uses a 1024 key). The deprecation message will not change anything either but only prepare the users that it will be gone in the major revision, thanks

darranl commented 4 years ago

@sberyozkin thanks that makes it clearer.

So for this issue I will send in a PR to flag it as deprecated and include a TCK test to verify the 1024 bit keys are supported.

starksm64 commented 4 years ago

That is a good resolution. We should raise an issue to drop support for 1024 bit keys in 2.0. @dblevins added the section on the key sizes, so perhaps there is a usecase in TomEE that requires the 1024 key size.

radcortez commented 4 years ago

I guess this needs to be moved to the 2.0 milestone?

sberyozkin commented 4 years ago

I'm waiting for @darranl to check what can realistically be done for 1.2

darranl commented 4 years ago

Thanks for the reminder, will make this one a priority.

sberyozkin commented 4 years ago

Hi All, lets try to finalize it for 1.2. As @darranl has been saying, supporting 1024 bit length keys is a spec requirement which is not TCK enforced. Therefore if we want to keep this requirement in 1.2 then we need to try to enforce it is actually supported by the implementations and tune the underlying Jose implementations as needed.

sberyozkin commented 4 years ago

This issue has been moved to MP JWT 2.0, #197 is for MP JWT 1.2