make an SA an oauthclient by deads2k · Pull Request #8878 · openshift/origin

deads2k · 2016-05-13T18:43:12Z

Still sorting out how to find the correct redirect URIs and figuring out how I can test it, but I'd like feedback on some API changes and the UPSTREAM patch I need to handle tokens.

Only some SA tokens are valid as client secrets, so users have to opt-in and they can still share SA tokens in general for other systems and purposes without exposing themselves to impersonation like this.

deads2k · 2016-05-13T18:44:34Z

pkg/oauth/api/v1/types.go


+	// AdditionalSecrets holds other secrets that may be used to identify the client.  This is useful for rotation
+	// and for service account token validation
+	AdditionalSecrets []string `json:"additionalSecrets,omitempty"`


@openshift/api-review AdditionalSecrets allows us to rotate secrets and allows us to accept multiple SA tokens as valid client secrets.

deads2k · 2016-05-13T18:50:33Z

Any suggestions for how to write an automated test to check these flows?

deads2k · 2016-05-13T20:30:34Z

@sgallagher this flow works. Tested using an app from @stevekuznetsov

https://nodejs.org/en/download/package-manager/#enterprise-linux-and-fedora (use the v6 one)
git clone git@github.com:stevekuznetsov/oauth-echo.git
npm install
edit config.js
node index.js

stevekuznetsov · 2016-05-14T01:48:07Z

I'm not exactly sure what needs to be done to test, but in terms of the app if it's useful, we should:

test the app on Node 0.10
S2I it into an image
write an extended test that deploys it as a pod

Although it would seem like a CLI OAuth client would be better.

deads2k · 2016-05-16T20:39:31Z

@soltysh unit tests to confirm the safety of what we return, if not the complete working flow.

deads2k · 2016-05-18T11:51:35Z

pkg/oauth/api/types.go


+	// AdditionalSecrets holds other secrets that may be used to identify the client.  This is useful for rotation
+	// and for service account token validation
+	AdditionalSecrets []string


@smarterclayton API change here.

soltysh · 2016-05-18T12:05:19Z

LGTM (including tests). Thanks @deads2k !!!

smarterclayton · 2016-05-18T19:44:38Z

Oops

On Wed, May 18, 2016 at 3:35 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/merge FAILURE (
https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5941/
)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8878 (comment)

deads2k · 2016-05-18T20:04:17Z

Oops

Indeed. I think I just thought of a problem I want to solve anyway. SA clients should never get grants automatically given to them. Removing the tag until I get that sorted.

smarterclayton · 2016-05-18T20:23:37Z

Yeah...

On Wed, May 18, 2016 at 4:04 PM, David Eads notifications@github.com
wrote:

Oops

Indeed. I think I just thought of a problem I want to solve anyway. SA
clients should never get grants automatically given to them. Removing the
tag until I get that sorted.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8878 (comment)

liggitt · 2016-05-18T22:01:17Z

@stevekuznetsov had a pull open to let the grant method be per client. Probably need that now

deads2k · 2016-05-18T22:49:30Z

@stevekuznetsov had a pull open to let the grant method be per client. Probably need that now

Setting up a switch just for SAs makes this mergeable. Allowing configuration per oauth client later would be good, but being able to say prompt or deny now on SAs is safe.

deads2k · 2016-05-19T15:04:20Z

@soltysh @openshift/api-review take a look at the last commit. It provides a cluster-admin with a switch to control the grant options for SA oauth clients. I think its required for this pull.

deads2k · 2016-05-19T15:04:30Z

[test]

soltysh · 2016-05-19T15:39:36Z

test/integration/sa_oauthclient_test.go

+		"redirect_uri": []string{clusterAdminClientConfig.Host},
+		"approve":      []string{"true"},
+	}).Encode())
+	authenticatedAuthorizeHTTPRequest3, err := http.NewRequest("POST", clusterAdminClientConfig.Host+"/oauth/approve", postBody)


Maybe use the OpenShiftApprovePrefix constant here, instead of hardcoding the /oauth/approve endpoint.

Maybe use the OpenShiftApprovePrefix constant here, instead of hardcoding the /oauth/approve endpoint.

done

soltysh · 2016-05-19T15:45:46Z

The last commit, LGTM, just a small nit 😉

deads2k · 2016-05-19T16:33:51Z

[merge]

openshift-bot · 2016-05-19T17:10:19Z

continuous-integration/openshift-jenkins/test Running (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3927/)

openshift-bot · 2016-05-19T17:10:19Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5949/) (Image: devenv-rhel7_4239)

openshift-bot · 2016-05-19T17:15:16Z

Evaluated for origin test up to e99d31e

openshift-bot · 2016-05-19T17:20:16Z

Evaluated for origin merge up to 026ba1f

deads2k reviewed May 13, 2016
View reviewed changes

deads2k added the needs-api-review label May 13, 2016

deads2k force-pushed the make-sa-an-oauthclient branch 2 times, most recently from 453145f to 76a0883 Compare May 16, 2016 20:36

deads2k changed the title ~~[WIP] make an SA an oauthclient~~ make an SA an oauthclient May 16, 2016

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2016

deads2k force-pushed the make-sa-an-oauthclient branch 2 times, most recently from 539592e to 66a58ae Compare May 17, 2016 11:40

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2016

deads2k force-pushed the make-sa-an-oauthclient branch 2 times, most recently from 8e40abd to 1b46039 Compare May 17, 2016 17:13

deads2k reviewed May 18, 2016
View reviewed changes

deads2k force-pushed the make-sa-an-oauthclient branch from 1b46039 to 25de1a1 Compare May 18, 2016 11:55

deads2k mentioned this pull request May 18, 2016

add explanation of scoped tokens openshift/openshift-docs#2111

Merged

smarterclayton added api-approved and removed needs-api-review labels May 18, 2016

deads2k force-pushed the make-sa-an-oauthclient branch from 25de1a1 to a80d1b1 Compare May 18, 2016 18:58

deads2k added 3 commits May 19, 2016 11:03

UPSTREAM: RangelReale/osin: <carry>: only request secret validation

2cba692

add AdditionalSecrets to oauthclients

84a3f64

use service account credentials as oauth token

633d698

deads2k force-pushed the make-sa-an-oauthclient branch from a80d1b1 to 28dc558 Compare May 19, 2016 15:03

deads2k force-pushed the make-sa-an-oauthclient branch from 28dc558 to f1536e7 Compare May 19, 2016 15:24

soltysh reviewed May 19, 2016
View reviewed changes

deads2k force-pushed the make-sa-an-oauthclient branch from f1536e7 to e99d31e Compare May 19, 2016 16:33

allow configuration of the SA oauth client grant flows

026ba1f

deads2k force-pushed the make-sa-an-oauthclient branch from e99d31e to 026ba1f Compare May 19, 2016 17:18

openshift-bot merged commit 8ac876b into openshift:master May 19, 2016

deads2k deleted the make-sa-an-oauthclient branch September 6, 2016 17:13

Conversation

deads2k commented May 13, 2016

Uh oh!

deads2k May 13, 2016

Choose a reason for hiding this comment

Uh oh!

deads2k commented May 13, 2016

Uh oh!

deads2k commented May 13, 2016

Uh oh!

stevekuznetsov commented May 14, 2016

Uh oh!

deads2k commented May 16, 2016

Uh oh!

deads2k May 18, 2016

Choose a reason for hiding this comment

Uh oh!

soltysh commented May 18, 2016

Uh oh!

smarterclayton commented May 18, 2016

Uh oh!

deads2k commented May 18, 2016

Uh oh!

smarterclayton commented May 18, 2016

Uh oh!

liggitt commented May 18, 2016

Uh oh!

deads2k commented May 18, 2016

Uh oh!

deads2k commented May 19, 2016

Uh oh!

deads2k commented May 19, 2016

Uh oh!

soltysh May 19, 2016

Choose a reason for hiding this comment

Uh oh!

deads2k May 19, 2016

Choose a reason for hiding this comment

Uh oh!

soltysh commented May 19, 2016

Uh oh!

deads2k commented May 19, 2016

Uh oh!

openshift-bot commented May 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented May 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented May 19, 2016

Uh oh!

openshift-bot commented May 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

openshift-bot commented May 19, 2016 •

edited

Loading

openshift-bot commented May 19, 2016 •

edited

Loading