From e6966f5e692af0cde791e0a42331b80bf531fdb8 Mon Sep 17 00:00:00 2001 From: Maksim An Date: Mon, 21 Mar 2022 09:21:06 -0700 Subject: [PATCH] pr feedback and minor fixes Signed-off-by: Maksim An --- .../guest/runtime/hcsv2/workload_container.go | 63 +++++++++---------- internal/spec/spec.go | 43 ------------- pkg/securitypolicy/securitypolicyenforcer.go | 24 +++---- .../Microsoft/hcsshim/internal/spec/spec.go | 43 ------------- .../securitypolicy/securitypolicyenforcer.go | 24 +++---- 5 files changed, 55 insertions(+), 142 deletions(-) diff --git a/internal/guest/runtime/hcsv2/workload_container.go b/internal/guest/runtime/hcsv2/workload_container.go index 7fe4639eb4..f17359e7c7 100644 --- a/internal/guest/runtime/hcsv2/workload_container.go +++ b/internal/guest/runtime/hcsv2/workload_container.go @@ -5,7 +5,6 @@ package hcsv2 import ( "context" - "fmt" "os" "path/filepath" "strings" @@ -36,56 +35,56 @@ func mkdirAllModePerm(target string) error { } func updateSandboxMounts(sbid string, spec *oci.Spec) error { - for i, specMount := range spec.Mounts { - for _, updMount := range specInternal.GenerateSandboxMounts(sbid, spec) { - if specMount.Destination != updMount.Destination { - continue - } - src := filepath.Clean(updMount.Source) - // Make sure that the cleaned path is still under the correct directory. - if !strings.HasPrefix(src, guestpath.SandboxMountsDir(sbid)) { - return fmt.Errorf("mount path %v for mount %v is not within sandbox's mounts dir", updMount.Source, specMount.Source) + for i, m := range spec.Mounts { + if strings.HasPrefix(m.Source, guestpath.SandboxMountPrefix) { + mountsDir := guestpath.SandboxMountsDir(sbid) + subPath := strings.TrimPrefix(m.Source, guestpath.SandboxMountPrefix) + sandboxSource := filepath.Join(mountsDir, subPath) + + // filepath.Join cleans the resulting path before returning, so it would resolve the relative path if one was given. + // Hence, we need to ensure that the resolved path is still under the correct directory + if !strings.HasPrefix(sandboxSource, mountsDir) { + return errors.Errorf("mount path %v for mount %v is not within sandbox's mounts dir", sandboxSource, m.Source) } - spec.Mounts[i].Source = updMount.Source + spec.Mounts[i].Source = sandboxSource - if _, err := os.Stat(src); os.IsNotExist(err) { - if err := mkdirAllModePerm(src); err != nil { + _, err := os.Stat(sandboxSource) + if os.IsNotExist(err) { + if err := mkdirAllModePerm(sandboxSource); err != nil { return err } } - break } } return nil } func updateHugePageMounts(sbid string, spec *oci.Spec) error { - for i, specMount := range spec.Mounts { - for _, updMount := range specInternal.GenerateHugePageMounts(sbid, spec) { - if specMount.Destination != updMount.Destination { - continue - } - hugePagesMountDir := guestpath.HugePagesMountsDir(sbid) - src := filepath.Clean(updMount.Source) - // Make sure that the cleaned path is still under the correct directory. - if !strings.HasPrefix(src, hugePagesMountDir) { - return fmt.Errorf("mount path %v for mount %v is not within hugepages's mounts dir", src, specMount.Source) + for i, m := range spec.Mounts { + if strings.HasPrefix(m.Source, guestpath.HugePagesMountPrefix) { + mountsDir := guestpath.HugePagesMountsDir(sbid) + subPath := strings.TrimPrefix(m.Source, guestpath.HugePagesMountPrefix) + pageSize := strings.Split(subPath, string(os.PathSeparator))[0] + hugePageMountSource := filepath.Join(mountsDir, subPath) + + // filepath.Join cleans the resulting path before returning so it would resolve the relative path if one was given. + // Hence, we need to ensure that the resolved path is still under the correct directory + if !strings.HasPrefix(hugePageMountSource, mountsDir) { + return errors.Errorf("mount path %v for mount %v is not within hugepages's mounts dir", hugePageMountSource, m.Source) } - spec.Mounts[i].Source = src + spec.Mounts[i].Source = hugePageMountSource - subPath := strings.TrimPrefix(src, hugePagesMountDir) - pageSize := strings.Split(subPath, string(os.PathSeparator))[0] - if _, err := os.Stat(src); os.IsNotExist(err) { - if err := mkdirAllModePerm(src); err != nil { + _, err := os.Stat(hugePageMountSource) + if os.IsNotExist(err) { + if err := mkdirAllModePerm(hugePageMountSource); err != nil { return err } - if err := unix.Mount("none", src, "hugetlbfs", 0, "pageSize="+pageSize); err != nil { - return fmt.Errorf("mount operation failed for %v failed with error %v", src, err) + if err := unix.Mount("none", hugePageMountSource, "hugetlbfs", 0, "pagesize="+pageSize); err != nil { + return errors.Errorf("mount operation failed for %v failed with error %v", hugePageMountSource, err) } } - break } } return nil diff --git a/internal/spec/spec.go b/internal/spec/spec.go index b605de6f83..cd2730843b 100644 --- a/internal/spec/spec.go +++ b/internal/spec/spec.go @@ -72,28 +72,6 @@ func SandboxMountSource(sandboxID, path string) string { return filepath.Join(mountsDir, subPath) } -// GenerateSandboxMounts generates an array of specs.Mount by replacing sandbox -// prefix with the actual path inside UVM. Original spec is left untouched and -// it's the responsibility of a caller to update it. -func GenerateSandboxMounts(sandboxID string, spec *specs.Spec) []specs.Mount { - var sandboxMounts []specs.Mount - - for _, m := range spec.Mounts { - if !strings.HasPrefix(m.Source, guestpath.SandboxMountPrefix) { - continue - } - // copy over all fields except for the Source. - mnt := specs.Mount{ - Source: SandboxMountSource(sandboxID, m.Source), - Destination: m.Destination, - Type: m.Type, - Options: m.Options, - } - sandboxMounts = append(sandboxMounts, mnt) - } - return sandboxMounts -} - // HugePagesMountSource returns hugepages mount path inside UVM func HugePagesMountSource(sandboxID, path string) string { mountsDir := guestpath.HugePagesMountsDir(sandboxID) @@ -101,27 +79,6 @@ func HugePagesMountSource(sandboxID, path string) string { return filepath.Join(mountsDir, subPath) } -// GenerateHugePageMounts generates an array of specs.Mount by replacing -// hugepages prefix with the actual path inside UVM. Original spec is left -// untouched and it's the responsibility of a caller to update it. -func GenerateHugePageMounts(sandboxID string, spec *specs.Spec) []specs.Mount { - var hpMounts []specs.Mount - for _, m := range spec.Mounts { - if !strings.HasPrefix(m.Source, guestpath.HugePagesMountPrefix) { - continue - } - // copy over all fields except for the Source - mnt := specs.Mount{ - Source: HugePagesMountSource(sandboxID, m.Source), - Destination: m.Destination, - Type: m.Type, - Options: m.Options, - } - hpMounts = append(hpMounts, mnt) - } - return hpMounts -} - // MountPresent checks if mountPath is present in the specMounts array. func MountPresent(mountPath string, specMounts []specs.Mount) bool { for _, m := range specMounts { diff --git a/pkg/securitypolicy/securitypolicyenforcer.go b/pkg/securitypolicy/securitypolicyenforcer.go index 9c7b44823e..6eca35924b 100644 --- a/pkg/securitypolicy/securitypolicyenforcer.go +++ b/pkg/securitypolicy/securitypolicyenforcer.go @@ -129,7 +129,7 @@ func NewStandardSecurityPolicyEnforcer(containers []securityPolicyContainer, enc } } -func (c *Containers) toInternal() ([]securityPolicyContainer, error) { +func (c Containers) toInternal() ([]securityPolicyContainer, error) { containerMapLength := len(c.Elements) if c.Length != containerMapLength { err := fmt.Errorf( @@ -153,33 +153,33 @@ func (c *Containers) toInternal() ([]securityPolicyContainer, error) { return nil, err } // save off new container - internal[i] = *cInternal + internal[i] = cInternal } return internal, nil } -func (c *Container) toInternal() (*securityPolicyContainer, error) { +func (c Container) toInternal() (securityPolicyContainer, error) { command, err := c.Command.toInternal() if err != nil { - return nil, err + return securityPolicyContainer{}, err } envRules, err := c.EnvRules.toInternal() if err != nil { - return nil, err + return securityPolicyContainer{}, err } layers, err := c.Layers.toInternal() if err != nil { - return nil, err + return securityPolicyContainer{}, err } mounts, err := c.Mounts.toInternal() if err != nil { - return nil, err + return securityPolicyContainer{}, err } - return &securityPolicyContainer{ + return securityPolicyContainer{ Command: command, EnvRules: envRules, Layers: layers, @@ -190,7 +190,7 @@ func (c *Container) toInternal() (*securityPolicyContainer, error) { }, nil } -func (c *CommandArgs) toInternal() ([]string, error) { +func (c CommandArgs) toInternal() ([]string, error) { if c.Length != len(c.Elements) { return nil, fmt.Errorf("command argument numbers don't match in policy. expected: %d, actual: %d", c.Length, len(c.Elements)) } @@ -217,7 +217,7 @@ func (e EnvRules) toInternal() ([]EnvRuleConfig, error) { return envRules, nil } -func (l *Layers) toInternal() ([]string, error) { +func (l Layers) toInternal() ([]string, error) { if l.Length != len(l.Elements) { return nil, fmt.Errorf("layer numbers don't match in policy. expected: %d, actual: %d", l.Length, len(l.Elements)) } @@ -225,7 +225,7 @@ func (l *Layers) toInternal() ([]string, error) { return stringMapToStringArray(l.Elements) } -func (o *Options) toInternal() ([]string, error) { +func (o Options) toInternal() ([]string, error) { optLength := len(o.Elements) if o.Length != optLength { return nil, fmt.Errorf("mount option numbers don't match in policy. expected: %d, actual: %d", o.Length, optLength) @@ -233,7 +233,7 @@ func (o *Options) toInternal() ([]string, error) { return stringMapToStringArray(o.Elements) } -func (m *Mounts) toInternal() ([]mountInternal, error) { +func (m Mounts) toInternal() ([]mountInternal, error) { mountLength := len(m.Elements) if m.Length != mountLength { return nil, fmt.Errorf("mount constraint numbers don't match in policy. expected: %d, actual: %d", m.Length, mountLength) diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/spec/spec.go b/test/vendor/github.com/Microsoft/hcsshim/internal/spec/spec.go index b605de6f83..cd2730843b 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/spec/spec.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/spec/spec.go @@ -72,28 +72,6 @@ func SandboxMountSource(sandboxID, path string) string { return filepath.Join(mountsDir, subPath) } -// GenerateSandboxMounts generates an array of specs.Mount by replacing sandbox -// prefix with the actual path inside UVM. Original spec is left untouched and -// it's the responsibility of a caller to update it. -func GenerateSandboxMounts(sandboxID string, spec *specs.Spec) []specs.Mount { - var sandboxMounts []specs.Mount - - for _, m := range spec.Mounts { - if !strings.HasPrefix(m.Source, guestpath.SandboxMountPrefix) { - continue - } - // copy over all fields except for the Source. - mnt := specs.Mount{ - Source: SandboxMountSource(sandboxID, m.Source), - Destination: m.Destination, - Type: m.Type, - Options: m.Options, - } - sandboxMounts = append(sandboxMounts, mnt) - } - return sandboxMounts -} - // HugePagesMountSource returns hugepages mount path inside UVM func HugePagesMountSource(sandboxID, path string) string { mountsDir := guestpath.HugePagesMountsDir(sandboxID) @@ -101,27 +79,6 @@ func HugePagesMountSource(sandboxID, path string) string { return filepath.Join(mountsDir, subPath) } -// GenerateHugePageMounts generates an array of specs.Mount by replacing -// hugepages prefix with the actual path inside UVM. Original spec is left -// untouched and it's the responsibility of a caller to update it. -func GenerateHugePageMounts(sandboxID string, spec *specs.Spec) []specs.Mount { - var hpMounts []specs.Mount - for _, m := range spec.Mounts { - if !strings.HasPrefix(m.Source, guestpath.HugePagesMountPrefix) { - continue - } - // copy over all fields except for the Source - mnt := specs.Mount{ - Source: HugePagesMountSource(sandboxID, m.Source), - Destination: m.Destination, - Type: m.Type, - Options: m.Options, - } - hpMounts = append(hpMounts, mnt) - } - return hpMounts -} - // MountPresent checks if mountPath is present in the specMounts array. func MountPresent(mountPath string, specMounts []specs.Mount) bool { for _, m := range specMounts { 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 9c7b44823e..6eca35924b 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go +++ b/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go @@ -129,7 +129,7 @@ func NewStandardSecurityPolicyEnforcer(containers []securityPolicyContainer, enc } } -func (c *Containers) toInternal() ([]securityPolicyContainer, error) { +func (c Containers) toInternal() ([]securityPolicyContainer, error) { containerMapLength := len(c.Elements) if c.Length != containerMapLength { err := fmt.Errorf( @@ -153,33 +153,33 @@ func (c *Containers) toInternal() ([]securityPolicyContainer, error) { return nil, err } // save off new container - internal[i] = *cInternal + internal[i] = cInternal } return internal, nil } -func (c *Container) toInternal() (*securityPolicyContainer, error) { +func (c Container) toInternal() (securityPolicyContainer, error) { command, err := c.Command.toInternal() if err != nil { - return nil, err + return securityPolicyContainer{}, err } envRules, err := c.EnvRules.toInternal() if err != nil { - return nil, err + return securityPolicyContainer{}, err } layers, err := c.Layers.toInternal() if err != nil { - return nil, err + return securityPolicyContainer{}, err } mounts, err := c.Mounts.toInternal() if err != nil { - return nil, err + return securityPolicyContainer{}, err } - return &securityPolicyContainer{ + return securityPolicyContainer{ Command: command, EnvRules: envRules, Layers: layers, @@ -190,7 +190,7 @@ func (c *Container) toInternal() (*securityPolicyContainer, error) { }, nil } -func (c *CommandArgs) toInternal() ([]string, error) { +func (c CommandArgs) toInternal() ([]string, error) { if c.Length != len(c.Elements) { return nil, fmt.Errorf("command argument numbers don't match in policy. expected: %d, actual: %d", c.Length, len(c.Elements)) } @@ -217,7 +217,7 @@ func (e EnvRules) toInternal() ([]EnvRuleConfig, error) { return envRules, nil } -func (l *Layers) toInternal() ([]string, error) { +func (l Layers) toInternal() ([]string, error) { if l.Length != len(l.Elements) { return nil, fmt.Errorf("layer numbers don't match in policy. expected: %d, actual: %d", l.Length, len(l.Elements)) } @@ -225,7 +225,7 @@ func (l *Layers) toInternal() ([]string, error) { return stringMapToStringArray(l.Elements) } -func (o *Options) toInternal() ([]string, error) { +func (o Options) toInternal() ([]string, error) { optLength := len(o.Elements) if o.Length != optLength { return nil, fmt.Errorf("mount option numbers don't match in policy. expected: %d, actual: %d", o.Length, optLength) @@ -233,7 +233,7 @@ func (o *Options) toInternal() ([]string, error) { return stringMapToStringArray(o.Elements) } -func (m *Mounts) toInternal() ([]mountInternal, error) { +func (m Mounts) toInternal() ([]mountInternal, error) { mountLength := len(m.Elements) if m.Length != mountLength { return nil, fmt.Errorf("mount constraint numbers don't match in policy. expected: %d, actual: %d", m.Length, mountLength)