Encode ClientId/Secret in Basic Authorization Header per OIDC Spec

[jdmichel]

• See 2.1.6.1 at http://openid.net/specs/openid-connect-basic-1_0.html#TokenRequest
• Also removes the id and secret from the body of the message, although it's unclear if any Identity Providers will actually fail if its included both places, so might be worth leaving for compatibility even though it is against the spec.

Addresses #1737

[Tratcher]

Do you know of any services that don't work with the current bahavior?

[Tratcher]

And for that matter, any providers that this change works with?

[kevinchalet]

it's unclear if any Identity Providers will actually fail if its included both places, so might be worth leaving for compatibility even though it is against the spec.

No, don't do that... it's illegal and standard-compliant servers will reject your request if you do that (e.g my 2 OIDC server libs will return a Multiple client credentials cannot be specified. error).

This change is a potential breaking change (if your credentials include a : or a non-ASCII char in either the identifier or the secret, your app will no longer work with client_secret_basic). You should make it opt-in.

[jdmichel]

This just cost our company an embarrassing amount of time and money to track down. It's mandated as optional in the OAUTH 2.0 spec, but required by the OIDC spec. Your current implementation isn't even allowed by the OIDC spec.
And you knew about this BUG a year and a half ago?
The point of having a spec is that each implementer doesn't a get a vote about what they think makes sense for one reason or another. We implement a spec so that all the different solutions will be interoperable. For once, someone actually makes one of those rare and precious specs that isn't ambiguous and open to interpretation, and then your team decides to ignore it anyway.

[jdmichel]

The identity provider for our company does not work if the client id and secret are included in the body. Any idp that does so is non-compliant.

I would be surprised if the change actually breaks any providers, but if it does, then the bug is on their end. At the very least, the default behavior should be compliant, and a simple flag could be added to do it the old way.

Why would a colon or non-ascii char in the id or secret cause a problem? The spec says that the authorization header should be base64 encoded, so putting the id and secret directly into the header separated by a colon would also be non-compliant.

[kevinchalet]

Your current implementation isn't even allowed by the OIDC spec.

Sending the client credentials in the request form (aka client_secret_post) is definitely allowed by the spec. I suggest reading the full spec instead of the basic paper: http://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication

The identity provider for our company does not work if the client id and secret are included in the body. Any idp that does so is non-compliant.

Wrong. Authorization servers can support client_secret_basic, client_secret_post, client_secret_jwt, private_key_jwt or any custom/implementation-specific mean they want. Only the first one is mandatory, but doesn't mean the client HAS to use basic authentication...

I would be surprised if the change actually breaks any providers, but if it does, then the bug is on their end.

Heh no. Sending credentials in both the headers and the form is not allowed:

The client MUST NOT use more than one authentication method in each request.

https://tools.ietf.org/html/rfc6749#section-2.3

invalid_request
The request is missing a required parameter, includes an unsupported parameter value (other than grant type), repeats a parameter, includes multiple credentials, utilizes more than one mechanism for authenticating the client, or is otherwise malformed.

https://tools.ietf.org/html/rfc6749#section-5.2

Why would a colon or non-ascii char in the id or secret cause a problem?

The identifier and the secret are separated by a :. Now imagine your identifier or your secret has a : and you'll immediately realize how problematic it will be.

Don't misinterpret what I'm saying: I believe the OIDC client handler should support basic authentication to deal with servers that only support this client authentication method. But it shouldn't be the default method because it would be a breaking change.

[kevinchalet]

Why would a colon or non-ascii char in the id or secret cause a problem?

Historically, most of the OAuth2/OpenID Connect servers have implemented client_id/client_secret deformatting the same exact way you did, which is sadly not compliant as both the client_id and the client_secret MUST be formurl-encoded before being concatenated. Otherwise, you'll end up with weird errors if either your client_id or your client_secret contains a : or a non-ASCII character.

Read https://tools.ietf.org/html/rfc7617#section-2 for more information.

[jdmichel]

My PR didn't actually send credentials in both header and form. I only mentioned it in the Issue, because I was trying to think of a way to keep everyone happy. I agree it's a bad idea.

I didn't read the whole spec, but I (maybe mistakenly) thought the OIDC was trying to simplify interop by mandating client_secret_basic.

I see what you mean now by the colon.

I also did notice the failure to specify UTF8, although it seemed to be implied by other places I noticed in the spec that did specify UTF8. I'm well familiar with ambiguity problems in specs, having spent years making CORBA implementations interoperate.

I also understand the resistance to making breaking changes. If I could get all of our (at least the newer core 2+) services to work out of the box with a one-line setting change instead of having to hack in a custom RedeemToken function then I would be very happy. Most of the teams at our company before me ended up either writing their own middleware from scratch, or even implementing the OIDC flows at the app layer with controllers. I've been pushing for our latest project to build all of our services relying on the stock core 2 middleware as much as possible. I don't want us to be in the OIDC/OAUTH business.

[jdmichel]

Just had a brief read through the full spec. Yikes.
I'll try to push our identity provider to at least support client_secret_post as an option, but it would still be nice if core 2+ at least supported client_secret_basic as an option since it was supposed to be the default.

[jdmichel]

For completeness, here's a link to the past discussion of this bug. #1032

[leastprivilege]

If the header is used for client authentication, it should use the encoding specified in RFC6749 - everything else is a bug.

[kevinchalet]

@leastprivilege assuming you're referring to the formurl encoding, even that is quite risky as it has never been implemented correctly in many (if not most) OAuth2/OIDC implementations, including yours and mine 😅

[leastprivilege]

It's correctly implemented in IS4 (but yes - we got it wrong and fixed it recently).

[kevinchalet]

Curious: did you do that in a major release or in a minor one? Did you implement a quirk?

[leastprivilege]

minor. we got some error reports. People accepted it. My client lib has a "legacy mode". The world moved on.

[leastprivilege]

I just checked the OIDC certification tests - they test that the OP supports both the authz header approach as well as POST body.

So it's totally fine for the OIDC handler to POST client id/secret.

BTW - the OIDC handler should get certified. I always tell customers to pick software from that list:
https://openid.net/certification/ - unfortunately the ASP.NET Core OIDC handler is not on it..

@blowdart

[Tratcher]

In summary: We could add this but it would need to be optional opt-in.

Certification issue: https://github.com/aspnet/Security/issues/1197

[Eilon]

Triage discussion: While we think it could be good to support this, we are not taking this PR as-is because we think it needs to be opt-in (off by default, for compatibility), and we would likely also want to add additional test coverage.