Skip to content

Commit a19edd0

Browse files
MahatiCMahatiC
authored
Merge pull request #2527 from MahatiC/securitypolicy-review
C-WCOW: SecurityPolicy and sidecar fixes
1 parent 7f7b0df commit a19edd0

File tree

3 files changed

+92
-89
lines changed

3 files changed

+92
-89
lines changed

‎internal/gcs-sidecar/bridge.go‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ import (
3333
type Bridge struct {
3434
mu sync.Mutex
3535
pendingMu sync.Mutex
36-
pending map[sequenceID]*prot.ContainerExecuteProcessResponse
36+
pending map[sequenceID]chan *prot.ContainerExecuteProcessResponse
3737

3838
hostState *Host
3939
// List of handlers for handling different rpc message requests.
@@ -83,7 +83,7 @@ type request struct {
8383
func NewBridge(shimConn io.ReadWriteCloser, inboxGCSConn io.ReadWriteCloser, initialEnforcer securitypolicy.SecurityPolicyEnforcer, logWriter io.Writer) *Bridge {
8484
hostState := NewHost(initialEnforcer)
8585
return &Bridge{
86-
pending: make(map[sequenceID]*prot.ContainerExecuteProcessResponse),
86+
pending: make(map[sequenceID]chan *prot.ContainerExecuteProcessResponse),
8787
rpcHandlerList: make(map[prot.RPCProc]HandlerFunc),
8888
hostState: hostState,
8989
shimConn: shimConn,
@@ -449,20 +449,19 @@ func (b *Bridge) ListenAndServeShimRequests() error {
449449
_ = sendWithContextCancel(ctx, sidecarErrChan, recverr)
450450
return
451451
}
452-
// If this is a ContainerExecuteProcessResponse, notify
452+
// If this is a ContainerExecuteProcessResponse, notify the channel
453453
const MsgExecuteProcessResponse prot.MsgType = prot.MsgTypeResponse | prot.MsgType(prot.RPCExecuteProcess)
454454

455455
if header.Type == MsgExecuteProcessResponse {
456-
logrus.Tracef("Printing after inbox exec resp")
457456
var procResp prot.ContainerExecuteProcessResponse
458457
if err := json.Unmarshal(message, &procResp); err != nil {
459-
logrus.Tracef("unmarshal failed")
458+
log.G(ctx).WithError(err).Error("failed to unmarshal the request")
459+
return
460460
}
461461

462462
b.pendingMu.Lock()
463-
if _, exists := b.pending[header.ID]; exists {
464-
logrus.Tracef("Header ID in pending exists")
465-
b.pending[header.ID] = &procResp
463+
if ch, ok := b.pending[header.ID]; ok {
464+
ch <- &procResp
466465
}
467466
b.pendingMu.Unlock()
468467
}

‎internal/gcs-sidecar/handlers.go‎

Lines changed: 29 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99
"fmt"
1010
"os"
1111
"path/filepath"
12-
"strings"
1312
"time"
1413

1514
"github.com/Microsoft/hcsshim/hcn"
@@ -19,15 +18,12 @@ import (
1918
hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
2019
"github.com/Microsoft/hcsshim/internal/log"
2120
"github.com/Microsoft/hcsshim/internal/oc"
22-
"github.com/Microsoft/hcsshim/internal/oci"
2321
"github.com/Microsoft/hcsshim/internal/protocol/guestrequest"
2422
"github.com/Microsoft/hcsshim/internal/protocol/guestresource"
2523
"github.com/Microsoft/hcsshim/internal/windevice"
26-
"github.com/Microsoft/hcsshim/pkg/annotations"
2724
"github.com/Microsoft/hcsshim/pkg/cimfs"
2825
"github.com/Microsoft/hcsshim/pkg/securitypolicy"
2926
"github.com/pkg/errors"
30-
"golang.org/x/sys/windows"
3127
)
3228

3329
const (
@@ -89,11 +85,19 @@ func (b *Bridge) createContainer(req *request) (err error) {
8985
if err != nil {
9086
return fmt.Errorf("CreateContainer operation is denied by policy: %w", err)
9187
}
88+
89+
if err := b.hostState.SetupSecurityContextDir(ctx, &spec); err != nil {
90+
return err
91+
}
92+
commandLine := len(spec.Process.Args) > 0
9293
c := &Container{
93-
id: containerID,
94-
spec: spec,
95-
processes: make(map[uint32]*containerProcess),
94+
id: containerID,
95+
spec: spec,
96+
processes: make(map[uint32]*containerProcess),
97+
commandLine: commandLine,
98+
commandLineExec: false,
9699
}
100+
97101
log.G(ctx).Tracef("Adding ContainerID: %v", containerID)
98102
if err := b.hostState.AddContainer(req.ctx, containerID, c); err != nil {
99103
log.G(ctx).Tracef("Container exists in the map.")
@@ -104,49 +108,6 @@ func (b *Bridge) createContainer(req *request) (err error) {
104108
b.hostState.RemoveContainer(ctx, containerID)
105109
}
106110
}(err)
107-
// Write security policy, signed UVM reference and host AMD certificate to
108-
// container's rootfs, so that application and sidecar containers can have
109-
// access to it. The security policy is required by containers which need to
110-
// extract init-time claims found in the security policy. The directory path
111-
// containing the files is exposed via UVM_SECURITY_CONTEXT_DIR env var.
112-
// It may be an error to have a security policy but not expose it to the
113-
// container as in that case it can never be checked as correct by a verifier.
114-
if oci.ParseAnnotationsBool(ctx, spec.Annotations, annotations.WCOWSecurityPolicyEnv, true) {
115-
encodedPolicy := b.hostState.securityPolicyEnforcer.EncodedSecurityPolicy()
116-
hostAMDCert := spec.Annotations[annotations.WCOWHostAMDCertificate]
117-
if len(encodedPolicy) > 0 || len(hostAMDCert) > 0 || len(b.hostState.uvmReferenceInfo) > 0 {
118-
// Use os.MkdirTemp to make sure that the directory is unique.
119-
securityContextDir, err := os.MkdirTemp(spec.Root.Path, securitypolicy.SecurityContextDirTemplate)
120-
if err != nil {
121-
return fmt.Errorf("failed to create security context directory: %w", err)
122-
}
123-
// Make sure that files inside directory are readable
124-
if err := os.Chmod(securityContextDir, 0755); err != nil {
125-
return fmt.Errorf("failed to chmod security context directory: %w", err)
126-
}
127-
128-
if len(encodedPolicy) > 0 {
129-
if err := writeFileInDir(securityContextDir, securitypolicy.PolicyFilename, []byte(encodedPolicy), 0777); err != nil {
130-
return fmt.Errorf("failed to write security policy: %w", err)
131-
}
132-
}
133-
if len(b.hostState.uvmReferenceInfo) > 0 {
134-
if err := writeFileInDir(securityContextDir, securitypolicy.ReferenceInfoFilename, []byte(b.hostState.uvmReferenceInfo), 0777); err != nil {
135-
return fmt.Errorf("failed to write UVM reference info: %w", err)
136-
}
137-
}
138-
139-
if len(hostAMDCert) > 0 {
140-
if err := writeFileInDir(securityContextDir, securitypolicy.HostAMDCertFilename, []byte(hostAMDCert), 0777); err != nil {
141-
return fmt.Errorf("failed to write host AMD certificate: %w", err)
142-
}
143-
}
144-
145-
containerCtxDir := fmt.Sprintf("/%s", filepath.Base(securityContextDir))
146-
secCtxEnv := fmt.Sprintf("UVM_SECURITY_CONTEXT_DIR=%s", containerCtxDir)
147-
spec.Process.Env = append(spec.Process.Env, secCtxEnv)
148-
}
149-
}
150111

151112
// Strip the spec field
152113
hostedSystemBytes, err := json.Marshal(cwcowHostedSystem)
@@ -265,15 +226,6 @@ func (b *Bridge) shutdownForced(req *request) (err error) {
265226
return nil
266227
}
267228

268-
// escapeArgs makes a Windows-style escaped command line from a set of arguments.
269-
func escapeArgs(args []string) string {
270-
escapedArgs := make([]string, len(args))
271-
for i, a := range args {
272-
escapedArgs[i] = windows.EscapeArg(a)
273-
}
274-
return strings.Join(escapedArgs, " ")
275-
}
276-
277229
func (b *Bridge) executeProcess(req *request) (err error) {
278230
_, span := oc.StartSpan(req.ctx, "sidecar::executeProcess")
279231
defer span.End()
@@ -313,15 +265,19 @@ func (b *Bridge) executeProcess(req *request) (err error) {
313265
return fmt.Errorf("failed to get created container: %w", err)
314266
}
315267

316-
// if this is an exec of Container command line, then it's already enforced
317-
// during container creation, hence skip it here
318-
containerCommandLine := escapeArgs(c.spec.Process.Args)
319-
if processParams.CommandLine != containerCommandLine {
268+
c.processesMutex.Lock()
269+
isCreateExec := c.commandLine && !c.commandLineExec
270+
if isCreateExec {
271+
// if this is an exec of Container command line, then it's already enforced
272+
// during container creation, hence skip it here
273+
c.commandLineExec = true
320274

275+
}
276+
c.processesMutex.Unlock()
277+
if !isCreateExec {
321278
user := securitypolicy.IDName{
322279
Name: processParams.User,
323280
}
324-
325281
log.G(req.ctx).Tracef("Enforcing policy on exec in container")
326282
_, _, _, err = b.hostState.securityPolicyEnforcer.
327283
EnforceExecInContainerPolicyV2(
@@ -339,9 +295,10 @@ func (b *Bridge) executeProcess(req *request) (err error) {
339295
}
340296
headerID := req.header.ID
341297

342-
// initiate process ID
298+
// initiate exec process response channel
299+
procRespCh := make(chan *prot.ContainerExecuteProcessResponse, 1)
343300
b.pendingMu.Lock()
344-
b.pending[headerID] = nil // nil means not yet received
301+
b.pending[headerID] = procRespCh
345302
b.pendingMu.Unlock()
346303

347304
defer func() {
@@ -354,13 +311,8 @@ func (b *Bridge) executeProcess(req *request) (err error) {
354311
b.forwardRequestToGcs(req)
355312

356313
// fetch the process ID from response
357-
deadline := time.Now().Add(5 * time.Second)
358-
for time.Now().Before(deadline) {
359-
log.G(req.ctx).Tracef("waiting for exec resp")
360-
b.pendingMu.Lock()
361-
resp := b.pending[headerID]
362-
b.pendingMu.Unlock()
363-
314+
select {
315+
case resp := <-procRespCh:
364316
// capture the Process details, so that we can later enforce
365317
// on the allowed signals on the Process
366318
if resp != nil {
@@ -374,10 +326,11 @@ func (b *Bridge) executeProcess(req *request) (err error) {
374326
}
375327
return nil
376328
}
377-
time.Sleep(10 * time.Millisecond) // backoff
329+
// Channel closed or received nil, treat as error
330+
return errors.New("received nil exec response")
331+
case <-time.After(5 * time.Second):
332+
return errors.New("timed out waiting for exec response")
378333
}
379-
380-
return errors.Wrap(err, "timedout waiting for exec response")
381334
}
382335
return nil
383336
}

‎internal/gcs-sidecar/host.go‎

Lines changed: 56 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,12 @@ import (
1313
hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
1414
"github.com/Microsoft/hcsshim/internal/log"
1515
"github.com/Microsoft/hcsshim/internal/logfields"
16+
oci "github.com/Microsoft/hcsshim/internal/oci"
1617
"github.com/Microsoft/hcsshim/internal/protocol/guestresource"
1718
"github.com/Microsoft/hcsshim/internal/pspdriver"
19+
"github.com/Microsoft/hcsshim/pkg/annotations"
1820
"github.com/Microsoft/hcsshim/pkg/securitypolicy"
19-
oci "github.com/opencontainers/runtime-spec/specs-go"
21+
specs "github.com/opencontainers/runtime-spec/specs-go"
2022
"github.com/pkg/errors"
2123
"github.com/sirupsen/logrus"
2224
)
@@ -33,10 +35,12 @@ type Host struct {
3335
}
3436

3537
type Container struct {
36-
id string
37-
spec oci.Spec
38-
processesMutex sync.Mutex
39-
processes map[uint32]*containerProcess
38+
id string
39+
spec specs.Spec
40+
processesMutex sync.Mutex
41+
processes map[uint32]*containerProcess
42+
commandLine bool
43+
commandLineExec bool
4044
}
4145

4246
// Process is a struct that defines the lifetime and operations associated with
@@ -56,6 +60,53 @@ func NewHost(initialEnforcer securitypolicy.SecurityPolicyEnforcer) *Host {
5660
}
5761
}
5862

63+
// Write security policy, signed UVM reference and host AMD certificate to
64+
// container's rootfs, so that application and sidecar containers can have
65+
// access to it. The security policy is required by containers which need to
66+
// extract init-time claims found in the security policy. The directory path
67+
// containing the files is exposed via UVM_SECURITY_CONTEXT_DIR env var.
68+
// It may be an error to have a security policy but not expose it to the
69+
// container as in that case it can never be checked as correct by a verifier.
70+
func (h *Host) SetupSecurityContextDir(ctx context.Context, spec *specs.Spec) error {
71+
if oci.ParseAnnotationsBool(ctx, spec.Annotations, annotations.WCOWSecurityPolicyEnv, true) {
72+
encodedPolicy := h.securityPolicyEnforcer.EncodedSecurityPolicy()
73+
hostAMDCert := spec.Annotations[annotations.WCOWHostAMDCertificate]
74+
if len(encodedPolicy) > 0 || len(hostAMDCert) > 0 || len(h.uvmReferenceInfo) > 0 {
75+
// Use os.MkdirTemp to make sure that the directory is unique.
76+
securityContextDir, err := os.MkdirTemp(spec.Root.Path, securitypolicy.SecurityContextDirTemplate)
77+
if err != nil {
78+
return fmt.Errorf("failed to create security context directory: %w", err)
79+
}
80+
// Make sure that files inside directory are readable
81+
if err := os.Chmod(securityContextDir, 0755); err != nil {
82+
return fmt.Errorf("failed to chmod security context directory: %w", err)
83+
}
84+
85+
if len(encodedPolicy) > 0 {
86+
if err := writeFileInDir(securityContextDir, securitypolicy.PolicyFilename, []byte(encodedPolicy), 0777); err != nil {
87+
return fmt.Errorf("failed to write security policy: %w", err)
88+
}
89+
}
90+
if len(h.uvmReferenceInfo) > 0 {
91+
if err := writeFileInDir(securityContextDir, securitypolicy.ReferenceInfoFilename, []byte(h.uvmReferenceInfo), 0777); err != nil {
92+
return fmt.Errorf("failed to write UVM reference info: %w", err)
93+
}
94+
}
95+
96+
if len(hostAMDCert) > 0 {
97+
if err := writeFileInDir(securityContextDir, securitypolicy.HostAMDCertFilename, []byte(hostAMDCert), 0777); err != nil {
98+
return fmt.Errorf("failed to write host AMD certificate: %w", err)
99+
}
100+
}
101+
102+
containerCtxDir := fmt.Sprintf("/%s", filepath.Base(securityContextDir))
103+
secCtxEnv := fmt.Sprintf("UVM_SECURITY_CONTEXT_DIR=%s", containerCtxDir)
104+
spec.Process.Env = append(spec.Process.Env, secCtxEnv)
105+
}
106+
}
107+
return nil
108+
}
109+
59110
// InjectFragment extends current security policy with additional constraints
60111
// from the incoming fragment. Note that it is base64 encoded over the bridge/
61112
//

0 commit comments

Comments
 (0)