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

Controlling session expiration #174

Closed
thomasleplus opened this issue Jul 12, 2018 · 13 comments
Closed

Controlling session expiration #174

thomasleplus opened this issue Jul 12, 2018 · 13 comments
Labels

Comments

@thomasleplus
Copy link
Contributor

thomasleplus commented Jul 12, 2018

Hello,

I am facing an issue getting precise control over the lifetime of the session. I think that there is something I have trouble understanding in the interaction between lua-resty-session and lua-resty-openidc. I have configured my OP to give tokens valid for 1 day in order to take the refreshing of the token out of the picture for now. I am only trying to figure out how the nginx session work.

Below is an extract of my nginx.conf where I am trying to set a lifetime for my session of 10 minutes:

...
  server {
    listen 443 default_server ssl;

    ssl_certificate     /etc/ssl/certs/crt/nginx.crt;
    ssl_certificate_key /etc/ssl/certs/key/nginx.key;
    ssl_session_timeout 1d;
    ssl_session_cache shared:TLS:100m;
    ssl_session_tickets off;
    ssl_protocols TLSv1.2;
    ssl_ciphers "EECDH+ECDSA+AESGCM:EECDH+aRSA+AESGCM:EECDH+ECDSA+SHA512:EECDH+ECDSA+SHA384:EECDH+ECDSA+SHA256:ECDH+AESGCM:ECDH+AES256!aNULL:!eNULL:!LOW:!DH:!RC4:!3DES:!MD5:!EXP:!PSK:!SRP:!DSS";
    ssl_prefer_server_ciphers on;

    set $session_secret  <session_secret>
    set $session_cookie_httponly     on;
    set $session_cookie_lifetime    600;
    set $session_cookie_renew       600;
    set $session_cookie_samesite Strict;
    set $session_cookie_secure       on;
                                                                                                                                                                                                                                    
      location /foo {
        access_by_lua_block {
          local opts = {
            redirect_uri_path = "/login",
            discovery = "https://localhost/.well-known/openid-configuration",
            client_id = "<client_id>",
            client_secret = "<client_secret>",
            scope = "openid email profile",
            ssl_verify = "no",
            renew_access_token_on_expiry = true,
            access_token_expires_leeway = 60,
            logout_path = "/logout"
          }
          local res, err = require("resty.openidc").authenticate(opts)
          if err then
            ngx.status = 500
            ngx.say(err)
            ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR)
          end
          ngx.req.set_header("X-USER", res.id_token.sub)
          ngx.req.set_header('Authorization', 'Bearer '..res.access_token)
        }
        proxy_ignore_client_abort on;
        proxy_set_header Cookies "";
        proxy_pass http://foo;
        rewrite ^/foo/(.*)$ /$1 break;
      }
...

However if I send a request after 5 minutes, I get a 401 response with the following messages in nginx logs:

2018/07/12 04:55:06 [debug] 21#21: *44 [lua] openidc.lua:470: openidc_discover(): openidc_discover: URL is: https://localhost/.well-known/openid-configuration
2018/07/12 04:55:06 [debug] 21#21: *44 [lua] openidc.lua:96: openidc_cache_get(): cache hit: type=discovery key=https://localhost/.well-known/openid-configuration
2018/07/12 04:55:06 [debug] 21#21: *44 [lua] openidc.lua:1063: openidc_get_token_auth_method(): 1 => client_secret_basic
2018/07/12 04:55:06 [debug] 21#21: *44 [lua] openidc.lua:1066: openidc_get_token_auth_method(): no configuration setting for option so select the first supported method specified by the OP: client_secret_basic
2018/07/12 04:55:06 [debug] 21#21: *44 [lua] openidc.lua:1080: openidc_get_token_auth_method(): token_endpoint_auth_method result set to client_secret_basic
2018/07/12 04:55:06 [debug] 21#21: *44 [lua] openidc.lua:1217: authenticate(): session.present=nil, session.data.id_token=false, session.data.authenticated=nil, opts.force_reauthorize=nil, opts.renew_access_token_on_expiry=true, try_to_renew=true, token_expired=false
2018/07/12 04:55:06 [warn] 21#21: *44 [lua] access_by_lua(nginx.conf:174):23: Session info request is either unauthenticated or authentication has expired., client: 172.20.0.1, server: , request: "GET /foo HTTP/1.1", host: "localhost", referrer: "https://localhost/bar"

I am failing to understand why "Session info request is either unauthenticated or authentication has expired" at only 5 minutes instead of 10. When I had $session_cookie_lifetime set at 1 hour (3600) it seemed like the session was expiring at around 30 minutes so again half of the intended value.

Any suggestion would be greatly appreciated.

Cheers,

Tom

@zandbelt
Copy link
Contributor

perhaps this is better asked at the lua-resty-session project

@thomasleplus
Copy link
Contributor Author

OK I will do that. I wasn't sure if lua-resty-openidc was invoking the methods of lua-resty-session itself to store it's cookie or if it just relied on whatever lua-resty-session is doing out of the box. Thanks.

@zandbelt
Copy link
Contributor

yes, it does the latter indeed

@thomasleplus
Copy link
Contributor Author

Thank you Hans. Indeed I found the answer to my question in bungle/lua-resty-session#44 (comment).

