From f094fd751c1bf673ddc91acd29dd47c6f4d09eeb Mon Sep 17 00:00:00 2001 From: Maksim An Date: Fri, 14 Jan 2022 16:50:57 -0800 Subject: [PATCH] Refactor code for security policy The current implementation exposes some internal structure, which is unnecessary as well as some structs are duplicated across security policy package and security policy tool. This PR refactors code to de-duplicate exported structures and hides internal implementation behind new factory methods. Signed-off-by: Maksim An --- internal/tools/securitypolicy/main.go | 176 ++++-------------- pkg/securitypolicy/securitypolicy.go | 124 +++++++++++- pkg/securitypolicy/securitypolicy_test.go | 8 +- pkg/securitypolicy/securitypolicyenforcer.go | 8 +- .../pkg/securitypolicy/securitypolicy.go | 124 +++++++++++- .../securitypolicy/securitypolicyenforcer.go | 8 +- 6 files changed, 280 insertions(+), 168 deletions(-) diff --git a/internal/tools/securitypolicy/main.go b/internal/tools/securitypolicy/main.go index ad7e6d4498..4c03e0e793 100644 --- a/internal/tools/securitypolicy/main.go +++ b/internal/tools/securitypolicy/main.go @@ -7,8 +7,6 @@ import ( "fmt" "io/ioutil" "os" - "regexp" - "strconv" "github.com/BurntSushi/toml" "github.com/Microsoft/hcsshim/ext4/tar2ext4" @@ -36,21 +34,18 @@ func main() { return err } - config := &Config{ - AllowAll: false, - Containers: []Container{}, - } + config := &securitypolicy.PolicyConfig{} err = toml.Unmarshal(configData, config) if err != nil { return err } - policy, err := func() (securitypolicy.SecurityPolicy, error) { + policy, err := func() (*securitypolicy.SecurityPolicy, error) { if config.AllowAll { - return createOpenDoorPolicy(), nil + return securitypolicy.NewOpenDoorPolicy(), nil } else { - return createPolicyFromConfig(*config) + return createPolicyFromConfig(config) } }() @@ -77,190 +72,87 @@ func main() { } } -type EnvironmentVariableRule struct { - Strategy securitypolicy.EnvVarRule `toml:"strategy"` - Rule string `toml:"rule"` -} - -type Container struct { - Name string `toml:"name"` - Auth ImageAuth `toml:"auth"` - Command []string `toml:"command"` - EnvRules []EnvironmentVariableRule `toml:"env_rule"` -} - -type ImageAuth struct { - Username string `toml:"username"` - Password string `toml:"password"` -} - -type Config struct { - AllowAll bool `toml:"allow_all"` - Containers []Container `toml:"container"` -} - -func createOpenDoorPolicy() securitypolicy.SecurityPolicy { - return securitypolicy.SecurityPolicy{ - AllowAll: true, - } -} - -func createPolicyFromConfig(config Config) (securitypolicy.SecurityPolicy, error) { - p := securitypolicy.SecurityPolicy{ - Containers: securitypolicy.Containers{ - Elements: map[string]securitypolicy.Container{}, - }, - } - +func createPolicyFromConfig(config *securitypolicy.PolicyConfig) (*securitypolicy.SecurityPolicy, error) { // Hardcode the pause container version and command. We still pull it // to get the root hash and any environment variable rules we might need. - pause := Container{ - Name: "k8s.gcr.io/pause:3.1", - Command: []string{"/pause"}, - EnvRules: []EnvironmentVariableRule{}} + pause := securitypolicy.NewContainerConfig( + "k8s.gcr.io/pause:3.1", + []string{"/pause"}, + []securitypolicy.EnvRule{}, + securitypolicy.AuthConfig{}, + ) config.Containers = append(config.Containers, pause) - for _, configContainer := range config.Containers { + var policyContainers []*securitypolicy.Container + for _, containerConfig := range config.Containers { var imageOptions []remote.Option - if configContainer.Auth.Username != "" && configContainer.Auth.Password != "" { + if containerConfig.Auth.Username != "" && containerConfig.Auth.Password != "" { auth := authn.Basic{ - Username: configContainer.Auth.Username, - Password: configContainer.Auth.Password} + Username: containerConfig.Auth.Username, + Password: containerConfig.Auth.Password} c, _ := auth.Authorization() authOption := remote.WithAuth(authn.FromConfig(*c)) imageOptions = append(imageOptions, authOption) } - // validate EnvRules - err := validateEnvRules(configContainer.EnvRules) + ref, err := name.ParseReference(containerConfig.ImageName) if err != nil { - return p, err - } - - command := convertCommand(configContainer.Command) - envRules := convertEnvironmentVariableRules(configContainer.EnvRules) - container := securitypolicy.Container{ - Command: command, - EnvRules: envRules, - Layers: securitypolicy.Layers{ - Elements: map[string]string{}, - }, - } - ref, err := name.ParseReference(configContainer.Name) - if err != nil { - return p, fmt.Errorf("'%s' isn't a valid image name", configContainer.Name) + return nil, fmt.Errorf("'%s' isn't a valid image name", containerConfig.ImageName) } img, err := remote.Image(ref, imageOptions...) if err != nil { - return p, fmt.Errorf("unable to fetch image '%s': %s", configContainer.Name, err.Error()) + return nil, fmt.Errorf("unable to fetch image '%s': %s", containerConfig.ImageName, err.Error()) } layers, err := img.Layers() if err != nil { - return p, err + return nil, err } + var layerHashes []string for _, layer := range layers { r, err := layer.Uncompressed() if err != nil { - return p, err + return nil, err } hashString, err := tar2ext4.ConvertAndComputeRootDigest(r) if err != nil { - return p, err + return nil, err } - addLayer(&container.Layers, hashString) + layerHashes = append(layerHashes, hashString) } // add rules for all known environment variables from the configuration // these are in addition to "other rules" from the policy definition file imgConfig, err := img.ConfigFile() if err != nil { - return p, err + return nil, err } + + envRules := containerConfig.EnvRules for _, env := range imgConfig.Config.Env { rule := securitypolicy.EnvRule{ Strategy: securitypolicy.EnvVarRuleString, Rule: env, } - - addEnvRule(&container.EnvRules, rule) + envRules = append(envRules, rule) } - // cri adds TERM=xterm for all workload containers. we add to all containers // to prevent any possible error rule := securitypolicy.EnvRule{ Strategy: securitypolicy.EnvVarRuleString, Rule: "TERM=xterm", } + envRules = append(envRules, rule) - addEnvRule(&container.EnvRules, rule) - - addContainer(&p.Containers, container) - } - - return p, nil -} - -func validateEnvRules(rules []EnvironmentVariableRule) error { - for _, rule := range rules { - switch rule.Strategy { - case securitypolicy.EnvVarRuleRegex: - _, err := regexp.Compile(rule.Rule) - if err != nil { - return err - } - } - } - - return nil -} - -func convertCommand(toml []string) securitypolicy.CommandArgs { - jsn := map[string]string{} - - for i, arg := range toml { - jsn[strconv.Itoa(i)] = arg - } - - return securitypolicy.CommandArgs{ - Elements: jsn, - } -} - -func convertEnvironmentVariableRules(toml []EnvironmentVariableRule) securitypolicy.EnvRules { - jsn := map[string]securitypolicy.EnvRule{} - - for i, rule := range toml { - jsonRule := securitypolicy.EnvRule{ - Strategy: rule.Strategy, - Rule: rule.Rule, + container, err := securitypolicy.NewContainer(containerConfig.Command, layerHashes, envRules) + if err != nil { + return nil, err } - - jsn[strconv.Itoa(i)] = jsonRule + policyContainers = append(policyContainers, container) } - return securitypolicy.EnvRules{ - Elements: jsn, - } -} - -func addContainer(containers *securitypolicy.Containers, container securitypolicy.Container) { - index := strconv.Itoa(len(containers.Elements)) - - containers.Elements[index] = container -} - -func addLayer(layers *securitypolicy.Layers, layer string) { - index := strconv.Itoa(len(layers.Elements)) - - layers.Elements[index] = layer -} - -func addEnvRule(rules *securitypolicy.EnvRules, rule securitypolicy.EnvRule) { - index := strconv.Itoa(len(rules.Elements)) - - rules.Elements[index] = rule + return securitypolicy.NewSecurityPolicy(false, policyContainers), nil } diff --git a/pkg/securitypolicy/securitypolicy.go b/pkg/securitypolicy/securitypolicy.go index 3b381ccf2a..7a78eb7b42 100644 --- a/pkg/securitypolicy/securitypolicy.go +++ b/pkg/securitypolicy/securitypolicy.go @@ -3,6 +3,8 @@ package securitypolicy import ( "encoding/base64" "encoding/json" + "regexp" + "strconv" "github.com/pkg/errors" ) @@ -14,12 +16,57 @@ const ( EnvVarRuleRegex EnvVarRule = "re2" ) +// PolicyConfig contains toml or JSON config for security policy. +type PolicyConfig struct { + AllowAll bool `json:"allow_all" toml:"allow_all"` + Containers []ContainerConfig `json:"containers" toml:"container"` +} + +// AuthConfig contains toml or JSON config for registry authentication. +type AuthConfig struct { + Username string `json:"username" toml:"username"` + Password string `json:"password" toml:"password"` +} + +// 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"` +} + +// ContainerConfig contains toml or JSON config for container described +// in security policy. +type ContainerConfig struct { + ImageName string `json:"image_name" toml:"image_name"` + Command []string `json:"command" toml:"command"` + Auth AuthConfig `json:"auth" toml:"auth"` + EnvRules []EnvRule `json:"env_rules" toml:"env_rule"` +} + +// NewContainerConfig creates a new ContainerConfig from the given values. +func NewContainerConfig(imageName string, command []string, envRules []EnvRule, auth AuthConfig) ContainerConfig { + return ContainerConfig{ + ImageName: imageName, + Command: command, + EnvRules: envRules, + Auth: auth, + } +} + +// NewOpenDoorPolicy creates a new SecurityPolicy with AllowAll set to `true` +func NewOpenDoorPolicy() *SecurityPolicy { + return &SecurityPolicy{ + AllowAll: true, + } +} + // Internal version of SecurityPolicyContainer type securityPolicyContainer struct { // The command that we will allow the container to execute Command []string `json:"command"` // The rules for determining if a given environment variable is allowed - EnvRules []securityPolicyEnvironmentVariableRule `json:"env_rules"` + EnvRules []EnvRule `json:"env_rules"` // An ordered list of dm-verity root hashes for each layer that makes up // "a container". Containers are constructed as an overlay file system. The // order that the layers are overlayed is important and needs to be enforced @@ -27,12 +74,6 @@ type securityPolicyContainer struct { Layers []string `json:"layers"` } -// Internal versino of SecurityPolicyEnvironmentVariableRule -type securityPolicyEnvironmentVariableRule struct { - Strategy EnvVarRule `json:"type"` - Rule string `json:"rule"` -} - // SecurityPolicyState is a structure that holds user supplied policy to enforce // we keep both the encoded representation and the unmarshalled representation // because different components need to have access to either of these @@ -127,6 +168,75 @@ type EnvRule struct { Rule string `json:"rule"` } +// NewContainer creates a new Container instance from the provided values +// or an error if envRules validation fails. +func NewContainer(command, layers []string, envRules []EnvRule) (*Container, error) { + if err := validateEnvRules(envRules); err != nil { + return nil, err + } + return &Container{ + Command: newCommandArgs(command), + Layers: newLayers(layers), + EnvRules: newEnvRules(envRules), + }, nil +} + +// NewSecurityPolicy creates a new SecurityPolicy from the provided values. +func NewSecurityPolicy(allowAll bool, containers []*Container) *SecurityPolicy { + containersMap := map[string]Container{} + for i, c := range containers { + containersMap[strconv.Itoa(i)] = *c + } + return &SecurityPolicy{ + AllowAll: allowAll, + Containers: Containers{ + Elements: containersMap, + }, + } +} + +func validateEnvRules(rules []EnvRule) error { + for _, rule := range rules { + switch rule.Strategy { + case EnvVarRuleRegex: + if _, err := regexp.Compile(rule.Rule); err != nil { + return err + } + } + } + return nil +} + +func newCommandArgs(args []string) CommandArgs { + command := map[string]string{} + for i, arg := range args { + command[strconv.Itoa(i)] = arg + } + return CommandArgs{ + Elements: command, + } +} + +func newEnvRules(rs []EnvRule) EnvRules { + envRules := map[string]EnvRule{} + for i, r := range rs { + envRules[strconv.Itoa(i)] = r + } + return EnvRules{ + Elements: envRules, + } +} + +func newLayers(ls []string) Layers { + layers := map[string]string{} + for i, l := range ls { + layers[strconv.Itoa(i)] = l + } + return Layers{ + Elements: layers, + } +} + // Custom JSON marshalling to add `lenth` field that matches the number of // elements present in the `elements` field. func (c Containers) MarshalJSON() ([]byte, error) { diff --git a/pkg/securitypolicy/securitypolicy_test.go b/pkg/securitypolicy/securitypolicy_test.go index f9d1daea25..f92019afb1 100644 --- a/pkg/securitypolicy/securitypolicy_test.go +++ b/pkg/securitypolicy/securitypolicy_test.go @@ -567,7 +567,7 @@ func Test_EnforceEnvironmentVariablePolicy_Re2Match(t *testing.T) { container := generateContainersContainer(r, 1) // add a rule to re2 match - re2MatchRule := securityPolicyEnvironmentVariableRule{ + re2MatchRule := EnvRule{ Strategy: EnvVarRuleRegex, Rule: "PREFIX_.+=.+"} container.EnvRules = append(container.EnvRules, re2MatchRule) @@ -838,12 +838,12 @@ func generateCommand(r *rand.Rand) []string { return args } -func generateEnvironmentVariableRules(r *rand.Rand) []securityPolicyEnvironmentVariableRule { - rules := []securityPolicyEnvironmentVariableRule{} +func generateEnvironmentVariableRules(r *rand.Rand) []EnvRule { + var rules []EnvRule numArgs := atLeastOneAtMost(r, maxGeneratedEnvironmentVariableRules) for i := 0; i < int(numArgs); i++ { - rule := securityPolicyEnvironmentVariableRule{ + rule := EnvRule{ Strategy: "string", Rule: randVariableString(r, maxGeneratedEnvironmentVariableRuleLength), } diff --git a/pkg/securitypolicy/securitypolicyenforcer.go b/pkg/securitypolicy/securitypolicyenforcer.go index 35807079d9..21605509f3 100644 --- a/pkg/securitypolicy/securitypolicyenforcer.go +++ b/pkg/securitypolicy/securitypolicyenforcer.go @@ -175,16 +175,16 @@ func (c CommandArgs) toInternal() ([]string, error) { return stringMapToStringArray(c.Elements), nil } -func (e EnvRules) toInternal() ([]securityPolicyEnvironmentVariableRule, error) { +func (e EnvRules) toInternal() ([]EnvRule, error) { envRulesMapLength := len(e.Elements) if e.Length != envRulesMapLength { return nil, fmt.Errorf("env rule numbers don't match in policy. expected: %d, actual: %d", e.Length, envRulesMapLength) } - envRules := make([]securityPolicyEnvironmentVariableRule, envRulesMapLength) + envRules := make([]EnvRule, envRulesMapLength) for i := 0; i < envRulesMapLength; i++ { eIndex := strconv.Itoa(i) - rule := securityPolicyEnvironmentVariableRule{ + rule := EnvRule{ Strategy: e.Elements[eIndex].Strategy, Rule: e.Elements[eIndex].Rule, } @@ -385,7 +385,7 @@ func (pe *StandardSecurityPolicyEnforcer) enforceEnvironmentVariablePolicy(conta return nil } -func envIsMatchedByRule(envVariable string, rules []securityPolicyEnvironmentVariableRule) bool { +func envIsMatchedByRule(envVariable string, rules []EnvRule) bool { for _, rule := range rules { switch rule.Strategy { case "string": diff --git a/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicy.go b/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicy.go index 3b381ccf2a..7a78eb7b42 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicy.go +++ b/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicy.go @@ -3,6 +3,8 @@ package securitypolicy import ( "encoding/base64" "encoding/json" + "regexp" + "strconv" "github.com/pkg/errors" ) @@ -14,12 +16,57 @@ const ( EnvVarRuleRegex EnvVarRule = "re2" ) +// PolicyConfig contains toml or JSON config for security policy. +type PolicyConfig struct { + AllowAll bool `json:"allow_all" toml:"allow_all"` + Containers []ContainerConfig `json:"containers" toml:"container"` +} + +// AuthConfig contains toml or JSON config for registry authentication. +type AuthConfig struct { + Username string `json:"username" toml:"username"` + Password string `json:"password" toml:"password"` +} + +// 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"` +} + +// ContainerConfig contains toml or JSON config for container described +// in security policy. +type ContainerConfig struct { + ImageName string `json:"image_name" toml:"image_name"` + Command []string `json:"command" toml:"command"` + Auth AuthConfig `json:"auth" toml:"auth"` + EnvRules []EnvRule `json:"env_rules" toml:"env_rule"` +} + +// NewContainerConfig creates a new ContainerConfig from the given values. +func NewContainerConfig(imageName string, command []string, envRules []EnvRule, auth AuthConfig) ContainerConfig { + return ContainerConfig{ + ImageName: imageName, + Command: command, + EnvRules: envRules, + Auth: auth, + } +} + +// NewOpenDoorPolicy creates a new SecurityPolicy with AllowAll set to `true` +func NewOpenDoorPolicy() *SecurityPolicy { + return &SecurityPolicy{ + AllowAll: true, + } +} + // Internal version of SecurityPolicyContainer type securityPolicyContainer struct { // The command that we will allow the container to execute Command []string `json:"command"` // The rules for determining if a given environment variable is allowed - EnvRules []securityPolicyEnvironmentVariableRule `json:"env_rules"` + EnvRules []EnvRule `json:"env_rules"` // An ordered list of dm-verity root hashes for each layer that makes up // "a container". Containers are constructed as an overlay file system. The // order that the layers are overlayed is important and needs to be enforced @@ -27,12 +74,6 @@ type securityPolicyContainer struct { Layers []string `json:"layers"` } -// Internal versino of SecurityPolicyEnvironmentVariableRule -type securityPolicyEnvironmentVariableRule struct { - Strategy EnvVarRule `json:"type"` - Rule string `json:"rule"` -} - // SecurityPolicyState is a structure that holds user supplied policy to enforce // we keep both the encoded representation and the unmarshalled representation // because different components need to have access to either of these @@ -127,6 +168,75 @@ type EnvRule struct { Rule string `json:"rule"` } +// NewContainer creates a new Container instance from the provided values +// or an error if envRules validation fails. +func NewContainer(command, layers []string, envRules []EnvRule) (*Container, error) { + if err := validateEnvRules(envRules); err != nil { + return nil, err + } + return &Container{ + Command: newCommandArgs(command), + Layers: newLayers(layers), + EnvRules: newEnvRules(envRules), + }, nil +} + +// NewSecurityPolicy creates a new SecurityPolicy from the provided values. +func NewSecurityPolicy(allowAll bool, containers []*Container) *SecurityPolicy { + containersMap := map[string]Container{} + for i, c := range containers { + containersMap[strconv.Itoa(i)] = *c + } + return &SecurityPolicy{ + AllowAll: allowAll, + Containers: Containers{ + Elements: containersMap, + }, + } +} + +func validateEnvRules(rules []EnvRule) error { + for _, rule := range rules { + switch rule.Strategy { + case EnvVarRuleRegex: + if _, err := regexp.Compile(rule.Rule); err != nil { + return err + } + } + } + return nil +} + +func newCommandArgs(args []string) CommandArgs { + command := map[string]string{} + for i, arg := range args { + command[strconv.Itoa(i)] = arg + } + return CommandArgs{ + Elements: command, + } +} + +func newEnvRules(rs []EnvRule) EnvRules { + envRules := map[string]EnvRule{} + for i, r := range rs { + envRules[strconv.Itoa(i)] = r + } + return EnvRules{ + Elements: envRules, + } +} + +func newLayers(ls []string) Layers { + layers := map[string]string{} + for i, l := range ls { + layers[strconv.Itoa(i)] = l + } + return Layers{ + Elements: layers, + } +} + // Custom JSON marshalling to add `lenth` field that matches the number of // elements present in the `elements` field. func (c Containers) MarshalJSON() ([]byte, error) { diff --git a/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go b/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go index 35807079d9..21605509f3 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go +++ b/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go @@ -175,16 +175,16 @@ func (c CommandArgs) toInternal() ([]string, error) { return stringMapToStringArray(c.Elements), nil } -func (e EnvRules) toInternal() ([]securityPolicyEnvironmentVariableRule, error) { +func (e EnvRules) toInternal() ([]EnvRule, error) { envRulesMapLength := len(e.Elements) if e.Length != envRulesMapLength { return nil, fmt.Errorf("env rule numbers don't match in policy. expected: %d, actual: %d", e.Length, envRulesMapLength) } - envRules := make([]securityPolicyEnvironmentVariableRule, envRulesMapLength) + envRules := make([]EnvRule, envRulesMapLength) for i := 0; i < envRulesMapLength; i++ { eIndex := strconv.Itoa(i) - rule := securityPolicyEnvironmentVariableRule{ + rule := EnvRule{ Strategy: e.Elements[eIndex].Strategy, Rule: e.Elements[eIndex].Rule, } @@ -385,7 +385,7 @@ func (pe *StandardSecurityPolicyEnforcer) enforceEnvironmentVariablePolicy(conta return nil } -func envIsMatchedByRule(envVariable string, rules []securityPolicyEnvironmentVariableRule) bool { +func envIsMatchedByRule(envVariable string, rules []EnvRule) bool { for _, rule := range rules { switch rule.Strategy { case "string":