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

feat(plugin-jwt) rs512 #2666

Closed
wants to merge 2 commits into from
Closed

Conversation

sarraz1
Copy link
Contributor

@sarraz1 sarraz1 commented Jul 3, 2017

Add support of RS512 algorithm for signing tokens in jwt plugin.
This add-on is based on the already-used module "crypto".

This pull request contains all the updates submitted by he review of the PR #2642 (from next branch). And this previous one have to be cancelled.

Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

Hi!

Looking good, thanks for the contribution. There is just a few details to take care of but most of them are style related, or scope/consistency issues, nothing too worrying. If you take care of them we'd be happy to merge this. Thanks!

@@ -12,7 +12,7 @@ local SCHEMA = {
key = {type = "string", unique = true, default = utils.random_string},
secret = {type = "string", unique = true, default = utils.random_string},
rsa_public_key = {type = "string"},
algorithm = {type = "string", enum = {"HS256", "RS256", "ES256"}, default = 'HS256'}
algorithm = {type = "string", enum = {"HS256", "RS256", "ES256","RS512"}, default = 'HS256'}
Copy link
Member

Choose a reason for hiding this comment

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

Could we move RS512 next to RS256? Thanks!

assert(jwt_parser:new(token))
end)


Copy link
Member

Choose a reason for hiding this comment

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

It seems like this file got many undesired blank lines. Could we remove them?

end)

Copy link
Member

Choose a reason for hiding this comment

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

here too for example

assert.True(jwt:verify_signature(fixtures.rs512_public_key))
assert.False(jwt:verify_signature(fixtures.rs512_public_key:gsub('AE=', 'zzz')))
end)

Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -232,13 +238,13 @@ describe("Plugin: jwt (access)", function()
it("verifies JWT", function()
PAYLOAD.iss = rsa_jwt_secret_1.key
local jwt = jwt_encoder.encode(PAYLOAD, fixtures.rs256_private_key, 'RS256')
local authorization = "Bearer "..jwt
local authorization = "Bearer " .. jwt
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem like this change is related to the scope of this PR?

Copy link
Member

Choose a reason for hiding this comment

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

(style updates of legacy - unrelated - code should not be part of such a feature PR, is what I mean)

local res = assert(proxy_client:send {
method = "GET",
path = "/request",
headers = {
["Authorization"] = authorization,
["Host"] = "jwt.com"
["Host"] = "jwt.com",
Copy link
Member

Choose a reason for hiding this comment

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

ditto

it("verifies JWT", function()
PAYLOAD.iss = rsa_jwt_secret_3.key
local jwt = jwt_encoder.encode(PAYLOAD, fixtures.rs512_private_key, 'RS512')
local authorization = "Bearer "..jwt
Copy link
Member

Choose a reason for hiding this comment

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

However, this line is missing spaces around the concatenation operator ;)

@@ -39,6 +39,46 @@ FLiGOm5uTMEk8S4txs2efueg1XyymilCKzzuXlJvrvPA4u6HI7qNvuvkvUjQmwBH
gwIDAQAB
-----END PUBLIC KEY-----
]],
rs512_private_key = [[
Copy link
Member

Choose a reason for hiding this comment

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

The spaces at the beginning of this line are undesired; other keys are not indented like so.

@thibaultcha thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Jul 4, 2017
@@ -21,6 +21,12 @@ local SCHEMA = {
if plugin_t.algorithm == "RS256" and crypto.pkey.from_pem(plugin_t.rsa_public_key) == nil then
return false, Errors.schema "'rsa_public_key' format is invalid"
end
if plugin_t.algorithm == "RS512" and plugin_t.rsa_public_key == nil then
return false, Errors.schema "no mandatory 'rsa_public_key'"
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we're indenting too far here? indents should be two spaces.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! @sarraz1 Any updates here? Those style issues are all that's left for us to merge it, if you address them we won't hesitate to push the button :)

Copy link
Contributor Author

@sarraz1 sarraz1 Jul 10, 2017

Choose a reason for hiding this comment

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

Hello, the requested changes were committed. Regards ;-)

thibaultcha pushed a commit that referenced this pull request Jul 10, 2017
* add a new value to the enum for `config.algorithm`: `RS512`
* add support for RS512 signing and verification in the `jwt_parser.lua`
module
* appropriate unit and integration tests
* add a new RS512 key pair to the tests fixtures

From #2666
Implements #2473

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
@thibaultcha
Copy link
Member

Manually merged to master with a few changes in the tests, code style and commit message. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants