rego and hardening: add enforcement and hardening for encrypted scratch #1538

anmaxvl · 2022-10-07T06:20:01Z

rego and hardening: add enforcement and hardening for encrypted scratch

The request to encrypt scratch comes from the host, but in confidential
scenario, the host cannot be trusted and it may attempt to mount the
read-write overlay of a container under an unencrypted path or omit the
encryption request altogether.

To address the issue, add scratch_mount, scratch_unmount enforcement
points and allow_unencrypted_scratch policy config, which can be used
to (you guessed it) allow containers to run with unencrypted scratch.

Since the request to add encrypted read-write scratch disk and mounting
container overlayfs come at different times, we also introduced minimal
(for now) hardening around adding read-write devices to the UVM. We
record the mounted read-write devices when they arrive and at the time
of adding overlayfs we check if the scratch path encrypted or not and
validate against the policy before enforcing overlay policy.

The policy config can be set at the top level, e.g.:

allow_unencrypted_scratch := true

containers := [...]

The scratch_mount enforcement point takes an input with the following
members:

{
    "target": "<mount target>",
    "encrypted": [true|false],
}

Add /internal/guest/runtime/hcsv2/uvm_state.go, which adds a new
hostMounts struct which keeps track of mounted RW devices. This can
be extended in the future for more GCS hardening purposes, e.g. overlay,
RO layer mounts and other container lifecycle management.

Signed-off-by: Maksim An maksiman@microsoft.com

anmaxvl · 2022-10-07T06:21:27Z

@SeanTAllen @matajoh @helsaawy putting as draft to get early feedback. The code hasn't been tested yet, working on the unit tests.

matajoh · 2022-10-07T08:38:10Z

internal/guest/runtime/hcsv2/uvm.go

