-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
Please sign your commits following these rules: $ 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. |
Tests are failing because I enabled the |
I think we should leave the entitlements keyword only for granting capabilities, not for enabling features. Maybe:
with possible other more fine-tuned values in the future. And for network
Use the test matrix instead of setting entitlements globally. buildkit/client/client_test.go Lines 107 to 110 in aee28f4
|
thanks, I will get it updated.
sure, if this is the consensus, I'll use this syntax |
ca56db5
to
4271975
Compare
|
||
dockerfile := []byte(` | ||
FROM busybox | ||
RUN --security=insecure [ "$(cat /proc/self/status | grep CapEff)" == "CapEff: 0000003fffffffff" ] |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, @tonistiigi updated
+1 for |
@tiborvass yes |
I'll get it updated to support |
@tonistiigi @tiborvass added |
I will squash commits, just keeping them separate to make it easier to review them. |
Updated |
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 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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>
squashed with docs updated |
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>
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>
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>
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>
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>
This follows syntax suggested in #950.
Example: