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

Add namespace config option to agent config #6988

Merged
merged 3 commits into from
Jul 3, 2019

Conversation

michelvocks
Copy link
Contributor

Fixes #6981

@michelvocks michelvocks requested a review from vishalnayak June 26, 2019 09:35
command/agent.go Outdated
@@ -219,6 +219,12 @@ func (c *AgentCommand) Run(args []string) int {
Default: "https://127.0.0.1:8200",
EnvVar: api.EnvVaultAddress,
})
c.setStringFlag(f, config.Vault.Namespace, &StringVar{
Copy link
Contributor

Choose a reason for hiding this comment

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

If we end up implementing this, we should also print in during agent's startup (probably by appending to infoKeys ).

@calvn
Copy link
Contributor

calvn commented Jun 26, 2019

I am not sure if it makes sense to have this config for agent caching since clients that hit the agent can fully control what namespace they want to hit. This is a bit different that Consul Template's scenario, where we don't actually control the API client, so there needs to be a config option exposed in order to specify which namespace we want to read/write from.

From a UX perspective, it might end up being confusing since my request on foo/ actually routes to my-ns/foo/ without me knowing unless I know how the agent was set up.

@michelvocks
Copy link
Contributor Author

@calvn I think your explanation makes sense. Consul Template's behavior and use-cases are different than the Vault Agent and what features it provides.

@mbeiter
Copy link

mbeiter commented Jun 27, 2019

@calvn I disagree: The client does not know (and should not know) which endpoint to hit. The vault agent is abstracting this from the client, in the same way that it abstracts the address of the Vault server away from the client.

As an example, we are running a Vault server in staging, and one in production -> two different VAULT_ADDR for this. Within each of them, there are namespaces for dev/test/prod -> three different VAULT_NAMESPACE for that.

I would accept your argument if the address config param in the vault stanza would not exist. In that case, it would be consistent to require the client launching vault agent to know what address and namespace to query, and to provide that through the environment variables.

However, as the address parameter exists, it would only be logical to also be able to specify the namespace, hence having the target environment for the vault agent fully specified in the vault stanza.

In my opinion, not supporting the namespace parameter results in either:

  1. Fragmentation of the configuration (part of it in the client launching vault agent, and part of it in the vault agent config file), with splitting of the vault client configuration in two locations (which is bad) OR
  2. In an attempt to keep the configuration atomic (from a configuration management perspective) the client not being able to use the address parameter in the vault agent config, thus rendering that option somewhat useless in deployments that use namespaces

Both of these are specifically a problem when multiple clients share the same vault agent: In that case, either one of the clients needs to be chosen to launch that vault agent (which is really bad from a coupling perspective), or another mechanism needs to be introduced to launch the vault agent (which needs to be maintained, monitored, etc - also bad).

If the configuration is all contained in the vault agent config file, then all it takes is a simple "if vault agent is not already running, then start it" logic, which makes this thing universal and fairly fail-safe.

@calvn
Copy link
Contributor

calvn commented Jun 27, 2019

Are you referring to Vault agent for auto-auth or caching capabilities? If it's the former I think that it would make sense to have the namespace be configurable. This should probably go under auto_auth.method.namespace though (at the same level as mount_path).

The vault stanza also gets used by agent's caching feature, which I don't think should have a particular namespace scoped in that mode of operation. I don't think it makes sense for agent caching to have that configuration set for its proxy client since clients that makes requests against agent (e.g. cURL, api.Client, etc.) can, and should, specify what namespace they want to hit independently.

@mbeiter
Copy link

mbeiter commented Jun 27, 2019

I am only using vault agent for auto auth, so from that perspective, your proposal of placing it into the auth method may work.

However, in our deployment, the auth method is configured in the namespace. The reason for this is that the namespace admins have 100% control over how they would like to control their namespace, which includes configuration of their own auth method. This is not uncommon for a SaaS service (which is how we operate our Vault Enterprise deployment).

Consequently, the auth token retrieved by the auth method is only useful in the specific namespace it has been retrieved for. In this case, if you placed the namespace config into the method, the caching would not work against other namespaces, because its token would be invalid there. Unless of course you would also change use_auto_auth_token = true from a binary to somehow reference the auth method of which it should use the auth token from.

I could imagine that someone may use different namespaces for the caching use case, but from all I know, when namespaces were introduced in Vault, they were marketed as a feature to create "virtual" Vault servers with decentralized administration. When looking at it from that perspective, it would not make much sense to have a single Vault agent instance serve multiple "virtual Vault servers". If I were to deploy clients that have to connect to different Vault namespaces, I would probably want them separated to avoid any potential contamination, and consequently run multiple instances of vault agent (one for each addr + namespace combo) to keep things strictly isolated (different PIDs, users, groups).

@calvn
Copy link
Contributor

calvn commented Jun 27, 2019

Consequently, the auth token retrieved by the auth method is only useful in the specific namespace it has been retrieved for. In this case, if you placed the namespace config into the method, the caching would not work against other namespaces, because its token would be invalid there. Unless of course you would also change use_auto_auth_token = true from a binary to somehow reference the auth method of which it should use the auth token from.

This would be true even if we placed namespace in the vault stanza, unless we also made agent caching proxy all requests to that namespace, which is a behavior that I am not too fond of.

For the case where caching is enabled, use_auto_auth_token = true, and a namespace'd token is used (i.e. auto_auth.method.namespace is set), we could allow such requests to happen (as they do now) and if the client issues a request against a namespace that the auto-auth token is not allowed to access, it will simply get denied.

If the client wants to make requests to a specific namespace through agent caching (and not provide its own token in the request), it will have to talk to the right agent which satisfies the "virtual Vault servers" model.

@mbeiter
Copy link

mbeiter commented Jun 27, 2019

Does this summarize your proposal correctly then:

  1. Place namespace into auto_auth.method (at the same level as mount_path)
  2. This will be used to authenticate, addressing the auto-auth use case. As there is only one auto_auth and one method stanza allowed each, this makes sure that (for each running instance of Vault agent) there is one and only one namespace used for authentication
  3. If clients want to authenticate to multiple namespaces, they must run multiple instances of Vault agent
  4. When using caching, clients must always specify the namespace they are requesting secrets against

From my perspective (as I am primarily interested in auto-auth at this point), this would work just fine.

However, I do see the risk of some inconsistency / confusion for the users of the caching function ("why do I have to specify the namespace for my caching request, if authN worked just fine without it?!?").

This would be true even if we placed namespace in the config stanza, unless we also made agent caching proxy all requests to that namespace, which is a behavior that I am not too fond of.

I understand. One more suggestion though before I give up on this one: What do you think about moving this back into the vault stanza after all, and additionally introducing another option use_default_namespace in the cache stanza to control this, like so:

vault {
        address = "https://127.0.0.1:8200"
        namespace = "my-namespace"
}

auto_auth {
<unchanged>
}

cache {
        use_auto_auth_token = true
        use_default_namespace = false
}

If use_default_namespace set to true, Vault agent uses the namespace from the vault stanza for caching requests. If set to false, the user must specify the namespace in the request as you suggested.

That seems to be backwards compatible and make for a good UX:

  • If you implement use_default_namespace = false and namespace = "" as the default settings, then we would get today's behavior
  • When the user sets namespace = "my-namespace", and does not touch use_default_namespace, then we get the behavior you described (caching client has to specify the namespace they want to query against for each request)
  • When the user additionally sets use_default_namespace = true, then both the authN (to get the token) and the subsequent caching requests go to the namespace (which would address the point of possible confusion I highlighted above).
  • If the user sets use_default_namespace = true, but happens to not set namespace, the default setting of "" should make sure that there is no failure.

@calvn
Copy link
Contributor

calvn commented Jun 27, 2019

I am awaiting feedback from the team on this one. However, it's been brought up that there are wider changes around the Vault API down the road that would facilitate the interaction with agent which will likely encompass namespace behavior, so it's possible that we will hold from committing to something until then.

@mbeiter
Copy link

mbeiter commented Jun 27, 2019

Sounds good, please keep me posted

@jefferai
Copy link
Member

I don't really think this needs to be coupled to caching; the fact that a path can be gotten to by full path vs namespace plus relative path means this is just an implementation nicety... exactly in the same vein as the -ns CLI parameter. I don't see a reason not to add this.

As for caching, I don't think we should set a default namespace. The client making normal requests to Vault is responsible for setting the namespace appropriately. The fact that there is a proxy in front doesn't change that. To do otherwise is to make the proxy less transparent.

@jefferai jefferai reopened this Jun 28, 2019
@jefferai jefferai added this to the 1.2 milestone Jun 28, 2019
@michelvocks michelvocks force-pushed the agent_namespace_config branch from 2769140 to 67f0a4f Compare July 2, 2019 14:00
mgaffney
mgaffney previously approved these changes Jul 2, 2019
calvn
calvn previously approved these changes Jul 2, 2019
Copy link
Contributor

@calvn calvn left a comment

Choose a reason for hiding this comment

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

Two small comments, looks good otherwise!

@jefferai jefferai added the beta label Jul 2, 2019
briankassouf
briankassouf previously approved these changes Jul 2, 2019
Copy link
Contributor

@briankassouf briankassouf left a comment

Choose a reason for hiding this comment

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

Agree with Calvin's comments, otherwise looks good!

@michelvocks michelvocks dismissed stale reviews from briankassouf, calvn, and mgaffney via 975a5c5 July 3, 2019 07:29
@michelvocks michelvocks merged commit b3cc25f into master Jul 3, 2019
@michelvocks michelvocks deleted the agent_namespace_config branch July 3, 2019 07:33
@TylerReid
Copy link
Contributor

@calvn have this changes been made or are still planned?

I am awaiting feedback from the team on this one. However, it's been brought up that there are wider changes around the Vault API down the road that would facilitate the interaction with agent which will likely encompass namespace behavior, so it's possible that we will hold from committing to something until then.

This scenario just happened for me:

However, I do see the risk of some inconsistency / confusion for the users of the caching function ("why do I have to specify the namespace for my caching request, if authN worked just fine without it?!?").

To give more details of why I am wanting to do that, we use different namespaces for various environments and I am looking to switch to using agent caching instead of the applications getting a token and managing everything. I would like to be able to remove all knowledge of namespaces out of the applications, so they just know that the secrets live at http://localhost:8200/v1/secrets/supersecretthing instead of having to build http://localhost:8200/v1/{dev | qa | prod}/secrets/supersecretthing based on the environment.

The tokens themselves are only good for the single namespace in my setup, and without this I have to have namespace info in both the agent config and the application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vault agent should have a "namespace" option in the "vault" stanza
7 participants