Multi stage builds cleanup for Windows by simonferquel · Pull Request #32084 · moby/moby

simonferquel · 2017-03-24T13:35:47Z

This PR improves Windows experience about multi-staged builds.
It fixes #32057

- What I did

Added tests for the forbidden paths rules (today, avoid copying from c:\ / c:\windows and from any relative path that might resolve to c:\ or c:\windows)
Fixed the forbidden paths detection
Added tests ensuring paths are case insensitive on Windows (both for forbidden path matches, and for actually copying a file)

- How I did it

Most of the work is in the form of cli integration tests, forbidden paths rules are in builder/dockerfile/internals.go.

- How to verify it

Already verified by integration tests

- Description for the changelog

Should be part of the multi-staged build change log. Might be usefull to indicate that sources paths on Windows are case insensitive and must be "drive-qualified" (and cannot be c:\ nor c:\windows

simonferquel · 2017-03-24T13:39:31Z

cc @tonistiigi

@jhowardmsft, c:\ and c:\windows are obvious blacklisted paths, but I suspect there can be other files we should not try to copy from on Windows. I suppose that with Hyper-v isolation there must be a swap file somewhere and IIRC, I think it is a system file at the root of file system. Do you know what it is called ? Have you other suggestions for system file blacklisting ?

tonistiigi · 2017-03-24T17:01:37Z

@simonferquel I think should allow the same syntax that regular COPY without --from allows and only restrict specific values. Otherwise, it becomes complicated for the user and COPY doesn't look like a consistent command.

simonferquel · 2017-03-24T17:09:46Z

I just changed that. the restriction list for now is:

.
..
windows
/windows
c:/
c:
c:/windows

That makes it easier for the users, and for us to write common tests between windows and linux engines

lowenna · 2017-03-24T17:20:39Z

I don't understand what this test is about ☹️ Can you explain?

There are some obvious things though - it should be using Execution.MinimalBaseImage rather than hard-coding an image name.

Also why is this hard coding c:? Are those container or host paths?

simonferquel · 2017-03-24T17:43:40Z

@jhowardmsft this tests are in the context of multi-stage builds #31257 (where you can build stuff from a build image, extract them and put them in a runtime image).

The problem, is that if we allow these copy to get the whole image data from the build image, some files appear to be locked and that fails (and currently even seem to hang somewhere). So we want to establish a blacklist of stuff we don't allow to copy from a windows image (for now c:\ and c:\windows are forbidden, as well as relative paths that could resolve to those forbidden paths). The 2 first tests cover this blacklist, the 3rd one ensures that on Windows the COPY --from=context <source> <dest> construct is case insensitive.

I used nanoserver instead of MinimalBaseImage because for now MinimalBaseImage is based on Windows Server Core.

lowenna · 2017-03-24T18:06:47Z

@simonferquel OK, I'm going to have to look at multi-stage build when I get a chance. Haven't looked at it at all.

However, please switch to MinimalBaseImage and not hard-code a name. That's why it's there and is very deliberate.

simonferquel · 2017-03-24T18:12:49Z

done

vdemeester

LGTM 🐯

dnephin

LGTM

tonistiigi · 2017-04-01T00:32:29Z

@jhowardmsft @johnstep Could one of you review as well? I'm not sure how we handle the c: case or if it is supported at all. Didn't spot where we clean it up before path.join.

thaJeztah

left some questions

thaJeztah · 2017-04-02T00:24:53Z

integration-cli/docker_cli_build_test.go

Does WORKDIR affect the "base" path?

For now it is not. We should document COPY --from in this way

thaJeztah · 2017-04-02T00:25:39Z

builder/dockerfile/internals.go

For comparing; would it be better to use filepath.Abs(origPath)?

What does "Abs" exactly, does it depend on WORKDIR ?

thaJeztah · 2017-04-02T00:28:08Z

builder/dockerfile/internals.go

This looks messy. Perhaps better to define a "blacklist" slice blacklist := []string{".", "..", ....}, and iterate?

Maybe this instead of iterating:

switch p { case ".", "..", "\\", "\\windows", "windows", "c:", "c:.", "c:\\", "c:\\windows", "c:\\windows\\": return nil, errors.Errorf("copy from %q is not allowed on windows", p) }

I like the blacklist idea, it would be much easier to modify

thaJeztah · 2017-04-02T00:39:34Z

builder/dockerfile/internals.go

Do we want to blacklist exactly c:\windows\ or also, e.g. c:\windows\system32? If the latter, we should match on prefix (or another way to check if p is inside "forbiddenpath".

What about symlinks?

We might want to have a more precise blacklist. I know that I dont want c:\windows to be completely copied (it might fail on some system files anyway), I also know that some files at FS root should not be copied as well, I am not sure about system32 though. We might want to allow to copy some dlls from there. The idea of this PR is to help the user avoid to do obvious WRONG things (like copying the 4GB of Windows Server Core) on Windows.

thaJeztah · 2017-04-02T00:41:08Z

Also, could you squash your commits?

simonferquel · 2017-04-07T12:34:21Z

@thaJeztah I squashed and I changed the path analysis to use a blacklist with only absolute paths, and a logic that reconstructs the absolute path assuming WORKDIR is c:\ (which is the case on COPY --from)

thaJeztah

Left some thoughts, but possibly someone else could have a look if they make sense 😅

thaJeztah · 2017-04-07T13:47:37Z

builder/dockerfile/internals.go

perhaps

windowsBlacklist := []string{ "c:\\", "c:\\windows" }

I'll go with the map solution to avoid looping (might become annoying if we put many system paths in the blacklist)

thaJeztah · 2017-04-07T13:54:31Z

builder/dockerfile/internals.go

Not on a Windows machine, but instead of manually making this path absolute, would this work?

p := strings.ToLower(filepath.Abs(origPath)) for _, forbidden := range windowsBlacklist { if p == forbidden { return nil, errors.New("copy from c:\\ or c:\\windows is not allowed on windows") }

To prevent the loop, perhaps;

windowsBlacklist := map[string]interface{}{ "c:\\" : true, "c:\\windows" : true, } if _, ok := windowsBlacklist[p]; ok { return nil, errors.New("copy from "+p+" is not allowed on windows") }

Done
Problem of filepath.Abs is that it works relative to the current process workdir. This is not what we want (we know the path is relative to the context's "c:")

thaJeztah · 2017-04-07T13:54:55Z

builder/dockerfile/internals.go

These checks are no longer needed?

It is not anymore (the blacklist replaces it)

Paths resolving to c:\ or c:\windows are forbidden Replaced the obscure (and non-working) regex with a simple case insensitive comparison to the black listed paths (we should forbid c:\, c:\windows but not d:\) Also, add a test ensuring paths are case insensitive on windows Also, made sure existing multi-staged build tests pass on windows Signed-off-by: Simon Ferquel <[email protected]>

thaJeztah

LGTM

GordonTheTurtle added the status/0-triage label Mar 24, 2017

vdemeester added status/2-code-review and removed status/0-triage labels Mar 24, 2017

AkihiroSuda added the status/failing-ci Indicates that the PR in its current state fails the test suite label Mar 24, 2017

simonferquel mentioned this pull request Mar 24, 2017

Add support for named build stages #32063

Merged

simonferquel force-pushed the multi-stage-builds-windows branch from 009c3e5 to f0a5c5f Compare March 24, 2017 18:03

vdemeester requested review from dnephin, thaJeztah and tonistiigi March 31, 2017 08:37

vdemeester approved these changes Mar 31, 2017

View reviewed changes

simonferquel force-pushed the multi-stage-builds-windows branch from 755c548 to 7141824 Compare March 31, 2017 08:45

dnephin approved these changes Mar 31, 2017

View reviewed changes

thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Apr 2, 2017

thaJeztah requested changes Apr 2, 2017

View reviewed changes

vdemeester requested review from johnstep and lowenna April 2, 2017 10:32

simonferquel force-pushed the multi-stage-builds-windows branch from 7141824 to 9524a60 Compare April 7, 2017 12:32

simonferquel force-pushed the multi-stage-builds-windows branch from 9524a60 to f530ee8 Compare April 7, 2017 12:37

thaJeztah requested changes Apr 7, 2017

View reviewed changes

simonferquel force-pushed the multi-stage-builds-windows branch from f530ee8 to b0e7588 Compare April 7, 2017 16:00

GordonTheTurtle assigned vdemeester Apr 8, 2017

thaJeztah approved these changes Apr 10, 2017

View reviewed changes

vdemeester added the rebuild/experimental label Apr 10, 2017

GordonTheTurtle removed the rebuild/experimental label Apr 10, 2017

vdemeester added rebuild/experimental status/4-merge and removed status/2-code-review labels Apr 10, 2017

GordonTheTurtle removed the rebuild/experimental label Apr 10, 2017

vdemeester merged commit a48f425 into moby:master Apr 10, 2017

GordonTheTurtle added this to the 17.05.0 milestone Apr 10, 2017

Conversation

simonferquel commented Mar 24, 2017

Uh oh!

simonferquel commented Mar 24, 2017

Uh oh!

tonistiigi commented Mar 24, 2017

Uh oh!

simonferquel commented Mar 24, 2017

Uh oh!

lowenna commented Mar 24, 2017

Uh oh!

simonferquel commented Mar 24, 2017

Uh oh!

lowenna commented Mar 24, 2017

Uh oh!

simonferquel commented Mar 24, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Apr 1, 2017

Uh oh!

thaJeztah 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!

thaJeztah commented Apr 2, 2017

Uh oh!

simonferquel commented Apr 7, 2017

Uh oh!

thaJeztah 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!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants