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

Add cli build warning about chmod bits on windows #11397

Merged
merged 1 commit into from
Mar 20, 2015
Merged

Add cli build warning about chmod bits on windows #11397

merged 1 commit into from
Mar 20, 2015

Conversation

ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Mar 16, 2015

This shows a warning message about adjusted file/directory permission bits
when the docker build cli command is executed on windows. This will help
Windows users understand the potential security problems.

We should be revisiting this warning when building against a Windows
docker engine. @jhowardmsft

Example:

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
cc: @tianon @tiborvass @ewindisch

@ahmetb
Copy link
Contributor Author

ahmetb commented Mar 16, 2015

@duglin TestBuildStderr (on Windows) doesn't seem like liked this (it wants to see stderr empty). Currently I'm using log.Warn which prints the warning to cli.Stderr. What do you advise for the docker build -q case and should we be writing to stdout instead?

// windows: show error message about modified file permissions
if runtime.GOOS == "windows" {
msg := `SECURITY WARNING: You are building a Docker image from Windows against a Linux Docker host. All files and directories added to build context will be set as executable and group/other permissions will be removed. It is recommended to double check permissions for sensitive files and directories.`
log.Warn(msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for a variable use the string directly.

@ahmetb
Copy link
Contributor Author

ahmetb commented Mar 16, 2015

@tiborvass fixed your comments. waiting on @duglin's input on failing TestBuildStderr. I'm still not sure writing to stderr is OK in this case. log methods are not really extensively used in CLI code, so I'm suspicious.

@jessfraz jessfraz added this to the 1.6.0 milestone Mar 16, 2015
@duglin
Copy link
Contributor

duglin commented Mar 17, 2015

I think http://stackoverflow.com/questions/1430956/should-i-output-warnings-to-stderr-or-stdout is a good explanation for why warnings should go to stderr.
Can we modify the test so that if its running on windows we only accept that warning in the output and if anything else is there then we fail?

@ahmetb
Copy link
Contributor Author

ahmetb commented Mar 17, 2015

@duglin thanks for taking a look and suggesting to modify the test. I will basically say: if windows, all lines in stderr must begin with SECURITY WARNING:. do you have a better idea?

@duglin
Copy link
Contributor

duglin commented Mar 17, 2015

Yep - something like that should work since all we really care about is that the builder output doesn't show up there.

@ahmetb
Copy link
Contributor Author

ahmetb commented Mar 17, 2015

@duglin fixed. thanks.

@ewindisch
Copy link
Contributor

I'm generally okay with this and agree a message should be printed.

@duglin
Copy link
Contributor

duglin commented Mar 18, 2015

@ahmetalpbalkan test are failing

This shows a warning message about adjusted file/directory permission bits
when the `docker build` cli command is executed on windows.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@jessfraz
Copy link
Contributor

LGTM

@jessfraz
Copy link
Contributor

ping @duglin :D

@duglin
Copy link
Contributor

duglin commented Mar 20, 2015

LGTM

duglin added a commit that referenced this pull request Mar 20, 2015
Add cli build warning about chmod bits on windows
@duglin duglin merged commit c536e5b into moby:master Mar 20, 2015
@ahmetb ahmetb deleted the win-cli/build-warning branch March 20, 2015 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants