Oauth2 client: Allow deescalating logged ERROR for invalid client registration ID

[PunchyRascal]

Current Behavior

Currently, when attempting to work withz invalid client ID, an ERROR is logged : Authorization Request failed: java.lang.IllegalArgumentException: Invalid Client Registration with Id: xxxl

(org.springframework.security.oauth2.client.web.DefaultOAuth2AuthorizationRequestResolver#resolve(javax.servlet.http.HttpServletRequest, java.lang.String, java.lang.String))

Desired Behavior

Avoid ERRORs in logs for this case (either setup or altogether - switch to warning maybe?)

Context

When a security/penetration scan is run on the app, many errors like this are logged, but do not represent any actual problem with the app. All the errors are just a result of the scan. Therefore it would be nice if this error could be silenced - maybe changed to warning.

Thank you.

[jgrandja]

@PunchyRascal There is only one log statement in OAuth2AuthorizationRequestRedirectFilter. Have you considered trying to turn off logging during penetration tests? For example:

logging.level.org.springframework.security.oauth2.client.web.OAuth2AuthorizationRequestRedirectFilter=OFF

[PunchyRascal]

Since the pentest is a regular occurrence, I think that would not be sustainable - to keep turning this off and on again and releasing to prod.

[jgrandja]

@PunchyRascal A correctly configured application would not send an invalid client registration id. I understand your pentest is a different testing scenario but it doesn't make sense to "convenience" a pentest scenario as this would also "convenience" a malicious request that was attempting to guess a valid registration id. I would prefer to be disruptive to a malicious request by keeping this existing behaviour and throwing an exception. Makes sense?

[PunchyRascal]

I could argue that protection against brute force attacks is not in the scope of this module, if that is what you're saying, but hey - who am I? If you do not think this is doable, even via settings, we'll have to manage otherwise 🙂

[jgrandja]

@PunchyRascal

I could argue that protection against brute force attacks is not in the scope of this module, if that is what you're saying

No that's not what I'm saying. As mentioned in previous comment

A correctly configured application would not send an invalid client registration id

An invalid client registration id in the request is an error in the calling application, hence, the signal of the error condition to the caller so they can correct.

[PunchyRascal]

This request is about log messages being generated. I am working under the assumption, that the caller does not have access to the Oauth app's logs.

[jgrandja]

Apologies @PunchyRascal, it looks like I misinterpreted the request. If it's as simple as changing the log message from error to warn for invalid client registration id scenarios then we can apply the enhancement. Please confirm if this is exactly what you're looking for.

[PunchyRascal]

Yes, that would be great.

[sjohnr]

Awesome, thanks @PunchyRascal!

I do want to point out for future users encountering this issue that this change (logging invalid client registration ID at WARN level) will still require many applications to tune their logs to get rid of this warning. However, it will allow this message to be turned off permanently if desired while still retaining other errors in the logs.