ClientSecretBasic should not url encode credentials before encoding in base64

[gbmarc1]

Bug in the ClientSecretBasic.
My request was working when using PostMan but not when using this library. I then realized that the Authorization header was not the same between the two requests. My client secret had non-friendly url characters.

class ClientSecretBasic(ClientAuthnMethod):
   ...
    def construct(self, cis, request_args=None, http_args=None, **kwargs):
        ...
        credentials = "{}:{}".format(quote_plus(user), quote_plus(passwd))  # This line should be "{}:{}".format(user, passwd)
        authz = base64.b64encode(credentials.encode("utf-8")).decode("utf-8")

https://github.com/rohe/pyoidc/blob/75275e90457ed7eec71ae383e38f5e43995981c2/src/oic/utils/authn/client.py#L125

[schlenk]

Indeed, the quote_plus is wrong in that case.

RFC 7617/RFC5234/RFC7613 place a few minor restrictions on the valid password and user strings, but those are mostly harmless (no CTL chars %x00-1F / %x7F and no : in the username and some ASCII compatibility or unicode NFC normalization.

(Edit: No, this ignores the explicity mention in RFC 6749)

[gbmarc1]

I have added the pull request #755

[tpazderka]

Looking at the commit message message that introduced that, I think that the quote_plus might need to stay. Or the next line might need to be changed to urlsafe_b64encode.

I am not entirely sure on this one.

[schlenk]

I think the commit message is a little misleading.

When passing the client_secret via HTTP Basic Auth in the header, the quote_plus is probably wrong.

BUT the OAuth2/OpenID connect spec also optionally allows to pass the client secret/id in the body of the request. And that usecase obviously needs the form encoding. (RFC 6749 2.3.1 or OpenID connect section 9) like all parameters in the body of the request.

On the other hand, the client_id/user is more restricted than Basic Auth allows, due to the various usecases where the client ID is passed via URL parameters, but the secret is not.

(Edit: This is wrong)

[schlenk]

Ok, misread the spec there. RFC 6749 explicitly mentions the urlencoding in 2.3.1, but HTTP Basic Auth RFC 7617 is less strict.

The client identifier is encoded using the
"application/x-www-form-urlencoded" encoding algorithm per
Appendix B, and the encoded value is used as the username; the client
password is encoded using the same algorithm and used as the
password.

[gbmarc1]

In rfc2617 page 6, the example contains a space character that would be "+" if it had been url-encoded. The base64 shows that the password is not url_encoded first. RFC 6749 refers to RFC 2617.

If the user agent wishes to send the userid "Aladdin" and password
"open sesame", it would use the following header field:

  Authorization: Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==                                        -> Aladdin:open sesame

[gbmarc1]

Using requests_oauthlib I can retrieve my token using basic auth. They do not "quote_plus" the username and password but encode it in "latin1" before encoding it in base64:

https://github.com/psf/requests/blob/1b417634721ec377abb7f17bc1f215e07202c2f7/requests/auth.py#L28

and msal from microsoft:

https://github.com/AzureAD/microsoft-authentication-library-for-python/blob/0ced4cb65f5a58c748c5792ddd428e2ef8000866/msal/oauth2cli/oauth2.py#L207

[schlenk]

I agree with you, that it is a common bug in the wild to not follow RFC6749 and skip the URL encoding of client_id/password.
RFC 2671 (or the newer, updated RFC 7617) defers encoding of the password to the application, in this case the application is RFC 6749, which explictly demands URL encoding for the password and client_id.

So the current behaviour is actually correct and standards compliant. Not a bug.

You can see that it is a common bug in in-the-wild implementations. If you google a bit for "OAuth2 basic authorization encoding of password" you'll find a few instances of the bug and confusal.

@tpazderka
But maybe we should allow some "escape hatch" for the client side to talk with broken OPs, like we do in a few other places where an extra kwarg allows slight deviations from the spec. As our own client_id/client_secret implementation creates urlsafe values by default, the server side should not need to do anything.

[tpazderka]

OK, I do agree with a kwarg that would allow to skip the quoting. The default should be the same as now.