Merge pull request #2735 from sirlatrom/one-secret-per-task

Assign secrets individually to each task

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,14 +63,26 @@ 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) {
 	if _, ok := sp.secretIDs[secretID]; !ok {
 		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}
 }

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()
+	}
+}

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

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)
+}

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)
+	}
+}

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
 }

manager/dispatcher/dispatcher_test.go

@@ -420,21 +420,26 @@ 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",
 	}
 
 	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)
+		}
 	}
 }