-
Notifications
You must be signed in to change notification settings - Fork 59
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
Update minimum key size to 2048 #142
Comments
@darranl Can you please provide a PR to the spec text dropping a requirement to support |
Sure will do. |
@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. |
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. |
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. |
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. |
@rdebusscher a |
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. |
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. |
@rdebusscher but MP JWT refers to the |
@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 |
@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. |
@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.
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. |
@darranl I believe the goal is to remove it asap, in 2.0, given that it appears technically impossible to do so for the |
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. |
@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 |
@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. |
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. |
I guess this needs to be moved to the 2.0 milestone? |
I'm waiting for @darranl to check what can realistically be done for 1.2 |
Thanks for the reminder, will make this one a priority. |
Hi All, lets try to finalize it for |
This issue has been moved to MP JWT 2.0, #197 is for MP JWT 1.2 |
Section 9.2 of the specification states the required supported key sizes as: -
Section 4.1 states "alg" must be "RS256": -
However RFC7518 "JSON Web Algorithms (JWA)" specifically mandates a minimum key size of 2048: -
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.
The text was updated successfully, but these errors were encountered: