Browse Source

fix(vault): Fix crash when caching is enabled and a token expires (#3598)

In the vault client library, LookupSelfWithContext calls ParseSecret,
which has a few places where it returns `nil, nil` instead of returning
a proper error. The most common scenario is when the token expires and
the Vault server returns:

    {
      "errors": [
        "permission denied"
      ]
    }

This commit adds an additional check to ensure that a nil response won't
be dereferenced in checkToken().

Signed-off-by: Andrew Gunnerson <andrew.gunnerson@elastic.co>
Andrew Gunnerson 1 year ago
parent
commit
c7fc730019
2 changed files with 48 additions and 0 deletions
  1. 5 0
      pkg/provider/vault/auth.go
  2. 43 0
      pkg/provider/vault/auth_test.go

+ 5 - 0
pkg/provider/vault/auth.go

@@ -147,6 +147,11 @@ func checkToken(ctx context.Context, token util.Token) (bool, error) {
 	if err != nil {
 		return false, err
 	}
+	// LookupSelfWithContext() calls ParseSecret(), which has several places
+	// that return no data and no error, including when a token is expired.
+	if resp == nil {
+		return false, fmt.Errorf("no response nor error for token lookup")
+	}
 	t, ok := resp.Data["type"]
 	if !ok {
 		return false, fmt.Errorf("could not assert token type")

+ 43 - 0
pkg/provider/vault/auth_test.go

@@ -16,10 +16,12 @@ package vault
 
 import (
 	"context"
+	"errors"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	vault "github.com/hashicorp/vault/api"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
@@ -165,3 +167,44 @@ func TestSetAuthNamespace(t *testing.T) {
 		})
 	}
 }
+
+func TestCheckTokenErrors(t *testing.T) {
+	cases := map[string]struct {
+		message string
+		secret  *vault.Secret
+		err     error
+	}{
+		"SuccessWithNoData": {
+			message: "should not cache if token lookup returned no data",
+			secret:  &vault.Secret{},
+			err:     nil,
+		},
+		"Error": {
+			message: "should not cache if token lookup errored",
+			secret:  nil,
+			err:     errors.New(""),
+		},
+		// This happens when a token is expired and the Vault server returns:
+		// {"errors":["permission denied"]}
+		"NoDataNorError": {
+			message: "should not cache if token lookup returned no data nor error",
+			secret:  nil,
+			err:     nil,
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			token := fake.Token{
+				LookupSelfWithContextFn: func(ctx context.Context) (*vault.Secret, error) {
+					return tc.secret, tc.err
+				},
+			}
+
+			cached, _ := checkToken(context.Background(), token)
+			if cached {
+				t.Errorf("%v", tc.message)
+			}
+		})
+	}
+}