Merge pull request #10041 from liggitt/token_delete_live_check

Be more judicious about cleaning up service account tokens

pkg/serviceaccount/jwt.go

@@ -24,11 +24,12 @@ import (
 	"io/ioutil"
 	"strings"
 
-	jwt "github.com/dgrijalva/jwt-go"
-
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/auth/authenticator"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/auth/user"
+
+	jwt "github.com/dgrijalva/jwt-go"
+	"github.com/golang/glog"
 )
 
 const (
@@ -141,7 +142,7 @@ type jwtTokenAuthenticator struct {
 func (j *jwtTokenAuthenticator) AuthenticateToken(token string) (user.Info, bool, error) {
 	var validationError error
 
-	for _, key := range j.keys {
+	for i, key := range j.keys {
 		// Attempt to verify with each key until we find one that works
 		parsedToken, err := jwt.Parse(token, func(token *jwt.Token) (interface{}, error) {
 			if _, ok := token.Method.(*jwt.SigningMethodRSA); !ok {
@@ -161,6 +162,7 @@ func (j *jwtTokenAuthenticator) AuthenticateToken(token string) (user.Info, bool
 				if (err.Errors & jwt.ValidationErrorSignatureInvalid) != 0 {
 					// Signature error, perhaps one of the other keys will verify the signature
 					// If not, we want to return this error
+					glog.V(4).Infof("Signature error (key %d): %v", i, err)
 					validationError = err
 					continue
 				}
@@ -204,18 +206,22 @@ func (j *jwtTokenAuthenticator) AuthenticateToken(token string) (user.Info, bool
 			// Make sure token hasn't been invalidated by deletion of the secret
 			secret, err := j.getter.GetSecret(namespace, secretName)
 			if err != nil {
+				glog.V(4).Infof("Could not retrieve token %s/%s for service account %s/%s: %v", namespace, secretName, namespace, serviceAccountName, err)
 				return nil, false, errors.New("Token has been invalidated")
 			}
 			if bytes.Compare(secret.Data[api.ServiceAccountTokenKey], []byte(token)) != 0 {
+				glog.V(4).Infof("Token contents no longer matches %s/%s for service account %s/%s", namespace, secretName, namespace, serviceAccountName)
 				return nil, false, errors.New("Token does not match server's copy")
 			}
 
 			// Make sure service account still exists (name and UID)
 			serviceAccount, err := j.getter.GetServiceAccount(namespace, serviceAccountName)
 			if err != nil {
+				glog.V(4).Infof("Could not retrieve service account %s/%s: %v", namespace, serviceAccountName, err)
 				return nil, false, err
 			}
 			if string(serviceAccount.UID) != serviceAccountUID {
+				glog.V(4).Infof("Service account UID no longer matches %s/%s: %q != %q", namespace, serviceAccountName, string(serviceAccount.UID), serviceAccountUID)
 				return nil, false, fmt.Errorf("ServiceAccount UID (%s) does not match claim (%s)", serviceAccount.UID, serviceAccountUID)
 			}
 		}

pkg/serviceaccount/tokens_controller.go

@@ -171,6 +171,7 @@ func (e *TokensController) serviceAccountDeleted(obj interface{}) {
 		return
 	}
 	for _, secret := range secrets {
+		glog.V(4).Infof("Deleting secret %s/%s because service account %s was deleted", secret.Namespace, secret.Name, serviceAccount.Name)
 		if err := e.deleteSecret(secret); err != nil {
 			glog.Errorf("Error deleting secret %s/%s: %v", secret.Namespace, secret.Name, err)
 		}
@@ -180,16 +181,16 @@ func (e *TokensController) serviceAccountDeleted(obj interface{}) {
 // secretAdded reacts to a Secret create by ensuring the referenced ServiceAccount exists, and by adding a token to the secret if needed
 func (e *TokensController) secretAdded(obj interface{}) {
 	secret := obj.(*api.Secret)
-	serviceAccount, err := e.getServiceAccount(secret)
+	serviceAccount, err := e.getServiceAccount(secret, true)
 	if err != nil {
 		glog.Error(err)
 		return
 	}
 	if serviceAccount == nil {
-		// We shouldn't delete a secret based on an invalid serviceAccount reference until the serviceAccount store is synced
+		glog.V(2).Infof(
-		if !e.serviceAccountsSynced() {
+			"Deleting new secret %s/%s because service account %s (uid=%s) was not found",
-			return
+			secret.Namespace, secret.Name,
-		}
+			secret.Annotations[api.ServiceAccountNameKey], secret.Annotations[api.ServiceAccountUIDKey])
 		if err := e.deleteSecret(secret); err != nil {
 			glog.Errorf("Error deleting secret %s/%s: %v", secret.Namespace, secret.Name, err)
 		}
@@ -201,16 +202,16 @@ func (e *TokensController) secretAdded(obj interface{}) {
 // secretUpdated reacts to a Secret update (or re-list) by deleting the secret (if the referenced ServiceAccount does not exist)
 func (e *TokensController) secretUpdated(oldObj interface{}, newObj interface{}) {
 	newSecret := newObj.(*api.Secret)
-	newServiceAccount, err := e.getServiceAccount(newSecret)
+	newServiceAccount, err := e.getServiceAccount(newSecret, true)
 	if err != nil {
 		glog.Error(err)
 		return
 	}
 	if newServiceAccount == nil {
-		// We shouldn't delete a secret based on an invalid serviceAccount reference until the serviceAccount store is synced
+		glog.V(2).Infof(
-		if !e.serviceAccountsSynced() {
+			"Deleting updated secret %s/%s because service account %s (uid=%s) was not found",
-			return
+			newSecret.Namespace, newSecret.Name,
-		}
+			newSecret.Annotations[api.ServiceAccountNameKey], newSecret.Annotations[api.ServiceAccountUIDKey])
 		if err := e.deleteSecret(newSecret); err != nil {
 			glog.Errorf("Error deleting secret %s/%s: %v", newSecret.Namespace, newSecret.Name, err)
 		}
@@ -228,7 +229,7 @@ func (e *TokensController) secretDeleted(obj interface{}) {
 		return
 	}
 
-	serviceAccount, err := e.getServiceAccount(secret)
+	serviceAccount, err := e.getServiceAccount(secret, false)
 	if err != nil {
 		glog.Error(err)
 		return
@@ -399,7 +400,7 @@ func (e *TokensController) removeSecretReferenceIfNeeded(serviceAccount *api.Ser
 
 // getServiceAccount returns the ServiceAccount referenced by the given secret. If the secret is not
 // of type ServiceAccountToken, or if the referenced ServiceAccount does not exist, nil is returned
-func (e *TokensController) getServiceAccount(secret *api.Secret) (*api.ServiceAccount, error) {
+func (e *TokensController) getServiceAccount(secret *api.Secret, fetchOnCacheMiss bool) (*api.ServiceAccount, error) {
 	name, uid := serviceAccountNameAndUID(secret)
 	if len(name) == 0 {
 		return nil, nil
@@ -424,6 +425,21 @@ func (e *TokensController) getServiceAccount(secret *api.Secret) (*api.ServiceAc
 		return serviceAccount, nil
 	}
 
+	if fetchOnCacheMiss {
+		serviceAccount, err := e.client.ServiceAccounts(secret.Namespace).Get(name)
+		if apierrors.IsNotFound(err) {
+			return nil, nil
+		}
+		if err != nil {
+			return nil, err
+		}
+		if len(uid) > 0 && uid != string(serviceAccount.UID) {
+			// If UID is specified, it must match
+			return nil, nil
+		}
+		return serviceAccount, nil
+	}
+
 	return nil, nil
 }
 

pkg/serviceaccount/tokens_controller_test.go

@@ -302,17 +302,10 @@ func TestTokenCreation(t *testing.T) {
 
 			AddedSecret: serviceAccountTokenSecret(),
 			ExpectedActions: []testclient.FakeAction{
+				{Action: "get-serviceaccount", Value: "default"},
 				{Action: "delete-secret", Value: "token-secret-1"},
 			},
 		},
-		"added secret without serviceaccount with unsynced service account store": {
-			ClientObjects: []runtime.Object{serviceAccountTokenSecret()},
-
-			ServiceAccountsSyncPending: true,
-
-			AddedSecret:     serviceAccountTokenSecret(),
-			ExpectedActions: []testclient.FakeAction{},
-		},
 		"added secret with serviceaccount": {
 			ExistingServiceAccount: serviceAccount(tokenSecretReferences()),
 
@@ -334,17 +327,10 @@ func TestTokenCreation(t *testing.T) {
 
 			UpdatedSecret: serviceAccountTokenSecret(),
 			ExpectedActions: []testclient.FakeAction{
+				{Action: "get-serviceaccount", Value: "default"},
 				{Action: "delete-secret", Value: "token-secret-1"},
 			},
 		},
-		"updated secret without serviceaccount with unsynced service account store": {
-			ClientObjects: []runtime.Object{serviceAccountTokenSecret()},
-
-			ServiceAccountsSyncPending: true,
-
-			UpdatedSecret:   serviceAccountTokenSecret(),
-			ExpectedActions: []testclient.FakeAction{},
-		},
 		"updated secret with serviceaccount": {
 			ExistingServiceAccount: serviceAccount(tokenSecretReferences()),