Add option to configure an external OAuth server #18969

simo5 · 2018-03-13T17:56:39Z

This helps replacing our internal Oauth Server with an external one.
REquires a metadata file to be generated by the external OAuth server and be provided for config.

juanvallejo · 2018-03-13T17:58:00Z

/unassign

simo5 · 2018-03-13T19:33:49Z

@enj PTAL

enj · 2018-03-14T05:31:26Z

pkg/cmd/server/origin/master.go

@@ -84,12 +84,15 @@ func (c *MasterConfig) newOpenshiftNonAPIConfig(kubeAPIServerConfig apiserver.Co
 			SharedInformerFactory: c.ClientGoKubeInformers,
 		},
 		ExtraConfig: NonAPIExtraConfig{
-			EnableOAuth: c.Options.OAuthConfig != nil,
+			EnableOAuth: c.Options.OAuthConfig != nil || c.Options.ExternalOAuthConfig != nil,


enj · 2018-03-14T05:33:32Z

pkg/cmd/server/origin/nonapiserver.go

@@ -60,7 +61,7 @@ func (c completedOpenshiftNonAPIConfig) New(delegationTarget genericapiserver.De
 	// TODO move this up to the spot where we wire the oauth endpoint
 	// Set up OAuth metadata only if we are configured to use OAuth
 	if c.ExtraConfig.EnableOAuth {


Instead have this also check the length of the metadata file

Why is this better ?
Validation already checks the lenght of the metadata file you can't run if it is 0

enj · 2018-03-14T05:35:41Z

pkg/cmd/server/origin/nonapiserver.go

+		var err error
+		oAuthMetadata, err = oauthutil.DecodeOAuthMetadataFile(ExtraConfig.OAuthMetadataFile)
+		if err != nil {
+			glog.Error(err)


Better message

The message is already provided by DecodeOAuthMetadataFile() I do not see why we need to re-wrap it, there is nothing to add really.

enj · 2018-03-14T05:38:57Z

pkg/cmd/server/origin/nonapiserver.go

 	if err != nil {
 		glog.Errorf("Unable to initialize OAuth authorization server metadata route: %v", err)
 		return
 	}

-	mux.UnlistedHandleFunc(path, func(w http.ResponseWriter, req *http.Request) {
+	mux.UnlistedHandleFunc(oauthMetadataEndpoint, func(w http.ResponseWriter, req *http.Request) {


Not sure why you removed the path.

Just moved things around

enj · 2018-03-14T05:39:35Z

pkg/oauth/util/discovery.go

+		return oAuthMetadata, fmt.Errorf("Unable to decode External OAuth Metadata file: %v", err)
+	}
+
+	return oAuthMetadata, nil


This needs validation on the actual fields

enj · 2018-03-14T05:44:01Z

pkg/cmd/server/origin/nonapiserver.go

+
+	if len(ExtraConfig.OAuthMetadataFile) > 0 {
+		var err error
+		oAuthMetadata, err = oauthutil.DecodeOAuthMetadataFile(ExtraConfig.OAuthMetadataFile)


I am wondering if it makes more sense to literally return the file data the user provided after it has passed validation. Then they could include extra fields that osin does not support.

Uhmm, that may be a good point.

Thinking more about this, I do not see how you can put more data without failing to unmarshal the json for verification. So I'll just add fields validation now.

That is not how unmarshalling works. Unknown fields are always ignored. Otherwise you could never add a new field to any serialized object.

simo5 · 2018-03-16T12:58:04Z

/retest

simo5 · 2018-03-23T17:49:35Z

Integration test is broken, uploaded so @enj can take a look as the suggestion to use two clusters in one test was his

simo5 · 2018-03-27T21:00:56Z

@enj finally the fully mocked integration test seem to be working, PTAL

stevekuznetsov · 2018-03-27T21:11:52Z

/retest

stevekuznetsov · 2018-03-27T21:12:55Z

/retest

simo5 · 2018-03-27T21:16:36Z

/retest

simo5 · 2018-03-29T21:37:50Z

@deads2k we discussed about always using a file for oauth metadata, however I think the new PrepOauthMetadata() function I added is a reasonable compromise for 3.10, PTAL

deads2k · 2018-04-02T12:08:04Z

pkg/cmd/server/apis/config/types.go

+	// MetadataFile is a path to a file containing the discovery endpoint for OAuth 2.0 Authorization Server Metadata
+	// for an External OAuth server.
+	// See IETF Draft: // https://tools.ietf.org/html/draft-ietf-oauth-discovery-04#section-2
+	MetadataFile string


Yeah, this field is fine.

deads2k · 2018-04-02T12:09:21Z

pkg/cmd/server/apis/config/types.go

@@ -871,6 +875,13 @@ type OAuthTemplates struct {
 	Error string
 }

+type ExternalOAuthConfig struct {


@liggitt you ready to start an AuthenticationConfig or shall we wait until we split configs and just a little more ugly here? I don't feel strongly.

Do you want me to move this under MasterAuthConfig like for the WebToken stuff ?
Wasn't sure this was appropriate.

I can have a simple ExternalOAuthMetadataFile string in there and avoid a new structure

deads2k · 2018-04-02T14:59:28Z

pkg/cmd/server/apis/config/v1/types.go

+	// OAuthMetadataFile is a path to a file containing the discovery endpoint for OAuth 2.0 Authorization
+	// Server Metadata for an external OAuth server.
+	// See IETF Draft: // https://tools.ietf.org/html/draft-ietf-oauth-discovery-04#section-2
+	OAuthMetadataFile string `json:"oauthMetadataFile"`


Note that this is optional and mutually exclusive in the doc.

deads2k · 2018-04-02T15:01:39Z

pkg/cmd/server/apis/config/types.go

+	// OAuthMetadataFile is a path to a file containing the discovery endpoint for OAuth 2.0 Authorization
+	// Server Metadata for an external OAuth server.
+	// See IETF Draft: // https://tools.ietf.org/html/draft-ietf-oauth-discovery-04#section-2
+	OAuthMetadataFile string


Forgot to update GetMasterFileReferences? I don't see it.

deads2k · 2018-04-02T15:05:00Z

pkg/cmd/server/apis/config/validation/master.go

@@ -171,6 +174,12 @@ func ValidateMasterConfig(config *configapi.MasterConfig, fldPath *field.Path) V
 func ValidateMasterAuthConfig(config configapi.MasterAuthConfig, fldPath *field.Path) ValidationResults {
 	validationResults := ValidationResults{}

+	if len(config.OAuthMetadataFile) > 0 {
+		if _, _, err := oauthutil.LoadOAuthMetadataFile(config.OAuthMetadataFile); err != nil {


This check has made the oauthmetadatafile format part of our API. Make a TODO to fix the package dependency order and promote the struct.

Added in discovery.go, I have a card to deal with this as a followup

simo5 · 2018-04-02T15:19:08Z

@deads2k @enj this last push should fix all nits, PTAL

deads2k · 2018-04-02T15:52:37Z

api approved.

Signed-off-by: Simo Sorce <[email protected]>

enj · 2018-04-02T17:31:45Z

/lgtm

openshift-ci-robot · 2018-04-02T17:31:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, simo5

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

enj · 2018-04-02T17:32:26Z

/hold cancel

openshift-bot · 2018-04-02T18:57:49Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 13, 2018

openshift-ci-robot requested review from enj and juanvallejo March 13, 2018 17:56

simo5 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 13, 2018

openshift-merge-robot added the needs-api-review label Mar 13, 2018

enj previously requested changes Mar 14, 2018

View reviewed changes

simo5 force-pushed the noauthsrv branch from c86d19e to 167f17e Compare March 14, 2018 19:45

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 14, 2018

simo5 requested review from deads2k and removed request for juanvallejo March 15, 2018 14:21

simo5 force-pushed the noauthsrv branch 2 times, most recently from cd0189c to 88f4c48 Compare March 15, 2018 20:37

simo5 force-pushed the noauthsrv branch 2 times, most recently from fd512ee to 495bc22 Compare March 19, 2018 17:41

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 19, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2018

simo5 force-pushed the noauthsrv branch from 495bc22 to 0d3d865 Compare March 19, 2018 19:27

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2018

simo5 force-pushed the noauthsrv branch from 0d3d865 to 18cca83 Compare March 19, 2018 20:01

simo5 force-pushed the noauthsrv branch from b00ccfe to 46cc007 Compare March 27, 2018 20:58

simo5 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 29, 2018

simo5 force-pushed the noauthsrv branch from 91c72f1 to 5bc10a1 Compare March 29, 2018 21:32

simo5 force-pushed the noauthsrv branch from 5bc10a1 to 0bf2e51 Compare March 30, 2018 00:03

deads2k reviewed Apr 2, 2018

View reviewed changes

simo5 force-pushed the noauthsrv branch from 0bf2e51 to 4e9b20c Compare April 2, 2018 13:53

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 2, 2018

simo5 force-pushed the noauthsrv branch from 4e9b20c to 123aa6f Compare April 2, 2018 14:40

deads2k reviewed Apr 2, 2018

View reviewed changes

simo5 force-pushed the noauthsrv branch from 5e51119 to 0fb76b0 Compare April 2, 2018 15:18

deads2k added the api-approved label Apr 2, 2018

simo5 added 2 commits April 2, 2018 13:21

Add option to configure an external OAuth server

855e005

Signed-off-by: Simo Sorce <[email protected]>

External OAuth integration test

17de8d9

Signed-off-by: Simo Sorce <[email protected]>

simo5 force-pushed the noauthsrv branch from 0fb76b0 to 17de8d9 Compare April 2, 2018 17:22

openshift-ci-robot assigned enj Apr 2, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2018

simo5 removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2018

openshift-merge-robot merged commit 5213e07 into openshift:master Apr 2, 2018

Add option to configure an external OAuth server #18969

Add option to configure an external OAuth server #18969

Conversation

simo5 commented Mar 13, 2018

juanvallejo commented Mar 13, 2018

simo5 commented Mar 13, 2018

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

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

Choose a reason for hiding this comment

simo5 commented Mar 16, 2018

simo5 commented Mar 23, 2018

simo5 commented Mar 27, 2018

stevekuznetsov commented Mar 27, 2018

stevekuznetsov commented Mar 27, 2018

simo5 commented Mar 27, 2018

simo5 commented Mar 29, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simo5 commented Apr 2, 2018

deads2k commented Apr 2, 2018

enj commented Apr 2, 2018

openshift-ci-robot commented Apr 2, 2018

enj commented Apr 2, 2018

openshift-bot commented Apr 2, 2018