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

Attempt to decode using Base64URL for JWK and JWKS. #161

Merged
merged 1 commit into from Jan 13, 2020

Conversation

darranl
Copy link
Contributor

@darranl darranl commented Jan 10, 2020

Presently when using location based resolution there is no attempt to decode using Base64 URL encoding, this is required in section 9.2 of the MicroProfile JWT specification.

@sberyozkin
Copy link
Contributor

@darranl it was there originally, I removed it, as I got the feeling this option was added to the text just in case. I don't know of a single case where JWK sets would be base64 encoded, it can probably lose some content if a given JWK contains a base64-encoded X509 cert chain.

@darranl
Copy link
Contributor Author

darranl commented Jan 13, 2020

TBH I don't follow, at the moment the Base64 encoding is required by the MicroProfile JWT specification and applies to the whole JWKS - the link you have references was for a specific field referencing a X500 certificate chain.

If steps were deliberatly taken to deviate from the specification have any steps been taken to propose an update to the specification?

When a project claims to implement a specification end users have the expectation that the project implements the specification, if a specification identifies some things as optional that is great as there is some flexibility in if they are implemented but they tend to expect the remainder is implemented as described in the specification.

As a result of this I am presently working with a QE team that feel SmallRye JWT is not ready for inclusion.

@sberyozkin
Copy link
Contributor

@darranl So just because base64 encoded JWK are not supported, the QE team already feels so strongly, really ?

Base64 encoding may be problematic because if a JWK key within a set contains that base64 encoded content, then there would be a case of the double Base64 encoding. Can you give me a favor and ask your QE team to confirm that this key will be the same after going through the Base64 encoding/decoding process.

Lets see what the QE team says, if they confirm all is good then I'll be happy to merge your PR.

Re your question why it was not addressed at the spec level. Because I haven't had time for a good reason and I'm only now getting into the process. Both of us can work on moving the spec forward.

Thanks

@darranl
Copy link
Contributor Author

darranl commented Jan 13, 2020

@sberyozkin Unfortunately yes, when we add a new feature to the application server if that feature is backed by a specification they verify that the feature matches the specification and where they detect the implementation has deviated from the specification and doesn't implement / support areas of the specification they consider that a Blocker.

For these issues we have been able to get a little bit of leeway in that we have a workaround of using a plain text JWKS file but that was on the basis we are submitting PRs to receive fixes in the future.

As we are in the closing stages where I need to get the final approval from our QE team to get the feature merged I don't really want to add additional tasks there way today - I will put together a small project to test the key you have identified and then we can also discuss it with them.

@sberyozkin
Copy link
Contributor

@darranl never mind, the code which was there before was base64 decoding, 40c4d8b#diff-18279bb19bacaff8611775ace12737eb

@sberyozkin
Copy link
Contributor

Your PR is good because it does base64URL decoding, I got confused by the PR title

@sberyozkin sberyozkin changed the title Attempt to decode using Base64 for JWK and JWKS. Attempt to decode using Base64URL for JWK and JWKS. Jan 13, 2020
@sberyozkin sberyozkin merged commit abe3801 into smallrye:master Jan 13, 2020
@darranl
Copy link
Contributor Author

darranl commented Jan 13, 2020

Thank you very much @sberyozkin I will confirm with our QE team this one is in ready for when it is next tagged.

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