Repository navigation
Replies: 2 comments 18 replies
|
@lexcao We also have problems due to this change - but it is still IMHO correct to be compliant to the spec, don't you think? Or why would you like to roll back the change? |
|
@lexcao Thanks for tracking the related issues. @lexcao @sschu @jbman +1 that we should be compliant with the specs. So if current behaviour is compliant with the specs, we should not change it. However what we possibly missed is to document this change in the "Upgrading guide", which we we did not and it is probably something, which we should have done due this is kind of change, which breaks the existing behaviour. Even if existing behaviour (before applying this change) relied on the non-specs compliant behaviour. Anyone is welcome to send PR to the "Upgrading guide" for add something about this. |
|
@mposolda We just discussed with @stianst that we will provide a PR to implement the compatible behaviour @jbman suggested. This should even be included in 20.0.2. Since one of our customers stumbled across this issue and he is using Spring, apparently even the Spring client libraries use the wrong encoding. |
|
After trying to implement this I realised the automatic detection might be a bit more complicated since you cannot really decide if the old or new encoding is used. For encoded values that neither contain "/","+" (Base64) or "-","_" (Base64URL), both the old and the new way could have been used. In the old way, this would just be client_id and secret and in the new way, both would have to be URLdecoded first. This is not a problem for the secret since Keycloak generates secrets to not contain URL-unsafe characters - so encoded and unencoded value is the same. For the client id, that is a problem since client ids can contain arbitrary characters. If the value received is not URL-safe, then it was not encoded so it can directly be used as client id. The only safe way I see for that case is to actually look up whether the client exists in Keycloak. WDYT @mposolda ? |
|
I missed the problem that the client ID and secret each of them are expected to be URL encoded by the client. This makes detection not work. Another option is complete new |
|
This could even be used to introduce a Basic-Auth-only-option for clients, as RFC6749 says Basic Auth should be preferred over POST. Would need two new client authenticators (based on same class):
|
|
A colleague of mine did some digging and we discussed this: Resources: Current Basic Auth RFC: https://www.rfc-editor.org/rfc/rfc7617 OAuth2 RFC section about Basic Auth for client authentication using password: https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1 There are to Errata referencing this: https://www.rfc-editor.org/errata/eid4749 by Phil Hunt (co-author of the original RFC) proposes to make the intention of 2-step / double encoding (urlencode + base64) explicit to resolve the conflict with RFC2617 basic auth. These errata are reported but neither rejected nor verified Opinion Keycloak code-wise it seems unlikely (if not impossible) to auto-detekt the method used by the client. Here ii or iii would be most attractive for us, as |
|
@mposolda True, this might be possible. Then maybe the explicit way with compatibility settings is the better one. Would also make it clear for users that there is a compatibility switch which might be deprecated in the future. |
|
+1 |
|
@mposolda I just realised there still needs to be some auto-detection magic because you have to decode the client before you can retrieve its compatibility settings. ;) I will try to create a PR for this. |
|
@mposolda There is one last ambiguity that cannot be resolved. Assume there is a client with client_id |
|
@sschu I wonder if it is possible to throw exception in case that such clients exists? As I don't see why someone want to use both cases in the valid deployment? We can possibly also reject create/update client |
Uh oh!
There was an error while loading. Please reload this page.
Background
This PR #14179 introduced changes on base64 encoding for HTTP basic header,
which made the default implementation to compliant with RFC 2617.
Changes
Tracked issues
I have tracked some issues that maybe be caused by these changes.
Need the Keycloak team to check.
WDYT, @stianst @mposolda
All reactions