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

jobspec: Add vault secret stanza #11473

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lukas-w
Copy link
Contributor

@lukas-w lukas-w commented Nov 8, 2021

This PR adds a new secret stanza for loading secrets from Vault similar to the credentials stanza proposed by @schmichael in #3854 (comment). Compared to the existing way of loading secrets into environment variables using template, this solution

  • Allows using secrets elsewhere in the jobspec where templates' environment variables are not available such as the artifact stanza ([Feature] secrets in artifact stanza #3854, e.g. S3 credentials) or the driver config (e.g. Docker auth Docker registry auth from Vault or Variables #9740)
  • Has security benefits because secrets that are only needed internally within Nomad will not be exposed to the task
  • Isn't as verbose with more complex use cases such as generating certificates using the PKI Secrets Engine

In contrast to a possible HCL2 vault function (see #9434) this doesn't require Vault access from the CLI and doesn't leak secrets into the job spec.

As part of the vault prestart hook, requested secrets will be fetched from Vault and the response's .Data is made available as ${secret.<name>}:

# Example adapted from the linked comment by @schmichael  
job "example" {
  vault {
    policies = ["s3_artifacts", "dockerhub"]
    secret "s3" {
      path = "secret/data/s3/artifacts"
    }
    secret "docker" {
      path = "secret/data/dockerhub"
    }
  }
  group "example" {
    task "example" {
      artifact {
        source      = "..."
        destination = "..."
        options { 
          aws_access_key_id = "${secret.s3.access_key}"
          aws_access_key_secret = "${secret.s3.secret_key}"
        }
      }
      driver = "docker"
      config {
        image = "private/image"
        auth {
          username = "${secret.docker.user}"
          password = "${secret.docker.password}"
        }
      }
    }
  }
}

This isn't finished yet because I wanted to ask for feedback before putting more time into this. I've hardly done any development in Go before and I'm totally unfamiliar with Nomad's code, so there may be rookie mistakes. The test I wrote in vault_hook_test.go is a copy-pasty mess. I've changed the task runner hook order so that artifactHook is executed after vaultHook, not sure if this causes any problems. Also, this implementation doesn't support updates when a secret changes in Vault. I personally believe that's fine as long as it's documented; use cases like artifact don't need it and for use cases that do, template is still available. Some other things that are missing:

  • Support for writing data to Vault (needed for certificate generation)
  • Support for secret update & renewal
  • Per jobspec checklist: Diff logic for the new VaultSecret struct
  • Per jobspec checklist: Change detection
  • Documentation

Looking forward to some feedback! If this is a candidate for merging, I'd be happy to continue working on the remaining points though any help is appreciated, especially with documentation since that may better be left to a native speaker.

@hashicorp-cla
Copy link

hashicorp-cla commented Nov 8, 2021

CLA assistant check
All committers have signed the CLA.

@cgst
Copy link

cgst commented Feb 26, 2022

Hi! Are there currently any plans to ship this feature or an equivalent feature? We'd love to use this for Docker auth.

@manhtukhang
Copy link

Hi @lukas-w, the feature is very useful to me! Are you working on this?

@lukas-w
Copy link
Contributor Author

lukas-w commented Apr 26, 2022

I haven't done any more work on this yet as I was hoping for a reaction from a maintainer.

@mikenomitch
Copy link
Contributor

mikenomitch commented Apr 27, 2022

Hey @lukas-w and anybody following this, I'm really sorry that this PR sat for so long without a reply from us. It fell through the cracks. No good excuse on our end for letting good work linger. Apologies!

Regarding next steps, there is some context that I hope is generally good news, but will mean we would have to rework this PR quite a bit to get it in: We are going to add basic secrets into Nomad itself in 1.4. The basic idea being that users who aren't on Vault (yet) have an easily accessible place to store encrypted, static config and secrets (GH issue here).Right now we have v1 of that feature slated for 1.4, and we're planning to follow up with something like this PR's functionality after 1.4.

Because secrets might now have a Vault or Nomad backend, we'll want to build with that in mind up front.

Again, apologies on letting this linger for so long. This is definitely functionality that would be valuable, but just pragmatically we'll have to let it wait for a bit longer.

@lukas-w
Copy link
Contributor Author

lukas-w commented May 1, 2022

@mikenomitch Thanks for chiming in! Great to hear that this feature is on the roadmap. What I'm understanding is that it's best to close this PR and have a new shot at implementing it once a Secure Variables feature has taken shape?

In the meantime, do you have any recommendation on how to use Vault secrets in a more secure manner? Currently an allocation's vault token is world-readable (since 8e54601, see #11900) and while template outputs allow specifying permissions, there's no support for setting a file owner or group (#5020), so I struggle to find a way to provide secrets to e.g. to a Docker container that's using a non-root user without exposing said secrets to all system users.

I'm happy to help if there's a way to get this situation improved preferably in a patch release. Maybe an option to disable writing of the vault_token file in the vault stanza would be a simple enough change to get reviewed and integrated quickly? With that change that'd only leave the aforementioned Docker problem which can at least be mitigated using a permission-fix hook in the container. With the current state of secret handling in Nomad however, I'm hesitant to go into production with it. Maybe I'm just missing something though?

@grembo
Copy link
Contributor

grembo commented May 31, 2022

I'm happy to help if there's a way to get this situation improved preferably in a patch release. Maybe an option to disable writing of the vault_token file in the vault stanza would be a simple enough change to get reviewed and integrated quickly? With that change that'd only leave the aforementioned Docker problem which can at least be mitigated using a permission-fix hook in the container. With the current state of secret handling in Nomad however, I'm hesitant to go into production with it. Maybe I'm just missing something though?

@lukas-w #11900 contains a patch to disable writing vault_token to /secrets (instead it's written to a private directory outside of the container using strict file permissions).

I've been thinking of reducing the patch to only that specific feature (as this is all we really needed).

I'll update #11900 description to make this more clear.

Update: The patch is in #11905

@tgross tgross added the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows
Projects
Development

Successfully merging this pull request may close these issues.

7 participants