fix client authn request parameter name

[nsklikas]

The previous PR had a typo. I tested it against a generic OIDC provider and it now works.

Disclaimer: This logic works for only 1 (client_secret_post) out of the 2 most common client authentication methods (for a list of all the client authn methods see here). I am not sure if there is a way to make it work for any of the other methods using the std go oauth2 lib.

[nsklikas]

We should probably add a note in the Wiki/README, telling users that if their client is not public and if they use a generic OIDC Provider they MUST register the client to use the client_secret_post method

[adombeck]

Thanks for the fix and also for fixing the linked OAuth2 issue, golang/oauth2#685 makes a lot more sense! 😄

I assume this is the section in the spec which defines the client_secret parameter, right? Could you link to that in the comment please?

That section says that HTTP Basic authentication should be preferred over passing the client secret in a URL parameter, which made we wonder if we should support that instead. I looked at what the oauth2 package does (in the cases where it does use the client secret), and it tries both basic auth and URL parameters (and caches which one worked), see https://github.com/golang/oauth2/blob/22134a41033e44c2cd074106770ab5b7ca910d15/internal/token.go#L228-L239.

[adombeck]

We should probably add a note in the Wiki/README, telling users that if their client is not public and if they use a generic OIDC Provider they MUST register the client to use the client_secret_post method

I don't understand this comment. What do you mean with "register the client"?

[3v1n0]

/cc @shipperizer

[nsklikas]

We should probably add a note in the Wiki/README, telling users that if their client is not public and if they use a generic OIDC Provider they MUST register the client to use the client_secret_post method

I don't understand this comment. What do you mean with "register the client"?

On most providers when you create (register) a client, to get the client_id/client_secret pair, you are also asked to configure the client, this can be what grant_types/scopes/audiences the client is allowed to request for, which redirect_uris are allowed for that client, etc. One metadata that you may be requested to configure is which client authentication methods are allowed.

Right now our code works only if the client authentication method is client_secret_post (or public, as in that case there is no client_secret), so my suggestion is that we should warn the user that their client needs to be allowed to use client_secret_post for the flow to work.

That section says that HTTP Basic authentication should be preferred over passing the client secret in a URL parameter, which made we wonder if we should support that instead. I looked at what the oauth2 package does (in the cases where it does use the client secret), and it tries both basic auth and URL parameters (and caches which one worked), see https://github.com/golang/oauth2/blob/22134a41033e44c2cd074106770ab5b7ca910d15/internal/token.go#L228-L239.

The problem is that we are allowed to add extra url params when calling the DeviceAuth function, but the library does not provide us with a way to add extra headers (which is what we would need to do if we wanted to use that client authn method). The only way I see we can implement this is if we patched the http client (we should be able to just add it to the context) that is used by the library to add the header when the request was made. But this sounds too intrusive of a solution, so I would rather avoid it.

[nsklikas]

Any news on this?

[adombeck]

Any news on this?

Sorry, it was a busy week and this PR fell through the cracks. To increase visibility, re-requesting the review would help (but I also repeatedly forget to do that myself, I wish the GitHub UI would make it more obvious whose action is required).

On most providers when you create (register) a client, to get the client_id/client_secret pair, you are also asked to configure the client, this can be what grant_types/scopes/audiences the client is allowed to request for, which redirect_uris are allowed for that client, etc. One metadata that you may be requested to configure is which client authentication methods are allowed.

Right now our code works only if the client authentication method is client_secret_post (or public, as in that case there is no client_secret), so my suggestion is that we should warn the user that their client needs to be allowed to use client_secret_post for the flow to work.

Thanks for explaining. I assume many identity providers don't support this option at all, and that documenting this in a generic way ("register the client to use the client_secret_post method") would be more confusing to users than it would help. Do you have examples of specific providers which can/need to be configured that way? If so, we could document it explicitly for those providers.

The problem is that we are allowed to add extra url params when calling the DeviceAuth function, but the library does not provide us with a way to add extra headers (which is what we would need to do if we wanted to use that client authn method). The only way I see we can implement this is if we patched the http client (we should be able to just add it to the context) that is used by the library to add the header when the request was made. But this sounds too intrusive of a solution, so I would rather avoid it.

It doesn't sound that intrusive to me to add a custom HTTP client to the context which adds the extra headers. We don't have to do it now, but I suspect that we have to do it eventually. So to prepare for that case, I would like us to add a comment that explains that:

1. the spec allows the client credentials to be provided either in the request header (using the HTTP Basic authentication scheme) or the request body
2. some identity providers only support the credentials being passed in the header, some only in the body
3. we only support passing them in the request body for now
4. the oauth2 package currently doesn't handle that for device authentication, but for RetrieveToken (with a link to this comment
5. we could implement that ourselves by adding a custom HTTP client to the context that's passed to oauth2.Config.DeviceAuth.

What do you think?

[nsklikas]

Don't worry, I understand the struggle. I always forget to re-request the review myself, I will try to remember it next time.

It doesn't sound that intrusive to me to add a custom HTTP client to the context which adds the extra headers. We don't have to do it now, but I suspect that we have to do it eventually. So to prepare for that case, I would like us to add a comment that explains that:

1. the spec allows the client credentials to be provided either in the request header (using the HTTP Basic authentication scheme) or the request body

2. some identity providers only support the credentials being passed in the header, some only in the body

3. we only support passing them in the request body for now

4. the oauth2 package currently doesn't handle that for device authentication, but for RetrieveToken (with a link to [this comment](https://github.com/golang/oauth2/blob/22134a41033e44c2cd074106770ab5b7ca910d15/internal/token.go#L228-L239)

5. we could implement that ourselves by adding a custom HTTP client to the context that's passed to `oauth2.Config.DeviceAuth`.

What do you think?

What you say makes sense in general, but let me clarify some things to be sure that we are on the same page:

• The spec says that client authentication must be present in the request. In OIDC/oAuth2 there are several authentication methods, the OIDC spec defines some and there are extra oauth2 RFCs for some more (e.g. jwt and mtls based authn). The most common ones are client_secret_basic and client_secret_post (but for example afaik apple requires a jwt based client authn), I am not sure which of the other client authn methods are supported by golang/oauth2, BUT this shouldn't concern us for now.
• Some providers support some of them and some (very few) support all of them. The public providers support few of them and it is not always clear when registering a client which one is used.
• Some identity providers may allow the user to register multiple client_authentication_methods, some allow only for a single client_authentication_method per client (e.g. hydra, the OIDC server used by the canonical identity platform, is like that) and others (e.g. most public providers) will not present this configuration option to the user.

Regarding patching the HTTP client, my main concern is that the code would be to messy, especially if implementing the client authentication method discovery that the oauth2 lib supports. I agree with leaving it as it is for now and keeping it in the backlog as something that we may have to implement if someone requests it.

I have updated the comment, please let me know if it makes sense to you now.

NOTE: As you said the oauth2 RFC states that HTTP Basic authentication should be preferred over passing the client secret in a URL parameter, BUT entra ID says that:

Due to possible security issues, client_secret_basic client authentication method is not supported.

TBH I am not sure if we should care about any of those 2 claims.

[adombeck]

What you say makes sense in general, but let me clarify some things to be sure that we are on the same page:

• The spec says that client authentication must be present in the request. In OIDC/oAuth2 there are several authentication methods, the OIDC spec defines some and there are extra oauth2 RFCs for some more (e.g. jwt and mtls based authn). The most common ones are client_secret_basic and client_secret_post (but for example afaik apple requires a jwt based client authn), I am not sure which of the other client authn methods are supported by golang/oauth2, BUT this shouldn't concern us for now.
• Some providers support some of them and some (very few) support all of them. The public providers support few of them and it is not always clear when registering a client which one is used.
• Some identity providers may allow the user to register multiple client_authentication_methods, some allow only for a single client_authentication_method per client (e.g. hydra, the OIDC server used by the canonical identity platform, is like that) and others (e.g. most public providers) will not present this configuration option to the user.

Thanks for the additional context. I think it makes sense to reconsider supporting additional authentication methods once they are requested by users.

I have updated the comment, please let me know if it makes sense to you now.

Looks good, thanks!

One last thing: You said in your first comment that you tested this against a generic OIDC provider. Which one is that? Just so that it's clear which providers we already tested (and which we might want to test again if we change this part of code again).

[nsklikas]

I tested against google (but they don't support refresh tokens for the device flow) and against our identity platform deployed at https://iam.dev.canonical.com/stg-identity-jaas-dev-hydra/.well-known/openid-configuration (which uses Ory Hydra) using the generic implementation.

[adombeck]

I tested against google (but they don't support refresh tokens for the device flow) and against our identity platform deployed at https://iam.dev.canonical.com/stg-identity-jaas-dev-hydra/.well-known/openid-configuration (which uses Ory Hydra) using the generic implementation.

Great, I added that to the code comment.