-			if err != nil {
-				return errors.Wrapf(err, "unmounting scsi device at %s denied by policy", mvd.MountPath)
-			}
+			if mvd.ReadOnly {


Doesn't this logic change behavior? If ReadOnly and Encrypted is false, there is no policy enforcement.

I think we just missed this during review, but currently we don't have any enforcement for non-readonly devices: https://github.com/microsoft/hcsshim/blob/main/internal/guest/runtime/hcsv2/uvm.go#L657-L668.

pkg/securitypolicy/framework.rego

pkg/securitypolicy/regopolicy_test.go

pkg/securitypolicy/securitypolicyenforcer.go

internal/guest/runtime/hcsv2/uvm.go

internal/guest/storage/overlay/overlay.go

internal/guest/storage/scsi/scsi.go

SeanTAllen · 2022-10-07T14:57:09Z

This doesnt match my understanding of what we were going to be doing. Im going to set up a meeting.

anmaxvl · 2022-10-07T18:43:31Z

This doesnt match my understanding of what we were going to be doing. Im going to set up a meeting.

After discussing more, we came to a conclusion that in the current implementation we're doing more of a hardening inside the policy, instead we should do the hardening in GCS. The plan is to have a policy config that'll tell us whether running with unencrypted scratch is allowed or not and GCS will make sure that the overlays are mounted under encrypted mount points.

internal/guest/runtime/hcsv2/uvm.go

internal/guest/runtime/hcsv2/uvm_state.go

pkg/securitypolicy/regopolicy_scratch_test.go

helsaawy

Aside from merge conflicts and comments bellow, LGTM

internal/guest/runtime/hcsv2/uvm_state.go

internal/guest/runtime/hcsv2/uvm.go

helsaawy · 2022-10-13T21:35:20Z

internal/guest/runtime/hcsv2/uvm_state_test.go

+	"testing"
+)
+
+func Test_Add_Remove_RWDevice(t *testing.T) {


probably should add functional or gcs tests for this as well in a future pr

douglasmaciver · 2022-10-21T16:32:46Z

pkg/securitypolicy/securitypolicy_internal.go

-	AllowPropertiesAccess bool
-	AllowDumpStacks       bool
-	AllowRuntimeLogging   bool
+	Containers              []*securityPolicyContainer


Hi Maksim, the continued addition of bools to this function signature makes a good case for refactoring. I recommend a type or similar construct to avoid unreadable code at the function invocation:

code, err = marshalRego(
securityPolicy.AllowAll,
containers,
[]ExternalProcessConfig{},
true,
true,
true,
true,

yeah, we have "post feature complete" list of improvements and this one is on it 😄

douglasmaciver · 2022-10-21T16:43:03Z

pkg/securitypolicy/regopolicy_test.go

@@ -1968,6 +1968,55 @@ func Test_EnforceRuntimeLogging_Not_Allowed(t *testing.T) {
 	}
 }

+func Test_Rego_Scratch_Mount_Policy(t *testing.T) {


Kudos for your continued inclusion of test modules. Unit tests should be considered a requirement for all new code, and you consistently are a role model for this habit.

internal/guest/storage/scsi/scsi.go

pkg/securitypolicy/policy.rego

internal/guest/runtime/hcsv2/uvm_state.go

The request to encrypt scratch comes from the host, but in confidential scenario, the host cannot be trusted and it may attempt to mount the read-write overlay of a container under an unencrypted path or omit the encryption request altogether. To address the issue, add `scratch_mount`, `scratch_unmount` enforcement points and `allow_unencrypted_scratch` policy config, which can be used to (you guessed it) allow containers to run with unencrypted scratch. Since the request to add encrypted read-write scratch disk and mounting container overlayfs come at different times, we also introduced minimal (for now) hardening around adding read-write devices to the UVM. We record the mounted read-write devices when they arrive and at the time of adding overlayfs we check if the scratch path encrypted or not and validate against the policy before enforcing overlay policy. The policy config can be set at the top level, e.g.: ``` allow_unencrypted_scratch := true containers := [...] ``` The `scratch_mount` enforcement point takes an input with the following members: ``` { "target": "<mount target>", "encrypted": [true|false], } ``` Add `/internal/guest/runtime/hcsv2/uvm_state.go`, which adds a new `hostMounts` struct which keeps track of mounted RW devices. This can be extended in the future for more GCS hardening purposes, e.g. overlay, RO layer mounts and other container lifecycle management. Signed-off-by: Maksim An <maksiman@microsoft.com>

Signed-off-by: Maksim An <maksiman@microsoft.com>

As part of enforcing encrypted scratch policy, we need to make sure that the source matches. To figure out the source for a SCSI attachment we need to first find the actual controller number where the SCSI is presented, which can be different from what hcsshim has requested originally. Rename and export `fetchActualControllerNumber` as `ActualControllerNumber` and figure out the correct controller before calling to scsi Mount/Unmount. Remove wrappers and update unit tests. Signed-off-by: Maksim An <maksiman@microsoft.com>

Signed-off-by: Maksim An <maksiman@microsoft.com>

msscotb

lgtm

PR is for updating hcsshim ADO branch from github equivalent. Related work items: #1538, #1539, #1544, #1545, #1550, #1551, #1552, #1553, #1559, #1561

…ch (microsoft#1538) * rego and hardening: add enforcement and hardening for encrypted scratch The request to encrypt scratch comes from the host, but in confidential scenario, the host cannot be trusted and it may attempt to mount the read-write overlay of a container under an unencrypted path or omit the encryption request altogether. To address the issue, add `scratch_mount`, `scratch_unmount` enforcement points and `allow_unencrypted_scratch` policy config, which can be used to (you guessed it) allow containers to run with unencrypted scratch. Since the request to add encrypted read-write scratch disk and mounting container overlayfs come at different times, we also introduced minimal (for now) hardening around adding read-write devices to the UVM. We record the mounted read-write devices when they arrive and at the time of adding overlayfs we check if the scratch path encrypted or not and validate against the policy before enforcing overlay policy. The policy config can be set at the top level, e.g.: ``` allow_unencrypted_scratch := true containers := [...] ``` The `scratch_mount` enforcement point takes an input with the following members: ``` { "target": "<mount target>", "encrypted": [true|false], } ``` Add `/internal/guest/runtime/hcsv2/uvm_state.go`, which adds a new `hostMounts` struct which keeps track of mounted RW devices. This can be extended in the future for more GCS hardening purposes, e.g. overlay, RO layer mounts and other container lifecycle management. * crypt package refactor and adding more unit tests * tests: add e2e tests for scratch encryption policy * export and rename fetchActualControllerNumber As part of enforcing encrypted scratch policy, we need to make sure that the source matches. To figure out the source for a SCSI attachment we need to first find the actual controller number where the SCSI is presented, which can be different from what hcsshim has requested originally. Rename and export `fetchActualControllerNumber` as `ActualControllerNumber` and figure out the correct controller before calling to scsi Mount/Unmount. Remove wrappers and update unit tests. Signed-off-by: Maksim An <maksiman@microsoft.com>

anmaxvl force-pushed the encrypted-scratch-policy branch 2 times, most recently from 149326b to ec141d0 Compare October 7, 2022 07:01

matajoh reviewed Oct 7, 2022

View reviewed changes

SeanTAllen reviewed Oct 7, 2022

View reviewed changes

internal/guest/runtime/hcsv2/uvm.go Outdated Show resolved Hide resolved

SeanTAllen reviewed Oct 7, 2022

View reviewed changes

internal/guest/storage/overlay/overlay.go Outdated Show resolved Hide resolved

SeanTAllen reviewed Oct 7, 2022

View reviewed changes

internal/guest/storage/scsi/scsi.go Show resolved Hide resolved

anmaxvl force-pushed the encrypted-scratch-policy branch from ec141d0 to 0ef8783 Compare October 10, 2022 08:52

anmaxvl changed the title ~~policy enforcement: add encrypted scratch enforcement~~ rego and hardening: add enforcement and hardening for encrypted scratch Oct 10, 2022

anmaxvl force-pushed the encrypted-scratch-policy branch from 0ef8783 to 58eb832 Compare October 10, 2022 08:57

anmaxvl marked this pull request as ready for review October 10, 2022 09:15

anmaxvl requested a review from a team as a code owner October 10, 2022 09:15

matajoh reviewed Oct 10, 2022

View reviewed changes

internal/guest/runtime/hcsv2/uvm.go Show resolved Hide resolved

internal/guest/runtime/hcsv2/uvm_state.go Outdated Show resolved Hide resolved

pkg/securitypolicy/regopolicy_scratch_test.go Outdated Show resolved Hide resolved

anmaxvl force-pushed the encrypted-scratch-policy branch 2 times, most recently from 586492b to 776fe7a Compare October 10, 2022 20:24

msscotb assigned helsaawy and msscotb Oct 12, 2022

anmaxvl force-pushed the encrypted-scratch-policy branch 3 times, most recently from f557838 to 23cad7b Compare October 13, 2022 21:41

helsaawy requested changes Oct 14, 2022

View reviewed changes

anmaxvl force-pushed the encrypted-scratch-policy branch 2 times, most recently from dede668 to 6b8a53f Compare October 19, 2022 16:24

helsaawy approved these changes Oct 19, 2022

View reviewed changes

douglasmaciver reviewed Oct 21, 2022

View reviewed changes

msscotb reviewed Oct 22, 2022

View reviewed changes

internal/guest/storage/scsi/scsi.go Outdated Show resolved Hide resolved

pkg/securitypolicy/policy.rego Outdated Show resolved Hide resolved

anmaxvl force-pushed the encrypted-scratch-policy branch from da50fe6 to 62b7ec2 Compare October 25, 2022 00:13

anmaxvl force-pushed the encrypted-scratch-policy branch 3 times, most recently from 596f978 to 3407acb Compare October 26, 2022 07:25

helsaawy reviewed Oct 26, 2022

View reviewed changes

internal/guest/runtime/hcsv2/uvm_state.go Show resolved Hide resolved

msscotb reviewed Oct 27, 2022

View reviewed changes

internal/guest/runtime/hcsv2/uvm_state.go Outdated Show resolved Hide resolved

anmaxvl force-pushed the encrypted-scratch-policy branch from 3407acb to e5fc898 Compare November 2, 2022 23:21

anmaxvl added 10 commits November 3, 2022 14:49

pr feedback

a43b98b

Signed-off-by: Maksim An <maksiman@microsoft.com>

pr feedback: fix scratch_unmount and add test coverage

f5434f2

Signed-off-by: Maksim An <maksiman@microsoft.com>

crypt package refactor and adding more unit tests

0cceb28

Signed-off-by: Maksim An <maksiman@microsoft.com>

fix rego unit tests

790fbe3

Signed-off-by: Maksim An <maksiman@microsoft.com>

tests: add e2e tests for scratch encryption policy

497559c

Signed-off-by: Maksim An <maksiman@microsoft.com>

change to RWMutex

010821d

Signed-off-by: Maksim An <maksiman@microsoft.com>

pr feedback: nested unencrypted

a58642b

Signed-off-by: Maksim An <maksiman@microsoft.com>

add test for matching path

8b4f695

Signed-off-by: Maksim An <maksiman@microsoft.com>

anmaxvl force-pushed the encrypted-scratch-policy branch from e5fc898 to 8b4f695 Compare November 3, 2022 21:49

msscotb approved these changes Nov 5, 2022

View reviewed changes

anmaxvl merged commit 0a55db7 into microsoft:main Nov 6, 2022

anmaxvl deleted the encrypted-scratch-policy branch November 6, 2022 22:18

anmaxvl pushed a commit that referenced this pull request Feb 7, 2023

Merged PR 7142507: Update hcsshim ADO from github hcsshim

6fe31c3

PR is for updating hcsshim ADO branch from github equivalent. Related work items: #1538, #1539, #1544, #1545, #1550, #1551, #1552, #1553, #1559, #1561

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rego and hardening: add enforcement and hardening for encrypted scratch #1538

rego and hardening: add enforcement and hardening for encrypted scratch #1538

anmaxvl commented Oct 7, 2022 •

edited

Loading

anmaxvl commented Oct 7, 2022

matajoh Oct 7, 2022

anmaxvl Oct 7, 2022 •

edited

Loading

SeanTAllen commented Oct 7, 2022

anmaxvl commented Oct 7, 2022

helsaawy left a comment •

edited

Loading

helsaawy Oct 13, 2022

douglasmaciver Oct 21, 2022

anmaxvl Oct 21, 2022

douglasmaciver Oct 21, 2022

msscotb left a comment

rego and hardening: add enforcement and hardening for encrypted scratch #1538

rego and hardening: add enforcement and hardening for encrypted scratch #1538

Conversation

anmaxvl commented Oct 7, 2022 • edited Loading

anmaxvl commented Oct 7, 2022

matajoh Oct 7, 2022

Choose a reason for hiding this comment

anmaxvl Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

SeanTAllen commented Oct 7, 2022

anmaxvl commented Oct 7, 2022

helsaawy left a comment • edited Loading

Choose a reason for hiding this comment

helsaawy Oct 13, 2022

Choose a reason for hiding this comment

douglasmaciver Oct 21, 2022

Choose a reason for hiding this comment

anmaxvl Oct 21, 2022

Choose a reason for hiding this comment

douglasmaciver Oct 21, 2022

Choose a reason for hiding this comment

msscotb left a comment

Choose a reason for hiding this comment

anmaxvl commented Oct 7, 2022 •

edited

Loading

anmaxvl Oct 7, 2022 •

edited

Loading

helsaawy left a comment •

edited

Loading