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 <yong.tang.github@outlook.com>

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) {
+func getCliRequestedSecretIDs(ctx context.Context, client client.APIClient, terms []string) ([]string, error) {
-	ids := names
+	secrets, err := getSecretsByNameOrIDPrefixes(ctx, client, terms)
-
-	// attempt to lookup secret by name
-	secrets, err := getSecretsByName(ctx, client, ids)
 	if err != nil {
 		return nil, err
 	}
 
-	lookup := make(map[string]struct{})
-	for _, id := range ids {
-		lookup[id] = struct{}{}
-	}
-
 	if len(secrets) > 0 {
-		ids = []string{}
+		found := make(map[string]struct{})
-
+	next:
+		for _, term := range terms {
+			// attempt to lookup secret by full ID
 			for _, s := range secrets {
-			if _, ok := lookup[s.Spec.Annotations.Name]; ok {
+				if s.ID == term {
-				ids = append(ids, s.ID)
+					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 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)
+}