feat(plugin-jwt) support RS512 algorithm for signing tokens. #2642

sarraz1 · 2017-06-20T22:09:29Z

Add support of RS512 algorithm for signing tokens in jwt plugin.

This add-on is based on the already-used module "crypto".

We now avoid reading all request headers via `ngx.req.get_headers()` and use the `ngx.var` API instead. In the future, we'll distinguish Host fetching and full headers fetching when arbitrary headers matching will be added to the router.

When we don't route an API via its `hosts` property (no Host recorded), this test ensures that we still forward the incoming request `Host` header when `preserve_host = true`.

…without-hosts tests(router) ensure preserve_host without hosts matching

…ding perf(router) efficient host header reading

ngx.time uses a cached time struct that obviates the need for the syscall performed by os.time. additionally, os.time is not JITable.

perf(aws-lambda) use ngx.time in place of os.time

Support user directive of Nginx. From Kong#2180

Signed-off-by: Wei.ZHAO <zhaowei@qiyi.com>

fix(test) remove the redundant right parentheses

Signed-off-by: Wei.ZHAO <zhaowei@qiyi.com>

fix(conf) set `nobody nobody` as default nginx user

Right now we suggest compiling LuaJIT with `--without-luajit-lua52`. All the official OpenResty packages compile LuaJIT with Lua 5.2 compatibility, and we should follow the suit. Also it looks like some of the tests were using `os.execute("sleep ...")` instead of `ngx.sleep(...)` for legacy reasons. This pull-request fixes that as well. From Kong#2296

Add two new configuration properties `server_tokens` and `latency_tokens` to toggle `Server` and `X-Kong-*-Latency` response headers. From Kong#2259 Fix Kong#1009

We can set the `ctx` value in a single places for both branches where it is required for it to be `true`. Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

The `user` directive only makes sense if the master process runs with super-user privileges. From Kong#2321

* Add real_ip_recursive and set_real_ip_from Kong configuration fields to configure ngx_http_realip_module directives. * Move the real_ip directives to the Kong proxy location block. * Add configuration building unit tests for those 2 new directives. Fix Kong#1661 Deprecates Kong#1662

Partially fixes Kong#2202

* Introduce trusted_ips config property * Introduce real_ip_header * New lua-resty-mediador * Implement logic to validate client ips as trusted * Implement Lua logic for the X-Forwarded-For upstream header * Move the WebSocket upstream headers dance to the Lua-land instead of the old Nginx maps. * Related tests (renamed real_ip test suite to upstream_headers) Fix Kong#2202

When `real_ip_header = proxy_protocol`, we enable the PROXY protocol support on the Nginx side, by appending the `proxy_procotol` option to the `listen` directive of the proxy server. Implement Kong#2240

Changes from Kong#2236.

Those parts of the configuration were erroneously removed during my cleaning up of the git history from the X-Forwarded-* headers branch.

The compound primary key, coupled with a lack of unique constraint on id, allows for duplicate plugin UUIDs, breaking expected behavior. This commit adds a unique constraint in the Kong schema, and directly to the Postgres table definition via a migration.

In addition to BAD_REQUEST messages sent by Kong, we should also handle natively-generated 400 response codes.

Extra care should have been given to a few CRUD helper methods after the recent merge to 'master'. This fixes the behavior of a few routes in the Admin API and takes both branches' recent changes into account

The recent .luacheckrc file disabled both unused args and unused variables. We make sure to re-enable unused variables linting, and fix all of the mistakes made in the meanwhile.

Installation methods such as homebrew/kong must use unusual rock trees locations. Currently, this patch is shipped with it since Kong/homebrew-kong#43

This commit provides an optional plugin configuration field to search the request body for the authentication credentials. This behavior respects the hide_credentials field. This commit also expands the access test suite to cover a few more use cases.

Follow-up from the previous merge from master which re-introduces linting for unused variables.

From Kong#2548

From Kong#2554

