pr feedback: move config closer to tooling and refactor code

Signed-off-by: Maksim An <[email protected]>

Author: anmaxvl

internal/spec/spec.go

@@ -20,12 +20,12 @@ func DevShmMountWithSize(sizeString string) (*specs.Mount, error) {
 		return nil, errors.New("only linux runtimes are supported")
 	}
 
-	size, err := strconv.ParseInt(sizeString, 10, 64)
+	size, err := strconv.ParseUint(sizeString, 10, 64)
 	if err != nil {
 		return nil, fmt.Errorf("/dev/shm size must be a valid integer: %w", err)
 	}
-	if size <= 0 {
-		return nil, fmt.Errorf("/dev/shm size must be a positive integer, got: %d", size)
+	if size == 0 {
+		return nil, errors.New("/dev/shm size must be non-zero")
 	}
 
 	// Use the same options as in upstream https://github.com/containerd/containerd/blob/0def98e462706286e6eaeff4a90be22fda75e761/oci/mounts.go#L49
@@ -39,7 +39,7 @@ func DevShmMountWithSize(sizeString string) (*specs.Mount, error) {
 }
 
 // GenerateWorkloadContainerNetworkMounts generates an array of specs.Mount
-// required for container networking. Original spec is left untouched and
+// required for container networking. Original spec is left untouched, so
 // it's the responsibility of a caller to update it.
 func GenerateWorkloadContainerNetworkMounts(sandboxID string, spec *specs.Spec) []specs.Mount {
 	var nMounts []specs.Mount

pkg/securitypolicy/config.go renamed to internal/tools/securitypolicy/config/config.go

@@ -1,10 +1,7 @@
-package securitypolicy
+package config
 
-type EnvVarRule string
-
-const (
-	EnvVarRuleString EnvVarRule = "string"
-	EnvVarRuleRegex  EnvVarRule = "re2"
+import (
+	"github.com/Microsoft/hcsshim/pkg/securitypolicy"
 )
 
 // PolicyConfig contains toml or JSON config for security policy.
@@ -22,8 +19,8 @@ type AuthConfig struct {
 // EnvRuleConfig contains toml or JSON config for environment variable
 // security policy enforcement.
 type EnvRuleConfig struct {
-	Strategy EnvVarRule `json:"strategy" toml:"strategy"`
-	Rule     string     `json:"rule" toml:"rule"`
+	Strategy securitypolicy.EnvRuleStrategy `json:"strategy" toml:"strategy"`
+	Rule     string                         `json:"rule" toml:"rule"`
 }
 
 // ContainerConfig contains toml or JSON config for container described
@@ -46,13 +43,13 @@ type MountConfig struct {
 	Readonly      bool   `json:"readonly" toml:"readonly"`
 }
 
-// NewEnvVarRules creates slice of EnvRuleConfig's from environment variables
+// NewEnvVarRuleConfigs creates slice of EnvRuleConfig's from environment variables
 // strings slice.
-func NewEnvVarRules(envVars []string) []EnvRuleConfig {
+func NewEnvVarRuleConfigs(envVars []string) []EnvRuleConfig {
 	var rules []EnvRuleConfig
 	for _, env := range envVars {
 		r := EnvRuleConfig{
-			Strategy: EnvVarRuleString,
+			Strategy: securitypolicy.EnvRuleString,
 			Rule:     env,
 		}
 		rules = append(rules, r)

internal/tools/securitypolicy/helpers/helpers.go

@@ -2,13 +2,17 @@ package helpers
 
 import (
 	"fmt"
+	"regexp"
+	"strings"
 
+	"github.com/Microsoft/hcsshim/internal/guestpath"
 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/name"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 
 	"github.com/Microsoft/hcsshim/ext4/tar2ext4"
+	"github.com/Microsoft/hcsshim/internal/tools/securitypolicy/config"
 	"github.com/Microsoft/hcsshim/pkg/securitypolicy"
 )
 
@@ -67,17 +71,17 @@ func ParseEnvFromImage(img v1.Image) ([]string, error) {
 // DefaultContainerConfigs returns a hardcoded slice of container configs, which should
 // be included by default in the security policy.
 // The slice includes only a sandbox pause container.
-func DefaultContainerConfigs() []securitypolicy.ContainerConfig {
-	pause := securitypolicy.NewContainerConfig(
+func DefaultContainerConfigs() []config.ContainerConfig {
+	pause := config.NewContainerConfig(
 		"k8s.gcr.io/pause:3.1",
 		[]string{"/pause"},
-		[]securitypolicy.EnvRuleConfig{},
-		securitypolicy.AuthConfig{},
+		[]config.EnvRuleConfig{},
+		config.AuthConfig{},
 		"",
 		[]string{},
-		[]securitypolicy.MountConfig{},
+		[]config.MountConfig{},
 	)
-	return []securitypolicy.ContainerConfig{pause}
+	return []config.ContainerConfig{pause}
 }
 
 // ParseWorkingDirFromImage inspects the image spec and returns working directory if
@@ -95,9 +99,9 @@ func ParseWorkingDirFromImage(img v1.Image) (string, error) {
 }
 
 // PolicyContainersFromConfigs returns a slice of securitypolicy.Container generated
-// from a slice of securitypolicy.ContainerConfig's
-func PolicyContainersFromConfigs(containerConfigs []securitypolicy.ContainerConfig) ([]*securitypolicy.Container, error) {
-	var policyContainers []*securitypolicy.Container
+// from a slice of config.ContainerConfig's
+func PolicyContainersFromConfigs(containerConfigs []config.ContainerConfig) ([]securitypolicy.Container, error) {
+	var policyContainers []securitypolicy.Container
 	for _, containerConfig := range containerConfigs {
 		var imageOptions []remote.Option
 
@@ -126,7 +130,7 @@ func PolicyContainersFromConfigs(containerConfigs []securitypolicy.ContainerConf
 		if err != nil {
 			return nil, err
 		}
-		envRules := securitypolicy.NewEnvVarRules(envVars)
+		envRules := config.NewEnvVarRuleConfigs(envVars)
 		envRules = append(envRules, containerConfig.EnvRules...)
 
 		workingDir, err := ParseWorkingDirFromImage(img)
@@ -138,7 +142,7 @@ func PolicyContainersFromConfigs(containerConfigs []securitypolicy.ContainerConf
 			workingDir = containerConfig.WorkingDir
 		}
 
-		container, err := securitypolicy.CreateContainerPolicy(
+		container, err := CreateContainerPolicy(
 			containerConfig.Command,
 			layerHashes,
 			envRules,
@@ -154,3 +158,107 @@ func PolicyContainersFromConfigs(containerConfigs []securitypolicy.ContainerConf
 
 	return policyContainers, nil
 }
+
+// CreateContainerPolicy creates a new Container policy instance from the
+// provided constraints or an error if parameter validation fails.
+func CreateContainerPolicy(
+	command, layers []string,
+	envRuleConfigs []config.EnvRuleConfig,
+	workingDir string,
+	eMounts []string,
+	mntConfigs []config.MountConfig,
+) (securitypolicy.Container, error) {
+	envRules, err := createEnvVarRules(envRuleConfigs)
+	if err != nil {
+		return securitypolicy.Container{}, err
+	}
+	mntConstraints, err := createMountConstraints(mntConfigs)
+	if err != nil {
+		return securitypolicy.Container{}, err
+	}
+	return securitypolicy.NewContainer(
+		command,
+		layers,
+		envRules,
+		workingDir,
+		eMounts,
+		mntConstraints,
+	), nil
+}
+
+func createEnvVarRules(rules []config.EnvRuleConfig) ([]securitypolicy.EnvVarRule, error) {
+	var envRules []securitypolicy.EnvVarRule
+	for _, rule := range rules {
+		switch rule.Strategy {
+		case securitypolicy.EnvRuleRegex:
+			if _, err := regexp.Compile(rule.Rule); err != nil {
+				return nil, err
+			}
+		case securitypolicy.EnvRuleString:
+		default:
+			return nil, fmt.Errorf("invalid env_var strategy: %s", rule.Strategy)
+		}
+		envRules = append(envRules, securitypolicy.EnvVarRule{
+			Strategy: rule.Strategy,
+			Rule:     rule.Rule,
+		})
+	}
+	return envRules, nil
+}
+
+func createMountConstraints(mntConfigs []config.MountConfig) ([]securitypolicy.Mount, error) {
+	var mntConstraints []securitypolicy.Mount
+	for _, m := range mntConfigs {
+		if _, err := regexp.Compile(m.HostPath); err != nil {
+			return nil, err
+		}
+		mntConstraints = append(mntConstraints, newMountFromConfig(&m))
+	}
+	return mntConstraints, nil
+}
+
+// newOptionsFromConfig applies the same logic as CRI plugin to generate
+// mount options given readonly and propagation config.
+// TODO: (anmaxvl) update when support for other mount types is added,
+//   e.g., vhd:// or evd://
+// TODO: (anmaxvl) Do we need to set/validate Linux rootfs propagation?
+//   In case we do, update securityPolicyContainer and Container structs
+//   as well as mount enforcement logic.
+func newOptionsFromConfig(mCfg *config.MountConfig) []string {
+	mountOpts := []string{"rbind"}
+
+	if strings.HasPrefix(mCfg.HostPath, guestpath.SandboxMountPrefix) ||
+		strings.HasPrefix(mCfg.HostPath, guestpath.HugePagesMountPrefix) {
+		mountOpts = append(mountOpts, "rshared")
+	} else {
+		mountOpts = append(mountOpts, "rprivate")
+	}
+
+	if mCfg.Readonly {
+		mountOpts = append(mountOpts, "ro")
+	} else {
+		mountOpts = append(mountOpts, "rw")
+	}
+	return mountOpts
+}
+
+// newMountTypeFromConfig mimics the behavior in CRI when figuring out OCI
+// mount type.
+func newMountTypeFromConfig(mCfg *config.MountConfig) string {
+	if strings.HasPrefix(mCfg.HostPath, guestpath.SandboxMountPrefix) ||
+		strings.HasPrefix(mCfg.HostPath, guestpath.HugePagesMountPrefix) {
+		return "bind"
+	}
+	return "none"
+}
+
+// newMountFromConfig converts user provided MountConfig into internal representation
+// of mount constraint.
+func newMountFromConfig(mntConfig *config.MountConfig) securitypolicy.Mount {
+	return securitypolicy.NewMountConstraint(
+		mntConfig.HostPath,
+		mntConfig.ContainerPath,
+		newMountTypeFromConfig(mntConfig),
+		newOptionsFromConfig(mntConfig),
+	)
+}

internal/tools/securitypolicy/main.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/BurntSushi/toml"
 
+	"github.com/Microsoft/hcsshim/internal/tools/securitypolicy/config"
 	"github.com/Microsoft/hcsshim/internal/tools/securitypolicy/helpers"
 	"github.com/Microsoft/hcsshim/pkg/securitypolicy"
 )
@@ -32,18 +33,18 @@ func main() {
 			return err
 		}
 
-		config := &securitypolicy.PolicyConfig{}
+		cfg := &config.PolicyConfig{}
 
-		err = toml.Unmarshal(configData, config)
+		err = toml.Unmarshal(configData, cfg)
 		if err != nil {
 			return err
 		}
 
 		policy, err := func() (*securitypolicy.SecurityPolicy, error) {
-			if config.AllowAll {
+			if cfg.AllowAll {
 				return securitypolicy.NewOpenDoorPolicy(), nil
 			} else {
-				return createPolicyFromConfig(config)
+				return createPolicyFromConfig(cfg)
 			}
 		}()
 
@@ -70,12 +71,12 @@ func main() {
 	}
 }
 
-func createPolicyFromConfig(config *securitypolicy.PolicyConfig) (*securitypolicy.SecurityPolicy, error) {
+func createPolicyFromConfig(cfg *config.PolicyConfig) (*securitypolicy.SecurityPolicy, error) {
 	// Add default containers to the policy config to get the root hash
 	// and any environment variable rules we might need
 	defaultContainers := helpers.DefaultContainerConfigs()
-	config.Containers = append(config.Containers, defaultContainers...)
-	policyContainers, err := helpers.PolicyContainersFromConfigs(config.Containers)
+	cfg.Containers = append(cfg.Containers, defaultContainers...)
+	policyContainers, err := helpers.PolicyContainersFromConfigs(cfg.Containers)
 	if err != nil {
 		return nil, err
 	}