openid session storage should be deleted when the authcode is exchanged
Closed this issue · 9 comments
Preflight checklist
- I could not find a solution in the existing issues, docs, nor discussions.
- I agree to follow this project's Code of Conduct.
- I have read and am following this repository's Contribution Guidelines.
- I have joined the Ory Community Slack.
- I am signed up to the Ory Security Patch Newsletter.
Ory Network Project
No response
Describe the bug
I work on a project that use fosite as a golang package and implements the fosite storage interfaces in a custom way. The DeleteOpenIDConnectSession interface has never been called and more recently has been marked deprecated. However, this is a problem for the fosite storage interfaces.
The storage interfaces are very nicely designed and gives implementors lots of flexibility in how the underlying storage works. In our project, for several technical reasons (including that we are not using SQL), each storage interface gets stored in the underlying datastore separately, with no connection to each other. Because the openid sessions are never deleted, we have no choice but to allow them to linger for longer than is actually necessary, using up storage in the underlying datastore.
If I understand the code correctly, there is no reason to keep the openid session data after the authcode is successfully exchanged in PopulateTokenEndpointResponse of flow_explicit_token.go, so that function could call DeleteOpenIDConnectSession before it returns.
I understand that other implementations of the storage interface might choose to store the openid session data along with the authcode session data (e.g. together in a SQL table), and therefore incur less overhead in the underlying datastore. It's nice that the fosite storage interface gives that flexibility. However, the storage interfaces should not require or assume that style of implementation, and none of the other fosite storage interfaces require it except for the openid interface.
Might you consider accepting a PR which removes the deprecation notice on DeleteOpenIDConnectSession and adds a call to that function in the appropriate place? Storage interfaces which do not need it could simply implement it as an empty (no-op) function, which they are most likely already doing anyways.
Reproducing the bug
In the source code, you can see that the DeleteOpenIDConnectSession is never called, and is marked deprecated.
Relevant log output
No response
Relevant configuration
No response
Version
v0.44.0
On which operating system are you observing this issue?
None
In which environment are you deploying?
None
Additional Context
No response
According to the comments on #538 (comment) it was decided to deprecate the DeleteOpenIDConnectSession function because:
Not sure if it makes sense to delete the rows though as it contains info required for refreshing the oidc session
Note that discussion about "rows" is specific to how Hydra chooses to implement the storage interface.
In general, the above is not a true statement about how the fosite storage interfaces work. Note that GetOpenIDConnectSession is only ever called by PopulateTokenEndpointResponse in handler/openid/flow_explicit_token.go. Also note that the PopulateTokenEndpointResponse function only works for authorization code exchanges because the first line of the function returns an error unless the grant type is authorization_code. Therefore, the openid session storage is only ever needed during authorization code exchange, and has no impact on refresh grants.
If Hydra's storage implementation does not want to delete openid session storage during authorization code exchange, it could choose to make its implementation of DeleteOpenIDConnectSession a no-op, rather than deprecating DeleteOpenIDConnectSession.
What about DeleteAccessTokenSession? That also seems it is never called?
See also #798.
@mitar, fosite will call RevokeAccessToken from the TokenRevocationStorage interface instead of calling DeleteAccessTokenSession. I'm not sure of the history or reason behind that, but it has been that way for years.
But that is done only in memory storage implementation. This is not done necessary in all storage implementations?
For example, RevokeAccessToken is called by the token endpoint in several cases where the access token should be revoked based upon client actions. It is up to the implementor of the storage interface to cause that function to delete the access token session from storage.
However, I think you are correct that when the access token has expired, nothing in fosite will call the storage interface to tell it that the token has expired. It seems to be up to the implementor of the storage interface to handle garbage collecting old expired sessions without any help from fosite, if I understand correctly.
Conceptually, why it is safe to delete OIDC session after authcode is exchanged, but not the regular session? Is this because for regular session it is still needed to be able to support refresh tokens?
There's not a "regular session" in the fosite storage interfaces. There are CreateAuthorizeCodeSession, CreatePKCERequestSession, CreateOpenIDConnectSession, CreateAccessTokenSession, CreateRefreshTokenSession (and maybe others?).
Each of those session storage types serve a different purpose and have a different lifecycle based on their purpose.
The data stored by CreateOpenIDConnectSession can only be read by calling GetOpenIDConnectSession. The only caller of GetOpenIDConnectSession is during authorization code exchange in flow_explicit_token.go. So if the authorization code exchange is successful, then nobody will ever call GetOpenIDConnectSession for that session ever again, so it is safe to call DeleteOpenIDConnectSession for it.
This lifecycle is approximately the same for the PKCE request session storage. That is also only needed during authcode exchange, and will never be needed again, so it is deleted.
However, this lifecycle is not the same for the other storage types.
Authorize code session storage lives beyond authcode exchange because fosite uses that as a place to store a true/false state that the authcode has already been exchanged. If another client later tries to exchange the same authcode again, fosite wants to treat that as a special case for security reasons.
Access token and refresh token session storage are both used during the refresh grant, so they need to live longer than the OpenID and PKCE session storage.
Is that what you were asking? Hope that helps.
Thanks!