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

Assign secrets individually to each task #2735

Merged
merged 5 commits into from Oct 22, 2018

Conversation

Projects
None yet
6 participants
@sirlatrom
Copy link
Contributor

commented Aug 30, 2018

Signed-off-by: Sune Keller sune.keller@gmail.com

- What I did
Made every task get its own copy of a secret such that it can be different for each task in case a secrets plugin driver provides different values per task. The use case is to install a secrets plugin driver that fetches a unique value for every task from HashiCorp Vault, e.g. a set of database credentials, or an opaque token. The preliminary plugin code is available here: https://gitlab.com/sirlatrom/docker-secretprovider-plugin-vault.

- How I did it
I made the following changes:

  1. I added a new func CombineTwoIDs() in identity/combined_id.go that combines two existing IDs into a third by concatenating the IDs, separated by a ..

  2. While assigning a task to a node and considering its secret references, it is now checked whether the secret with the referred ID has been added as a dependency for the task in question as opposed to whether it has been added as a dependency for the entire assignment. Previously, this was done once per secret ID in an entire assignment, which meant the value for any given secret ID would only ever be fetched once during an assignment.

  3. Iff any secret driver is used, and the driver returns DoNotReuse: true in the SecretsProviderResponse, the secret is given a new ID, namely the concatenation of the secret ID and the task ID separated by a ., and marked as internal, resulting in a "secret update" change added to the assignment per task rather than per secret.

  4. At the time when a secret is retrieved for a task, the secret ID and the task ID are combined and the secret with the resulting ID is looked up. If it does not exist, the original secret ID is looked up, as per default behavior.

An improvement would be to allow the driver/plugin to decide this behaviour rather than always doing it for all drivers, but currently, the Options field is never filled out for a secret driver object, as it is created when the secret spec is created, and I cannot find any mechanism in swarmkit for looking up information about a given secret driver plugin, which is installed in the Engine. Another approach would be to reserve a special secret label for the purpose of deciding this behaviour, e.g. 'com.docker.swarmkit.secret.driver.per-task=true`, but that would leave the burden on the user to know which types of secrets in the plugin would need that label.

- How to test it
Install the plugin, run a Vault dev server, verify that each task gets its own token when the plugin's vault_token mode is used on a secret:

plugin_name=sirlatrom/docker-secretprovider-plugin-vault
docker container run --detach --name vault --publish 8200:8200 vault server -dev -dev-root-token-id=1234
sleep 1
docker container exec -i --env VAULT_ADDR=http://127.0.0.1:8200 --env VAULT_TOKEN=1234 vault vault policy write snitch -<<EOF
path "secret/data/*" {
  capabilities = ["create","read","update"]
}
path "auth/token/create" {
  capabilities = ["create", "update"]
}
EOF
echo -n '1234' | docker secret create secret-zero -
docker service create --mode global --constraint 'node.role == manager' --name vault-helper-service --secret secret-zero --restart-condition on-failure busybox tail -f /dev/null
docker plugin install --grant-all-permissions ${plugin_name}
docker secret create --driver ${plugin_name} --label "dk.almbrand.docker.plugin.secretprovider.vault.type"="vault_token" generic_vault_token
docker node ls --filter role=worker -q | wc -l | grep -q 0 && snitch_role=manager || snitch_role=worker
docker service create --constraint 'node.role == '$snitch_role --detach --name snitch --replicas 2 --env VAULT_ADDR=http://172.17.0.1:8200 --secret generic_vault_token vault sh -c 'echo -n "generic_vault_token: "; cat /run/secrets/generic_vault_token; echo'
docker service logs -f snitch

The result should be that each task of the snitch service outputs a different value for the generic_vault_token secret, e.g.:

snitch.1.mrqrnv6cu7b9@redacted      | generic_vault_token: 17275db7-c139-a10d-e40c-847a9074345b
snitch.2.ok18z0rqw01c@redacted      | generic_vault_token: e1b24367-81cf-a86e-279c-f4187be6c820
snitch.2.usunxsf926kh@redacted      | generic_vault_token: 00b42ad2-d75c-24f3-38ca-f7ad8177201a
snitch.1.vc5hq4yoqqiv@redacted      | generic_vault_token: 4458b0e2-f1fd-bde7-36aa-da3d15e8d7e0
snitch.2.qygl3f1jrvqz@redacted      | generic_vault_token: 646bcac2-1e88-afe3-fdc4-ceadfc22e33e
snitch.1.nnwqfl0pa0x4@redacted      | generic_vault_token: 838ad7f2-793c-4727-73c6-4abd38ec5757

- Description for the changelog

Assign individual secret values to each task in a service when a secrets driver plugin indicates the secret is not to be reused.

- Cute animal
2018-09-06_08-25-39

@sirlatrom

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2018

Ping @dperny

@sirlatrom sirlatrom force-pushed the sirlatrom:one-secret-per-task branch from d8b74a9 to 1ed1e8d Sep 4, 2018

@codecov

This comment has been minimized.

Copy link

commented Sep 4, 2018

Codecov Report

Merging #2735 into master will decrease coverage by 0.12%.
The diff coverage is 93.1%.

@@            Coverage Diff             @@
##           master    #2735      +/-   ##
==========================================
- Coverage   61.85%   61.72%   -0.13%     
==========================================
  Files         134      136       +2     
  Lines       21876    21907      +31     
==========================================
- Hits        13532    13523       -9     
- Misses       6882     6918      +36     
- Partials     1462     1466       +4

@sirlatrom sirlatrom force-pushed the sirlatrom:one-secret-per-task branch 2 times, most recently from fe4b50c to 32a084b Sep 4, 2018

@sirlatrom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

I've updated the description with the approach I settled on, which works with passing CI.

@dperny PTAL

Show resolved Hide resolved manager/dispatcher/assignments.go Outdated
@dperny

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

To address my comment (and also yours about options being only in the spec) we may need to add fields to the protos to accommodate the desired behavior.

@sirlatrom sirlatrom force-pushed the sirlatrom:one-secret-per-task branch 4 times, most recently from 6b92120 to 9a511db Sep 6, 2018

@sirlatrom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

@dperny I've made changes as per your feedback by adding a SecretMappings field to the Task proto. I updated the PR description to match this.

@sirlatrom sirlatrom force-pushed the sirlatrom:one-secret-per-task branch 2 times, most recently from bd6fc3b to 5efe9ef Sep 10, 2018

@sirlatrom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

@justincormack Any chance you could weigh in here?

@sirlatrom sirlatrom force-pushed the sirlatrom:one-secret-per-task branch 3 times, most recently from b1850e3 to e26502d Sep 12, 2018

@sirlatrom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2018

@dperny The changes made should satisfy the requested changes. Is there any path forward for this PR? CC: @justincormack

@dperny

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

Sorry, @sirlatrom. I have too many things going on concurrently right now and things are slipping, including a review of this. I'm reviewing right now.

secretIDs map[string]struct{} // allow list of secret ids
secrets exec.SecretGetter
secretIDs map[string]struct{} // allow list of secret ids
secretMappings map[string]string // for mapping referenced secret id to an internal secret

This comment has been minimized.

Copy link
@dperny

dperny Sep 20, 2018

Member

is the information in secretMappings not a superset of the information in secretIDs? or does secretIDs also include the ephemeral task-specific secrets?

This comment has been minimized.

Copy link
@sirlatrom

sirlatrom Sep 20, 2018

Author Contributor

The information in secretMappings only overlaps with secretIDs, as in all the keys of secretMappings are present in secretIDs, but not all the keys of secretIDs are necessarily present in secretMappings. Besides, the values in secretMappings are only present in that map, as secretIDs is a map[string]struct{}.

@dperny

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

I feel like this has been mentioned before, but I cannot recall what the answer is... is there a reason we can't remap secrets in a way that is transparent for the task? For example, for secrets that are task-specific, using a key of secret ID and task ID together to pick the real secret information?

@sirlatrom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2018

I feel like this has been mentioned before, but I cannot recall what the answer is... is there a reason we can't remap secrets in a way that is transparent for the task? For example, for secrets that are task-specific, using a key of secret ID and task ID together to pick the real secret information?

I'm very much in favor of such an ID, as long as it does not invalidate any constraints there may be on ID lengths or anything like that. I presume having an ID of <secretID>.<taskID> would make troubleshooting the local store contents easier as compared to the new random ID. I'll make the change shortly.

@dperny

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

I have this gut feeling that adding a secrets_mapping to the Task object is a bad idea, but I'm unsure how to articulate it.

However, after discussing out-of-band with @sirlatrom, we have the kernel of an idea that might work in lieu of that.

Basically, the rough idea is that you would leave the task object totally alone, and push all the changes off onto the Secret object and the things that manage its lifecycle.

When a Task using a secret that is task-specific (still unclear how we would indicate this), instead of assigning the secret directly, the Dispatcher get the secret value from the Driver, and then mints a new api.Secret object. The ID of this object would be the Task ID somehow combined (xor'd, maybe? concatenating is also a possibility, but I like it less) with the secret ID. This new secret created solely service this one task is ephemeral; it never gets written out to disk, and is basically totally gone from the manager as soon as it's been sent down as an assignment. It is, consequentially, opaque to the user. The only place the user can ever see the actual value of the secret is in the task itself.

When the executor goes to retrieve the Secret for a Task, it goes through a SecretGetter interface, which is implemented in agent/secrets/secrets.go as a taskRestrictedSecretsProvider. We'd alter the functionality of taskRestrictedSecretsProvider to do something like this:

// taskRestrictedSecretsProvider restricts the ids to the task.
type taskRestrictedSecretsProvider struct {
	secrets   exec.SecretGetter
	secretIDs map[string]struct{} // allow list of secret ids
        taskID    string
}

func (sp *taskRestrictedSecretsProvider) Get(secretID string) (*api.Secret, error) {
        if _, ok := sp.secretIDs[secretID]; !ok {
                return nil, fmt.Errorf("task not authorized to access secret %s", secretID)
        }

        // we'll first try getting a secret that is the result of combining the secret and task ID
        if secret, err := sp.secrets.Get(SomeIDXorFunc(secretID, sp.taskID)); err != nil {
                // if that fails, the secret must not be task-specific, so we get just the secret ID
                return sp.secrets.Get(secretID)
        }
        // then, we alter the secret ID, re-write it so the task never knows it's dealing 
        // with a "special" secret. In practice, this might involve copying the object so
        // we don't alter its value in the local secret store
        secret.ID = secretID
        return secret, err
}

Another option may be to set a flag on the secret spec of the "real" secret that says it's a task-specific secret, and then try getting by raw secret ID first, returning an error if the secret is one that is task specific, and trying again for the task-specific version in the case that we get that error.

The over-writing of the secret ID should be fine; once the secret leaves the local secret store, it's no longer really any of swarmkit's concern. It should only be used for bootstrapping the container, so if there are multiple secret objects floating around with the same ID but different data, they should never actually come into conflict with each other.

This approach gives us one really big benefit, in my opinion: it removes all visibility of the implementation of task-specific secrets from the user's view. This means that if this architecture turns out to be inflexible, insecure, or otherwise bad, we can change it substantially without worrying about breaking users.

I'm not sure what the correct answer is. This is certainly a more complicated approach to things. Additionally, I'm unsure about the feasibility of combining IDs like this, both from a usability and security perspective.

I'd appreciate a weigh-in from someone who has more experience with this kind of architecture. I'd be happy to write up a more detailed description of the secrets lifecycle, if needed.

@sirlatrom sirlatrom force-pushed the sirlatrom:one-secret-per-task branch 4 times, most recently from 9801ffc to 052b785 Sep 21, 2018

Support individual secrets per task
Signed-off-by: Sune Keller <absukl@almbrand.dk>

@sirlatrom sirlatrom force-pushed the sirlatrom:one-secret-per-task branch from 3b29114 to 1ce2840 Oct 9, 2018

@GordonTheTurtle GordonTheTurtle removed the dco/no label Oct 9, 2018

@sirlatrom

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

Its probably ok to squash everything into a single commit @sirlatrom

OK, I've squashed it into a single commit . Awaiting flaky CI or, if I'm lucky, a review :) and rebased on master.

@anshulpundir
Copy link
Contributor

left a comment

Looks good, minor comments.

Show resolved Hide resolved agent/secrets/secrets.go Outdated
Show resolved Hide resolved identity/randomid.go Outdated
Show resolved Hide resolved identity/randomid.go Outdated
Show resolved Hide resolved manager/dispatcher/assignments.go
Show resolved Hide resolved manager/dispatcher/assignments.go
Show resolved Hide resolved manager/dispatcher/assignments.go
Show resolved Hide resolved manager/drivers/secrets.go
@justincormack

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

This xoring IDs is really gross. You should not xor strings, thats a type failure. If the ids are random IDs they should be stored as 256 bits in fixed length byte arrays, and then maybe xor is ok (although there are perhaps better options).

@sirlatrom

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

This xoring IDs is really gross. You should not xor strings, thats a type failure. If the ids are random IDs they should be stored as 256 bits in fixed length byte arrays, and then maybe xor is ok (although there are perhaps better options).

My initial take was to append the task ID to the secret ID (separated by a dot) in order to generate a task-specific ID.

My second take was to generate new IDs entirely and add a secrets mapping field to the Secret proto to hold a map from the original IDs present in the spec to the new IDs present in the assignment.

