Merge pull request #42491 from deads2k/auth-03-adder

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 https://github.com/kubernetes/kubernetes/pull/42421 for 1.6.

@kubernetes/sig-auth-pr-reviews @enj @liggitt

pkg/kubeapiserver/authenticator/BUILD

@@ -23,7 +23,6 @@ go_library(
         "//vendor:k8s.io/apiserver/pkg/authentication/request/union",
         "//vendor:k8s.io/apiserver/pkg/authentication/request/x509",
         "//vendor:k8s.io/apiserver/pkg/authentication/token/tokenfile",
-        "//vendor:k8s.io/apiserver/pkg/authentication/user",
         "//vendor:k8s.io/apiserver/plugin/pkg/authenticator/password/keystone",
         "//vendor:k8s.io/apiserver/plugin/pkg/authenticator/password/passwordfile",
         "//vendor:k8s.io/apiserver/plugin/pkg/authenticator/request/basicauth",

pkg/kubeapiserver/authenticator/config.go

@@ -30,7 +30,6 @@ import (
 	"k8s.io/apiserver/pkg/authentication/request/union"
 	"k8s.io/apiserver/pkg/authentication/request/x509"
 	"k8s.io/apiserver/pkg/authentication/token/tokenfile"
-	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/plugin/pkg/authenticator/password/keystone"
 	"k8s.io/apiserver/plugin/pkg/authenticator/password/passwordfile"
 	"k8s.io/apiserver/plugin/pkg/authenticator/request/basicauth"
@@ -207,7 +206,7 @@ func (config AuthenticatorConfig) New() (authenticator.Request, *spec.SecurityDe
 
 	authenticator := union.New(authenticators...)
 
-	authenticator = group.NewGroupAdder(authenticator, []string{user.AllAuthenticated})
+	authenticator = group.NewAuthenticatedGroupAdder(authenticator)
 
 	if config.Anonymous {
 		// If the authenticator chain returns an error, return an error (don't consider a bad bearer token anonymous).

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

@@ -30,7 +30,6 @@ import (
 	"k8s.io/apiserver/pkg/authentication/request/headerrequest"
 	unionauth "k8s.io/apiserver/pkg/authentication/request/union"
 	"k8s.io/apiserver/pkg/authentication/request/x509"
-	"k8s.io/apiserver/pkg/authentication/user"
 	webhooktoken "k8s.io/apiserver/plugin/pkg/authenticator/token/webhook"
 	authenticationclient "k8s.io/client-go/kubernetes/typed/authentication/v1beta1"
 	"k8s.io/client-go/util/cert"
@@ -107,7 +106,7 @@ func (c DelegatingAuthenticatorConfig) New() (authenticator.Request, *spec.Secur
 		return nil, nil, errors.New("No authentication method configured")
 	}
 
-	authenticator := group.NewGroupAdder(unionauth.New(authenticators...), []string{user.AllAuthenticated})
+	authenticator := group.NewAuthenticatedGroupAdder(unionauth.New(authenticators...))
 	if c.Anonymous {
 		authenticator = unionauth.NewFailOnError(authenticator, anonymous.NewAuthenticator())
 	}

staging/src/k8s.io/apiserver/pkg/authentication/group/authenticated_group_adder.go

@@ -0,0 +1,60 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package group
+
+import (
+	"net/http"
+
+	"k8s.io/apiserver/pkg/authentication/authenticator"
+	"k8s.io/apiserver/pkg/authentication/user"
+)
+
+// AuthenticatedGroupAdder adds system:authenticated group when appropriate
+type AuthenticatedGroupAdder struct {
+	// Authenticator is delegated to make the authentication decision
+	Authenticator authenticator.Request
+}
+
+// NewAuthenticatedGroupAdder wraps a request authenticator, and adds the system:authenticated group when appropriate.
+// Authentication must succeed, the user must not be system:anonymous, the groups system:authenticated or system:unauthenticated must
+// not be present
+func NewAuthenticatedGroupAdder(auth authenticator.Request) authenticator.Request {
+	return &AuthenticatedGroupAdder{auth}
+}
+
+func (g *AuthenticatedGroupAdder) AuthenticateRequest(req *http.Request) (user.Info, bool, error) {
+	u, ok, err := g.Authenticator.AuthenticateRequest(req)
+	if err != nil || !ok {
+		return nil, ok, err
+	}
+
+	if u.GetName() == user.Anonymous {
+		return u, true, nil
+	}
+	for _, group := range u.GetGroups() {
+		if group == user.AllAuthenticated || group == user.AllUnauthenticated {
+			return u, true, nil
+		}
+	}
+
+	return &user.DefaultInfo{
+		Name:   u.GetName(),
+		UID:    u.GetUID(),
+		Groups: append(u.GetGroups(), user.AllAuthenticated),
+		Extra:  u.GetExtra(),
+	}, true, nil
+}

staging/src/k8s.io/apiserver/pkg/authentication/group/group_adder_test.go

@@ -40,3 +40,72 @@ func TestGroupAdder(t *testing.T) {
 		t.Errorf("Expected original,added groups, got %#v", user.GetGroups())
 	}
 }
+
+func TestAuthenticatedGroupAdder(t *testing.T) {
+	tests := []struct {
+		name         string
+		inputUser    user.Info
+		expectedUser user.Info
+	}{
+		{
+			name: "add",
+			inputUser: &user.DefaultInfo{
+				Name:   "user",
+				Groups: []string{"some-group"},
+			},
+			expectedUser: &user.DefaultInfo{
+				Name:   "user",
+				Groups: []string{"some-group", user.AllAuthenticated},
+			},
+		},
+		{
+			name: "don't double add",
+			inputUser: &user.DefaultInfo{
+				Name:   "user",
+				Groups: []string{user.AllAuthenticated, "some-group"},
+			},
+			expectedUser: &user.DefaultInfo{
+				Name:   "user",
+				Groups: []string{user.AllAuthenticated, "some-group"},
+			},
+		},
+		{
+			name: "don't add for anon",
+			inputUser: &user.DefaultInfo{
+				Name:   user.Anonymous,
+				Groups: []string{"some-group"},
+			},
+			expectedUser: &user.DefaultInfo{
+				Name:   user.Anonymous,
+				Groups: []string{"some-group"},
+			},
+		},
+		{
+			name: "don't add for unauthenticated group",
+			inputUser: &user.DefaultInfo{
+				Name:   "user",
+				Groups: []string{user.AllUnauthenticated, "some-group"},
+			},
+			expectedUser: &user.DefaultInfo{
+				Name:   "user",
+				Groups: []string{user.AllUnauthenticated, "some-group"},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		adder := authenticator.Request(
+			NewAuthenticatedGroupAdder(
+				authenticator.RequestFunc(func(req *http.Request) (user.Info, bool, error) {
+					return test.inputUser, true, nil
+				}),
+			),
+		)
+
+		user, _, _ := adder.AuthenticateRequest(nil)
+		if !reflect.DeepEqual(user, test.expectedUser) {
+			t.Errorf("got %#v", user)
+		}
+	}
+
+}

vendor/BUILD

@@ -9637,7 +9637,10 @@ go_test(
 
 go_library(
     name = "k8s.io/apiserver/pkg/authentication/group",
-    srcs = ["k8s.io/apiserver/pkg/authentication/group/group_adder.go"],
+    srcs = [
+        "k8s.io/apiserver/pkg/authentication/group/authenticated_group_adder.go",
+        "k8s.io/apiserver/pkg/authentication/group/group_adder.go",
+    ],
     tags = ["automanaged"],
     deps = [
         "//vendor:k8s.io/apiserver/pkg/authentication/authenticator",