Improve Vault integration and validation #2226

dadgar · 2017-01-22T01:50:11Z

This PR:

Validates the token given to Nomad to ensure it has the required capabilities on the needed paths
Allows specifying a role to create a token from even if the token is not from the given role.
Adds tests to the blacklist pattern of Vault roles (disallowed_policies).
updates the Vault API to use the new POST rather than GET methods such that the tokens are not written in the URL.

Check the capabilities of the Vault token to ensure it is valid and also allow targetting of a role that the token is not from.

jefferai · 2017-01-23T19:25:33Z

nomad/structs/config/vault.go

+	// Nomad does not have to be created from this role but must have "update"
+	// capability on "auth/token/create/<create_from_role>". If this value is
+	// unset and the token is created from a role, the value is defaulted to the
+	// role the token is from.


Why? What is the use case, ever, for using a role to create a token to use the role?

Backwards compatibility, we are in a point release, would break peoples configs.

Has anyone actually set it up like this? There has never been a need for this to be the required setup.

There was no other option which is what started this all 👍 If one was using role based tokens, they would be hit by the behavior change

There was always an option, well before the first release with Vault integration.

jefferai · 2017-01-23T19:26:08Z

nomad/vault.go

+	v.tokenData = &data
+
+	// The criteria that must be met for the token to be valid are as follows:
+	// 1) If token is non-root or is but has a creation ttl


Tokens should never be root unless Nomad is in dev mode.

We don't restrict people to that. That is an organizational decision not one Nomad will enforce on people. We just highly discourage it in our docs

I use a root token and would really like to keep doing that... we only use vault for ease of storage, not for the hardcore security and ACL level stuff

@dadgar Per our original discussions around implementation it was never supposed to be allowed outside of dev mode in the first place.

@jippi You should still not use root tokens, if for no other reason than that they are quite difficult to generate at this point. Just use a periodic token with policies giving all privileges to /*.

jefferai · 2017-01-23T19:30:01Z

nomad/vault.go

+// capabilities the token does have as well as any error that occured.
+func (v *vaultClient) hasCapability(path string, required []string) (bool, []string, error) {
+	caps, err := v.client.Sys().CapabilitiesSelf(path)
+	if err != nil {


"root" in the required slice should be conditional on Nomad being in dev mode.

jefferai · 2017-01-23T19:31:31Z

nomad/vault.go

+	vaultTokenRenewPath = "auth/token/renew-self"
+
+	// vaultTokenLookupPath is the path used to lookup a token
+	vaultTokenLookupPath = "auth/token/lookup"


You should add auth/token/lookup-self as a lookup possibility. Most tokens can be looked up using that path as it's in the default policy, so if auth/token/lookup doesn't work you should use that instead (and probably prefer it).

I did this to limit the set of policies we have to document as Nomad requiring since we can reuse that for both looking our own token up and incoming tokens. But since I guard that validation (https://github.com/hashicorp/nomad/pull/2226/files#diff-87a4c76eca3de54716c55ba6b8c06475R682) I agree. Will make the change

Using lookup-self can be a good secondary option. I agree that keeping lookup there is not an issue.

jefferai · 2017-01-23T19:32:40Z

nomad/vault.go

+		return v.config.Role
+	}
+
+	return v.tokenData.Role


Don't do this.

Mentioned above why it is like this for now

jefferai · 2017-01-23T19:33:39Z

nomad/vault.go

@@ -602,6 +762,12 @@ func (v *vaultClient) validateRole(role string) error {
 		multierror.Append(&mErr, fmt.Errorf("Role must have a non-zero period to make tokens periodic."))
 	}

+	for _, d := range data.DisallowedPolicies {
+		if d == "default" {
+			multierror.Append(&mErr, fmt.Errorf("Role can not disallow allow default policy"))


Sure it can.

Ah good catch! That was for me while I was testing something!

jefferai · 2017-01-23T19:53:19Z

nomad/vault.go

+	//   a) Must have read capability for "auth/token/roles/<role_name" (Can just attemp a read)
+	//   b) Must have update capability for path "auth/token/create/<role_name>"
+	//   c) Role must:
+	//     1) Not allow orphans


I'm guessing that this is copypasta from Asana but you probably mean must allow orphans. Or should allow orphans.

Was not changing this behavior in this PR. Down the line we may want to relax this constraint and let the operator decide if they would like to allow orphans. But for now we are not allowing it so that when Nomad revokes a token the whole tree is revoked.

By extension this means that if Nomad's token becomes invalid all jobs also now have invalid tokens. This is not the suggested operational model that we tell people to use; we recommend orphan periodic tokens for services. There is no reason to artificially impose this upon administrators; they should be free to use their own judgement, especially since you're already requiring periodic tokens.

jefferai · 2017-01-23T19:55:57Z

nomad/vault.go

+	//   c) Role must:
+	//     1) Not allow orphans
+	//     2) Must allow tokens to be renewed
+	//     3) Must not have an explicit max TTL


If you can limit job length, allowing an explicit max TTL is a decent idea as an additional check.

It is not a bad idea for those who want it but for now I am keeping it at being a periodic token with no max TTL. We can revisit in another PR/release

jefferai · 2017-01-23T19:56:52Z

nomad/vault.go

+	//     2) Must allow tokens to be renewed
+	//     3) Must not have an explicit max TTL
+	//     4) Must have non-zero period
+	// 5) If not configured against a role, the token must be root


If not configured against a role, you still shouldn't require a root token. There's no need. You could require a token that has sudo capability against auth/token/create. Or you could just try to make a token and see if it works.

This was based on your own suggestion:

5a) Don't require this, it's nearly the same as requiring a root token.

Exactly. Don't require a root token. You also shouldn't require a sudo token, but if you insist that you really must have something with super high privileges on that endpoint, make it a sudo token so that you aren't encouraging people to pass in root tokens.

What you should do instead is to just try making the token, and it will work or it will not. There's no reason to be running these checks.

github-actions · 2023-04-06T02:10:06Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

dadgar added 9 commits January 19, 2017 13:40

Check capabilities, allow creation against role

5fb01bf

Check the capabilities of the Vault token to ensure it is valid and also allow targetting of a role that the token is not from.

Vendor vault/api

6f9367a

closer on the tests

50bab64

Test pass

9119ec8

Cleanup errors/comments

fe59808

verify we can renew ourselves

af4330a

Test new functionality

a4a3de4

update website

1119f27

Fix blacklist test

c9115b8

jefferai reviewed Jan 23, 2017

View reviewed changes

Prefer looking up using self path and remove checking for default policy

e5536ac

jefferai reviewed Jan 23, 2017

View reviewed changes

dadgar merged commit 7bd018e into master Jan 23, 2017

dadgar deleted the b-vault branch January 23, 2017 22:59

stevenscg mentioned this pull request Feb 14, 2017

[question] Vault Integration with Token Role #2316

Closed

BSick7 mentioned this pull request Mar 13, 2017

[question] Does the agent configuration support hot reloading? #2437

Closed

github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Vault integration and validation #2226

Improve Vault integration and validation #2226

dadgar commented Jan 22, 2017 •

edited

Loading

jefferai Jan 23, 2017

dadgar Jan 23, 2017

jefferai Jan 23, 2017

dadgar Jan 23, 2017

jefferai Jan 24, 2017

jefferai Jan 23, 2017

dadgar Jan 23, 2017 •

edited

Loading

jippi Jan 23, 2017

jefferai Jan 23, 2017

jefferai Jan 23, 2017

jefferai Jan 23, 2017

jefferai Jan 23, 2017

dadgar Jan 23, 2017

jefferai Jan 23, 2017

jefferai Jan 23, 2017

dadgar Jan 23, 2017

jefferai Jan 23, 2017

dadgar Jan 23, 2017

jefferai Jan 23, 2017

dadgar Jan 23, 2017

jefferai Jan 24, 2017

jefferai Jan 23, 2017

dadgar Jan 23, 2017

jefferai Jan 23, 2017

dadgar Jan 23, 2017

jefferai Jan 24, 2017

github-actions bot commented Apr 6, 2023

Improve Vault integration and validation #2226

Improve Vault integration and validation #2226

Conversation

dadgar commented Jan 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadgar Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 6, 2023

dadgar commented Jan 22, 2017 •

edited

Loading

dadgar Jan 23, 2017 •

edited

Loading