Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix misattribution of activity log entries to incorrect auth methods #18809

Merged
merged 7 commits into from
Aug 21, 2023

Conversation

maxb
Copy link
Contributor

@maxb maxb commented Jan 24, 2023

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.

maxb added 2 commits January 24, 2023 00:03
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.
distinctEntities uint64
distinctNonEntities uint64
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewer: Removed unused struct noticed in passing

@@ -1790,39 +1785,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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewer: A great deal of this PR is just dealing with this method now being able to return an error.

@maxb
Copy link
Contributor Author

maxb commented Aug 17, 2023

Hi @hsimon-hashicorp ,

This PR has been open for 6 months... are you able to help get it in front of someone who could review it?

I no longer have much interest in it as I no longer work with Vault Enterprise, but it's my last Vault PR that has been waiting for 6 months or more, and it would nice to get it closed out one way or the other.

@heatherezell
Copy link
Contributor

Hi @hsimon-hashicorp ,

This PR has been open for 6 months... are you able to help get it in front of someone who could review it?

I no longer have much interest in it as I no longer work with Vault Enterprise, but it's my last Vault PR that has been waiting for 6 months or more, and it would nice to get it closed out one way or the other.

Hey @maxb - thanks for your patience. I'll ping our engineers now. Appreciate the ping!

Copy link
Contributor

@VioletHynes VioletHynes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks for the PR, and thanks for the fix of this nuanced bug. Going to see if I can get this one merged.

@VioletHynes
Copy link
Contributor

Hey @maxb! It looks like TestActivityLog_SaveTokensToStorageDoesNotUpdateTokenCount is failing (due to the context in the test not having a namespace) and the linter is failing (should be fixed with a make fmt). Could you look into these? Then I'd love to get this one over the line!

@maxb
Copy link
Contributor Author

maxb commented Aug 19, 2023

@VioletHynes CI is green now

@raskchanky
Copy link
Contributor

I'm backporting this to 1.13 and 1.14 because a PR that I merged to main required fixes based on these changes and my PR needs to get backported to 1.13 and 1.14, so I need these changes to come along with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to indicate a potential bug core/auth core/log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants