diff --git a/agent/secrets/secrets.go b/agent/secrets/secrets.go index 233101d0fb..fb0ee1d3b9 100644 --- a/agent/secrets/secrets.go +++ b/agent/secrets/secrets.go @@ -6,6 +6,7 @@ import ( "github.com/docker/swarmkit/agent/exec" "github.com/docker/swarmkit/api" + "github.com/docker/swarmkit/identity" ) // secrets is a map that keeps all the currently available secrets to the agent @@ -62,6 +63,7 @@ func (s *secrets) Reset() { type taskRestrictedSecretsProvider struct { secrets exec.SecretGetter secretIDs map[string]struct{} // allow list of secret ids + taskID string // ID of the task the provider restricts for } func (sp *taskRestrictedSecretsProvider) Get(secretID string) (*api.Secret, error) { @@ -69,7 +71,18 @@ func (sp *taskRestrictedSecretsProvider) Get(secretID string) (*api.Secret, erro return nil, fmt.Errorf("task not authorized to access secret %s", secretID) } - return sp.secrets.Get(secretID) + // First check if the secret is available with the task specific ID, which is the concatenation + // of the original secret ID and the task ID with a dot in between. + // That is the case when a secret driver has returned DoNotReuse == true for a secret value. + taskSpecificID := identity.CombineTwoIDs(secretID, sp.taskID) + secret, err := sp.secrets.Get(taskSpecificID) + if err != nil { + // Otherwise, which is the default case, the secret is retrieved by its original ID. + return sp.secrets.Get(secretID) + } + // For all intents and purposes, the rest of the flow should deal with the original secret ID. + secret.ID = secretID + return secret, err } // Restrict provides a getter that only allows access to the secrets @@ -84,5 +97,5 @@ func Restrict(secrets exec.SecretGetter, t *api.Task) exec.SecretGetter { } } - return &taskRestrictedSecretsProvider{secrets: secrets, secretIDs: sids} + return &taskRestrictedSecretsProvider{secrets: secrets, secretIDs: sids, taskID: t.ID} } diff --git a/agent/secrets/secrets_test.go b/agent/secrets/secrets_test.go new file mode 100644 index 0000000000..e15fffb8d2 --- /dev/null +++ b/agent/secrets/secrets_test.go @@ -0,0 +1,113 @@ +package secrets + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/docker/swarmkit/agent/exec" + "github.com/docker/swarmkit/api" + "github.com/docker/swarmkit/identity" + "github.com/stretchr/testify/assert" +) + +func TestTaskRestrictedSecretsProvider(t *testing.T) { + type testCase struct { + desc string + secretIDs map[string]struct{} + secrets exec.SecretGetter + secretID string + taskID string + secretIDToGet string + value string + expected string + expectedErr string + } + + originalSecretID := identity.NewID() + taskID := identity.NewID() + taskSpecificID := fmt.Sprintf("%s.%s", originalSecretID, taskID) + + testCases := []testCase{ + // The default case when not using a secrets driver or not returning + // DoNotReuse: true in the SecretsProviderResponse. + { + desc: "Test getting secret by original ID when restricted by task", + value: "value", + expected: "value", + secretIDs: map[string]struct{}{ + originalSecretID: {}, + }, + // Simulates inserting a secret returned by a driver which sets the + // DoNotReuse flag to false. + secretID: originalSecretID, + // Internal API calls would request to get the secret by the + // original ID. + secretIDToGet: originalSecretID, + taskID: taskID, + }, + // The case for when a secrets driver returns DoNotReuse: true in the + // SecretsProviderResponse. + { + desc: "Test getting secret by task specific ID when restricted by task", + value: "value", + expected: "value", + secretIDs: map[string]struct{}{ + originalSecretID: {}, + }, + // Simulates inserting a secret returned by a driver which sets the + // DoNotReuse flag to true. This would result in the assignment + // containing a secret with the ID set to the cibcatebatuib of the + // secret and task IDs separated by a dot. + secretID: taskSpecificID, + // Internal API calls would still request to get the secret by the + // original ID. + secretIDToGet: originalSecretID, + taskID: taskID, + }, + // This case should catch regressions in the logic coupling of the ID + // given to secrets in assignments and the corresponding retrieval of + // the same secrets. If a secret can be got by the task specific ID + // without it being added as such in an assignment, something has been + // changed inconsistently. + { + desc: "Test attempting to get a secret by task specific ID when secret is added with original ID", + value: "value", + expectedErr: fmt.Sprintf("task not authorized to access secret %s", taskSpecificID), + secretIDs: map[string]struct{}{ + originalSecretID: {}, + }, + secretID: originalSecretID, + secretIDToGet: taskSpecificID, + taskID: taskID, + }, + } + secretsManager := NewManager() + for _, testCase := range testCases { + t.Logf("secretID=%s, taskID=%s, taskSpecificID=%s", originalSecretID, taskID, taskSpecificID) + secretsManager.Add(api.Secret{ + ID: testCase.secretID, + Spec: api.SecretSpec{ + Data: []byte(testCase.value), + }, + }) + secretsGetter := Restrict(secretsManager, &api.Task{ + ID: taskID, + }) + (secretsGetter.(*taskRestrictedSecretsProvider)).secretIDs = testCase.secretIDs + secret, err := secretsGetter.Get(testCase.secretIDToGet) + if testCase.expectedErr != "" { + assert.Error(t, err, testCase.desc) + assert.Equal(t, testCase.expectedErr, err.Error(), testCase.desc) + } else { + t.Logf("secretIDs=%v", testCase.secretIDs) + assert.NoError(t, err, testCase.desc) + require.NotNil(t, secret, testCase.desc) + require.NotNil(t, secret.Spec, testCase.desc) + require.NotNil(t, secret.Spec.Data, testCase.desc) + assert.Equal(t, testCase.expected, string(secret.Spec.Data), testCase.desc) + } + secretsManager.Reset() + } +} diff --git a/api/api.pb.txt b/api/api.pb.txt index 0709671931..d0964ad5a2 100755 --- a/api/api.pb.txt +++ b/api/api.pb.txt @@ -787,6 +787,20 @@ file { type: TYPE_STRING json_name: "phpNamespace" } + field { + name: "php_metadata_namespace" + number: 44 + label: LABEL_OPTIONAL + type: TYPE_STRING + json_name: "phpMetadataNamespace" + } + field { + name: "ruby_package" + number: 45 + label: LABEL_OPTIONAL + type: TYPE_STRING + json_name: "rubyPackage" + } field { name: "uninterpreted_option" number: 999 diff --git a/identity/combined_id.go b/identity/combined_id.go new file mode 100644 index 0000000000..2c4a292761 --- /dev/null +++ b/identity/combined_id.go @@ -0,0 +1,8 @@ +package identity + +import "fmt" + +// CombineTwoIDs combines the given IDs into a new ID, e.g. a secret and a task ID. +func CombineTwoIDs(id1, id2 string) string { + return fmt.Sprintf("%s.%s", id1, id2) +} diff --git a/identity/combined_id_test.go b/identity/combined_id_test.go new file mode 100644 index 0000000000..74ea76d6cf --- /dev/null +++ b/identity/combined_id_test.go @@ -0,0 +1,19 @@ +package identity + +import ( + "fmt" + "math/rand" + "testing" +) + +func TestCombineTwoIDs(t *testing.T) { + idReader = rand.New(rand.NewSource(0)) + id1 := NewID() + id2 := NewID() + combinedID := CombineTwoIDs(id1, id2) + + expected := fmt.Sprintf("%s.%s", id1, id2) + if combinedID != expected { + t.Fatalf("%s != %s", combinedID, expected) + } +} diff --git a/manager/dispatcher/assignments.go b/manager/dispatcher/assignments.go index 5a56348053..101c449edd 100644 --- a/manager/dispatcher/assignments.go +++ b/manager/dispatcher/assignments.go @@ -6,6 +6,7 @@ import ( "github.com/docker/swarmkit/api" "github.com/docker/swarmkit/api/equality" "github.com/docker/swarmkit/api/validation" + "github.com/docker/swarmkit/identity" "github.com/docker/swarmkit/manager/drivers" "github.com/docker/swarmkit/manager/state/store" "github.com/sirupsen/logrus" @@ -35,8 +36,10 @@ func newAssignmentSet(log *logrus.Entry, dp *drivers.DriverProvider) *assignment } func assignSecret(a *assignmentSet, readTx store.ReadTx, mapKey typeAndID, t *api.Task) { - a.tasksUsingDependency[mapKey] = make(map[string]struct{}) - secret, err := a.secret(readTx, t, mapKey.id) + if _, exists := a.tasksUsingDependency[mapKey]; !exists { + a.tasksUsingDependency[mapKey] = make(map[string]struct{}) + } + secret, doNotReuse, err := a.secret(readTx, t, mapKey.id) if err != nil { a.log.WithFields(logrus.Fields{ "resource.type": "secret", @@ -45,6 +48,19 @@ func assignSecret(a *assignmentSet, readTx store.ReadTx, mapKey typeAndID, t *ap }).Debug("failed to fetch secret") return } + // If the secret should not be reused for other tasks, give it a unique ID for the task to allow different values for different tasks. + if doNotReuse { + // Give the secret a new ID and mark it as internal + originalSecretID := secret.ID + taskSpecificID := identity.CombineTwoIDs(originalSecretID, t.ID) + secret.ID = taskSpecificID + secret.Internal = true + // Create a new mapKey with the new ID and insert it into the dependencies map for the task. + // This will make the changes map contain an entry with the new ID rather than the original one. + mapKey = typeAndID{objType: mapKey.objType, id: secret.ID} + a.tasksUsingDependency[mapKey] = make(map[string]struct{}) + a.tasksUsingDependency[mapKey][t.ID] = struct{}{} + } a.changes[mapKey] = &api.AssignmentChange{ Assignment: &api.Assignment{ Item: &api.Assignment_Secret{ @@ -104,7 +120,12 @@ func (a *assignmentSet) addTaskDependencies(readTx store.ReadTx, t *api.Task) { secretID := secretRef.SecretID mapKey := typeAndID{objType: api.ResourceType_SECRET, id: secretID} - if len(a.tasksUsingDependency[mapKey]) == 0 { + // This checks for the presence of each task in the dependency map for the + // secret. This is currently only done for secrets since the other types of + // dependencies do not support driver plugins. Arguably, the same task would + // not have the same secret as a dependency more than once, but this check + // makes sure the task only gets the secret assigned once. + if _, exists := a.tasksUsingDependency[mapKey][t.ID]; !exists { assignSecret(a, readTx, mapKey, t) } a.tasksUsingDependency[mapKey][t.ID] = struct{}{} @@ -290,27 +311,29 @@ func (a *assignmentSet) message() api.AssignmentsMessage { } // secret populates the secret value from raft store. For external secrets, the value is populated -// from the secret driver. -func (a *assignmentSet) secret(readTx store.ReadTx, task *api.Task, secretID string) (*api.Secret, error) { +// from the secret driver. The function returns: a secret object; an indication of whether the value +// is to be reused across tasks; and an error if the secret is not found in the store, if the secret +// driver responds with one or if the payload does not pass validation. +func (a *assignmentSet) secret(readTx store.ReadTx, task *api.Task, secretID string) (*api.Secret, bool, error) { secret := store.GetSecret(readTx, secretID) if secret == nil { - return nil, fmt.Errorf("secret not found") + return nil, false, fmt.Errorf("secret not found") } if secret.Spec.Driver == nil { - return secret, nil + return secret, false, nil } d, err := a.dp.NewSecretDriver(secret.Spec.Driver) if err != nil { - return nil, err + return nil, false, err } - value, err := d.Get(&secret.Spec, task) + value, doNotReuse, err := d.Get(&secret.Spec, task) if err != nil { - return nil, err + return nil, false, err } if err := validation.ValidateSecretPayload(value); err != nil { - return nil, err + return nil, false, err } // Assign the secret secret.Spec.Data = value - return secret, nil + return secret, doNotReuse, nil } diff --git a/manager/dispatcher/dispatcher_test.go b/manager/dispatcher/dispatcher_test.go index e62baa8052..718e5ce401 100644 --- a/manager/dispatcher/dispatcher_test.go +++ b/manager/dispatcher/dispatcher_test.go @@ -420,13 +420,16 @@ func TestAssignmentsSecretDriver(t *testing.T) { t.Parallel() const ( - secretDriver = "secret-driver" - existingSecretName = "existing-secret" - serviceName = "service-name" - serviceHostname = "service-hostname" - serviceEndpointMode = 2 + secretDriver = "secret-driver" + existingSecretName = "existing-secret" + doNotReuseExistingSecretName = "do-not-reuse-existing-secret" + errSecretName = "err-secret" + serviceName = "service-name" + serviceHostname = "service-hostname" + serviceEndpointMode = 2 ) secretValue := []byte("custom-secret-value") + doNotReuseSecretValue := []byte("custom-do-not-reuse-secret-value") serviceLabels := map[string]string{ "label-name": "label-value", } @@ -434,7 +437,9 @@ func TestAssignmentsSecretDriver(t *testing.T) { portConfig := drivers.PortConfig{Name: "port", PublishMode: 5, TargetPort: 80, Protocol: 10, PublishedPort: 8080} responses := map[string]*drivers.SecretsProviderResponse{ - existingSecretName: {Value: secretValue}, + existingSecretName: {Value: secretValue}, + doNotReuseExistingSecretName: {Value: doNotReuseSecretValue, DoNotReuse: true}, + errSecretName: {Err: "Error from driver"}, } mux := http.NewServeMux() @@ -471,13 +476,27 @@ func TestAssignmentsSecretDriver(t *testing.T) { Driver: &api.Driver{Name: secretDriver}, }, } + doNotReuseSecret := &api.Secret{ + ID: "driverDoNotReuseSecret", + Spec: api.SecretSpec{ + Annotations: api.Annotations{Name: doNotReuseExistingSecretName}, + Driver: &api.Driver{Name: secretDriver}, + }, + } + errSecret := &api.Secret{ + ID: "driverErrSecret", + Spec: api.SecretSpec{ + Annotations: api.Annotations{Name: errSecretName}, + Driver: &api.Driver{Name: secretDriver}, + }, + } config := &api.Config{ ID: "config", Spec: api.ConfigSpec{ Data: []byte("config"), }, } - spec := taskSpecFromDependencies(secret, config) + spec := taskSpecFromDependencies(secret, doNotReuseSecret, errSecret, config) spec.GetContainer().Hostname = serviceHostname task := &api.Task{ NodeID: nodeID, @@ -507,6 +526,8 @@ func TestAssignmentsSecretDriver(t *testing.T) { err = gd.Store.Update(func(tx store.Tx) error { assert.NoError(t, store.CreateSecret(tx, secret)) + assert.NoError(t, store.CreateSecret(tx, doNotReuseSecret)) + assert.NoError(t, store.CreateSecret(tx, errSecret)) assert.NoError(t, store.CreateConfig(tx, config)) assert.NoError(t, store.CreateTask(tx, task)) return nil @@ -521,9 +542,17 @@ func TestAssignmentsSecretDriver(t *testing.T) { assert.NoError(t, err) _, _, secretChanges := splitChanges(resp.Changes) - assert.Len(t, secretChanges, 1) + assert.Len(t, secretChanges, 2) for _, s := range secretChanges { - assert.Equal(t, secretValue, s.Spec.Data) + if s.ID == "driverSecret" { + assert.Equal(t, secretValue, s.Spec.Data) + } else if s.ID == "driverDoNotReuseSecret" { + assert.Fail(t, "Secret with DoNotReuse==true should not retain its original ID in the assignment", "%s != %s", "driverDoNotReuseSecret", s.ID) + } else { + taskSpecificID := fmt.Sprintf("%s.%s", "driverDoNotReuseSecret", task.ID) + assert.Equal(t, taskSpecificID, s.ID) + assert.Equal(t, doNotReuseSecretValue, s.Spec.Data) + } } } diff --git a/manager/drivers/secrets.go b/manager/drivers/secrets.go index 2e7bc392b6..a9bdaa46a6 100644 --- a/manager/drivers/secrets.go +++ b/manager/drivers/secrets.go @@ -25,13 +25,16 @@ func NewSecretDriver(plugin plugingetter.CompatPlugin) *SecretDriver { return &SecretDriver{plugin: plugin} } -// Get gets a secret from the secret provider -func (d *SecretDriver) Get(spec *api.SecretSpec, task *api.Task) ([]byte, error) { +// Get gets a secret from the secret provider. The function returns: the secret value; +// a bool indicating whether the value should be reused across different tasks (defaults to false); +// and an error if either the spec or task are nil, if calling the driver returns an error, or if +// the driver returns an error in the payload. +func (d *SecretDriver) Get(spec *api.SecretSpec, task *api.Task) ([]byte, bool, error) { if spec == nil { - return nil, fmt.Errorf("secret spec is nil") + return nil, false, fmt.Errorf("secret spec is nil") } if task == nil { - return nil, fmt.Errorf("task is nil") + return nil, false, fmt.Errorf("task is nil") } var secretResp SecretsProviderResponse @@ -67,13 +70,13 @@ func (d *SecretDriver) Get(spec *api.SecretSpec, task *api.Task) ([]byte, error) err := d.plugin.Client().Call(SecretsProviderAPI, secretReq, &secretResp) if err != nil { - return nil, err + return nil, false, err } if secretResp.Err != "" { - return nil, fmt.Errorf(secretResp.Err) + return nil, secretResp.DoNotReuse, fmt.Errorf(secretResp.Err) } // Assign the secret value - return secretResp.Value, nil + return secretResp.Value, secretResp.DoNotReuse, nil } // SecretsProviderRequest is the secrets provider request. @@ -89,6 +92,11 @@ type SecretsProviderRequest struct { type SecretsProviderResponse struct { Value []byte `json:",omitempty"` // Value is the value of the secret Err string `json:",omitempty"` // Err is the error response of the plugin + + // DoNotReuse indicates that the secret returned from this request should + // only be used for one task, and any further tasks should call the secret + // driver again. + DoNotReuse bool `json:",omitempty"` } // EndpointSpec represents the spec of an endpoint.