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.

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

Add policy enforcement for shutting down a container #1518

Merged
merged 1 commit into from
Sep 22, 2022
Merged

Add policy enforcement for shutting down a container #1518

merged 1 commit into from
Sep 22, 2022

Conversation

matajoh
Copy link
Member

@matajoh matajoh commented Sep 19, 2022

Adds policy enforcement around shutting down a container. In the supplied framework, shutting down a container is always allowed. We update metadata based on the change in state that will be used by other framework rules. This enforcement point is important for custom policies for metadata tracking and also for allowing the creation of rules like "this container isn't allowed to shutdown if some other container is running".

This commit rewrites signalContainerV2 and gives it a new name. signalContainerV2 was only called from the kill and shutdown container functions. Despite only having 2 possible signals it could receive, it accepted any signal. There's a replacement method called signalContainerShutdownV2 that has "the same functionality" as signalContainerV2. signalContainerShutdownV2 doesn't accept arbitrary signals. Instead, it takes a boolean for whether the shutdown should be graceful (aka SIGTERM) or if we should do a non-graceful shutdown of SIGKILL.

This commit aims to keep things "as they are" except for changes that are required for proper and non-evadable policy enforcement.

Actual policy enforcement is in a new method ShutdownContainer on the Host type in uvm.go. It is our design goal to have all enforcement functions called from within methods in uvm.go.

There's an additional new method on the Host as well: SignalContainerProcess. SignalContainerProcess allows for the sending of arbitrary signals from the untrusted host computer to the UVM. The code has been extracted from signalProcessV2 in the bridge and moved onto the Host type. The move was required in order to add additional logic to SignalContainerProcess that signalProcessV2 lacked.

SignalContainerProcess will check to see if the process being signaled is the init process of the container. If it is and the signal is SIGTERM or SIGKILL then the shutdown container enforcement rule will be used. We create this "special case" as shutting down a container is the process of sending SIGTERM or SIGKILL to the container's init process. The information for whether a process is the init process for a container is available, but not from the module that bridge is part of, thus the move of functionality into Host.

The creation of SignalContainerProcess was going to be required for when we add support for enforcing policy around sending arbitrary signals to processes. The current SignalContainerProcess was written with that forthcoming changing in mind, but doesn't include any logic for the additional enforcement as that will be coming in a commit that will arrive "in the not so distant future".

Shutdown container policy, because it is always allowed by our framework, doesn't require changing the securitypolicy policy generation tool as there's no user provided input to the new policy rule shutdown_container.

When not using an open door policy, a "container not started" error will be returned if a shutdown is attempted on a container identifier that wasn't used to start a container.

@matajoh matajoh requested a review from a team as a code owner September 19, 2022 20:16
@matajoh
Copy link
Member Author

matajoh commented Sep 20, 2022

@anmaxvl @dcantah

Adds policy enforcement around shutting down a container. In the supplied framework, shutting down a container is always
allowed. We update metadata based on the change in state that will be used by other framework rules. This enforcement point
is important for custom policies for metadata tracking and also for allowing the creation of rules like "this container
isn't allowed to shutdown if some other container is running".

This commit rewrites `signalContainerV2` and gives it a new name. `signalContainerV2` was only called from the kill and
shutdown container functions. Despite only having 2 possible signals it could receive, it accepted any signal. There's a
replacement method called `signalContainerShutdownV2` that has "the same functionality" as `signalContainerV2`.
`signalContainerShutdownV2` doesn't accept arbitrary signals. Instead, it takes a boolean for whether the shutdown should be
graceful (aka `SIGTERM`) or if we should do a non-graceful shutdown of `SIGKILL`.

This commit aims to keep things "as they are" except for changes that are required for proper and non-evadable policy
enforcement.

Actual policy enforcement is in a new method `ShutdownContainer` on the `Host` type in `uvm.go`. It is our design goal to
have all enforcement functions called from within methods in `uvm.go`.

There's an additional new method on the `Host` as well: `SignalContainerProcess`. `SignalContainerProcess` allows for the sending
of arbitrary signals from the untrusted host computer to the UVM. The code has been extracted from `signalProcessV2` in the
bridge and moved onto the `Host` type. The move was required in order to add additional logic to `SignalContainerProcess` that
`signalProcessV2` lacked.

`SignalContainerProcess` will check to see if the process being signaled is the init process of the container. If it is and the signal
is `SIGTERM` or `SIGKILL` then the shutdown container enforcement rule will be used. We create this "special case" as shutting
down a container is the process of sending `SIGTERM` or `SIGKILL` to the container's init process. The information for whether
a process is the init process for a container is available, but not from the module that bridge is part of, thus the move of
functionality into `Host`.

The creation of `SignalContainerProcess` was going to be required for when we add support for enforcing policy around
sending arbitrary signals to processes. The current `SignalContainerProcess` was written with that forthcoming changing in
mind, but doesn't include any logic for the additional enforcement as that will be coming in a commit that will arrive "in
the not so distant future".

Shutdown container policy, because it is always allowed by our framework, doesn't require changing the `securitypolicy` policy
generation tool as there's no user provided input to the new policy rule `shutdown_container`.

When not using an open door policy, a "container not started" error will be returned if a shutdown is attempted on a container
identifier that wasn't used to start a container.
Signed-off-by: Sean T. Allen <seanallen@microsoft.com>
Signed-off-by: Matthew A Johnson <matjoh@microsoft.com>
@anmaxvl anmaxvl merged commit be9d388 into microsoft:main Sep 22, 2022
@anmaxvl anmaxvl deleted the shutdown-container-enforcement-2 branch September 22, 2022 01:21
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
Adds policy enforcement around shutting down a container. In the supplied framework, shutting down a container is always
allowed. We update metadata based on the change in state that will be used by other framework rules. This enforcement point
is important for custom policies for metadata tracking and also for allowing the creation of rules like "this container
isn't allowed to shutdown if some other container is running".

This commit rewrites `signalContainerV2` and gives it a new name. `signalContainerV2` was only called from the kill and
shutdown container functions. Despite only having 2 possible signals it could receive, it accepted any signal. There's a
replacement method called `signalContainerShutdownV2` that has "the same functionality" as `signalContainerV2`.
`signalContainerShutdownV2` doesn't accept arbitrary signals. Instead, it takes a boolean for whether the shutdown should be
graceful (aka `SIGTERM`) or if we should do a non-graceful shutdown of `SIGKILL`.

This commit aims to keep things "as they are" except for changes that are required for proper and non-evadable policy
enforcement.

Actual policy enforcement is in a new method `ShutdownContainer` on the `Host` type in `uvm.go`. It is our design goal to
have all enforcement functions called from within methods in `uvm.go`.

There's an additional new method on the `Host` as well: `SignalContainerProcess`. `SignalContainerProcess` allows for the sending
of arbitrary signals from the untrusted host computer to the UVM. The code has been extracted from `signalProcessV2` in the
bridge and moved onto the `Host` type. The move was required in order to add additional logic to `SignalContainerProcess` that
`signalProcessV2` lacked.

`SignalContainerProcess` will check to see if the process being signaled is the init process of the container. If it is and the signal
is `SIGTERM` or `SIGKILL` then the shutdown container enforcement rule will be used. We create this "special case" as shutting
down a container is the process of sending `SIGTERM` or `SIGKILL` to the container's init process. The information for whether
a process is the init process for a container is available, but not from the module that bridge is part of, thus the move of
functionality into `Host`.

The creation of `SignalContainerProcess` was going to be required for when we add support for enforcing policy around
sending arbitrary signals to processes. The current `SignalContainerProcess` was written with that forthcoming changing in
mind, but doesn't include any logic for the additional enforcement as that will be coming in a commit that will arrive "in
the not so distant future".

Shutdown container policy, because it is always allowed by our framework, doesn't require changing the `securitypolicy` policy
generation tool as there's no user provided input to the new policy rule `shutdown_container`.

When not using an open door policy, a "container not started" error will be returned if a shutdown is attempted on a container
identifier that wasn't used to start a container.
Signed-off-by: Sean T. Allen <seanallen@microsoft.com>
Signed-off-by: Matthew A Johnson <matjoh@microsoft.com>

Co-authored-by: Sean T. Allen <seanallen@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants