Skip to content

Improve Bearer Token Error Handling #7826

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

Closed
wants to merge 2 commits into from

Conversation

jzheaux
Copy link
Contributor

@jzheaux jzheaux commented Jan 14, 2020

This PR improves how Resource Server handles Bearer Token errors.

Specifically:

  • It adds InvalidBearerTokenException to DefaultAuthenticationEventPublisher.
  • It differentiates between JWT processing errors that are token-related vs configuration or service-related.

These are in the same PR because Resource Server cannot currently determine whether or not a JWT processing error is really an invalid token, and I'd like to address that before registering that exception with DefaultAuthenticationEventPublisher.

Copy link

@sdavids13 sdavids13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InvalidBearerTokenException extends OAuth2AuthenticationException, yet there still isn't a fall-back for catching the OAuth2AuthenticationException in the AuthenticationEventPublisher. Should OAuth2AuthenticationException be converted to an abstract class?

@jzheaux
Copy link
Contributor Author

jzheaux commented Jan 14, 2020

OAuth2AuthenticationException is a generic exception that is thrown in numerous circumstances, including security misconfiguration scenarios. It doesn't seem like a good fit to tie to an event since you'd get alerted to problems that aren't related to bearer token authentication failures.

In the ticket, you said:

I would like to log requests for invalid attempts for authenticating via OAuth 2.0 bearer tokens (via JWT)

By this, I interpret your intent to be Authorization: Bearer scenarios, which is what this PR addresses. Are there other scenarios you are looking to cover?

@jzheaux
Copy link
Contributor Author

jzheaux commented Jan 18, 2020

@sdavids13 Can you describe other scenarios that you'd like to event on? This PR is certainly something that we can iterate on.

@sdavids13
Copy link

Yes, I think this PR fits the bill with what I need/requested. I was merely concerned that someone might inadvertently use the OAuth2AuthenticationException when there is a legitimate authentication error and not get a corresponding audit event. Looking at the documentation of OAuth2AuthenticationException:

This exception is thrown for all OAuth 2.0 related Authentication errors.
There are a number of scenarios where an error may occur, for example:

  • The authorization request or token request is missing a required parameter
  • Missing or invalid client identifier
  • Invalid or mismatching redirection URI
  • The requested scope is invalid, unknown, or malformed
  • The resource owner or authorization server denied the access request
  • Client authentication failed
  • The provided authorization grant (authorization code, resource owner credentials) is invalid, expired, or revoked

I would think for all of those problems we would want to know if someone is attempting to authenticate but being denied for any one of those reasons.

As of right now, I think this PR results in a better out-of-box experience and addressed my primary issue, my only fear is that we aren't catching all events which we may want to audit. But, at the same time, we could merge this PR, and backlog a different ticket which may ask those other questions as to which other events should be audited.

@jzheaux jzheaux force-pushed the gh-7793 branch 3 times, most recently from b1c610c to 6ee8dd6 Compare February 3, 2020 20:49
Updated NimbusJwtDecoder and NimbusReactiveJwtDecoder to throw.
Updated JwtAuthenticationProvider and JwtReactiveAuthenticationManager
to catch.

Fixes spring-projectsgh-7885
@jzheaux
Copy link
Contributor Author

jzheaux commented Feb 5, 2020

Merged to master via fbdecda

@jzheaux jzheaux closed this Feb 5, 2020
@jzheaux jzheaux deleted the gh-7793 branch February 5, 2020 05:00
@jzheaux jzheaux added this to the 5.3.0.RC1 milestone Feb 5, 2020
@jzheaux jzheaux added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: enhancement A general enhancement labels Feb 5, 2020
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 type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants