From 9c1fc22b13f4e04e104b6007fc530fc105faa320 Mon Sep 17 00:00:00 2001 From: Veeraiah Chowdary Nuvvula <6164424+veerun14@users.noreply.github.com> Date: Mon, 14 Sep 2026 21:14:54 -0500 Subject: [PATCH] securitypolicy tool: don't add the pause container to open-door policies The non-fragment linux path unconditionally appended helpers.DefaultContainerConfigs() to config.Containers before calling MarshalPolicy. MarshalPolicy rejects allow_all combined with a non-empty container list, so `securitypolicytool -c -t rego` with `allow_all = true` always failed with "Invalid policy for open-door enforcer" -- but only after pulling k8s.gcr.io/pause:3.1 and computing its dm-verity root hash, so the failure was both guaranteed and slow. The equivalent windows path does not append the default containers and works correctly today. Move the decision into linuxPolicyContainerConfigs and skip the append when allow_all is set, so an open-door policy enumerates no containers. It now generates in about a second with no registry access. Behaviour for every other config is unchanged, and the fragment path is untouched: allow_all does not apply to fragments, since MarshalFragment takes no allowAll argument. Extracting the helper makes the rule testable; main_test.go locks it in, and also asserts the underlying constraint that an open-door policy must not enumerate containers. Verified that the new test fails against the previous behaviour. Also document open-door generation in the tool README, including that such a policy is stamped with the generating hcsshim's api_version and lists exactly that version's enforcement points, so it must not be checked in as a Base64 literal and reused across versions. Signed-off-by: Veeraiah Chowdary Nuvvula <6164424+veerun14@users.noreply.github.com> --- internal/tools/securitypolicy/README.md | 28 ++++++++++ internal/tools/securitypolicy/main.go | 20 +++++++- internal/tools/securitypolicy/main_test.go | 60 ++++++++++++++++++++++ 3 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 internal/tools/securitypolicy/main_test.go diff --git a/internal/tools/securitypolicy/README.md b/internal/tools/securitypolicy/README.md index 433cb97ebc..defe7a6de2 100644 --- a/internal/tools/securitypolicy/README.md +++ b/internal/tools/securitypolicy/README.md @@ -320,3 +320,31 @@ isn't in the TOML configuration. If the version of the pause container changes from 3.1, you will need to update the hardcoded root hash by running the [dmverity-vhd](https://github.com/microsoft/integrity-vhd/tree/main/cmd/dmverity-vhd) to compute the root hash for the new container and update this tool accordingly. + +The pause container is *not* added when generating an open-door policy +(`allow_all = true` with `-t rego`), because an open-door policy must not +enumerate any container. This exception applies only to that combination: +`-t fragment` always adds the pause container, since `MarshalFragment` takes no +`allowAll` argument and a fragment cannot be open-door. + +## Open-door policies + +Setting `allow_all = true` emits an open-door policy — one that permits every +enforcement point known to this version of hcsshim. It applies to `-t rego` +only; `allow_all` is silently ignored when generating a fragment: + +```toml +allow_all = true +``` + + securitypolicytool -c allow-all.toml -t rego + +An open-door policy is still version-specific: it is stamped with the +`api_version` of the hcsshim it was generated from and lists exactly that +version's enforcement points. A policy generated by an older hcsshim will be +missing enforcement points that a newer guest expects, which the guest treats as +default-deny. Always generate the policy with the same hcsshim build that +produced the guest enforcing it, rather than reusing a checked-in Base64 blob. + +`allow_all` must not be combined with `[[container]]` entries; `MarshalPolicy` +rejects that combination. diff --git a/internal/tools/securitypolicy/main.go b/internal/tools/securitypolicy/main.go index f05d0e4231..8a2accb3d4 100644 --- a/internal/tools/securitypolicy/main.go +++ b/internal/tools/securitypolicy/main.go @@ -22,6 +22,24 @@ var ( outputRaw = flag.Bool("r", false, "whether to print the raw output") ) +// linuxPolicyContainerConfigs returns the container configs to enumerate in a +// non-fragment linux policy. +// +// An open-door policy allows every container, so it must not enumerate any: +// MarshalPolicy rejects allow_all combined with a non-empty container list. +// Adding the default pause container would also force a pointless registry +// pull to compute its dm-verity root hash. +// +// This applies only to whole policies. Fragments always include the default +// containers, since MarshalFragment takes no allowAll argument and a fragment +// cannot be open-door. +func linuxPolicyContainerConfigs(config *securitypolicy.PolicyConfig) []securitypolicy.ContainerConfig { + if config.AllowAll { + return config.Containers + } + return append(config.Containers, helpers.DefaultContainerConfigs()...) +} + func main() { flag.Parse() if flag.NArg() != 0 || len(*configFile) == 0 { @@ -77,7 +95,7 @@ func main() { switch *guestOS { case "linux": // windows_container entries are ignored when targeting linux. - config.Containers = append(config.Containers, helpers.DefaultContainerConfigs()...) + config.Containers = linuxPolicyContainerConfigs(config) policyContainers, cerr := helpers.PolicyContainersFromConfigs(config.Containers) if cerr != nil { return cerr diff --git a/internal/tools/securitypolicy/main_test.go b/internal/tools/securitypolicy/main_test.go new file mode 100644 index 0000000000..db686f325a --- /dev/null +++ b/internal/tools/securitypolicy/main_test.go @@ -0,0 +1,60 @@ +package main + +import ( + "testing" + + "github.com/Microsoft/hcsshim/internal/tools/securitypolicy/helpers" + "github.com/Microsoft/hcsshim/pkg/securitypolicy" +) + +func TestLinuxPolicyContainerConfigs_AllowAllEnumeratesNoContainers(t *testing.T) { + config := &securitypolicy.PolicyConfig{AllowAll: true} + + if got := linuxPolicyContainerConfigs(config); len(got) != 0 { + t.Fatalf("open-door policy must enumerate no containers, got %d", len(got)) + } +} + +// An open-door policy is rejected outright if it enumerates any container, so +// the guard above is what makes `-t rego` with allow_all work at all. +func TestMarshalPolicy_AllowAllRejectsContainers(t *testing.T) { + withPause, err := helpers.PolicyContainersFromConfigs(helpers.DefaultContainerConfigs()) + if err != nil { + t.Skipf("cannot resolve default containers offline: %v", err) + } + + if _, err := marshalOpenDoor(withPause); err == nil { + t.Fatal("expected allow_all combined with a container list to be rejected") + } + + if _, err := marshalOpenDoor(nil); err != nil { + t.Fatalf("open-door policy with no containers should marshal, got %v", err) + } +} + +func marshalOpenDoor(containers []*securitypolicy.Container) (string, error) { + return securitypolicy.MarshalPolicy( + "rego", + true, // allowAll + containers, + nil, + nil, + false, + false, + false, + false, + false, + false, + false, + false, + ) +} + +func TestLinuxPolicyContainerConfigs_DefaultsAddedWhenNotAllowAll(t *testing.T) { + config := &securitypolicy.PolicyConfig{} + + got := linuxPolicyContainerConfigs(config) + if want := len(helpers.DefaultContainerConfigs()); len(got) != want { + t.Fatalf("expected %d default containers, got %d", want, len(got)) + } +}