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

Support OAuth providers that are not RFC6749 compliant #14563

Merged
merged 6 commits into from
Dec 19, 2018
Merged

Support OAuth providers that are not RFC6749 compliant #14563

merged 6 commits into from
Dec 19, 2018

Conversation

tdabasinskas
Copy link
Contributor

@tdabasinskas tdabasinskas commented Dec 18, 2018

Fixes #14562, as per which, Grafana is not able to use some OAuth providers that are not RFC6749 compliant because they do not support client_id and client_secret passed via Basic Authentication HTTP header.

Grafana uses golang.org/x/oauth2 library for OAuth authentication. The library already maintains a list of providers that are consider to be broken and uses POST body payload instead of HTTP headers when using them.

Anyhow, there are some other (usually internal) providers that also do not work with Grafana. To solve the issue while using these providers, the idea is to allow mark them as broken an then use oauth2.BrokenAuthHeaderProvider to append them to the broken provider list.

As part of the PR, I'm introducing new broken_auth_header_provider setting for auth.generic_oauth. The setting is false by default not to break any clients the users have already configured, while setting it to true would trigger the behavior mentioned above.

Please let me know if you prefer naming broken_auth_header_provider somehow differently. Also, I was not entirely sure whether it's better to call RegisterBrokenAuthHeaderProvider() inside social.go, or if login_oauth.go would be a better place. If so, just let me know and I'll move it.

Thanks.

@CLAassistant
Copy link

CLAassistant commented Dec 18, 2018

CLA assistant check
All committers have signed the CLA.

@tdabasinskas tdabasinskas changed the title Broken oauth provider (fixes #14562) Support OAuth providers that are not RFC6749 compliant (fixes #14562) Dec 18, 2018
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity what kind of oauth provider are you using?

Think this is something we may accept. Haven't tested it yet though.

Some initial thoughts. The naming of the setting broken_auth_header_provider isn't that good. It's good that it per default is false, but the name isn't very well describing and basically reflects the technical term used in oauth2 package. Tried to come up with something else but non_oauth2_compatible or non_basic_auth_compatible are not good either. Please think about this and try come up with some suggestions.

Documentation would need to be updated as well, see https://github.com/grafana/grafana/edit/master/docs/sources/auth/generic-oauth.md,

@marefr marefr changed the title Support OAuth providers that are not RFC6749 compliant (fixes #14562) Support OAuth providers that are not RFC6749 compliant Dec 19, 2018
@tdabasinskas
Copy link
Contributor Author

tdabasinskas commented Dec 19, 2018

Hi, @marefr,

Thanks for the comment.

The exact provider we are having issues is an internal one, spanning the whole organization. It is based on IdentityServer3, which is no longer being maintained.

As for naming the setting, if we don't wanna base it on the technical term, we can try referring to the actual thing it does (sending identifier via POST instead headers), e.g.:

  • send_client_credentials_via_post
  • send_client_identifier_in_payload
  • pass_credentials_via_body
  • authenticate_via_post
  • does_not_support_header_auth
  • basic_auth_not_supported
  • use_body_for_credentials

Some of the names above are quite verbose, but I see we already have long names inside Grafana config (e.g. disable_brute_force_login_protection). Please let me know if one of them works. Personally, the first one looks quite OK for me.

I will update the mentioned docs (as well as the code), once we agree on the name of the setting.

@bergquist
Copy link
Contributor

send_client_credentials_via_post sounds good to me. Please add some details to https://github.com/grafana/grafana/blob/master/docs/sources/installation/configuration.md#auth and comment the setting in sample.ini since it might not be very obvious for most users.

@tdabasinskas
Copy link
Contributor Author

Thanks!

I've changed the setting to send_client_credentials_via_post and added documentation to both sample.ini as well as generic-oauth.md.

Please let me know if any further changes are required.

@bergquist
Copy link
Contributor

Great! Thank you for contributing :)

@bergquist bergquist merged commit c201fc1 into grafana:master Dec 19, 2018
@tdabasinskas tdabasinskas deleted the broken_oauth_provider branch December 19, 2018 15:12
@marefr marefr added this to the 6.0 milestone Dec 19, 2018
@tdabasinskas
Copy link
Contributor Author

By the way, @bergquist, seesing the milestone, I guess this is going to be released as part of v6.0 only? No chances seeing this in v5.5 (or some other non-major release), right?

@bergquist bergquist modified the milestones: 6.0, 5.5 Dec 20, 2018
@bergquist
Copy link
Contributor

Moved it to 5.5 but its not likely that we will release 5.5 before 6.0.

@bergquist
Copy link
Contributor

But our goal is to release 6.0 beta in the end of January. Which will include this.

@torkelo torkelo modified the milestones: 5.5, 6.0-beta1 Jan 7, 2019
@ying-jeanne ying-jeanne added the pr/external This PR is from external contributor label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/external This PR is from external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support OAuth providers that are not RFC6749 compliant
6 participants