Added Invalid Token Error Message that will be returned for bad tokens (#25953)

Edited changelog

Added dummy policy to CE file to make tests pass

Added changelog

Author: divyaac

changelog/25953.txt

@@ -0,0 +1,4 @@
+```release-note:change
+core: return an additional "invalid token" error message in 403 response when the provided request token is expired,
+exceeded the number of uses, or is a bogus value
+```

http/auth_token_test.go

@@ -6,9 +6,40 @@ package http
 import (
 	"strings"
 	"testing"
+	"time"
 
+	"github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/vault/api"
+	"github.com/hashicorp/vault/sdk/helper/logging"
+	"github.com/hashicorp/vault/sdk/logical"
 	"github.com/hashicorp/vault/vault"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	rootLeasePolicies = `
+path "sys/internal/ui/*" {
+capabilities = ["create", "read", "update", "delete", "list"]
+}
+
+path "auth/token/*" {
+capabilities = ["create", "update", "read", "list"]
+}
+
+path "kv/foo*" {
+  capabilities = ["create", "read", "update", "delete", "list"]
+}
+`
+
+	dummy = `
+path "/ns1/sys/leases/*" {
+	capabilities = ["sudo", "create", "read", "update", "delete", "list"]
+}
+
+path "/ns1/auth/token/*" {
+	capabilities = ["sudo", "create", "read", "update", "delete", "list"]
+}
+`
 )
 
 func TestAuthTokenCreate(t *testing.T) {
@@ -207,3 +238,106 @@ func TestAuthTokenRenew(t *testing.T) {
 		t.Error("expected lease to be renewable")
 	}
 }
+
+// TestToken_InvalidTokenError checks that an InvalidToken error is only returned
+// for tokens that have (1) exceeded the token TTL and (2) exceeded the number of uses
+func TestToken_InvalidTokenError(t *testing.T) {
+	coreConfig := &vault.CoreConfig{
+		DisableMlock: true,
+		DisableCache: true,
+		Logger:       logging.NewVaultLogger(hclog.Trace),
+	}
+
+	// Init new test cluster
+	cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
+		HandlerFunc: Handler,
+	})
+
+	cluster.Start()
+	defer cluster.Cleanup()
+
+	cores := cluster.Cores
+	vault.TestWaitActive(t, cores[0].Core)
+
+	client := cores[0].Client
+
+	// Add policy
+	if err := client.Sys().PutPolicy("root-lease-policy", rootLeasePolicies); err != nil {
+		t.Fatal(err)
+	}
+	// Add a dummy policy
+	if err := client.Sys().PutPolicy("dummy", dummy); err != nil {
+		t.Fatal(err)
+	}
+
+	rootToken := client.Token()
+
+	// Enable kv secrets and mount initial secrets
+	err := client.Sys().Mount("kv", &api.MountInput{Type: "kv"})
+	require.NoError(t, err)
+
+	writeSecretsToMount(t, client, "kv/foo", map[string]interface{}{
+		"user":     "admin",
+		"password": "password",
+	})
+
+	// Create a token that has a TTL of 5s
+	tokenCreateRequest := &api.TokenCreateRequest{
+		Policies: []string{"root-lease-policy"},
+		TTL:      "5s",
+	}
+	secret, err := client.Auth().Token().CreateOrphan(tokenCreateRequest)
+	token := secret.Auth.ClientToken
+	client.SetToken(token)
+
+	// Verify that token works to read from kv mount
+	_, err = client.Logical().Read("kv/foo")
+	require.NoError(t, err)
+
+	time.Sleep(time.Second * 5)
+
+	// Verify that token is expired and shows an "invalid token" error
+	_, err = client.Logical().Read("kv/foo")
+	require.ErrorContains(t, err, logical.ErrInvalidToken.Error())
+	require.ErrorContains(t, err, logical.ErrPermissionDenied.Error())
+
+	// Create a second approle token with a token use limit
+	client.SetToken(rootToken)
+	tokenCreateRequest = &api.TokenCreateRequest{
+		Policies: []string{"root-lease-policy"},
+		NumUses:  5,
+	}
+
+	secret, err = client.Auth().Token().CreateOrphan(tokenCreateRequest)
+	token = secret.Auth.ClientToken
+	client.SetToken(token)
+
+	for i := 0; i < 5; i++ {
+		_, err = client.Logical().Read("kv/foo")
+		require.NoError(t, err)
+	}
+	// Verify that the number of uses is exceeded so the "invalid token" error is displayed
+	_, err = client.Logical().Read("kv/foo")
+	require.ErrorContains(t, err, logical.ErrInvalidToken.Error())
+	require.ErrorContains(t, err, logical.ErrPermissionDenied.Error())
+
+	// Create a third approle token that will have incorrect policy access to the subsequent request
+	client.SetToken(rootToken)
+	tokenCreateRequest = &api.TokenCreateRequest{
+		Policies: []string{"dummy"},
+	}
+
+	secret, err = client.Auth().Token().CreateOrphan(tokenCreateRequest)
+	token = secret.Auth.ClientToken
+	client.SetToken(token)
+
+	// Incorrect policy access should only return an ErrPermissionDenied error
+	_, err = client.Logical().Read("kv/foo")
+	require.ErrorContains(t, err, logical.ErrPermissionDenied.Error())
+	require.NotContains(t, err.Error(), logical.ErrInvalidToken)
+}
+
+func writeSecretsToMount(t *testing.T, client *api.Client, mountPath string, data map[string]interface{}) {
+	_, err := client.Logical().Write(mountPath, data)
+	require.NoError(t, err)
+}

sdk/logical/error.go

