Make OAuth provider discoverable from within a Pod #10845

enj · 2016-09-08T01:07:29Z

Implementation of https://tools.ietf.org/html/draft-ietf-oauth-discovery-04

From within a pod:

root@test:/# curl -k https://openshift.default.svc/.well-known/oauth-authorization-server
{
  "issuer": "https://10.13.129.56:8443",
  "authorization_endpoint": "https://10.13.129.56:8443/oauth/authorize",
  "token_endpoint": "https://10.13.129.56:8443/oauth/token",
  "scopes_supported": [
    "user:full",
    "user:info",
    "user:check-access",
    "user:list-scoped-projects",
    "user:list-projects"
  ],
  "response_types_supported": [
    "code",
    "token"
  ],
  "grant_types_supported": [
    "authorization_code",
    "implicit"
  ],
  "code_challenge_methods_supported": [
    "plain",
    "S256"
  ]
}

cc @gabemontero @bparees

Trello ref:
https://trello.com/c/7uYQSTdR

liggitt · 2016-09-08T04:43:17Z

pkg/oauth/discovery/discovery.go

+			scope.UserListAllProjects,
+		},
+		ResponseTypesSupported: config.AllowedAuthorizeTypes,
+		GrantTypesSupported:    config.AllowedAccessTypes,


only include "authorization_code" for now. the others are included in the osin config, but the impl given to actually validate the other types (client-credentials, assertion, etc) just denies

enj · 2016-09-08T17:34:06Z

[test] since I changed existing code.

enj · 2016-09-08T22:45:28Z

re[test] AWS error

liggitt · 2016-09-10T13:00:50Z

Let's keep the redirect uri stuff in a separate PR. The discovery bits are useful on their own

liggitt · 2016-09-12T15:44:17Z

pkg/cmd/server/origin/master.go

@@ -346,37 +347,76 @@ func (c *MasterConfig) InstallProtectedAPI(container *restful.Container) ([]stri
 	initControllerRoutes(root, "/controllers", c.Options.Controllers != configapi.ControllersDisabled, c.ControllerPlug)
 	initHealthCheckRoute(root, "/healthz")
 	initReadinessCheckRoute(root, "/healthz/ready", c.ProjectAuthorizationCache.ReadyForAccess)
-	initVersionRoute(container, "/version/openshift")
+
+	if err := initVersionRoute(container, "/version/openshift"); err != nil {


why did you make this fatal?

liggitt · 2016-09-12T15:49:12Z

@deads2k I'm wondering how this will fit with the oauth server split stuff we discussed

enj · 2016-09-12T18:13:24Z

re[test] flake on #10080

deads2k · 2016-09-13T12:54:53Z

@deads2k I'm wondering how this will fit with the oauth server split stuff we discussed

You really want to build it like this? Even if you decide to do it like this, you should version this API.

enj · 2016-09-13T14:44:42Z

@deads2k I am not sure if I would call this an API since you can't really perform any actions with it. That being said, the draft has no concept of versioning. Granted I suppose they could just make a new well known URI if they ever needed to make a backwards incompatible change. The data returned by this endpoint is essentially never supposed to change structure.

deads2k · 2016-09-13T15:05:45Z

@deads2k I am not sure if I would call this an API since you can't really perform any actions with it.

You call it externally and rely on its output to behave consistently for the purposes of deserialization so that a client can semantically interpret it. API.

The data returned by this endpoint is essentially never supposed to change structure.

That rarely works out.

liggitt · 2016-09-13T15:20:37Z

the URL and output are dictated by the rfc. curious how you would version that

deads2k · 2016-09-13T15:22:48Z

Didn't realize it was for a spec. Your link to the spec is dead.

deads2k · 2016-09-15T18:32:05Z

@enj Consider a world where the oauth server lives in a process that is separate from the API server. Because other authentication techniques exist, it could actually be in a pod. This has several repercussions:

other than this endpoint, the api server doesn't have to know where the oauth server lives
you won't be able to derive this information through self-inspection
callers of this API will sometimes want externally routable URLs (when redirecting clients) and sometimes want internally routable URLs (when exchanging codes for tokens)

Items 1 and 2 suggest that you could try to find this information via API inspection (magic config map or magic service/route come to mind).

Item 3 could probably be punted on if we expect overall internal traffic to be low.

soltysh

Only two questions, the code itself looks good. As for @deads2k suggestion I'm opposed to versioning that endpoint.

soltysh · 2016-09-20T14:10:48Z

pkg/oauth/discovery/discovery.go

+
+// OauthAuthorizationServerMetadata holds OAuth 2.0 Authorization Server Metadata used for discovery
+// https://tools.ietf.org/id/draft-ietf-oauth-discovery-04.html#rfc.section.2
+type OauthAuthorizationServerMetadata struct {


What about the remaining metadata attributes? Why only those?

I focused on the REQUIRED and RECOMMENDED metadata. A lot of the OPTIONAL ones are not supported by OpenShift (for example, jwks_uri and token_endpoint_auth_signing_alg_values_supported). Since @liggitt just added PKCE support, I could add code_challenge_methods_supported (but that may be out of scope and could certainly be added later). token_endpoint_auth_methods_supported, service_documentation, ui_locales_supported, op_policy_uri, op_tos_uri, protected_resources, etc may all be valid now or in the future, but are probably out of scope as well.

soltysh · 2016-09-20T14:12:24Z

pkg/oauth/discovery/discovery.go

+		Issuer:                masterPublicURL,
+		AuthorizationEndpoint: authorizeURL,
+		TokenEndpoint:         tokenURL,
+		ScopesSupported: []string{ // Note: this list is incomplete, which is allowed per the draft spec


Why these scopes and not others?

I didn't see an easy way for me to get any other scopes. I welcome suggestions for making this list more complete.

Right, the remaining role scope is varying, since it contains actual role name and namespace. Well, we're complaint with the spec, since it allows not to advertise all. Thanks for the info

soltysh

LGTM

soltysh · 2016-09-21T09:34:07Z

pkg/oauth/discovery/discovery.go

+		Issuer:                masterPublicURL,
+		AuthorizationEndpoint: authorizeURL,
+		TokenEndpoint:         tokenURL,
+		ScopesSupported: []string{ // Note: this list is incomplete, which is allowed per the draft spec


Right, the remaining role scope is varying, since it contains actual role name and namespace. Well, we're complaint with the spec, since it allows not to advertise all. Thanks for the info

deads2k · 2016-10-05T11:58:23Z

@deads2k mentioned that this should return a different URL based on if the request comes from inside or outside the cluster. @openshift/api-review @jwforres @gabemontero

I'm not sure its that simple. Sometimes you want to discover externally routable names and sometimes you want to discover internally routable names. Assuming that internal components can hit externally routable names (like this does) gets us a long way.

smarterclayton · 2016-10-05T13:59:40Z

Expectation is author set the route name to a resolvable one. Def add to
docs for this (and godoc)

On Oct 5, 2016, at 7:58 AM, David Eads [email protected] wrote:

@deads2k https://github.com/deads2k mentioned that this should return a
different URL based on if the request comes from inside or outside the
cluster. @openshift/api-review
https://github.com/orgs/openshift/teams/api-review @jwforres
https://github.com/jwforres @gabemontero https://github.com/gabemontero

I'm not sure its that simple. Sometimes you want to discover externally
routable names and sometimes you want to discover internally routable
names. Assuming that internal components can hit externally routable names
(like this does) gets us a long way.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#10845 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABG_p_A8DLW21-wvXEDmK_F0nB4du3u5ks5qw5DhgaJpZM4J3grZ
.

enj · 2016-10-07T13:21:46Z

re[test] flake #11114

liggitt · 2016-10-07T13:27:52Z

pkg/oauth/discovery/discovery.go

+			scope.UserListAllProjects,
+		},
+		ResponseTypesSupported:        config.AllowedAuthorizeTypes,
+		GrantTypesSupported:           osin.AllowedAccessType{osin.AUTHORIZATION_CODE}, // TODO use config.AllowedAccessTypes once our implementation handles other grant types


we also support implicit

smarterclayton · 2016-10-14T19:42:05Z

API is approved

liggitt · 2016-10-14T20:44:22Z

pkg/oauth/discovery/discovery_test.go

+		},
+		GrantTypesSupported: osin.AllowedAccessType{
+			"authorization_code",
+			"__implicit",


"implicit", not "__implicit"

liggitt · 2016-10-14T20:45:24Z

fix the implicit grant type, then LGTM

https://trello.com/c/7uYQSTdR Signed-off-by: Monis Khan <[email protected]>

openshift-bot · 2016-10-17T12:47:17Z

Evaluated for origin test up to fda7222

API Approved by Clayton

enj · 2016-10-17T13:01:14Z

@mfojtik Ready for merge.

mfojtik · 2016-10-17T13:15:58Z

[merge]

openshift-bot · 2016-10-17T14:07:19Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10108/) (Base Commit: 2142bdb)

enj · 2016-10-18T15:58:53Z

Flake on #11381
@mfojtik Please re-merge as that flake is fixed now.

mfojtik · 2016-10-20T13:05:50Z

[merge]

mfojtik · 2016-10-20T16:24:46Z

flake #11240

[merge]

openshift-bot · 2016-10-20T16:29:25Z

Evaluated for origin merge up to fda7222

openshift-bot · 2016-10-20T18:05:55Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10327/) (Base Commit: 9ef2b7f) (Image: devenv-rhel7_5210)

gabemontero · 2016-10-21T00:02:44Z

I pulled down the latest origin containing this merged PR and was able to successfully access .well-known/oauth-authorization-server from the jenkins oauth plugin !!

enj assigned mfojtik and liggitt Sep 8, 2016

liggitt reviewed Sep 8, 2016
View reviewed changes

enj force-pushed the enj/f/oauth_pod_flow branch from cae7b06 to 184e1d8 Compare September 12, 2016 13:55

enj changed the title ~~WIP: Make OAuth provider discoverable from within a Pod~~ Make OAuth provider discoverable from within a Pod Sep 12, 2016

liggitt reviewed Sep 12, 2016
View reviewed changes

enj force-pushed the enj/f/oauth_pod_flow branch 2 times, most recently from 22ec011 to 4068743 Compare September 12, 2016 16:10

soltysh self-assigned this Sep 20, 2016

mfojtik added this to the 1.4.0 milestone Sep 20, 2016

soltysh reviewed Sep 20, 2016

View reviewed changes

enj force-pushed the enj/f/oauth_pod_flow branch from 4068743 to 087c823 Compare September 20, 2016 14:42

soltysh approved these changes Sep 21, 2016

View reviewed changes

enj force-pushed the enj/f/oauth_pod_flow branch 2 times, most recently from 922c14f to ef95e46 Compare October 6, 2016 18:32

liggitt reviewed Oct 7, 2016

View reviewed changes

enj force-pushed the enj/f/oauth_pod_flow branch 2 times, most recently from ec40da1 to 53263fe Compare October 7, 2016 14:53

smarterclayton added api-approved and removed needs-api-review labels Oct 14, 2016

liggitt reviewed Oct 14, 2016

View reviewed changes

Make OAuth provider discoverable from within a Pod

fda7222

https://trello.com/c/7uYQSTdR Signed-off-by: Monis Khan <[email protected]>

enj force-pushed the enj/f/oauth_pod_flow branch from 53263fe to fda7222 Compare October 17, 2016 12:43

openshift-bot merged commit 1ec25ef into openshift:master Oct 20, 2016

liggitt mentioned this pull request Oct 21, 2016

canonical way to find the OAuth authorize URL in a pod? #11486

Closed

enj mentioned this pull request Oct 21, 2016

Add OAuth discovery and SA annotations docs openshift/openshift-docs#3092

Closed

enj mentioned this pull request Jun 20, 2017

Add securityDefinitions to the generated OpenAPI spec #14745

Merged

Make OAuth provider discoverable from within a Pod #10845

Make OAuth provider discoverable from within a Pod #10845

Conversation

enj commented Sep 8, 2016 • edited Loading

Choose a reason for hiding this comment

enj commented Sep 8, 2016

enj commented Sep 8, 2016

liggitt commented Sep 10, 2016

Choose a reason for hiding this comment

liggitt commented Sep 12, 2016

enj commented Sep 12, 2016

deads2k commented Sep 13, 2016

enj commented Sep 13, 2016

deads2k commented Sep 13, 2016

liggitt commented Sep 13, 2016

deads2k commented Sep 13, 2016

deads2k commented Sep 15, 2016

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Oct 5, 2016

smarterclayton commented Oct 5, 2016

enj commented Oct 7, 2016

liggitt Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

smarterclayton commented Oct 14, 2016

Choose a reason for hiding this comment

liggitt commented Oct 14, 2016

openshift-bot commented Oct 17, 2016

enj commented Oct 17, 2016

mfojtik commented Oct 17, 2016

openshift-bot commented Oct 17, 2016

enj commented Oct 18, 2016

mfojtik commented Oct 20, 2016

mfojtik commented Oct 20, 2016

openshift-bot commented Oct 20, 2016

openshift-bot commented Oct 20, 2016 • edited Loading

gabemontero commented Oct 21, 2016

enj commented Sep 8, 2016 •

edited

Loading

liggitt Oct 7, 2016 •

edited

Loading

openshift-bot commented Oct 20, 2016 •

edited

Loading