@justincormack Are any of those takes more preferable, or is there a fourth one that would be better?

@justincormack

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

I think appending with a dot is most readable unless there is any reason not to do so (I can't think of one).

Address review comments from @anshulpundir
Signed-off-by: Sune Keller <absukl@almbrand.dk>

@sirlatrom sirlatrom force-pushed the sirlatrom:one-secret-per-task branch from bacc101 to 554ddbd Oct 11, 2018

Addess PR comment by @justincormack
Signed-off-by: Sune Keller <absukl@almbrand.dk>
@sirlatrom

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

I think appending with a dot is most readable unless there is any reason not to do so (I can't think of one).

@justincormack I just added 7b98e00, which implements that. The only thing I can think of, which speaks against appending the task ID to the secret ID, is that the length of the ID string will double. But does that matter?

@dperny @anshulpundir Any pros/cons to add?

@sirlatrom

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

I would argue the CI failure for 554ddbd is not related to this PR.

@wk8

wk8 approved these changes Oct 11, 2018

Copy link
Member

left a comment

Clever hack... Just a minor nitpick :)

Show resolved Hide resolved agent/secrets/secrets.go
Address review comment by @wk8
Signed-off-by: Sune Keller <absukl@almbrand.dk>
@sirlatrom

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

I've updated the PR description to fit with the recent changes.

@sirlatrom sirlatrom force-pushed the sirlatrom:one-secret-per-task branch 2 times, most recently from e5cb39e to 2fcd8ad Oct 17, 2018

@sirlatrom

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Looks like flaky CI (2fcd8ad), I just wanted to add a comment explaining why I changed the check for task dependencies...

Add comment on difference in dependency map check
Signed-off-by: Sune Keller <absukl@almbrand.dk>

@sirlatrom sirlatrom force-pushed the sirlatrom:one-secret-per-task branch from 2fcd8ad to fc555b2 Oct 17, 2018

@dperny dperny merged commit 7b75232 into docker:master Oct 22, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 61.72% (target 0%)
Details
dco-signed All commits are signed

@sirlatrom sirlatrom deleted the sirlatrom:one-secret-per-task branch Oct 22, 2018

thaJeztah added a commit to thaJeztah/docker that referenced this pull request Nov 1, 2018

Bump SwarmKit to 8d8689d5a94ac42406883a4cef89b3a5eaec3d11
Changes included;

- docker/swarmkit#2735 Assign secrets individually to each task
- docker/swarmkit#2759 Adding a new `Deallocator` component
- docker/swarmkit#2738 Add additional info for secret drivers
- docker/swarmkit#2775 Increase grpc max recv message size
  - addresses moby#37941
  - addresses moby#37997
  - follow-up to moby#38103

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

docker-jenkins pushed a commit to docker/docker-ce that referenced this pull request Nov 12, 2018

Bump SwarmKit to 8d8689d5a94ac42406883a4cef89b3a5eaec3d11
Changes included;

- docker/swarmkit#2735 Assign secrets individually to each task
- docker/swarmkit#2759 Adding a new `Deallocator` component
- docker/swarmkit#2738 Add additional info for secret drivers
- docker/swarmkit#2775 Increase grpc max recv message size
  - addresses moby/moby#37941
  - addresses moby/moby#37997
  - follow-up to moby/moby#38103

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: be3843c8c8fb30b4a604dae9d0dad3d393db717c
Component: engine

antonmoeriksson added a commit to antonmoeriksson/moby that referenced this pull request Dec 12, 2018

Bump SwarmKit to 8d8689d5a94ac42406883a4cef89b3a5eaec3d11
Changes included;

- docker/swarmkit#2735 Assign secrets individually to each task
- docker/swarmkit#2759 Adding a new `Deallocator` component
- docker/swarmkit#2738 Add additional info for secret drivers
- docker/swarmkit#2775 Increase grpc max recv message size
  - addresses moby#37941
  - addresses moby#37997
  - follow-up to moby#38103

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

adhulipa added a commit to adhulipa/docker that referenced this pull request Apr 11, 2019

Bump SwarmKit to 8d8689d5a94ac42406883a4cef89b3a5eaec3d11
Changes included;

- docker/swarmkit#2735 Assign secrets individually to each task
- docker/swarmkit#2759 Adding a new `Deallocator` component
- docker/swarmkit#2738 Add additional info for secret drivers
- docker/swarmkit#2775 Increase grpc max recv message size
  - addresses moby#37941
  - addresses moby#37997
  - follow-up to moby#38103

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.