@@ -23,6 +23,9 @@ var (
 	// ErrPermissionDenied is returned if the client is not authorized
 	ErrPermissionDenied = errors.New("permission denied")
 
+	// ErrInvalidToken is returned if the token is revoked, expired, or non-existent
+	ErrInvalidToken = errors.New("invalid token")
+
 	// ErrInvalidCredentials is returned when the provided credentials are incorrect
 	// This is used internally for user lockout purposes. This is not seen externally.
 	// The status code returned does not change because of this error

vault/core_test.go

@@ -1175,7 +1175,7 @@ func TestCore_HandleRequest_InvalidToken(t *testing.T) {
 	if err == nil || !errwrap.Contains(err, logical.ErrPermissionDenied.Error()) {
 		t.Fatalf("err: %v", err)
 	}
-	if resp.Data["error"] != "permission denied" {
+	if !strings.Contains(resp.Data["error"].(string), "permission denied") {
 		t.Fatalf("bad: %#v", resp)
 	}
 }
@@ -1251,6 +1251,26 @@ func TestCore_HandleRequest_RootPath_WithSudo(t *testing.T) {
 	}
 }
 
+// TestCore_HandleRequest_TokenErrInvalidToken checks that a request made
+// with a non-existent token will return the "permission denied" and "invalid token" error
+func TestCore_HandleRequest_TokenErrInvalidToken(t *testing.T) {
+	c, _, _ := TestCoreUnsealed(t)
+
+	req := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "secret/test",
+		Data: map[string]interface{}{
+			"foo":   "bar",
+			"lease": "1h",
+		},
+		ClientToken: "bogus",
+	}
+	resp, err := c.HandleRequest(namespace.RootContext(nil), req)
+	if err == nil || !errwrap.Contains(err, logical.ErrInvalidToken.Error()) || !errwrap.Contains(err, logical.ErrPermissionDenied.Error()) {
+		t.Fatalf("err: %v, resp: %v", err, resp)
+	}
+}
+
 // Check that standard permissions work
 func TestCore_HandleRequest_PermissionDenied(t *testing.T) {
 	c, _, root := TestCoreUnsealed(t)
@@ -1271,6 +1291,69 @@ func TestCore_HandleRequest_PermissionDenied(t *testing.T) {
 	}
 }
 
+// TestCore_RevokedToken_InvalidTokenError checks that a request
+// returns an "invalid token" and a "permission denied" error when a token
+// that has been revoked is used in a request
+func TestCore_RevokedToken_InvalidTokenError(t *testing.T) {
+	c, _, root := TestCoreUnsealed(t)
+
+	// Set the 'test' policy object to permit access to sys/policy
+	req := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "sys/policy/test", // root protected!
+		Data: map[string]interface{}{
+			"rules": `path "sys/policy" { policy = "sudo" }`,
+		},
+		ClientToken: root,
+	}
+	resp, err := c.HandleRequest(namespace.RootContext(nil), req)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	if resp != nil && (resp.IsError() || len(resp.Data) > 0) {
+		t.Fatalf("bad: %#v", resp)
+	}
+
+	// Child token (non-root) but with 'test' policy should have access
+	testMakeServiceTokenViaCore(t, c, root, "child", "", []string{"test"})
+	req = &logical.Request{
+		Operation:   logical.ReadOperation,
+		Path:        "sys/policy", // root protected!
+		ClientToken: "child",
+	}
+	resp, err = c.HandleRequest(namespace.RootContext(nil), req)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	if resp == nil {
+		t.Fatalf("bad: %#v", resp)
+	}
+
+	// Revoke the token
+	req = &logical.Request{
+		ClientToken: root,
+		Operation:   logical.UpdateOperation,
+		Path:        "auth/token/revoke",
+		Data: map[string]interface{}{
+			"token": "child",
+		},
+	}
+	resp, err = c.HandleRequest(namespace.RootContext(nil), req)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	req = &logical.Request{
+		Operation:   logical.ReadOperation,
+		Path:        "sys/policy", // root protected!
+		ClientToken: "child",
+	}
+	_, err = c.HandleRequest(namespace.RootContext(nil), req)
+	if err == nil || !errwrap.Contains(err, logical.ErrPermissionDenied.Error()) || !errwrap.Contains(err, logical.ErrInvalidToken.Error()) {
+		t.Fatalf("err: %v, resp: %v", err, resp)
+	}
+}
+
 // Check that standard permissions work
 func TestCore_HandleRequest_PermissionAllowed(t *testing.T) {
 	c, _, root := TestCoreUnsealed(t)

vault/request_handling.go

@@ -248,7 +248,7 @@ func (c *Core) fetchACLTokenEntryAndEntity(ctx context.Context, req *logical.Req
 
 	// Ensure the token is valid
 	if te == nil {
-		return nil, nil, nil, nil, logical.ErrPermissionDenied
+		return nil, nil, nil, nil, multierror.Append(logical.ErrPermissionDenied, logical.ErrInvalidToken)
 	}
 
 	// CIDR checks bind all tokens except non-expiring root tokens
@@ -627,6 +627,9 @@ func (c *Core) handleCancelableRequest(ctx context.Context, req *logical.Request
 
 	err = c.PopulateTokenEntry(ctx, req)
 	if err != nil {
+		if errwrap.Contains(err, logical.ErrPermissionDenied.Error()) {
+			return nil, multierror.Append(err, logical.ErrInvalidToken)
+		}
 		return nil, err
 	}
 
@@ -1025,7 +1028,7 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp
 		}
 		if te == nil {
 			// Token has been revoked by this point
-			retErr = multierror.Append(retErr, logical.ErrPermissionDenied)
+			retErr = multierror.Append(retErr, logical.ErrPermissionDenied, logical.ErrInvalidToken)
 			return nil, nil, retErr
 		}
 		if te.NumUses == tokenRevocationPending {