Optional client secret

[ctron]

I looks like the client secret is optional, and missing for "public" (non-confidential) clients.

To me it looks like in this implementation the client secret is required though.

[kilork]

Hi, I am not quite sure I understand, can you explain what do you mean?

[ctron]

To my understanding, confidential clients have an ID and secret. Public clients only have an ID. So the secret is optional, also on the server side, when validating a token.

Looks like other clients can support this: https://docs.rs/oauth2/4.1.0/oauth2/struct.Client.html#method.new

Taking a look at the spec, I might be that just passing in an empty string would be valid too: https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1 … client_secret is "required" but may be omitted :grin:

[ctron]

I dug a bit deeper, and found out that this crate (despite using String instead of Option<String> for the client secret) can very well handle an empty string for a client secret. That works for my use case, I would suggest to close this issue.

[kilork]

Hi, great it works even without proper API. I recently spend some time reading Rust API guidelines and also specifically about optional parameters in context of one another task. I think we should always do better if we can.

I need in any case check implemented workflows in terms of matching the rfcs. Because we are not only OAuth2 but also OpenID.

[kilork]

I think probably all places there we do:

body.append_pair("client_id", &self.client_id);
body.append_pair("client_secret", &self.client_secret);

in fact should look like this:

if self.provider.credentials_in_body() {
    body.append_pair("client_id", &self.client_id);
    body.append_pair("client_secret", &self.client_secret);
}

@ctron what do you think? Would it be more correct implementation?

[ctron]

The ID seems to be required, but the secret is optional.

What would it more clear would be to use make the client_secret of type Option<String> … and then simply to .unwrap_default().

Then again, it also works "as is" :smiley:

[kilork]

In RFC it is not recommended to include client_id and client_secret in body.

In example for password grant_type they have:

POST /token HTTP/1.1
Host: server.example.com
Authorization: Basic czZCaGRSa3F0MzpnWDFmQmF0M2JW
Content-Type: application/x-www-form-urlencoded

grant_type=password&username=johndoe&password=A3ddj3w

client_id and client_secret here transferred in Authorization header.

https://datatracker.ietf.org/doc/html/rfc6749#section-4.3.2 https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication

[ctron]

Looks like both is possible: https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1 … still "basic auth" seems to be the default and better bet:

Including the client credentials in the request-body using the two parameters is NOT RECOMMENDED and SHOULD be limited to clients unable to directly utilize the HTTP Basic authentication scheme (or other password-based HTTP authentication schemes).

I am just not sure what to do for a public client, one that doesn't have a client secret.

[kilork]

Yeap, I just wonder, can I setup all these flows with keycloak and test them. Or maybe just some public reference implementation endpoint to test all flows :) Hope we do not need another startup and there is something ready to use already.

[ctron]

Keycloak should be able to test all these flows. At least, that is how I ran into those issues :)

[kilork]

I would reproduce issue and check how we can extend library to be more secure.