Allow post and basic client authentication mode for OIDC #1792
Conversation
|
|
| Tests? What servers require this that don't work with the current version? |
| | ||
| if (this.Options.ClientAuthenticationMode == OpenIdConnectClientAuthenticationMode.Basic) | ||
| { | ||
| requestMessage.Headers.Authorization = new AuthenticationHeaderValue("Basic", Convert.ToBase64String(Encoding.UTF8.GetBytes(tokenEndpointRequest.ClientId+":"+tokenEndpointRequest.ClientSecret))); |
There was a problem hiding this comment.
Separate into multiple statements.
| this implementation by default need it |
| A feature like this needs unit tests. See https://github.com/aspnet/Security/tree/dev/test/Microsoft.AspNetCore.Authentication.Test/OpenIdConnect |
| @blowdart for feature triage. |
| We'd need to go beyond unit tests to functional tests against a compliant OIDC service which implements basic auth. However we're not adding major features like this until v3, so I'm putting it in the v3 milestone for now. |
| Note that functional testing would be manual on our part, that's how we do it for all of the other providers. |
| if (this.Options.ClientAuthenticationMode == OpenIdConnectClientAuthenticationMode.Basic) | ||
| { | ||
| var basicHeader = Convert.ToBase64String( | ||
| Encoding.UTF8.GetBytes($"{tokenEndpointRequest.ClientId}:{tokenEndpointRequest.ClientSecret}")); |
There was a problem hiding this comment.
It's worth noting that while it's how it's been implemented by most servers, it's actually not standard-compliant: the OAuth2 specification requires formURL-encoding the client_id and client_secret before base64-encoding them.
There was a problem hiding this comment.
It s the way it is with openid. So why not having it and make it work for everyone
There was a problem hiding this comment.
That’s my point: your PR doesn’t match what the specification says.
To make it compliant, you’ll need to use formURL-encoding.
| Look better it s what it s doing by default |
If you read the specification, you'll see that's not what your PR is doing:
You MUST encode both the username and the password using the form-urlencoded format before base64-encoding and sending them to the server. (I agree the wording is a bit confusing... and that's why most server or client implementations have been historically non-standard-compliant). |
I will had more test on implementation
| Triage discussion: Hi, we're closing this PR because next week we're moving all the code to a different repo and there's no way to move over PRs. But, we do like this feature and would like to work with you to get it in after we move the code. If that sounds good to you, please let us know, and we will work with you on next steps (such as a few code cleanup things). |
| Hi, feel free to contact me when your done |
| Hello, any idea of when this pr can be merged? Let me know if there are anything I can do to help? Am in the process of integrate against an openid provider which only supports basic auth. |
| @bhugot the code has moved to https://github.com/aspnet/aspnetcore. The experience is still a bit rough, but it would be possible to cherry pick your PR over there. |
| @Tratcher did the PR dotnet/aspnetcore#4533 |
7 participants
Introduce the possibility to configure the client authentication mode on access_token endpoint https://tools.ietf.org/html/rfc6749#section-2.3.1
this is another proposal that not break the current behavior and allow basic auth for
Encode ClientId/Secret in Basic Authorization Header per OIDC Spec #1738