secrets control-api implementation by cyli · Pull Request #1567 · moby/swarmkit

cyli · 2016-09-24T02:56:17Z

Sorry, this is biggish because it also includes the swarmctl implementations for command line interaction with swarmd. I can split that off into another PR if needed.

This implements the Get/Create/Remove/List functionality for secrets in the control API as specified in #1511.

codecov-io · 2016-09-24T03:31:31Z

Current coverage is 54.00% (diff: 52.31%)

Merging #1567 into master will decrease coverage by 0.02%

@@             master      #1567   diff @@
==========================================
  Files            83         84     +1   
  Lines         13621      13833   +212   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           7360       7471   +111   
- Misses         5265       5358    +93   
- Partials        996       1004     +8

Powered by Codecov. Last update a126286...aebced0

aaronlehmann

Overall LGTM! Just one comment about digest formatting.

aaronlehmann · 2016-09-24T05:36:00Z

manager/controlapi/secret.go

+		ID:         identity.NewID(),
+		Spec:       *spec,
+		SecretSize: uint32(len(spec.Data)),
+		Digest:     hex.EncodeToString(checksumBytes[:]),


I suggest using the github.com/docker/distribution/digest package for this. digest.FromBytes(spec.Data) will give a result like sha256: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855. Including the digest algorithm as a prefix is nice because then we aren't implicitly tied to a particular format going forward. We use this package in a few other places in swarmkit.

Ah, thanks for the link to that package! I originally had the algorithm in the front, but it made the display of the hash a little longer, so removed it, but agree it's a better idea to include. And thanks for the quick review!

AkihiroSuda · 2016-09-24T08:33:51Z

cmd/swarmctl/secrets/inspect.go

+
+var (
+	inspectCmd = &cobra.Command{
+		Use:   "inspect <secret ID orname>",


orname --> or name

Nice catch, thanks!

stevvooe · 2016-09-26T22:34:12Z

manager/controlapi/secret_test.go

+		"with:colon",
+		"with;semicolon",
+		"snowman☃",
+		"o_______________________________________________________________o", // exactly 65 characters


strings.Repeat?

Aww, I liked the giant frowny face. :) But yes strings.Repeat would make it clearer, thanks.

stevvooe · 2016-09-26T22:38:38Z

manager/controlapi/secret.go

 // MaxSecretSize is the maximum byte length of the `Secret.Spec.Data` field.
 const MaxSecretSize = 500 * 1024 // 500KB

+var isValidSecretName = regexp.MustCompile(`^[a-zA-Z0-9](?:[a-zA-Z0-9-_.]{0,62}[a-zA-Z0-9])*$`)


validSecretNameRegexp.

Describe what this regexp is supposed to do.

Looks like we can structure this in a more simple manner:

^[a-zA-Z0-9]+(?:[-_.]{1}[a-zA-Z0-9]+)*$

Validate total length outside; regexp isn't good at doing length checks. As they change, maintaining the length property may break down.

Also, consider not starting with numbers.

I don't have any strong feelings about it, but any particular reason to not start with numbers?

Also, this change would consider multiple underscores/dashes/dots invalid - again any reason for the limitation? (am just curious - again I don't feel strongly; although double symbols ugly for names, not sure if it should be invalid)

IRL discussion: We just want to make sure the names would work for what we're going to use it for (for example, in case we wanted to be able to use the name for environment variables). Which I think excludes - and . However, if we wanted them to also work for filenames maybe having a . is ok?

Either way, I think we were going with requiring the user give us a target value, and it may make sense to let users be a little more expressive in their names (to make it easier to script naming conventions, etc.) if they need to be able to describe what the secret is.

I don't feel super strongly - I'll leave this as is for now, unless someone has a stronger opinion (and we can also resolve this in a later PR). I have made the variable name change, and updated the length checking to happen in the validation function instead of in the regex.

stevvooe · 2016-09-26T22:39:37Z

manager/controlapi/secret.go

+	return &api.Secret{
+		ID:         identity.NewID(),
+		Spec:       *spec,
+		SecretSize: uint32(len(spec.Data)),


Please change the type of this field to int64. No need to use unsigned here: we aren't interested in the field-arithmetic properties.

Just curious - what's wrong with using unsigned in situations like this? I tend to think it makes sense in cases where a negative value would clearly be invalid. In some cases it can avoid bugs that happen when something trying to use the value uses it in an arithmetic way that assumes the value is positive.

@aaronlehmann @stevvooe explained it as a way to signal invalid values - it's possible to accidentally set a uint to a value that will just overflow without it being an error (https://play.golang.org/p/ItoXPaiHtO), in which case it'd look like you'd have a potentially valid value, whereas if it were an int64 a bounds check would be able to detect that it's negative

stevvooe · 2016-09-26T22:40:10Z

manager/controlapi/secret.go

+		ID:         identity.NewID(),
+		Spec:       *spec,
+		SecretSize: uint32(len(spec.Data)),
+		Digest:     digest.FromBytes(spec.Data).String(),


You can use a custom type in the protobuf to avoid the string conversion.

A digest.Digest object is just a hex-encoded string with the algorithm as the prefix, and Digest.String() just returns itself cast into a string. I can create a custom object to store the algorithm + bytes, if that would make more sense to store as an object?

(It seems clearer to me to just use string in this case, rather than someone having to look up with digest.Digest is, especially on a protocol definition, but I don't feel particularly strongly.)

No, don't create a new protobuf type. Just tell the compiler to generate this field with type digest.Digest using the custom type extension.

Out of band discussion:

using: string digest = 4 [(gogoproto.customtype) = "github.com/docker/distribution/digest.Digest"];, make generate produces a compiled object.pb.go file with no errors, but that file refuses to build:

$ make build 🐳 build github.com/docker/swarmkit github.com/docker/swarmkit/api/duration github.com/docker/swarmkit/api/timestamp github.com/docker/swarmkit/protobuf/plugin github.com/docker/swarmkit/manager/testcluster github.com/docker/swarmkit/protobuf/plugin/deepcopy/test github.com/docker/swarmkit/protobuf/plugin/raftproxy/test github.com/docker/swarmkit/protobuf/ptypes github.com/docker/swarmkit/api github.com/docker/swarmkit/protobuf/plugin/authenticatedwrapper github.com/docker/swarmkit/protobuf/plugin/deepcopy github.com/docker/swarmkit/cmd/protoc-gen-gogoswarm # github.com/docker/swarmkit/api api/objects.pb.go:1307: invalid argument m.Digest (type *digest.Digest) for len api/objects.pb.go:1310: invalid argument m.Digest (type *digest.Digest) for len api/objects.pb.go:1311: second argument to copy should be slice or string; have *digest.Digest api/objects.pb.go:1597: invalid argument m.Digest (type *digest.Digest) for len api/objects.pb.go:3844: cannot use digest.Digest(data[iNdEx:postIndex]) (type digest.Digest) as type *digest.Digest in assignment make: *** [build] Error 2

This may be a bug with gogo, so we're just going to leave it a string for now and maybe work it out later.

aluzzardi

Overall looks good, a few nits in the CLI but the rest of the code (store/control API) looks great

aluzzardi · 2016-09-26T22:31:21Z

cmd/swarmctl/main.go

 	"github.com/docker/swarmkit/cmd/swarmctl/cluster"
 	"github.com/docker/swarmkit/cmd/swarmctl/network"
 	"github.com/docker/swarmkit/cmd/swarmctl/node"
+	"github.com/docker/swarmkit/cmd/swarmctl/secrets"


nit: This should be named secret to be consistent with the rest (e.g. node, network, ...)

aluzzardi · 2016-09-26T22:31:35Z

cmd/swarmctl/main.go

 		version.Cmd,
 		network.Cmd,
 		cluster.Cmd,
+		secrets.Cmd,


aluzzardi · 2016-09-26T22:36:51Z

cmd/swarmctl/secrets/create.go

+	Short: "Create a secret",
+	RunE: func(cmd *cobra.Command, args []string) error {
+		if len(args) != 1 {
+			return errors.New("create command takes a unique secret name as an argument, and accepts secret data via stdin")


I guess the UX doesn't really matter for swarmctl as users will be using the docker CLI, but how to you envision that UX?

As in, do we expect secrets to be passed through stdin only?

I have a counter proposal (assuming foo is the secret name and bar the secret value):

Pass as an argument: docker secret create foo bar Pass to stdin: echo bar | docker secret create foo - Pass as a file: echo bar > value.txt && docker secret create foo -f value.txt

Thoughts? /cc @cyli @diogomonica @aaronlehmann @stevvooe

I think @diogomonica proposed value in the doc, too. I'm +1 on this.

I actually like passing the secrets on stdin, because doing it on the command line is really bad practice (visible through the process table, and often saved to shell history). Building the UI to support it encourages risky practices and looks unprofessional (unfortunately, we made this mistake with join tokens).

So stdin and reading from files are both legitimate ways to input the secret, but it's really easy to just use shell pipelines and not have two ways of doing the same thing. (But I'm not opposed to a -f option, I guess).

I don't want to hold up this PR for this since the docker UX can and will be different, but I don't necessarily agree.

echo blah | docker secret create would show up in the process list / shell history as well

If you have local root access, you can grab the secrets from memory anyway

stdin can be messy for the users, especially since we don't display the secrets back to the user. Having a trailing \n will be a pretty common problem

All the secret stores I've played with so far allow to provide secrets through the command line

A manager is not "the enemy". We're not trying to protect secrets against the manager machine itself

Oh wait, sorry, I misread the first comment. I think taking a secret as an argument may not be great, since it's visible on the shell history. I was +1 on passing in a file via -f argument, but I don't feel super strongly between that and @aaronlehmann's stdin. The positive thing about @aaronlehmann's version of stdin being constructed by pipes was being able to pipe it in from some other source (e.g. maybe some other command line utility)

swarmctl is good practice for the big show...

aluzzardi · 2016-09-26T22:41:52Z

cmd/swarmctl/secrets/inspect.go

+	common.FprintfIfNotEmpty(w, "Digest\t: %s\n", secret.Digest)
+	common.FprintfIfNotEmpty(w, "Size\t: %d\n", secret.SecretSize)
+
+	created := time.Unix(int64(secret.Meta.CreatedAt.Seconds), int64(secret.Meta.CreatedAt.Nanos))


There's a ptypes package that provides helpers to format protobuf datetimes so you don't need to convert manually.

These 2 lines could become this:

common.FprintfIfNotEmpty(w, "Created\t: %s\n", ptypes.TimestampString(secret.Meta.CreatedAt))

There are examples in various inspect.go such as https://github.com/docker/swarmkit/blob/master/cmd/swarmctl/task/inspect.go

s.Meta.CreatedAt seems to be the wrong type (I get type *"github.com/docker/swarmkit/api/timestamp".Timestamp) as type *"github.com/golang/protobuf/ptypes/timestamp".Timestamp in argument to ptypes.Timestamp when I tried to do it previously), so none of the ptypes functions work with it. Am I missing something in the protocols, or some kind of translation, etc. that I'm supposed to do such that it's the right type?

I can convert to a ptypes.Timestamp type instead of a time.Time type to use it with that package instead?

You need to use "github.com/docker/swarmkit/protobuf/ptypes"

Oops, that's what I get for depending on autocomplete and gofmt. :D Thanks!

Fixed, thanks!

aluzzardi · 2016-09-26T22:44:12Z

cmd/swarmctl/secrets/list.go

+func (k secretSorter) Len() int      { return len(k) }
+func (k secretSorter) Swap(i, j int) { k[i], k[j] = k[j], k[i] }
+func (k secretSorter) Less(i, j int) bool {
+	iTime := time.Unix(k[i].Meta.CreatedAt.Seconds, int64(k[i].Meta.CreatedAt.Nanos))


ptypes can help here as well:

iTime, err := ptypes.Timestamp(k[i].Meta.CreatedAt) if err != nil { panic(err) } jTime, err := ptypes.Timestamp(k[j].Meta.CreatedAt) if err != nil { panic(err) } return jTime.Before(iTime)

Fixed, thanks!

aluzzardi · 2016-09-26T22:45:46Z

cmd/swarmctl/secrets/list.go

+				}()
+				common.PrintHeader(w, "ID", "Name", "Created", "Digest", "Size")
+				output = func(s *api.Secret) {
+					created := time.Unix(int64(s.Meta.CreatedAt.Seconds), int64(s.Meta.CreatedAt.Nanos))


created, err := ptypes.Timestamp(s.Meta.CreatedAt)

Fixed, thanks!

diogomonica

LGTM, small nits.

diogomonica · 2016-09-27T16:11:30Z

api/objects.proto


-	// Size represents the size (number of bytes) of the secret data, and is
+	// SecretSize represents the size (number of bytes) of the secret data, and is
 	// calculated from the data contained in `Secret.Spec.data`..


Fixed, thanks

diogomonica · 2016-09-27T16:15:27Z

manager/controlapi/secret.go

+		return nil, grpc.Errorf(codes.NotFound, "secret %s not found", request.SecretID)
+	}
+
+	secret.Spec.Data = nil


Add comment as to why.

diogomonica · 2016-09-27T16:17:05Z

manager/controlapi/secret.go

+	s.store.View(func(tx store.ReadTx) {
+		secrets, err = store.FindSecrets(tx, by)
+	})
+


remove extra \n

diogomonica · 2016-09-27T16:18:41Z

manager/controlapi/secret.go

+	if request.SecretID == "" {
+		return nil, grpc.Errorf(codes.InvalidArgument, "secret ID must be provided")
+	}
+


For this and the above comment, I just removed a couple newlines in several of the functions.

diogomonica · 2016-09-27T16:19:33Z

manager/controlapi/secret.go

+	case store.ErrNotExist:
+		return nil, grpc.Errorf(codes.NotFound, "secret %s not found", request.SecretID)
+	case nil:
+		return &api.RemoveSecretResponse{}, nil


should secret response include the secret that was removed?

Not sure if that would be useful. The other control APIs return empty responses - do we want to make that the case for all the others as well?

diogomonica · 2016-09-27T16:20:37Z

manager/controlapi/secret.go

+	}
+
+	if len(spec.Data) > MaxSecretSize || len(spec.Data) == 0 {
+		return grpc.Errorf(codes.InvalidArgument, "secret data must be between 1 and %d bytes inclusive", MaxSecretSize)


Secret data must be larger than 0 and smaller than MaxSecret bytes

Updated, and also changed the comparison for MaxSecretSize to >=.

…n Lehmann <aaron.lehmann@docker.com> Signed-off-by: cyli <ying.li@docker.com>

Signed-off-by: cyli <ying.li@docker.com>

aluzzardi · 2016-09-27T22:16:19Z

Anything else or can we go ahead and merge this PR?

cyli · 2016-09-27T22:22:40Z

@aluzzardi I think I've semi-addressed #1567 (comment) and #1567 (comment) (the latter mainly I just commented). I think if @stevvooe is ok with those, and you are ok with addressing #1567 (comment) in a later PR or just in docker itself, it's ready to go. (e.g. I believe those were the only three comments I haven't fully addressed in code, just in comments)

aluzzardi · 2016-09-27T22:46:05Z

I'm okay with addressing #1567 (comment) in a follow up or even just in Docker so we can get this one going since it's not super priority and UX questions tend to take a while to get resolved :)

Signed-off-by: cyli <ying.li@docker.com>

cyli · 2016-09-27T23:59:42Z

(have rebased)

GordonTheTurtle added the status/0-triage label Sep 24, 2016

cyli force-pushed the secrets-implementation branch from e373f59 to a805231 Compare September 24, 2016 03:24

cyli changed the title ~~[WIP] secrets control-api implementation~~ secrets control-api implementation Sep 24, 2016

cyli force-pushed the secrets-implementation branch from a805231 to 733f210 Compare September 24, 2016 03:36

aaronlehmann reviewed Sep 24, 2016

View reviewed changes

AkihiroSuda reviewed Sep 24, 2016

View reviewed changes

cyli force-pushed the secrets-implementation branch from 733f210 to e53ed9e Compare September 24, 2016 23:13

cyli mentioned this pull request Sep 26, 2016

Deliver secrets using assignments #1544

Merged

stevvooe suggested changes Sep 26, 2016

View reviewed changes

aluzzardi suggested changes Sep 26, 2016

View reviewed changes

cyli force-pushed the secrets-implementation branch from e53ed9e to 8777042 Compare September 27, 2016 01:23

diogomonica approved these changes Sep 27, 2016

View reviewed changes

cyli force-pushed the secrets-implementation branch from 8777042 to 2f2b67c Compare September 27, 2016 17:00

aluzzardi approved these changes Sep 27, 2016

View reviewed changes

diogomonica and others added 3 commits September 27, 2016 14:25

Initial secret store in raft state. Cherry-picked from commit by Aaro…

030a81f

…n Lehmann <aaron.lehmann@docker.com> Signed-off-by: cyli <ying.li@docker.com>

Add controlapi implementation of create, get, remove, and list secrets

445b738

Signed-off-by: cyli <ying.li@docker.com>

Add swarmctl commands to create, inspect, list and remove secrets.

930d873

Signed-off-by: cyli <ying.li@docker.com>

Address review comments

aebced0

Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the secrets-implementation branch from 2f2b67c to aebced0 Compare September 27, 2016 23:59

stevvooe approved these changes Sep 28, 2016

View reviewed changes

stevvooe merged commit 4cf6f72 into moby:master Sep 28, 2016

cyli deleted the secrets-implementation branch September 28, 2016 00:39

This was referenced Sep 30, 2016

Add a -f option to let a user pass a filename for creating secrets #1595

Merged

Implement swarmkit cli secrets add, list, remove and update #1463

Closed

Implement ControlAPI secrets add, list, remove and update #1462

Closed

ehazlett mentioned this pull request Oct 26, 2016

Secret Management moby/moby#27794

Merged

4 tasks

aaronlehmann mentioned this pull request Dec 17, 2016

Add cyli to maintainers #1818

Merged

Conversation

cyli commented Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 54.00% (diff: 52.31%)

Uh oh!

aaronlehmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aluzzardi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli commented Sep 24, 2016 •

edited

Loading

codecov-io commented Sep 24, 2016 •

edited

Loading

cyli Sep 27, 2016 •

edited

Loading

cyli Sep 27, 2016 •

edited

Loading

cyli Sep 26, 2016 •

edited

Loading

cyli Sep 26, 2016 •

edited

Loading