From fa8261a404c7ee863df63256eb10043db5509162 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Thu, 13 Jan 2022 22:40:16 +0100 Subject: [PATCH 1/3] Make auth/token/revoke-accessor idempotent The auth/token/revoke will not error out if the token does not exists, it always tries to revoke the token and return success to the client whether or not the token exists. This makes the behavior of auth/token/revoke-accessor coherent with this and remove the need to check whether the token still exists. --- vault/logical_system.go | 3 +++ vault/token_store.go | 33 +++++++++++++++++++++++++-------- vault/token_store_test.go | 7 +++++++ 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/vault/logical_system.go b/vault/logical_system.go index 42e5056e7b1c..783fce4c1b67 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -670,6 +670,9 @@ func (b *SystemBackend) handleCapabilitiesAccessor(ctx context.Context, req *log if err != nil { return nil, err } + if aEntry == nil { + return nil, &logical.StatusBadRequest{Err: "invalid accessor"} + } d.Raw["token"] = aEntry.TokenID return b.handleCapabilities(ctx, req, d) diff --git a/vault/token_store.go b/vault/token_store.go index e7faa50e2e31..7060506b6930 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -747,6 +747,10 @@ func (ts *TokenStore) tokenStoreAccessorList(ctx context.Context, req *logical.R resp.AddWarning(fmt.Sprintf("Found an accessor entry that could not be successfully decoded; associated error is %q", err.Error())) continue } + if aEntry == nil { + resp.AddWarning("Found an accessor entry that could not be successfully decoded: invalid accessor") + continue + } if aEntry.TokenID == "" { resp.AddWarning(fmt.Sprintf("Found an accessor entry missing a token: %v", aEntry.AccessorID)) @@ -1741,12 +1745,12 @@ func (ts *TokenStore) handleCreateAgainstRole(ctx context.Context, req *logical. return ts.handleCreateCommon(ctx, req, d, false, roleEntry) } -func (ts *TokenStore) lookupByAccessor(ctx context.Context, id string, salted, tainted bool) (accessorEntry, error) { +func (ts *TokenStore) lookupByAccessor(ctx context.Context, id string, salted, tainted bool) (*accessorEntry, error) { var aEntry accessorEntry ns, err := namespace.FromContext(ctx) if err != nil { - return aEntry, err + return nil, err } lookupID := id @@ -1755,7 +1759,7 @@ func (ts *TokenStore) lookupByAccessor(ctx context.Context, id string, salted, t if nsID != "" { accessorNS, err := NamespaceByID(ctx, nsID, ts.core) if err != nil { - return aEntry, err + return nil, err } if accessorNS != nil { if accessorNS.ID != ns.ID { @@ -1773,16 +1777,16 @@ func (ts *TokenStore) lookupByAccessor(ctx context.Context, id string, salted, t lookupID, err = ts.SaltID(ctx, id) if err != nil { - return aEntry, err + return nil, err } } entry, err := ts.accessorView(ns).Get(ctx, lookupID) if err != nil { - return aEntry, fmt.Errorf("failed to read index using accessor: %w", err) + return nil, fmt.Errorf("failed to read index using accessor: %w", err) } if entry == nil { - return aEntry, &logical.StatusBadRequest{Err: "invalid accessor"} + return nil, nil } err = jsonutil.DecodeJSON(entry.Value, &aEntry) @@ -1790,7 +1794,7 @@ func (ts *TokenStore) lookupByAccessor(ctx context.Context, id string, salted, t if err != nil { te, err := ts.lookupInternal(ctx, string(entry.Value), false, tainted) if err != nil { - return accessorEntry{}, fmt.Errorf("failed to look up token using accessor index: %w", err) + return nil, fmt.Errorf("failed to look up token using accessor index: %w", err) } // It's hard to reason about what to do here if te is nil -- it may be // that the token was revoked async, or that it's an old accessor index @@ -1808,7 +1812,7 @@ func (ts *TokenStore) lookupByAccessor(ctx context.Context, id string, salted, t aEntry.NamespaceID = namespace.RootNamespaceID } - return aEntry, nil + return &aEntry, nil } // handleTidy handles the cleaning up of leaked accessor storage entries and @@ -1959,6 +1963,10 @@ func (ts *TokenStore) handleTidy(ctx context.Context, req *logical.Request, data tidyErrors = multierror.Append(tidyErrors, fmt.Errorf("failed to read the accessor index: %w", err)) continue } + if accessorEntry == nil { + tidyErrors = multierror.Append(tidyErrors, fmt.Errorf("failed to read the accessor index: invalid accessor")) + continue + } // A valid accessor storage entry should always have a token ID // in it. If not, it is an invalid accessor entry and needs to @@ -2098,6 +2106,9 @@ func (ts *TokenStore) handleUpdateLookupAccessor(ctx context.Context, req *logic if err != nil { return nil, err } + if aEntry == nil { + return nil, &logical.StatusBadRequest{Err: "invalid accessor"} + } // Prepare the field data required for a lookup call d := &framework.FieldData{ @@ -2140,6 +2151,9 @@ func (ts *TokenStore) handleUpdateRenewAccessor(ctx context.Context, req *logica if err != nil { return nil, err } + if aEntry == nil { + return nil, &logical.StatusBadRequest{Err: "invalid accessor"} + } // Prepare the field data required for a lookup call d := &framework.FieldData{ @@ -2190,6 +2204,9 @@ func (ts *TokenStore) handleUpdateRevokeAccessor(ctx context.Context, req *logic if err != nil { return nil, err } + if aEntry == nil { + return nil, nil + } te, err := ts.Lookup(ctx, aEntry.TokenID) if err != nil { diff --git a/vault/token_store_test.go b/vault/token_store_test.go index a45b6c37be5f..8f5d9aac8a6a 100644 --- a/vault/token_store_test.go +++ b/vault/token_store_test.go @@ -931,6 +931,13 @@ func TestTokenStore_HandleRequest_Renew_Revoke_Accessor(t *testing.T) { t.Fatalf("err: %s", err) } + // Revoking the token using the accessor should be idempotent since + // auth/token/revoke is + _, err = ts.HandleRequest(namespace.RootContext(nil), req) + if err != nil { + t.Fatalf("err: %s", err) + } + time.Sleep(200 * time.Millisecond) out, err = ts.Lookup(namespace.RootContext(nil), "tokenid") From bc68caca4ab3a1ed9142001979185523fe4adb58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Thu, 13 Jan 2022 22:46:21 +0100 Subject: [PATCH 2/3] Add changelog --- changelog/13661.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelog/13661.txt diff --git a/changelog/13661.txt b/changelog/13661.txt new file mode 100644 index 000000000000..99ea592df13b --- /dev/null +++ b/changelog/13661.txt @@ -0,0 +1,4 @@ +```release-note:improvement +auth/token: The `auth/token/revoke-accessor` endpoint is now idempotent and will +not error out if the token has already been revoked. +``` From b2d934290b2c75c773b4313b20aba3ba2889bd2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Mon, 17 Jan 2022 22:31:39 +0100 Subject: [PATCH 3/3] Add warning when no token exists with this accessor Also removes the dubious warning when listing the tokens. --- vault/token_store.go | 5 +++-- vault/token_store_test.go | 5 ++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/vault/token_store.go b/vault/token_store.go index 7060506b6930..346efec3cf17 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -748,7 +748,6 @@ func (ts *TokenStore) tokenStoreAccessorList(ctx context.Context, req *logical.R continue } if aEntry == nil { - resp.AddWarning("Found an accessor entry that could not be successfully decoded: invalid accessor") continue } @@ -2205,7 +2204,9 @@ func (ts *TokenStore) handleUpdateRevokeAccessor(ctx context.Context, req *logic return nil, err } if aEntry == nil { - return nil, nil + resp := &logical.Response{} + resp.AddWarning("No token found with this accessor") + return resp, nil } te, err := ts.Lookup(ctx, aEntry.TokenID) diff --git a/vault/token_store_test.go b/vault/token_store_test.go index 8f5d9aac8a6a..8f0d36a0fd1b 100644 --- a/vault/token_store_test.go +++ b/vault/token_store_test.go @@ -933,10 +933,13 @@ func TestTokenStore_HandleRequest_Renew_Revoke_Accessor(t *testing.T) { // Revoking the token using the accessor should be idempotent since // auth/token/revoke is - _, err = ts.HandleRequest(namespace.RootContext(nil), req) + resp, err = ts.HandleRequest(namespace.RootContext(nil), req) if err != nil { t.Fatalf("err: %s", err) } + if len(resp.Warnings) != 1 { + t.Fatalf("Was expecting 1 warning, got %d", len(resp.Warnings)) + } time.Sleep(200 * time.Millisecond)