You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Should rollback the base64 encoding changes for HTTP Basic Auth?
#15427
@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@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.
If the value received is URL-safe and does not contain any % characters, it does not matter whether it was encoded or not as both values are the same.
If the received value is URL-safe and contains % characters, there is no safe way to say whether is was URL-encoded e.g. "%2F" could be part of the client id but it could also have been "/".
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 AbstractClientAuthenticator. Maybe this is the cleanest option, as the new implementation can be used for new clients, while others have the existing one configured.
In Admin Console the existing client authenticator has name "Client Id and Secret". This could be reverted as it was before. A new one would be named "Client ID and Secret (OAuth 2.0 compliant)" and contains the implementation as it is now.
There are to Errata referencing this: https://www.rfc-editor.org/errata/eid6017 proposing to drop the section about applying application/x-www-form-urlencoded which apparently is interpreted as applying urlencoding to the parts of the basic auth's username and password. "RFC 2617 has been superseded by RFC7617 which clearly defines in section 2.1 how a charset can be provided to solve the usecase described with encoding. The original text of this RFC violates the approach described for Basic authentication."
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
As to the RFC:
Personally we support the first errata entry (eid6017) which would result in a rollback of the mentioned change as we think
a) most clients currently do only standard basic-auth without double-encoding
b) basic auth is well established and thin obvious thing to do
c) double encoding seems not to provide measurable benefits in allowable characters for the username + password (only for very uncommon edge cases)
Keycloak code-wise it seems unlikely (if not impossible) to auto-detekt the method used by the client.
So alternatives are:
i) Keep the new implementation
ii) Roll back to previous state with no "extras"
iii) Make the behaviour configurable on Keycloak-installation level (with one or the other interpretation being the default)
iiii) Make the behaviour client selectable (with one or the other interpretation being the default)
Here ii or iii would be most attractive for us, as
i causes more problems than it solves
and iiii introduces a custom extension of the standard - again splitting users and communities.
@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.
@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 / and a client with client_id %2F that has compatibility mode enabled. The first will be encoded RFC-compliant as JTJG, the latter has the same encoded value using the compatible encoding. Keycloak would have to make a decision which one to choose. So even with explicit settings per client, there is a possibility for a client not being able to authenticate if Keycloak selects the wrong one... I am not sure how relevant this problem is, I would at least assume that %XYis not commonly used.
@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 %2F in case that client / exists and vice-versa.
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
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
Replies: 2 comments · 18 replies
Uh oh!
There was an error while loading. Please reload this page.
@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?
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
@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.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
@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.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
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.
If the value received is URL-safe and does not contain any % characters, it does not matter whether it was encoded or not as both values are the same.
If the received value is URL-safe and contains % characters, there is no safe way to say whether is was URL-encoded e.g. "%2F" could be part of the client id but it could also have been "/".
The only safe way I see for that case is to actually look up whether the client exists in Keycloak. WDYT @mposolda ?
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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
AbstractClientAuthenticator. Maybe this is the cleanest option, as the new implementation can be used for new clients, while others have the existing one configured.In Admin Console the existing client authenticator has name "Client Id and Secret". This could be reverted as it was before. A new one would be named "Client ID and Secret (OAuth 2.0 compliant)" and contains the implementation as it is now.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
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):
All reactions
Uh oh!
There was an error while loading. Please reload this page.
A colleague of mine did some digging and we discussed this:
Resources:
Obsoleted Basic Auth RFC referenced by OAuth2 RFC: https://www.rfc-editor.org/rfc/rfc2617
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/eid6017 proposing to drop the section about applying application/x-www-form-urlencoded which apparently is interpreted as applying urlencoding to the parts of the basic auth's username and password. "RFC 2617 has been superseded by RFC7617 which clearly defines in section 2.1 how a charset can be provided to solve the usecase described with encoding. The original text of this RFC violates the approach described for Basic authentication."
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
As to the RFC:
Personally we support the first errata entry (eid6017) which would result in a rollback of the mentioned change as we think
a) most clients currently do only standard basic-auth without double-encoding
b) basic auth is well established and thin obvious thing to do
c) double encoding seems not to provide measurable benefits in allowable characters for the username + password (only for very uncommon edge cases)
Keycloak code-wise it seems unlikely (if not impossible) to auto-detekt the method used by the client.
So alternatives are:
i) Keep the new implementation
ii) Roll back to previous state with no "extras"
iii) Make the behaviour configurable on Keycloak-installation level (with one or the other interpretation being the default)
iiii) Make the behaviour client selectable (with one or the other interpretation being the default)
Here ii or iii would be most attractive for us, as
i causes more problems than it solves
and iiii introduces a custom extension of the standard - again splitting users and communities.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
@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.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
+1
All reactions
Uh oh!
There was an error while loading. Please reload this page.
@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.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
@mposolda There is one last ambiguity that cannot be resolved. Assume there is a client with client_id
/and a client with client_id%2Fthat has compatibility mode enabled. The first will be encoded RFC-compliant asJTJG, the latter has the same encoded value using the compatible encoding. Keycloak would have to make a decision which one to choose. So even with explicit settings per client, there is a possibility for a client not being able to authenticate if Keycloak selects the wrong one... I am not sure how relevant this problem is, I would at least assume that%XYis not commonly used.All reactions
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
@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
%2Fin case that client/exists and vice-versa.All reactions