-
Notifications
You must be signed in to change notification settings - Fork 612
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
Conversation
Ping @dperny |
d8b74a9
to
1ed1e8d
Compare
Codecov Report
@@ 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 |
fe4b50c
to
32a084b
Compare
I've updated the description with the approach I settled on, which works with passing CI. @dperny PTAL |
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. |
6b92120
to
9a511db
Compare
@dperny I've made changes as per your feedback by adding a |
bd6fc3b
to
5efe9ef
Compare
@justincormack Any chance you could weigh in here? |
b1850e3
to
e26502d
Compare
@dperny The changes made should satisfy the requested changes. Is there any path forward for this PR? CC: @justincormack |
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. |
agent/secrets/secrets.go
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the information in secretMappings
not a superset of the information in secretIDs
? or does secretIDs
also include the ephemeral task-specific secrets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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{}
.
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 |
I have this gut feeling that adding a 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 When the executor goes to retrieve the // 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. |
9801ffc
to
052b785
Compare
Signed-off-by: Sune Keller <absukl@almbrand.dk>
3b29114
to
1ce2840
Compare
OK, I've squashed it into a single commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, minor comments.
This |
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 @justincormack Are any of those takes more preferable, or is there a fourth one that would be better? |
I think appending with a dot is most readable unless there is any reason not to do so (I can't think of one). |
Signed-off-by: Sune Keller <absukl@almbrand.dk>
bacc101
to
554ddbd
Compare
Signed-off-by: Sune Keller <absukl@almbrand.dk>
@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? |
I would argue the CI failure for 554ddbd is not related to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever hack... Just a minor nitpick :)
Signed-off-by: Sune Keller <absukl@almbrand.dk>
I've updated the PR description to fit with the recent changes. |
e5cb39e
to
2fcd8ad
Compare
Looks like flaky CI (2fcd8ad), I just wanted to add a comment explaining why I changed the check for task dependencies... |
Signed-off-by: Sune Keller <absukl@almbrand.dk>
2fcd8ad
to
fc555b2
Compare
Changes included; - moby/swarmkit#2735 Assign secrets individually to each task - moby/swarmkit#2759 Adding a new `Deallocator` component - moby/swarmkit#2738 Add additional info for secret drivers - moby/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>
Changes included; - moby/swarmkit#2735 Assign secrets individually to each task - moby/swarmkit#2759 Adding a new `Deallocator` component - moby/swarmkit#2738 Add additional info for secret drivers - moby/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
Changes included; - moby/swarmkit#2735 Assign secrets individually to each task - moby/swarmkit#2759 Adding a new `Deallocator` component - moby/swarmkit#2738 Add additional info for secret drivers - moby/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>
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:
I added a new func
CombineTwoIDs()
inidentity/combined_id.go
that combines two existing IDs into a third by concatenating the IDs, separated by a.
.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.
Iff any secret driver is used, and the driver returns
DoNotReuse: true
in theSecretsProviderResponse
, 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.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:The result should be that each task of the
snitch
service outputs a different value for thegeneric_vault_token
secret, e.g.:- 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