From 1ce2840137b2c70a5a520fb4833a3c2cb8cd0810 Mon Sep 17 00:00:00 2001 From: Sune Keller Date: Tue, 4 Sep 2018 12:39:39 +0200 Subject: [PATCH 1/5] Support individual secrets per task Signed-off-by: Sune Keller --- agent/secrets/secrets.go | 11 ++- agent/secrets/secrets_test.go | 113 ++++++++++++++++++++++++++ api/api.pb.txt | 14 ++++ identity/randomid.go | 29 +++++++ identity/randomid_test.go | 53 ++++++++++++ manager/dispatcher/assignments.go | 38 ++++++--- manager/dispatcher/dispatcher_test.go | 47 +++++++++-- manager/drivers/secrets.go | 17 ++-- 8 files changed, 294 insertions(+), 28 deletions(-) create mode 100644 agent/secrets/secrets_test.go diff --git a/agent/secrets/secrets.go b/agent/secrets/secrets.go index 233101d0fb..11c0c854df 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,12 @@ 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) + secret, err := sp.secrets.Get(identity.XorIDs(secretID, sp.taskID)) + if err != nil { + return sp.secrets.Get(secretID) + } + secret.ID = secretID + return secret, err } // Restrict provides a getter that only allows access to the secrets @@ -84,5 +91,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..6a11648162 --- /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() + xorID := identity.XorIDs(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 xor'ed 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 xor of the secret and + // task IDs. + secretID: xorID, + // 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 xor'ed ID without + // it being added as such in an assignment, something has been changed + // inconsistently. + { + desc: "Test attempting to get a secret by xor'ed ID when secret is added with original ID", + value: "value", + expectedErr: fmt.Sprintf("task not authorized to access secret %s", xorID), + secretIDs: map[string]struct{}{ + originalSecretID: {}, + }, + secretID: originalSecretID, + secretIDToGet: xorID, + taskID: taskID, + }, + } + secretsManager := NewManager() + for _, testCase := range testCases { + t.Logf("secretID=%s, taskID=%s, xorID=%s", originalSecretID, taskID, xorID) + 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/randomid.go b/identity/randomid.go index 0eb13527aa..9d2ca7b366 100644 --- a/identity/randomid.go +++ b/identity/randomid.go @@ -51,3 +51,32 @@ func NewID() string { p[0] |= 0x80 // set high bit to avoid the need for padding return (&big.Int{}).SetBytes(p[:]).Text(randomIDBase)[1 : maxRandomIDLength+1] } + +// XorIDs Xors two random IDs and produces a third random ID. This third random +// ID can then be reliably xorred back with one of the original IDs to get out +// the other original ID +// +// Note that XorIDs is not guaranteed to produce an ID of the same length as +// NewID, or with the same properties. +func XorIDs(id1, id2 string) string { + // first, we convert the ids back to big.Int, so we can do math on them + id1int, success := (&big.Int{}).SetString(id1, randomIDBase) + if !success { + panic(fmt.Errorf("Failed to convert first ID to big.Int{}")) + } + id2int, success := (&big.Int{}).SetString(id2, randomIDBase) + if !success { + panic(fmt.Errorf("Failed to convert second ID to big.Int{}")) + } + + // now, we xor the two numbers together. + xorint := (&big.Int{}).Xor(id1int, id2int) + // if there are leading 0s in the result, they might get omitted, but we + // want to keep them affirmatively. thus, if the resulting string is less + // than maxRandomIDLength, then we should leading pad with 0s to make it + xorstr := xorint.Text(randomIDBase) + for len(xorstr) < maxRandomIDLength { + xorstr = "0" + xorstr + } + return xorstr +} diff --git a/identity/randomid_test.go b/identity/randomid_test.go index 9688a1108e..ecd16975b7 100644 --- a/identity/randomid_test.go +++ b/identity/randomid_test.go @@ -31,3 +31,56 @@ func TestGenerateGUID(t *testing.T) { } } } + +func TestXorIDs(t *testing.T) { + idReader = rand.New(rand.NewSource(0)) + for i := 0; i < 1000; i++ { + id1 := NewID() + id2 := NewID() + + xor := XorIDs(id1, id2) + + t.Logf("\n-- iteration %v --\n", i) + t.Logf("id1: %v, id2: %v, xor: %v", id1, id2, xor) + t.Logf("in binary:") + + var id1int, id2int, xorint big.Int + id1int.SetString(id1, randomIDBase) + id2int.SetString(id2, randomIDBase) + xorint.SetString(xor, randomIDBase) + + t.Logf("\tid1: %130v", id1int.Text(2)) + t.Logf("\tid2: %130v", id2int.Text(2)) + t.Logf("\txor: %130v", xorint.Text(2)) + + var i big.Int + _, ok := i.SetString(xor, randomIDBase) + if !ok { + t.Fatal("xor result should be base 36") + } + + // the length issue is a result of the truncation and shift in the generation of ids + // it means that some values that fit in the same number of bytes do _not_ fit in the same number of base36 characters + /* + if len(xor) < maxRandomIDLength { + t.Errorf("len(%s) (%d) != %v", xor, len(xor), maxRandomIDLength) + } + */ + + unxor1 := XorIDs(xor, id1) + if unxor1 != id2 { + t.Errorf( + "unxoring by id1 != id2 (%s ^ %s != %s, got %s)", + xor, id1, id2, unxor1, + ) + } + + unxor2 := XorIDs(xor, id2) + if unxor2 != id1 { + t.Errorf( + "unxoring by id2 != id1 (%s ^ %s != %s, got %v)", + xor, id2, id1, unxor2, + ) + } + } +} diff --git a/manager/dispatcher/assignments.go b/manager/dispatcher/assignments.go index 5a56348053..7a2d497ad9 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 a driver is used, give a unique ID per task to allow different values for different tasks + if doNotReuse { + // Give the secret a new ID and mark it as internal + originalSecretID := secret.ID + xorID := identity.XorIDs(originalSecretID, t.ID) + secret.ID = xorID + 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,7 @@ 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 { + if _, exists := a.tasksUsingDependency[mapKey][t.ID]; !exists { assignSecret(a, readTx, mapKey, t) } a.tasksUsingDependency[mapKey][t.ID] = struct{}{} @@ -291,26 +307,26 @@ 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) { +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..af6044d299 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 { + xorID := identity.XorIDs("driverDoNotReuseSecret", task.ID) + assert.Equal(t, xorID, s.ID) + assert.Equal(t, doNotReuseSecretValue, s.Spec.Data) + } } } diff --git a/manager/drivers/secrets.go b/manager/drivers/secrets.go index 2e7bc392b6..43f609ac45 100644 --- a/manager/drivers/secrets.go +++ b/manager/drivers/secrets.go @@ -26,12 +26,12 @@ func NewSecretDriver(plugin plugingetter.CompatPlugin) *SecretDriver { } // Get gets a secret from the secret provider -func (d *SecretDriver) Get(spec *api.SecretSpec, task *api.Task) ([]byte, error) { +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 +67,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 +89,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. From 554ddbd7bad2a73d1770be1d0d1ea56490383eea Mon Sep 17 00:00:00 2001 From: Sune Keller Date: Thu, 11 Oct 2018 14:56:45 +0200 Subject: [PATCH 2/5] Address review comments from @anshulpundir Signed-off-by: Sune Keller --- agent/secrets/secrets.go | 4 ++++ identity/randomid.go | 11 ++++++----- manager/dispatcher/assignments.go | 6 ++++-- manager/drivers/secrets.go | 5 ++++- 4 files changed, 18 insertions(+), 8 deletions(-) diff --git a/agent/secrets/secrets.go b/agent/secrets/secrets.go index 11c0c854df..464c0cefed 100644 --- a/agent/secrets/secrets.go +++ b/agent/secrets/secrets.go @@ -71,10 +71,14 @@ func (sp *taskRestrictedSecretsProvider) Get(secretID string) (*api.Secret, erro return nil, fmt.Errorf("task not authorized to access secret %s", secretID) } + // First check if the secret is available with the task specific ID, which is the XOR of the original secret ID and the task ID. + // That is the case when a secret driver has returned DoNotReuse == true for a secret value. secret, err := sp.secrets.Get(identity.XorIDs(secretID, sp.taskID)) 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 } diff --git a/identity/randomid.go b/identity/randomid.go index 9d2ca7b366..b85883fb34 100644 --- a/identity/randomid.go +++ b/identity/randomid.go @@ -59,7 +59,7 @@ func NewID() string { // Note that XorIDs is not guaranteed to produce an ID of the same length as // NewID, or with the same properties. func XorIDs(id1, id2 string) string { - // first, we convert the ids back to big.Int, so we can do math on them + // First, we convert the ids back to big.Int, so we can do math on them id1int, success := (&big.Int{}).SetString(id1, randomIDBase) if !success { panic(fmt.Errorf("Failed to convert first ID to big.Int{}")) @@ -69,13 +69,14 @@ func XorIDs(id1, id2 string) string { panic(fmt.Errorf("Failed to convert second ID to big.Int{}")) } - // now, we xor the two numbers together. + // Now, we xor the two numbers together. xorint := (&big.Int{}).Xor(id1int, id2int) - // if there are leading 0s in the result, they might get omitted, but we - // want to keep them affirmatively. thus, if the resulting string is less + // If there are leading 0s in the result, they might get omitted, but we + // want to keep them affirmatively. Thus, if the resulting string is less // than maxRandomIDLength, then we should leading pad with 0s to make it + // the same length as other IDs. xorstr := xorint.Text(randomIDBase) - for len(xorstr) < maxRandomIDLength { + for i := len(xorstr); i < maxRandomIDLength; i++ { xorstr = "0" + xorstr } return xorstr diff --git a/manager/dispatcher/assignments.go b/manager/dispatcher/assignments.go index 7a2d497ad9..685c3af81f 100644 --- a/manager/dispatcher/assignments.go +++ b/manager/dispatcher/assignments.go @@ -48,7 +48,7 @@ func assignSecret(a *assignmentSet, readTx store.ReadTx, mapKey typeAndID, t *ap }).Debug("failed to fetch secret") return } - // If a driver is used, give a unique ID per task to allow different values for different tasks + // 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 @@ -306,7 +306,9 @@ 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. +// 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 { diff --git a/manager/drivers/secrets.go b/manager/drivers/secrets.go index 43f609ac45..a9bdaa46a6 100644 --- a/manager/drivers/secrets.go +++ b/manager/drivers/secrets.go @@ -25,7 +25,10 @@ func NewSecretDriver(plugin plugingetter.CompatPlugin) *SecretDriver { return &SecretDriver{plugin: plugin} } -// Get gets a secret from the secret provider +// 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, false, fmt.Errorf("secret spec is nil") From 7b98e00762b96e000307f8d90b7e6e39fb1c7a2e Mon Sep 17 00:00:00 2001 From: Sune Keller Date: Thu, 11 Oct 2018 15:07:03 +0200 Subject: [PATCH 3/5] Addess PR comment by @justincormack Signed-off-by: Sune Keller --- agent/secrets/secrets.go | 7 ++-- agent/secrets/secrets_test.go | 24 ++++++------ identity/randomid.go | 30 --------------- identity/randomid_test.go | 53 --------------------------- manager/dispatcher/assignments.go | 5 +-- manager/dispatcher/dispatcher_test.go | 4 +- 6 files changed, 20 insertions(+), 103 deletions(-) diff --git a/agent/secrets/secrets.go b/agent/secrets/secrets.go index 464c0cefed..d6b54d83ad 100644 --- a/agent/secrets/secrets.go +++ b/agent/secrets/secrets.go @@ -6,7 +6,6 @@ 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 @@ -71,9 +70,11 @@ func (sp *taskRestrictedSecretsProvider) Get(secretID string) (*api.Secret, erro return nil, fmt.Errorf("task not authorized to access secret %s", secretID) } - // First check if the secret is available with the task specific ID, which is the XOR of the original secret ID and the task ID. + // 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. - secret, err := sp.secrets.Get(identity.XorIDs(secretID, sp.taskID)) + taskSpecificID := fmt.Sprintf("%s.%s", 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) diff --git a/agent/secrets/secrets_test.go b/agent/secrets/secrets_test.go index 6a11648162..e15fffb8d2 100644 --- a/agent/secrets/secrets_test.go +++ b/agent/secrets/secrets_test.go @@ -27,7 +27,7 @@ func TestTaskRestrictedSecretsProvider(t *testing.T) { originalSecretID := identity.NewID() taskID := identity.NewID() - xorID := identity.XorIDs(originalSecretID, taskID) + taskSpecificID := fmt.Sprintf("%s.%s", originalSecretID, taskID) testCases := []testCase{ // The default case when not using a secrets driver or not returning @@ -50,7 +50,7 @@ func TestTaskRestrictedSecretsProvider(t *testing.T) { // The case for when a secrets driver returns DoNotReuse: true in the // SecretsProviderResponse. { - desc: "Test getting secret by xor'ed ID when restricted by task", + desc: "Test getting secret by task specific ID when restricted by task", value: "value", expected: "value", secretIDs: map[string]struct{}{ @@ -58,9 +58,9 @@ func TestTaskRestrictedSecretsProvider(t *testing.T) { }, // 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 xor of the secret and - // task IDs. - secretID: xorID, + // 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, @@ -68,24 +68,24 @@ func TestTaskRestrictedSecretsProvider(t *testing.T) { }, // 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 xor'ed ID without - // it being added as such in an assignment, something has been changed - // inconsistently. + // 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 xor'ed ID when secret is added with original ID", + 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", xorID), + expectedErr: fmt.Sprintf("task not authorized to access secret %s", taskSpecificID), secretIDs: map[string]struct{}{ originalSecretID: {}, }, secretID: originalSecretID, - secretIDToGet: xorID, + secretIDToGet: taskSpecificID, taskID: taskID, }, } secretsManager := NewManager() for _, testCase := range testCases { - t.Logf("secretID=%s, taskID=%s, xorID=%s", originalSecretID, taskID, xorID) + t.Logf("secretID=%s, taskID=%s, taskSpecificID=%s", originalSecretID, taskID, taskSpecificID) secretsManager.Add(api.Secret{ ID: testCase.secretID, Spec: api.SecretSpec{ diff --git a/identity/randomid.go b/identity/randomid.go index b85883fb34..0eb13527aa 100644 --- a/identity/randomid.go +++ b/identity/randomid.go @@ -51,33 +51,3 @@ func NewID() string { p[0] |= 0x80 // set high bit to avoid the need for padding return (&big.Int{}).SetBytes(p[:]).Text(randomIDBase)[1 : maxRandomIDLength+1] } - -// XorIDs Xors two random IDs and produces a third random ID. This third random -// ID can then be reliably xorred back with one of the original IDs to get out -// the other original ID -// -// Note that XorIDs is not guaranteed to produce an ID of the same length as -// NewID, or with the same properties. -func XorIDs(id1, id2 string) string { - // First, we convert the ids back to big.Int, so we can do math on them - id1int, success := (&big.Int{}).SetString(id1, randomIDBase) - if !success { - panic(fmt.Errorf("Failed to convert first ID to big.Int{}")) - } - id2int, success := (&big.Int{}).SetString(id2, randomIDBase) - if !success { - panic(fmt.Errorf("Failed to convert second ID to big.Int{}")) - } - - // Now, we xor the two numbers together. - xorint := (&big.Int{}).Xor(id1int, id2int) - // If there are leading 0s in the result, they might get omitted, but we - // want to keep them affirmatively. Thus, if the resulting string is less - // than maxRandomIDLength, then we should leading pad with 0s to make it - // the same length as other IDs. - xorstr := xorint.Text(randomIDBase) - for i := len(xorstr); i < maxRandomIDLength; i++ { - xorstr = "0" + xorstr - } - return xorstr -} diff --git a/identity/randomid_test.go b/identity/randomid_test.go index ecd16975b7..9688a1108e 100644 --- a/identity/randomid_test.go +++ b/identity/randomid_test.go @@ -31,56 +31,3 @@ func TestGenerateGUID(t *testing.T) { } } } - -func TestXorIDs(t *testing.T) { - idReader = rand.New(rand.NewSource(0)) - for i := 0; i < 1000; i++ { - id1 := NewID() - id2 := NewID() - - xor := XorIDs(id1, id2) - - t.Logf("\n-- iteration %v --\n", i) - t.Logf("id1: %v, id2: %v, xor: %v", id1, id2, xor) - t.Logf("in binary:") - - var id1int, id2int, xorint big.Int - id1int.SetString(id1, randomIDBase) - id2int.SetString(id2, randomIDBase) - xorint.SetString(xor, randomIDBase) - - t.Logf("\tid1: %130v", id1int.Text(2)) - t.Logf("\tid2: %130v", id2int.Text(2)) - t.Logf("\txor: %130v", xorint.Text(2)) - - var i big.Int - _, ok := i.SetString(xor, randomIDBase) - if !ok { - t.Fatal("xor result should be base 36") - } - - // the length issue is a result of the truncation and shift in the generation of ids - // it means that some values that fit in the same number of bytes do _not_ fit in the same number of base36 characters - /* - if len(xor) < maxRandomIDLength { - t.Errorf("len(%s) (%d) != %v", xor, len(xor), maxRandomIDLength) - } - */ - - unxor1 := XorIDs(xor, id1) - if unxor1 != id2 { - t.Errorf( - "unxoring by id1 != id2 (%s ^ %s != %s, got %s)", - xor, id1, id2, unxor1, - ) - } - - unxor2 := XorIDs(xor, id2) - if unxor2 != id1 { - t.Errorf( - "unxoring by id2 != id1 (%s ^ %s != %s, got %v)", - xor, id2, id1, unxor2, - ) - } - } -} diff --git a/manager/dispatcher/assignments.go b/manager/dispatcher/assignments.go index 685c3af81f..a412e53923 100644 --- a/manager/dispatcher/assignments.go +++ b/manager/dispatcher/assignments.go @@ -6,7 +6,6 @@ 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" @@ -52,8 +51,8 @@ func assignSecret(a *assignmentSet, readTx store.ReadTx, mapKey typeAndID, t *ap if doNotReuse { // Give the secret a new ID and mark it as internal originalSecretID := secret.ID - xorID := identity.XorIDs(originalSecretID, t.ID) - secret.ID = xorID + taskSpecificID := fmt.Sprintf("%s.%s", 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. diff --git a/manager/dispatcher/dispatcher_test.go b/manager/dispatcher/dispatcher_test.go index af6044d299..718e5ce401 100644 --- a/manager/dispatcher/dispatcher_test.go +++ b/manager/dispatcher/dispatcher_test.go @@ -549,8 +549,8 @@ func TestAssignmentsSecretDriver(t *testing.T) { } 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 { - xorID := identity.XorIDs("driverDoNotReuseSecret", task.ID) - assert.Equal(t, xorID, s.ID) + taskSpecificID := fmt.Sprintf("%s.%s", "driverDoNotReuseSecret", task.ID) + assert.Equal(t, taskSpecificID, s.ID) assert.Equal(t, doNotReuseSecretValue, s.Spec.Data) } } From 5840dbe55c874c3691ed25b707f70f4a4f13a70d Mon Sep 17 00:00:00 2001 From: Sune Keller Date: Wed, 17 Oct 2018 10:20:30 +0200 Subject: [PATCH 4/5] Address review comment by @wk8 Signed-off-by: Sune Keller --- agent/secrets/secrets.go | 3 ++- identity/combined_id.go | 8 ++++++++ identity/combined_id_test.go | 19 +++++++++++++++++++ manager/dispatcher/assignments.go | 3 ++- 4 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 identity/combined_id.go create mode 100644 identity/combined_id_test.go diff --git a/agent/secrets/secrets.go b/agent/secrets/secrets.go index d6b54d83ad..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 @@ -73,7 +74,7 @@ func (sp *taskRestrictedSecretsProvider) Get(secretID string) (*api.Secret, erro // 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 := fmt.Sprintf("%s.%s", secretID, sp.taskID) + 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. 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 a412e53923..43733048b5 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" @@ -51,7 +52,7 @@ func assignSecret(a *assignmentSet, readTx store.ReadTx, mapKey typeAndID, t *ap if doNotReuse { // Give the secret a new ID and mark it as internal originalSecretID := secret.ID - taskSpecificID := fmt.Sprintf("%s.%s", originalSecretID, t.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. From fc555b278b17bdcb74bce75b235e6cda758d5210 Mon Sep 17 00:00:00 2001 From: Sune Keller Date: Wed, 17 Oct 2018 23:17:41 +0200 Subject: [PATCH 5/5] Add comment on difference in dependency map check Signed-off-by: Sune Keller --- manager/dispatcher/assignments.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/manager/dispatcher/assignments.go b/manager/dispatcher/assignments.go index 43733048b5..101c449edd 100644 --- a/manager/dispatcher/assignments.go +++ b/manager/dispatcher/assignments.go @@ -120,6 +120,11 @@ func (a *assignmentSet) addTaskDependencies(readTx store.ReadTx, t *api.Task) { secretID := secretRef.SecretID mapKey := typeAndID{objType: api.ResourceType_SECRET, id: secretID} + // 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) }