-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Support Vault entity aliases #12449
Support Vault entity aliases #12449
Conversation
b6c0b80
to
ed14438
Compare
Move some common Vault API data struct decoding out of the Vault client so it can be reused in other situations. Make Vault job validation its own function so it's easier to expand it. Rename the `Job.VaultPolicies` method to just `Job.Vault` since it returns the full Vault block, not just their policies. Set `ChangeMode` on `Vault.Canonicalize`. Add some missing tests.
Allows specifying an entity alias that will be used by Nomad when deriving the task Vault token. An entity alias assigns an indentity to a token, allowing better control and management of Vault clients since all tokens with the same indentity alias will now be considered the same client. This helps track Nomad activity in Vault's audit logs and better control over Vault billing.
Add support for a new Nomad server configuration to define a default entity alias to be used when deriving Vault tokens. This default value will be used if the task doesn't have an entity alias defined.
ed14438
to
69dc22b
Compare
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.
+1 from me on the UX!
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 is great @lgfa29! Just the usual suggestions ~
} | ||
expect := []string{"foo", "bar", "baz"} | ||
got := SetToSliceString(set) | ||
require.ElementsMatch(t, expect, got) |
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.
neato, was gonna comment about iteration order but this gets around that!
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.
Yup! It's a handy function, the problem is that I keep forgetting how it's called 😅
nomad/job_endpoint.go
Outdated
} | ||
} | ||
} | ||
if err := j.validateVault(args.Job); err != nil { |
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.
nice refactoring! in the long term I'd like to get these all into the admissions controller plumbing
nomad/nomad/job_endpoint_hooks.go
Line 53 in 69dc22b
func (j *Job) admissionControllers(job *structs.Job) (out *structs.Job, warnings []error, err error) { |
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.
Nice! Since there are a few refactorings going on already, I may as well do this now 🙂
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.
nomad/job_endpoint.go
Outdated
} | ||
} | ||
|
||
// Check entity aliases. |
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.
For the entity alias validation, can we move this chunk into a helper method? similar to multiVaultNamespaceValidation
above.
nomad/job_endpoint.go
Outdated
// - the token used to submit the job must be allowed to use the default | ||
// entity alias | ||
// - except if all Vault blocks in the job define an alias, since in this | ||
// case the server alias would not be used. |
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 is implied from the summary above, but an explicit comment here saying:
"// Assign the default entity alias to any vault block with no entity alias already set"
would be helpful
To allows tasks to receive tokens that are associated with an entity, the role | ||
must have a list of `allowed_entity_aliases` that includes this entity aliases | ||
that are expected to be used. This field supports globbing to cover multiple | ||
aliases. |
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.
trying to make it more succinct
For tasks to receive tokens associated with a Vault entity, the role must include a list of
allowed_entity_aliases
indicating the entity aliases allowed for use. This field supports
globbing to cover multiple entity aliases.
also is there vault docs to link to for the globbing rules?
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.
Thanks!
The most comprehensive doc I found for globbing was this: https://www.vaultproject.io/docs/concepts/policies#policy-syntax
But I don't know if it applies to allowed_entity_aliases
so I kept it kind of vague 😅
After a more detailed analysis of this feature, the approach taken in PR #12449 was found to be not ideal due to poor UX (users are responsible for setting the entity alias they would like to use) and issues around jobs potentially masquerading itself as another Vault entity.
After a more detailed analysis of this feature, the approach taken in PR #12449 was found to be not ideal due to poor UX (users are responsible for setting the entity alias they would like to use) and issues around jobs potentially masquerading itself as another Vault entity.
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. |
This PR adds support for deriving task tokens using an entity alias. This allows operators to better control the identity of the dynamic tokens that Nomad generates, which provide better control on billing and audit log parsing.
The entity alias can be set in the job and also a default value in the server configuration. If the server has a default alias, it will be used for tasks that don't define one.
In order to facilitate development, this PR starts by refactoring some of the Vault client internals. It may be easier to review this per commit.
To test this, you can use this script that will spin-up a Vault and Nomad agent preconfigured to support entity aliases.
Sample script run
Notice how only one token as
entity_id
. Uncommenting line 218 of the script would use a default entity alias for the second token.