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

adding RS256 support to JWT plugin #1053

Closed
wants to merge 1 commit into from
Closed

adding RS256 support to JWT plugin #1053

wants to merge 1 commit into from

Conversation

kdstew
Copy link
Contributor

@kdstew kdstew commented Mar 8, 2016

Our use case has added the need for being able to use RS256 signed tokens, so we took a stab at adding this functionality to the JWT plugin.

The following are the changes included in this PR:

  • adds algorithm specification on the credential, defaults to HS256
  • adds support for JWTs that use the RS256 algorithm

The reason for adding the algorithm check is to address the vulnerability described here that is introduced by supporting both HS256 and RS256 signed tokens.

Steps to test

Generate private and public keys:

openssl genrsa -out private.key 2048
openssl req -newkey rsa:2048 -sha256 -nodes -keyout private.key -x509 -out certificate.pem
openssl x509 -inform pem -in certificate.pem -pubkey -noout > publickey.pem

Enable jwt plugin on an api

curl -X POST http://kong:8001/apis/{api}/plugins \
    --data "name=jwt"
    --data "config.secret_is_base64=true"

Create a consumer

curl  -X POST http://kong:8001/consumers \
    --data "username=jwt_tester"

Base64 encode public key

** using Ruby for example
require 'jwt'
pub_key = <<-EOS
{paste contents of publickey.pem here}
EOS
JWT.base64url_encode pub_key

Add jwt credential to consumer

curl -X POST http://kong:8001/consumers/jwt_tester/jwt \
    --data "secret={output of base64 encoded publickey.pem}" \
    --data "algorithm=RS256" 

Create a JWT and sign with private key

  • this can be done easily on jwt.io
  • select 'RS256` in the Algorithm drop down
  • paste in the private.key
  • add the credential's key value to the value of the iss field in the payload

Make a request with the generated token!

@thibaultcha
Copy link
Member

This is very nice, I just gave it a try locally. Thank you fro the feature + the fix!

@kdstew
Copy link
Contributor Author

kdstew commented Mar 10, 2016

Thank you! Hopefully others will find this useful as well.

@subnetmarco
Copy link
Member

@kdstew can you please fix the conflict errors? Happy to go ahead and merge this.

@kdstew
Copy link
Contributor Author

kdstew commented Mar 15, 2016

@thefosk I've rebased with next again, but now some tests are failing. The failures are not in the jwt plugin. Are these failures to be expected at this point?

adds `algorithm` specification on the credential, defaults to `HS256`
@kdstew
Copy link
Contributor Author

kdstew commented Mar 17, 2016

@thefosk @thibaultcha would you mind looking at the failing tests here? I made a fix to address the migrations being run when the tables already existed. However I'm not sure why this test run failed.

@thibaultcha
Copy link
Member

Hey,

Would you please remove your last commit? It's actually not needed (the schema is already dropped before and after). Dropping the schema is not even necessary actually. The tests are just not as reliable as they should be. I tested your changes on my end and they were fine. Will merge when I get a chance to.

On Mar 17, 2016, at 4:04 PM, Kevin Stewart [email protected] wrote:

@thefosk @thibaultcha would you mind looking at the failing tests here? I made a fix to address the migrations being run when the tables already existed. However I'm not sure why this test run failed.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@kdstew
Copy link
Contributor Author

kdstew commented Mar 17, 2016

Sounds good. That last commit has been removed.

@thibaultcha
Copy link
Member

Thanks, I just merged this in next with a few modification (Postgres support) and added your contribution to the Changelog :) Thank you very much!

@thibaultcha
Copy link
Member

Just spotted a flaw here: since secret has a unique constraint, no two consumer can have the same public key for the RSA algorithm. I suppose you use this feature with only 1 Consumer, somewhat as an abstract entity?

I just opened #1090 to fix that (which was originally only to add integration tests).

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.

3 participants