Proposal: Pluginspecific secrets management

[tboerger]

The current implementation of the secrets already works pretty well, but if you got more complex build pipelines it gets much more difficult because you want to publish to different environments or different docker registries depending on the environment.

Scenario

If we take a docker image publishing as an example, you want to push a docker image for every pull request to a private registry, and all tags to the docker hub, today you end up with something like that:

pipeline:
  docker-tag:
    image: plugins/docker
    repo: foo/bar
    tags: [ '${TAG}' ]
    when:
      event: [ tag ]
      branch: [ refs/tags/* ]

  docker-pr:
    image: plugins/docker
    environment:
      DOCKER_USERNAME: $PRIVATE_DOCKER_USERNAME
      DOCKER_PASSWORD: $PRIVATE_DOCKER_PASSWORD
      DOCKER_EMAIL: $PRIVATE_DOCKER_EMAIL
    repo: foo/bar
    tags: [ 'pr-${DRONE_PULL_REQUEST}' ]
    when:
      event: [ pull_request ]

If you want to add more scenarios you always have to add more secrets with a different variable name that have to be mapped within the environment: block.

Solution

I would suggest to add another flag to the secrets command like --name or --plugin which defines the name of the plugin step additionally to --event and --image, that way you can define multiple times environment variables like DOCKER_PASSWORD for the same image but for different pipeline steps.

Problems

1. Currently we are building the identifier of the secret based on repository + variable name, this needs to be extended to repository + variable name + pipeline step
2. We need to differentiate between a secret added including --name attribute and excluding it, IMO a secret including the --name attribute would override the secret excluding this flag
3. Deleting a secret via drone rm foo/bar DOCKER_PASSWORD is potentially not precise enough to match a single secret record, there we can return a list of matches and the user of the cli can decide which one should be deleted or we just return a useful error that complains about too much matching records.
4. Keep backward compatibility, this is a pretty important part because it's handling secrets that never have to be exposed to the wrong people. To keep it backward compatible I would take a default of a wildcard for the --name flag.

Suggestions

Do you have any further suggestions? Or maybe you got even a better idea how to handle that?

[bradrydzewski]

@tboerger thanks for taking the time to write the specification!

Keep backward compatibility, this is a pretty important part because it's handling secrets that never have to be exposed to the wrong people. To keep it backward compatible I would take a default of a wildcard for the --name flag.

This is also important because most people won't require the functionality described in this proposal (myself included). So this feature should not add any additional burden or complexity to those that do not require it. I think the default wildcard should suffice here.

[bradrydzewski]

return a useful error that complains about too much matching records.

I prefer the error message, since it is more simple

[bradrydzewski]

We need to differentiate between a secret added including --name attribute and excluding it, IMO a secret including the --name attribute would override the secret excluding this flag

I would prefer to avoid secret selection and ranking. I believe this feature will be used by a minority of users, and this edge case described here will impact an even smaller subset. Creating ranking logic seems like overkill until we know it is a real problem. I would rather document that people should avoid using --name=* and --name={step} for the same secret.

[vaijab]

Doesn't this contradict the fact that we explicitly trust plugins with our secrets?

[bradrydzewski]

Doesn't this contradict the fact that we explicitly trust plugins with our secrets?

This proposal shouldn't change that secrets are explicitly tied to plugin images. This would allow us to further refine our logic (optionally) to also restrict secrets by step name.

Consider the following Yaml:

pipeline:

  development:
    image: plugins/heroku
    app: dev.foo.com
    when:
      branch: master
      event: push

  production:
    image: plugins/heroku
    app: prod.foo.com
    when:
      event: tag

This would use the same secret for both steps:

--image=plugins/heroku HEROKU_SECRET foo

This would allow us to further refine and limit the secret individual steps. This solves the issue where we potentially want to use different secret values for different steps, even though the plugin image is the same:

--image=plugins/heroku --name=production  HEROKU_SECRET foo
--image=plugins/heroku --name=development HEROKU_SECRET bar

fwiw I don't feel extremely confident that our current secrets implementation is the best it could be. I definitely think it has improved since 0.4, but I think there is still room for improvement and we should therefore consider secrets to be a moving specification with potential for breaking changes in 0.6 and 0.7 releases.

[vaijab]

Understood. I believe this feature is a nice to have, but after using and supporting drone in a rather large scale environment, I have never seen anyone asking for this or something similar. Of course, that does not mean all users think the same way.

[tboerger]

I had this requirement already multiple times if the credentials are different. Currently I can only work around that with the mentioned environment block, but that's not really optimal.

[bradrydzewski]

Just throwing out an alternate proposal. There are some definite cons to the approach, but you could let the plugin configure the secret prefix. For example, the default docker plugin uses DOCKER_ prefix

docker:
  image: plugins/docker
  repo: foo/bar

We could configure an alternate prefix:

docker:
  image: plugins/docker
  repo: foo/bar
  secret_prefix: PROD

The above would instruct the plugin to look for PROD_DOCKER_* environment variables. This would delegate the problem to the plugin, which may not be ideal. But it is a solution that works today without adding more complexity to secrets.

My biggest concern, right now, is that everyone seems to be having difficult with secrets. This has me wondering if the current secret implementation, while powerful, might be too complex and might turn people away from the project ...

[tboerger]

That should be another solution but requires touching of every plugin where somebody needs different credentials

[tboerger]

Beside that it gets pretty complicated to add dynamic environment variables to the cli lib

[bradrydzewski]

@tboerger yes, true

I've been thinking about maybe simplifying secrets for 0.6, and combining aspects of the 0.4 and 0.5 implementation. I think we can keep the below commands for managing secrets from the command line:

drone secret add octocat/hello-world DOCKER_USERNAME foo
drone secret add octocat/hello-world DOCKER_PASSWORD bar
drone sign octocat/hello-world

We remove the --image flag

-drone secret add --image=plugins/docker octocat/hello-world DOCKER_USERNAME foo
+drone secret add octocat/hello-world DOCKER_USERNAME foo

We inject secrets into the Yaml similar to 0.4

pipeline:
  build:
    image: golang
    commands: [ go build, go test ]
  docker:
    image: plugins/docker
    repo: foo/bar
    username: ${DOCKER_USERNAME}
    password: ${DOCKER_PASSWORD}

I think we can do this is a way that supports backward compatibility to avoid breaking everyones build configurations with the change. We can address this in a more official proposal.

So why do I propose this change?

1) I have been discussing with @donny-dont and we both agree that the new secret implementation is too complex and results in end-user confusion and frustration, in addition to support burden.

2) The interpolation is also how docker-compose handles secrets, and parity with docker-compose is a project goal. This is also how people seem to expect secrets to work, although I'm not sure if it is because they are used to 0.4 or used to docker-compose, or both.

3) The existing implementation is also not flexible enough for using different secrets with the same name for different steps. The proposed fix makes complete sense, but also adds more complexity to something that is already complex. I don't think it is a problem with @tboerger's proposal, but with our underlying secrets implementation.

So this is why I'm thinking of simplifying a bit. We should also provide the ability to manage secrets in the user interface in order to improve the UX.

So what are the drawbacks to this combined approach?

• We lose the ability to only expose secrets to individual images (this is somewhat mitigated by signing secrets)
• We need to document how to inject multi-line secrets to avoid parsing errors. This was a source of confusion in 0.4. We could also recommend plugin authors request base64 encoded values for secrets that are multiple lines
• We are making changes when we need stability (this can be mitigated by deprecating --image but still supporting it going forward)

This is just an idea. I'm open to other proposals for simplification based on real world feedback. I'm interested if @donny-dont and @jmccann have seen their co-workers experiencing issues with secrets and if they had any thoughts for simplification.

I'm not going to do anything rash here, and won't break the existing feature, so don't worry :)

[jmccann]

I know for us some things that have been "hard" is:

• --image - people would constantly be "why aren't my secrets injected" and this would be a major cause ... either wrong name, not specified or wrong/no tag.
• Wanting to use different secrets in different steps for same plugin ... which is what this PR was originally attempting to address
• Matching secrets name to what plugin is looking for - what you propose should help alleviate that since they will be assigning the secret value to the plugin via the .drone.yml which then clearly shows how the secret is being consumed.

I think the proposal here make sense. I also think think it's NOT a drawback if we can no longer limit secrets to an image IF we are controlling where they are injected via the .drone.yml.

[jmccann]

Thinking about it a little more I'm curious how this would impact org and global secrets. I said in last comment:

I think the proposal here make sense. I also think think it's NOT a drawback if we can no longer limit secrets to an image IF we are controlling where they are injected via the .drone.yml.

But I think that statement may only be true for repo secrets and not org or global secrets. Not having the --image filter anymore may matter for org and global secrets. Like I may want to add a global secret only to be used for drone-s3-cache plugin and not want someone to try and expose it from another image.

[bradrydzewski]

@jmccann agreed. I think security and usability are in conflict here, unfortunately.

There is an open issue where I'm gathering requirements for introducing server and agent plugins. One of the use cases is for plugins to mutate the yaml prior to pipeline execution. This could be used to inject secrets using Go code, with much more control over when and where secrets are injected. Maybe this ends up being a better way to provide global or organization secrets?

---

EDIT

I should also point out that global and organization secrets, while more restricted than other CI systems, still have security gaps and require end-user trust. Consider a global Dockerhub username and password. A user with write access could change the Docker repository address to send authorization requests to a server that captures global credentials.

I think a plugin could provide much better security here. I think the negative is that it would be another service you need to deploy alongside drone. If you are using something like docker-compose this probably wouldn't be too onerous:

services:
  drone-agent:
    image: drone/drone:0.5
    command: agent
    environment:
      - DRONE_SERVER=ws://drone.company.com/ws/broker
      - DRONE_PLUGIN=tcp://drone-secret-injector-plugin

  drone-secret-injector-plugin:
    image: jmccann/drone-secret-injector
    environment:
      PLUGIN_CONFIG: |
        - match: plugins/docker|plugins/docker:(.+)
          environment:
            PLUGIN_USERNAME: foo
            PLUGIN_PASSWORD: bar
            PLUGIN_REGISTRY: registry.foo.com # forces registry so it can't be changed

[gtaylor]

I like --image conceptually, but we are probably going to see a lot of support load from this over here. We've got like five different teams using Drone, but not enough to be familiar with its innards.

[benschumacher]

Just to add my two cents, the --image filters do seem to cause unexpected issues. I liked the explicitness of Drone 0.4 of having an indication of where secrets will be used in the .drone.yml, keeping secrets in the repo did have issues. I haven't hit the precise issues that @tboerger indicated, but we have had to modify secret filters more than I'd like as we end up branching and tweaking plugin behavior to tune for our use cases.

[donny-dont]

I think the larger problem is that there is no UI currently which can be used to communicate what secrets are and how to use them. Everything is done through the CLI which is really power user territory.

I do really like --image but there are a bunch of gotchas with it. As an example if I want to do a secret to node I need to do --image=node --image=node:* --image=library/node --image=library/node:*. The same goes for --event. A UI could handle this case easier as well as inform the user of how to then use the secret.

[bradrydzewski]

I just patched master to use ${variable} syntax for interpolating secrets in the yaml similar to 0.5. This solves the issue by allowing individuals to use whatever secret names they want.

I also removed all reference to the old method of using secrets in the docs and plugin docs. The old method of providing secrets to plugins via environment variable still works, but is considered deprecated and will be removed in the future. This probably won't be for a few weeks, though. I'll post a note to drone-dev before doing so.