CWCOW: Logging enforcement by takuro-sato · Pull Request #2763 · microsoft/hcsshim

takuro-sato · 2026-06-01T10:55:31Z

Based on #2728.

Changes

Adds the log_provider enforcement point so the gcs-sidecar checks every
ETW provider name forwarded by the host against allowed_log_providers in
the signed CWCOW rego policy.

Two modes (mirrors allow_environment_variable_dropping):

allow_log_provider_dropping := false (default, fail-close): if any
requested provider is not in the allow-list, modifyServiceSettings
returns a policy-decision error, the sidecar enforcer is locked down to
closed-door, and the host then tears the UVM down. No log forwarding
happens.
allow_log_provider_dropping := true: providers not in the allow-list
are silently dropped; the sidecar forwards only the remaining subset to
the inbox GCS and emits a warning naming what was kept vs dropped.

LockDown also marks PolicyEnforcerSet = true, so a later
SetConfidentialOptions cannot overwrite the closed-door enforcer with
policy-derived state.

Edit: Discussed with Ken about it and we decided to remove it. 155f9b3 That way it's consistent with existing enforcement points like exec command. i.e. gcs sidecar allows host to fails with policy. It rejects the operation but the UVM can be alive.

Test

New and existing unit tests pass.
Manually verified on an SNP-capable test VM:
- Allow-listed provider → pod starts and the attestation URL responds.
- Disallowed provider with allow_log_provider_dropping := false → pod
  start fails with log providers denied by policy and the decoded
  policyDecision payload identifies the offending provider.
- Mixed allowed + disallowed providers with
  allow_log_provider_dropping := true → pod starts and the gcs-sidecar
  log contains log providers trimmed by policy with the kept / dropped
  sets.

TODOs for future PRs.

Not directly related to this PR, but you can see a TODO in internal\gcs-sidecar\handlers.go // Todo: Add policy enforcement for modifying service settings. Indeed some requests for modifyServiceSetting don't have enforcement at the moment. It needs to be addressed.

	default:
		log.G(req.ctx).Warningf("modifyServiceSettings with PropertyType: %v, skipping policy enforcement", modifyRequest.PropertyType)

edit: Unknown requests are blocked per the PR discussion now.

Signed-off-by: Mahati Chamarthy <mahati.chamarthy@gmail.com>

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 <mahati.chamarthy@gmail.com>

…witch 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 <takurosato@microsoft.com>

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 <takurosato@microsoft.com>

Signed-off-by: Takuro Sato <takurosato@microsoft.com>

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR adds policy enforcement for Windows ETW log providers (including optional “drop disallowed providers” behavior) and introduces a “lock down” mechanism to fail-close after policy violations, plus test coverage for both features.

Changes:

Add EnforceLogProviderPolicy to the security policy enforcer (rego + open/closed door implementations) and plumb allow_log_provider_dropping through policy marshal/config.
Enforce log provider policy in the sidecar’s modifyServiceSettings for LogForwardService requests, trimming providers when configured and locking down on deny.
Add SecurityOptions.LockDown() and new tests for lockdown behavior and log-provider policy behavior.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
test/pkg/securitypolicy/policy.go	Plumbs the new `AllowLogProviderDropping` option into test policy construction.
pkg/securitypolicy/securitypolicyenforcer_rego.go	Adds rego enforcement entrypoint and result parsing for providers to keep.
pkg/securitypolicy/securitypolicyenforcer.go	Extends enforcer interface and implements default open/closed door behavior.
pkg/securitypolicy/securitypolicy_options.go	Introduces `LockDown()` to swap to closed-door enforcer and prevent later overrides.
pkg/securitypolicy/securitypolicy_options_test.go	Adds unit tests validating `LockDown()` semantics.
pkg/securitypolicy/securitypolicy_marshal.go	Threads `allow_log_provider_dropping` through marshalling and rego generation.
pkg/securitypolicy/securitypolicy_internal.go	Adds internal field for `AllowLogProviderDropping`.
pkg/securitypolicy/securitypolicy.go	Adds `AllowLogProviderDropping` to `PolicyConfig`.
pkg/securitypolicy/regopolicy_windows_test.go	Adds Windows rego policy tests for log provider allow/deny and dropping modes.
pkg/securitypolicy/regopolicy_linux_test.go	Updates Linux marshal test inputs to include the new flag.
pkg/securitypolicy/rego_utils_test.go	Updates generated constraints/policies to include the new flag.
pkg/securitypolicy/policy.rego	Adds `log_provider` enforcement point alias to framework rule.
pkg/securitypolicy/opts.go	Adds `WithAllowLogProviderDropping(...)` option.
pkg/securitypolicy/open_door.rego	Allows `log_provider` in open-door rego.
pkg/securitypolicy/framework.rego	Implements `log_provider` rule and error message, adds flag plumb.
pkg/securitypolicy/api.rego	Registers the `log_provider` enforcement point and default results for older APIs.
internal/vm/vmutils/etw/provider_map.go	Exports `DecodeAndUnmarshalLogSources` and updates callers.
internal/uvm/start.go	Makes log-forwarding fail-close during UVM start for confidential policy UVMs.
internal/tools/securitypolicy/main.go	Plumbs new allow-drop flag into tool marshalling calls.
internal/gcs-sidecar/handlers.go	Enforces log provider policy during LogForwardService modify settings; trims + locks down.
internal/gcs-sidecar/handlers_test.go	Adds tests for allow/deny/drop behavior and checks forwarding behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

takuro-sato · 2026-06-01T12:39:25Z

Adding @marma-dev to ask if the idea of allow_log_provider_dropping looks good.

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 <takurosato@microsoft.com>

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 <takurosato@microsoft.com>

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 <takurosato@microsoft.com>

MahatiC · 2026-06-03T09:29:01Z

@takuro-sato A note about the TODO re: ModifyServiceSettings enforcement on other Property type - the only valid one that passes through gcs-sidecar is "LogForwardService" and that is being handled in this PR, so that todo is no longer valid. (Manish wrote that todocomment, you could check with him too)

micromaomao

(I will probably handle this given Takuro is busy and is going on holiday)

micromaomao · 2026-06-04T23:37:57Z

Also should block anything other than modifying LogForwardService, and disallow other RPCType too (default: branch of switch settings.RPCType)

anmaxvl · 2026-06-08T07:03:34Z

