From e563727338642405f0b128c948d634a1df6e94f3 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 18 Jun 2015 10:41:35 -0400 Subject: [PATCH 1/2] Add logging for invalid JWT tokens --- pkg/serviceaccount/jwt.go | 12 +++++++++--- pkg/serviceaccount/tokens_controller.go | 9 +++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/pkg/serviceaccount/jwt.go b/pkg/serviceaccount/jwt.go index 77a058b4ffa..735ec7d24da 100644 --- a/pkg/serviceaccount/jwt.go +++ b/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) } } diff --git a/pkg/serviceaccount/tokens_controller.go b/pkg/serviceaccount/tokens_controller.go index a800ec9840a..ef3cca65e33 100644 --- a/pkg/serviceaccount/tokens_controller.go +++ b/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) } @@ -190,6 +191,10 @@ func (e *TokensController) secretAdded(obj interface{}) { if !e.serviceAccountsSynced() { return } + glog.V(2).Infof( + "Deleting new secret %s/%s because service account %s (uid=%s) was not found", + 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) } @@ -211,6 +216,10 @@ func (e *TokensController) secretUpdated(oldObj interface{}, newObj interface{}) if !e.serviceAccountsSynced() { return } + glog.V(2).Infof( + "Deleting updated secret %s/%s because service account %s (uid=%s) was not found", + 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) } From 1ee557814676fcf69c5c7c9e8d77ccb684939009 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 18 Jun 2015 10:42:41 -0400 Subject: [PATCH 2/2] Ensure service account does not exist before deleting added/updated tokens --- pkg/serviceaccount/tokens_controller.go | 31 ++++++++++++-------- pkg/serviceaccount/tokens_controller_test.go | 18 ++---------- 2 files changed, 21 insertions(+), 28 deletions(-) diff --git a/pkg/serviceaccount/tokens_controller.go b/pkg/serviceaccount/tokens_controller.go index ef3cca65e33..562fdccee8e 100644 --- a/pkg/serviceaccount/tokens_controller.go +++ b/pkg/serviceaccount/tokens_controller.go @@ -181,16 +181,12 @@ 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 - if !e.serviceAccountsSynced() { - return - } glog.V(2).Infof( "Deleting new secret %s/%s because service account %s (uid=%s) was not found", secret.Namespace, secret.Name, @@ -206,16 +202,12 @@ 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 - if !e.serviceAccountsSynced() { - return - } glog.V(2).Infof( "Deleting updated secret %s/%s because service account %s (uid=%s) was not found", newSecret.Namespace, newSecret.Name, @@ -237,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 @@ -408,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 @@ -433,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 } diff --git a/pkg/serviceaccount/tokens_controller_test.go b/pkg/serviceaccount/tokens_controller_test.go index ebd96845aba..16ac25a4fa0 100644 --- a/pkg/serviceaccount/tokens_controller_test.go +++ b/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()),