From 35a5fbfc6002e0440c708e722dc8aabbcb7a81b2 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Mon, 21 Aug 2023 13:59:39 +0100 Subject: [PATCH] Fix misattribution of activity log entries to incorrect auth methods (#18809) * Fix misattribution of activity log entries to incorrect auth methods In a production Vault Enterprise instance, I noticed incorrect information in the sys/internal/counters/activity endpoints. Eventually, I was able to spot a pattern of entities being misattributed to auth methods of the same name in child namespaces, which led me to this bug in the code. When attempting to map from a token's path to an auth method, we need to do so with respect to the namespace of the token, which may be different from the namespace of the request, as tokens from parent namespaces can make requests that reach into child namespaces. * Changelog * Use a real namespace ID in tests where it now matters * gofumpt --------- Co-authored-by: Violet Hynes --- changelog/18809.txt | 3 +++ vault/activity_log.go | 28 +++++++++++++++++----------- vault/activity_log_test.go | 24 ++++++++++++++++++------ vault/request_handling.go | 9 ++++++--- 4 files changed, 44 insertions(+), 20 deletions(-) create mode 100644 changelog/18809.txt diff --git a/changelog/18809.txt b/changelog/18809.txt new file mode 100644 index 0000000000..a1ec06f579 --- /dev/null +++ b/changelog/18809.txt @@ -0,0 +1,3 @@ +```release-note:bug +activity (enterprise): Fix misattribution of entities to no or child namespace auth methods +``` diff --git a/vault/activity_log.go b/vault/activity_log.go index 1891e63183..c5214da19e 100644 --- a/vault/activity_log.go +++ b/vault/activity_log.go @@ -96,11 +96,6 @@ type segmentInfo struct { tokenCount *activity.TokenCount } -type clients struct { - distinctEntities uint64 - distinctNonEntities uint64 -} - // ActivityLog tracks unique entity counts and non-entity token counts. // It handles assembling log fragments (and sending them to the active // node), writing log segments, and precomputing queries. @@ -1915,39 +1910,50 @@ func (a *ActivityLog) loadConfigOrDefault(ctx context.Context) (activityConfig, // HandleTokenUsage adds the TokenEntry to the current fragment of the activity log // This currently occurs on token usage only. -func (a *ActivityLog) HandleTokenUsage(ctx context.Context, entry *logical.TokenEntry, clientID string, isTWE bool) { +func (a *ActivityLog) HandleTokenUsage(ctx context.Context, entry *logical.TokenEntry, clientID string, isTWE bool) error { // First, check if a is enabled, so as to avoid the cost of creating an ID for // tokens without entities in the case where it not. a.fragmentLock.RLock() if !a.enabled { a.fragmentLock.RUnlock() - return + return nil } a.fragmentLock.RUnlock() // Do not count wrapping tokens in client count if IsWrappingToken(entry) { - return + return nil } // Do not count root tokens in client count. if entry.IsRoot() { - return + return nil } // Tokens created for the purpose of Link should bypass counting for billing purposes if entry.InternalMeta != nil && entry.InternalMeta[IgnoreForBilling] == "true" { - return + return nil } + // Look up the mount accessor of the auth method that issued the token, taking care to resolve the token path + // against the token namespace, which may not be the same as the request namespace! + tokenNS, err := NamespaceByID(ctx, entry.NamespaceID, a.core) + if err != nil { + return err + } + if tokenNS == nil { + return namespace.ErrNoNamespace + } + tokenCtx := namespace.ContextWithNamespace(ctx, tokenNS) mountAccessor := "" - mountEntry := a.core.router.MatchingMountEntry(ctx, entry.Path) + mountEntry := a.core.router.MatchingMountEntry(tokenCtx, entry.Path) if mountEntry != nil { mountAccessor = mountEntry.Accessor } // Parse an entry's client ID and add it to the activity log a.AddClientToFragment(clientID, entry.NamespaceID, entry.CreationTime, isTWE, mountAccessor) + return nil } func (a *ActivityLog) namespaceToLabel(ctx context.Context, nsID string) string { diff --git a/vault/activity_log_test.go b/vault/activity_log_test.go index 268d92901a..226cc01a03 100644 --- a/vault/activity_log_test.go +++ b/vault/activity_log_test.go @@ -136,7 +136,10 @@ func TestActivityLog_Creation_WrappingTokens(t *testing.T) { } id, isTWE := te.CreateClientID() - a.HandleTokenUsage(context.Background(), te, id, isTWE) + err := a.HandleTokenUsage(context.Background(), te, id, isTWE) + if err != nil { + t.Fatal(err) + } a.fragmentLock.Lock() if a.fragment != nil { @@ -153,7 +156,10 @@ func TestActivityLog_Creation_WrappingTokens(t *testing.T) { } id, isTWE = teNew.CreateClientID() - a.HandleTokenUsage(context.Background(), teNew, id, isTWE) + err = a.HandleTokenUsage(context.Background(), teNew, id, isTWE) + if err != nil { + t.Fatal(err) + } a.fragmentLock.Lock() if a.fragment != nil { @@ -380,18 +386,24 @@ func TestActivityLog_SaveTokensToStorageDoesNotUpdateTokenCount(t *testing.T) { tokenPath := fmt.Sprintf("%sdirecttokens/%d/0", ActivityLogPrefix, a.GetStartTimestamp()) clientPath := fmt.Sprintf("sys/counters/activity/log/entity/%d/0", a.GetStartTimestamp()) // Create some entries without entityIDs - tokenEntryOne := logical.TokenEntry{NamespaceID: "ns1_id", Policies: []string{"hi"}} - entityEntry := logical.TokenEntry{EntityID: "foo", NamespaceID: "ns1_id", Policies: []string{"hi"}} + tokenEntryOne := logical.TokenEntry{NamespaceID: namespace.RootNamespaceID, Policies: []string{"hi"}} + entityEntry := logical.TokenEntry{EntityID: "foo", NamespaceID: namespace.RootNamespaceID, Policies: []string{"hi"}} idNonEntity, isTWE := tokenEntryOne.CreateClientID() for i := 0; i < 3; i++ { - a.HandleTokenUsage(ctx, &tokenEntryOne, idNonEntity, isTWE) + err := a.HandleTokenUsage(ctx, &tokenEntryOne, idNonEntity, isTWE) + if err != nil { + t.Fatal(err) + } } idEntity, isTWE := entityEntry.CreateClientID() for i := 0; i < 2; i++ { - a.HandleTokenUsage(ctx, &entityEntry, idEntity, isTWE) + err := a.HandleTokenUsage(ctx, &entityEntry, idEntity, isTWE) + if err != nil { + t.Fatal(err) + } } err := a.saveCurrentSegmentToStorage(ctx, false) if err != nil { diff --git a/vault/request_handling.go b/vault/request_handling.go index d206b15767..3459fc2737 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -271,12 +271,12 @@ func (c *Core) CheckToken(ctx context.Context, req *logical.Request, unauth bool var te *logical.TokenEntry var entity *identity.Entity var identityPolicies map[string][]string - var err error // Even if unauth, if a token is provided, there's little reason not to // gather as much info as possible for the audit log and to e.g. control // trace mode for EGPs. if !unauth || (unauth && req.ClientToken != "") { + var err error acl, te, entity, identityPolicies, err = c.fetchACLTokenEntryAndEntity(ctx, req) // In the unauth case we don't want to fail the command, since it's // unauth, we just have no information to attach to the request, so @@ -438,9 +438,12 @@ func (c *Core) CheckToken(ctx context.Context, req *logical.Request, unauth bool c.activityLogLock.RLock() activityLog := c.activityLog c.activityLogLock.RUnlock() - // If it is an authenticated ( i.e with vault token ) request, increment client count + // If it is an authenticated ( i.e. with vault token ) request, increment client count if !unauth && activityLog != nil { - activityLog.HandleTokenUsage(ctx, te, clientID, isTWE) + err := c.activityLog.HandleTokenUsage(ctx, te, clientID, isTWE) + if err != nil { + return auth, te, err + } } return auth, te, nil }