Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The OAuth2 client violates the RFC 6749 spec for Basic Auth when encoding client_id & password #231

Closed
schlenk opened this issue Jul 20, 2020 · 4 comments
Assignees

Comments

@schlenk
Copy link

schlenk commented Jul 20, 2020

Describe the bug
RFC 6749 states in 2.3.1 that the client_id and password must be URL encoded before being base64 encoded.
(also see the errata https://www.rfc-editor.org/errata/eid4749 which clarifies it).

The current code in oauth2.py does not do the urlencoding step as far as i can see:

_headers["Authorization"] = "Basic " + base64.b64encode(

@rayluo rayluo self-assigned this Jul 20, 2020
@rayluo
Copy link
Collaborator

rayluo commented Jul 20, 2020

Thanks for the nice finding! Out of curiosity, you were just doing a code review, weren't you? Your existing Microsoft Identity confidential app powered by MSAL still works, doesn't it?
Or, are you using MSAL on other identity platform?

@schlenk
Copy link
Author

schlenk commented Jul 21, 2020

It got mentioned in a bugreport for CZ-NIC/pyoidc#754 and as we decided that we were spec compliant there, i reported it here. So this might actually be buggy on the Azure server side too, did not try with a proper client_id/secret that needs to be quoted.

@rayluo
Copy link
Collaborator

rayluo commented Jul 21, 2020

OK then. For the record, Microsoft identity system is not susceptible to this tricky issue, because:

  • client_id and client_secret generated by Azure AD does not contain special characters, so that encoding would be a no-op anyway.
  • This MSAL Python library runs a different code path.

We will still make some change to make our code base conceptually right, but our existing customers would not be affected by this.

@rayluo
Copy link
Collaborator

rayluo commented Jul 21, 2020

Fixed. Again, this does not affect our existing customers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

2 participants