Disable unsafe container options (microsoft#1260)

Add annotations to disable unsafe container operations, regardless of container spec:
* adding writable vSMB or plan9 file shares to hypervisor isolated containers' UVM
* using gMSAs for WCOW containers
(Annotation to disable vSMB direct maps already exists)

Signed-off-by: Hamza El-Saawy hamzaelsaawy@microsoft.com

hcs/errors.go

@@ -51,6 +51,9 @@ var (
 	// ErrUnexpectedValue is an error encountered when hcs returns an invalid value
 	ErrUnexpectedValue = errors.New("unexpected value returned from hcs")
 
+	// ErrOperationDenied is an error when hcs attempts an operation that is explicitly denied
+	ErrOperationDenied = errors.New("operation denied")
+
 	// ErrVmcomputeAlreadyStopped is an error encountered when a shutdown or terminate request is made on a stopped container
 	ErrVmcomputeAlreadyStopped = syscall.Errno(0xc0370110)
 

hcsoci/create.go

@@ -45,7 +45,7 @@ type CreateOptions struct {
 	HostingSystem    *uvm.UtilityVM     // Utility or service VM in which the container is to be created.
 	NetworkNamespace string             // Host network namespace to use (overrides anything in the spec)
 
-	// This is an advanced debugging parameter. It allows for diagnosibility by leaving a containers
+	// This is an advanced debugging parameter. It allows for diagnosability by leaving a containers
 	// resources allocated in case of a failure. Thus you would be able to use tools such as hcsdiag
 	// to look at the state of a utility VM to see what resources were allocated. Obviously the caller
 	// must a) not tear down the utility VM on failure (or pause in some way) and b) is responsible for
@@ -170,6 +170,15 @@ func validateContainerConfig(ctx context.Context, coi *createOptionsInternal) er
 			return fmt.Errorf("user specified mounts are not permitted for template containers")
 		}
 	}
+
+	// check if gMSA is disabled
+	if coi.Spec.Windows != nil {
+		disableGMSA := oci.ParseAnnotationsDisableGMSA(ctx, coi.Spec)
+		if _, ok := coi.Spec.Windows.CredentialSpec.(string); ok && disableGMSA {
+			return fmt.Errorf("gMSA credentials are disabled: %w", hcs.ErrOperationDenied)
+		}
+	}
+
 	return nil
 }
 

oci/annotations.go

@@ -0,0 +1,150 @@
+package oci
+
+import (
+	"context"
+	"errors"
+	"strconv"
+	"strings"
+
+	"github.com/Microsoft/hcsshim/internal/log"
+	"github.com/Microsoft/hcsshim/internal/logfields"
+	"github.com/Microsoft/hcsshim/pkg/annotations"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/sirupsen/logrus"
+)
+
+var ErrAnnotationExpansionConflict = errors.New("annotation expansion conflict")
+
+// ProcessAnnotations expands annotations into their corresponding annotation groups
+func ProcessAnnotations(ctx context.Context, s *specs.Spec) (err error) {
+	// Named `Process` and not `Expand` since this function may be expanded (pun intended) to
+	// deal with other annotation issues and validation.
+
+	// Rathen than give up part of the way through on error, this just emits a warning (similar
+	// to the `parseAnnotation*` functions) and continues through, so the spec is not left in a
+	// (partially) unusable form.
+	// If multiple different errors are to be raised, they should be combined or, if they
+	// are logged, only the last kept, depending on their severity.
+
+	// expand annotations
+	for key, exps := range annotations.AnnotationExpansions {
+		// check if annotation is present
+		if val, ok := s.Annotations[key]; ok {
+			// ideally, some normalization would occur here (ie, "True" -> "true")
+			// but strings may be case-sensitive
+			for _, k := range exps {
+				if v, ok := s.Annotations[k]; ok && val != v {
+					err = ErrAnnotationExpansionConflict
+					log.G(ctx).WithFields(logrus.Fields{
+						logfields.OCIAnnotation:               key,
+						logfields.Value:                       val,
+						logfields.OCIAnnotation + "-conflict": k,
+						logfields.Value + "-conflict":         v,
+					}).WithError(err).Warning("annotation expansion would overwrite conflicting value")
+					continue
+				}
+				s.Annotations[k] = val
+			}
+		}
+	}
+
+	return err
+}
+
+// handle specific annotations
+
+// ParseAnnotationsDisableGMSA searches for the boolean value which specifies
+// if providing a gMSA credential should be disallowed. Returns the value found,
+// if parsable, otherwise returns false otherwise.
+func ParseAnnotationsDisableGMSA(ctx context.Context, s *specs.Spec) bool {
+	return parseAnnotationsBool(ctx, s.Annotations, annotations.WCOWDisableGMSA, false)
+}
+
+// ParseAnnotationsSaveAsTemplate searches for the boolean value which specifies
+// if this create request should be considered as a template creation request. If value
+// is found the returns the actual value, returns false otherwise.
+func ParseAnnotationsSaveAsTemplate(ctx context.Context, s *specs.Spec) bool {
+	return parseAnnotationsBool(ctx, s.Annotations, annotations.SaveAsTemplate, false)
+}
+
+// ParseAnnotationsTemplateID searches for the templateID in the create request. If the
+// value is found then returns the value otherwise returns the empty string.
+func ParseAnnotationsTemplateID(ctx context.Context, s *specs.Spec) string {
+	return parseAnnotationsString(s.Annotations, annotations.TemplateID, "")
+}
+
+// general annotation parsing
+
+// parseAnnotationsBool searches `a` for `key` and if found verifies that the
+// value is `true` or `false` in any case. If `key` is not found returns `def`.
+func parseAnnotationsBool(ctx context.Context, a map[string]string, key string, def bool) bool {
+	if v, ok := a[key]; ok {
+		switch strings.ToLower(v) {
+		case "true":
+			return true
+		case "false":
+			return false
+		default:
+			logAnnotationParseError(ctx, key, v, logfields.Bool, nil)
+		}
+	}
+	return def
+}
+
+// parseAnnotationsUint32 searches `a` for `key` and if found verifies that the
+// value is a 32 bit unsigned integer. If `key` is not found returns `def`.
+func parseAnnotationsUint32(ctx context.Context, a map[string]string, key string, def uint32) uint32 {
+	if v, ok := a[key]; ok {
+		countu, err := strconv.ParseUint(v, 10, 32)
+		if err == nil {
+			v := uint32(countu)
+			return v
+		}
+		logAnnotationParseError(ctx, key, v, logfields.Uint32, err)
+	}
+	return def
+}
+
+// parseAnnotationsUint64 searches `a` for `key` and if found verifies that the
+// value is a 64 bit unsigned integer. If `key` is not found returns `def`.
+func parseAnnotationsUint64(ctx context.Context, a map[string]string, key string, def uint64) uint64 {
+	if v, ok := a[key]; ok {
+		countu, err := strconv.ParseUint(v, 10, 64)
+		if err == nil {
+			return countu
+		}
+		logAnnotationParseError(ctx, key, v, logfields.Uint64, err)
+	}
+	return def
+}
+
+// parseAnnotationsString searches `a` for `key`. If `key` is not found returns `def`.
+func parseAnnotationsString(a map[string]string, key string, def string) string {
+	if v, ok := a[key]; ok {
+		return v
+	}
+	return def
+}
+
+// ParseAnnotationCommaSeparated searches `annotations` for `annotation` corresponding to a
+// list of comma separated strings
+func ParseAnnotationCommaSeparated(annotation string, annotations map[string]string) []string {
+	cs, ok := annotations[annotation]
+	if !ok || cs == "" {
+		return nil
+	}
+	results := strings.Split(cs, ",")
+	return results
+}
+
+func logAnnotationParseError(ctx context.Context, k, v, et string, err error) {
+	entry := log.G(ctx).WithFields(logrus.Fields{
+		logfields.OCIAnnotation: k,
+		logfields.Value:         v,
+		logfields.ExpectedType:  et,
+	})
+	if err != nil {
+		entry = entry.WithError(err)
+	}
+	entry.Warning("annotation could not be parsed")
+}

oci/annotations_test.go

@@ -0,0 +1,86 @@
+package oci
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"testing"
+
+	"github.com/Microsoft/hcsshim/pkg/annotations"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/sirupsen/logrus"
+)
+
+func Test_ProccessAnnotations_Expansion(t *testing.T) {
+	// suppress warnings raised by process annotation
+	defer func(l logrus.Level) {
+		logrus.SetLevel(l)
+	}(logrus.GetLevel())
+	logrus.SetLevel(logrus.ErrorLevel)
+	ctx := context.Background()
+
+	tests := []struct {
+		name string
+		spec specs.Spec
+	}{
+		{
+			name: "lcow",
+			spec: specs.Spec{
+				Linux: &specs.Linux{},
+			},
+		},
+		{
+			name: "wcow-hypervisor",
+			spec: specs.Spec{
+				Windows: &specs.Windows{
+					HyperV: &specs.WindowsHyperV{},
+				},
+			},
+		},
+		{
+			name: "wcow-process",
+			spec: specs.Spec{
+				Windows: &specs.Windows{},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		// test correct expansion
+		for _, v := range []string{"true", "false"} {
+			t.Run(tt.name+"_disable_unsafe_"+v, func(subtest *testing.T) {
+				tt.spec.Annotations = map[string]string{
+					annotations.DisableUnsafeOperations: v,
+				}
+
+				err := ProcessAnnotations(ctx, &tt.spec)
+				if err != nil {
+					subtest.Fatalf("could not update spec from options: %v", err)
+				}
+
+				for _, k := range annotations.AnnotationExpansions[annotations.DisableUnsafeOperations] {
+					if vv := tt.spec.Annotations[k]; vv != v {
+						subtest.Fatalf("annotation %q was incorrectly expanded to %q, expected %q", k, vv, v)
+					}
+				}
+			})
+		}
+
+		// test errors raised on conflict
+		t.Run(tt.name+"_disable_unsafe_error", func(subtest *testing.T) {
+			tt.spec.Annotations = map[string]string{
+				annotations.DisableUnsafeOperations:   "true",
+				annotations.DisableWritableFileShares: "false",
+			}
+
+			errExp := fmt.Sprintf("could not expand %q into %q",
+				annotations.DisableUnsafeOperations,
+				annotations.DisableWritableFileShares)
+
+			err := ProcessAnnotations(ctx, &tt.spec)
+			if !errors.Is(err, ErrAnnotationExpansionConflict) {
+				t.Fatalf("UpdateSpecFromOptions should have failed with %q, actual was %v", errExp, err)
+			}
+		})
+	}
+}