Migrate lua-resty-session to 4.0.3 [tested, works]

[oldium]

Complete rework of #478, all issues of #478 should be addressed. Uses session:set and session:get to manipulate the session variables.

Tested by unit tests and on real-world project.

[lordgreg]

Hi. Will this get merged? @bodewig

[ja-softdevel]

@bodewig
Hello, is there an ETA on this merge?
It seems this is causing issues downstream with kong oidc.

[oldium]

Adding a cute kitten usually helps speed things up 😅

[ja-softdevel]

@oldium Hello, I git cloned your branch and built it in a Dockerfile along with kong-oidc. I was hoping your changes would resolve the error I keep getting.

using Docker:
Kong Gateway (OSS)
service: https://cat-fact.herokuapp.com/facts
route: /cats
plugin: kong-oidc
Keycloak for auth

The route is set to redirect to Keycloak for testing auth. But I get the following error in the Kong logs when Keycloak redirects back to the route.

[error] 1264#0: *2785 [lua] openidc.lua:1484: authenticate(): request to the redirect_uri path but there's no session state found, client: 172.25.17.1, server: kong, request: "GET /cats?state=41b59f8cd04f7404a603ba9908d18930&session_state=ba1bf1a8-cf0f-4563-8028-fcf3ddd9e0b1&code=16da05eb-78ea-4323-a742-bdbf2b8362c1.ba1bf1a8-cf0f-4563-8028-fcf3ddd9e0b1.9406568b-bc51-4841-a8ae-99a3fdb381c2 HTTP/1.1", host: "localhost"

What am I missing? The session_state object is clearly there.

Thanks.

[lordgreg]

@ja-softdevel - can you list which luarocks plugins you have installed?

usually, its lua-resty-session, which appears twice and causes the exact problem you are having.

[ja-softdevel]

@ja-softdevel - can you list which luarocks plugins you have installed?

usually, its lua-resty-session, which appears twice and causes the exact problem you are having.

FYI: I haven't had much experience with lua. C++, Python but not lua. So still figuring out how it does things.

Looks like lua-resty-session is appearing twice and lua-resty-openidc in the list:

$ luarocks list resty
Rocks installed for Lua 5.1
---------------------------
lua-resty-acme
   0.11.0-1 (installed) - /usr/local/lib/luarocks/rocks-5.1
lua-resty-counter
   0.2.1-1 (installed) - /usr/local/lib/luarocks/rocks-5.1
lua-resty-healthcheck
   1.6.2-1 (installed) - /usr/local/lib/luarocks/rocks-5.1
lua-resty-http
   0.17.1-0 (installed) - /usr/local/lib/luarocks/rocks-5.1
lua-resty-ipmatcher
   0.6.1-0 (installed) - /usr/local/lib/luarocks/rocks-5.1
lua-resty-jit-uuid
   0.0.7-2 (installed) - /usr/local/lib/luarocks/rocks-5.1
lua-resty-jwt
   0.2.3-0 (installed) - /usr/local/lib/luarocks/rocks-5.1
lua-resty-openidc
   1.7.6-3 (installed) - /usr/local/lib/luarocks/rocks-5.1
   1.6.0-1 (installed) - /usr/local/lib/luarocks/rocks-5.1
lua-resty-openssl
   0.8.23-1 (installed) - /usr/local/lib/luarocks/rocks-5.1
lua-resty-session
   4.0.4-1 (installed) - /usr/local/lib/luarocks/rocks-5.1
   3.10-1 (installed) - /usr/local/lib/luarocks/rocks-5.1
lua-resty-timer
   1.1.0-1 (installed) - /usr/local/lib/luarocks/rocks-5.1
lua-resty-timer-ng
   0.2.5-1 (installed) - /usr/local/lib/luarocks/rocks-5.1

In the Dockerfile, I'm installing 3 lua packages.:

git clone --branch resty-session-4.x https://github.com/oldium/lua-resty-openidc.git
git clone --branch v1.4.0-1 https://github.com/revomatico/kong-oidc.git
git clone --branch v1.1.0 https://github.com/dkoltsuk/kong-plugin-jwt-keycloak.git

Should I first uninstall lua-resty-openidc and lua-resty-session packages before doing the git clones?
Thanks.

[lordgreg]

How did you install the plugins? You've added the git clone commands, but do you made any luarocks make *.spec?

Anyways,

