Merge pull request #44102 from ncdc/fix-serviceaccount-token-admission

Automatic merge from submit-queue

serviceaccount admission: return correct tokens

Fix a bug in serviceaccount admission introduced when we switched
everything to use shared informers. That change accidentally reused the
list of secrets instead of creating a new one, resulting in all secrets
in the namespace being returned as possible service account tokens,
instead of limiting it only to the actual service account tokens, as it
did before the shared informer conversion. This also adds a unit test to
ensure there is no future regression here.

This will need to be cherry-picked to 1.6.



**What this PR does / why we need it**:

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
Fixed an issue mounting the wrong secret into pods as a service account token
```

cc @smarterclayton @liggitt @sttts @derekwaynecarr @calebamiles @ethernetdan @eparis

plugin/pkg/admission/serviceaccount/BUILD

@@ -43,12 +43,14 @@ go_test(
         "//pkg/api:go_default_library",
         "//pkg/client/clientset_generated/internalclientset/fake:go_default_library",
         "//pkg/client/informers/informers_generated/internalversion:go_default_library",
+        "//pkg/client/listers/core/internalversion:go_default_library",
         "//pkg/controller:go_default_library",
         "//pkg/kubelet/types:go_default_library",
         "//vendor:k8s.io/apimachinery/pkg/api/errors",
         "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",
         "//vendor:k8s.io/apimachinery/pkg/types",
         "//vendor:k8s.io/apiserver/pkg/admission",
+        "//vendor:k8s.io/client-go/tools/cache",
     ],
 )
 

plugin/pkg/admission/serviceaccount/admission.go

@@ -287,18 +287,20 @@ func (s *serviceAccount) getReferencedServiceAccountToken(serviceAccount *api.Se
 
 // getServiceAccountTokens returns all ServiceAccountToken secrets for the given ServiceAccount
 func (s *serviceAccount) getServiceAccountTokens(serviceAccount *api.ServiceAccount) ([]*api.Secret, error) {
-	tokens, err := s.secretLister.Secrets(serviceAccount.Namespace).List(labels.Everything())
+	secrets, err := s.secretLister.Secrets(serviceAccount.Namespace).List(labels.Everything())
 	if err != nil {
 		return nil, err
 	}
 
-	for _, token := range tokens {
-		if token.Type != api.SecretTypeServiceAccountToken {
+	tokens := []*api.Secret{}
+
+	for _, secret := range secrets {
+		if secret.Type != api.SecretTypeServiceAccountToken {
 			continue
 		}
 
-		if serviceaccount.InternalIsServiceAccountToken(token, serviceAccount) {
-			tokens = append(tokens, token)
+		if serviceaccount.InternalIsServiceAccountToken(secret, serviceAccount) {
+			tokens = append(tokens, secret)
 		}
 	}
 	return tokens, nil

plugin/pkg/admission/serviceaccount/admission_test.go

@@ -25,9 +25,11 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apiserver/pkg/admission"
+	"k8s.io/client-go/tools/cache"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
 	informers "k8s.io/kubernetes/pkg/client/informers/informers_generated/internalversion"
+	corelisters "k8s.io/kubernetes/pkg/client/listers/core/internalversion"
 	"k8s.io/kubernetes/pkg/controller"
 	kubelet "k8s.io/kubernetes/pkg/kubelet/types"
 )
@@ -849,3 +851,59 @@ func TestMultipleReferencedSecrets(t *testing.T) {
 		t.Errorf("expected first referenced secret to be mounted, got %q", name)
 	}
 }
+
+func newSecret(secretType api.SecretType, namespace, name, serviceAccountName, serviceAccountUID string) *api.Secret {
+	return &api.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: namespace,
+			Name:      name,
+			Annotations: map[string]string{
+				api.ServiceAccountNameKey: serviceAccountName,
+				api.ServiceAccountUIDKey:  serviceAccountUID,
+			},
+		},
+		Type: secretType,
+	}
+}
+
+func TestGetServiceAccountTokens(t *testing.T) {
+	admit := NewServiceAccount()
+	indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
+	admit.secretLister = corelisters.NewSecretLister(indexer)
+
+	ns := "namespace"
+	serviceAccountUID := "12345"
+
+	sa := &api.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      DefaultServiceAccountName,
+			Namespace: ns,
+			UID:       types.UID(serviceAccountUID),
+		},
+	}
+
+	nonSATokenSecret := newSecret(api.SecretTypeDockercfg, ns, "nonSATokenSecret", DefaultServiceAccountName, serviceAccountUID)
+	indexer.Add(nonSATokenSecret)
+
+	differentSAToken := newSecret(api.SecretTypeServiceAccountToken, ns, "differentSAToken", "someOtherSA", "someOtherUID")
+	indexer.Add(differentSAToken)
+
+	matchingSAToken := newSecret(api.SecretTypeServiceAccountToken, ns, "matchingSAToken", DefaultServiceAccountName, serviceAccountUID)
+	indexer.Add(matchingSAToken)
+
+	tokens, err := admit.getServiceAccountTokens(sa)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if len(tokens) != 1 {
+		names := make([]string, 0, len(tokens))
+		for _, token := range tokens {
+			names = append(names, token.Name)
+		}
+		t.Fatalf("expected only 1 token, got %v", names)
+	}
+	if e, a := matchingSAToken.Name, tokens[0].Name; e != a {
+		t.Errorf("expected token %s, got %s", e, a)
+	}
+}