From 0e19c1566eec0da633e315f8e41c5e0bfa83691d Mon Sep 17 00:00:00 2001 From: divyaac Date: Thu, 14 Mar 2024 10:22:57 -0700 Subject: [PATCH] Added Invalid Token Error Message that will be returned for bad tokens Edited changelog Added dummy policy to CE file to make tests pass Added changelog --- changelog/25953.txt | 4 ++ http/auth_token_test.go | 134 ++++++++++++++++++++++++++++++++++++++ sdk/logical/error.go | 3 + vault/core_test.go | 85 +++++++++++++++++++++++- vault/request_handling.go | 7 +- 5 files changed, 230 insertions(+), 3 deletions(-) create mode 100644 changelog/25953.txt diff --git a/changelog/25953.txt b/changelog/25953.txt new file mode 100644 index 000000000000..42132176bc61 --- /dev/null +++ b/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 +``` \ No newline at end of file diff --git a/http/auth_token_test.go b/http/auth_token_test.go index 37903a418583..92bf67e00fa9 100644 --- a/http/auth_token_test.go +++ b/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) +} diff --git a/sdk/logical/error.go b/sdk/logical/error.go index 4d78651648f9..dcd030dedf63 100644 --- a/sdk/logical/error.go +++ b/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 diff --git a/vault/core_test.go b/vault/core_test.go index 789071b7872e..824188ae06e0 100644 --- a/vault/core_test.go +++ b/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) diff --git a/vault/request_handling.go b/vault/request_handling.go index 725b622565ed..5ac83c1d7522 100644 --- a/vault/request_handling.go +++ b/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 {