If anybody who reads this has the same issue, the main point is that the session cookie is updated if the session:save() method is invoked. This happens when lua-resty-openidc refreshes the JWT tokens. But if you don't have anything else touching the session and you have configured your OP to issue access token with a longer lifetime than your session, you can reach the end of your session without any token refresh and therefor the session cookie would expire even if you had many requests/responses going through Nginx.

In my case I have resolved the issue by making the token lifetime a fraction of the session cookie lifetime. I guess another solution could be to call session:save() or session:regenerate() explicitly myself when needed. But I didn't want to have to deal with the logic of deciding when it is time to renew the session cookie or not. It would be nice if lua-resty-session had some sort of session:regenerateIfNeeded() method.

By the way another pitfall is the way $session_cookie_renew is used by default bungle/lua-resty-session#44 (comment). I've set my $session_cookie_renew equal to my $session_cookie_lifetime just to be safe.

@zandbelt
Copy link
Contributor

zandbelt commented Jul 17, 2018

I'm re-opening because I guess we need to take a better look at this.
@thomasleplus thanks for the work on this, your feedback is much appreciated!

@zandbelt zandbelt reopened this Jul 17, 2018
@zandbelt zandbelt added the bug label Sep 25, 2018
zandbelt added a commit that referenced this issue Sep 25, 2018
have the session cookie renewed according to `session.cookie.renew` and
`session.cookie.lifetime` settings

Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
@zandbelt
Copy link
Contributor

zandbelt commented Sep 25, 2018

I believe we should just call session.start instead of session.open and everything should work automagically, controlled by lua-resty-session settings session.cookie.renew and session.cookie.lifetime. That's because lua-resty-session does have "some sort of session:regenerateIfNeeded() method" as @thomasleplus suggested. @bodewig whaddayathink? @thomasleplus can you verify the branch with this modification? https://github.com/zmartzone/lua-resty-openidc/tree/call-session-start-instead-of-open

@thomasleplus
Copy link
Contributor Author

Sure, I can do that. I might need a couple of days however before I can get to it.

@bodewig
Copy link
Collaborator

bodewig commented Sep 27, 2018

Sorry for the delay, have been traveling.
One major difference seems to be that the call to session.start is going to destroy a session that has already been created by code calling openidc.authenticate or openidc.access_token. If we switch to start we probably need a way to pass in an existing session to allow people to use the session for their own code outside of lua-resty-openidc.

@zandbelt
Copy link
Contributor

zandbelt commented Sep 27, 2018

ah, good point indeed; but: didn't we call start for those paths anyhow?

not 100% sure but it seems like managing your own session outside of lua-resty-openidc was (nearly) impossible anyway so no backwards compatibility issue

@bodewig
Copy link
Collaborator

bodewig commented Sep 27, 2018

session:start was/is used only if

  • authenticate is used and the user is not authorized or the existing token needs to be refreshed
  • access_token is used and the token gets refreshed

which means you could have used the session before calling into lua-resty-openidc when you knew the user was authenticated and knew the token wasn't going to get refreshed. So I agree, it is very unlikely this is going to break backwards compatibility unless you've been willing to deal with your session getting destroyed every now and then anyway.

@zandbelt
Copy link
Contributor

ok, I don't feel we should worry about that since it was never part of any "advertised" capability
we could make it into a feature but I'd suggest to do that in a separate issue/feature/branch
let's wait for @thomasleplus's feedback before merging for 1.7.0

@thomasleplus
Copy link
Contributor Author

thomasleplus commented Oct 2, 2018

Everything works as expected as far as I can tell. I have done two test cases. In both cases my IDP (Ping Federate) is configured to issue tokens that expire far in the future in order to avoid any interaction with the IDP after the initial authentication flow.

In test case 1, my NGINX config had:

set $session_cookie_lifetime 180;
set $session_cookie_renew 60;

After the initial authentication, I get a cookie that has an expiration in 3 minutes as expected. Following requests in the next 2 minutes don't get new cookies (no Set-Cookie header in the response). After 2 minutes but before 3 (i.e. during the renew period), if I send a request, the responses comes with a new set of cookies valid for another 3 minutes. I continue to send requests for 2 minutes, the cookies remain the same again. This time in the last minute before the expiration of the current cookie (i.e. during the renew period), I stop sending requests to let the session expire, then once it happened if I send a request I get a 401 as expected.

In test case 2, I changed my NGINX config to:

set $session_cookie_lifetime 180;
set $session_cookie_renew 180;

This is closer to what I am planning to use in production. After the initial authentication, I get a cookie that has an expiration in 3 minutes as expected. Every subsequent request gets a new set of cookies valid for another 3 minutes. If I don't send any request for more than 3 minutes, then I get a 401 as expected.

I am closing this issue but let me know if you see another worthwhile test case. Now that I have everything setup, it should be pretty quick for me to test.

@bodewig
Copy link
Collaborator

bodewig commented Oct 3, 2018

many thanks @thomasleplus

zandbelt added a commit that referenced this issue Oct 4, 2018
…open

use session.start instead of session.open; see #174; thanks @thomasleplus
davidlougheed added a commit to c3g/chord_singularity that referenced this issue Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants