Use docker-credential-helper to manage secrets

[sam-github]

Store service credentials securely in the stores supported by docker:

• https://github.com/docker/docker-credential-helpers#available-programs

Introduces a top-level config property, "secretStore" and additional
command line arguments to manage the stored secrets.

The value of secretStore is used to find a helper command,
docker-credential-<secretStore>.

The docker project currently provides 4 store helpers:

• "osxkeychain" (OS X only)
• "secretservice" (Linux only)
• "wincred" (Windows only)
• "pass" (any OS supporting pass, which uses gpg2)

Docker-for-desktop provides the credential helpers above, as well as
docker-credential-desktop.

Installation instructions for the helpers:

• https://github.com/docker/docker-credential-helpers#installation

Users could provide additional helpers, the only requirement is that the
helper implements the credential store protocol:

• https://github.com/docker/docker-credential-helpers#development

The credential protocol is open, and new credential stores can be
implemented by any CLI satisfying the protocol:

• https://github.com/docker/docker-credential-helpers#development

The modifications to existing modules is not tested due to lack
of API keys, but demonstrates the unobtrusive changes required to
use the secret store.

---

This is one approach to moving secrets out of config.yml, see #517

I'm curious to see if this scratches people's itch. The setup is pretty minimal, though the docker credential helpers need to be available, which they might be for any users of docker-for-desktop.

It uses existing secure storage mechanisms rather than inventing any. The secrets are stored in either the OS keyring, which should be pretty secure, or in https://www.passwordstore.org/ (which should work on any system with gpg2).

It requires pretty minimal changes to modules to support it (I included a bunch of - untested - module hacks in this PR as examples). Generally, you'd add wtf.secretStore: "" (it has a reasonable store default based on GOOS), and then remove your apiKey, and possibly username and API URL , from the module, and then save them with wtfutil save-secret URL_or_NAME SECRET , and that's it.

I also considered an approach where api keys and the like have a special string format, like https://www.home-assistant.io/docs/tools/keyring/, and that triggers the store lookup, but it seemed more opaque, and more onerous to the user, as well as not dealing so obviously well with usernames or API endpoints.

There are some limitations: this is struck to the docker-credential-helper's view of the world, which is that there is a SERVICE (any string, its not forced to be a URL, but it is a unique key), and that maps to a SECRET for one, single, named USER. The user can be defaulted to "default" in modules that don't need or want to store that, and the service can be the module name, so this seems to cover most use-cases reasonably.

The save-secret interface is a bit WIP ATM. It should really prompt for the secret (secrets should not be put in CLI arguments, they are visible briefly with ps). For the truly energetic, I assume an entire secret manager UI could be implemented (the helpers support listing existing credentials). The sub-command structure is pretty awful, too, for which I blame go-flags, I spent more time struggling with it than writing the actual code.

I've tested only with enterprise github, its the only one of the modules I have an API key for, so YMMV.

[senorprogrammer]

This is sweet, I like this a lot. Saving to the macOS keychain worked perfectly when saving my DO credentials.

Are you interested in continuing on the bits you describe as WIP?

[senorprogrammer]

For macOS keychain storage, it stores the "Name" as "Docker Credentials" - is that configurable? Storing as "WTFUtil" would be preferable.

[sam-github]

re: configurability, I'll check, but I suspect its hard-coded in their go code. I don't think its worth maintaining a docker-credential-helpers fork just to get that feature, but they might accept a PR to make it configurable with an env var.

I'll put together a check list of things to do before landing.

Thoughts on changing all the existing modules to use it, should I do that, or should it be an opt-in feature for the module maintainers (if that's actually a role). I can keep working through them, I think I did about half --- but I don't have a way to test, so I might not have gotten them all correct, and I didn't update their docs yet.

[senorprogrammer]

For the modules, I'm happy to do the ones I use, and put an indication in the documentation for each module which modules support secret storage. If anyone wants it added to a module that doesn't already have it, PRs would then always be welcome.

As for docs, when this lands, I'm happy to do those.

[jonhadfield]

Just had a quick try with your PR on MacOS. Integration with a module I wrote was as simple as adding the cfg.ConfigureSecret bit and it used keychain as expected. Nice work!

I'm wondering if it would be clearer to tell a module developer to add a prefix, e.g. 'secured', to a yaml key, e.g. token becomes securedToken, as a standard when using this. It's just that when I put a kv pair in the keychain and the yaml file, the yaml file entry won out, despite having to enter the password for the keychain.

I'll give some more feedback once I've had a chance to play with it properly.

[sam-github]

the yaml file entry won out, despite having to enter the password for the keychain

Can you post code? cfg.ConfigureSecret doesn't do a fetch unless at least one of the secret or username is wanted, where "wanted" means the module passed in a pointer to the string, and the string is not already set. That's why it uses the same name config vars. The secret fetcher isn't invoked unless unless required data was missing from the config. At least, that's the intention! I'd have to see your module to see what's going on. Is it one of the ones already in wtf?

[jonhadfield]

Try cloning this: https://github.com/jonhadfield/wtf/tree/secret-store-x. It's your PR plus integration with the pihole module.

I added a secret to my keychain using:
wtfutil save-secret pihole abcdefg12345678abcdefg12345678abcdefg12345678 token

    pihole:
      enabled: true
      apiUrl: http://192.168.0.1:1010/admin/api.php
      token: abcdefg12345678abcdefg12345678abcdefg12345678

Not sure if I messed up the integration, but it still prompts me for keychain if the token value exists, as below.

[sam-github]

@jonhadfield It looks to me that is invalid yaml, so token ends up being "" (empty string), so a lookup in the store occurs.

Try quoting the config

  mods:
    pihole:
      enabled: true
      apiUrl: "http://192.168.0.1:1010/admin/api.php"
      token: "abcdefg12345678abcdefg12345678abcdefg12345678"

That gets me non secret lookup (but of course I don't have a pihole account. I think this is unrelated to secrets:

p/wtf (jonhadfield u=) % ./bin/wtfutil

Errors in pihole.position configuration
 - Invalid value for top:       0       Error: Nonexistent map key at "position"
 - Invalid value for left:      0       Error: Nonexistent map key at "position"
 - Invalid value for width:     0       Error: Nonexistent map key at "position"
 - Invalid value for height:    0       Error: Nonexistent map key at "position"

[sam-github]

Hm, I think a secret prompt is necessary to land this, but golang.org/x/crypto/ssh/terminal doesn't look to support Windows, I'll take a try at https://github.com/chzyer/readline/blob/master/password.go

It looks like I could use tView as well, but it would have to be a full-screen prompt. But maybe that's not a big deal. You're the tView expert @senorprogrammer , would it be a reasonable way to put up a secret and (optional) username prompt?

[senorprogrammer]

@sam-github For what you're thinking, a modal made from a custom flex view with a form inside it could do that for you

• https://github.com/rivo/tview/wiki/Modal

See the flex example mid-way down the page. I can post example code if you like.

[sam-github]

readline seems to be working OK, and I think it supports windows, but I haven't tried.

Take a look. Code can always be improved, but I think this works from a functional perspective.

I tried to add support to all the modules in a way that loads what looks like "secrets" from config, but its hard to know for sure I got it right.

The most gaping hole is docs. Each module would need to have documented which of its config keys can be loaded, and which ones are secret (the username isn't).

[sam-github]

I'll take a shot at building a tview UI for browsing (list/add/remove) secrets, but not this week.

[senorprogrammer]

Just came across one issue with the save secrets implementation. If the username contains a space, it saves the username as the password in the keychain.

Examples:

wtfutil save-secret pagerduty Chris Cummer uDWxxxxxCD33xx-RrRfg
wtfutil save-secret pagerduty "Chris Cummer" uDWxxxxxCD33xx-RrRfg
wtfutil save-secret pagerduty Chris\ Cummer uDWxxxxxCD33xx-RrRfg

Perhaps I'm not understanding from this wtfutil save-secret URL_or_NAME SECRET how the username should be included in the saved data.

Should the username be included in the secret data? The implementation seems to suggest so:

if username != nil && *username == "" {
  *username = cred.Username
}

if secret != nil && *secret == "" {
  *secret = cred.Secret
}

[sam-github]

wtfutil save-secret URL_or_NAME SECRET

Oops, that's not the syntax anymore, it was:

% ./bin/wtfutil -h
...

Commands:
  save-secret <service> [secret [username]]
...

But its not that anymore, either :-). I removed secret so it can't be specified on the command line.

I'm not sure what to do about the meaning of service and username, they have to be defined by the module itself. Some might just have a service of "coolthing.com", that's fine.

Some modules might support multiple service names, maybe the module could have one instance for a public svc, another for a private, "https://api.coolthing.yourcorp.com" and "https://api.coolthing.com".

I saw examples of both patterns in the modules.

Current usage is below, and if extra args are provided on the CLI, it warns and exits.

% ./bin/wtfutil -h
Usage:
  wtfutil [OPTIONS] [command] [args...]

...

Commands:
  save-secret <service> [username]
    service      Service URL or name for secret.
    username     Username to associate with the service. Optional.
  Save a secret into the secret store. The secret will be prompted for.
  Requires wtf.secretStore to be configured.  See individual modules for
  information on what service, secret, and username means for their
  configuration. Not all modules use secrets, and not all secrets require
  a username.

[sam-github]

I pushed a new API.

@senorprogrammer Are you using the "username" feature? I want to remove it. Its there in the underlying credential helpers, but doesn't appear to be useful in any way. It doesn't allow multiple users per service API. It doesn't encrypt the username. Its just .... there.

And so while the username can be specified at save-secret time and then loaded later, explaining how it works, and why, etc. just seems like it creates a more confusing user-experience while not adding value. Lose-lose. :-)

The username just defaults to "default", ATM, which works fine with the helpers.

I'd just flat-out delete the support now, but if you used a username in your local setup, you'll need to resave the secret with no username.

[senorprogrammer]

@sam-github I'm not using username.

[sam-github]

PTAL, I think this might be landable now. EDIT: and I removed username support and docs, makes it cleaner, loses no security.

Main thing missing would be docs. There are in-line API docs but each module's docs need a bit added.

[sam-github]

note for self: also, the wtf.secretStore property mentioned in the cli usage message needs docing, and the info in this PR about how to find and install the helpers should go with that, and maybe some of the info from the commit message.

[senorprogrammer]

Documentation notwithstanding, unless there's a reason not to, I'm going to be rolling this in. I think this approach is easy to understand, and is compatible with other approaches considered in the security issue (e.g: inline back-ticked commands).