Merge pull request #28896 from yongtang/28884-secret-name-mask-ID

Fix issue where secret ID is masked by name

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
 }

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