Merge pull request #41661 from liggitt/satoken

Automatic merge from submit-queue

Make controller-manager resilient to stale serviceaccount tokens

Now that the controller manager is spinning up controller loops using service accounts, we need to be more proactive in making sure the clients will actually work.

Future additional work:
* make a controller that reaps invalid service account tokens (c.f. https://github.com/kubernetes/kubernetes/issues/20165)
* allow updating the client held by a controller with a new token while the controller is running (c.f. https://github.com/kubernetes/kubernetes/issues/4672)

cmd/cloud-controller-manager/app/controllermanager.go

@@ -135,9 +135,10 @@ func Run(s *options.CloudControllerManagerServer, cloud cloudprovider.Interface)
 		var clientBuilder controller.ControllerClientBuilder
 		if len(s.ServiceAccountKeyFile) > 0 && s.UseServiceAccountCredentials {
 			clientBuilder = controller.SAControllerClientBuilder{
-				ClientConfig: restclient.AnonymousClientConfig(kubeconfig),
-				CoreClient:   kubeClient.Core(),
-				Namespace:    "kube-system",
+				ClientConfig:         restclient.AnonymousClientConfig(kubeconfig),
+				CoreClient:           kubeClient.Core(),
+				AuthenticationClient: kubeClient.Authentication(),
+				Namespace:            "kube-system",
 			}
 		} else {
 			clientBuilder = rootClientBuilder

cmd/kube-controller-manager/app/controllermanager.go

@@ -159,9 +159,10 @@ func Run(s *options.CMServer) error {
 		var clientBuilder controller.ControllerClientBuilder
 		if len(s.ServiceAccountKeyFile) > 0 && s.UseServiceAccountCredentials {
 			clientBuilder = controller.SAControllerClientBuilder{
-				ClientConfig: restclient.AnonymousClientConfig(kubeconfig),
-				CoreClient:   kubeClient.Core(),
-				Namespace:    "kube-system",
+				ClientConfig:         restclient.AnonymousClientConfig(kubeconfig),
+				CoreClient:           kubeClient.Core(),
+				AuthenticationClient: kubeClient.Authentication(),
+				Namespace:            "kube-system",
 			}
 		} else {
 			clientBuilder = rootClientBuilder

pkg/controller/BUILD

@@ -22,8 +22,10 @@ go_library(
         "//pkg/api:go_default_library",
         "//pkg/api/v1:go_default_library",
         "//pkg/api/validation:go_default_library",
+        "//pkg/apis/authentication/v1:go_default_library",
         "//pkg/apis/extensions/v1beta1:go_default_library",
         "//pkg/client/clientset_generated/clientset:go_default_library",
+        "//pkg/client/clientset_generated/clientset/typed/authentication/v1:go_default_library",
         "//pkg/client/clientset_generated/clientset/typed/core/v1:go_default_library",
         "//pkg/client/retry:go_default_library",
         "//pkg/serviceaccount:go_default_library",

pkg/controller/client_builder.go

@@ -31,7 +31,9 @@ import (
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/v1"
+	v1authenticationapi "k8s.io/kubernetes/pkg/apis/authentication/v1"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
+	v1authentication "k8s.io/kubernetes/pkg/client/clientset_generated/clientset/typed/authentication/v1"
 	v1core "k8s.io/kubernetes/pkg/client/clientset_generated/clientset/typed/core/v1"
 	"k8s.io/kubernetes/pkg/serviceaccount"
 
@@ -109,6 +111,10 @@ type SAControllerClientBuilder struct {
 	// to construct a controller client
 	CoreClient v1core.CoreV1Interface
 
+	// AuthenticationClient is used to check API tokens to make sure they are valid before
+	// building a controller client from them
+	AuthenticationClient v1authentication.AuthenticationV1Interface
+
 	// Namespace is the namespace used to host the service accounts that will back the
 	// controllers.  It must be highly privileged namespace which normal users cannot inspect.
 	Namespace string
@@ -117,29 +123,13 @@ type SAControllerClientBuilder struct {
 // config returns a complete clientConfig for constructing clients.  This is separate in anticipation of composition
 // which means that not all clientsets are known here
 func (b SAControllerClientBuilder) Config(name string) (*restclient.Config, error) {
-	clientConfig := restclient.AnonymousClientConfig(b.ClientConfig)
-
-	// we need the SA UID to find a matching SA token
-	sa, err := b.CoreClient.ServiceAccounts(b.Namespace).Get(name, metav1.GetOptions{})
-	if err != nil && !apierrors.IsNotFound(err) {
+	sa, err := b.getOrCreateServiceAccount(name)
+	if err != nil {
 		return nil, err
-	} else if apierrors.IsNotFound(err) {
-		// check to see if the namespace exists.  If it isn't a NotFound, just try to create the SA.
-		// It'll probably fail, but perhaps that will have a better message.
-		if _, err := b.CoreClient.Namespaces().Get(b.Namespace, metav1.GetOptions{}); apierrors.IsNotFound(err) {
-			_, err = b.CoreClient.Namespaces().Create(&v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: b.Namespace}})
-			if err != nil && !apierrors.IsAlreadyExists(err) {
-				return nil, err
-			}
-		}
-
-		sa, err = b.CoreClient.ServiceAccounts(b.Namespace).Create(
-			&v1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Namespace: b.Namespace, Name: name}})
-		if err != nil {
-			return nil, err
-		}
 	}
 
+	var clientConfig *restclient.Config
+
 	lw := &cache.ListWatch{
 		ListFunc: func(options metav1.ListOptions) (runtime.Object, error) {
 			options.FieldSelector = fields.SelectorFromSet(map[string]string{api.SecretTypeField: string(v1.SecretTypeServiceAccountToken)}).String()
@@ -159,14 +149,32 @@ func (b SAControllerClientBuilder) Config(name string) (*restclient.Config, erro
 				return false, fmt.Errorf("error watching")
 
 			case watch.Added, watch.Modified:
-				secret := event.Object.(*v1.Secret)
-				if !serviceaccount.IsServiceAccountToken(secret, sa) ||
-					len(secret.Data[v1.ServiceAccountTokenKey]) == 0 {
+				secret, ok := event.Object.(*v1.Secret)
+				if !ok {
+					return false, fmt.Errorf("unexpected object type: %T", event.Object)
+				}
+				if !serviceaccount.IsServiceAccountToken(secret, sa) {
 					return false, nil
 				}
-				// TODO maybe verify the token is valid
-				clientConfig.BearerToken = string(secret.Data[v1.ServiceAccountTokenKey])
-				restclient.AddUserAgent(clientConfig, apiserverserviceaccount.MakeUsername(b.Namespace, name))
+				if len(secret.Data[v1.ServiceAccountTokenKey]) == 0 {
+					return false, nil
+				}
+				validConfig, valid, err := b.getAuthenticatedConfig(sa, string(secret.Data[v1.ServiceAccountTokenKey]))
+				if err != nil {
+					glog.Warningf("error validating API token for %s/%s in secret %s: %v", sa.Name, sa.Namespace, secret.Name, err)
+					// continue watching for good tokens
+					return false, nil
+				}
+				if !valid {
+					glog.Warningf("secret %s contained an invalid API token for %s/%s", secret.Name, sa.Name, sa.Namespace)
+					// try to delete the secret containing the invalid token
+					if err := b.CoreClient.Secrets(secret.Namespace).Delete(secret.Name, &metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
+						glog.Warningf("error deleting secret %s containing invalid API token for %s/%s: %v", secret.Name, sa.Name, sa.Namespace, err)
+					}
+					// continue watching for good tokens
+					return false, nil
+				}
+				clientConfig = validConfig
 				return true, nil
 
 			default:
@@ -180,6 +188,69 @@ func (b SAControllerClientBuilder) Config(name string) (*restclient.Config, erro
 	return clientConfig, nil
 }
 
+func (b SAControllerClientBuilder) getOrCreateServiceAccount(name string) (*v1.ServiceAccount, error) {
+	sa, err := b.CoreClient.ServiceAccounts(b.Namespace).Get(name, metav1.GetOptions{})
+	if err == nil {
+		return sa, nil
+	}
+	if !apierrors.IsNotFound(err) {
+		return nil, err
+	}
+
+	// Create the namespace if we can't verify it exists.
+	// Tolerate errors, since we don't know whether this component has namespace creation permissions.
+	if _, err := b.CoreClient.Namespaces().Get(b.Namespace, metav1.GetOptions{}); err != nil {
+		b.CoreClient.Namespaces().Create(&v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: b.Namespace}})
+	}
+
+	// Create the service account
+	sa, err = b.CoreClient.ServiceAccounts(b.Namespace).Create(&v1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Namespace: b.Namespace, Name: name}})
+	if apierrors.IsAlreadyExists(err) {
+		// If we're racing to init and someone else already created it, re-fetch
+		return b.CoreClient.ServiceAccounts(b.Namespace).Get(name, metav1.GetOptions{})
+	}
+	return sa, err
+}
+
+func (b SAControllerClientBuilder) getAuthenticatedConfig(sa *v1.ServiceAccount, token string) (*restclient.Config, bool, error) {
+	username := apiserverserviceaccount.MakeUsername(sa.Namespace, sa.Name)
+
+	clientConfig := restclient.AnonymousClientConfig(b.ClientConfig)
+	clientConfig.BearerToken = token
+	restclient.AddUserAgent(clientConfig, username)
+
+	// Try token review first
+	tokenReview := &v1authenticationapi.TokenReview{Spec: v1authenticationapi.TokenReviewSpec{Token: token}}
+	if tokenResult, err := b.AuthenticationClient.TokenReviews().Create(tokenReview); err == nil {
+		if !tokenResult.Status.Authenticated {
+			glog.Warningf("Token for %s/%s did not authenticate correctly", sa.Name, sa.Namespace)
+			return nil, false, nil
+		}
+		if tokenResult.Status.User.Username != username {
+			glog.Warningf("Token for %s/%s authenticated as unexpected username: %s", sa.Name, sa.Namespace, tokenResult.Status.User.Username)
+			return nil, false, nil
+		}
+		glog.V(4).Infof("Verified credential for %s/%s", sa.Name, sa.Namespace)
+		return clientConfig, true, nil
+	}
+
+	// If we couldn't run the token review, the API might be disabled or we might not have permission.
+	// Try to make a request to /apis with the token. If we get a 401 we should consider the token invalid.
+	clientConfigCopy := *clientConfig
+	clientConfigCopy.NegotiatedSerializer = api.Codecs
+	client, err := restclient.UnversionedRESTClientFor(&clientConfigCopy)
+	if err != nil {
+		return nil, false, err
+	}
+	err = client.Get().AbsPath("/apis").Do().Error()
+	if apierrors.IsUnauthorized(err) {
+		glog.Warningf("Token for %s/%s did not authenticate correctly: %v", sa.Name, sa.Namespace, err)
+		return nil, false, nil
+	}
+
+	return clientConfig, true, nil
+}
+
 func (b SAControllerClientBuilder) ConfigOrDie(name string) *restclient.Config {
 	clientConfig, err := b.Config(name)
 	if err != nil {

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

@@ -294,6 +294,8 @@ func ClusterRoles() []rbac.ClusterRole {
 				rbac.NewRule("delete").Groups(legacyGroup).Resources("secrets").RuleOrDie(),
 				rbac.NewRule("get").Groups(legacyGroup).Resources("endpoints", "namespaces", "serviceaccounts").RuleOrDie(),
 				rbac.NewRule("update").Groups(legacyGroup).Resources("endpoints", "serviceaccounts").RuleOrDie(),
+				// Needed to check API access.  These creates are non-mutating
+				rbac.NewRule("create").Groups(authenticationGroup).Resources("tokenreviews").RuleOrDie(),
 
 				rbac.NewRule("list", "watch").Groups(legacyGroup).Resources(
 					"configmaps",

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-roles.yaml

@@ -461,6 +461,12 @@ items:
     - serviceaccounts
     verbs:
     - update
+  - apiGroups:
+    - authentication.k8s.io
+    resources:
+    - tokenreviews
+    verbs:
+    - create
   - apiGroups:
     - ""
     resources: