From 3c3891a413d8629ab1c14dc5b34c4a2eee656603 Mon Sep 17 00:00:00 2001 From: Mahati Chamarthy Date: Thu, 7 May 2026 22:36:54 +0100 Subject: [PATCH 1/8] CWCOW: Remove hostedSystemConfig Signed-off-by: Mahati Chamarthy --- internal/gcs-sidecar/handlers.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/internal/gcs-sidecar/handlers.go b/internal/gcs-sidecar/handlers.go index a51c3fd8f5..47535bc3f5 100644 --- a/internal/gcs-sidecar/handlers.go +++ b/internal/gcs-sidecar/handlers.go @@ -57,10 +57,9 @@ func (b *Bridge) createContainer(req *request) (err error) { return errors.Wrap(err, "failed to unmarshal createContainer") } - // containerConfig can be of type uvnConfig or hcsschema.HostedSystem or guestresource.CWCOWHostedSystem + // containerConfig can be of type uvmConfig or guestresource.CWCOWHostedSystem var ( uvmConfig prot.UvmConfig - hostedSystemConfig hcsschema.HostedSystem cwcowHostedSystemConfig guestresource.CWCOWHostedSystem ) if err = commonutils.UnmarshalJSONWithHresult(containerConfig, &uvmConfig); err == nil && @@ -68,11 +67,6 @@ func (b *Bridge) createContainer(req *request) (err error) { systemType := uvmConfig.SystemType timeZoneInformation := uvmConfig.TimeZoneInformation log.G(ctx).Tracef("createContainer: uvmConfig: {systemType: %v, timeZoneInformation: %v}}", systemType, timeZoneInformation) - } else if err = commonutils.UnmarshalJSONWithHresult(containerConfig, &hostedSystemConfig); err == nil && - hostedSystemConfig.SchemaVersion != nil && hostedSystemConfig.Container != nil { - schemaVersion := hostedSystemConfig.SchemaVersion - container := hostedSystemConfig.Container - log.G(ctx).Tracef("rpcCreate: HostedSystemConfig: {schemaVersion: %v, container: %v}}", schemaVersion, container) } else if err = commonutils.UnmarshalJSONWithHresult(containerConfig, &cwcowHostedSystemConfig); err == nil && cwcowHostedSystemConfig.Spec.Version != "" && cwcowHostedSystemConfig.CWCOWHostedSystem.Container != nil { cwcowHostedSystem := cwcowHostedSystemConfig.CWCOWHostedSystem From 5a6f3f8c425e70676e826d4e0279c8cbf81ece5e Mon Sep 17 00:00:00 2001 From: Mahati Chamarthy Date: Thu, 7 May 2026 23:01:03 +0100 Subject: [PATCH 2/8] CWCOW: Add logging enforcement policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds guest-side enforcement: each requested ETW provider is validated against allowed_log_providers declared in the security policy (case-insensitive match). Providers not in the allowlist are silently dropped. The filtered config is re-encoded and forwarded to GCS with GUIDs resolved. Also removes the hostedSystemConfig dead code path from createContainer — the sidecar only runs for confidential containers, which always use CWCOWHostedSystem. Signed-off-by: Mahati Chamarthy --- internal/gcs-sidecar/handlers.go | 47 ++++++-- internal/vm/vmutils/etw/provider_map.go | 12 +- pkg/securitypolicy/api.rego | 1 + pkg/securitypolicy/framework.rego | 13 +++ pkg/securitypolicy/open_door.rego | 1 + pkg/securitypolicy/policy.rego | 1 + pkg/securitypolicy/regopolicy_windows_test.go | 110 ++++++++++++++++++ pkg/securitypolicy/securitypolicyenforcer.go | 9 ++ .../securitypolicyenforcer_rego.go | 8 ++ 9 files changed, 184 insertions(+), 18 deletions(-) diff --git a/internal/gcs-sidecar/handlers.go b/internal/gcs-sidecar/handlers.go index 47535bc3f5..ea8615624d 100644 --- a/internal/gcs-sidecar/handlers.go +++ b/internal/gcs-sidecar/handlers.go @@ -545,21 +545,44 @@ func (b *Bridge) modifyServiceSettings(req *request) (err error) { switch settings.RPCType { case guestrequest.RPCModifyServiceSettings, guestrequest.RPCStartLogForwarding, guestrequest.RPCStopLogForwarding: log.G(req.ctx).Tracef("%v request received for LogForwardService, proceeding with policy enforcement for log sources", settings.RPCType) - // Enforce the policy for log sources in the request and update the settings with allowed log sources. - // For cwcow, the sidecar-GCS will verify the allowed log sources against policy and append the necessary GUIDs to the ones allowed. Rest are dropped. - // The Enforcer will have to unmarshal the log sources, enforce the policy and then marshal it back to a Base64 encoded JSON string which is what inbox GCS expects. - // It can query etw.GetDefaultLogSources to get the default log sources if the policy allows, and allow providers matching the default list during policy enforcement. - // This is because the log sources can be a combination of default and user specified log sources for which GUIDs need to be appended based on the policy enforcement. if settings.Settings != "" { - // - // allowedLogSources, err := b.hostState.securityOptions.PolicyEnforcer.EnforceLogForwardServiceSettingsPolicy(req.ctx, settings.LogSources) + // Decode the base64-encoded log sources config + logSources, err := etw.DecodeAndUnmarshalLogSources(settings.Settings) + if err != nil { + return fmt.Errorf("failed to decode log sources: %w", err) + } + + // Filter providers against policy — keep only those allowed + var filteredSources []etw.Source + for _, source := range logSources.LogConfig.Sources { + var allowedProviders []etw.EtwProvider + for _, provider := range source.Providers { + if err := b.hostState.securityOptions.PolicyEnforcer.EnforceLogProviderPolicy( + req.ctx, provider.ProviderName); err != nil { + log.G(req.ctx).Tracef("Log provider %q denied by policy", provider.ProviderName) + continue + } + allowedProviders = append(allowedProviders, provider) + } + if len(allowedProviders) > 0 { + filteredSources = append(filteredSources, etw.Source{ + Type: source.Type, + Providers: allowedProviders, + }) + } + } + + filteredLogSources := etw.LogSourcesInfo{ + LogConfig: etw.LogConfig{Sources: filteredSources}, + } - // For now, we are skipping the policy enforcement and allowing all log sources as the policy enforcer implementation is in progress. We will add the enforcement back once it's implemented. - allowedLogSources := settings.Settings // This is Base64 encoded JSON string of log sources - log.G(req.ctx).Tracef("Allowed log sources after policy enforcement: %v", allowedLogSources) + // Re-encode and apply GUID resolution + encodedFiltered, err := etw.MarshalAndEncodeLogSources(filteredLogSources) + if err != nil { + return fmt.Errorf("failed to encode filtered log sources: %w", err) + } - // Update the allowed log sources in the settings. This will be forwarded to inbox GCS which expects the log sources in a JSON string format with GUIDs for providers included. - allowedLogSources, err := etw.UpdateLogSources(allowedLogSources, false, true) + allowedLogSources, err := etw.UpdateLogSources(encodedFiltered, false, true) if err != nil { return fmt.Errorf("failed to update log sources: %w", err) } diff --git a/internal/vm/vmutils/etw/provider_map.go b/internal/vm/vmutils/etw/provider_map.go index 5b35206602..83ac060d8a 100644 --- a/internal/vm/vmutils/etw/provider_map.go +++ b/internal/vm/vmutils/etw/provider_map.go @@ -92,8 +92,8 @@ func mergeLogSources(resultSources []Source, userSources []Source) []Source { return resultSources } -// decodeAndUnmarshalLogSources decodes a base64-encoded JSON string and unmarshals it into a LogSourcesInfo. -func decodeAndUnmarshalLogSources(base64EncodedJSONLogConfig string) (LogSourcesInfo, error) { +// DecodeAndUnmarshalLogSources decodes a base64-encoded JSON string and unmarshals it into a LogSourcesInfo. +func DecodeAndUnmarshalLogSources(base64EncodedJSONLogConfig string) (LogSourcesInfo, error) { jsonBytes, err := base64.StdEncoding.DecodeString(base64EncodedJSONLogConfig) if err != nil { return LogSourcesInfo{}, fmt.Errorf("error decoding base64 log config: %w", err) @@ -174,9 +174,9 @@ func applyGUIDPolicy(sources []Source, includeGUIDs bool) ([]Source, error) { return stripRedundantGUIDs(sources) } -// marshalAndEncodeLogSources marshals the given LogSourcesInfo to JSON and encodes it as a base64 string. +// MarshalAndEncodeLogSources marshals the given LogSourcesInfo to JSON and encodes it as a base64 string. // On error, it logs and returns the original fallback string. -func marshalAndEncodeLogSources(logCfg LogSourcesInfo) (string, error) { +func MarshalAndEncodeLogSources(logCfg LogSourcesInfo) (string, error) { jsonBytes, err := json.Marshal(logCfg) if err != nil { return "", fmt.Errorf("error marshalling log config: %w", err) @@ -194,7 +194,7 @@ func UpdateLogSources(base64EncodedJSONLogConfig string, useDefaultLogSources bo } if base64EncodedJSONLogConfig != "" { - userLogSources, err := decodeAndUnmarshalLogSources(base64EncodedJSONLogConfig) + userLogSources, err := DecodeAndUnmarshalLogSources(base64EncodedJSONLogConfig) if err != nil { return "", fmt.Errorf("failed to decode and unmarshal user log sources: %w", err) } @@ -208,7 +208,7 @@ func UpdateLogSources(base64EncodedJSONLogConfig string, useDefaultLogSources bo return "", fmt.Errorf("failed to apply GUID policy: %w", err) } - result, err := marshalAndEncodeLogSources(resultLogCfg) + result, err := MarshalAndEncodeLogSources(resultLogCfg) if err != nil { return "", fmt.Errorf("failed to marshal and encode log sources: %w", err) } diff --git a/pkg/securitypolicy/api.rego b/pkg/securitypolicy/api.rego index 88c3d64d14..539f680f12 100644 --- a/pkg/securitypolicy/api.rego +++ b/pkg/securitypolicy/api.rego @@ -24,4 +24,5 @@ enforcement_points := { "load_fragment": {"introducedVersion": "0.9.0", "default_results": {"allowed": false, "add_module": false}, "use_framework": false}, "scratch_mount": {"introducedVersion": "0.10.0", "default_results": {"allowed": true}, "use_framework": false}, "scratch_unmount": {"introducedVersion": "0.10.0", "default_results": {"allowed": true}, "use_framework": false}, + "log_provider": {"introducedVersion": "0.11.0", "default_results": {"allowed": true}, "use_framework": false}, } diff --git a/pkg/securitypolicy/framework.rego b/pkg/securitypolicy/framework.rego index daa0fe864e..9450698d9b 100644 --- a/pkg/securitypolicy/framework.rego +++ b/pkg/securitypolicy/framework.rego @@ -1299,6 +1299,14 @@ scratch_unmount := {"metadata": [remove_scratch_mount], "allowed": true} { } } +# Log provider validation for Windows containers +default log_provider := {"allowed": false} + +log_provider := {"allowed": true} { + some allowed_provider in data.policy.allowed_log_providers + lower(input.providerName) == lower(allowed_provider) +} + # Registry changes validation default registry_changes := {"allowed": false} @@ -1827,6 +1835,11 @@ errors["no scratch at path to unmount"] { not scratch_mounted(input.unmountTarget) } +errors["log provider not allowed by policy"] { + input.rule == "log_provider" + not log_provider.allowed +} + errors[framework_version_error] { policy_framework_version == null framework_version_error := concat(" ", ["framework_version is missing. Current version:", version]) diff --git a/pkg/securitypolicy/open_door.rego b/pkg/securitypolicy/open_door.rego index 02da3fa9b6..fa277d9e7e 100644 --- a/pkg/securitypolicy/open_door.rego +++ b/pkg/securitypolicy/open_door.rego @@ -23,3 +23,4 @@ runtime_logging := {"allowed": true} load_fragment := {"allowed": true} scratch_mount := {"allowed": true} scratch_unmount := {"allowed": true} +log_provider := {"allowed": true} diff --git a/pkg/securitypolicy/policy.rego b/pkg/securitypolicy/policy.rego index 195d462931..2702075305 100644 --- a/pkg/securitypolicy/policy.rego +++ b/pkg/securitypolicy/policy.rego @@ -26,4 +26,5 @@ runtime_logging := data.framework.runtime_logging load_fragment := data.framework.load_fragment scratch_mount := data.framework.scratch_mount scratch_unmount := data.framework.scratch_unmount +log_provider := data.framework.log_provider reason := data.framework.reason diff --git a/pkg/securitypolicy/regopolicy_windows_test.go b/pkg/securitypolicy/regopolicy_windows_test.go index 33b49a64f8..629f1b0773 100644 --- a/pkg/securitypolicy/regopolicy_windows_test.go +++ b/pkg/securitypolicy/regopolicy_windows_test.go @@ -1514,3 +1514,113 @@ func substituteUVMPath(sandboxID string, m mountInternal) mountInternal { _ = sandboxID return m } + +// Tests for log provider enforcement + +func Test_Rego_EnforceLogProviderPolicy_Allowed_Windows(t *testing.T) { + rego := fmt.Sprintf(`package policy + api_version := "%s" + framework_version := "%s" + + allowed_log_providers := [ + "microsoft.windows.hyperv.compute", + "microsoft-windows-guest-network-service", + ] + + log_provider := data.framework.log_provider + `, apiVersion, frameworkVersion) + + policy, err := newRegoPolicy(rego, []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Fatalf("failed to create policy: %v", err) + } + + ctx := context.Background() + err = policy.EnforceLogProviderPolicy(ctx, "microsoft.windows.hyperv.compute") + if err != nil { + t.Errorf("expected allowed provider to pass: %v", err) + } +} + +func Test_Rego_EnforceLogProviderPolicy_Denied_Windows(t *testing.T) { + rego := fmt.Sprintf(`package policy + api_version := "%s" + framework_version := "%s" + + allowed_log_providers := [ + "microsoft.windows.hyperv.compute", + ] + + log_provider := data.framework.log_provider + `, apiVersion, frameworkVersion) + + policy, err := newRegoPolicy(rego, []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Fatalf("failed to create policy: %v", err) + } + + ctx := context.Background() + err = policy.EnforceLogProviderPolicy(ctx, "some-malicious-provider") + if err == nil { + t.Errorf("expected unknown provider to be denied") + } +} + +func Test_Rego_EnforceLogProviderPolicy_CaseInsensitive_Windows(t *testing.T) { + rego := fmt.Sprintf(`package policy + api_version := "%s" + framework_version := "%s" + + allowed_log_providers := [ + "microsoft.windows.hyperv.compute", + ] + + log_provider := data.framework.log_provider + `, apiVersion, frameworkVersion) + + policy, err := newRegoPolicy(rego, []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Fatalf("failed to create policy: %v", err) + } + + ctx := context.Background() + err = policy.EnforceLogProviderPolicy(ctx, "Microsoft.Windows.Hyperv.Compute") + if err != nil { + t.Errorf("expected case-insensitive match to pass: %v", err) + } +} + +func Test_Rego_EnforceLogProviderPolicy_OpenDoor_AllowsAll_Windows(t *testing.T) { + policy, err := newRegoPolicy(openDoorRego, []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Fatalf("failed to create policy: %v", err) + } + + ctx := context.Background() + err = policy.EnforceLogProviderPolicy(ctx, "any-provider-at-all") + if err != nil { + t.Errorf("open door should allow any provider: %v", err) + } +} + +func Test_Rego_EnforceLogProviderPolicy_EmptyAllowList_DeniesAll_Windows(t *testing.T) { + rego := fmt.Sprintf(`package policy + api_version := "%s" + framework_version := "%s" + + allowed_log_providers := [] + + log_provider := data.framework.log_provider + `, apiVersion, frameworkVersion) + + policy, err := newRegoPolicy(rego, []oci.Mount{}, []oci.Mount{}, testOSType) + if err != nil { + t.Fatalf("failed to create policy: %v", err) + } + + ctx := context.Background() + err = policy.EnforceLogProviderPolicy(ctx, "microsoft.windows.hyperv.compute") + if err == nil { + t.Errorf("expected empty allow list to deny all providers") + } +} diff --git a/pkg/securitypolicy/securitypolicyenforcer.go b/pkg/securitypolicy/securitypolicyenforcer.go index 2a4edefce1..63a3b9e92f 100644 --- a/pkg/securitypolicy/securitypolicyenforcer.go +++ b/pkg/securitypolicy/securitypolicyenforcer.go @@ -128,6 +128,7 @@ type SecurityPolicyEnforcer interface { GetUserInfo(spec *oci.Process, rootPath string) (IDName, []IDName, string, error) EnforceVerifiedCIMsPolicy(ctx context.Context, containerID string, layerHashes []string, mountedCim []string) (err error) EnforceRegistryChangesPolicy(ctx context.Context, containerID string, registryValues interface{}) error + EnforceLogProviderPolicy(ctx context.Context, providerName string) error } //nolint:unused @@ -324,6 +325,10 @@ func (OpenDoorSecurityPolicyEnforcer) EnforceRegistryChangesPolicy(ctx context.C return nil } +func (OpenDoorSecurityPolicyEnforcer) EnforceLogProviderPolicy(context.Context, string) error { + return nil +} + type ClosedDoorSecurityPolicyEnforcer struct{} var _ SecurityPolicyEnforcer = (*ClosedDoorSecurityPolicyEnforcer)(nil) @@ -452,3 +457,7 @@ func (ClosedDoorSecurityPolicyEnforcer) EnforceVerifiedCIMsPolicy(ctx context.Co func (ClosedDoorSecurityPolicyEnforcer) EnforceRegistryChangesPolicy(ctx context.Context, containerID string, registryValues interface{}) error { return errors.New("registry changes are denied by policy") } + +func (ClosedDoorSecurityPolicyEnforcer) EnforceLogProviderPolicy(context.Context, string) error { + return errors.New("log provider is denied by policy") +} diff --git a/pkg/securitypolicy/securitypolicyenforcer_rego.go b/pkg/securitypolicy/securitypolicyenforcer_rego.go index 96c5613dd6..718e0044d9 100644 --- a/pkg/securitypolicy/securitypolicyenforcer_rego.go +++ b/pkg/securitypolicy/securitypolicyenforcer_rego.go @@ -1188,6 +1188,14 @@ func (policy *regoEnforcer) EnforceRegistryChangesPolicy(ctx context.Context, co return err } +func (policy *regoEnforcer) EnforceLogProviderPolicy(ctx context.Context, providerName string) error { + input := inputData{ + "providerName": providerName, + } + _, err := policy.enforce(ctx, "log_provider", input) + return err +} + func (policy *regoEnforcer) GetUserInfo(process *oci.Process, rootPath string) (IDName, []IDName, string, error) { return GetAllUserInfo(process, rootPath) } From 9a0d5a1a596ffd384c5767ea23c98ade75a62f8b Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Mon, 1 Jun 2026 10:22:41 +0100 Subject: [PATCH 3/8] CWCOW: Enforce log_provider policy with allow_log_provider_dropping switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tightens log_provider enforcement so that, by default, the sidecar fails-close on any disallowed provider and locks down further policy calls (LockDown now installs the closed enforcer so SetConfidentialOptions cannot reinstall a permissive policy). The previous silent-drop behaviour is preserved behind a new allow_log_provider_dropping policy flag — when true, providers missing from allowed_log_providers are dropped and only the kept subset is re-encoded and forwarded to GCS. Threads the flag through the rego framework, marshaller, and Go enforcer (EnforceLogProviderPolicy now returns the providers to keep), and adds rego + sidecar tests covering both fail-close and dropping modes. Signed-off-by: Takuro Sato --- internal/gcs-sidecar/handlers.go | 74 +++++-- internal/gcs-sidecar/handlers_test.go | 195 +++++++++++++++++ internal/tools/securitypolicy/main.go | 1 + internal/uvm/start.go | 17 ++ internal/vm/vmutils/etw/provider_map.go | 6 +- pkg/securitypolicy/api.rego | 2 +- pkg/securitypolicy/framework.rego | 44 +++- pkg/securitypolicy/opts.go | 7 + pkg/securitypolicy/rego_utils_test.go | 6 + pkg/securitypolicy/regopolicy_linux_test.go | 1 + pkg/securitypolicy/regopolicy_windows_test.go | 199 +++++++++++++----- pkg/securitypolicy/securitypolicy.go | 6 + pkg/securitypolicy/securitypolicy_internal.go | 4 + pkg/securitypolicy/securitypolicy_marshal.go | 17 +- pkg/securitypolicy/securitypolicy_options.go | 22 ++ .../securitypolicy_options_test.go | 86 ++++++++ pkg/securitypolicy/securitypolicyenforcer.go | 17 +- .../securitypolicyenforcer_rego.go | 43 +++- test/pkg/securitypolicy/policy.go | 1 + 19 files changed, 655 insertions(+), 93 deletions(-) create mode 100644 pkg/securitypolicy/securitypolicy_options_test.go diff --git a/internal/gcs-sidecar/handlers.go b/internal/gcs-sidecar/handlers.go index ea8615624d..0165bc71e0 100644 --- a/internal/gcs-sidecar/handlers.go +++ b/internal/gcs-sidecar/handlers.go @@ -4,6 +4,7 @@ package bridge import ( + "encoding/base64" "encoding/hex" "encoding/json" "fmt" @@ -546,43 +547,70 @@ func (b *Bridge) modifyServiceSettings(req *request) (err error) { case guestrequest.RPCModifyServiceSettings, guestrequest.RPCStartLogForwarding, guestrequest.RPCStopLogForwarding: log.G(req.ctx).Tracef("%v request received for LogForwardService, proceeding with policy enforcement for log sources", settings.RPCType) if settings.Settings != "" { - // Decode the base64-encoded log sources config + // Decode the base64-encoded log sources config so we can + // enforce policy on the requested provider list. logSources, err := etw.DecodeAndUnmarshalLogSources(settings.Settings) if err != nil { return fmt.Errorf("failed to decode log sources: %w", err) } - // Filter providers against policy — keep only those allowed - var filteredSources []etw.Source + // Collect every requested provider name and ask the + // enforcer to validate them as a batch. The enforcer's + // behaviour depends on allow_log_provider_dropping in the + // active policy: + // - false (default, fail-close): any disallowed provider + // causes the call to be denied. + // - true: disallowed providers are silently dropped and + // the kept subset is returned for forwarding. + var requestedNames []string for _, source := range logSources.LogConfig.Sources { - var allowedProviders []etw.EtwProvider for _, provider := range source.Providers { - if err := b.hostState.securityOptions.PolicyEnforcer.EnforceLogProviderPolicy( - req.ctx, provider.ProviderName); err != nil { - log.G(req.ctx).Tracef("Log provider %q denied by policy", provider.ProviderName) - continue - } - allowedProviders = append(allowedProviders, provider) - } - if len(allowedProviders) > 0 { - filteredSources = append(filteredSources, etw.Source{ - Type: source.Type, - Providers: allowedProviders, - }) + requestedNames = append(requestedNames, provider.ProviderName) } } - filteredLogSources := etw.LogSourcesInfo{ - LogConfig: etw.LogConfig{Sources: filteredSources}, + keptNames, err := b.hostState.securityOptions.PolicyEnforcer.EnforceLogProviderPolicy( + req.ctx, requestedNames) + if err != nil { + b.hostState.securityOptions.LockDown(req.ctx) + return fmt.Errorf("log providers denied by policy: %w", err) } - // Re-encode and apply GUID resolution - encodedFiltered, err := etw.MarshalAndEncodeLogSources(filteredLogSources) - if err != nil { - return fmt.Errorf("failed to encode filtered log sources: %w", err) + // Build a quick lookup for the kept set so we can trim the + // LogSourcesInfo to only those providers the policy allowed. + keepSet := make(map[string]struct{}, len(keptNames)) + for _, name := range keptNames { + keepSet[name] = struct{}{} + } + + payload := settings.Settings + if len(keptNames) != len(requestedNames) { + // Subset kept. Trim each source's provider list to + // only the allowed names, then re-encode for the + // inbox GCS. Empty sources are preserved to keep the + // shape stable; inbox GCS handles them as no-ops. + trimmed := logSources + for i := range trimmed.LogConfig.Sources { + src := &trimmed.LogConfig.Sources[i] + filtered := make([]etw.EtwProvider, 0, len(src.Providers)) + for _, p := range src.Providers { + if _, ok := keepSet[p.ProviderName]; ok { + filtered = append(filtered, p) + } + } + src.Providers = filtered + } + trimmedJSON, err := json.Marshal(trimmed) + if err != nil { + return fmt.Errorf("failed to marshal trimmed log sources: %w", err) + } + payload = base64.StdEncoding.EncodeToString(trimmedJSON) } - allowedLogSources, err := etw.UpdateLogSources(encodedFiltered, false, true) + // Apply GUID resolution (and any other inbox-GCS prep) + // against the policy-trimmed payload and hand off to + // inbox GCS. + allowedLogSources, err := etw.UpdateLogSources(payload, false, true) if err != nil { return fmt.Errorf("failed to update log sources: %w", err) } diff --git a/internal/gcs-sidecar/handlers_test.go b/internal/gcs-sidecar/handlers_test.go index 6de3a0a605..843d062f0c 100644 --- a/internal/gcs-sidecar/handlers_test.go +++ b/internal/gcs-sidecar/handlers_test.go @@ -5,6 +5,7 @@ package bridge import ( "context" + "encoding/base64" "encoding/json" "io" "testing" @@ -14,6 +15,7 @@ import ( "github.com/Microsoft/hcsshim/internal/gcs/prot" "github.com/Microsoft/hcsshim/internal/protocol/guestrequest" "github.com/Microsoft/hcsshim/internal/protocol/guestresource" + "github.com/Microsoft/hcsshim/internal/vm/vmutils/etw" "github.com/Microsoft/hcsshim/pkg/securitypolicy" ) @@ -327,3 +329,196 @@ func TestModifySettings_PolicyFragment_TypeAssertionFailure(t *testing.T) { t.Fatal("expected error for empty fragment, got nil") } } + +// buildLogForwardServiceRequest builds a serialized ServiceModificationRequest +// for the LogForwardService with the given provider names baked into a +// base64-encoded LogSourcesInfo payload. +func buildLogForwardServiceRequest(t *testing.T, providerNames ...string) []byte { + t.Helper() + + providers := make([]etw.EtwProvider, 0, len(providerNames)) + for _, name := range providerNames { + providers = append(providers, etw.EtwProvider{ProviderName: name}) + } + info := etw.LogSourcesInfo{ + LogConfig: etw.LogConfig{ + Sources: []etw.Source{{ + Type: "etw", + Providers: providers, + }}, + }, + } + infoBytes, err := json.Marshal(info) + if err != nil { + t.Fatalf("failed to marshal log sources: %v", err) + } + encoded := base64.StdEncoding.EncodeToString(infoBytes) + + inner := &guestrequest.LogForwardServiceRPCRequest{ + RPCType: guestrequest.RPCModifyServiceSettings, + Settings: encoded, + } + req := prot.ServiceModificationRequest{ + RequestBase: prot.RequestBase{ + ContainerID: UVMContainerID, + ActivityID: guid.GUID{}, + }, + PropertyType: string(prot.LogForwardService), + Settings: inner, + } + b, err := json.Marshal(req) + if err != nil { + t.Fatalf("failed to marshal request: %v", err) + } + return b +} + +// newModifyServiceSettingsRequest wraps the given LogForwardService payload +// in a bridge `request` ready for modifyServiceSettings. +func newModifyServiceSettingsRequest(payload []byte) *request { + return &request{ + ctx: context.Background(), + header: messageHeader{ + Type: prot.MsgTypeRequest | prot.MsgType(prot.RPCModifyServiceSettings), + Size: uint32(len(payload)) + prot.HdrSize, + ID: 1, + }, + activityID: guid.GUID{}, + message: payload, + } +} + +// TestModifyServiceSettings_LogForward_PolicyAllow_ForwardsToGCS verifies that +// when every requested provider is allowed by policy, the call succeeds and +// the (possibly GUID-resolved) request is forwarded to inbox GCS. +func TestModifyServiceSettings_LogForward_PolicyAllow_ForwardsToGCS(t *testing.T) { + b := newTestBridge(&securitypolicy.OpenDoorSecurityPolicyEnforcer{}) + + // Use a provider that is in the known etw_map so UpdateLogSources's GUID + // resolution succeeds. + payload := buildLogForwardServiceRequest(t, "microsoft.windows.hyperv.compute") + req := newModifyServiceSettingsRequest(payload) + + if err := b.modifyServiceSettings(req); err != nil { + t.Fatalf("modifyServiceSettings with allowed provider returned error: %v", err) + } + + select { + case <-b.sendToGCSCh: + // Forwarded to GCS as expected. + case <-time.After(time.Second): + t.Fatal("timed out waiting for request to be forwarded to GCS") + } +} + +// TestModifyServiceSettings_LogForward_PolicyDeny_ReturnsErrorAndLocksDown +// verifies that when any requested provider is denied by policy, the call +// fails (no forward to GCS) and the sidecar enforcer is locked down to +// closed-door so subsequent requests also deny. +func TestModifyServiceSettings_LogForward_PolicyDeny_ReturnsErrorAndLocksDown(t *testing.T) { + b := newTestBridge(&securitypolicy.ClosedDoorSecurityPolicyEnforcer{}) + + payload := buildLogForwardServiceRequest(t, "microsoft.windows.hyperv.compute") + req := newModifyServiceSettingsRequest(payload) + + err := b.modifyServiceSettings(req) + if err == nil { + t.Fatal("expected modifyServiceSettings to fail under ClosedDoor enforcer") + } + + // The request must NOT have been forwarded to GCS. + select { + case fwd := <-b.sendToGCSCh: + t.Fatalf("denied request must not be forwarded to GCS: %+v", fwd) + default: + // Good. + } + + // Enforcer should now be locked down. (ClosedDoor was already installed; + // LockDown's idempotency check keeps the same instance, so we verify the + // type rather than identity.) + if _, ok := b.hostState.securityOptions.PolicyEnforcer.(*securitypolicy.ClosedDoorSecurityPolicyEnforcer); !ok { + t.Errorf("after deny: expected ClosedDoor enforcer, got %T", b.hostState.securityOptions.PolicyEnforcer) + } +} + +// droppingLogProviderEnforcer is a test stub that approves only the configured +// allow-list of provider names; any others are silently dropped from the +// returned subset. It mirrors the regoEnforcer's behaviour under +// allow_log_provider_dropping := true and never returns an error. +type droppingLogProviderEnforcer struct { + securitypolicy.OpenDoorSecurityPolicyEnforcer + allowed map[string]struct{} +} + +func (e *droppingLogProviderEnforcer) EnforceLogProviderPolicy(_ context.Context, providerNames []string) ([]string, error) { + kept := make([]string, 0, len(providerNames)) + for _, name := range providerNames { + if _, ok := e.allowed[name]; ok { + kept = append(kept, name) + } + } + return kept, nil +} + +// TestModifyServiceSettings_LogForward_PolicyDropping_TrimsForwardedPayload +// verifies the silent-drop path in the sidecar: when the enforcer returns a +// strict subset of the requested providers, the call succeeds and the payload +// forwarded to inbox GCS contains only the kept providers (not the original +// disallowed ones). +func TestModifyServiceSettings_LogForward_PolicyDropping_TrimsForwardedPayload(t *testing.T) { + kept := "microsoft.windows.hyperv.compute" + dropped := "some-bogus-provider" + enforcer := &droppingLogProviderEnforcer{ + allowed: map[string]struct{}{kept: {}}, + } + b := newTestBridge(enforcer) + + payload := buildLogForwardServiceRequest(t, kept, dropped) + req := newModifyServiceSettingsRequest(payload) + + if err := b.modifyServiceSettings(req); err != nil { + t.Fatalf("modifyServiceSettings under dropping enforcer returned error: %v", err) + } + + var forwarded request + select { + case forwarded = <-b.sendToGCSCh: + case <-time.After(time.Second): + t.Fatal("timed out waiting for request to be forwarded to GCS") + } + + // Decode the forwarded request back into LogSourcesInfo and confirm the + // disallowed provider has been stripped while the allowed one survives. + var fwdReq prot.ServiceModificationRequest + fwdReq.Settings = &guestrequest.LogForwardServiceRPCRequest{} + if err := json.Unmarshal(forwarded.message, &fwdReq); err != nil { + t.Fatalf("failed to unmarshal forwarded request: %v", err) + } + innerSettings, ok := fwdReq.Settings.(*guestrequest.LogForwardServiceRPCRequest) + if !ok { + t.Fatalf("forwarded settings has unexpected type: %T", fwdReq.Settings) + } + logSources, err := etw.DecodeAndUnmarshalLogSources(innerSettings.Settings) + if err != nil { + t.Fatalf("failed to decode forwarded log sources: %v", err) + } + + var sawKept, sawDropped bool + for _, src := range logSources.LogConfig.Sources { + for _, p := range src.Providers { + if p.ProviderName == kept { + sawKept = true + } + if p.ProviderName == dropped { + sawDropped = true + } + } + } + if !sawKept { + t.Errorf("expected forwarded payload to contain kept provider %q", kept) + } + if sawDropped { + t.Errorf("expected dropped provider %q to be absent from forwarded payload", dropped) + } +} diff --git a/internal/tools/securitypolicy/main.go b/internal/tools/securitypolicy/main.go index d5ad89e28b..be1860959d 100644 --- a/internal/tools/securitypolicy/main.go +++ b/internal/tools/securitypolicy/main.go @@ -68,6 +68,7 @@ func main() { config.AllowEnvironmentVariableDropping, config.AllowUnencryptedScratch, config.AllowCapabilityDropping, + config.AllowLogProviderDropping, ) } if err != nil { diff --git a/internal/uvm/start.go b/internal/uvm/start.go index e429fcc4a0..5d59883cdb 100644 --- a/internal/uvm/start.go +++ b/internal/uvm/start.go @@ -289,11 +289,28 @@ func (uvm *UtilityVM) Start(ctx context.Context) (err error) { if uvm.OS() == "windows" && uvm.logForwardingEnabled { // If the UVM is Windows and log forwarding is enabled, set the log sources // and start the log forwarding service. + // + // For confidential (CWCOW) UVMs, a failure here may be a policy + // violation (e.g. log_provider denied by the rego). In that case we + // must fail-close: return the error so the deferred + // uvm.hcsSystem.Terminate above tears the UVM down rather than + // leaving it half-initialised with a known-violating log + // configuration. For non-confidential WCOW, log-forwarding is + // best-effort (transient ETW issues, missing providers, etc. must + // not abort UVM start), so preserve the original log-and-continue + // behaviour. + failClose := uvm.HasConfidentialPolicy() if err := uvm.SetLogSources(ctx); err != nil { e.WithError(err).Error("failed to set log sources") + if failClose { + return fmt.Errorf("failed to set log sources: %w", err) + } } if err := uvm.StartLogForwarding(ctx); err != nil { e.WithError(err).Error("failed to start log forwarding") + if failClose { + return fmt.Errorf("failed to start log forwarding: %w", err) + } } } diff --git a/internal/vm/vmutils/etw/provider_map.go b/internal/vm/vmutils/etw/provider_map.go index 83ac060d8a..962d0c586a 100644 --- a/internal/vm/vmutils/etw/provider_map.go +++ b/internal/vm/vmutils/etw/provider_map.go @@ -174,9 +174,9 @@ func applyGUIDPolicy(sources []Source, includeGUIDs bool) ([]Source, error) { return stripRedundantGUIDs(sources) } -// MarshalAndEncodeLogSources marshals the given LogSourcesInfo to JSON and encodes it as a base64 string. +// marshalAndEncodeLogSources marshals the given LogSourcesInfo to JSON and encodes it as a base64 string. // On error, it logs and returns the original fallback string. -func MarshalAndEncodeLogSources(logCfg LogSourcesInfo) (string, error) { +func marshalAndEncodeLogSources(logCfg LogSourcesInfo) (string, error) { jsonBytes, err := json.Marshal(logCfg) if err != nil { return "", fmt.Errorf("error marshalling log config: %w", err) @@ -208,7 +208,7 @@ func UpdateLogSources(base64EncodedJSONLogConfig string, useDefaultLogSources bo return "", fmt.Errorf("failed to apply GUID policy: %w", err) } - result, err := MarshalAndEncodeLogSources(resultLogCfg) + result, err := marshalAndEncodeLogSources(resultLogCfg) if err != nil { return "", fmt.Errorf("failed to marshal and encode log sources: %w", err) } diff --git a/pkg/securitypolicy/api.rego b/pkg/securitypolicy/api.rego index 539f680f12..9474c75bce 100644 --- a/pkg/securitypolicy/api.rego +++ b/pkg/securitypolicy/api.rego @@ -24,5 +24,5 @@ enforcement_points := { "load_fragment": {"introducedVersion": "0.9.0", "default_results": {"allowed": false, "add_module": false}, "use_framework": false}, "scratch_mount": {"introducedVersion": "0.10.0", "default_results": {"allowed": true}, "use_framework": false}, "scratch_unmount": {"introducedVersion": "0.10.0", "default_results": {"allowed": true}, "use_framework": false}, - "log_provider": {"introducedVersion": "0.11.0", "default_results": {"allowed": true}, "use_framework": false}, + "log_provider": {"introducedVersion": "0.11.0", "default_results": {"allowed": true, "providers_to_keep": null}, "use_framework": false}, } diff --git a/pkg/securitypolicy/framework.rego b/pkg/securitypolicy/framework.rego index 9450698d9b..3c204e3594 100644 --- a/pkg/securitypolicy/framework.rego +++ b/pkg/securitypolicy/framework.rego @@ -1299,12 +1299,45 @@ scratch_unmount := {"metadata": [remove_scratch_mount], "allowed": true} { } } -# Log provider validation for Windows containers -default log_provider := {"allowed": false} +# Log provider validation for Windows containers. +# +# Two modes (mirrors allow_environment_variable_dropping): +# - allow_log_provider_dropping := false (default, fail-close): every +# requested provider name must appear in allowed_log_providers, otherwise +# the rule denies the entire request. +# - allow_log_provider_dropping := true: providers not in the allow-list are +# silently dropped from providers_to_keep; the call still allows and the +# caller is expected to only forward the remaining providers. +# +# Input: {"providers": [name, ...]} +# Output: {"allowed": bool, "providers_to_keep": [name, ...]} +default log_provider := {"allowed": false, "providers_to_keep": []} + +valid_log_providers := providers { + allow_log_provider_dropping + + providers := [name | + name := input.providers[_] + some allowed_provider in data.policy.allowed_log_providers + lower(name) == lower(allowed_provider) + ] +} + +valid_log_providers := providers { + not allow_log_provider_dropping + providers := input.providers +} + +log_providers_ok(providers) { + every name in providers { + some allowed_provider in data.policy.allowed_log_providers + lower(name) == lower(allowed_provider) + } +} -log_provider := {"allowed": true} { - some allowed_provider in data.policy.allowed_log_providers - lower(input.providerName) == lower(allowed_provider) +log_provider := {"allowed": true, "providers_to_keep": providers} { + providers := valid_log_providers + log_providers_ok(providers) } # Registry changes validation @@ -2320,6 +2353,7 @@ allow_dump_stacks := data.policy.allow_dump_stacks allow_runtime_logging := data.policy.allow_runtime_logging allow_environment_variable_dropping := data.policy.allow_environment_variable_dropping allow_unencrypted_scratch := data.policy.allow_unencrypted_scratch +allow_log_provider_dropping := data.policy.allow_log_provider_dropping # all flags not in the base set need to have default logic applied diff --git a/pkg/securitypolicy/opts.go b/pkg/securitypolicy/opts.go index a11685abc4..9446f9dd30 100644 --- a/pkg/securitypolicy/opts.go +++ b/pkg/securitypolicy/opts.go @@ -115,6 +115,13 @@ func WithAllowEnvVarDropping(allow bool) PolicyConfigOpt { } } +func WithAllowLogProviderDropping(allow bool) PolicyConfigOpt { + return func(config *PolicyConfig) error { + config.AllowLogProviderDropping = allow + return nil + } +} + func WithAllowCapabilityDropping(allow bool) PolicyConfigOpt { return func(config *PolicyConfig) error { config.AllowCapabilityDropping = allow diff --git a/pkg/securitypolicy/rego_utils_test.go b/pkg/securitypolicy/rego_utils_test.go index 2967b5a6b7..1e78278f9b 100644 --- a/pkg/securitypolicy/rego_utils_test.go +++ b/pkg/securitypolicy/rego_utils_test.go @@ -2009,6 +2009,7 @@ func (constraints *generatedConstraints) toPolicy() *securityPolicyInternal { AllowEnvironmentVariableDropping: constraints.allowEnvironmentVariableDropping, AllowUnencryptedScratch: constraints.allowUnencryptedScratch, AllowCapabilityDropping: constraints.allowCapabilityDropping, + AllowLogProviderDropping: constraints.allowLogProviderDropping, } } @@ -2270,6 +2271,7 @@ func generateConstraints(r *rand.Rand, maxContainers int32) *generatedConstraint namespace: generateFragmentNamespace(testRand), svn: generateSVN(testRand), allowCapabilityDropping: false, + allowLogProviderDropping: false, ctx: context.Background(), } } @@ -2909,6 +2911,7 @@ type generatedConstraints struct { namespace string svn string allowCapabilityDropping bool + allowLogProviderDropping bool ctx context.Context } @@ -2924,6 +2927,7 @@ type generatedWindowsConstraints struct { namespace string svn string allowCapabilityDropping bool + allowLogProviderDropping bool ctx context.Context } @@ -2938,6 +2942,7 @@ func (constraints *generatedWindowsConstraints) toPolicy() *securityPolicyWindow AllowEnvironmentVariableDropping: constraints.allowEnvironmentVariableDropping, AllowUnencryptedScratch: constraints.allowUnencryptedScratch, AllowCapabilityDropping: constraints.allowCapabilityDropping, + AllowLogProviderDropping: constraints.allowLogProviderDropping, } } @@ -2982,6 +2987,7 @@ func generateWindowsConstraints(r *rand.Rand, maxContainers int32) *generatedWin allowEnvironmentVariableDropping: false, allowUnencryptedScratch: false, allowCapabilityDropping: false, + allowLogProviderDropping: false, namespace: generateFragmentNamespace(r), svn: generateSVN(r), ctx: context.Background(), diff --git a/pkg/securitypolicy/regopolicy_linux_test.go b/pkg/securitypolicy/regopolicy_linux_test.go index 8dd409fccf..f40e6ffd3d 100644 --- a/pkg/securitypolicy/regopolicy_linux_test.go +++ b/pkg/securitypolicy/regopolicy_linux_test.go @@ -74,6 +74,7 @@ func Test_MarshalRego_Policy(t *testing.T) { p.allowEnvironmentVariableDropping, p.allowUnencryptedScratch, p.allowCapabilityDropping, + p.allowLogProviderDropping, ) if err != nil { t.Error(err) diff --git a/pkg/securitypolicy/regopolicy_windows_test.go b/pkg/securitypolicy/regopolicy_windows_test.go index 629f1b0773..9f9a9bb0b6 100644 --- a/pkg/securitypolicy/regopolicy_windows_test.go +++ b/pkg/securitypolicy/regopolicy_windows_test.go @@ -1517,77 +1517,85 @@ func substituteUVMPath(sandboxID string, m mountInternal) mountInternal { // Tests for log provider enforcement -func Test_Rego_EnforceLogProviderPolicy_Allowed_Windows(t *testing.T) { +// newLogProviderTestPolicy builds a Rego policy whose allowed_log_providers +// list contains the given providers and returns the compiled enforcer. +// Pass no providers to get an empty allow-list. +// +// allow_log_provider_dropping is left unset so the test exercises the +// default fail-close mode. Use newLogProviderTestPolicyWithDropping to flip +// the mode. +func newLogProviderTestPolicy(t *testing.T, allowedProviders ...string) *regoEnforcer { + t.Helper() + return newLogProviderTestPolicyWithDropping(t, false, allowedProviders...) +} + +// newLogProviderTestPolicyWithDropping is the more general helper used by the +// mode-specific tests. It compiles a Rego policy that defines +// allowed_log_providers, sets allow_log_provider_dropping to dropping, and +// routes log_provider through the framework rule. +func newLogProviderTestPolicyWithDropping(t *testing.T, dropping bool, allowedProviders ...string) *regoEnforcer { + t.Helper() + var listLines string + for _, p := range allowedProviders { + listLines += fmt.Sprintf("\t\t%q,\n", p) + } rego := fmt.Sprintf(`package policy api_version := "%s" framework_version := "%s" + allow_log_provider_dropping := %t + allowed_log_providers := [ - "microsoft.windows.hyperv.compute", - "microsoft-windows-guest-network-service", - ] +%s ] log_provider := data.framework.log_provider - `, apiVersion, frameworkVersion) + `, apiVersion, frameworkVersion, dropping, listLines) policy, err := newRegoPolicy(rego, []oci.Mount{}, []oci.Mount{}, testOSType) if err != nil { t.Fatalf("failed to create policy: %v", err) } + return policy +} - ctx := context.Background() - err = policy.EnforceLogProviderPolicy(ctx, "microsoft.windows.hyperv.compute") +func Test_Rego_EnforceLogProviderPolicy_Allowed_Windows(t *testing.T) { + policy := newLogProviderTestPolicy(t, + "microsoft.windows.hyperv.compute", + "microsoft-windows-guest-network-service", + ) + + kept, err := policy.EnforceLogProviderPolicy(context.Background(), + []string{"microsoft.windows.hyperv.compute"}) if err != nil { t.Errorf("expected allowed provider to pass: %v", err) } + if len(kept) != 1 || kept[0] != "microsoft.windows.hyperv.compute" { + t.Errorf("expected kept=[microsoft.windows.hyperv.compute]; got %v", kept) + } } func Test_Rego_EnforceLogProviderPolicy_Denied_Windows(t *testing.T) { - rego := fmt.Sprintf(`package policy - api_version := "%s" - framework_version := "%s" + policy := newLogProviderTestPolicy(t, "microsoft.windows.hyperv.compute") - allowed_log_providers := [ - "microsoft.windows.hyperv.compute", - ] - - log_provider := data.framework.log_provider - `, apiVersion, frameworkVersion) - - policy, err := newRegoPolicy(rego, []oci.Mount{}, []oci.Mount{}, testOSType) - if err != nil { - t.Fatalf("failed to create policy: %v", err) - } - - ctx := context.Background() - err = policy.EnforceLogProviderPolicy(ctx, "some-malicious-provider") + _, err := policy.EnforceLogProviderPolicy(context.Background(), + []string{"some-malicious-provider"}) if err == nil { t.Errorf("expected unknown provider to be denied") } } func Test_Rego_EnforceLogProviderPolicy_CaseInsensitive_Windows(t *testing.T) { - rego := fmt.Sprintf(`package policy - api_version := "%s" - framework_version := "%s" - - allowed_log_providers := [ - "microsoft.windows.hyperv.compute", - ] - - log_provider := data.framework.log_provider - `, apiVersion, frameworkVersion) - - policy, err := newRegoPolicy(rego, []oci.Mount{}, []oci.Mount{}, testOSType) - if err != nil { - t.Fatalf("failed to create policy: %v", err) - } + policy := newLogProviderTestPolicy(t, "microsoft.windows.hyperv.compute") - ctx := context.Background() - err = policy.EnforceLogProviderPolicy(ctx, "Microsoft.Windows.Hyperv.Compute") + kept, err := policy.EnforceLogProviderPolicy(context.Background(), + []string{"Microsoft.Windows.Hyperv.Compute"}) if err != nil { t.Errorf("expected case-insensitive match to pass: %v", err) } + // Rego preserves the input casing; we just confirm the name survived. + if len(kept) != 1 || kept[0] != "Microsoft.Windows.Hyperv.Compute" { + t.Errorf("expected kept=[Microsoft.Windows.Hyperv.Compute]; got %v", kept) + } } func Test_Rego_EnforceLogProviderPolicy_OpenDoor_AllowsAll_Windows(t *testing.T) { @@ -1597,21 +1605,42 @@ func Test_Rego_EnforceLogProviderPolicy_OpenDoor_AllowsAll_Windows(t *testing.T) } ctx := context.Background() - err = policy.EnforceLogProviderPolicy(ctx, "any-provider-at-all") + kept, err := policy.EnforceLogProviderPolicy(ctx, []string{"any-provider-at-all"}) if err != nil { t.Errorf("open door should allow any provider: %v", err) } + if len(kept) != 1 || kept[0] != "any-provider-at-all" { + t.Errorf("open door should keep the requested provider; got %v", kept) + } } func Test_Rego_EnforceLogProviderPolicy_EmptyAllowList_DeniesAll_Windows(t *testing.T) { - rego := fmt.Sprintf(`package policy - api_version := "%s" - framework_version := "%s" + policy := newLogProviderTestPolicy(t) - allowed_log_providers := [] + _, err := policy.EnforceLogProviderPolicy(context.Background(), + []string{"microsoft.windows.hyperv.compute"}) + if err == nil { + t.Errorf("expected empty allow list to deny all providers") + } +} - log_provider := data.framework.log_provider - `, apiVersion, frameworkVersion) +// Test_Rego_EnforceLogProviderPolicy_PreFeatureAPIVersion_Allows_Windows pins +// the non-regression behaviour for policies authored before log_provider was +// introduced (api.rego entry: introducedVersion=0.11.0, default_results.allowed=true). +// Such policies omit allowed_log_providers entirely; EnforceLogProviderPolicy +// must return the input list unchanged with no error so existing CWCOW/WCOW +// policies do not break when the framework gains the new enforcement point. +func Test_Rego_EnforceLogProviderPolicy_PreFeatureAPIVersion_Allows_Windows(t *testing.T) { + // A pre-feature policy does not define log_provider at all (it was + // authored before the rule existed). The version-gated default_results + // path only fires when the policy has no rule for the enforcement + // point — including `log_provider := data.framework.log_provider` + // here would shadow the default and route through the framework rule, + // which defaults to deny. + rego := fmt.Sprintf(`package policy + api_version := "0.10.0" + framework_version := "%s" + `, frameworkVersion) policy, err := newRegoPolicy(rego, []oci.Mount{}, []oci.Mount{}, testOSType) if err != nil { @@ -1619,8 +1648,78 @@ func Test_Rego_EnforceLogProviderPolicy_EmptyAllowList_DeniesAll_Windows(t *test } ctx := context.Background() - err = policy.EnforceLogProviderPolicy(ctx, "microsoft.windows.hyperv.compute") + kept, err := policy.EnforceLogProviderPolicy(ctx, + []string{"any-provider-not-in-any-list"}) + if err != nil { + t.Errorf("expected pre-0.11.0 policy to allow any provider via default_results: %v", err) + } + // default_results.providers_to_keep is null, so getProvidersToKeep + // returns the input list unchanged. + if len(kept) != 1 || kept[0] != "any-provider-not-in-any-list" { + t.Errorf("expected kept=[any-provider-not-in-any-list]; got %v", kept) + } +} + +// Test_Rego_EnforceLogProviderPolicy_EmptyProviderName_Denied_Windows pins +// the behaviour for the host-scrubbed-unknown-provider edge case: when the +// providerName is the empty string, no allow-list entry can match (allow-lists +// never contain ""), so enforcement must deny. +func Test_Rego_EnforceLogProviderPolicy_EmptyProviderName_Denied_Windows(t *testing.T) { + policy := newLogProviderTestPolicy(t, "microsoft.windows.hyperv.compute") + + _, err := policy.EnforceLogProviderPolicy(context.Background(), []string{""}) if err == nil { - t.Errorf("expected empty allow list to deny all providers") + t.Errorf("expected empty providerName to be denied") + } +} + +// Test_Rego_EnforceLogProviderPolicy_Dropping_KeepsSubset_Windows exercises +// the silent-drop mode: when allow_log_provider_dropping := true the call +// allows even if some requested providers are not on the allow-list, and only +// the matching subset is returned. +func Test_Rego_EnforceLogProviderPolicy_Dropping_KeepsSubset_Windows(t *testing.T) { + policy := newLogProviderTestPolicyWithDropping(t, true, + "microsoft.windows.hyperv.compute", + "microsoft-windows-guest-network-service", + ) + + kept, err := policy.EnforceLogProviderPolicy(context.Background(), []string{ + "microsoft.windows.hyperv.compute", + "some-bogus-provider", + "microsoft-windows-guest-network-service", + }) + if err != nil { + t.Errorf("dropping mode should allow regardless of unknown providers: %v", err) + } + + keptSet := make(map[string]struct{}, len(kept)) + for _, n := range kept { + keptSet[n] = struct{}{} + } + if _, ok := keptSet["microsoft.windows.hyperv.compute"]; !ok { + t.Errorf("expected 'microsoft.windows.hyperv.compute' kept; got %v", kept) + } + if _, ok := keptSet["microsoft-windows-guest-network-service"]; !ok { + t.Errorf("expected 'microsoft-windows-guest-network-service' kept; got %v", kept) + } + if _, ok := keptSet["some-bogus-provider"]; ok { + t.Errorf("expected 'some-bogus-provider' to be dropped; got %v", kept) + } +} + +// Test_Rego_EnforceLogProviderPolicy_FailClose_AnyMissDenies_Windows confirms +// the inverse of the dropping test: with allow_log_provider_dropping := false +// (default) even one unknown provider in a batch fails the entire call. +func Test_Rego_EnforceLogProviderPolicy_FailClose_AnyMissDenies_Windows(t *testing.T) { + policy := newLogProviderTestPolicyWithDropping(t, false, + "microsoft.windows.hyperv.compute", + ) + + _, err := policy.EnforceLogProviderPolicy(context.Background(), []string{ + "microsoft.windows.hyperv.compute", + "some-bogus-provider", + }) + if err == nil { + t.Errorf("fail-close mode should deny when any provider is unknown") } } diff --git a/pkg/securitypolicy/securitypolicy.go b/pkg/securitypolicy/securitypolicy.go index 5b05160492..7f5d2ac39b 100644 --- a/pkg/securitypolicy/securitypolicy.go +++ b/pkg/securitypolicy/securitypolicy.go @@ -67,6 +67,12 @@ type PolicyConfig struct { // all containers within a pod to be run without scratch encryption. AllowUnencryptedScratch bool `json:"allow_unencrypted_scratch" toml:"allow_unencrypted_scratch"` AllowCapabilityDropping bool `json:"allow_capability_dropping" toml:"allow_capability_dropping"` + // AllowLogProviderDropping controls how EnforceLogProviderPolicy handles + // requested ETW providers that are not on the allow-list. When false + // (default, fail-close) any disallowed provider causes the entire + // modifyServiceSettings call to be denied. When true, disallowed providers + // are silently dropped and the call continues with the kept subset. + AllowLogProviderDropping bool `json:"allow_log_provider_dropping" toml:"allow_log_provider_dropping"` } func NewPolicyConfig(opts ...PolicyConfigOpt) (*PolicyConfig, error) { diff --git a/pkg/securitypolicy/securitypolicy_internal.go b/pkg/securitypolicy/securitypolicy_internal.go index c736fb58ed..42c10dbce0 100644 --- a/pkg/securitypolicy/securitypolicy_internal.go +++ b/pkg/securitypolicy/securitypolicy_internal.go @@ -19,6 +19,7 @@ type securityPolicyInternal struct { AllowEnvironmentVariableDropping bool AllowUnencryptedScratch bool AllowCapabilityDropping bool + AllowLogProviderDropping bool } // Internal version of Windows SecurityPolicy @@ -32,6 +33,7 @@ type securityPolicyWindowsInternal struct { AllowEnvironmentVariableDropping bool AllowUnencryptedScratch bool AllowCapabilityDropping bool + AllowLogProviderDropping bool } type securityPolicyFragment struct { @@ -97,6 +99,7 @@ func newSecurityPolicyInternal( allowDropEnvironmentVariables bool, allowUnencryptedScratch bool, allowDropCapabilities bool, + allowLogProviderDropping bool, ) (*securityPolicyInternal, error) { containersInternal, err := containersToInternal(containers) if err != nil { @@ -113,6 +116,7 @@ func newSecurityPolicyInternal( AllowEnvironmentVariableDropping: allowDropEnvironmentVariables, AllowUnencryptedScratch: allowUnencryptedScratch, AllowCapabilityDropping: allowDropCapabilities, + AllowLogProviderDropping: allowLogProviderDropping, }, nil } diff --git a/pkg/securitypolicy/securitypolicy_marshal.go b/pkg/securitypolicy/securitypolicy_marshal.go index 665dc9e4f0..9db7e3f19d 100644 --- a/pkg/securitypolicy/securitypolicy_marshal.go +++ b/pkg/securitypolicy/securitypolicy_marshal.go @@ -67,6 +67,7 @@ type OSAwareMarshalFunc func( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilityDropping bool, + allowLogProviderDropping bool, ) (string, error) // osAwareMarshalRego handles both Linux and Windows containers @@ -83,6 +84,7 @@ func osAwareMarshalRego( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilityDropping bool, + allowLogProviderDropping bool, ) (string, error) { if allowAll { if len(linuxContainers) > 0 || len(windowsContainers) > 0 { @@ -98,7 +100,8 @@ func osAwareMarshalRego( } return marshalRego(allowAll, linuxContainers, externalProcesses, fragments, allowPropertiesAccess, allowDumpStacks, allowRuntimeLogging, - allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping) + allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping, + allowLogProviderDropping) case "windows": if len(linuxContainers) > 0 { @@ -106,7 +109,8 @@ func osAwareMarshalRego( } return marshalWindowsRego(allowAll, windowsContainers, externalProcesses, fragments, allowPropertiesAccess, allowDumpStacks, allowRuntimeLogging, - allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping) + allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping, + allowLogProviderDropping) default: return "", fmt.Errorf("unsupported OS type: %s", osType) @@ -125,6 +129,7 @@ func marshalWindowsRego( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilityDropping bool, + allowLogProviderDropping bool, ) (string, error) { if allowAll { if len(containers) > 0 { @@ -149,6 +154,7 @@ func marshalWindowsRego( AllowEnvironmentVariableDropping: allowEnvironmentVariableDropping, AllowUnencryptedScratch: allowUnencryptedScratch, AllowCapabilityDropping: allowCapabilityDropping, + AllowLogProviderDropping: allowLogProviderDropping, } return policy.marshalWindowsRego(), nil @@ -167,6 +173,7 @@ func marshalJSON( _ bool, _ bool, _ bool, + _ bool, ) (string, error) { var policy *SecurityPolicy if allowAll { @@ -198,6 +205,7 @@ func marshalRego( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapabilityDropping bool, + allowLogProviderDropping bool, ) (string, error) { if allowAll { if len(containers) > 0 { @@ -217,6 +225,7 @@ func marshalRego( allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapabilityDropping, + allowLogProviderDropping, ) if err != nil { return "", err @@ -251,6 +260,7 @@ func MarshalPolicy( allowEnvironmentVariableDropping bool, allowUnencryptedScratch bool, allowCapbilitiesDropping bool, + allowLogProviderDropping bool, ) (string, error) { if marshaller == "" { marshaller = defaultMarshaller @@ -272,6 +282,7 @@ func MarshalPolicy( allowEnvironmentVariableDropping, allowUnencryptedScratch, allowCapbilitiesDropping, + allowLogProviderDropping, ) } } @@ -596,6 +607,7 @@ func (p securityPolicyInternal) marshalRego() string { writeLine(builder, "allow_environment_variable_dropping := %t", p.AllowEnvironmentVariableDropping) writeLine(builder, "allow_unencrypted_scratch := %t", p.AllowUnencryptedScratch) writeLine(builder, "allow_capability_dropping := %t", p.AllowCapabilityDropping) + writeLine(builder, "allow_log_provider_dropping := %t", p.AllowLogProviderDropping) result := strings.Replace(policyRegoTemplate, "@@OBJECTS@@", builder.String(), 1) result = strings.Replace(result, "@@API_VERSION@@", apiVersion, 1) result = strings.Replace(result, "@@FRAMEWORK_VERSION@@", frameworkVersion, 1) @@ -621,6 +633,7 @@ func (p securityPolicyWindowsInternal) marshalWindowsRego() string { writeLine(builder, "allow_environment_variable_dropping := %t", p.AllowEnvironmentVariableDropping) writeLine(builder, "allow_unencrypted_scratch := %t", p.AllowUnencryptedScratch) writeLine(builder, "allow_capability_dropping := %t", p.AllowCapabilityDropping) + writeLine(builder, "allow_log_provider_dropping := %t", p.AllowLogProviderDropping) result := strings.Replace(policyRegoTemplate, "@@OBJECTS@@", builder.String(), 1) result = strings.Replace(result, "@@API_VERSION@@", apiVersion, 1) result = strings.Replace(result, "@@FRAMEWORK_VERSION@@", frameworkVersion, 1) diff --git a/pkg/securitypolicy/securitypolicy_options.go b/pkg/securitypolicy/securitypolicy_options.go index 35dd755367..9a21fd4e9f 100644 --- a/pkg/securitypolicy/securitypolicy_options.go +++ b/pkg/securitypolicy/securitypolicy_options.go @@ -40,6 +40,28 @@ func NewSecurityOptions(enforcer SecurityPolicyEnforcer, enforcerSet bool, uvmRe } } +// LockDown irreversibly replaces the active policy enforcer with a closed-door +// (deny-everything) enforcer in response to a policy violation. All subsequent +// Enforce* calls will deny. +// +// LockDown is idempotent: subsequent calls after the first are no-ops. +// +// LockDown also sets PolicyEnforcerSet so that a subsequent +// SetConfidentialOptions call (which checks PolicyEnforcerSet and refuses to +// install a policy if it is already set) cannot replace the closed-door +// enforcer with a permissive one. This makes lockdown sticky regardless of +// whether LockDown was called before or after the initial policy load. +func (s *SecurityOptions) LockDown(ctx context.Context) { + s.policyMutex.Lock() + defer s.policyMutex.Unlock() + if _, alreadyLocked := s.PolicyEnforcer.(*ClosedDoorSecurityPolicyEnforcer); alreadyLocked { + return + } + log.G(ctx).Error("policy violation: locking down sidecar enforcer to closed-door; all subsequent policy decisions will deny") + s.PolicyEnforcer = &ClosedDoorSecurityPolicyEnforcer{} + s.PolicyEnforcerSet = true +} + // SetConfidentialOptions takes guestresource.ConfidentialOptions // to set up our internal data structures we use to store and enforce // security policy. The options can contain security policy enforcer type, diff --git a/pkg/securitypolicy/securitypolicy_options_test.go b/pkg/securitypolicy/securitypolicy_options_test.go new file mode 100644 index 0000000000..865243c08f --- /dev/null +++ b/pkg/securitypolicy/securitypolicy_options_test.go @@ -0,0 +1,86 @@ +package securitypolicy + +import ( + "context" + "io" + "testing" +) + +// TestLockDown_SwapsEnforcerToClosedDoor verifies that LockDown replaces +// the active enforcer with a ClosedDoorSecurityPolicyEnforcer. +func TestLockDown_SwapsEnforcerToClosedDoor(t *testing.T) { + opts := NewSecurityOptions(&OpenDoorSecurityPolicyEnforcer{}, true, "", io.Discard) + + if _, ok := opts.PolicyEnforcer.(*OpenDoorSecurityPolicyEnforcer); !ok { + t.Fatalf("setup: expected initial enforcer to be OpenDoor, got %T", opts.PolicyEnforcer) + } + + opts.LockDown(context.Background()) + + if _, ok := opts.PolicyEnforcer.(*ClosedDoorSecurityPolicyEnforcer); !ok { + t.Errorf("after LockDown: expected ClosedDoor enforcer, got %T", opts.PolicyEnforcer) + } +} + +// TestLockDown_Idempotent verifies that LockDown is a no-op once already locked. +// The same enforcer instance is preserved on the second call. +func TestLockDown_Idempotent(t *testing.T) { + opts := NewSecurityOptions(&OpenDoorSecurityPolicyEnforcer{}, true, "", io.Discard) + ctx := context.Background() + + opts.LockDown(ctx) + firstLocked := opts.PolicyEnforcer + + opts.LockDown(ctx) + secondLocked := opts.PolicyEnforcer + + if firstLocked != secondLocked { + t.Errorf("LockDown should be idempotent: enforcer pointer changed between calls (%p -> %p)", firstLocked, secondLocked) + } +} + +// TestLockDown_SetsPolicyEnforcerSet verifies that LockDown marks the policy +// as set, so that a subsequent SetConfidentialOptions call cannot replace +// the closed-door enforcer with a permissive one. +// +// SetConfidentialOptions short-circuits with an error when PolicyEnforcerSet +// is already true; this is the mechanism that makes LockDown sticky against +// future policy-install attempts regardless of call order. +func TestLockDown_SetsPolicyEnforcerSet(t *testing.T) { + // Construct with PolicyEnforcerSet=false to simulate LockDown being + // called before the initial policy was loaded. + opts := NewSecurityOptions(&OpenDoorSecurityPolicyEnforcer{}, false, "", io.Discard) + + if opts.PolicyEnforcerSet { + t.Fatal("setup: expected PolicyEnforcerSet=false before LockDown") + } + + opts.LockDown(context.Background()) + + if !opts.PolicyEnforcerSet { + t.Error("expected PolicyEnforcerSet=true after LockDown so subsequent SetConfidentialOptions refuses to install a policy") + } +} + +// TestLockDown_StickyAgainstSetConfidentialOptions verifies the end-to-end +// stickiness property: after LockDown, SetConfidentialOptions refuses to +// install a new (potentially permissive) policy and the enforcer remains +// closed-door. +func TestLockDown_StickyAgainstSetConfidentialOptions(t *testing.T) { + opts := NewSecurityOptions(&OpenDoorSecurityPolicyEnforcer{}, false, "", io.Discard) + ctx := context.Background() + + opts.LockDown(ctx) + + // Try to install a fresh policy after lockdown. The actual policy + // content does not matter — SetConfidentialOptions should refuse on + // the PolicyEnforcerSet check before touching anything else. + err := opts.SetConfidentialOptions(ctx, "", "", "") + if err == nil { + t.Fatal("expected SetConfidentialOptions to refuse after LockDown") + } + + if _, ok := opts.PolicyEnforcer.(*ClosedDoorSecurityPolicyEnforcer); !ok { + t.Errorf("after LockDown + SetConfidentialOptions: enforcer was replaced; got %T, want ClosedDoor", opts.PolicyEnforcer) + } +} diff --git a/pkg/securitypolicy/securitypolicyenforcer.go b/pkg/securitypolicy/securitypolicyenforcer.go index 63a3b9e92f..37f77a81f4 100644 --- a/pkg/securitypolicy/securitypolicyenforcer.go +++ b/pkg/securitypolicy/securitypolicyenforcer.go @@ -128,7 +128,14 @@ type SecurityPolicyEnforcer interface { GetUserInfo(spec *oci.Process, rootPath string) (IDName, []IDName, string, error) EnforceVerifiedCIMsPolicy(ctx context.Context, containerID string, layerHashes []string, mountedCim []string) (err error) EnforceRegistryChangesPolicy(ctx context.Context, containerID string, registryValues interface{}) error - EnforceLogProviderPolicy(ctx context.Context, providerName string) error + // EnforceLogProviderPolicy validates a batch of requested ETW provider + // names against the policy's allowed_log_providers list. It returns the + // subset of provider names that the caller should forward to the inbox + // GCS, plus any policy error. When the policy has + // allow_log_provider_dropping := true, providers not on the allow-list are + // silently dropped from the returned slice; otherwise the whole call is + // denied (returning a non-nil error) if any provider is not allowed. + EnforceLogProviderPolicy(ctx context.Context, providerNames []string) ([]string, error) } //nolint:unused @@ -325,8 +332,8 @@ func (OpenDoorSecurityPolicyEnforcer) EnforceRegistryChangesPolicy(ctx context.C return nil } -func (OpenDoorSecurityPolicyEnforcer) EnforceLogProviderPolicy(context.Context, string) error { - return nil +func (OpenDoorSecurityPolicyEnforcer) EnforceLogProviderPolicy(_ context.Context, providerNames []string) ([]string, error) { + return providerNames, nil } type ClosedDoorSecurityPolicyEnforcer struct{} @@ -458,6 +465,6 @@ func (ClosedDoorSecurityPolicyEnforcer) EnforceRegistryChangesPolicy(ctx context return errors.New("registry changes are denied by policy") } -func (ClosedDoorSecurityPolicyEnforcer) EnforceLogProviderPolicy(context.Context, string) error { - return errors.New("log provider is denied by policy") +func (ClosedDoorSecurityPolicyEnforcer) EnforceLogProviderPolicy(context.Context, []string) ([]string, error) { + return nil, errors.New("log provider is denied by policy") } diff --git a/pkg/securitypolicy/securitypolicyenforcer_rego.go b/pkg/securitypolicy/securitypolicyenforcer_rego.go index 718e0044d9..6f255a33ec 100644 --- a/pkg/securitypolicy/securitypolicyenforcer_rego.go +++ b/pkg/securitypolicy/securitypolicyenforcer_rego.go @@ -580,6 +580,38 @@ func getEnvsToKeep(envList []string, results rpi.RegoQueryResult) ([]string, err return keepSet.toArray(), nil } +// getProvidersToKeep extracts the "providers_to_keep" field from the rego +// query results for log_provider enforcement and intersects it (defensively) +// with the originally requested providerNames. Mirrors getEnvsToKeep. +// +// When the policy is older / does not return providers_to_keep, the entire +// requested list is kept (analogous to env_list). +func getProvidersToKeep(providerNames []string, results rpi.RegoQueryResult) ([]string, error) { + value, err := results.Value("providers_to_keep") + if err != nil || value == nil { + // policy did not return 'providers_to_keep'. Interpret as + // "proceed with provided providers". + return providerNames, nil + } + + providersAsInterfaces, ok := value.([]interface{}) + if !ok { + return nil, fmt.Errorf("policy returned incorrect type for 'providers_to_keep', expected []interface{}, received %T", value) + } + + keepSet := make(stringSet) + for _, providerAsInterface := range providersAsInterfaces { + if provider, ok := providerAsInterface.(string); ok { + keepSet.add(provider) + } else { + return nil, fmt.Errorf("members of providers_to_keep from policy must be strings, received %T", providerAsInterface) + } + } + + keepSet = keepSet.intersect(toStringSet(providerNames)) + return keepSet.toArray(), nil +} + func getCapsToKeep(capsList *oci.LinuxCapabilities, results rpi.RegoQueryResult) (*oci.LinuxCapabilities, error) { value, err := results.Value("caps_list") if err != nil || value == nil { @@ -1188,12 +1220,15 @@ func (policy *regoEnforcer) EnforceRegistryChangesPolicy(ctx context.Context, co return err } -func (policy *regoEnforcer) EnforceLogProviderPolicy(ctx context.Context, providerName string) error { +func (policy *regoEnforcer) EnforceLogProviderPolicy(ctx context.Context, providerNames []string) ([]string, error) { input := inputData{ - "providerName": providerName, + "providers": providerNames, } - _, err := policy.enforce(ctx, "log_provider", input) - return err + results, err := policy.enforce(ctx, "log_provider", input) + if err != nil { + return nil, err + } + return getProvidersToKeep(providerNames, results) } func (policy *regoEnforcer) GetUserInfo(process *oci.Process, rootPath string) (IDName, []IDName, string, error) { diff --git a/test/pkg/securitypolicy/policy.go b/test/pkg/securitypolicy/policy.go index eeb93f67c8..bdf748bac6 100644 --- a/test/pkg/securitypolicy/policy.go +++ b/test/pkg/securitypolicy/policy.go @@ -64,6 +64,7 @@ func PolicyWithOpts(tb testing.TB, policyType string, pOpts ...securitypolicy.Po config.AllowEnvironmentVariableDropping, config.AllowUnencryptedScratch, config.AllowCapabilityDropping, + config.AllowLogProviderDropping, ) if err != nil { tb.Fatal(err) From 146271584c01101dcab2fb9f25da9ed65a257c10 Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Mon, 1 Jun 2026 10:48:31 +0100 Subject: [PATCH 4/8] CWCOW: warn when log providers are trimmed by policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When allow_log_provider_dropping is enabled and the policy returns a strict subset of the requested providers, the sidecar silently rebuilt the LogSourcesInfo payload, leaving operators with no signal that any provider had been dropped — and under typical confidential setups forwardlogs itself may be off, so the trim was effectively invisible. Emit a single Warn at the moment of trimming with the requested, kept, and dropped provider names. The log still lands inside the UVM and is reachable via shimdiag even when forwardlogs is disabled. Signed-off-by: Takuro Sato --- internal/gcs-sidecar/handlers.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/internal/gcs-sidecar/handlers.go b/internal/gcs-sidecar/handlers.go index 0165bc71e0..1b8b1921dc 100644 --- a/internal/gcs-sidecar/handlers.go +++ b/internal/gcs-sidecar/handlers.go @@ -585,10 +585,26 @@ func (b *Bridge) modifyServiceSettings(req *request) (err error) { payload := settings.Settings if len(keptNames) != len(requestedNames) { - // Subset kept. Trim each source's provider list to - // only the allowed names, then re-encode for the - // inbox GCS. Empty sources are preserved to keep the - // shape stable; inbox GCS handles them as no-ops. + // Subset kept. Surface the drop so operators have a + // breadcrumb — under allow_log_provider_dropping the + // pod boots silently, and forwardlogs may itself be + // off, so without this warning the trim is invisible. + dropped := make([]string, 0, len(requestedNames)-len(keptNames)) + for _, name := range requestedNames { + if _, ok := keepSet[name]; !ok { + dropped = append(dropped, name) + } + } + log.G(req.ctx).WithFields(map[string]interface{}{ + "requested": requestedNames, + "kept": keptNames, + "dropped": dropped, + }).Warn("log providers trimmed by policy (allow_log_provider_dropping)") + + // Trim each source's provider list to only the + // allowed names, then re-encode for the inbox GCS. + // Empty sources are preserved to keep the shape + // stable; inbox GCS handles them as no-ops. trimmed := logSources for i := range trimmed.LogConfig.Sources { src := &trimmed.LogConfig.Sources[i] From 7371616ee4c186e3be62cd592184019140d8bf4e Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Mon, 1 Jun 2026 12:48:21 +0100 Subject: [PATCH 5/8] ci: empty commit to retrigger checks Signed-off-by: Takuro Sato From 346bfb003f3f0b8934db5e292bc70faa19df602c Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Mon, 1 Jun 2026 14:05:51 +0100 Subject: [PATCH 6/8] securitypolicy: always set PolicyEnforcerSet in LockDown Without this, LockDown on a ClosedDoor enforcer with PolicyEnforcerSet=false (the sidecar's boot-time state) leaves the flag unset, and a later SetConfidentialOptions can still install a permissive policy. Signed-off-by: Takuro Sato --- pkg/securitypolicy/securitypolicy_options.go | 16 +++++++++--- .../securitypolicy_options_test.go | 26 +++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/pkg/securitypolicy/securitypolicy_options.go b/pkg/securitypolicy/securitypolicy_options.go index 9a21fd4e9f..a04b08f16c 100644 --- a/pkg/securitypolicy/securitypolicy_options.go +++ b/pkg/securitypolicy/securitypolicy_options.go @@ -44,22 +44,30 @@ func NewSecurityOptions(enforcer SecurityPolicyEnforcer, enforcerSet bool, uvmRe // (deny-everything) enforcer in response to a policy violation. All subsequent // Enforce* calls will deny. // -// LockDown is idempotent: subsequent calls after the first are no-ops. +// LockDown is idempotent with respect to the enforcer swap: if the enforcer is +// already a ClosedDoorSecurityPolicyEnforcer the swap and the error log are +// skipped. // -// LockDown also sets PolicyEnforcerSet so that a subsequent +// LockDown also sets PolicyEnforcerSet unconditionally so that a subsequent // SetConfidentialOptions call (which checks PolicyEnforcerSet and refuses to // install a policy if it is already set) cannot replace the closed-door // enforcer with a permissive one. This makes lockdown sticky regardless of -// whether LockDown was called before or after the initial policy load. +// whether LockDown was called before or after the initial policy load, and in +// particular covers the boot-time case where the sidecar starts with a +// ClosedDoorSecurityPolicyEnforcer but PolicyEnforcerSet is still false. func (s *SecurityOptions) LockDown(ctx context.Context) { s.policyMutex.Lock() defer s.policyMutex.Unlock() + // Mark the policy as set first, unconditionally. This is the property that + // makes lockdown sticky against a follow-up SetConfidentialOptions, and it + // must hold even when the enforcer is already a ClosedDoor instance + // (e.g. the sidecar's boot-time default before any user policy arrives). + s.PolicyEnforcerSet = true if _, alreadyLocked := s.PolicyEnforcer.(*ClosedDoorSecurityPolicyEnforcer); alreadyLocked { return } log.G(ctx).Error("policy violation: locking down sidecar enforcer to closed-door; all subsequent policy decisions will deny") s.PolicyEnforcer = &ClosedDoorSecurityPolicyEnforcer{} - s.PolicyEnforcerSet = true } // SetConfidentialOptions takes guestresource.ConfidentialOptions diff --git a/pkg/securitypolicy/securitypolicy_options_test.go b/pkg/securitypolicy/securitypolicy_options_test.go index 865243c08f..71e71c3bf9 100644 --- a/pkg/securitypolicy/securitypolicy_options_test.go +++ b/pkg/securitypolicy/securitypolicy_options_test.go @@ -84,3 +84,29 @@ func TestLockDown_StickyAgainstSetConfidentialOptions(t *testing.T) { t.Errorf("after LockDown + SetConfidentialOptions: enforcer was replaced; got %T, want ClosedDoor", opts.PolicyEnforcer) } } + +// TestLockDown_StickyFromBootClosedDoor covers the boot-time case the +// sidecar actually hits: the enforcer is already a ClosedDoor instance (the +// PSP fail-close default in cmd/gcs-sidecar/main.go) and PolicyEnforcerSet +// is still false because no user policy has been loaded yet. LockDown must +// still flip PolicyEnforcerSet so that a follow-up SetConfidentialOptions +// is refused, otherwise a permissive policy could replace the closed door. +func TestLockDown_StickyFromBootClosedDoor(t *testing.T) { + opts := NewSecurityOptions(&ClosedDoorSecurityPolicyEnforcer{}, false, "", io.Discard) + ctx := context.Background() + + opts.LockDown(ctx) + + if !opts.PolicyEnforcerSet { + t.Fatal("expected PolicyEnforcerSet=true after LockDown on a boot-time ClosedDoor enforcer; otherwise SetConfidentialOptions would still accept a fresh policy") + } + + err := opts.SetConfidentialOptions(ctx, "", "", "") + if err == nil { + t.Fatal("expected SetConfidentialOptions to refuse after LockDown on a boot-time ClosedDoor enforcer") + } + + if _, ok := opts.PolicyEnforcer.(*ClosedDoorSecurityPolicyEnforcer); !ok { + t.Errorf("after LockDown + SetConfidentialOptions: enforcer was replaced; got %T, want ClosedDoor", opts.PolicyEnforcer) + } +} From 7e5929a94454b0351fe816d36b11b2205f2ef1f2 Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Mon, 1 Jun 2026 14:24:30 +0100 Subject: [PATCH 7/8] gcs-sidecar: detect log-provider trim via missing names, not length The rego enforcer returns providers_to_keep as a set, so a request like [A, A] against an allowlist of [A] came back as [A] and tripped a spurious warning + re-marshal. Scan requestedNames against keepSet. Signed-off-by: Takuro Sato --- internal/gcs-sidecar/handlers.go | 36 ++++++++++----- internal/gcs-sidecar/handlers_test.go | 63 +++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 11 deletions(-) diff --git a/internal/gcs-sidecar/handlers.go b/internal/gcs-sidecar/handlers.go index 1b8b1921dc..1e2c26f1db 100644 --- a/internal/gcs-sidecar/handlers.go +++ b/internal/gcs-sidecar/handlers.go @@ -583,18 +583,32 @@ func (b *Bridge) modifyServiceSettings(req *request) (err error) { keepSet[name] = struct{}{} } - payload := settings.Settings - if len(keptNames) != len(requestedNames) { - // Subset kept. Surface the drop so operators have a - // breadcrumb — under allow_log_provider_dropping the - // pod boots silently, and forwardlogs may itself be - // off, so without this warning the trim is invisible. - dropped := make([]string, 0, len(requestedNames)-len(keptNames)) - for _, name := range requestedNames { - if _, ok := keepSet[name]; !ok { - dropped = append(dropped, name) - } + // Detect trimming by scanning requested names against + // keepSet. We cannot use len(kept) != len(requested): + // the rego enforcer returns providers_to_keep via a set + // (see getProvidersToKeep → keepSet.toArray()), so a + // duplicate-name request like [A, A, B] returns [A, B] + // even when nothing was dropped, which would otherwise + // trip a false-positive warning and a needless re-marshal. + dropped := make([]string, 0) + seenDropped := make(map[string]struct{}) + for _, name := range requestedNames { + if _, ok := keepSet[name]; ok { + continue + } + if _, dup := seenDropped[name]; dup { + continue } + seenDropped[name] = struct{}{} + dropped = append(dropped, name) + } + + payload := settings.Settings + if len(dropped) > 0 { + // Surface the drop so operators have a breadcrumb — + // under allow_log_provider_dropping the pod boots + // silently, and forwardlogs may itself be off, so + // without this warning the trim is invisible. log.G(req.ctx).WithFields(map[string]interface{}{ "requested": requestedNames, "kept": keptNames, diff --git a/internal/gcs-sidecar/handlers_test.go b/internal/gcs-sidecar/handlers_test.go index 843d062f0c..fbbda0e51f 100644 --- a/internal/gcs-sidecar/handlers_test.go +++ b/internal/gcs-sidecar/handlers_test.go @@ -17,6 +17,7 @@ import ( "github.com/Microsoft/hcsshim/internal/protocol/guestresource" "github.com/Microsoft/hcsshim/internal/vm/vmutils/etw" "github.com/Microsoft/hcsshim/pkg/securitypolicy" + "github.com/sirupsen/logrus" ) // buildModifySettingsRequest creates a serialized ModifySettings request message @@ -522,3 +523,65 @@ func TestModifyServiceSettings_LogForward_PolicyDropping_TrimsForwardedPayload(t t.Errorf("expected dropped provider %q to be absent from forwarded payload", dropped) } } + +// captureHook is a tiny logrus hook that records every entry it sees. +// Used by TestModifyServiceSettings_LogForward_PolicyDropping_NoFalsePositive +// to assert the "log providers trimmed by policy" Warn is *not* emitted when +// the only reason kept and requested differ is set-deduplication. +type captureHook struct { + entries []*logrus.Entry +} + +func (h *captureHook) Levels() []logrus.Level { return logrus.AllLevels } +func (h *captureHook) Fire(e *logrus.Entry) error { + h.entries = append(h.entries, e) + return nil +} + +// TestModifyServiceSettings_LogForward_PolicyDropping_NoFalsePositive guards +// against a false-positive trim warning + needless re-marshal when the +// enforcer returns a deduplicated set. The rego implementation builds +// providers_to_keep via a stringSet (see getProvidersToKeep), so a request +// with duplicate provider names like [A, A] comes back as [A] even when +// nothing was actually dropped. Detection must be based on "some requested +// name is missing from keepSet", not len(kept) != len(requested). +func TestModifyServiceSettings_LogForward_PolicyDropping_NoFalsePositive(t *testing.T) { + name := "microsoft.windows.hyperv.compute" + enforcer := &droppingLogProviderEnforcer{ + allowed: map[string]struct{}{name: {}}, + } + b := newTestBridge(enforcer) + + // Two copies of the same allowed provider. dedup in the enforcer means + // kept=[name] while requested=[name, name]; the lengths differ but the + // set of requested names is fully covered, so this is NOT a trim. + payload := buildLogForwardServiceRequest(t, name, name) + req := newModifyServiceSettingsRequest(payload) + + hook := &captureHook{} + logrus.AddHook(hook) + defer func() { + // logrus has no public RemoveHook; reset all hooks to clear ours. + logrus.StandardLogger().ReplaceHooks(logrus.LevelHooks{}) + }() + + if err := b.modifyServiceSettings(req); err != nil { + t.Fatalf("modifyServiceSettings under dropping enforcer (dedup) returned error: %v", err) + } + + // Must forward to GCS. + select { + case <-b.sendToGCSCh: + case <-time.After(time.Second): + t.Fatal("timed out waiting for request to be forwarded to GCS") + } + + // Must NOT have emitted the trim warning: nothing was actually dropped. + for _, e := range hook.entries { + if e.Level == logrus.WarnLevel && + e.Message == "log providers trimmed by policy (allow_log_provider_dropping)" { + t.Errorf("false-positive trim warning emitted on a dedup-only mismatch (kept=%v requested=%v dropped=%v)", + e.Data["kept"], e.Data["requested"], e.Data["dropped"]) + } + } +} From 6f69276a0bc2e365e25cd6dd24d3196332683794 Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Mon, 1 Jun 2026 14:33:25 +0100 Subject: [PATCH 8/8] etw: add UpdateLogSourcesFromInfo to skip redundant decode The sidecar already decodes the base64+JSON payload to enforce log_provider policy. Hand the parsed LogSourcesInfo to a new UpdateLogSourcesFromInfo helper instead of re-encoding so the inbox prep can decode it again. UpdateLogSources is reimplemented on top. Signed-off-by: Takuro Sato --- internal/gcs-sidecar/handlers.go | 20 ++++++++------------ internal/vm/vmutils/etw/provider_map.go | 24 +++++++++++++++++------- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/internal/gcs-sidecar/handlers.go b/internal/gcs-sidecar/handlers.go index 1e2c26f1db..65edccd6cd 100644 --- a/internal/gcs-sidecar/handlers.go +++ b/internal/gcs-sidecar/handlers.go @@ -4,7 +4,6 @@ package bridge import ( - "encoding/base64" "encoding/hex" "encoding/json" "fmt" @@ -603,7 +602,11 @@ func (b *Bridge) modifyServiceSettings(req *request) (err error) { dropped = append(dropped, name) } - payload := settings.Settings + // Trim happens in-place on the parsed structure so we can + // hand it to UpdateLogSourcesFromInfo without a redundant + // base64-decode + JSON-unmarshal round-trip (we already + // decoded above for enforcement). + trimmed := logSources if len(dropped) > 0 { // Surface the drop so operators have a breadcrumb — // under allow_log_provider_dropping the pod boots @@ -616,10 +619,8 @@ func (b *Bridge) modifyServiceSettings(req *request) (err error) { }).Warn("log providers trimmed by policy (allow_log_provider_dropping)") // Trim each source's provider list to only the - // allowed names, then re-encode for the inbox GCS. - // Empty sources are preserved to keep the shape - // stable; inbox GCS handles them as no-ops. - trimmed := logSources + // allowed names. Empty sources are preserved to keep + // the shape stable; inbox GCS handles them as no-ops. for i := range trimmed.LogConfig.Sources { src := &trimmed.LogConfig.Sources[i] filtered := make([]etw.EtwProvider, 0, len(src.Providers)) @@ -630,17 +631,12 @@ func (b *Bridge) modifyServiceSettings(req *request) (err error) { } src.Providers = filtered } - trimmedJSON, err := json.Marshal(trimmed) - if err != nil { - return fmt.Errorf("failed to marshal trimmed log sources: %w", err) - } - payload = base64.StdEncoding.EncodeToString(trimmedJSON) } // Apply GUID resolution (and any other inbox-GCS prep) // against the policy-trimmed payload and hand off to // inbox GCS. - allowedLogSources, err := etw.UpdateLogSources(payload, false, true) + allowedLogSources, err := etw.UpdateLogSourcesFromInfo(trimmed, false, true) if err != nil { return fmt.Errorf("failed to update log sources: %w", err) } diff --git a/internal/vm/vmutils/etw/provider_map.go b/internal/vm/vmutils/etw/provider_map.go index 962d0c586a..f07d254af5 100644 --- a/internal/vm/vmutils/etw/provider_map.go +++ b/internal/vm/vmutils/etw/provider_map.go @@ -188,19 +188,29 @@ func marshalAndEncodeLogSources(logCfg LogSourcesInfo) (string, error) { // configuration and returns the updated log sources as a base64 encoded JSON string. // If there is an error in the process, it returns the original user provided log sources string. func UpdateLogSources(base64EncodedJSONLogConfig string, useDefaultLogSources bool, includeGUIDs bool) (string, error) { - var resultLogCfg LogSourcesInfo - if useDefaultLogSources { - resultLogCfg = defaultLogSourcesInfo - } - + var userLogSources LogSourcesInfo if base64EncodedJSONLogConfig != "" { - userLogSources, err := DecodeAndUnmarshalLogSources(base64EncodedJSONLogConfig) + var err error + userLogSources, err = DecodeAndUnmarshalLogSources(base64EncodedJSONLogConfig) if err != nil { return "", fmt.Errorf("failed to decode and unmarshal user log sources: %w", err) } - resultLogCfg.LogConfig.Sources = mergeLogSources(resultLogCfg.LogConfig.Sources, userLogSources.LogConfig.Sources) + } + return UpdateLogSourcesFromInfo(userLogSources, useDefaultLogSources, includeGUIDs) +} +// UpdateLogSourcesFromInfo is the parsed-input variant of UpdateLogSources for +// callers that already have a LogSourcesInfo in hand (e.g. the gcs-sidecar +// after it decoded the payload for policy enforcement) and want to avoid a +// second base64-decode + JSON-unmarshal round-trip. +// +// An empty userLogSources is equivalent to passing "" to UpdateLogSources. +func UpdateLogSourcesFromInfo(userLogSources LogSourcesInfo, useDefaultLogSources bool, includeGUIDs bool) (string, error) { + var resultLogCfg LogSourcesInfo + if useDefaultLogSources { + resultLogCfg = defaultLogSourcesInfo } + resultLogCfg.LogConfig.Sources = mergeLogSources(resultLogCfg.LogConfig.Sources, userLogSources.LogConfig.Sources) var err error resultLogCfg.LogConfig.Sources, err = applyGUIDPolicy(resultLogCfg.LogConfig.Sources, includeGUIDs)