C-WCOW: SecurityPolicy implementation and tests by MahatiC · Pull Request #2508 · microsoft/hcsshim

MahatiC · 2025-08-28T10:21:45Z

This contains commits that implement Security policy for C-WCOW.
~~This PR has dependency on commits from Invoke fsformatter for container scratch #2498. So, at the moment this is rebased on top of that PR.~~

rawahars · 2025-09-15T10:47:04Z

+
+// GetProcess returns the Process with the matching 'pid'. If the 'pid' does
+// not exit returns error.
+func (c *Container) GetProcess(pid uint32) (*containerProcess, error) {


Can this be moved to a new file named container.go?

Please move this method along with the declaration of container struct to that file. There should be a clear boundary between host methods vs container methods.

rawahars · 2025-09-15T11:08:21Z

+
+	if containerID == UVMContainerID {
+		log.G(req.ctx).Tracef("Enforcing policy on external exec process")
+		_, _, err := b.hostState.securityPolicyEnforcer.EnforceExecExternalProcessPolicy(


Do we need User validation in this enforcement point?

rawahars · 2025-09-15T11:11:08Z

+		// if this is an exec of Container command line, then it's already enforced
+		// during container creation, hence skip it here
+		containerCommandLine := escapeArgs(c.spec.Process.Args)
+		if processParams.CommandLine != containerCommandLine {


If another exec is made by the user with same command args as the original container, then this will not be enforced. Maybe we enforce all the exec including the container init exec.

rawahars · 2025-09-15T11:12:13Z

+		b.forwardRequestToGcs(req)
+
+		// fetch the process ID from response
+		deadline := time.Now().Add(5 * time.Second)


nit: Please declare the timeout at the beginning of the file so that we have a unified way for all the important constants.

rawahars · 2025-09-15T11:25:10Z

 					_ = sendWithContextCancel(ctx, sidecarErrChan, recverr)
 					return
 				}
+				// If this is a ContainerExecuteProcessResponse, notify


Instead of this approach, we can use channels for capturing the process response.
Approach would be-

Create a channel on which you write the response to when the same is received in ListenAndServeShimRequests method.

Same channel is used in executeProcess method. We listens on the channel with a deadline of 5 seconds. If you don't hear back then return an error message. If we hear back about another exec then we skip and keep waiting for next response.

With this approach, you won't need- b.pending, b.pendingMu and other polling logic.

rawahars · 2025-09-15T11:25:50Z

+		}
+		headerID := req.header.ID
+
+		// initiate process ID


Use channels instead of this logic. Please refer to earlier comment.

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

KenGordon

As discussed. Merge now (but do not delete the PR) and add some issues for the coding/style/refactoring that will be needed afterwards but which is not critical path for the demo and enabling others to get on with their work.

KenGordon · 2025-08-28T11:27:46Z

 	SeccompProfileSHA256 string              `json:"-"`
 }

+type WindowsContainer struct {


Can these various windows versions of pre-existing types be made common, either by using the go interface scheme (or whatever is appropriate - I can't claim to be a golang guru) or by just using the same objects but not filling out the parts that don't apply to the given platform?

KenGordon reviewed Aug 28, 2025

View reviewed changes

Comment thread internal/uvm/security_policy.go Outdated

MahatiC force-pushed the rebased-securitypolicy-tests branch 10 times, most recently from e9c8277 to 3995b88 Compare September 4, 2025 16:05

MahatiC marked this pull request as ready for review September 9, 2025 14:59

MahatiC requested a review from a team as a code owner September 9, 2025 14:59

ambarve reviewed Sep 14, 2025

View reviewed changes

Comment thread internal/hcsoci/create.go Outdated

ambarve reviewed Sep 14, 2025

View reviewed changes

Comment thread internal/uvm/types.go Outdated

ambarve reviewed Sep 14, 2025

View reviewed changes

Comment thread pkg/securitypolicy/securitypolicyenforcer.go Outdated

rawahars reviewed Sep 15, 2025

View reviewed changes

takuro-sato reviewed Sep 15, 2025

View reviewed changes

Comment thread internal/gcs-sidecar/handlers.go

Comment thread internal/gcs-sidecar/handlers.go

Comment thread internal/regopolicyinterpreter/regopolicyinterpreter.go

Comment thread pkg/annotations/annotations.go

MahatiC force-pushed the rebased-securitypolicy-tests branch from 887b690 to d0815d2 Compare September 15, 2025 14:13

MahatiC added 7 commits September 18, 2025 13:12

C-WCOW: Implement SecurityPolicy enforcement

5901338

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

C-WCOW: Add SecurityPolicy tests

cec74fc

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

C-WCOW: Fix lint errors

3ddb303

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

C-WCOW: Runtime logging enforcement, misc cleanup

e4407ad

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

C-WCOW: CI test and misc cleanup

0621400

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

C-WCOW: Remove obsolete policy CI and clean up

1b56509

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

C-WCOW: Use existing policy related function

d744385

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

MahatiC force-pushed the rebased-securitypolicy-tests branch from d0815d2 to d744385 Compare September 18, 2025 12:32

CWCOW: Fix rebase conflict

77fec0e

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

MahatiC force-pushed the rebased-securitypolicy-tests branch 2 times, most recently from 3098d26 to 9d6b64d Compare September 18, 2025 14:43

MahatiC force-pushed the rebased-securitypolicy-tests branch from 9d6b64d to 9ef1de2 Compare September 18, 2025 15:07

C-WCOW: Address review comments

24fcf74

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

MahatiC force-pushed the rebased-securitypolicy-tests branch from 9ef1de2 to 24fcf74 Compare September 18, 2025 15:21

MahatiC requested review from KenGordon and takuro-sato September 18, 2025 16:10

KenGordon approved these changes Sep 18, 2025

View reviewed changes

ambarve approved these changes Sep 19, 2025

View reviewed changes

ambarve merged commit 0fc5d6e into microsoft:main Sep 19, 2025
17 checks passed

Uh oh!

Conversation

MahatiC commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rawahars Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

rawahars Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

rawahars Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

rawahars Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

rawahars Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

rawahars Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KenGordon left a comment

Choose a reason for hiding this comment

Uh oh!

KenGordon Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MahatiC commented Aug 28, 2025 •

edited

Loading