Client Secret should not be mandatory

[gburboz]

Summary

As per OIDC spec 9. ClientAuthentication one of the client authentication method is none hence we should not have client registration's client-secret as mandatory

Actual Behavior

Current code validates client secret at following two locations and causes exception

OAuth2ClientProperties.validateRegistration()

Caused by: java.lang.IllegalStateException: Client secret must not be empty.
	at org.springframework.boot.autoconfigure.security.oauth2.client.OAuth2ClientProperties.validateRegistration(OAuth2ClientProperties.java:65) ~[spring-boot-autoconfigure-2.0.4.RELEASE.jar:2.0.4.RELEASE]

ClientRegistration.validateAuthorizationCodeGrantType()

Caused by: java.lang.IllegalArgumentException: clientSecret cannot be empty
	at org.springframework.util.Assert.hasText(Assert.java:276) ~[spring-core-5.0.8.RELEASE.jar:5.0.8.RELEASE]
	at org.springframework.security.oauth2.client.registration.ClientRegistration$Builder.validateAuthorizationCodeGrantType(ClientRegistration.java:437) ~[spring-security-oauth2-client-5.0.7.RELEASE.jar:5.0.7.RELEASE]

Expected Behavior

Above validations must be removed and clientId can be populated as query parameter without any client secret (when clientAuthenticationMethod is none).

Version

Spring Security 5

[jgrandja]

@gburboz Client secret is manadatory for Confidential Clients, which is the case for clients configured for the authorization_code grant. It's not mandatory for clients configured for the implicit grant.

As per OIDC spec 9. ClientAuthentication one of the client authentication method is none hence we should not have client registration's client-secret as mandatory

As you can see, the none client authentication method is for Public Clients:

none
The Client does not authenticate itself at the Token Endpoint, either because it uses only the Implicit Flow (and so does not use the Token Endpoint) or because it is a Public Client with no Client Secret or other authentication mechanism.

I'm going to close this issue as the implementation is correct.
Hope this explains things?

[gburboz]

@jgrandja , I urge to please reconsider the decision to close this issue or explain why my below interpretation is incorrect.

My interpretation of none from specification, implies either one of the below and not just first one

1. Either Implicit Flow (token endpoint is not even used in this flow)
2. OR Public Client with no Client Secret (e.g. OP does not care who is RP/Client as long as user authorizes the consent)
3. OR other authentication mechanism (e.g. RP/Client authenticates with 2-way-SSL)

Furthermore if client secret was not set, where by it was really required, in that case OP will reject the invocation anyhow.

[vpavic]

Client secret is manadatory for Confidential Clients, which is the case for clients configured for the authorization_code grant. It's not mandatory for clients configured for the implicit grant.

@jgrandja This statement isn't correct - there's no implication that clients that use authorization code grant are (or must be) confidential clients. The very reason for existence of PKCE (see RFC 7636) is to improve security concerns for public clients that use authorization code grant.

To put it in other words, it's the nature of client application that determines whether client registration should be confidential or public, based on application's capability to confidentially store secrets, not the grant flow that is used.

Besides, the use implicit grant is generally not recommended for some time now, and both native and single-page apps (i.e. the ones that are not able to store secret confidentially) are encouraged to use authorization code grant without client secret, with an additional layer of security in form of PKCE for native apps.

So @gburboz's requirement presented here is perfectly valid as you should be able to use Spring Security in a native app. Of course, as stated above, this should be ideally coupled with PKCE (I see there's #4943) but that's dependent on whether authorization server supports it.

[jgrandja]

@vpavic Thanks for the additional details.

there's no implication that clients that use authorization code grant are (or must be) confidential clients. The very reason for existence of PKCE

Yes this is correct. PKCE is targeted for public clients using the authorization_code code grant and therefore in this flow the client_secret is not used and instead the code_verifier (secret) is used instead.

So @gburboz's requirement presented here is perfectly valid as you should be able to use Spring Security in a native app. Of course, as stated above, this should be ideally coupled with PKCE

As you may have noticed, we haven't implemented PKCE with our current implementation of authorization_code grant so client_secret is required.

@gburboz Regarding your 3rd point:

