proxy not providing user info should cause error #42421

deads2k · 2017-03-02T16:46:04Z

When using a front proxy authenticator it should behave like this:

unrecognized client-cert: skip
unrecognized subject: skip
recognized client-cert/subject, missing username: fail (the data provided was invalid)

In addition, the group information should come from the front proxy and not be later manipulated. Having that separation eliminates weird cases like the proxy saying a user is system:anonymous and the authentication chain adding group system:authenticated.

@kubernetes/sig-auth-pr-reviews

You cannot use a front proxy client certificate as front proxy client and a direct user against the API.

k8s-reviewable · 2017-03-02T16:47:20Z

This change is

deads2k · 2017-03-02T19:55:12Z

@ncdc @enj ptal.

liggitt · 2017-03-02T20:04:02Z

staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go

@@ -111,5 +95,23 @@ func (c DelegatingAuthenticatorConfig) New() (authenticator.Request, *spec.Secur
 	if c.Anonymous {
 		authenticator = unionauth.NewFailOnError(authenticator, anonymous.NewAuthenticator())
 	}
+
+	// front proxies go first, but since they provide group information, we need to ensure that we don't add *more*


watch the if len(authenticators) == 0 case above

might be more straightforward to build the top-level chain at the end, and skip the early returns, e.g.

topLevelAuthenticators []authenticator.Request{} if c.RequestHeaderConfig != nil { topLevelAuthenticators = append(topLevelAuthenticators, ...) } if len(authenticators) > 0 { topLevelAuthenticators = append(topLevelAuthenticators, group.NewGroupAdder(unionauth.New(authenticators...), []string{user.AllAuthenticated})) } if c.Anonymous { topLevelAuthenticators = append(topLevelAuthenticators, anonymous.NewAuthenticator()) } return unionauth.NewFailOnError(topLevelAuthenticators...)

deads2k · 2017-03-02T20:31:00Z

watch the if len(authenticators) == 0 case above

made it a switch case covering all four cases.

liggitt · 2017-03-02T21:05:58Z

staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509.go

@@ -122,10 +123,13 @@ func (a *Verifier) AuthenticateRequest(req *http.Request) (user.Info, bool, erro
 	}

 	if _, err := req.TLS.PeerCertificates[0].Verify(optsCopy); err != nil {
-		return nil, false, err
+		if invalidCertErr, ok := err.(x509.CertificateInvalidError); ok && invalidCertErr.Reason == x509.Expired {


unknown authority is a separate error (x509.UnknownAuthorityError). all the CertificateInvalidError reasons seem like things we'd want to bubble on (and the godoc supports that that error is for "odd" errors and should probably be dealt with uniformly)

liggitt · 2017-03-02T21:21:16Z

/lgtm

k8s-github-robot · 2017-03-02T21:21:41Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: deads2k, liggitt

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubeapiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k,liggitt]

We suggest the following people:
cc @sttts
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

enj · 2017-03-02T21:30:08Z

staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader.go

@@ -107,7 +108,7 @@ func NewSecure(clientCA string, proxyClientNames []string, nameHeaders []string,
 func (a *requestHeaderAuthRequestHandler) AuthenticateRequest(req *http.Request) (user.Info, bool, error) {
 	name := headerValue(req.Header, a.nameHeaders)
 	if len(name) == 0 {
-		return nil, false, nil
+		return nil, false, errors.New("proxy did not provide user information")
 	}
 	groups := allHeaderValues(req.Header, a.groupHeaders)


Don't we need to require at least one group as well?

nope. auth proxies would be well-advised to include either system:authenticated or system:unauthenticated, but are the final say on the identity of the incoming user

hmm, bears more thought. will discuss tomorrow.

deads2k · 2017-03-02T22:42:23Z

We could make an empty usrername from a front proxy turn into system:anonymous and system:unauthenticated and then have a conditional group adder for system:authenticated. It would mean I went through a lot of motion today, but might match expectation.

enj · 2017-03-03T14:32:31Z

How about:

username and groups required, if missing, error
if username == system:anonymous, requires groups == [system:unauthenticated], extras == empty, else error
if any other username, groups must contain system:authenticated and must not contain system:unauthenticated, else error

I see a few problems with it though:

Lots of special casing around username and groups
Not sure if you could go through the front proxy with a token that the API server would understand but the proxy does not (i.e. proxy says you are system:anonymous but if you hit the API server directly you would be authenticated as a valid user).

deads2k · 2017-03-03T16:19:15Z

Updated to special case the anonymous user in our proxy.

liggitt · 2017-03-03T17:27:24Z

This has implications for auth proxies that pass through requests with authorization headers without adding user info. Will revisit the proxy client cert fallback issue in 1.7. Go ahead and open a smaller PR for 1.6 that just includes the improvement to the group adder to fix #42437

k8s-ci-robot · 2017-03-03T17:34:05Z

@deads2k: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins GCE Node e2e	`83f7d34`	link	`@k8s-bot node e2e test this`
Jenkins GKE smoke e2e	`83f7d34`	link	`@k8s-bot cvm gke e2e test this`
Jenkins GCI GKE smoke e2e	`83f7d34`	link	`@k8s-bot gci gke e2e test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@enj

Automatic merge from submit-queue make the system:authenticated group adder smarter Fixes #42437 This prevents the group adder from adding the system:authenticated group when: 1. it's already in the list 2. the user is system:anonymous 3. system:unauthenticated is in the list Smaller alternative to #42421 for 1.6. @kubernetes/sig-auth-pr-reviews @enj @liggitt

k8s-github-robot · 2017-03-06T14:47:22Z

@deads2k PR needs rebase

enj · 2017-05-04T21:01:21Z

Keep open for now I suppose?

k8s-github-robot · 2017-08-03T00:05:00Z

This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review.

cc @deads2k @liggitt

You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days

deads2k added the kind/bug Categorizes issue or PR as related to a bug. label Mar 2, 2017

deads2k assigned liggitt Mar 2, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 2, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Mar 2, 2017

deads2k added this to the v1.6 milestone Mar 2, 2017

liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Mar 2, 2017

liggitt reviewed Mar 2, 2017

View reviewed changes

deads2k force-pushed the auth-02-no-double-group branch from 748887b to 238dc3e Compare March 2, 2017 20:30

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 2, 2017

liggitt added area/security sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Mar 2, 2017

deads2k force-pushed the auth-02-no-double-group branch 2 times, most recently from 8dcee4b to 21c3ff3 Compare March 2, 2017 20:46

liggitt reviewed Mar 2, 2017

View reviewed changes

deads2k force-pushed the auth-02-no-double-group branch from 21c3ff3 to 4e660fb Compare March 2, 2017 21:11

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2017

enj reviewed Mar 2, 2017

View reviewed changes

liggitt added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 2, 2017

deads2k force-pushed the auth-02-no-double-group branch from 4e660fb to d28a8de Compare March 3, 2017 16:18

k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2017

proxy not providing user info should cause error

83f7d34

deads2k force-pushed the auth-02-no-double-group branch from d28a8de to 83f7d34 Compare March 3, 2017 16:19

liggitt modified the milestones: v1.7, v1.6 Mar 3, 2017

deads2k mentioned this pull request Mar 3, 2017

make the system:authenticated group adder smarter #42491

Merged

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2017

liggitt removed this from the v1.7 milestone May 31, 2017

k8s-github-robot closed this Aug 3, 2017

deads2k deleted the auth-02-no-double-group branch August 3, 2017 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proxy not providing user info should cause error #42421

proxy not providing user info should cause error #42421

deads2k commented Mar 2, 2017 •

edited by k8s-github-robot

k8s-reviewable commented Mar 2, 2017

deads2k commented Mar 2, 2017

liggitt Mar 2, 2017

liggitt Mar 2, 2017 •

edited

deads2k commented Mar 2, 2017

liggitt Mar 2, 2017 •

edited

deads2k Mar 2, 2017

liggitt commented Mar 2, 2017

k8s-github-robot commented Mar 2, 2017

enj Mar 2, 2017

liggitt Mar 2, 2017 •

edited

liggitt Mar 2, 2017

deads2k commented Mar 2, 2017

enj commented Mar 3, 2017

deads2k commented Mar 3, 2017

liggitt commented Mar 3, 2017 •

edited

k8s-ci-robot commented Mar 3, 2017

k8s-github-robot commented Mar 6, 2017

enj commented May 4, 2017

k8s-github-robot commented Aug 3, 2017

proxy not providing user info should cause error #42421

proxy not providing user info should cause error #42421

Conversation

deads2k commented Mar 2, 2017 • edited by k8s-github-robot

k8s-reviewable commented Mar 2, 2017

deads2k commented Mar 2, 2017

liggitt Mar 2, 2017

Choose a reason for hiding this comment

liggitt Mar 2, 2017 • edited

Choose a reason for hiding this comment

deads2k commented Mar 2, 2017

liggitt Mar 2, 2017 • edited

Choose a reason for hiding this comment

deads2k Mar 2, 2017

Choose a reason for hiding this comment

liggitt commented Mar 2, 2017

k8s-github-robot commented Mar 2, 2017

enj Mar 2, 2017

Choose a reason for hiding this comment

liggitt Mar 2, 2017 • edited

Choose a reason for hiding this comment

liggitt Mar 2, 2017

Choose a reason for hiding this comment

deads2k commented Mar 2, 2017

enj commented Mar 3, 2017

deads2k commented Mar 3, 2017

liggitt commented Mar 3, 2017 • edited

k8s-ci-robot commented Mar 3, 2017

k8s-github-robot commented Mar 6, 2017

enj commented May 4, 2017

k8s-github-robot commented Aug 3, 2017

deads2k commented Mar 2, 2017 •

edited by k8s-github-robot

liggitt Mar 2, 2017 •

edited

liggitt Mar 2, 2017 •

edited

liggitt Mar 2, 2017 •

edited

liggitt commented Mar 3, 2017 •

edited