+						}
+					}
+
+					keptNames, err := b.hostState.securityOptions.PolicyEnforcer.EnforceLogProviderPolicy(


feels like all the kept/dropped names and the logging of them should be encapsulated by the policy enforcement and the handlers should just forward the "final log sources request" to inbox GCS?

It follows the existing function getEnvsToKeep for env var. I think the dropping should be consistent with env var and they can stay like this at least for now. What do you think?

getEnvsToKeep returns a list of env-vars to keep and we do a simple assignment afterwards given what the enforcement point returns. I'm basically suggesting doing the same thing here, we enforce log provider policy, the enforcer returns a list of allowed providers, we then call etw.UpdateLogSources(allowedLogSources) and that's it. having all the logic below on the bridge seems completely unnecessary.

Different from env var, it needs the conversion from string to etw.LogSourcesInfo.
I feel bridge is the best place to put the logic. Having it in PolicyEnforcer would pull etw in as a new dependency, and the enforcer is also compiled for Linux, so importing etw there inverts the layering.
For now I extracted it into a helper function in f97bf02.

Signed-off-by: Takuro Sato <takurosato@microsoft.com>

…-test

takuro-sato · 2026-06-23T12:00:06Z

@takuro-sato A note about the TODO re: ModifyServiceSettings enforcement on other Property type - the only valid one that passes through gcs-sidecar is "LogForwardService" and that is being handled in this PR, so that todo is no longer valid. (Manish wrote that todocomment, you could check with him too)

Also should block anything other than modifying LogForwardService, and disallow other RPCType too (default: branch of switch settings.RPCType)

Now the sidecar blocks these unknown requests. 7e59bcf

anmaxvl · 2026-07-02T00:53:12Z

+//
+// Name-only entries are passed through unchanged; the sidecar fills in the
+// canonical GUID after enforcement via etw.UpdateLogSourcesFromInfo.
+func validateLogProviders(sources []etw.Source) error {


this also seems to be "policy enforcement specific", so maybe we should move that to enforcer as well.

Similarly to #2763 (comment), are we really happy to have etw as a dependency for the enforcer code?

anmaxvl · 2026-07-02T00:59:38Z

+	req := newModifyServiceSettingsRequest(payload)
+
+	hook := &captureHook{}
+	logrus.AddHook(hook)


this modifies logrus globally, not that it matters too much for test code, but consider using logrus.New() instead.

Changed 42e84eb

…tion Signed-off-by: Takuro Sato <takurosato@microsoft.com>

Signed-off-by: Takuro Sato <takurosato@microsoft.com>

MahatiC and others added 5 commits May 7, 2026 22:36

CWCOW: Remove hostedSystemConfig

3c3891a

Signed-off-by: Mahati Chamarthy <mahati.chamarthy@gmail.com>

ci: empty commit to retrigger checks

7371616

Signed-off-by: Takuro Sato <takurosato@microsoft.com>

takuro-sato marked this pull request as ready for review June 1, 2026 12:26

takuro-sato requested a review from a team as a code owner June 1, 2026 12:26

takuro-sato requested review from KenGordon and MahatiC June 1, 2026 12:27

takuro-sato assigned anmaxvl and msscotb Jun 1, 2026

takuro-sato requested a review from Copilot June 1, 2026 12:34

Copilot AI reviewed Jun 1, 2026

View reviewed changes

Comment thread pkg/securitypolicy/securitypolicy_options.go Outdated

Comment thread internal/gcs-sidecar/handlers.go

Comment thread internal/gcs-sidecar/handlers.go Outdated

Comment thread internal/gcs-sidecar/handlers.go

Comment thread internal/gcs-sidecar/handlers.go Outdated

takuro-sato requested a review from marma-dev June 1, 2026 12:38

takuro-sato added 3 commits June 1, 2026 14:05

micromaomao suggested changes Jun 4, 2026

View reviewed changes

Comment thread internal/gcs-sidecar/handlers.go Outdated

micromaomao mentioned this pull request Jun 4, 2026

CWCOW: Logging enforcement #2728

Closed

anmaxvl reviewed Jun 8, 2026

View reviewed changes

msscotb reviewed Jun 16, 2026

View reviewed changes

Comment thread internal/gcs-sidecar/handlers.go

takuro-sato added 4 commits June 22, 2026 14:47

Remove LockDown()

155f9b3

Signed-off-by: Takuro Sato <takurosato@microsoft.com>

Block unsupported property type and RPC type

7e59bcf

Signed-off-by: Takuro Sato <takurosato@microsoft.com>

Validate Name and GUID pair before policy enforcement

b665de2

Signed-off-by: Takuro Sato <takurosato@microsoft.com>

Merge remote-tracking branch 'upstream/main' into logging-enforcement…

398d839

…-test

anmaxvl reviewed Jul 2, 2026

View reviewed changes

takuro-sato added 2 commits July 2, 2026 11:34

Extract the conversion from string to LogSourcesInfo to a helper func…

f97bf02

…tion Signed-off-by: Takuro Sato <takurosato@microsoft.com>

Avoid change the global logger for testing

42e84eb

Signed-off-by: Takuro Sato <takurosato@microsoft.com>

Uh oh!

Conversation

takuro-sato commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Test

TODOs for future PRs.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

takuro-sato commented Jun 1, 2026

Uh oh!

MahatiC commented Jun 3, 2026

Uh oh!

micromaomao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

micromaomao commented Jun 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

takuro-sato commented Jun 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

takuro-sato commented Jun 1, 2026 •

edited

Loading