3. OR other authentication mechanism (e.g. RP/Client authenticates with 2-way-SSL)

Yes, if the provider supports other authentication mechanisms and Spring Security provides that support than client_secret would not be required. As an FYI, the current implementation of Spring Security only supports client_secret_basic and client_secret_post. We do have a couple of issues logged that support other authentication mechanisms for the token endpoint, see #4498 #4943. However, we haven't had the cycles to resolve these issues so it won't make it into the upcoming 5.1 release.

Does this answer your question more thoroughly? Is there any other questions you have?

[gburboz]

Apart from implementation effort that is required to make it optional, do you see any reason why client secret should not be made as optional?

I am okay to make the necessary change and raise the pull request if that helps.

Considering that;

• implementation for client secret to be optional is simple
• this change will automatically support other use cases (e.g. 2-way-ssl for client authentication and PKCE)
• when we start to support other use cases we will be required to make this change
• there is no valid reason why it has to be mandatory even if we are currently not supporting other scenarios (note client_secret_basic and client_secret_post can still send empty secret)
• there is not new threat introduced as OP will reject the invocation requests where by secret is required

[jgrandja]

do you see any reason why client secret should not be made as optional?

As mentioned in my previous comment, the current implementation of Spring Security only supports client_secret_basic and client_secret_post so client_secret is required or the token request will fail.

there is no valid reason why it has to be mandatory even if we are currently not supporting other scenarios (note client_secret_basic and client_secret_post can still send empty secret)

True that an empty client_secret would be sent in the token request and the provider would reject it given that authentication will fail. But what value does this bring to the user? Instead of failing at startup with the validation check, it fails during app runtime when the flow is initiated. This may cause confusion to the user on wondering why it failed and having to troubleshoot the issue. It simply does not make sense to fail at a later point rather than fail-fast at startup.

I agree this is a very simple change to make it optional and we intend on making this change when we implement other authentication mechanisms.

I'm really trying to understand your stance on making this optional at this point in time. Are you being blocked from using the client features? If so, can you please provide specific details on your use case so I can better understand?

[jgrandja]

@vpavic From your comment

you should be able to use Spring Security in a native app

This is not clear to me as the authorization_code grant implementation is based on a Servlet implementation and therefore is targeted for a traditional web application (server deployment - confidential client) - not native or single page app (public client). Can you please clarify so I can understand your comment?

[vpavic]

@jgrandja I don't think you should be making assumptions on client application's packaging/deployment and capability of storing secrets (which is what should determine whether the client is web or native, and confidential or public) based on the technology used for implementation.

These days, you can pretty much package anything inside a container, and likewise, you can run a container pretty much everywhere. Including the environments where you cannot confidentially manage a secret. I'd also add that if someone packages a client secret in an application.properties file inside a Boot app that's also not quite confidential either no matter where you deploy it that way. 🙂

As @gburboz says, the spec permits a client with Token Endpoint authentication method of none, and I believe (as I've also said in our previous discussions) that the framework should align with the specs and provide support for what is allowed there rather than restricting users, or making assumptions on their use cases.

[gburboz]

Thanks @vpavic for further clarifications.

@jgrandja the spec does not dictate any restrictions on client_secret. So as per spec, even in case of client_secret_basic and client_secret_post, it is perfectly valid to have an empty client_secret. Spring Security as a tool should not impose additional validations for client than what is specified in spec or it becomes unusable in some edge cases unless there is strong justification to do so.

Furthermore to accommodate some non-complying popular social OAuth players, I have raised another spring-security/issues/5657.

[jgrandja]

@gburboz After re-reading the spec:

client_secret
REQUIRED. The client secret. The client MAY omit the
parameter if the client secret is an empty string.

Even though it says REQUIRED it also says the client may omit the parameter if it's an empty string. I find this strange to be honest. Also, if a provider allows an empty client secret to be sent for authentication than this is a pretty serious exposure.

Either way, we need to ensure we comply with the spec.
I'll apply this change shortly.

[gburboz]

Thanks for taking up this improvement change request.

Yes I agree with you @jgrandja , that statement is self contradictory and it is only said in an alternative scenario when client_secret is sent as request body while no such thing is mentioned for HTTP Basic Auth.