The current implementation of utils.random_string() leverages the LuaJIT math.random() under the hood to generate bytes; this is unsuitable for cases where cryptographically secure random data is needed, such as fallback values for authentication plugin secret values. To correct this, we introduce a wrapper around the kernel CSPRNG (via /dev/urandom) to read random bytes, and wrap utils.random_string around this. We also return these bytes in a modified Base64 encoding (replacing non-alphanumeric characters with random alphanumeric replacements); this serves to increase the size of the keyspace significantly while attempting to maintain some backwards compatibility with previous generated string parameters (e.g. by generating a string of the same size and a somewhat matching pattern). The underlying get_rand_bytes implementation is modified to read from /dev/urandom when explicitly requested, and falling back to OpenSSL's RAND_bytes when reading from urandom fails. The blocking read from urandom is acceptable when explicitly requested, as this is typically done in asynchronous environments (e.g. admin API requests), where the need for strong psuedorandomness outweighs the overhead of I/O and talking to the kernel.

additionally makes some more options configurable

Add support of RS512 algorithm for signing tokens.

p0pr0ck5

Thank you for the submission! Overall looks good, mostly minor style/cleanup comments. Also, as this does not appear to be a breaking change, it can probably be merged against master instead of next. Thank you!

p0pr0ck5 · 2017-06-21T13:40:04Z

kong/plugins/jwt/daos.lua

@@ -10,9 +10,9 @@ local SCHEMA = {
    created_at = {type = "timestamp", immutable = true, dao_insert_value = true},
    consumer_id = {type = "id", required = true, foreign = "consumers:id"},
    key = {type = "string", unique = true, default = utils.random_string},
-    secret = {type = "string", default = utils.random_string},
+    secret = {type = "string", unique = true, default = utils.random_string},


This seems like an undesirable change? It was recently removed, and I'm not sure I see a reason here for it to be added in?

there is no reason.

p0pr0ck5 · 2017-06-21T13:41:17Z

spec/03-plugins/17-jwt/01-jwt_parser_spec.lua

+      assert.False(jwt:verify_signature(fixtures.rs512_public_key:gsub('AE=', 'zzz')))
+    end)
+    -- it("test", function()
+    --assert.True(TRUE)


Let's remove this.

p0pr0ck5 · 2017-06-21T13:42:10Z

spec/03-plugins/17-jwt/03-access_spec.lua

+    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


Style: spaces around the string concat operator

p0pr0ck5 · 2017-06-21T13:42:26Z

spec/03-plugins/17-jwt/03-access_spec.lua

+    it("identifies Consumer", function()
+      PAYLOAD.iss = rsa_jwt_secret_3.key
+      local jwt = jwt_encoder.encode(PAYLOAD, fixtures.rs512_private_key, 'RS512')
+      local authorization = "Bearer "..jwt


Style: spaces around the string concat operator

p0pr0ck5 · 2017-06-21T13:43:38Z

kong/plugins/jwt/daos.lua

@@ -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'"


Seems we're indenting by four spaces here, instead of two? Can we align this with the rest of the block?

p0pr0ck5 · 2017-06-21T15:24:51Z

kong/plugins/jwt/jwt_parser.lua

@@ -49,6 +50,10 @@ local alg_verify = {
    local pkey = assert(crypto.pkey.from_pem(key), "Consumer Public Key is Invalid")
    return crypto.verify('sha256', data, signature, pkey)
  end,
+  ["RS512"] = function(data, signature, key)
+     local pkey = assert(crypto.pkey.from_pem(key), "Consumer Public Key is Invalid")


please check indentation spacing here as well.

p0pr0ck5 · 2017-06-21T15:25:24Z

spec/03-plugins/17-jwt/01-jwt_parser_spec.lua

+    it("accepts a valid RS512 encoding request", function()
+      local token = jwt_parser.encode({sub = "1234"}, "secret", nil, {
+        typ = "JWT",
+        alg = "RS512"


minor style nitpick, its preferred to leave a trailing comma after the last item in a table to facilitate cleaner future commit histories.

so:
alg = "RS512",

p0pr0ck5 · 2017-06-21T15:25:31Z

spec/03-plugins/17-jwt/01-jwt_parser_spec.lua

+    it("accepts a valid RS512 encoding request with lowercase TYP", function()
+      local token = jwt_parser.encode({sub = "1234"}, "secret", nil, {
+        typ = "jwt",
+        alg = "RS512"


same as above.

p0pr0ck5 · 2017-06-21T15:26:23Z

spec/03-plugins/17-jwt/03-access_spec.lua

+        path = "/request",
+        headers = {
+          ["Authorization"] = authorization,
+          ["Host"] = "jwt.com"


trailing coma here

p0pr0ck5 · 2017-06-21T15:26:47Z

spec/03-plugins/17-jwt/03-access_spec.lua

+        path = "/request",
+        headers = {
+          ["Authorization"] = authorization,
+          ["Host"] = "jwt.com"


trailing coma here

p0pr0ck5 · 2017-06-30T04:47:14Z

@sarraz1 any update based on the comments above?

sarraz1 · 2017-06-30T07:03:59Z

I will submit on master with every required updates soon (not today). Le ven. 30 juin 2017 à 06:47, Robert <notifications@github.com> a écrit :

…

@sarraz1 <https://github.com/sarraz1> any update based on the comments above? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2642 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AO8sORWCovPVeVZXkkhz26CC4_e8o9GIks5sJH3hgaJpZM4OAOEy> .

sarraz1 · 2017-07-03T21:23:43Z

Like you suggested, I submitted a new PR from master: #2666
So you can cancel this one.

Regards

thibaultcha · 2017-07-04T00:53:42Z

Closing as this is replaced by the new PR.

Tieske and others added 30 commits March 27, 2017 21:57

Merge branch 'master' into next

052750b

Merge branch 'release/0.10.1' into next

5beb0e2

docs(changelog) fix a confusing typo

a1abe8c

perf(router) read single Host header

636af88

We now avoid reading all request headers via `ngx.req.get_headers()` and use the `ngx.var` API instead. In the future, we'll distinguish Host fetching and full headers fetching when arbitrary headers matching will be added to the router.

tests(router) add a test case for preserve_host

676d822

When we don't route an API via its `hosts` property (no Host recorded), this test ensures that we still forward the incoming request `Host` header when `preserve_host = true`.

Merge pull request Kong#2272 from Mashape/tests/router-preserve-host-…

eb9cc3a

…without-hosts tests(router) ensure preserve_host without hosts matching

Merge pull request Kong#2271 from Mashape/perf/router-host-header-rea…

98d379d

…ding perf(router) efficient host header reading

Merge branch 'docs/code-of-conduct' into next

1a7ba56

perf(aws-lambda) use ngx.time in place of os.time

cbc3768

ngx.time uses a cached time struct that obviates the need for the syscall performed by os.time. additionally, os.time is not JITable.

Merge pull request Kong#2281 from Mashape/perf/lambda-timestamp

82a6951

perf(aws-lambda) use ngx.time in place of os.time

feat(conf) support for the nginx 'user' directive

f3572ac

Support user directive of Nginx. From Kong#2180

fix(test) remove the redundant right parentheses

986a94f

Signed-off-by: Wei.ZHAO <zhaowei@qiyi.com>

Merge pull request Kong#2298 from depay/fix_typo_in_test

df49932

fix(test) remove the redundant right parentheses

fix(conf) set nobody nobody as default nginx user

043f14f

Signed-off-by: Wei.ZHAO <zhaowei@qiyi.com>

Merge pull request Kong#2311 from depay/set_nobody_nobody_as_default

b12c971

fix(conf) set `nobody nobody` as default nginx user

feat(proxy) toggle server and latency tokens

fd18d0d

Add two new configuration properties `server_tokens` and `latency_tokens` to toggle `Server` and `X-Kong-*-Latency` response headers. From Kong#2259 Fix Kong#1009

refactor(cors) minor simplification

b1dd9ad

We can set the `ctx` value in a single places for both branches where it is required for it to be `true`. Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

fix(conf) avoid warn from 'user' directive in nginx.conf

0274f60

The `user` directive only makes sense if the master process runs with super-user privileges. From Kong#2321

fix(migrations): set default for anonymous with auth plugins (Kong#2313)

371744f

Merge branch 'master' into next

3c47101

fix(migrations) typo in anonymous field

01c4c2e

Merge remote-tracking branch 'origin/master' into next

efb6612

feat(proxy) proper X-Forwarded-* upstream headers

2d0e077

Partially fixes Kong#2202

feat(proxy) support for the PROXY protocol

fd7b477

When `real_ip_header = proxy_protocol`, we enable the PROXY protocol support on the Nginx side, by appending the `proxy_procotol` option to the `listen` directive of the proxy server. Implement Kong#2240

docs(changelog) add recent upstream headers changes

a9cb664

Changes from Kong#2236.

Merge branch 'feat/upstream-headers' into next

f42a29e

fix(conf) follow-up from mistake in previous commit

aa0884a

Those parts of the configuration were erroneously removed during my cleaning up of the git history from the X-Forwarded-* headers branch.

p0pr0ck5 and others added 17 commits April 25, 2017 16:56

feat(core) handle native 400 response codes

a0abe84

In addition to BAD_REQUEST messages sent by Kong, we should also handle natively-generated 400 response codes.

Merge remote-tracking branch 'origin/master' into next

5cc9679

fix(admin) use new find_by_id_or_field helper

1dc9a87

Extra care should have been given to a few CRUD helper methods after the recent merge to 'master'. This fixes the behavior of a few routes in the Admin API and takes both branches' recent changes into account

fix(oauth2) remove unique constraint on client_secret

a1bdede

tests(http-log) set pending due to mockbin.com failures

54bb094

chore(luacheck) re-enable unused vars linting

5d5edeb

The recent .luacheckrc file disabled both unused args and unused variables. We make sure to re-enable unused variables linting, and fix all of the mistakes made in the meanwhile.

feat(*) make Kong's require LuaRocks-aware

febfac9

Installation methods such as homebrew/kong must use unusual rock trees locations. Currently, this patch is shipped with it since Kong/homebrew-kong#43

style(migrations) fix linting errors from unused vars

0b2d6b1

Follow-up from the previous merge from master which re-introduces linting for unused variables.

Merge branch 'master' into next

c680b53

Merge branch 'master' into next

c8b60c4

fix(jwt) remove unique constraint on secret

74a2b14

From Kong#2548

feat(admin) remove DB counters in favor of a connection test

7a8a42b

From Kong#2554

feat(resolver) implements search and ndots options

9e7506e

additionally makes some more options configurable

feat(plugin-jwt) RS512

0b6a9b4

Add support of RS512 algorithm for signing tokens.

p0pr0ck5 added the pr/status/please review label Jun 20, 2017

p0pr0ck5 suggested changes Jun 21, 2017

View reviewed changes

p0pr0ck5 added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Jun 21, 2017

sarraz1 changed the base branch from next to master July 3, 2017 20:33

sarraz1 changed the base branch from master to next July 3, 2017 20:34

sarraz1 changed the base branch from next to master July 3, 2017 20:35

sarraz1 mentioned this pull request Jul 3, 2017

feat(plugin-jwt) rs512 #2666

Closed

thibaultcha closed this Jul 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(plugin-jwt) support RS512 algorithm for signing tokens. #2642

feat(plugin-jwt) support RS512 algorithm for signing tokens. #2642

sarraz1 commented Jun 20, 2017

p0pr0ck5 left a comment

p0pr0ck5 Jun 21, 2017

sarraz1 Jul 3, 2017

p0pr0ck5 Jun 21, 2017

sarraz1 Jul 3, 2017

p0pr0ck5 Jun 21, 2017

p0pr0ck5 Jun 21, 2017

p0pr0ck5 Jun 21, 2017

p0pr0ck5 Jun 21, 2017

p0pr0ck5 Jun 21, 2017

sarraz1 Jul 3, 2017

p0pr0ck5 Jun 21, 2017

sarraz1 Jul 3, 2017

p0pr0ck5 Jun 21, 2017

p0pr0ck5 Jun 21, 2017

p0pr0ck5 commented Jun 30, 2017

sarraz1 commented Jun 30, 2017 via email

sarraz1 commented Jul 3, 2017

thibaultcha commented Jul 4, 2017

feat(plugin-jwt) support RS512 algorithm for signing tokens. #2642

feat(plugin-jwt) support RS512 algorithm for signing tokens. #2642

Conversation

sarraz1 commented Jun 20, 2017

p0pr0ck5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0pr0ck5 commented Jun 30, 2017

sarraz1 commented Jun 30, 2017 via email

sarraz1 commented Jul 3, 2017

thibaultcha commented Jul 4, 2017