Merge pull request #89583 from liggitt/bound-token-grace-period

Consider future deletionTimestamps when validating bound tokens

pkg/serviceaccount/BUILD

@@ -57,7 +57,9 @@ go_test(
         "//pkg/controller/serviceaccount:go_default_library",
         "//pkg/routes:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/authenticator:go_default_library",
         "//staging/src/k8s.io/client-go/kubernetes:go_default_library",
         "//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library",

pkg/serviceaccount/claims.go

@@ -98,8 +98,9 @@ func (v *validator) Validate(_ string, public *jwt.Claims, privateObj interface{
 		klog.Errorf("jwt validator expected private claim of type *privateClaims but got: %T", privateObj)
 		return nil, errors.New("Token could not be validated.")
 	}
+	nowTime := now()
 	err := public.Validate(jwt.Expected{
-		Time: now(),
+		Time: nowTime,
 	})
 	switch {
 	case err == nil:
@@ -110,6 +111,8 @@ func (v *validator) Validate(_ string, public *jwt.Claims, privateObj interface{
 		return nil, errors.New("Token could not be validated.")
 	}
 
+	// consider things deleted prior to now()-leeway to be invalid
+	invalidIfDeletedBefore := nowTime.Add(-jwt.DefaultLeeway)
 	namespace := private.Kubernetes.Namespace
 	saref := private.Kubernetes.Svcacct
 	podref := private.Kubernetes.Pod
@@ -120,7 +123,7 @@ func (v *validator) Validate(_ string, public *jwt.Claims, privateObj interface{
 		klog.V(4).Infof("Could not retrieve service account %s/%s: %v", namespace, saref.Name, err)
 		return nil, err
 	}
-	if serviceAccount.DeletionTimestamp != nil {
+	if serviceAccount.DeletionTimestamp != nil && serviceAccount.DeletionTimestamp.Time.Before(invalidIfDeletedBefore) {
 		klog.V(4).Infof("Service account has been deleted %s/%s", namespace, saref.Name)
 		return nil, fmt.Errorf("ServiceAccount %s/%s has been deleted", namespace, saref.Name)
 	}
@@ -136,7 +139,7 @@ func (v *validator) Validate(_ string, public *jwt.Claims, privateObj interface{
 			klog.V(4).Infof("Could not retrieve bound secret %s/%s for service account %s/%s: %v", namespace, secref.Name, namespace, saref.Name, err)
 			return nil, errors.New("Token has been invalidated")
 		}
-		if secret.DeletionTimestamp != nil {
+		if secret.DeletionTimestamp != nil && secret.DeletionTimestamp.Time.Before(invalidIfDeletedBefore) {
 			klog.V(4).Infof("Bound secret is deleted and awaiting removal: %s/%s for service account %s/%s", namespace, secref.Name, namespace, saref.Name)
 			return nil, errors.New("Token has been invalidated")
 		}
@@ -154,7 +157,7 @@ func (v *validator) Validate(_ string, public *jwt.Claims, privateObj interface{
 			klog.V(4).Infof("Could not retrieve bound pod %s/%s for service account %s/%s: %v", namespace, podref.Name, namespace, saref.Name, err)
 			return nil, errors.New("Token has been invalidated")
 		}
-		if pod.DeletionTimestamp != nil {
+		if pod.DeletionTimestamp != nil && pod.DeletionTimestamp.Time.Before(invalidIfDeletedBefore) {
 			klog.V(4).Infof("Bound pod is deleted and awaiting removal: %s/%s for service account %s/%s", namespace, podref.Name, namespace, saref.Name)
 			return nil, errors.New("Token has been invalidated")
 		}

pkg/serviceaccount/claims_test.go

@@ -24,7 +24,10 @@ import (
 
 	"gopkg.in/square/go-jose.v2/jwt"
 
+	v1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/kubernetes/pkg/apis/core"
 )
 
@@ -182,3 +185,166 @@ func TestClaims(t *testing.T) {
 		})
 	}
 }
+
+type deletionTestCase struct {
+	name      string
+	time      *metav1.Time
+	expectErr bool
+}
+
+type claimTestCase struct {
+	name      string
+	getter    ServiceAccountTokenGetter
+	private   *privateClaims
+	expectErr bool
+}
+
+func TestValidatePrivateClaims(t *testing.T) {
+	var (
+		nowUnix = int64(1514764800)
+
+		serviceAccount = &v1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "saname", Namespace: "ns", UID: "sauid"}}
+		secret         = &v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "secretname", Namespace: "ns", UID: "secretuid"}}
+		pod            = &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "podname", Namespace: "ns", UID: "poduid"}}
+	)
+
+	deletionTestCases := []deletionTestCase{
+		{
+			name: "valid",
+			time: nil,
+		},
+		{
+			name: "deleted now",
+			time: &metav1.Time{Time: time.Unix(nowUnix, 0)},
+		},
+		{
+			name: "deleted near past",
+			time: &metav1.Time{Time: time.Unix(nowUnix-1, 0)},
+		},
+		{
+			name: "deleted near future",
+			time: &metav1.Time{Time: time.Unix(nowUnix+1, 0)},
+		},
+		{
+			name: "deleted now-leeway",
+			time: &metav1.Time{Time: time.Unix(nowUnix-60, 0)},
+		},
+		{
+			name:      "deleted now-leeway-1",
+			time:      &metav1.Time{Time: time.Unix(nowUnix-61, 0)},
+			expectErr: true,
+		},
+	}
+
+	testcases := []claimTestCase{
+		{
+			name:      "missing serviceaccount",
+			getter:    fakeGetter{nil, nil, nil},
+			private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Namespace: "ns"}},
+			expectErr: true,
+		},
+		{
+			name:      "missing secret",
+			getter:    fakeGetter{serviceAccount, nil, nil},
+			private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Secret: &ref{Name: "secretname", UID: "secretuid"}, Namespace: "ns"}},
+			expectErr: true,
+		},
+		{
+			name:      "missing pod",
+			getter:    fakeGetter{serviceAccount, nil, nil},
+			private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Pod: &ref{Name: "podname", UID: "poduid"}, Namespace: "ns"}},
+			expectErr: true,
+		},
+		{
+			name:      "different uid serviceaccount",
+			getter:    fakeGetter{serviceAccount, nil, nil},
+			private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauidold"}, Namespace: "ns"}},
+			expectErr: true,
+		},
+		{
+			name:      "different uid secret",
+			getter:    fakeGetter{serviceAccount, secret, nil},
+			private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Secret: &ref{Name: "secretname", UID: "secretuidold"}, Namespace: "ns"}},
+			expectErr: true,
+		},
+		{
+			name:      "different uid pod",
+			getter:    fakeGetter{serviceAccount, nil, pod},
+			private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Pod: &ref{Name: "podname", UID: "poduidold"}, Namespace: "ns"}},
+			expectErr: true,
+		},
+	}
+
+	for _, deletionTestCase := range deletionTestCases {
+		var (
+			deletedServiceAccount = serviceAccount.DeepCopy()
+			deletedPod            = pod.DeepCopy()
+			deletedSecret         = secret.DeepCopy()
+		)
+		deletedServiceAccount.DeletionTimestamp = deletionTestCase.time
+		deletedPod.DeletionTimestamp = deletionTestCase.time
+		deletedSecret.DeletionTimestamp = deletionTestCase.time
+
+		testcases = append(testcases,
+			claimTestCase{
+				name:      deletionTestCase.name + " serviceaccount",
+				getter:    fakeGetter{deletedServiceAccount, nil, nil},
+				private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Namespace: "ns"}},
+				expectErr: deletionTestCase.expectErr,
+			},
+			claimTestCase{
+				name:      deletionTestCase.name + " secret",
+				getter:    fakeGetter{serviceAccount, deletedSecret, nil},
+				private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Secret: &ref{Name: "secretname", UID: "secretuid"}, Namespace: "ns"}},
+				expectErr: deletionTestCase.expectErr,
+			},
+			claimTestCase{
+				name:      deletionTestCase.name + " pod",
+				getter:    fakeGetter{serviceAccount, nil, deletedPod},
+				private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Pod: &ref{Name: "podname", UID: "poduid"}, Namespace: "ns"}},
+				expectErr: deletionTestCase.expectErr,
+			},
+		)
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			v := &validator{tc.getter}
+			_, err := v.Validate("", &jwt.Claims{Expiry: jwt.NumericDate(nowUnix)}, tc.private)
+			if err != nil && !tc.expectErr {
+				t.Fatal(err)
+			}
+			if err == nil && tc.expectErr {
+				t.Fatal("expected error, got none")
+			}
+			if err != nil {
+				return
+			}
+		})
+	}
+}
+
+type fakeGetter struct {
+	serviceAccount *v1.ServiceAccount
+	secret         *v1.Secret
+	pod            *v1.Pod
+}
+
+func (f fakeGetter) GetServiceAccount(namespace, name string) (*v1.ServiceAccount, error) {
+	if f.serviceAccount == nil {
+		return nil, apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "serviceaccounts"}, name)
+	}
+	return f.serviceAccount, nil
+}
+func (f fakeGetter) GetPod(namespace, name string) (*v1.Pod, error) {
+	if f.pod == nil {
+		return nil, apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "pods"}, name)
+	}
+	return f.pod, nil
+}
+func (f fakeGetter) GetSecret(namespace, name string) (*v1.Secret, error) {
+	if f.secret == nil {
+		return nil, apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "secrets"}, name)
+	}
+	return f.secret, nil
+}