security entitlement support #570

kunalkushwaha · 2018-08-14T02:46:54Z

This is just a rough draft for review of approach.
[edit] rebased with #560

What this PR includes:

entitlement package based on libentitlement project.
usage of entitlement package for setting security settings.
rest of flow of passing information of entitlements based on PR solver: net host with basic entitlements support #560

kunalkushwaha · 2018-08-15T02:29:07Z

tonistiigi

I don't see a reason for copying code from libentitlement. It will be impossible to maintain it like this. It is unclear to me if there are any changes in that code. We should either use it directly(at least a fork if there are changes) or implement the unconfined mode manually without the help of these packages.

fyi @justincormack

tonistiigi · 2018-08-24T21:00:32Z

client/llb/state.go

@@ -257,6 +258,13 @@ func (s State) Network(n pb.NetMode) State {
 func (s State) GetNetwork() pb.NetMode {
 	return getNetwork(s)
 }
+func (s State) Security(n pb.SecMode) State {


Add RunOption version as well.

tonistiigi · 2018-08-24T21:02:41Z

frontend/dockerfile/builder/build.go

@@ -40,6 +40,7 @@ const (
 	keyImageResolveMode   = "image-resolve-mode"
 	keyGlobalAddHosts     = "add-hosts"
 	keyForceNetwork       = "force-network-mode"
+	keyForceSecurity      = "force-security-mode"


lets not add this yet. Can be a follow-up.

tonistiigi · 2018-08-24T21:05:27Z

solver/llbsolver/solver.go

@@ -269,13 +269,17 @@ func notifyCompleted(ctx context.Context, v *client.Vertex, err error, cached bo
 	pw.Write(v.Digest.String(), *v)
 }

-var AllowNetworkHostUnstable = false // TODO: enable in constructor
+var AllowNetworkHostUnstable = false       // TODO: enable in constructor
+var AllowSecurityUnconfinedUnstable = true // TODO: enable in constructor


Leave this to false or add the constructor initialization in this PR.

tonistiigi · 2018-08-24T21:10:13Z

solver/llbsolver/solver.go


 func supportedEntitlements() []entitlements.Entitlement {
 	out := []entitlements.Entitlement{} // nil means no filter
 	if AllowNetworkHostUnstable {
 		out = append(out, entitlements.EntitlementNetworkHost)
 	}
+	if AllowSecurityUnconfinedUnstable {
+		out = append(out, entitlements.EntitlementSecurityUnconfined)


Where is this validated? I think it is missing the condition in ValidateEntitlements()

tonistiigi · 2018-08-24T21:15:32Z

executor/oci/spec_unix.go


-	s.Mounts = GetMounts(ctx,
+	ociProfile := entitlements.NewOCIProfile(s, meta.SecMode.String())
+	ociProfile, err = ociProfile.SecurityUnconfined()


This seems to be always called (and why the double call) atm.

kunalkushwaha · 2018-08-27T09:00:26Z

other simple alternative could be, build unconfined secomp profile as on lines defaultProfile() defined in containerd and remove whole libentitlement code.

But at some point if more entitlements are required a sub library to define all entitlements will be easy to manage.

kunalkushwaha · 2018-08-28T08:27:11Z

The entitlements package from libentitlement is removed and replaced by a simple function based on defaultSecompProfile of containerd.
fb002f5#diff-20b4a13522a5dac1bba70cce7b8e4e44R14

This function is required to reviewed, if its fit for security.unconfined profile.

tonistiigi · 2018-08-28T17:02:30Z

executor/oci/spec_unix.go

+			opts = append(opts, seccomp.WithDefaultProfile())
+		}
+		if meta.SecMode == pb.SecMode_UNCONFINED {
+			opts = append(opts, entitlements.WithDefaultAdminProfile())


This should add all caps, disable seccomp/apparmor completely (no seccompsupported check), remove maskpaths and readonly overrides and change sys mounts to rw.

tonistiigi · 2018-08-30T17:13:38Z

util/entitlements/security.go

+			s.Process.Capabilities.Inheritable = append(s.Process.Capabilities.Inheritable, cap)
+			s.Process.Capabilities.Permitted = append(s.Process.Capabilities.Permitted, cap)
+		}
+		s.Linux.Seccomp = DefaultAdminProfile(s)


For the unconfied, we can just skip it and not set seccomp at all. For security.admin (future) we would use this profile.

tonistiigi · 2018-08-30T17:22:03Z

solver/llbsolver/solver.go

@@ -269,13 +269,17 @@ func notifyCompleted(ctx context.Context, v *client.Vertex, err error, cached bo
 	pw.Write(v.Digest.String(), *v)
 }

-var AllowNetworkHostUnstable = false // TODO: enable in constructor
+var AllowNetworkHostUnstable = false        // TODO: enable in constructor
+var AllowSecurityUnconfinedUnstable = false // TODO: enable in constructor


@kunalkushwaha Could you implement this as well so we can have an integration test or would you prefer that to be a follow-up? Something like buildkitd --allow-insecure-entitlement=foo.

I am working on this, shall update the PR on Monday.

Facing issue while buildctl build --allow=security.unconfined is passed and security.confined is also set by default.

kunalkushwaha · 2018-09-05T01:40:52Z

Have updated the code with test case and buildkitd flags to enable unsecured entitlements like network.host and security.unconfined.

There is missing something for both network.host & security.unconfined, even from client or from code llb.Security(llb.Security(llb.SecurityModeUnconfined)) is enabled, the Executor always receive default values i.e. security.confined & network.host.

Also, why dispatchRun() is never invoked.
Please help on this.

tonistiigi

@kunalkushwaha The issue seems to be that you are not calling WithDefaultAdminProfile anywhere (should be WithUnconfinedSecurityProfile)

tonistiigi · 2018-09-06T04:39:22Z

cmd/buildkitd/main.go

+		for _, e := range ents {
+			switch e {
+			case "security.unconfined":
+				llbsolver.AllowSecurityUnconfinedUnstable = true


Don't use these variables anymore. Instead, add them to a config array and pass to the solver constructor.

@kunalkushwaha

tonistiigi · 2018-09-06T04:43:11Z

cmd/buildkitd/main.go

@@ -207,6 +212,18 @@ func main() {

 		controller.Register(server)

+		ents := c.GlobalStringSlice("allow-insecure-entitlement")


Add this to config and override with the flag.

@kunalkushwaha

tonistiigi · 2018-09-06T04:51:37Z

executor/executor.go

@@ -18,6 +18,7 @@ type Meta struct {
 	ReadonlyRootFS bool
 	ExtraHosts     []HostIP
 	NetMode        pb.NetMode
+	SecMode        pb.SecMode


SecurityMode

tonistiigi · 2018-09-06T04:58:13Z

util/entitlements/security.go

+// Note: must follow the setting of process capabilities
+func WithDefaultAdminProfile() oci.SpecOpts {
+	return func(_ context.Context, _ oci.Client, _ *containers.Container, s *specs.Spec) error {
+		addCaps := []string{


This is still a limited list. For unconfined use all caps(different from security.admin).

ping @kunalkushwaha

And no seccomp for unconfined.

tonistiigi · 2018-09-14T01:48:24Z

@kunalkushwaha Ping. Did my answer clear things up or did I miss anything?

kunalkushwaha · 2018-09-14T01:52:05Z

@tonistiigi Sorry for delay.
Yes its clear, was working on some other stuff so just got delayed. Will update the PR soon.

PS: I will be on leave for 2 weeks from tomorrow.

AkihiroSuda · 2018-10-30T08:40:15Z

@kunalkushwaha What's current status?

kunalkushwaha · 2018-11-02T01:28:00Z

@AkihiroSuda Sorry have not been able to resume work on this.. Shall start from Monday.

kunalkushwaha · 2018-11-22T08:54:39Z

Hi,
Sorry for delay in resuming the work.

I need some help in understanding how the entitlements can passed from client (buildctl build) workers of buildkitd

Current approach in this PR.

The entitlement information in passed with --allow flag e.g buildctl build --allow security.unconfined
store entitlements information in SolveOpt
This is further passed ControleClient SoleveRequest in function client.solve()
similar to step 3, the entitlements is passed to solver with frontend.SolveRequest
Problem arises here, when llbbridge.Solve() passes only frontendOpt of SolveRequest

When control comes to frontend/builder/build.Build() via GatewayForwarder.Solve the options passed from client can be retrieved through BuildOpts() i.e. Opts and LLBCaps and rest of information is lost.

so currently defaultSecurityMode have only default security.confined & network.none entitlements.

I want to use the LLBCaps as it will be defined for both network and security entitlements.

The point where I am stuck is , where to set these LLBCaps, i.e. CapExecMetaSecurity, So this can be access in builder/build.Buid() function?

@tonistiigi @AkihiroSuda

tonistiigi · 2018-12-02T16:28:06Z

@kunalkushwaha Why do you need to pass them with frontend.Solve() at all. They are only validated on vertex load and the bridge that is doing the loading already has all of this information initialized to the job in

buildkit/solver/llbsolver/solver.go

Line 103 in 5b7f759

j.SetValue(keyEntitlements, set)

kunalkushwaha · 2018-12-03T08:06:43Z

@tonistiigi I want to retrieve the value of entitlements in executor.Exec() functions. I am not getting what should be ideal way of retrieving.

If possible, please give me a overview of control flow how it goes once control comes to buildkitd for any build command.

tonistiigi · 2018-12-10T11:37:21Z

The value you need in exec is not entitlements that is passed with the build and enabled from daemon. What you need there is only the SecMode that is defined in the Meta and available to you. The validation that this secmode value is allowed to be used with current entitlement settings has already happened before that.

There are 2 sets of values here. One is entitlements that define the allowed actions a current build can perform. The other is the actual action that will be done for specific vertexes. The entitlements only affects the validation of the actual actions. Eg. if you just pass entitlements but don't change your vertexes, nothing would change in your build.

tonistiigi · 2019-03-01T19:34:50Z

@kunalkushwaha Haven't reviewed yet but the CI seems to be failing on vendor check.

tonistiigi · 2019-03-01T19:37:00Z

client/client_test.go

+	secMode := sb.Value("secmode")
+
+	if secMode != securityUnconfined {
+		t.Skip()


We should check that caps are properly restricted in securityConfined mode.

kunalkushwaha · 2019-03-04T03:56:07Z

Updated the testcase for SecurityConfined check and fixed CI issue.

kunalkushwaha · 2019-03-06T00:27:54Z

@tonistiigi PTAL. Have fixed the testcase with check for Confined & Unconfined both. Also CI issue is resolved.

AkihiroSuda · 2019-03-19T05:05:15Z

util/entitlements/security_linux.go

+		}
+		s.Linux.ReadonlyPaths = []string{}
+		s.Linux.MaskedPaths = []string{}
+		s.Process.ApparmorProfile = ""


remove seccomp profile as well?

yes, it was discussed here #570 (comment).
I am updating function comments too.

AkihiroSuda · 2019-03-19T05:06:00Z

sorry needs rebase

kunalkushwaha · 2019-03-19T08:10:05Z

Its rebased and CI error gone.

tiborvass · 2019-03-21T03:15:44Z

solver/pb/ops.pb.go

+
+var SecurityMode_name = map[int32]string{
+	0: "CONFINED",
+	1: "UNCONFINED",


@justincormack Would you be okay calling unconfined insecure instead? I know all security profiles call it unconfined, but the user intent feels clearer with insecure.

Yes I think that makes sense, although confined vs insecure is kind of weird, and I don't think we should call it secure vs insecure...

@justincormack sandbox/sandboxed ? I'm not too bothered by having both confined and insecure, because those who will see confined will probably have read about its technical meaning, whereas insecure should mean something strong to those who won't read much about it.

@justincormack told me offline he prefers confined and insecure.

@kunalkushwaha would you mind updating UNCONFINED to INSECURE everywhere in this PR? We can keep the word CONFINED.

btw, I like sandbox/insecure pairing

ok with sandbox/insecure too.

@tiborvass ^^

Perfect let’s roll with sandbox/insecure then. Do you mind updating @kunalkushwaha

I will update with sandbox/insecure.

tonistiigi

Looks good. Small comments

tonistiigi · 2019-03-25T17:59:06Z

client/client_test.go

+	require.NoError(t, err)
+
+	_, err = c.Solve(context.TODO(), def, SolveOpt{
+		AllowedEntitlements: []entitlements.Entitlement{entitlement},


Please add a case where you don't pass the entitlement and get an error on securityUnconfined mode and where you try to pass entitlement on securityConfined and get an error as well.

tonistiigi · 2019-03-25T18:00:47Z

cmd/buildkitd/main.go

@@ -159,6 +159,10 @@ func main() {
 			Usage: "ca certificate to verify clients",
 			Value: defaultConf.GRPC.TLS.CA,
 		},
+		cli.StringSliceFlag{
+			Name:  "allow-insecure-entitlement",
+			Usage: "allows insecure entitlements like network.host & security.unconfined",


like network.host & security.unconfined -> (eg. network.host, security.unconfined)

tonistiigi · 2019-03-25T20:58:38Z

util/entitlements/security_linux.go

+
+		for _, m := range s.Mounts {
+			if m.Type == "sysfs" {
+				m.Options = []string{"nosuid", "noexec", "nodev", "rw"}


This is being reset by the oci.GetMounts call. Probably move it to that function instead for a fix.

tonistiigi · 2019-03-25T23:48:26Z

Now that I think of it, I think it was a mistake to add "default entitlements"

buildkit/util/entitlements/entitlements.go

Line 21 in 2453e6f

var defaults = map[Entitlement]struct{}{

. I think you should just remove EntitlementSecurityConfined cause defining that doesn't make sense in any case. You still need Security(SecurityModeConfined) though, just allowing the entitlement is pointless because it is allowed always anyway. All the functionality remains, just remove the constant. Same goes for network-none but if you want I can clean that up myself after this PR is merged.

edit: I guess I took it from https://github.com/moby/libentitlement but doesn't really fit the design. These should more be like the configuration templates for the executor, not capabilities user needs to give access to.

Signed-off-by: Kunal Kushwaha <[email protected]>

Signed-off-by: Tonis Tiigi <[email protected]> Signed-off-by: Kunal Kushwaha <[email protected]>

kunalkushwaha · 2019-03-27T05:39:53Z

Updated the PR with changes including.

Confined/Unconfined changed to Sandbox/Insecure security entitlement support #570 (comment)
removed constants for SecurityConfined & NetworkNone and there is no default entitlement now. security entitlement support #570 (comment)
testcase added for error checks for entitlements as suggested in security entitlement support #570 (comment)
moved logic for sysfs mount in r/w mode in GenerateSpec() security entitlement support #570 (comment)

tonistiigi

Thanks @kunalkushwaha , LGTM

PTAL @AkihiroSuda

kunalkushwaha force-pushed the entirlement-revised branch from 774f7b1 to 560cf5c Compare August 17, 2018 05:59

tonistiigi reviewed Aug 24, 2018

View reviewed changes

kunalkushwaha force-pushed the entirlement-revised branch from 560cf5c to 19a2961 Compare August 27, 2018 08:55

kunalkushwaha force-pushed the entirlement-revised branch 3 times, most recently from 9f12f29 to fb002f5 Compare August 28, 2018 08:24

kunalkushwaha force-pushed the entirlement-revised branch 2 times, most recently from dee930c to e08ef67 Compare August 28, 2018 08:58

tonistiigi reviewed Aug 28, 2018

View reviewed changes

kunalkushwaha force-pushed the entirlement-revised branch from e08ef67 to b11b78b Compare August 29, 2018 07:35

tonistiigi reviewed Aug 30, 2018

View reviewed changes

kunalkushwaha force-pushed the entirlement-revised branch 2 times, most recently from f01c803 to 50c12a4 Compare September 5, 2018 01:25

tonistiigi reviewed Sep 6, 2018

View reviewed changes

tonistiigi added the status/do-not-merge label Sep 6, 2018

tonistiigi mentioned this pull request Oct 5, 2018

hack: update test dockerfiles to buildkit #666

Merged

kunalkushwaha force-pushed the entirlement-revised branch from 50c12a4 to 2472d41 Compare December 12, 2018 08:30

tonistiigi removed the status/do-not-merge label Mar 1, 2019

tonistiigi reviewed Mar 1, 2019

View reviewed changes

kunalkushwaha force-pushed the entirlement-revised branch 2 times, most recently from 1982a30 to e9b1f19 Compare March 4, 2019 03:29

kunalkushwaha force-pushed the entirlement-revised branch from e9b1f19 to 5d52724 Compare March 6, 2019 07:22

AkihiroSuda added this to the v0.5.0 milestone Mar 13, 2019

AkihiroSuda reviewed Mar 19, 2019

View reviewed changes

kunalkushwaha force-pushed the entirlement-revised branch 3 times, most recently from 2445d51 to d40781c Compare March 19, 2019 07:28

tiborvass reviewed Mar 21, 2019

View reviewed changes

kunalkushwaha force-pushed the entirlement-revised branch from d40781c to 0c9dd65 Compare March 22, 2019 02:11

tonistiigi reviewed Mar 25, 2019

View reviewed changes

kunalkushwaha force-pushed the entirlement-revised branch from 0c9dd65 to 4fd84a6 Compare March 27, 2019 03:23

proto defination for security entitlements support

86c7933

Signed-off-by: Kunal Kushwaha <[email protected]>

kunalkushwaha force-pushed the entirlement-revised branch 2 times, most recently from 8fc7880 to 01a8de7 Compare March 27, 2019 04:56

kunalkushwaha and others added 2 commits March 27, 2019 13:57

security entitlement support

a2bbb5f

Signed-off-by: Kunal Kushwaha <[email protected]>

integration: secmode switch support and security testcase

01a8de7

Signed-off-by: Tonis Tiigi <[email protected]> Signed-off-by: Kunal Kushwaha <[email protected]>

tonistiigi approved these changes Mar 27, 2019

View reviewed changes

AkihiroSuda approved these changes Mar 28, 2019

View reviewed changes

tonistiigi merged commit 758c61e into moby:master Mar 28, 2019

kunalkushwaha mentioned this pull request Apr 19, 2019

Implement entitlement security.unconfined #950

Closed

		@@ -207,6 +212,18 @@ func main() {

		controller.Register(server)

		ents := c.GlobalStringSlice("allow-insecure-entitlement")

security entitlement support #570

security entitlement support #570

Conversation

kunalkushwaha commented Aug 14, 2018 • edited Loading

kunalkushwaha commented Aug 15, 2018

tonistiigi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalkushwaha commented Aug 27, 2018

kunalkushwaha commented Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalkushwaha commented Sep 5, 2018

tonistiigi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi commented Sep 14, 2018

kunalkushwaha commented Sep 14, 2018

AkihiroSuda commented Oct 30, 2018

kunalkushwaha commented Nov 2, 2018

kunalkushwaha commented Nov 22, 2018

tonistiigi commented Dec 2, 2018

kunalkushwaha commented Dec 3, 2018 • edited Loading

tonistiigi commented Dec 10, 2018

tonistiigi commented Mar 1, 2019

Choose a reason for hiding this comment

kunalkushwaha commented Mar 4, 2019

kunalkushwaha commented Mar 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Mar 19, 2019

kunalkushwaha commented Mar 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi left a comment

Choose a reason for hiding this comment

tonistiigi Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi commented Mar 25, 2019 • edited Loading

kunalkushwaha commented Mar 27, 2019 • edited Loading

tonistiigi left a comment

Choose a reason for hiding this comment

kunalkushwaha commented Aug 14, 2018 •

edited

Loading

kunalkushwaha commented Aug 28, 2018 •

edited

Loading

kunalkushwaha commented Dec 3, 2018 •

edited

Loading

tonistiigi Mar 25, 2019 •

edited

Loading

tonistiigi commented Mar 25, 2019 •

edited

Loading

kunalkushwaha commented Mar 27, 2019 •

edited

Loading