From 2d6ae5c90ba239e2835ad99d820c163bf77c9ff9 Mon Sep 17 00:00:00 2001 From: CrazyMax <1951866+crazy-max@users.noreply.github.com> Date: Wed, 21 Feb 2024 11:29:44 +0100 Subject: [PATCH] driver: set network.host entitlement by default for container drivers Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com> --- builder/builder.go | 55 +++++++++++--- builder/builder_test.go | 113 +++++++++++++++++++++++++++++ driver/docker-container/factory.go | 3 - tests/inspect.go | 31 ++++++++ 4 files changed, 188 insertions(+), 14 deletions(-) diff --git a/builder/builder.go b/builder/builder.go index 74bee7b6ee86..dbd7d4cfcc02 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -26,6 +26,7 @@ import ( "github.com/google/shlex" "github.com/moby/buildkit/util/progress/progressui" "github.com/pkg/errors" + "github.com/spf13/pflag" "golang.org/x/sync/errgroup" ) @@ -429,12 +430,14 @@ func Create(ctx context.Context, txn *store.Txn, dockerCli command.Cli, opts Cre } } - var buildkitdFlags []string - if opts.BuildkitdFlags != "" { - buildkitdFlags, err = shlex.Split(opts.BuildkitdFlags) - if err != nil { - return nil, errors.Wrap(err, "failed to parse BuildKit daemon flags") - } + driverOpts, err := csvToMap(opts.DriverOpts) + if err != nil { + return nil, err + } + + buildkitdFlags, err := parseBuildkitdFlags(opts.BuildkitdFlags, driverName, driverOpts) + if err != nil { + return nil, err } var ep string @@ -493,11 +496,6 @@ func Create(ctx context.Context, txn *store.Txn, dockerCli command.Cli, opts Cre setEp = false } - driverOpts, err := csvToMap(opts.DriverOpts) - if err != nil { - return nil, err - } - buildkitdConfigFile := opts.BuildkitdConfigFile if buildkitdConfigFile == "" { // if buildkit daemon config is not provided, check if the default one @@ -642,3 +640,38 @@ func validateBuildkitEndpoint(ep string) (string, error) { } return ep, nil } + +// parseBuildkitdFlags parses buildkit flags +func parseBuildkitdFlags(inp string, driver string, driverOpts map[string]string) (res []string, err error) { + if inp != "" { + res, err = shlex.Split(inp) + if err != nil { + return nil, errors.Wrap(err, "failed to parse buildkit flags") + } + } + + var allowInsecureEntitlements []string + flags := pflag.NewFlagSet("buildkitd", pflag.ContinueOnError) + flags.Usage = func() {} + flags.StringArrayVar(&allowInsecureEntitlements, "allow-insecure-entitlement", nil, "") + _ = flags.Parse(res) + + var hasNetworkHostEntitlement bool + for _, e := range allowInsecureEntitlements { + if e == "network.host" { + hasNetworkHostEntitlement = true + break + } + } + + if v, ok := driverOpts["network"]; ok && v == "host" && !hasNetworkHostEntitlement && driver == "docker-container" { + // always set network.host entitlement if user has set network=host + res = append(res, "--allow-insecure-entitlement=network.host") + } else if len(allowInsecureEntitlements) == 0 && (driver == "kubernetes" || driver == "docker-container") { + // set network.host entitlement if user does not provide any as + // network is isolated for container drivers. + res = append(res, "--allow-insecure-entitlement=network.host") + } + + return res, nil +} diff --git a/builder/builder_test.go b/builder/builder_test.go index 853e8069a555..46037affbb90 100644 --- a/builder/builder_test.go +++ b/builder/builder_test.go @@ -3,6 +3,7 @@ package builder import ( "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -24,3 +25,115 @@ func TestCsvToMap(t *testing.T) { require.Contains(t, r, "namespace") require.Equal(t, r["namespace"], "default") } + +func TestParseBuildkitdFlags(t *testing.T) { + testCases := []struct { + name string + flags string + driver string + driverOpts map[string]string + expected []string + wantErr bool + }{ + { + "docker-container no flags", + "", + "docker-container", + nil, + []string{ + "--allow-insecure-entitlement=network.host", + }, + false, + }, + { + "kubernetes no flags", + "", + "kubernetes", + nil, + []string{ + "--allow-insecure-entitlement=network.host", + }, + false, + }, + { + "remote no flags", + "", + "remote", + nil, + nil, + false, + }, + { + "docker-container with insecure flag", + "--allow-insecure-entitlement=security.insecure", + "docker-container", + nil, + []string{ + "--allow-insecure-entitlement=security.insecure", + }, + false, + }, + { + "docker-container with insecure and host flag", + "--allow-insecure-entitlement=network.host --allow-insecure-entitlement=security.insecure", + "docker-container", + nil, + []string{ + "--allow-insecure-entitlement=network.host", + "--allow-insecure-entitlement=security.insecure", + }, + false, + }, + { + "docker-container with network host opt", + "", + "docker-container", + map[string]string{"network": "host"}, + []string{ + "--allow-insecure-entitlement=network.host", + }, + false, + }, + { + "docker-container with host flag and network host opt", + "--allow-insecure-entitlement=network.host", + "docker-container", + map[string]string{"network": "host"}, + []string{ + "--allow-insecure-entitlement=network.host", + }, + false, + }, + { + "docker-container with insecure, host flag and network host opt", + "--allow-insecure-entitlement=network.host --allow-insecure-entitlement=security.insecure", + "docker-container", + map[string]string{"network": "host"}, + []string{ + "--allow-insecure-entitlement=network.host", + "--allow-insecure-entitlement=security.insecure", + }, + false, + }, + { + "error parsing flags", + "foo'", + "docker-container", + nil, + nil, + true, + }, + } + for _, tt := range testCases { + tt := tt + t.Run(tt.name, func(t *testing.T) { + flags, err := parseBuildkitdFlags(tt.flags, tt.driver, tt.driverOpts) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.expected, flags) + }) + } +} diff --git a/driver/docker-container/factory.go b/driver/docker-container/factory.go index b4c1189c378c..74ba34a86019 100644 --- a/driver/docker-container/factory.go +++ b/driver/docker-container/factory.go @@ -55,9 +55,6 @@ func (f *factory) New(ctx context.Context, cfg driver.InitConfig) (driver.Driver switch { case k == "network": d.netMode = v - if v == "host" { - d.InitConfig.BuildkitdFlags = append(d.InitConfig.BuildkitdFlags, "--allow-insecure-entitlement=network.host") - } case k == "image": d.image = v case k == "memory": diff --git a/tests/inspect.go b/tests/inspect.go index c7c4df096be7..bb513d7e9c7a 100644 --- a/tests/inspect.go +++ b/tests/inspect.go @@ -17,6 +17,7 @@ func inspectCmd(sb integration.Sandbox, opts ...cmdOpt) (string, error) { var inspectTests = []func(t *testing.T, sb integration.Sandbox){ testInspect, + testInspectNetworkHostEntitlement, } func testInspect(t *testing.T, sb integration.Sandbox) { @@ -47,3 +48,33 @@ func testInspect(t *testing.T, sb integration.Sandbox) { require.Empty(t, hostGatewayIP, "host-gateway-ip worker label should not be set with non-docker driver") } } + +func testInspectNetworkHostEntitlement(t *testing.T, sb integration.Sandbox) { + if sb.Name() != "docker-container" { + t.Skip("only testing for docker-container driver") + } + + var builderName string + t.Cleanup(func() { + if builderName == "" { + return + } + out, err := rmCmd(sb, withArgs(builderName)) + require.NoError(t, err, out) + }) + + out, err := createCmd(sb, withArgs("--driver", "docker-container")) + require.NoError(t, err, out) + builderName = strings.TrimSpace(out) + + out, err = inspectCmd(sb, withArgs(builderName)) + require.NoError(t, err, out) + + for _, line := range strings.Split(out, "\n") { + if v, ok := strings.CutPrefix(line, "Flags:"); ok { + require.Contains(t, v, "--allow-insecure-entitlement=network.host") + return + } + } + require.Fail(t, "network.host insecure entitlement not found in inspect output") +}