-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
VAULT-12940 Vault Agent uses Vault Agent specific User-Agent header when issuing requests #19776
Conversation
command/agent_test.go
Outdated
@@ -514,6 +515,256 @@ listener "tcp" { | |||
} | |||
} | |||
|
|||
// TestAgent_Template_UserAgent Validates that the User-Agent sent to Vault | |||
// as part of Templating requests is correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through this test but I am confused as to where we are validating User-Agent header here? Sorry, if I missed something here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, this is admittedly a bit of a weird test, but this seemed to be the best way to test it.
These tests use a custom handler func:
HandlerFunc: vaulthttp.HandlerFunc(
func(properties *vault.HandlerProperties) http.Handler {
h.props = properties
h.userAgentToCheckFor = useragent.AgentTemplatingString()
h.pathToCheck = "/v1/secret/data"
h.requestMethodToCheck = "GET"
h.t = t
return &h
}),
Using the userAgentHandler
in the same file.
In particular, when Vault receives a request that matches the paths/method, it can check the request's user agent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll improve the comment to point this out :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few comments for you, feel free to take a spy and see if anything is useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might feel like overkill, but I'm wondering now that we've got lots of functions that all create user agent strings that follow the same rules:
Is it worth introducing a single place they're 'built' for example:
func generateUserAgentString(product string, productVersion string, extension string) string {
userAgent := product
if productVersion != "" {
userAgent += fmt.Sprintf(" %s", productVersion)
}
userAgent += fmt.Sprintf("/%s (+%s; %s)", versionFunc(), projectURL, rt)
if extension != "" {
userAgent += fmt.Sprintf("; %s", extension)
}
return userAgent
}
Then pulling out some const
values for 'product' e.g.Vault
Vault Agent
, and product versions 'API Proxy', 'Auto-Auth', 'Templating'. It felt to me like that would reduce the chance of a formatting error in the strings for each function and group the various supported versions in a single place.
Just a suggestion/thought, as I appreciate we've got the tests in place now and it's not likely the user agents are going to change often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did consider this approach, but I thought there was value to the way it is now - it could just be my perception, but I find that this makes it clearer that you shouldn't be going around making custom user agents - and that the set we have today is an intentional set that could theoretically diverge in the future. Open to other opinions, but unless you feel strongly, I think I'll leave it as-is!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the thing that brought it to mind was the copy/paste stuff that caught us out in the comments, as strings are the prime target for things like that to slip in. So having params which were constants for the most part (and could be re-used) felt like a safer bet. But sure, leave it as-is, as I said .. there's tests in place for the output anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was struggling to follow along with some of the tests, I think mainly it seems quite hard to be able to set things up (Vault + Agent + all the config/state).
This is just an observation, and I'm guessing you've already tried to streamline the test code as much as possible, I'm just not the most familiar with it so it was a bit of a slog :)
Awaiting a fix to the pre-commit hooks before I can fix the branch's merge conflicts :) |
Vault Agent now uses the following User-Agent headers in the following situations (with version information, in all cases):
Vault Agent
Vault Agent API Proxy
(with the original User-Agent preserved at the end)Vault Agent API Proxy
Vault Agent Auto-Auth
Vault Agent Templating
For example,
Vault Agent Auto-Auth/1.14.0-beta1 (+https://www.vaultproject.io/; go1.20.1)
would be a full, complete string.There is a separate project to report User-Agent strings in the audit logs, making it easy for customers to determine which requests Vault received came from Agent, and in particular, which subsystem.
I've added tests that validate that Vault receives the correct User-Agent in all of these circumstances.
Sorry this is a big PR, but I think most of the size is in
agent_test
, so it isn't as intimidating as hopefully it first seems.