From fa9158dd8b120c61ddaa58c5ac38d21e6250d4dc Mon Sep 17 00:00:00 2001 From: Mahati Chamarthy Date: Wed, 24 Sep 2025 15:02:19 +0100 Subject: [PATCH 1/3] C-WCOW: Use go channel Signed-off-by: Mahati Chamarthy --- internal/gcs-sidecar/bridge.go | 15 +++++++-------- internal/gcs-sidecar/handlers.go | 19 ++++++++----------- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/internal/gcs-sidecar/bridge.go b/internal/gcs-sidecar/bridge.go index 1c1cefdcd9..b7bc2f9522 100644 --- a/internal/gcs-sidecar/bridge.go +++ b/internal/gcs-sidecar/bridge.go @@ -33,7 +33,7 @@ import ( type Bridge struct { mu sync.Mutex pendingMu sync.Mutex - pending map[sequenceID]*prot.ContainerExecuteProcessResponse + pending map[sequenceID]chan *prot.ContainerExecuteProcessResponse hostState *Host // List of handlers for handling different rpc message requests. @@ -83,7 +83,7 @@ type request struct { func NewBridge(shimConn io.ReadWriteCloser, inboxGCSConn io.ReadWriteCloser, initialEnforcer securitypolicy.SecurityPolicyEnforcer, logWriter io.Writer) *Bridge { hostState := NewHost(initialEnforcer) return &Bridge{ - pending: make(map[sequenceID]*prot.ContainerExecuteProcessResponse), + pending: make(map[sequenceID]chan *prot.ContainerExecuteProcessResponse), rpcHandlerList: make(map[prot.RPCProc]HandlerFunc), hostState: hostState, shimConn: shimConn, @@ -449,20 +449,19 @@ func (b *Bridge) ListenAndServeShimRequests() error { _ = sendWithContextCancel(ctx, sidecarErrChan, recverr) return } - // If this is a ContainerExecuteProcessResponse, notify + // If this is a ContainerExecuteProcessResponse, notify the channel const MsgExecuteProcessResponse prot.MsgType = prot.MsgTypeResponse | prot.MsgType(prot.RPCExecuteProcess) if header.Type == MsgExecuteProcessResponse { - logrus.Tracef("Printing after inbox exec resp") var procResp prot.ContainerExecuteProcessResponse if err := json.Unmarshal(message, &procResp); err != nil { - logrus.Tracef("unmarshal failed") + log.G(ctx).WithError(err).Error("failed to unmarshal the request") + return } b.pendingMu.Lock() - if _, exists := b.pending[header.ID]; exists { - logrus.Tracef("Header ID in pending exists") - b.pending[header.ID] = &procResp + if ch, ok := b.pending[header.ID]; ok { + ch <- &procResp } b.pendingMu.Unlock() } diff --git a/internal/gcs-sidecar/handlers.go b/internal/gcs-sidecar/handlers.go index 7660e6d069..2203381cb6 100644 --- a/internal/gcs-sidecar/handlers.go +++ b/internal/gcs-sidecar/handlers.go @@ -340,8 +340,9 @@ func (b *Bridge) executeProcess(req *request) (err error) { headerID := req.header.ID // initiate process ID + procRespCh := make(chan *prot.ContainerExecuteProcessResponse, 1) b.pendingMu.Lock() - b.pending[headerID] = nil // nil means not yet received + b.pending[headerID] = procRespCh b.pendingMu.Unlock() defer func() { @@ -354,13 +355,8 @@ func (b *Bridge) executeProcess(req *request) (err error) { b.forwardRequestToGcs(req) // fetch the process ID from response - deadline := time.Now().Add(5 * time.Second) - for time.Now().Before(deadline) { - log.G(req.ctx).Tracef("waiting for exec resp") - b.pendingMu.Lock() - resp := b.pending[headerID] - b.pendingMu.Unlock() - + select { + case resp := <-procRespCh: // capture the Process details, so that we can later enforce // on the allowed signals on the Process if resp != nil { @@ -374,10 +370,11 @@ func (b *Bridge) executeProcess(req *request) (err error) { } return nil } - time.Sleep(10 * time.Millisecond) // backoff + // Channel closed or received nil, treat as error + return errors.New("received nil exec response") + case <-time.After(5 * time.Second): + return errors.New("timed out waiting for exec response") } - - return errors.Wrap(err, "timedout waiting for exec response") } return nil } From f4d680707d8d12cf71a9f851cffb6be92333d492 Mon Sep 17 00:00:00 2001 From: Mahati Chamarthy Date: Thu, 25 Sep 2025 11:17:08 +0100 Subject: [PATCH 2/3] C-WCOW: Move security context dir code Signed-off-by: Mahati Chamarthy --- internal/gcs-sidecar/handlers.go | 49 +++-------------------------- internal/gcs-sidecar/host.go | 53 ++++++++++++++++++++++++++++++-- 2 files changed, 55 insertions(+), 47 deletions(-) diff --git a/internal/gcs-sidecar/handlers.go b/internal/gcs-sidecar/handlers.go index 2203381cb6..1ae2279dcf 100644 --- a/internal/gcs-sidecar/handlers.go +++ b/internal/gcs-sidecar/handlers.go @@ -19,11 +19,9 @@ import ( hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" "github.com/Microsoft/hcsshim/internal/log" "github.com/Microsoft/hcsshim/internal/oc" - "github.com/Microsoft/hcsshim/internal/oci" "github.com/Microsoft/hcsshim/internal/protocol/guestrequest" "github.com/Microsoft/hcsshim/internal/protocol/guestresource" "github.com/Microsoft/hcsshim/internal/windevice" - "github.com/Microsoft/hcsshim/pkg/annotations" "github.com/Microsoft/hcsshim/pkg/cimfs" "github.com/Microsoft/hcsshim/pkg/securitypolicy" "github.com/pkg/errors" @@ -89,6 +87,10 @@ func (b *Bridge) createContainer(req *request) (err error) { if err != nil { return fmt.Errorf("CreateContainer operation is denied by policy: %w", err) } + + if err := b.hostState.SetupSecurityContextDir(ctx, &spec); err != nil { + return err + } c := &Container{ id: containerID, spec: spec, @@ -104,49 +106,6 @@ func (b *Bridge) createContainer(req *request) (err error) { b.hostState.RemoveContainer(ctx, containerID) } }(err) - // Write security policy, signed UVM reference and host AMD certificate to - // container's rootfs, so that application and sidecar containers can have - // access to it. The security policy is required by containers which need to - // extract init-time claims found in the security policy. The directory path - // containing the files is exposed via UVM_SECURITY_CONTEXT_DIR env var. - // It may be an error to have a security policy but not expose it to the - // container as in that case it can never be checked as correct by a verifier. - if oci.ParseAnnotationsBool(ctx, spec.Annotations, annotations.WCOWSecurityPolicyEnv, true) { - encodedPolicy := b.hostState.securityPolicyEnforcer.EncodedSecurityPolicy() - hostAMDCert := spec.Annotations[annotations.WCOWHostAMDCertificate] - if len(encodedPolicy) > 0 || len(hostAMDCert) > 0 || len(b.hostState.uvmReferenceInfo) > 0 { - // Use os.MkdirTemp to make sure that the directory is unique. - securityContextDir, err := os.MkdirTemp(spec.Root.Path, securitypolicy.SecurityContextDirTemplate) - if err != nil { - return fmt.Errorf("failed to create security context directory: %w", err) - } - // Make sure that files inside directory are readable - if err := os.Chmod(securityContextDir, 0755); err != nil { - return fmt.Errorf("failed to chmod security context directory: %w", err) - } - - if len(encodedPolicy) > 0 { - if err := writeFileInDir(securityContextDir, securitypolicy.PolicyFilename, []byte(encodedPolicy), 0777); err != nil { - return fmt.Errorf("failed to write security policy: %w", err) - } - } - if len(b.hostState.uvmReferenceInfo) > 0 { - if err := writeFileInDir(securityContextDir, securitypolicy.ReferenceInfoFilename, []byte(b.hostState.uvmReferenceInfo), 0777); err != nil { - return fmt.Errorf("failed to write UVM reference info: %w", err) - } - } - - if len(hostAMDCert) > 0 { - if err := writeFileInDir(securityContextDir, securitypolicy.HostAMDCertFilename, []byte(hostAMDCert), 0777); err != nil { - return fmt.Errorf("failed to write host AMD certificate: %w", err) - } - } - - containerCtxDir := fmt.Sprintf("/%s", filepath.Base(securityContextDir)) - secCtxEnv := fmt.Sprintf("UVM_SECURITY_CONTEXT_DIR=%s", containerCtxDir) - spec.Process.Env = append(spec.Process.Env, secCtxEnv) - } - } // Strip the spec field hostedSystemBytes, err := json.Marshal(cwcowHostedSystem) diff --git a/internal/gcs-sidecar/host.go b/internal/gcs-sidecar/host.go index 85930cd496..ea6f358f5d 100644 --- a/internal/gcs-sidecar/host.go +++ b/internal/gcs-sidecar/host.go @@ -20,10 +20,12 @@ import ( hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2" "github.com/Microsoft/hcsshim/internal/log" "github.com/Microsoft/hcsshim/internal/logfields" + oci "github.com/Microsoft/hcsshim/internal/oci" "github.com/Microsoft/hcsshim/internal/protocol/guestresource" "github.com/Microsoft/hcsshim/internal/pspdriver" + "github.com/Microsoft/hcsshim/pkg/annotations" "github.com/Microsoft/hcsshim/pkg/securitypolicy" - oci "github.com/opencontainers/runtime-spec/specs-go" + specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -41,7 +43,7 @@ type Host struct { type Container struct { id string - spec oci.Spec + spec specs.Spec processesMutex sync.Mutex processes map[uint32]*containerProcess } @@ -63,6 +65,53 @@ func NewHost(initialEnforcer securitypolicy.SecurityPolicyEnforcer) *Host { } } +// Write security policy, signed UVM reference and host AMD certificate to +// container's rootfs, so that application and sidecar containers can have +// access to it. The security policy is required by containers which need to +// extract init-time claims found in the security policy. The directory path +// containing the files is exposed via UVM_SECURITY_CONTEXT_DIR env var. +// It may be an error to have a security policy but not expose it to the +// container as in that case it can never be checked as correct by a verifier. +func (h *Host) SetupSecurityContextDir(ctx context.Context, spec *specs.Spec) error { + if oci.ParseAnnotationsBool(ctx, spec.Annotations, annotations.WCOWSecurityPolicyEnv, true) { + encodedPolicy := h.securityPolicyEnforcer.EncodedSecurityPolicy() + hostAMDCert := spec.Annotations[annotations.WCOWHostAMDCertificate] + if len(encodedPolicy) > 0 || len(hostAMDCert) > 0 || len(h.uvmReferenceInfo) > 0 { + // Use os.MkdirTemp to make sure that the directory is unique. + securityContextDir, err := os.MkdirTemp(spec.Root.Path, securitypolicy.SecurityContextDirTemplate) + if err != nil { + return fmt.Errorf("failed to create security context directory: %w", err) + } + // Make sure that files inside directory are readable + if err := os.Chmod(securityContextDir, 0755); err != nil { + return fmt.Errorf("failed to chmod security context directory: %w", err) + } + + if len(encodedPolicy) > 0 { + if err := writeFileInDir(securityContextDir, securitypolicy.PolicyFilename, []byte(encodedPolicy), 0777); err != nil { + return fmt.Errorf("failed to write security policy: %w", err) + } + } + if len(h.uvmReferenceInfo) > 0 { + if err := writeFileInDir(securityContextDir, securitypolicy.ReferenceInfoFilename, []byte(h.uvmReferenceInfo), 0777); err != nil { + return fmt.Errorf("failed to write UVM reference info: %w", err) + } + } + + if len(hostAMDCert) > 0 { + if err := writeFileInDir(securityContextDir, securitypolicy.HostAMDCertFilename, []byte(hostAMDCert), 0777); err != nil { + return fmt.Errorf("failed to write host AMD certificate: %w", err) + } + } + + containerCtxDir := fmt.Sprintf("/%s", filepath.Base(securityContextDir)) + secCtxEnv := fmt.Sprintf("UVM_SECURITY_CONTEXT_DIR=%s", containerCtxDir) + spec.Process.Env = append(spec.Process.Env, secCtxEnv) + } + } + return nil +} + // InjectFragment extends current security policy with additional constraints // from the incoming fragment. Note that it is base64 encoded over the bridge/ // From d7e2efefd21a0c0cbce1873b6866ed84d3c2c78a Mon Sep 17 00:00:00 2001 From: Mahati Chamarthy Date: Thu, 25 Sep 2025 13:27:42 +0100 Subject: [PATCH 3/3] C-WCOW: Differentiate container cmdline Signed-off-by: Mahati Chamarthy --- internal/gcs-sidecar/handlers.go | 37 +++++++++++++++----------------- internal/gcs-sidecar/host.go | 10 +++++---- 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/internal/gcs-sidecar/handlers.go b/internal/gcs-sidecar/handlers.go index 1ae2279dcf..ab3d878399 100644 --- a/internal/gcs-sidecar/handlers.go +++ b/internal/gcs-sidecar/handlers.go @@ -9,7 +9,6 @@ import ( "fmt" "os" "path/filepath" - "strings" "time" "github.com/Microsoft/hcsshim/hcn" @@ -25,7 +24,6 @@ import ( "github.com/Microsoft/hcsshim/pkg/cimfs" "github.com/Microsoft/hcsshim/pkg/securitypolicy" "github.com/pkg/errors" - "golang.org/x/sys/windows" ) const ( @@ -91,11 +89,15 @@ func (b *Bridge) createContainer(req *request) (err error) { if err := b.hostState.SetupSecurityContextDir(ctx, &spec); err != nil { return err } + commandLine := len(spec.Process.Args) > 0 c := &Container{ - id: containerID, - spec: spec, - processes: make(map[uint32]*containerProcess), + id: containerID, + spec: spec, + processes: make(map[uint32]*containerProcess), + commandLine: commandLine, + commandLineExec: false, } + log.G(ctx).Tracef("Adding ContainerID: %v", containerID) if err := b.hostState.AddContainer(req.ctx, containerID, c); err != nil { log.G(ctx).Tracef("Container exists in the map.") @@ -224,15 +226,6 @@ func (b *Bridge) shutdownForced(req *request) (err error) { return nil } -// escapeArgs makes a Windows-style escaped command line from a set of arguments. -func escapeArgs(args []string) string { - escapedArgs := make([]string, len(args)) - for i, a := range args { - escapedArgs[i] = windows.EscapeArg(a) - } - return strings.Join(escapedArgs, " ") -} - func (b *Bridge) executeProcess(req *request) (err error) { _, span := oc.StartSpan(req.ctx, "sidecar::executeProcess") defer span.End() @@ -272,15 +265,19 @@ func (b *Bridge) executeProcess(req *request) (err error) { return fmt.Errorf("failed to get created container: %w", err) } - // 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 { + c.processesMutex.Lock() + isCreateExec := c.commandLine && !c.commandLineExec + if isCreateExec { + // if this is an exec of Container command line, then it's already enforced + // during container creation, hence skip it here + c.commandLineExec = true + } + c.processesMutex.Unlock() + if !isCreateExec { user := securitypolicy.IDName{ Name: processParams.User, } - log.G(req.ctx).Tracef("Enforcing policy on exec in container") _, _, _, err = b.hostState.securityPolicyEnforcer. EnforceExecInContainerPolicyV2( @@ -298,7 +295,7 @@ func (b *Bridge) executeProcess(req *request) (err error) { } headerID := req.header.ID - // initiate process ID + // initiate exec process response channel procRespCh := make(chan *prot.ContainerExecuteProcessResponse, 1) b.pendingMu.Lock() b.pending[headerID] = procRespCh diff --git a/internal/gcs-sidecar/host.go b/internal/gcs-sidecar/host.go index ea6f358f5d..593bac0131 100644 --- a/internal/gcs-sidecar/host.go +++ b/internal/gcs-sidecar/host.go @@ -42,10 +42,12 @@ type Host struct { } type Container struct { - id string - spec specs.Spec - processesMutex sync.Mutex - processes map[uint32]*containerProcess + id string + spec specs.Spec + processesMutex sync.Mutex + processes map[uint32]*containerProcess + commandLine bool + commandLineExec bool } // Process is a struct that defines the lifetime and operations associated with