-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Enable use CAS throttling support with OAuth module. #3013
Enable use CAS throttling support with OAuth module. #3013
Conversation
… AddCASThrottleToOAuth5.2.x
return oAuth20HandlerInterceptorAdapter; | ||
} else { | ||
final HandlerInterceptor throttledInterceptor = this.applicationContext.getBean(throttler, HandlerInterceptor.class); | ||
final String throttledUrl = BASE_OAUTH20_URL.concat("/").concat(ACCESS_TOKEN_URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this only applies to the access-token URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was addressing vulnerabilities with brute forcing password grant type. It should also cover brute forcing a code grant too. I'm struggling to see how authorize path since it redirects to CAS for authentication could be used to get a token, if throttling is configured for CAS authentication too. I suppose that path could be exploited in a DOS attack. The /profile path maybe exploited to try to brute force getting a auth token and profile data as well. That more difficult to exploit given size of the token. I could see wanting to have different throttling rules with /profile path as this is used to verify the token. Given stateless nature of apps using OAuth this may happen often. (I personally don't see how an oauth token isn't just another form of a session id.)
Community should really examine this in detail. I was trying to plug one hole, but there probably other concerns here. Honestly, I don't think throttling should be optional. I think if it is an option it should be possible to disable it but by default it should really be enabled. Having some reasonable defaults to throttling and having it enabled by default would reduce CAS vulnerability to brute force attacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the notes. I recommend you do 2/3 things before we can merge:
- Please update docs to note the settings. Also update the setting with full javadocs to explain what it does. (Much of what you noted down here should go into the javadoc field).
- Please update docs to explain the new functionality and its behavior better.
Then, it would also be good to ping the cas user list and ask how folks feel about turning on throttling by default and what those reasonable settings should be. (In my experience, most deployments tend it want to keep it off; not that I agree but it's sort of the norm).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed. Asked for documentation updates.
@mmoayyed I want to understand the source code ,so i download and gradle it,cause the gradle can't |
Enables CAS Throttle to be configured for accessing an OAuth token. Prevents brute force attacks on OAuth configured CAS on the /accessToken path. Similar to the CAS Rest configuration for CAS Throttle.