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

OAuth2AuthorizationRequest should expire #5145

Closed
jgrandja opened this issue Mar 20, 2018 · 9 comments
Closed

OAuth2AuthorizationRequest should expire #5145

jgrandja opened this issue Mar 20, 2018 · 9 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue

Comments

@jgrandja
Copy link
Contributor

We should ensure the OAuth2AuthorizationRequest expires after the configured time limit.

@jgrandja jgrandja added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Mar 20, 2018
@jgrandja jgrandja added this to the 5.1.0.M2 milestone Mar 20, 2018
@jgrandja jgrandja modified the milestones: 5.1.0.M2, 5.1.0.RC1 Jul 24, 2018
@jgrandja
Copy link
Contributor Author

@rwinch I've been thinking about assigning this as a first-timer issue. However, as I looked into this further, I'm not sure if we really need to implement this. The current and only implementation for AuthorizationRequestRepository is HttpSession-based, including it's reactive counterpart. So the OAuth2AuthorizationRequest(s) will be removed whenever a session is terminated anyway. Although we can be more proactive in expiring OAuth2AuthorizationRequest(s) before a session is terminated, I'm not sure there is value here. What are your thoughts?

@rwinch
Copy link
Member

rwinch commented Nov 30, 2018

Can you remind me why we want to expire it? Where is the configured limit coming from?

@jgrandja
Copy link
Contributor Author

@rwinch You proposed this change to me way back and I logged the issue as I thought it was a good idea at the time as well.

Can you remind me why we want to expire it?

If the user triggers the Authorization Code flow OR the oauth2Login() flow and does not proceed through to the end of the flow, for example, closes the browser at the consent screen, than the OAuth2AuthorizationRequest will essentially be orphaned in the AuthorizationRequestRepository until the session is expired. IMO, I don't see an issue with this? Do you?

Where is the configured limit coming from?

This would be part of the feature if implemented. I'm guessing we would have a setter in the implementation(s) of AuthorizationRequestRepository.

@rwinch
Copy link
Member

rwinch commented Nov 30, 2018

Thanks for the reminder.

It's probably not a huge deal, but I'd like to leave this open. It is technically a memory leak and sessions can be kept around quite a long time.

@AndreasKl
Copy link
Contributor

@rwinch @jgrandja We recently observed this issues which caused an increase in per call overhead in our gateway, due to near 4k of OAuth2AuthorizationRequest attached to a few sessions.

As I did not want to invalidate existing sessions a companion map was added to the session holding a configurable amount of OAuth2AuthorizationRequest state keys.

If you like I could provide a PR for this change? Imho I would prefer a limit based solution over a time based one as a possible attacker could just create a lot of OAuth2AuthorizationRequest just by coincident.

@jgrandja
Copy link
Contributor Author

jgrandja commented Sep 5, 2019

@AndreasKl I believe this solution should have both time-limit and size-limit. The size-limit will prevent resource exhaustion and the time-limit will ensure resources are not wasted.

For the time-limit feature, I believe an OAuth2AuthorizationRequest should expire after about 1-2 minutes as the default (should be configurable). I think this makes sense as a user should proceed past the consent screen after about a minute. Anything longer might mean the user aborted the authorization process and left the OAuth2AuthorizationRequest in an orphaned state.

With the time-limit feature alone, I wonder if the size-limit would be needed?

@AndreasKl
Copy link
Contributor

@jgrandja Ok, started to build a solution for the time based expiration (the prototype can be found in this draft PR)

Now I have a few questions..

  • I'm not sure if 60 seconds are enough, considering time divergences between badly configured servers without NTP in a on-premise cloud. Should we go with 120 seconds or should I add a skew?

  • Should the cleanup become a exchangeable component.
    This component could receive the whole session attributes with the benefit of storing state in the session and could share the same implementation between webflux and servlet.
    Currently it is only one duplicated line so my opinion is more like the "it's not big enough for it's own class".

  • The expireAt is set to null if it should not expire. This has the benefit that I do not have to break the contract of the builder as I otherwise have no easy way to provide a custom Clock instance to the builder. I'm not sure if this is acceptable from an user experience view?

  • I think it is good enough to clean-up on save, but I'm not really sure. What do you think?

@jgrandja jgrandja self-assigned this Sep 30, 2019
@jgrandja
Copy link
Contributor Author

jgrandja commented Oct 1, 2019

@AndreasKl

Should we go with 120 seconds or should I add a skew?

We should definitely add a skew setting and Clock. Maybe 120 secs is better for default.

Should the cleanup become a exchangeable component
This component could receive the whole session attributes with the benefit of storing state in the session and could share the same implementation between webflux and servlet.

I would prefer a component that can be used between webflux and servlet. This would be ideal of course but we'll need to ensure we're not blocking on the reactive impl.

I think it is good enough to clean-up on save, but I'm not really sure. What do you think?

I still feel this is a cross-cutting concern so I would prefer to externalize the expiry logic and have it scheduled. I'd rather not embed the logic in load/save/remove

@jgrandja jgrandja added this to the 5.3.x milestone Nov 18, 2019
@jgrandja jgrandja modified the milestones: 5.3.x, 5.4.x Feb 10, 2020
@jgrandja jgrandja removed this from the 5.4.x milestone Mar 12, 2020
candrews added a commit to candrews/spring-security that referenced this issue Mar 23, 2021
@jgrandja jgrandja removed their assignment Apr 12, 2021
@jgrandja jgrandja self-assigned this Apr 20, 2021
@jgrandja jgrandja added this to the 5.5.0 milestone Apr 20, 2021
candrews added a commit to candrews/spring-security that referenced this issue May 10, 2021
… default

Add setAllowMultipleAuthorizationRequests allowing applications to
revert to the previous functionality should they need to do so.

Closes spring-projectsgh-5145
Intentionally regresses spring-projectsgh-5110
@rwinch rwinch added the status: duplicate A duplicate of another issue label May 12, 2021
@rwinch
Copy link
Member

rwinch commented May 12, 2021

Closing in favor of gh-9649

@rwinch rwinch closed this as completed May 12, 2021
rwinch pushed a commit that referenced this issue May 12, 2021
… default

Add setAllowMultipleAuthorizationRequests allowing applications to
revert to the previous functionality should they need to do so.

Closes gh-5145
Intentionally regresses gh-5110
@jgrandja jgrandja removed this from the 5.5.0 milestone May 13, 2021
@jgrandja jgrandja removed the type: enhancement A general enhancement label May 13, 2021
jgrandja pushed a commit that referenced this issue May 13, 2021
… default

Add setAllowMultipleAuthorizationRequests allowing applications to
revert to the previous functionality should they need to do so.

Closes gh-5145
Intentionally regresses gh-5110
jgrandja pushed a commit that referenced this issue May 14, 2021
… default

Add setAllowMultipleAuthorizationRequests allowing applications to
revert to the previous functionality should they need to do so.

Closes gh-5145
Intentionally regresses gh-5110
jgrandja pushed a commit that referenced this issue May 14, 2021
… default

Add setAllowMultipleAuthorizationRequests allowing applications to
revert to the previous functionality should they need to do so.

Closes gh-5145
Intentionally regresses gh-5110
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
… default

Add setAllowMultipleAuthorizationRequests allowing applications to
revert to the previous functionality should they need to do so.

Closes spring-projectsgh-5145
Intentionally regresses spring-projectsgh-5110
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue
Projects
None yet
3 participants