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

Implement frontend support for RUN --security=insecure #1081

Merged
merged 1 commit into from
Jul 22, 2019

Conversation

smira
Copy link
Collaborator

@smira smira commented Jul 16, 2019

This follows syntax suggested in #950.

Example:

RUN --security=insecure cat /proc/self/status | grep CapEff
#84 0.093 CapEff:	0000003fffffffff

@GordonTheTurtle
Copy link
Collaborator

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "frontend-ents" git@github.com:smira/buildkit.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@smira
Copy link
Collaborator Author

smira commented Jul 16, 2019

Tests are failing because I enabled the security.insecure entitlement for dockerfile tests to pass.

@tonistiigi
Copy link
Member

I think we should leave the entitlements keyword only for granting capabilities, not for enabling features.

Maybe:

RUN --security=insecure

with possible other more fine-tuned values in the future.

And for network

RUN --net=host|none

Tests are failing because I enabled the security.insecure entitlement for dockerfile tests to pass.

Use the test matrix instead of setting entitlements globally.

integration.WithMatrix("secmode", map[string]interface{}{
"sandbox": securitySandbox,
"insecure": securityInsecure,
}),

@AkihiroSuda @tiborvass @justincormack @kunalkushwaha

@smira
Copy link
Collaborator Author

smira commented Jul 16, 2019

Use the test matrix instead of setting entitlements globally.

thanks, I will get it updated.

Maybe:
RUN --security=insecure

sure, if this is the consensus, I'll use this syntax

@smira smira force-pushed the frontend-ents branch 2 times, most recently from ca56db5 to 4271975 Compare July 17, 2019 17:17
@smira smira changed the title Implement frontend support for RUN --ents=... Implement frontend support for RUN --security=insecure Jul 17, 2019
frontend/dockerfile/docs/experimental.md Outdated Show resolved Hide resolved
frontend/dockerfile/docs/experimental.md Outdated Show resolved Hide resolved
frontend/dockerfile/dockerfile_runsecurity_test.go Outdated Show resolved Hide resolved

dockerfile := []byte(`
FROM busybox
RUN --security=insecure [ "$(cat /proc/self/status | grep CapEff)" == "CapEff: 0000003fffffffff" ]
Copy link
Member

@tonistiigi tonistiigi Jul 17, 2019

Choose a reason for hiding this comment

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

Make sure there is a test that checks that this is not set when --security=insecure is not specified(even if entitlement is allowed).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added another test for that, when entitlements are allowed, solver fails if insecure is not enabled server side still

Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe CapBnd would be slightly better to check instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, @tonistiigi updated

@tiborvass
Copy link
Collaborator

+1 for RUN --security=insecure. @tonistiigi Should we support sandbox to allow RUN --security=$param ?

@tonistiigi
Copy link
Member

@tiborvass yes

@smira
Copy link
Collaborator Author

smira commented Jul 17, 2019

@tiborvass yes

I'll get it updated to support =sandbox

@smira
Copy link
Collaborator Author

smira commented Jul 17, 2019

@tonistiigi @tiborvass added security=sandbox

@smira
Copy link
Collaborator Author

smira commented Jul 17, 2019

I will squash commits, just keeping them separate to make it easier to review them.

@smira
Copy link
Collaborator Author

smira commented Jul 18, 2019

Updated CapEff -> CapBnd, squashed commits (no other changes)

smira added a commit to smira/talos that referenced this pull request Jul 18, 2019
This relies on two PRs to the buildkit, which aren't merged yet, so I
had to do some overrides to apply them:

* moby/buildkit#1081
* moby/buildkit#1085

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
smira added a commit to smira/talos that referenced this pull request Jul 18, 2019
This relies on two PRs to the buildkit, which aren't merged yet, so I
had to do some overrides to apply them:

* moby/buildkit#1081
* moby/buildkit#1085

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
This runs the command without sandbox in insecure mode, which allows to run flows requiring elevated
privileges (e.g. containerd). This is equivalent to running `docker run --privileged`.

Default sandbox mode can be activated via `--security=sandbox`, but that is no-op.
Copy link
Member

Choose a reason for hiding this comment

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

add: In order to build Dockerfiles with --security=insecure user needs to enable security.insecure entitlement when starting the buildkit daemon and allow it for a specific build request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated!

@@ -138,3 +138,21 @@ $ buildctl build --frontend=dockerfile.v0 --local context=. --local dockerfile=.
You can also specify a path to `*.pem` file on the host directly instead of `$SSH_AUTH_SOCK`.
However, pem files with passphrases are not supported.

### RUN --security=insecure
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer --security=sandbox|insecure for completeness. You can tweak the next sentence a bit so that it only applies if the value is set to "insecure".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated!

Example:

    RUN --security=insecure cat /proc/self/status | grep CapEff
    moby#84 0.093 CapEff:	0000003fffffffff

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
@smira
Copy link
Collaborator Author

smira commented Jul 18, 2019

squashed with docs updated

smira added a commit to smira/talos that referenced this pull request Jul 19, 2019
This relies on two PRs to the buildkit:

* moby/buildkit#1081
* moby/buildkit#1085

Sysfs fix was merged to upstream, so updated tag, while using
`Dockerfile` slug I can switch to dockerfile2llb with support for
`--security=insecure`.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
andrewrynhard pushed a commit to siderolabs/talos that referenced this pull request Jul 19, 2019
This relies on two PRs to the buildkit:

* moby/buildkit#1081
* moby/buildkit#1085

Sysfs fix was merged to upstream, so updated tag, while using
`Dockerfile` slug I can switch to dockerfile2llb with support for
`--security=insecure`.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
@tonistiigi tonistiigi merged commit 7e31f58 into moby:master Jul 22, 2019
smira added a commit to smira/talos that referenced this pull request Jul 23, 2019
This allows to do `make test TESTPKGS=./internal/app/machined`.

Also update Dockerfile slug as
moby/buildkit#1081 was merged into master.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
smira added a commit to smira/talos that referenced this pull request Jul 23, 2019
This allows to do `make test TESTPKGS=./internal/app/machined`.

Also update Dockerfile slug as
moby/buildkit#1081 was merged into master.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
smira added a commit to siderolabs/talos that referenced this pull request Jul 23, 2019
This allows to do `make test TESTPKGS=./internal/app/machined`.

Also update Dockerfile slug as
moby/buildkit#1081 was merged into master.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
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.

5 participants