Merge pull request #105917 from enj/enj/i/sa_err_print

serviceaccount/claims: include validation failure error in the log

pkg/serviceaccount/claims.go

@@ -111,20 +111,31 @@ var _ = Validator(&validator{})
 func (v *validator) Validate(ctx context.Context, _ string, public *jwt.Claims, privateObj interface{}) (*apiserverserviceaccount.ServiceAccountInfo, error) {
 	private, ok := privateObj.(*privateClaims)
 	if !ok {
-		klog.Errorf("jwt validator expected private claim of type *privateClaims but got: %T", privateObj)
+		klog.Errorf("service account jwt validator expected private claim of type *privateClaims but got: %T", privateObj)
-		return nil, errors.New("Token could not be validated.")
+		return nil, errors.New("service account token claims could not be validated due to unexpected private claim")
 	}
 	nowTime := now()
 	err := public.Validate(jwt.Expected{
 		Time: nowTime,
 	})
-	switch {
+	switch err {
-	case err == nil:
+	case nil:
-	case err == jwt.ErrExpired:
+		// successful validation
-		return nil, errors.New("Token has expired.")
+
+	case jwt.ErrExpired:
+		return nil, errors.New("service account token has expired")
+
+	case jwt.ErrNotValidYet:
+		return nil, errors.New("service account token is not valid yet")
+
+	// our current use of jwt.Expected above should make these cases impossible to hit
+	case jwt.ErrInvalidAudience, jwt.ErrInvalidID, jwt.ErrInvalidIssuer, jwt.ErrInvalidSubject:
+		klog.Errorf("service account token claim validation got unexpected validation failure: %v", err)
+		return nil, fmt.Errorf("service account token claims could not be validated: %w", err) // safe to pass these errors back to the user
+
 	default:
-		klog.Errorf("unexpected validation error: %T", err)
+		klog.Errorf("service account token claim validation got unexpected error type: %T", err)                         // avoid leaking unexpected information into the logs
-		return nil, errors.New("Token could not be validated.")
+		return nil, errors.New("service account token claims could not be validated due to unexpected validation error") // return an opaque error
 	}
 
 	// consider things deleted prior to now()-leeway to be invalid
@@ -141,11 +152,11 @@ func (v *validator) Validate(ctx context.Context, _ string, public *jwt.Claims,
 	}
 	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)
+		return nil, fmt.Errorf("service account %s/%s has been deleted", namespace, saref.Name)
 	}
 	if string(serviceAccount.UID) != saref.UID {
 		klog.V(4).Infof("Service account UID no longer matches %s/%s: %q != %q", namespace, saref.Name, string(serviceAccount.UID), saref.UID)
-		return nil, fmt.Errorf("ServiceAccount UID (%s) does not match claim (%s)", serviceAccount.UID, saref.UID)
+		return nil, fmt.Errorf("service account UID (%s) does not match claim (%s)", serviceAccount.UID, saref.UID)
 	}
 
 	if secref != nil {
@@ -153,15 +164,15 @@ func (v *validator) Validate(ctx context.Context, _ string, public *jwt.Claims,
 		secret, err := v.getter.GetSecret(namespace, secref.Name)
 		if err != nil {
 			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")
+			return nil, errors.New("service account token has been invalidated")
 		}
 		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")
+			return nil, errors.New("service account token has been invalidated")
 		}
 		if secref.UID != string(secret.UID) {
 			klog.V(4).Infof("Secret UID no longer matches %s/%s: %q != %q", namespace, secref.Name, string(secret.UID), secref.UID)
-			return nil, fmt.Errorf("Secret UID (%s) does not match claim (%s)", secret.UID, secref.UID)
+			return nil, fmt.Errorf("secret UID (%s) does not match service account secret ref claim (%s)", secret.UID, secref.UID)
 		}
 	}
 
@@ -171,15 +182,15 @@ func (v *validator) Validate(ctx context.Context, _ string, public *jwt.Claims,
 		pod, err := v.getter.GetPod(namespace, podref.Name)
 		if err != nil {
 			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")
+			return nil, errors.New("service account token has been invalidated")
 		}
 		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")
+			return nil, errors.New("service account token has been invalidated")
 		}
 		if podref.UID != string(pod.UID) {
 			klog.V(4).Infof("Pod UID no longer matches %s/%s: %q != %q", namespace, podref.Name, string(pod.UID), podref.UID)
-			return nil, fmt.Errorf("Pod UID (%s) does not match claim (%s)", pod.UID, podref.UID)
+			return nil, fmt.Errorf("pod UID (%s) does not match service account pod ref claim (%s)", pod.UID, podref.UID)
 		}
 		podName = podref.Name
 		podUID = podref.UID

pkg/serviceaccount/claims_test.go

@@ -223,7 +223,9 @@ type claimTestCase struct {
 	name      string
 	getter    ServiceAccountTokenGetter
 	private   *privateClaims
-	expectErr bool
+	expiry    jwt.NumericDate
+	notBefore jwt.NumericDate
+	expectErr string
 }
 
 func TestValidatePrivateClaims(t *testing.T) {
@@ -264,41 +266,61 @@ func TestValidatePrivateClaims(t *testing.T) {
 	}
 
 	testcases := []claimTestCase{
+		{
+			name:      "good",
+			getter:    fakeGetter{serviceAccount, nil, nil},
+			private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Namespace: "ns"}},
+			expectErr: "",
+		},
+		{
+			name:      "expired",
+			getter:    fakeGetter{serviceAccount, nil, nil},
+			private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Namespace: "ns"}},
+			expiry:    jwt.NewNumericDate(now().Add(-1_000 * time.Hour)),
+			expectErr: "service account token has expired",
+		},
+		{
+			name:      "not yet valid",
+			getter:    fakeGetter{serviceAccount, nil, nil},
+			private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Namespace: "ns"}},
+			notBefore: jwt.NewNumericDate(now().Add(1_000 * time.Hour)),
+			expectErr: "service account token is not valid yet",
+		},
 		{
 			name:      "missing serviceaccount",
 			getter:    fakeGetter{nil, nil, nil},
 			private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauid"}, Namespace: "ns"}},
-			expectErr: true,
+			expectErr: `serviceaccounts "saname" not found`,
 		},
 		{
 			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,
+			expectErr: "service account token has been invalidated",
 		},
 		{
 			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,
+			expectErr: "service account token has been invalidated",
 		},
 		{
 			name:      "different uid serviceaccount",
 			getter:    fakeGetter{serviceAccount, nil, nil},
 			private:   &privateClaims{Kubernetes: kubernetes{Svcacct: ref{Name: "saname", UID: "sauidold"}, Namespace: "ns"}},
-			expectErr: true,
+			expectErr: "service account UID (sauid) does not match claim (sauidold)",
 		},
 		{
 			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,
+			expectErr: "secret UID (secretuid) does not match service account secret ref claim (secretuidold)",
 		},
 		{
 			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,
+			expectErr: "pod UID (poduid) does not match service account pod ref claim (poduidold)",
 		},
 	}
 
@@ -312,24 +334,30 @@ func TestValidatePrivateClaims(t *testing.T) {
 		deletedPod.DeletionTimestamp = deletionTestCase.time
 		deletedSecret.DeletionTimestamp = deletionTestCase.time
 
+		var saDeletedErr, deletedErr string
+		if deletionTestCase.expectErr {
+			saDeletedErr = "service account ns/saname has been deleted"
+			deletedErr = "service account token has been invalidated"
+		}
+
 		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,
+				expectErr: saDeletedErr,
 			},
 			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,
+				expectErr: deletedErr,
 			},
 			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,
+				expectErr: deletedErr,
 			},
 		)
 	}
@@ -337,20 +365,30 @@ func TestValidatePrivateClaims(t *testing.T) {
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
 			v := &validator{tc.getter}
-			_, err := v.Validate(context.Background(), "", &jwt.Claims{Expiry: jwt.NumericDate(nowUnix)}, tc.private)
+			expiry := jwt.NumericDate(nowUnix)
-			if err != nil && !tc.expectErr {
+			if tc.expiry != 0 {
-				t.Fatal(err)
+				expiry = tc.expiry
 			}
-			if err == nil && tc.expectErr {
+			_, err := v.Validate(context.Background(), "", &jwt.Claims{Expiry: expiry, NotBefore: tc.notBefore}, tc.private)
-				t.Fatal("expected error, got none")
+			if len(tc.expectErr) > 0 {
-			}
+				if errStr := errString(err); tc.expectErr != errStr {
-			if err != nil {
+					t.Fatalf("expected error %q but got %q", tc.expectErr, errStr)
-				return
+				}
+			} else if err != nil {
+				t.Fatalf("unexpected error: %v", err)
 			}
 		})
 	}
 }
 
+func errString(err error) string {
+	if err == nil {
+		return ""
+	}
+
+	return err.Error()
+}
+
 type fakeGetter struct {
 	serviceAccount *v1.ServiceAccount
 	secret         *v1.Secret