-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 file
and file_perms
parameter to job's vault
stanza
#11905
Conversation
c19089c
to
da3d2f3
Compare
da3d2f3
to
175afd5
Compare
175afd5
to
9d28243
Compare
At the moment, `/secrets/vault_token` is written using the nomad process' umask, which - by default - is `0022`, which results in writing the vault_token world-readable (`0644` / `rw-r--r--`). This patch aims to address this by allowing to change the permissions used from the default `666` (before umask). The name `file_perms` was chosen to distinguish this from the parameter `perms` in the `template` stanza (as templates always operate on files, while Vault tokens are about permissions as well, which would've been confusing otherwise). Example of use: vault { policies = ["nomad-tls-policy"] change_mode = "noop" file_perms = "600" } I tried to extend unit tests to include basic checks for the feature, not sure if this is sufficient, though. Resolves hashicorp#11900
9d28243
to
04acc0c
Compare
A few force-pushes later the only failing test seems unrelated (and is marked FLAKY since Jan 12th):
|
2b3503c
to
ff688e3
Compare
This parameter controls whether the Vault token is actually written to `secrets/vault_token`. The Vault token is always written to `<task_dir>/private/vault_token`, `private` being a new directory to hold private secrets data not to be shared with the chroot and also not to be shared with Nomad UI. If `file` is set to `true` (the default), it is also written to `<task_dir>/secrets/vault_token`, using the permissions configured in `file_perms`. There are two rationales for this design: 1. Have one authoritative place where `vault_token` exists. 2. Don't read `vault_token` from a place that operates on a lower security level on Nomad restart. Ultimately, `secrets/vault_token` could have been altered in unfortunate or malicious ways from within the chroot. This was the primary reason to add an additional write operation instead of just modifying `tokenPath` depending on the value of `file`. Open questions: - Is creating a separate `private` directory worth the overhead?
ff688e3
to
5e52838
Compare
file
and file_perms
parameter to job's vault stanza
file
and file_perms
parameter to job's vault stanzafile
and file_perms
parameter to job's vault
stanza
Hi @hashicorp-cla @lgfa29 @tgross, I already signed the CLA in early January as part of #11783, so I don't think signing it again should be necessary. Please see here for when I signed it. Screenshot from #11783: The HashiCorp OAuth app still shows as authorized in my github settings: |
Sorry about that @grembo. The CLA bot went a bit wild over the weekend across all our repos. 😊 Should be fixed now. |
@tgross Any updates on this? In practice we ended up only using Based on this approach, we can issue all kinds of credentials (including vault tokens) to the container, without giving anything within the container the power to issue additional credentials itself, which is quite important to us. So if it helps to move things forward, I could reduce this PR to only contain I can also share more about our setup, in case there are open questions. |
@grembo sorry this never got marked for triage. I've marked it as such so that someone will take a look. At first glance I'm not sure this is the right approach. Why make the value configurable at all if you're putting the token in its own directory? I feel like this is handing operators a footgun. |
@tgross Just to re-iterate the motivation: The goal is to store the token in a separate directory that is not accessible from inside the container. If If payload backwards compatibility is not an issue - ultimately, job definitions can probably be altered to mimic the current behavior - I would be totally happy to change the patch so that it only consists of nomad always writing the vault token to If some compatibility is required, changing [0] Detour: This is also in line with nomad writing vault {
policies = ["tls-policy", "nomad-job-policy"]
change_mode = "noop"
env = false
file = false
} which allows us to do things like: template {
data = <<-EOH
{{ with secret "pki_int/issue/nomad-task"
"common_name=my.service.consul" "ttl=90m"
"alt_names=localhost" "ip_sans=127.0.0.1"}}
{{ .Data.certificate }}
{{ .Data.private_key }}
{{ .Data.issuing_ca }}
{{ end }}
EOH
destination = "${NOMAD_SECRETS_DIR}/my.crt"
change_mode = "noop"
perms = "600"
} |
@grembo sorry I didn't notice there was already an extensive discussion in #11900 about this. This goal
doesn't seen to be in evidence in that discussion though? Let's move this discussion about the overall design over to the issue though. I'll let @schmichael and @lgfa29 take it from there. |
@tgross Yeah, originally #11900 was about changing Hence my idea to split this PR/the issue and reduce the scope it handles, so it becomes single-topic and actionable. |
Closing this pull request in favor of #13343, which is smaller in scope. |
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. |
At the moment,
secrets/vault_token
is written using the nomadprocess' umask, which - by default - is
0022
, which results inwriting the vault_token world-readable (
0644
/rw-r--r--
).This patch aims to address this by allowing to change the
permissions used from the default
666
(before umask).The name
file_perms
was chosen to distinguish this from theparameter
perms
in thetemplate
stanza (as templates alwaysoperate on files, while Vault tokens are about permissions as
well, which would've been confusing otherwise).
Example of use:
The patch also adds the
file
parameter, which controls ifsecrets/vault_token
is written at all.The Vault token is always written to
<task_dir>/private/vault_token
,private
being a new directory to hold private secrets data not to beshared with the chroot and also not to be shared with Nomad UI.
If
file
is set totrue
(the default), it is also written to<task_dir>/secrets/vault_token
, using the permissions configuredin
file_perms
.There are two rationales for this design:
vault_token
exists.vault_token
from a place that operates on a lowersecurity level on Nomad restart. Ultimately,
secrets/vault_token
could have been altered in unfortunate or malicious ways from within
the chroot. This was the primary reason to add an additional write
operation instead of just modifying
tokenPath
depending on thevalue of
file
.Resolves #11900