luarocks uninstall lua-resty-session 4.0.4-1 --force will fix your issue. The 3.10.1 gets installed using kong-oidc > lua-resty-openidc > lua-resty-session dependency hell :)

Which is why we are talking merging this PR into the master. :)

[oldium]

Newest Kong GW images use lua-resty-session 4, so I think that would not work. I guess you have this for Kong GW, right?

[ja-softdevel]

Alrighty!

So I added this line to the Dockerfile just after my apt updates but before doing the git clones which removed 4.0.4-1.

RUN luarocks remove lua-resty-session --force

After I clone @oldium 's fork and set to the resty-session-4.x branch, that repo will cause lua-resty-session 4.0.5-1 to get installed.

Then starting docker-compose and letting decK complete, I added the kong-oidc plugin to the (oh so lovely) '/cats' route. Only items set on the plugin were:
client id, client secret, discovery uri, and I changed the realm to match the one I set in keycloak.

I have deleted the image and rebuilt it. Then ran docker up/down to ensure this worked.

Thank you so much @lordgreg and @oldium for the assistance. Hopefully, @bodewig can get this merged and maybe add you two as maintainers to help prevent development stall out.

[Dark3clipse]

I've tested it and it seems to works for me too.

[zaygraveyard]

I get the following error when trying to logout a user that happens to not be logged-in, which used to work:

lua entry thread aborted: runtime error: /.../lualib/resty/session.lua:2077: unable to destroy nonexistent or closed session
stack traceback:
coroutine 0:
	[C]: in function 'assert'
	/.../lualib/resty/session.lua:2077: in function 'destroy'
	/.../lualib/resty/openidc.lua:1298: in function 'openidc_logout'
	/.../lualib/resty/openidc.lua:1508: in function 'authenticate'

Replacing

lua-resty-openidc/lib/resty/openidc.lua

Line 1287 in 734a3f4

session:destroy()

with the following fixes the issue:

  if next(session:get_data()) ~= nil then
    session:destroy()
  end

[oldium]

Thanks, I will check it

[giafar]

Hi. Will this get merged? @bodewig

[bodewig]

So people don't need to keep asking: I can not foresee when (or even if) I will find to properly review this or #478 myself.

It is my impression that lua-resty-session has also changed semantics so changing the code in way that Lua is happy will not be enough. Reports by people who have tried the patches in both PRs confirm this is not a simple "apply and all is well" fix. I do not have the cycles to understand what has changed between 3.x and 4.x over in lua-resty-session. lua-resty-openidc lacks any tests that would make use of sessions and personally I don't run any environment using lua-resty-openidc at all right now so wouldn't be able to properly test things myself.

[oldium]

I get the following error when trying to logout a user that happens to not be logged-in, which used to work:

lua entry thread aborted: runtime error: /.../lualib/resty/session.lua:2077: unable to destroy nonexistent or closed session
stack traceback:
coroutine 0:
	[C]: in function 'assert'
	/.../lualib/resty/session.lua:2077: in function 'destroy'
	/.../lualib/resty/openidc.lua:1298: in function 'openidc_logout'
	/.../lualib/resty/openidc.lua:1508: in function 'authenticate'

Replacing

lua-resty-openidc/lib/resty/openidc.lua

Line 1287 in 734a3f4

session:destroy()

with the following fixes the issue:

  if next(session:get_data()) ~= nil then
    session:destroy()
  end

I checked it and it seems this is a problem of lua-resty-session, so I reported a bug there. I also added the suggested workaround to lua-resty-openidc and a test case until this is fixed.

[oldium]

I have renamed the Pull Request to indicate this is more up-to-date than the second (unmaintained) one.

Anyway, enjoy 🚀😁

[oldium]

Just for info: We are using the code from this Pull Request in our project in Dockerfile in the following way:

ARG KONG_GW_FLAVOR="kong/kong-gateway"
ARG KONG_GW_VERSION="3.4.3.2-ubuntu"

FROM docker.io/${KONG_GW_FLAVOR}:${KONG_GW_VERSION}
...
# Download with https://codeload.github.com/zmartzone/lua-resty-openidc/legacy.tar.gz/pull/489/head
# See https://github.com/zmartzone/lua-resty-openidc/pull/489
ADD zmartzone-lua-resty-openidc-v1.7.6-21-g1553a39.tar.gz /install

RUN ls -l /install; cd /install/zmartzone-lua-resty-openidc-1553a39 \
    && find -name *.rockspec -exec luarocks make {} \;
...

[oldium]

It looks like the tests fail randomly - may be because of timing of flushing the nginx logs to disk? EDIT: This was caused by non-padded Base64 decoding

[oldium]

Fixed decoding of Base64Url in tests (old issue), so they should pass now.

[oldium]

I have to say that ChatGPT recommended the Base64 fix and it really worked...

[lukeyeager]

Tested working for me

[GitDemis]

amazing work, thanks!

[justin-octo]

@bodewig just curious if there is someone else you trust that can review lua-resty-session and how it impacts lua-resty-openidc if you are unable at the moment.

I am sensitive to supply chain attacks and people trying to force/manipulate maintainers to approve things, especially an identity tools like this. So, not trying to pressure you. Just don't want to see this project get stalled, and if there is someone you trust to review this for you...

[piotrgasior]

Hi,
Are there any plans to merge this branch to master and make official release?

[justin-octo]

@bodewig this seems to be stalled because it's not a priority for you. How can the community help get this over the line?

Perhaps someone who worked on the changes from lua-resty-session could assist? Along with someone who uses lua-resty-openidc more frequently?

[oldium]

Rebased on top of master

[zandbelt]

I believe that by now we should really merge this, as to be compliant with lua-resty-session 4.x moving forward. But we can only do so if someone can take responsibility for fixing stuff that may arise after releasing it, most probably in a v2.0.0. It appears that neither @bodewig or me are able to do that so @oldium if we could add you as a maintainer I believe we can move forward with this assuming @bodewig agrees as well. Both, please confirm.

[oldium]

I can do bugfixing after release same as I did in this thread, so no problem for me.

[bodewig]

It is a lack of time and energy, not "not a priority", but the result is the same. I'm totally fine with adding @oldium

I have glanced over the code in the past and did the same just now. I am not sure this patch is going to create a new session cookie as it no longer calls regenerate when the token has been refreshed - so the result might be prone to the race condition we tried to make less likely by regenerating (see #190 ). I'm not sure what lua-resty-session 4 does under the covers and not whether it would even be possible to force a new cookie - and this is where I've always run out of steam in the past.

In less words this patch may bring back #190 or not, I don't really know.

[zandbelt]

@oldium you've been invited as a collaborator on this project with write access; if you could address @bodewig 's comment and merge to master, we'll release whenever you think it is ready

[justin-octo]

Hello @bodewig, I just meant you had other things in your life with higher priority. Which is totally understandable. I did NOT mean to imply that you didn't care. You obviously have put a lot of time and effort into this, and it is appreciated by many.

[bodewig]

Hello @bodewig, I just meant you had other things in your life with higher priority. Which is totally understandable. I did NOT mean to imply that you didn't care. You obviously have put a lot of time and effort into this, and it is appreciated by many.

Don't worry @justin-octo , there are no hard feelings and I didn't take it as an insult. Just wanted to explain my situation a bit.

Thank you for your kind words.

[bodewig]

In less words this patch may bring back #190 or not, I don't really know.

The new code invokes session:save and the docs seem to say this will result in a new session cookie with a new session id, so things should work more or less like the old regenerate path did. I haven't verified it, though.

[oldium]

In less words this patch may bring back #190 or not, I don't really know.

The new code invokes session:save and the docs seem to say this will result in a new session cookie with a new session id, so things should work more or less like the old regenerate path did. I haven't verified it, though.

I remember investigating this and using save at the end. The code really generates new sid on every call to save, which is then encoded into the cookie itself (stored into internal cookie's header part and used to derive an AES encryption key) and used by the storage backend as a data key (optionally hashed). So race condition should be handled fine, the first cookie should still work.

[oldium]

The only problem could arise when multiple use of a refresh token is being prevented (when a refresh issues a new refresh token and invalidates the token used), but since a failed refresh does not update the cookie, this should only occur once when multiple requests are issued in parallel with an expired access token.

[bodewig]

right - and this is a situation that we also could not prevent with the regenerate "solution" of 1.7.0.

[zandbelt]

@bodewig @oldium it seems we have feature/known-issue parity with 1.7.6 then? ; @oldium just let me know when you think we should cut a new release 1.8.0, perhaps after receiving feedback from @markbanierink in #526 (comment) ?

[oldium]

The tenant-specific usage worked, so I think we can cut a new release 😊

[zandbelt]

https://github.com/zmartzone/lua-resty-openidc/releases/tag/v1.8.0

[Dark3clipse]

Great news! Thanks everyone for all the hard work