allow front proxy in front of API #12803

deads2k · 2017-02-03T18:48:15Z

This allows a front proxy to provide complete user information to our API server. This is needed to integrate smoothly with the kube-aggregator.

WIP because I haven't written an integration test yet to handle the wiring. Let's make sure we all agree this roughly right first.

@liggitt @mfojtik
@openshift/api-review for the config api types
@mfojtik this is probably card worthy to get it tested.
@jwforres because you hit it.

liggitt

Looks reasonable

liggitt · 2017-02-05T03:15:34Z

pkg/cmd/server/api/types.go

+// API instead of against the /oauth endpoint.
+type RequestHeaderAuthenticationOptions struct {
+	// ClientCA is a file with the trusted signer certs.  If empty, no request verification is done, and any direct request to the OAuth server can impersonate any identity from this provider, merely by setting a request header.
+	ClientCA string


Require a value here? I wish we had for the RequestHeaderIdentityProvider as well.

edit: looks like validation is requiring it. Update godoc

Add to file references function

Needs to make it into the client ca pool used by the server

Needs to make it into the client ca pool used by the server

Yes, one step at a time I think. I'll open an issue, but I think the wiring was nasty until 1.6.

liggitt · 2017-02-05T03:20:26Z

pkg/cmd/server/origin/master_config.go

+		if err != nil {
+			return nil, err
+		}
+		ret.Handlers = append([]authenticator.Request{requestHeaderAuthenticator}, ret.Handlers...)


Should be first inside the list of handlers in the group adder, right? Why not just add it to authenticators first at the top of the method?

cc @enj - might require additions to the integration test ensuring failures in the auth chain short-circuit

Should be first inside the list of handlers in the group adder, right? Why not just add it to authenticators first at the top of the method?

I was going to make the front proxy tell us all the groups, but I don't feel strongly.

I was going to make the front proxy tell us all the groups, but I don't feel strongly.

I changed my mind. I think I feel strongly. If the front proxy is telling us its user=system:anonymous, group=system:unauthenticated, then we don't want to add system:authenticated.

liggitt · 2017-02-05T03:25:49Z

pkg/cmd/server/api/v1/types.go

+	ClientCommonNames []string `json:"clientCommonNames"`
+
+	// UsernameHeaders is the list of headers to check for user information.  First hit wins.
+	// X-Remote-User is the default


No defaults, right?

No defaults, right?

Isn't this the default 80+% of the time? We can distinguish between nil and []string (or at least we should be able to), why make them specify things they likely want as default.

When it comes to headers being used to convey auth, make them tell us

When it comes to headers being used to convey auth, make them tell us

done.

soltysh · 2017-02-07T13:50:35Z

/cc

deads2k · 2017-02-15T16:31:56Z

rebased.

@enj @mfojtik do you think you could pick up integration tests for this pull?

enj · 2017-02-23T00:21:41Z

pkg/cmd/server/api/validation/master.go

+	if len(config.RequestHeader.ClientCA) == 0 {
+		validationResults.AddErrors(field.Required(fldPath.Child("requestHeader.clientCA"), "must be specified for a secure connection"))
+	}
+	if len(config.RequestHeader.ClientCommonNames) == 0 {


Isn't this supposed to be optional?

It's net new, so we can require it if we want. I'd rather start by requiring it and relax later if we want

enj · 2017-02-23T00:23:03Z

pkg/cmd/server/origin/master_config.go

+		requestHeaderAuthenticator, err := headerrequest.NewSecure(
+			config.AuthConfig.RequestHeader.ClientCA,
+			config.AuthConfig.RequestHeader.ClientCommonNames,
+			config.AuthConfig.RequestHeader.UsernameHeaders,


This only gets defaulted if you round trip the config right?

This only gets defaulted if you round trip the config right?

We do, don't we?

I do not think we do so in integration tests (which is why I had to specify it directly).

deads2k · 2017-02-23T16:43:03Z

Thanks to @enj for tests and the client-cert request wiring.

[test]

enj · 2017-02-23T18:36:27Z

test/integration/front_proxy_test.go

+		expectedProjects sets.String
+	}{
+		{
+			name:           "empty user",


Hmm this may be the wrong behavior. Should we expect the anonymous user or an unauthorized error when you do a GET to the front proxy but it does not set any headers on your behalf? @liggitt

Hmm this may be the wrong behavior. Should we expect the anonymous user or an unauthorized error when you do a GET to the front proxy but it does not set any headers on your behalf? @liggitt

The kube-aggregator won't ever send an empty user. In my head, going through a proxy and not having an identity was a failure, but I can see the case for a proxy only attaches credentials it knows about and sends the rest through.

front proxies already have to set the system:authenticated group explicitly. We should make them explicitly set system:anonymous and system:unauthenticated if they want to proxy an anonymous user

deads2k · 2017-02-28T19:50:38Z

Defaulting removed, validation tightened.

liggitt · 2017-03-01T05:03:44Z

pkg/cmd/server/kubernetes/master_config.go

@@ -112,6 +113,17 @@ func BuildDefaultAPIServer(options configapi.MasterConfig) (*apiserveroptions.Se
 	server.GenericServerRunOptions.TLSCertFile = options.ServingInfo.ServerCert.CertFile
 	server.GenericServerRunOptions.TLSPrivateKeyFile = options.ServingInfo.ServerCert.KeyFile
 	server.GenericServerRunOptions.ClientCAFile = options.ServingInfo.ClientCA
+
+	// TODO this is a terrible hack that should be removed in 1.6


issue or it won't happen

liggitt · 2017-03-01T05:04:29Z

pkg/cmd/server/kubernetes/master_config.go

+		data = append(data, fileBytes...)
+		data = append(data, []byte(separator)...)
+	}
+	tmpFile, err := ioutil.TempFile("", prefix)


permissions on this file are important, given what write access to it allows

deads2k · 2017-03-01T17:48:13Z

@liggitt @enj updated the code to a field we added in the rebase for the client cert CA.

enj · 2017-03-01T21:29:56Z

pkg/cmd/server/kubernetes/master_config.go

@@ -542,3 +552,27 @@ func getAPIResourceConfig(options configapi.MasterConfig) genericapiserver.APIRe

 	return resourceConfig
 }
+
+// TODO remove this func in 1.6 when we get rid of the hack above


Remove this.

@deads2k you forgot to remove concatenateFiles...

enj · 2017-03-01T21:36:58Z

pkg/cmd/server/api/v1/types.go

+type RequestHeaderAuthenticationOptions struct {
+	// ClientCA is a file with the trusted signer certs.  It is required.
+	ClientCA string `json:"clientCA"`
+	// ClientCommonNames is an optional list of common names to require a match from. If empty, any client certificate validated against the clientCA bundle is considered authoritative.


Docs need to be updated to reflect tighter validation.

enj · 2017-03-01T21:39:11Z

pkg/cmd/server/kubernetes/master_config.go

+	if err != nil {
+		glog.Fatalf("Error setting up request header client certificates: %v", err)
+	}
+	if len(requestHeaderCACerts) > 0 {


This check is probably unnecessary.

Eh, nvm that code will give you a nil slice even with a nil error.

enj · 2017-03-01T21:52:06Z

vendor/k8s.io/kubernetes/plugin/pkg/auth/authenticator/request/headerrequest/requestheader.go

@@ -81,8 +109,27 @@ func (a *requestHeaderAuthRequestHandler) AuthenticateRequest(req *http.Request)
 	if len(name) == 0 {
 		return nil, false, nil
 	}
+	groups := allHeaderValues(req.Header, a.groupHeaders)


Does this have to be updated to fail if the proxy sent no headers per @liggitt's comment?

Signed-off-by: Monis Khan <[email protected]>

openshift-bot · 2017-03-02T21:37:40Z

Evaluated for origin test up to 76db97a

openshift-bot · 2017-03-02T22:58:51Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/768/) (Base Commit: e019eb7)

deads2k · 2017-03-03T02:33:26Z

All comments addressed. If we have to revert the upstream pick (it was lgtm'ed) we can do that, but this is needed to move aggregation and aggregated API servers forward. [merge]

openshift-bot · 2017-03-03T02:37:26Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/768/) (Image: devenv-rhel7_6027)

openshift-bot · 2017-03-03T02:37:26Z

Evaluated for origin merge up to 76db97a

deads2k · 2017-03-03T15:18:05Z

@deads2k you forgot to remove concatenateFiles...

Ok, I'll squash it into the permissions commit for later today.

deads2k added the needs-api-review label Feb 3, 2017

deads2k assigned mfojtik and liggitt Feb 3, 2017

liggitt reviewed Feb 5, 2017

View reviewed changes

deads2k force-pushed the auth-01-proxy branch from ba03e6c to 7fc31b9 Compare February 15, 2017 16:31

enj mentioned this pull request Feb 23, 2017

Add integration test for front proxy deads2k/origin#32

Merged

enj reviewed Feb 23, 2017

View reviewed changes

deads2k changed the title ~~[WIP] allow front proxy in front of API~~ allow front proxy in front of API Feb 23, 2017

enj reviewed Feb 23, 2017

View reviewed changes

deads2k added api-approved and removed needs-api-review labels Feb 28, 2017

deads2k force-pushed the auth-01-proxy branch from 3c8d9b9 to ad369ea Compare February 28, 2017 19:49

liggitt reviewed Mar 1, 2017

View reviewed changes

deads2k force-pushed the auth-01-proxy branch 2 times, most recently from c39d81d to fb70686 Compare March 1, 2017 20:55

enj suggested changes Mar 1, 2017

View reviewed changes

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

add front proxy as an option for authenticating to the API

e204190

deads2k force-pushed the auth-01-proxy branch from fb70686 to d16df2c Compare March 2, 2017 19:54

deads2k added 2 commits March 2, 2017 16:06

wire up front proxy authenticator

98bf37e

UPSTREAM: 36774: allow auth proxy to set groups and extra

8d20a24

deads2k force-pushed the auth-01-proxy branch from d16df2c to 7c8a562 Compare March 2, 2017 21:06

UPSTREAM: 42421: proxy not providing user info should cause error

76db97a

enj and others added 2 commits March 2, 2017 16:13

Add integration test for front proxy

ee008eb

Signed-off-by: Monis Khan <[email protected]>

use the extraClientCA as it was intended

f40bb8a

deads2k force-pushed the auth-01-proxy branch from 7c8a562 to f40bb8a Compare March 2, 2017 21:13

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

openshift-bot merged commit a86ef13 into openshift:master Mar 3, 2017

deads2k deleted the auth-01-proxy branch August 3, 2017 19:23

allow front proxy in front of API #12803

allow front proxy in front of API #12803

Conversation

deads2k commented Feb 3, 2017

liggitt 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

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

deads2k Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Feb 7, 2017

deads2k commented Feb 15, 2017

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

deads2k commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Mar 1, 2017

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

openshift-bot commented Mar 2, 2017

openshift-bot commented Mar 2, 2017

deads2k commented Mar 3, 2017

openshift-bot commented Mar 3, 2017 • edited Loading

openshift-bot commented Mar 3, 2017

deads2k commented Mar 3, 2017

deads2k Feb 6, 2017 •

edited

Loading

openshift-bot commented Mar 3, 2017 •

edited

Loading