Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

Use minimal OpenID scope #29

Merged
merged 1 commit into from
Apr 28, 2018
Merged

Use minimal OpenID scope #29

merged 1 commit into from
Apr 28, 2018

Conversation

Miouge1
Copy link
Contributor

@Miouge1 Miouge1 commented Mar 19, 2018

The email and profile scopes are optional in the OpenID spec, therefore it would increase compatibility to not include them in the DefaultScopes. They can always be re-added with --extra-scopes where necessary.

I have tested this with GitLab as an OpenID Provider, which does not support the profile or email scopes.

This works best with #28

@codecov
Copy link

codecov bot commented Mar 19, 2018

Codecov Report

Merging #29 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #29   +/-   ##
=======================================
  Coverage   34.87%   34.87%           
=======================================
  Files           1        1           
  Lines         195      195           
=======================================
  Hits           68       68           
  Misses        121      121           
  Partials        6        6
Impacted Files Coverage Δ
cmd/kuberos/kuberos.go 34.87% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de5c528...eeeac40. Read the comment docs.

@negz
Copy link
Owner

negz commented Mar 21, 2018

I like this, but I worry that it will be a subtle, breaking change for existing users of Kuberos who do use the removed scopes.

What if we renamed --extra-scopes to --scopes and set the default to "profile", "email"? It's a little awkward, but at least:

  • Nothing will change for folks who are not using --extra-scopes.
  • Folks who are using --extra-scopes will have Kuberos refuse to start due to the missing flag, rather than having authentication potentially fail strangely due to the missing scopes.

What do you think?

@codepainters
Copy link

I second to that. My experiments with Kuberos + GitLab-CE failed for this very reason.

@Miouge1 Miouge1 force-pushed the token-scope branch 3 times, most recently from 9135763 to aca93d7 Compare April 25, 2018 15:29
@negz
Copy link
Owner

negz commented Apr 25, 2018

@Miouge1 I haven't heard from you in a while. Are you still interested in working on this PR? If not I can take a look at it next time I get a block of time to work on Kuberos.

@Miouge1
Copy link
Contributor Author

Miouge1 commented Apr 25, 2018

Sure. I kind of forgot about this PR.

Latest commit should cover the --extra-scopes to --scopes rework.
I tested it as follows:

# Compile
$ docker build . -t kuberos

# Overwrite
$ docker run -p 10003:10003 kuberos /kuberos https://accounts.google.com test /dev/null /dev/null --scopes=''
$ curl localhost:10003
<a href="https://accounts.google.com/o/oauth2/v2/auth?access_type=offline&amp;client_id=test&amp;prompt=consent&amp;redirect_uri=http%3A%2F%2Flocalhost%3A10003%2Fui&amp;response_type=code&amp;scope=openid+&amp;state=2a9cef30423cc08be448f3e8842051ee0fc2a71348c9ac481199280900931767">See Other</a>.

# Default
$ docker run -p 10003:10003 kuberos /kuberos https://accounts.google.com test /dev/null /dev/null
$ curl localhost:10003
<a href="https://accounts.google.com/o/oauth2/v2/auth?access_type=offline&amp;client_id=test&amp;prompt=consent&amp;redirect_uri=http%3A%2F%2Flocalhost%3A10003%2Fui&amp;response_type=code&amp;scope=openid+profile+email&amp;state=359e1a4a4e4cbdb0886c244ee748378315b09d732995495ae6bf829c1fb56d03">See Other</a>.

So it looks like it would fit the comment from 21st of March?

@codepainters note that OpenID is broken in GitLab since 10.6, see this GitLab issue

@codepainters
Copy link

@Miouge1 yes, I'm aware of this problem, I already have a workaround deployed (serving the discovery data statically with NginX).

I hit another problem - kubectl fails to refresh token, with some scope issue again. I'm investigating it.

@Miouge1
Copy link
Contributor Author

Miouge1 commented Apr 26, 2018

@codepainters yes it's a known issue with GitLab OIDC, basically their refresh token is missing the id_token field. There is a GitLab issue about it

Any workaround or input is welcome, since I'm running into the same problems :)

@codepainters
Copy link

Thanks for the hint. As a temporary workaround I hacked GitLab - file /opt/gitlab/embedded/service/gitlab-rails/config/initializers/doorkeeper_openid_connect.rb, added a line with expiration 30.days. Obviously insecure, but at least I can move forward, I can live without token refreshing for a while.

@codepainters
Copy link

Actually I've just realised there are also these GitLab issues (and the pull request mentioned at the top of this ticket):
https://gitlab.com/gitlab-org/gitlab-ce/issues/44435
https://gitlab.com/gitlab-org/gitlab-ee/issues/5365
I'm afraid this means I have to switch to other OIDC provider (at least for now). GitLab's implementation seems very immature.

@Miouge1
Copy link
Contributor Author

Miouge1 commented Apr 26, 2018

@codepainters I'm trying to push GitLab to get their act together on the OIDC front, meanwhile I am using auth0.com as a middleware. The free version worked out of the box for me.

@negz
Copy link
Owner

negz commented Apr 28, 2018

@Miouge1 This looks good! Thanks for working on this.

@negz negz merged commit 618b3b1 into negz:master Apr 28, 2018
@codepainters
Copy link

Thanks guys! Docker image is updated as well, right?

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.

None yet

3 participants