Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set 0444 as default secret mode in stack deploy #31006

Merged
merged 1 commit into from Feb 17, 2017

Conversation

vdemeester
Copy link
Member

Change the default secret mode to match the default one used in
service subcommands.

Fix #30991

/cc @dnephin @mstanleyjones @nathanleclaire @aaronlehmann @ehazlett @thaJeztah

🐸

Signed-off-by: Vincent Demeester vincent@sbr.pm

@ehazlett
Copy link
Contributor

LGTM (nam)

@@ -217,13 +217,17 @@ func convertServiceSecrets(
if gid == "" {
gid = "0"
}
mode := secret.Mode
if mode == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right check? Mode 0 should be settable since it exists as a mode. If I tried to set the mode to 0 I'd be surprised if it ended up 444.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @mstanleyjones .

Probably the type of Mode needs to be *uint32 rather than uint32
https://github.com/docker/docker/blob/60c1eaf8f08f904248cbfa0d07d850025a35e6d0/cli/compose/types/types.go#L232

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mstanleyjones @AkihiroSuda yeah.. didn't thought of that, but yeah should probably be a pointer instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, is there a situation one would want mode 0 for? It would not be readable or writeable by any user including root so to me that would be kind of like setting --name for a container to an empty string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a situation where it would be useful, but it's a valid mode, so I feel we shouldn't overload it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdemeester do you have another way to do this that doesn't actively prevent the user from setting mode 0 on purpose? Principle of least surprise?

@nathanleclaire
Copy link
Contributor

👍

Copy link
Contributor

@mdlinville mdlinville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few other questions while we are in here:

  • What is secretSpec (line 207)
  • Should you be allowed to pass non-numeric user or group information? It looks like only numeric uid and gid are evaluated here.

@thaJeztah thaJeztah added this to the 1.13.2 milestone Feb 14, 2017
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This should really be handled by the server, instead of in every client, but I'm fine with this patch fix for now. I did the same for gid/uid.

Change the default secret mode to match the default one used in
`service` subcommands.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester vdemeester force-pushed the 30991-secret-mode-in-stack-deploy branch from 98e5689 to f2b68c6 Compare February 16, 2017 10:26
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cpuguy83 cpuguy83 merged commit a12454d into moby:master Feb 17, 2017
@vdemeester vdemeester deleted the 30991-secret-mode-in-stack-deploy branch February 17, 2017 08:36
@thaJeztah thaJeztah added this to pick in 17.03.2-maybe Feb 18, 2017
thaJeztah pushed a commit to thaJeztah/docker that referenced this pull request Feb 18, 2017
…ack-deploy

Set 0444 as default secret mode in stack deploy
(cherry picked from commit a12454d)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah mentioned this pull request Feb 18, 2017
@thaJeztah thaJeztah moved this from pick to picked in 17.03.2-maybe Feb 18, 2017
@thaJeztah thaJeztah removed this from picked in 17.03.2-maybe Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants