From 696130c949df0d49adc22c737acbda421b669d28 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Mon, 28 Nov 2016 12:18:15 -0800 Subject: [PATCH] Fix issue where secret ID is masked by name This fix tries to address the issue in 28884 where it is possible to mask the secret ID by name. The reason was that searching a secret is based on name. However, searching a secret should be done based on: - Full ID - Full Name - Partial ID (prefix) This fix addresses the issue by changing related implementation in `getCliRequestedSecretIDs()` An integration test has been added to cover the changes. This fix fixes 28884 Signed-off-by: Yong Tang (cherry picked from commit 3638ca4d14bcca9bc924f21314e4a1020cd5172f) Signed-off-by: Victor Vieux --- cli/command/secret/utils.go | 64 +++++++++++++------ .../docker_cli_secret_create_test.go | 58 +++++++++++++++++ 2 files changed, 104 insertions(+), 18 deletions(-) diff --git a/cli/command/secret/utils.go b/cli/command/secret/utils.go index 0134853e09..42493896ca 100644 --- a/cli/command/secret/utils.go +++ b/cli/command/secret/utils.go @@ -1,6 +1,9 @@ package secret import ( + "fmt" + "strings" + "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" "github.com/docker/docker/api/types/swarm" @@ -8,10 +11,11 @@ import ( "golang.org/x/net/context" ) -func getSecretsByName(ctx context.Context, client client.APIClient, names []string) ([]swarm.Secret, error) { +func getSecretsByNameOrIDPrefixes(ctx context.Context, client client.APIClient, terms []string) ([]swarm.Secret, error) { args := filters.NewArgs() - for _, n := range names { + for _, n := range terms { args.Add("names", n) + args.Add("id", n) } return client.SecretList(ctx, types.SecretListOptions{ @@ -19,29 +23,53 @@ func getSecretsByName(ctx context.Context, client client.APIClient, names []stri }) } -func getCliRequestedSecretIDs(ctx context.Context, client client.APIClient, names []string) ([]string, error) { - ids := names - - // attempt to lookup secret by name - secrets, err := getSecretsByName(ctx, client, ids) +func getCliRequestedSecretIDs(ctx context.Context, client client.APIClient, terms []string) ([]string, error) { + secrets, err := getSecretsByNameOrIDPrefixes(ctx, client, terms) if err != nil { return nil, err } - lookup := make(map[string]struct{}) - for _, id := range ids { - lookup[id] = struct{}{} - } - if len(secrets) > 0 { - ids = []string{} - - for _, s := range secrets { - if _, ok := lookup[s.Spec.Annotations.Name]; ok { - ids = append(ids, s.ID) + found := make(map[string]struct{}) + next: + for _, term := range terms { + // attempt to lookup secret by full ID + for _, s := range secrets { + if s.ID == term { + found[s.ID] = struct{}{} + continue next + } + } + // attempt to lookup secret by full name + for _, s := range secrets { + if s.Spec.Annotations.Name == term { + found[s.ID] = struct{}{} + continue next + } + } + // attempt to lookup secret by partial ID (prefix) + // return error if more than one matches found (ambiguous) + n := 0 + for _, s := range secrets { + if strings.HasPrefix(s.ID, term) { + found[s.ID] = struct{}{} + n++ + } + } + if n > 1 { + return nil, fmt.Errorf("secret %s is ambiguous (%d matches found)", term, n) } } + + // We already collected all the IDs found. + // Now we will remove duplicates by converting the map to slice + ids := []string{} + for id := range found { + ids = append(ids, id) + } + + return ids, nil } - return ids, nil + return terms, nil } diff --git a/integration-cli/docker_cli_secret_create_test.go b/integration-cli/docker_cli_secret_create_test.go index 3126a0db14..9c45f8a0ae 100644 --- a/integration-cli/docker_cli_secret_create_test.go +++ b/integration-cli/docker_cli_secret_create_test.go @@ -46,3 +46,61 @@ func (s *DockerSwarmSuite) TestSecretCreateWithLabels(c *check.C) { c.Assert(secret.Spec.Labels["key1"], checker.Equals, "value1") c.Assert(secret.Spec.Labels["key2"], checker.Equals, "value2") } + +// Test case for 28884 +func (s *DockerSwarmSuite) TestSecretCreateResolve(c *check.C) { + d := s.AddDaemon(c, true, true) + + name := "foo" + id := d.createSecret(c, swarm.SecretSpec{ + swarm.Annotations{ + Name: name, + }, + []byte("foo"), + }) + c.Assert(id, checker.Not(checker.Equals), "", check.Commentf("secrets: %s", id)) + + fake := d.createSecret(c, swarm.SecretSpec{ + swarm.Annotations{ + Name: id, + }, + []byte("fake foo"), + }) + c.Assert(fake, checker.Not(checker.Equals), "", check.Commentf("secrets: %s", fake)) + + out, err := d.Cmd("secret", "ls") + c.Assert(err, checker.IsNil) + c.Assert(out, checker.Contains, name) + c.Assert(out, checker.Contains, fake) + + out, err = d.Cmd("secret", "rm", id) + c.Assert(out, checker.Contains, id) + + // Fake one will remain + out, err = d.Cmd("secret", "ls") + c.Assert(err, checker.IsNil) + c.Assert(out, checker.Not(checker.Contains), name) + c.Assert(out, checker.Contains, fake) + + // Remove based on name prefix of the fake one + // (which is the same as the ID of foo one) should not work + // as search is only done based on: + // - Full ID + // - Full Name + // - Partial ID (prefix) + out, err = d.Cmd("secret", "rm", id[:5]) + c.Assert(out, checker.Not(checker.Contains), id) + out, err = d.Cmd("secret", "ls") + c.Assert(err, checker.IsNil) + c.Assert(out, checker.Not(checker.Contains), name) + c.Assert(out, checker.Contains, fake) + + // Remove based on ID prefix of the fake one should succeed + out, err = d.Cmd("secret", "rm", fake[:5]) + c.Assert(out, checker.Contains, fake) + out, err = d.Cmd("secret", "ls") + c.Assert(err, checker.IsNil) + c.Assert(out, checker.Not(checker.Contains), name) + c.Assert(out, checker.Not(checker.Contains), id) + c.Assert(out, checker.Not(checker.Contains), fake) +}