Skip to content
This repository has been archived by the owner on Dec 7, 2020. It is now read-only.

Add option to select oauth auth method #436

Closed
wants to merge 2 commits into from
Closed

Add option to select oauth auth method #436

wants to merge 2 commits into from

Conversation

aklinkert
Copy link

Hi,

TL;DR:
This PR implements a config option to choose between basic and post authentication during communication between OAuth client and OAuth server. ✌️

Background:
I am currently building a kubernetes infrastructure for one of my clients and stumbled across this issue yesterday during implementing this gatekeeper as an OIDC proxy for https://onelogin.com . The issue I ran into is the following: https://stackoverflow.com/questions/52635508/what-is-the-meaning-of-this-error-client-authentication-must-only-be-provided , which states to use either basic or post authentication. 🤔

Due to the fact that the gatekeeper is still using go-oidc v1 (as decided and argued about in #407) it is still having a bug as described in coreos/go-oidc#181 which sets the post authentication even when basic is used, so the only quick chance for me to get this one up and running is to implement a config option to let the user choose between basic and post auth (which fixes the problem for me, tested with custom build in my cluster). 😑

@abstractj
Copy link

abstractj commented Nov 30, 2018

@apinnecke could you please file a Jira (https://issues.jboss.org/browse/KEYCLOAK), add the affected version and steps to reproduce the issue? It helps us to plan/prioritize the next releases.

@aklinkert
Copy link
Author

aklinkert commented Nov 30, 2018

@abstractj this is a PR, not an issue. And you obviously didn‘t even read the PR, so please do so first an then merce the PR please. 🤷🏻‍♂️

@abstractj
Copy link

abstractj commented Dec 7, 2018

@apinnecke Jiras are not only for bugs, but for features, tasks and other things as well. Please, read https://github.com/keycloak/keycloak-gatekeeper#help-and-documentation.

@stianst
Copy link
Contributor

stianst commented Dec 7, 2018

The fact that client credentials are sent both in the basic header and the body is a bug for sure. Please file a JIRA as @abstractj has requested.

@@ -49,6 +49,7 @@ func newDefaultConfig() *Config {
MaxIdleConns: 100,
MaxIdleConnsPerHost: 50,
OAuthURI: "/oauth",
OAuthAuthMethod: "basic",
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to 'ClientAuthMethod'. Supported values should be 'secret-basic' and 'secret-body'. It should be prefixed with 'secret-' to differentiate between other authentication mechanisms that may be added in the future. Also, it should be 'body' not 'post' (both basic and body are after all sent in a post).

@abstractj
Copy link

It has been a while since we requested changes to this PR. While the submission is appreciated, we are not able to merge it without the changes request and a Jira as well. Because it has been so long, we are going to close the issue. Feel free to reopen with the changes included, if your time permits.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants