Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disable host process containers when disable unsafe operations is enabled #2164

Merged
merged 1 commit into from
Jun 21, 2024
Merged

Disable host process containers when disable unsafe operations is enabled #2164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions internal/oci/annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
)

var ErrAnnotationExpansionConflict = errors.New("annotation expansion conflict")
var ErrGenericAnnotationConflict = errors.New("specified annotations conflict")

// ProcessAnnotations expands annotations into their corresponding annotation groups.
func ProcessAnnotations(ctx context.Context, s *specs.Spec) (err error) {
Expand Down Expand Up @@ -55,6 +56,19 @@ func ProcessAnnotations(ctx context.Context, s *specs.Spec) (err error) {
}
}

// validate host process containers annotations are not conflicting
disableHPC := ParseAnnotationsBool(ctx, s.Annotations, annotations.DisableHostProcessContainer, false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably the only annotation that needs to be handled like this for now. In the future we may want to consider defining a "conflict annotations map" or similar, e.g.

conflictingAnnotations := map[string]string{
    annotations.DisableHostProcessContainer: annotations.HostProcessContainer,
}

for k, v := range conflictingAnnotations {
    v1 := ParseAnnotationsBool(ctx, s.Annotations, k, false)
    v2 := ParseAnnotationsBool(ctx, s.Annotations, v, false)
    if v1 && v2 {
        // handle conflict
    }
}

Maybe it's worth just doing it now too, rather than special casing it to HPC? thoughts?

enableHPC := ParseAnnotationsBool(ctx, s.Annotations, annotations.HostProcessContainer, false)
if disableHPC && enableHPC {
err = ErrGenericAnnotationConflict
log.G(ctx).WithFields(logrus.Fields{
logfields.OCIAnnotation: annotations.DisableHostProcessContainer,
logfields.Value: s.Annotations[annotations.DisableHostProcessContainer],
logfields.OCIAnnotation + "-conflict": annotations.HostProcessContainer,
logfields.Value + "-conflict": s.Annotations[annotations.HostProcessContainer],
}).WithError(err).Warning("Host process container and disable host process container cannot both be true")
}

return err
}

Expand Down
105 changes: 105 additions & 0 deletions internal/oci/annotations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,111 @@ import (
"github.com/Microsoft/hcsshim/pkg/annotations"
)

func TestProccessAnnotations_HostProcessContainer(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()

testAnnotations := []struct {
name string
an map[string]string
expectedSuccess bool
}{
{
name: "DisableUnsafeOperations-DisableHostProcessContainer",
an: map[string]string{
annotations.DisableUnsafeOperations: "true",
annotations.DisableHostProcessContainer: "true",
annotations.HostProcessContainer: "false",
},
expectedSuccess: true,
},
{
name: "DisableUnsafeOperations-DisableHostProcessContainer-HostProcessContainer",
an: map[string]string{
annotations.DisableUnsafeOperations: "true",
annotations.DisableHostProcessContainer: "true",
annotations.HostProcessContainer: "true",
},
expectedSuccess: false,
},
{
name: "DisableUnsafeOperations-HostProcessContainer",
an: map[string]string{
annotations.DisableUnsafeOperations: "true",
annotations.DisableHostProcessContainer: "false",
annotations.HostProcessContainer: "true",
},
expectedSuccess: false,
},
{
name: "DisableUnsafeOperations",
an: map[string]string{
annotations.DisableUnsafeOperations: "true",
annotations.DisableHostProcessContainer: "false",
annotations.HostProcessContainer: "false",
},
expectedSuccess: false,
},
{
name: "HostProcessContainer",
an: map[string]string{
annotations.DisableUnsafeOperations: "false",
annotations.DisableHostProcessContainer: "false",
annotations.HostProcessContainer: "true",
},
expectedSuccess: true,
},
{
name: "DisableHostProcessContainer-HostProcessContainer",
an: map[string]string{
annotations.DisableUnsafeOperations: "false",
annotations.DisableHostProcessContainer: "true",
annotations.HostProcessContainer: "true",
},
expectedSuccess: false,
},
{
name: "DisableHostProcessContainer",
an: map[string]string{
annotations.DisableUnsafeOperations: "false",
annotations.DisableHostProcessContainer: "true",
annotations.HostProcessContainer: "false",
},
expectedSuccess: false,
},
{
name: "All false",
an: map[string]string{
annotations.DisableUnsafeOperations: "false",
annotations.DisableHostProcessContainer: "false",
annotations.HostProcessContainer: "false",
},
expectedSuccess: true,
},
}

for _, tt := range testAnnotations {
t.Run(tt.name, func(subtest *testing.T) {
spec := specs.Spec{
Windows: &specs.Windows{},
Annotations: tt.an,
}

err := ProcessAnnotations(ctx, &spec)
if err != nil && tt.expectedSuccess {
t.Fatalf("ProcessAnnotations should have succeeded, instead got %v", err)
}
if err == nil && !tt.expectedSuccess {
t.Fatal("ProcessAnnotations should have failed due to conflicting annotations, instead returned success")
}
})
}
}

func TestProccessAnnotations_Expansion(t *testing.T) {
// suppress warnings raised by process annotation
defer func(l logrus.Level) {
Expand Down
4 changes: 4 additions & 0 deletions pkg/annotations/annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ const (
// HostProcessContainer indicates to launch a host process container (job container in this repository).
HostProcessContainer = "microsoft.com/hostprocess-container"

// DisableHostProcessContainer disables the ability to start a host process container (job container in this repository).
DisableHostProcessContainer = "microsoft.com/disable-hostprocess-container"

// HostProcessRootfsLocation indicates where the rootfs for a host process container should be located. If file binding support is
// available (Windows versions 20H1 and up) this will be the absolute path where the rootfs for a container will be located on the host
// and will be unique per container. On < 20H1 hosts, the location will be C:\<path-supplied>\<containerID>. So for example, if the value
Expand Down Expand Up @@ -322,5 +325,6 @@ var AnnotationExpansions = map[string][]string{
WCOWDisableGMSA,
DisableWritableFileShares,
VSMBNoDirectMap,
DisableHostProcessContainer,
},
}