Vault credential manager supports lookup path templates

[agurney]

Existing Issue

Fixes #4965.

Why do we need this PR?

Some credential managers support lookup according to templates, but the Vault one didn't - the search paths were hardcoded, aside from the ability to customize a prefix. Now, with more general support for templated lookup paths, people whose secrets are organized in other ways can have an easier time integrating.

This will work for var_sources as well as for a globally configured credential manager. In #4965, the use case was for global configuration of lookups into

• /secret/TEAM/ci/PIPELINE
• /secret/TEAM/ci/

so that non-CI-relevant secrets in each TEAM namespace could be separated from Concourse's permissions. In the case of var_sources, somebody could now configure one or several sources in a pipeline pointing to specifically chosen sub-areas of a vault, without having to use a TEAM/PIPELINE scheme for that data.

Changes proposed in this pull request

• Refactor template support in all relevant credential managers (ssm, secretsmanager, conjur), which was previously duplicated. There is now a creds.SecretLookupWithTemplate type, analogously to creds.SecretLookupWithPrefix; when created, this gets a Go template.Template, and team and pipeline names. That template is instantiated with those, plus the variable name, on lookup. Some validation logic is also now transferred into the BuildSecretTemplate function.

• Alter the Vault manager to use templates. As discussed, it can use an arbitrary list of lookup templates. The default is to have /{{.Team}}/{{.Pipeline}}/{{.Secret}} and /{{.Team}}/{{.Secret}}, which matches the existing logic.

These templates are interpreted subject to any provided path prefix and namespace in Vault configuration, so if I set a namespace of /foo, prefix of /bar and a template of /baz/{{.Secret}}, then I'll end up looking in /foo/bar/baz/{{.Secret}}. This is consistent with how a "shared path" is handled currently, though really such a path is just a template that doesn't vary with Team and Pipeline.

In the linked issue, there was an idea of exposing different config knobs for vault when it's exposed in var_sources, compared to when it's set globally. Having not thought very deeply about it, my rough sense is in favor of being consistent and flexible, over cleaning up a set of options that's admittedly become a little cluttered. If we do want the config managers to take different options in general when appearing in var_sources, I'd appreciate any guidance about the overall cleanest way to express that structure.

Templates can be provided on the command line with --vault-lookup-templates or with an environment variable:

CONCOURSE_VAULT_LOOKUP_TEMPLATES=/foo/{{.Team}}/{{.Secret}},/bar/{{.Team}}/{{.Secret}}

For var_sources the configuration (I think?) ends up as lookup_templates: [...] under the vault config.

Contributor Checklist

• Unit tests
• Integration tests (if applicable)
• Updated documentation (located at https://github.com/concourse/docs)
• Updated release notes (located at https://github.com/concourse/concourse/tree/master/release-notes)

Not sure what integration tests are needed; I saw Topgun has Vault-related tests, but really just for making sure auth works, as opposed to probing any other details of the configuration. I also noticed the "don't worry about this" note in CONTRIBUTORS so I'm not worrying about it until informed otherwise 😉

I'll update docs if all this seems correct and desirable - at least the vault page and the var_sources one.
Documentation PR at concourse/docs#292
concourse-bosh-release PR at concourse/concourse-bosh-release#88

There should also be the same in concourse-chart, but as I'm not a Helm user I feel less confident on that one.

Reviewer Checklist

This section is intended for the core maintainers only, to track review progress. Please do not
fill out this section.

• Code reviewed
• Tests reviewed
• Documentation reviewed
• Release notes reviewed
• PR acceptance performed
• New config flags added? Ensure that they are added to the BOSH
  and Helm packaging; otherwise, ignored for the integration tests (for example, if they are Garden configs that are not displayed in the --help text).

[vito]

Sorry for the wait, taking a look at this now! 🙂

[vito]

Didn't find anything to suggest in code review but haven't done any testing locally yet. Thanks for adding test coverage for this! Too bad the other credential managers don't really have a good starting point for adding tests, but I don't think it'd be fair for me to ask you to backfill all that.

Just leaving my feedback for now in case you're interested in replying. I'll get back to this soon and try it out manually and merge unless something comes up. 🙂

[agurney]

@vito I've added some subsidiary PRs for bosh and docs, and fixed that bug you noticed.

[vito]

@agurney Sorry for the wait on this, getting back to it now!

The changes to latest.md will need to be updated - once we're good to go I'll be better about merging this quickly. Thanks for doing all the docs and BOSH updates too!

[agurney]

Thanks for getting back to this!
I've rebased and updated latest.md as requested. Plus, I was able to get the "proper" logic for detecting pipeline-dependent templates, in the newest commit 5f7d920 (and see its commit message for detail on my thinking). The implementation now tries to expand the template with no pipeline parameter given, at initialization time, and if that works with no error, it means the template didn't use .Pipeline. We save that information, so that where we previously tried to detect // when the pipeline name was an empty string, we now just refer to that cached flag.

[vito]

@agurney Awesome, thanks for looking into that! 👍

Notes from testing (in addition to inline comments):

• I had to explicitly prefix the lookup templates with / for it to work properly with the configured path prefix. I don't think this is a deal-breaker but I could see others running into it. 🤷‍♂️
• Historically test coverage has been pretty poor for some of these credential managers, and that this PR touches just about all of'em - though it's definitely a worthwhile change. Were you able to test any of this by hand? How do you feel about the existing coverage? (I'm not keen to force you to backfill a bunch of tests, especially considering I have a goal to phase these out in favor of Prototype-based implementations anyway.)

fyi: the release notes have a conflict again but if that's becoming too annoying don't worry about it, I can resolve it and push from my machine to merge.

[vito]

This name argument seems a little awkward to use - is it required, or can it just be a made-up string in creds.BuildSecretTemplate? 🤔

[agurney]

I think the only thing the names are good for, apart from maybe appearing in error messages, is for being invoked by name. Hopefully nobody is doing such a thing in the case of any of these templates! Errors are like template: foo:1: unexpected unclosed action in command for a template named foo that's defined as "{{". So there's a bit of convenience value in having that bubble up with a name attached. We could create with an empty name, which yields the same sort of error message in the end, just with template: :1:....

[vito]

I think we might need to do something different here. I noticed in testing that when configuring as a var source, the defaults don't seem to get replaced - the configuration is additive. They're replaced properly if I configure CONCOURSE_VAULT_LOOKUP_TEMPLATES though.

If you need a quick way to try this out, here's my pipeline:

var_sources:
- name: myvault
  type: vault
  config:
    url: https://vault:8200
    ca_cert: ((vault_ca_cert))
    client_cert: ((vault_client_cert))
    client_key: ((vault_client_key))
    lookup_templates: [/woahnelly]
    auth_backend: cert

jobs:
- name: use-creds
  plan:
  - task: print-secret
    config:
      platform: linux
      image_resource:
        type: mock
        source: {mirror_self: true}
      run:
        path: /bin/sh
        args:
          - -c
          - |
              echo ((myvault:some-secret))

Here's what I did:

# in concourse repo
$ ./hack/dc up
$ ./hack/vault/init
$ vault secrets enable -version=1 -path=concourse kv
$ vault write concourse/main/vault-repro/some-secret value=hello

# (replace these paths)
$ cat <<EOF > vault-values.yml
vault_ca_cert: $(cat ~/src/concourse/hack/vault/certs/vault-ca.crt | jq -Rs .)
vault_client_cert: $(cat ~/src/concourse/hack/vault/certs/concourse.crt | jq -Rs .)
vault_client_key: $(cat ~/src/concourse/hack/vault/certs/concourse.key | jq -Rs .)
EOF
$ fly -t l sp -p vault-repro -c vault-repro.yml -l vault-values.yml

When I triggered use-creds, I expected it to only look for /concourse/woahnelly/some-secret and result in an error, however it actually finds the credential in the default path and succeeds.

[agurney]

Thanks, trying to reproduce this.

[agurney]

Ah, got it. I didn't catch this earlier because it happens just when configuring <2 custom templates. They're individually overwriting the entries in the default array, as opposed to replacing it as one unit, or even appending. Looks like behavior from the mapstructure library which should be fine to tweak or work around.

[agurney]

It now inserts / when joining prefix to template in a more clever way than not at all, and I fiddled with the defaulting logic. There is an old-ish issue mitchellh/mapstructure#74 around the behavior for updating arrays and slices. This could possibly be achieved by a custom DecodeHook but I thought "eh, let's just write the logic directly instead of introducing a use-once abstraction".

On testing, I am very much relying on the existing suite for the other cred managers touched here. I agree it would be nice to have more coverage in order to be really confident that this sort of change doesn't accidentally destroy the intended end-to-end behavior. The unit tests don't quite get us there since we are a little short of verifying the full journey from configuration to execution.

Perhaps there's a way to get functional tests that invoke mock/stub versions of the various APIs? That might even be a little future-proof for when all this gets torn out in favor of the new model. I guess that means the cred managers become special resources? Then those tests could move fairly smoothly out of this repo into their future home. Or there's the material in topgun/, which is a bunch of setup, but apparently can test against the real backends.

[vito]

Thanks a bunch - gonna merge this now!