Possible session management issues

[bungle]

It has been reported to lua-resty-session:
bungle/lua-resty-session#116

That sessions stay locked after the requests. I did some analysis and found these:
bungle/lua-resty-session#116 (comment)
bungle/lua-resty-session#116 (comment)

The main issue is in two public apis:

1. https://github.com/zmartzone/lua-resty-openidc/blob/master/lib/resty/openidc.lua#L1385
2. https://github.com/zmartzone/lua-resty-openidc/blob/master/lib/resty/openidc.lua#L1514

The first issue:
https://github.com/zmartzone/lua-resty-openidc/blob/master/lib/resty/openidc.lua#L1393

There you start the session and start holding the possible lock. If any code path after that does return/error the lock will still be held. You must ensure that lock is always released (with save, close, regenerate or destroy) when start is called. There are a lot of places where the lock is not released, but the code returns. The OpenResty does not have a hook where the session library could hook itself to do it always, so it is library users' responsibility. If it is meant for users to close sessions with openidc.authenticate then it is the calling code that needs to do this always. But I think here your library is the user and it is lua-resty-openidc's responsibility to "close" after start.

The second issue is easy to fix, replace:
https://github.com/zmartzone/lua-resty-openidc/blob/master/lib/resty/openidc.lua#L1518

  return openidc_access_token(opts, session, true)

with:

  local token, err = openidc_access_token(opts, session, true)
  if err then
    session:close()
  end
  return token, err

[bodewig]

Thanks, @bungle . Have you seen https://github.com/zmartzone/lua-resty-openidc#sessions-and-locking ?

we are deliberately not closing the session as users may want to use it after we have returned from authenticate. For access_token things are different as we never return the session.

[bungle]

@bodewig, thanks for the info. Makes sense.

[bodewig]

I'll make sure we close the session in the access_token call the coming days.

Is there any reason why you wouldn't call close in the non-error case in your example above?

[bodewig]

I've also grep-ed for the places where authenticate doesn't return at all (i.e. cases where ngx.exit or ngx.redirect are invoked) and think only

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

Line 282 in 49be803

ngx.exit(ngx.HTTP_BAD_REQUEST)

might cause issues and I'll ensure we handle that as well - unlikely as the code path may be.

[bungle]

@bodewig,

Great findings!

Is there any reason why you wouldn't call close in the non-error case in your example above?

I don't think there is any reason why :close cannot be called on already closed session. You are right, it looks more like a premature optimization.