allow JWT token to be sent as a cookie #2974

mvanholsteijn · 2017-10-19T12:59:47Z

Summary

Added support for passing the JWT in a cookie. When one or more 'cookie_names' are specified, the plugin will try to retrieve the jwt from the specified cookies in order.

There are a number of pull requests open which seem to be aborted. I will take it on me to get it done.

Full changelog

When the jwt plugin is configured with the property 'cookie_names', the plugin will
attempt to retrieve the jwt token from one of the named cookies.

Issues resolved

#2911
#2894
#2363

When the jwt plugin is configured with the property cookie_names, the plugin will get the jwt token from one of the named cookies.

mvanholsteijn · 2017-10-19T13:04:37Z

I have a getkong.org documentation change too. https://github.com/mvanholsteijn/getkong.org/tree/jwt-authentication-in-cookies

danotrampus · 2017-10-19T19:15:49Z

Nice work!! I already implemented you solution and works great.
+1

mvanholsteijn · 2017-10-20T20:22:59Z

Hmm, that is strange. There is an error travis-ci on code that I did not touch..

hishamhm · 2017-10-23T15:12:18Z

Hmm, that is strange. There is an error travis-ci on code that I did not touch..

We are aware that some tests in Travis CI are occasionally producing flaky results (they usually pass when re-running) but we're working on fixing this, sorry about the noise it occasionally produces.

mvanholsteijn · 2017-10-24T19:43:42Z

Thanks for fixing the integration test! Can you review to code too? there is already two happy users ;-)

hishamhm

@mvanholsteijn Sure! Adding a new config field requires a migration to the database for updating the default value of the new field in existing tables. See commits 2040ede and fb55de3 for an example of adding a migration.

mvanholsteijn · 2017-10-25T20:08:27Z

@hishamhm I manually tested the migrations from kong 0.11.0 to this branch on both postgres and cassandra. There is no automated CI test for it, is there?

hishamhm · 2017-10-26T17:53:56Z

@mvanholsteijn thank you! unfortunately no automated tests for migrations at the moments. It's in our to-do list, though!

thibaultcha · 2017-10-26T18:00:20Z

kong/plugins/jwt/handler.lua

@@ -28,6 +28,13 @@ local function retrieve_token(request, conf)
    end
  end

+  for _, v in ipairs(conf.cookie_names) do
+    local jwt_cookie = ngx.unescape_uri(ngx.var["cookie_" .. v])


The access to the ngx global in a loop in such a hot code path warrants a caching of the unescape_uri function at the top of this module:

local unescape_uri = ngx.unescape_uri

The same way, the ngx.var access should be cached locally at the function's level:

local var = ngx.var for _, v in ipairs(conf.cookie_names) do local escaped_cookie = var["cookie_" .. v] if escaped_cookie then return unescape_uri(escaped_cookie) end end

on second looks; the jwt itself is safe-base64 encoded, so the unescape_uri call is not required.

Yes, was also having second thoughts about it. Curious to hear @mvanholsteijn 's take on why this seemed to be required?

I removed the unescape_uri code ; I do not believe it is necessary too.

thibaultcha · 2017-10-26T18:00:34Z

kong/plugins/jwt/handler.lua

+  for _, v in ipairs(conf.cookie_names) do
+    local jwt_cookie = ngx.unescape_uri(ngx.var["cookie_" .. v])
+    if jwt_cookie and jwt_cookie ~= "" then
+      return jwt_cookie, nil


No need for the second return value (nil) here.

thibaultcha · 2017-10-26T18:00:53Z

kong/plugins/jwt/handler.lua

@@ -14,7 +14,7 @@ local JwtHandler = BasePlugin:extend()
 JwtHandler.PRIORITY = 1005

 --- Retrieve a JWT in a request.
-- Checks for the JWT in URI parameters, then in the `Authorization` header.
+-- Checks for the JWT in URI parameters, then for a JWT token in a cookie names and then the `Authorization` header.


Please try to keep lines under 80 character long, as per our contribution guidelines :) Thank you!

thibaultcha · 2017-10-26T18:25:37Z

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

+        }
+      })
+      assert.res_status(200, res)
+    end)


While the state of the code today implies that a fallback on the Authorization header will also imply an HTTP 403 will occur when necessary, this might not be the case in the future, say, if a refactor of the code occurs. I'd feel more comfortable if we included a test for HTTP 403 on a plugin with cookie_names, when not sending cookies nor an Authorization header.

What do you think?

I believe it should be a 401 (Unauthenticated). I added the test to include this case.

Oops, sorry - sent this review from a meeting... I think the new one you added is a nice addition, thanks. I also think we need the HTTP 403 case (Forbidden) which is present in other tests paths, but not the one from a cookie.

I added one in the JWT cookie case. I left it out before, as all the other tests already validate the proper operation after a token was found. The added tests for cookie_names merely focused on finding the cookie token. Anyway, it is in there!

Yes, those are integration tests and leaving such a test out would mean testing this behavior with expectations and assumptions made about the underlying implementation. Preferably, integration tests don't make any assumptions about the tested code and does assume that the system is a black box. Thank you for adding it.

thibaultcha · 2017-10-26T18:25:50Z

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

+    end)
+    it("reports a 401 if the JWT in cookie is corrupt", function()
+      PAYLOAD.iss = jwt_secret.key
+      local jwt = 'no-way-this-works' .. jwt_encoder.encode(PAYLOAD, jwt_secret.secret)


(Please only use double quotes when possible, thank you!)

thibaultcha · 2017-10-26T22:20:02Z

kong/plugins/jwt/handler.lua

@@ -6,6 +6,7 @@ local jwt_decoder = require "kong.plugins.jwt.jwt_parser"
 local string_format = string.format
 local ngx_re_gmatch = ngx.re.gmatch

+local ngx_var = ngx.var


I don't think it is a good idea to cache ngx.var (a per-request variable) value at the module level... This is why I suggested caching it in the function (as per other parts of our codebase do).

I understand. I moved it into the function.

When the JWT plugin is configured with the property cookie_names, the plugin will get the JWT token from one of the named cookies. * add `config.cookie_names` ocnfiguration option * add migration for previous records of this plugin * add integration test suite From #2974 Fix #2911 #2894 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

thibaultcha · 2017-10-27T16:58:35Z

@mvanholsteijn Thank you for helping us and driving this! I just merged this feature as a (slightly modified) squashed commit in next. Will you please open a PR against Kong/getkong.org with the changes you made to the documentation? Thank you!

mvanholsteijn · 2017-10-27T20:11:20Z

sure will do!

See Kong/kong#2974 From #539 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

When the JWT plugin is configured with the property cookie_names, the plugin will get the JWT token from one of the named cookies. * add `config.cookie_names` ocnfiguration option * add migration for previous records of this plugin * add integration test suite From #2974 Fix #2911 #2894 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

When the JWT plugin is configured with the property cookie_names, the plugin will get the JWT token from one of the named cookies. * add `config.cookie_names` ocnfiguration option * add migration for previous records of this plugin * add integration test suite From Kong#2974 Fix Kong#2911 Kong#2894 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

allow JWT token to be sent as a cookie

db2ad9b

When the jwt plugin is configured with the property cookie_names, the plugin will get the jwt token from one of the named cookies.

mvanholsteijn added 2 commits October 19, 2017 13:13

fixed lint errors

f12457b

added test to check result of a corrupt token

baa7e06

hishamhm suggested changes Oct 24, 2017

View reviewed changes

hishamhm added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Oct 24, 2017

migrations to set default cookie_names to empty list

7fd22b4

hishamhm approved these changes Oct 26, 2017

View reviewed changes

hishamhm added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Oct 26, 2017

thibaultcha requested changes Oct 26, 2017

View reviewed changes

processed the review comments

21124b8

thibaultcha reviewed Oct 26, 2017

View reviewed changes

mvanholsteijn added 3 commits October 27, 2017 06:35

added 403 test for jwt token in cookie

4dec823

undo caching of ngx.var

6add562

cache ngx.var in the function

7ca3e27

thibaultcha closed this Oct 27, 2017

mvanholsteijn mentioned this pull request Oct 29, 2017

hmac-auth plugin cannot work behind a AWS Application Load Balancer #2952

Closed

thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Dec 19, 2017

docs(jwt) add config.cookie_names parameter

f05eb42

See Kong/kong#2974 From #539 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Jan 12, 2018

docs(jwt) add config.cookie_names parameter

1366ca8

See Kong/kong#2974 From #539 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Jan 16, 2018

docs(jwt) add config.cookie_names parameter

06a335c

See Kong/kong#2974 From #539 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Jan 16, 2018

docs(jwt) add config.cookie_names parameter

176ae15

See Kong/kong#2974 From #539 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow JWT token to be sent as a cookie #2974

allow JWT token to be sent as a cookie #2974

mvanholsteijn commented Oct 19, 2017

mvanholsteijn commented Oct 19, 2017

danotrampus commented Oct 19, 2017

mvanholsteijn commented Oct 20, 2017

hishamhm commented Oct 23, 2017

mvanholsteijn commented Oct 24, 2017

hishamhm left a comment

mvanholsteijn commented Oct 25, 2017 •

edited

Loading

hishamhm commented Oct 26, 2017

thibaultcha Oct 26, 2017

Tieske Oct 26, 2017

thibaultcha Oct 26, 2017

mvanholsteijn Oct 26, 2017

thibaultcha Oct 26, 2017

mvanholsteijn Oct 26, 2017

thibaultcha Oct 26, 2017

mvanholsteijn Oct 26, 2017

thibaultcha Oct 26, 2017

mvanholsteijn Oct 26, 2017

thibaultcha Oct 26, 2017

mvanholsteijn Oct 27, 2017 •

edited

Loading

thibaultcha Oct 27, 2017

thibaultcha Oct 26, 2017

thibaultcha Oct 26, 2017

mvanholsteijn Oct 27, 2017

thibaultcha commented Oct 27, 2017

mvanholsteijn commented Oct 27, 2017

allow JWT token to be sent as a cookie #2974

allow JWT token to be sent as a cookie #2974

Conversation

mvanholsteijn commented Oct 19, 2017

Summary

Full changelog

Issues resolved

mvanholsteijn commented Oct 19, 2017

danotrampus commented Oct 19, 2017

mvanholsteijn commented Oct 20, 2017

hishamhm commented Oct 23, 2017

mvanholsteijn commented Oct 24, 2017

hishamhm left a comment

Choose a reason for hiding this comment

mvanholsteijn commented Oct 25, 2017 • edited Loading

hishamhm commented Oct 26, 2017

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

mvanholsteijn Oct 27, 2017 • edited Loading

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

thibaultcha commented Oct 27, 2017

mvanholsteijn commented Oct 27, 2017

mvanholsteijn commented Oct 25, 2017 •

edited

Loading

mvanholsteijn Oct 27, 2017 •

edited

Loading