From 92c859600260a3fdfa0cf79b7137f55a4a6bd345 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 26 Oct 2021 13:48:18 -0400 Subject: [PATCH] serviceaccount/claims: include validation failure error in the log Without this fix, the errors are logged as: unexpected validation error: *errors.errorString Signed-off-by: Monis Khan --- pkg/serviceaccount/claims.go | 43 +++++++++++------- pkg/serviceaccount/claims_test.go | 74 +++++++++++++++++++++++-------- 2 files changed, 83 insertions(+), 34 deletions(-) diff --git a/pkg/serviceaccount/claims.go b/pkg/serviceaccount/claims.go index 80ebd7a5192..1e1475c779f 100644 --- a/pkg/serviceaccount/claims.go +++ b/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) - return nil, errors.New("Token could not be validated.") + klog.Errorf("service account jwt validator expected private claim of type *privateClaims but got: %T", privateObj) + 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 { - case err == nil: - case err == jwt.ErrExpired: - return nil, errors.New("Token has expired.") + switch err { + case nil: + // successful validation + + 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) - return nil, errors.New("Token could not be validated.") + klog.Errorf("service account token claim validation got unexpected error type: %T", err) // avoid leaking unexpected information into the logs + 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 diff --git a/pkg/serviceaccount/claims_test.go b/pkg/serviceaccount/claims_test.go index 76e4dc2bb08..2e968f60335 100644 --- a/pkg/serviceaccount/claims_test.go +++ b/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) - if err != nil && !tc.expectErr { - t.Fatal(err) + expiry := jwt.NumericDate(nowUnix) + if tc.expiry != 0 { + expiry = tc.expiry } - if err == nil && tc.expectErr { - t.Fatal("expected error, got none") - } - if err != nil { - return + _, err := v.Validate(context.Background(), "", &jwt.Claims{Expiry: expiry, NotBefore: tc.notBefore}, tc.private) + if len(tc.expectErr) > 0 { + if errStr := errString(err); tc.expectErr != errStr { + t.Fatalf("expected error %q but got %q", tc.expectErr, errStr) + } + } 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