Add new gcs hooks, add expected mounts to security policy #1258

anmaxvl · 2021-12-29T22:06:33Z

Introduce a new wait-paths binary, which polls file system
until requested paths are available or a timeout is reached.

Security policy has been updated to have ExpectedMounts entries,
which will be used in conjunction with "wait-paths" hook for
synchronization purposes.

Refactor oci-hook logic into its own internal package and update
existing code to use that package. Copy runc HookName and constants
definitions to break dependency on runc

Introduce ExpectedMounts as part of security policy language and
the logic to enforce the policy, which resolves the expected mounts
in the UVM and adds a wait-paths hook to the spec.

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

anmaxvl · 2022-01-04T21:23:28Z

@SeanTAllen @KenGordon @svolos FYI

cmd/hooks/wait-paths/main.go

internal/constants/constants.go

internal/guest/runtime/hcsv2/workload_container.go

internal/hooks/spec.go

pkg/securitypolicy/securitypolicy.go

internal/hcsoci/create.go

SeanTAllen · 2022-01-25T17:39:23Z

I checked this out a while ago, but forgot to comment that it looked good and I didnt see anything problematic with the approach.

cmd/hooks/wait-paths/main.go

internal/guestpath/paths.go

pkg/securitypolicy/securitypolicyenforcer.go

internal/hcsoci/resources_lcow.go

internal/hcsoci/hcsdoc_wcow.go

cmd/hooks/wait-paths/main.go

pkg/securitypolicy/securitypolicyenforcer.go

katiewasnothere · 2022-02-23T23:06:01Z

Do we have any tests for the expected mounts work in here?

anmaxvl · 2022-02-24T00:54:39Z

Do we have any tests for the expected mounts work in here?

If you're talking about CRI integration tests, then it's currently blocked on the integrity validation PRs. I am planning on adding the tests once those are merged in as a separate PR. I've done local testing though.

anmaxvl · 2022-03-03T22:13:16Z

@katiewasnothere PTAL when you get a chance.

pkg/securitypolicy/securitypolicyenforcer.go

Introduce a new `wait-paths` binary, which polls file system until requested paths are available or a timeout is reached. Security policy has been updated to have ExpectedMounts entries, which will be used in conjunction with "wait-paths" hook for synchronization purposes. Refactor oci-hook logic into its own internal package and update existing code to use that package. Copy runc HookName and constants definitions to break dependency on runc Introduce ExpectedMounts as part of security policy language and the logic to enforce the policy, which resolves the expected mounts in the UVM and adds a wait-paths hook to the spec. Signed-off-by: Maksim An <maksiman@microsoft.com>

Per pr feedback, only container paths under sandbox mounts are supported as wait-paths. The support for other 2 scenarios can be added as needed. Add positive and negative CRI tests. Signed-off-by: Maksim An <maksiman@microsoft.com>

test/cri-containerd/policy_test.go

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

katiewasnothere

lgtm

Prior to this change, GCS allowed only one CreateContainer operation at a time. This isn't an issue in general case, however this doesn't work properly with synchronization via OCI runtime hook. Synchronization via runtime hook was introduced in: microsoft#1258 It injects a CreateRuntime OCI hook, if security policy provides wait paths. This allows container-A to run after container-B, where container-B writes to an empty directory volume shared between the two containers to signal that it's done some setup container-A depends on. In general case, container-A can be started before container-B which results in a deadlock, because CreateContainer request holds a lock to a map, which keeps track of running containers. To resolve the issue, the code has been updated to do a more granular locking when reading/updating the containers map: - Add a new "Status" field to Container object, which can be either "Running" or "Creating". - Remove locking from CreateContainer function - Rework "GetContainer" to "GetRunningContainer", which returns the container object only when it's in "Running" state, otherwise either `gcserr.HrVmcomputeSystemNotFound` or `gcserr.HrVmcomputeInvalidState` error returned. - Add new "AddContainer(id, container)" method, which updates the containers map with new container instances. - Rework CreateContainer to initially add new container objects into the containers map and set the "Status" to "Creating" at the start of the function and set it to "Running" only when the container successfully starts. Changin "GetContainer" to "GetRunningContainer" seemed to be the least invasive change, which allows us to limit code changes in the affected places. If "GetContainer" is left unchanged, then handling of containers in status "Creating" needs to take place and this requires handling cases when (e.g.) a modification request is sent to a container which isn't yet running. Signed-off-by: Maksim An <maksiman@microsoft.com>

Prior to this change, GCS allowed only one CreateContainer operation at a time. This isn't an issue in general case, however this doesn't work properly with synchronization via OCI runtime hook. Synchronization via runtime hook was introduced in: microsoft#1258 It injects a CreateRuntime OCI hook, if security policy provides wait paths. This allows container-A to run after container-B, where container-B writes to an empty directory volume shared between the two containers to signal that it's done some setup container-A depends on. In general case, container-A can be started before container-B which results in a deadlock, because CreateContainer request holds a lock to a map, which keeps track of running containers. To resolve the issue, the code has been updated to do a more granular locking when reading/updating the containers map: - Add a new "Status" field to Container object, which can be either "Running" or "Creating". - Remove locking from CreateContainer function - Rework "GetContainer" to "GetRunningContainer", which returns the container object only when it's in "Running" state, otherwise either `gcserr.HrVmcomputeSystemNotFound` or `gcserr.HrVmcomputeInvalidState` error returned. - Add new "AddContainer(id, container)" function, which updates the containers map with new container instances. - Rework CreateContainer to initially add new container objects into the containers map and set the "Status" to "Creating" at the start of the function and set it to "Running" only when the container successfully starts. Changin "GetContainer" to "GetRunningContainer" seemed to be the least invasive change, which allows us to limit code changes in the affected places. If "GetContainer" is left unchanged, then handling of containers in status "Creating" needs to take place and this requires handling cases when (e.g.) a modification request is sent to a container which isn't yet running. Signed-off-by: Maksim An <maksiman@microsoft.com>

Prior to this change, GCS allowed only one CreateContainer operation at a time. This isn't an issue in general case, however this doesn't work properly with synchronization via OCI runtime hook. Synchronization via runtime hook was introduced in: microsoft#1258 It injects a CreateRuntime OCI hook, if security policy provides wait paths. This allows container-A to run after container-B, where container-B writes to an empty directory volume shared between the two containers to signal that it's done some setup container-A depends on. In general case, container-A can be started before container-B which results in a deadlock, because CreateContainer request holds a lock to a map, which keeps track of running containers. To resolve the issue, the code has been updated to do a more granular locking when reading/updating the containers map: - Add a new "Status" field to Container object, which can be either "Running" or "Creating". - Remove locking from CreateContainer function - Rework "GetContainer" to "GetRunningContainer", which returns the container object only when it's in "Running" state, otherwise either `gcserr.HrVmcomputeSystemNotFound` or `gcserr.HrVmcomputeInvalidState` error returned. - Add new "AddContainer(id, container)" function, which updates the containers map with new container instances. - Rework CreateContainer to initially add new container objects into the containers map and set the "Status" to "Creating" at the start of the function and set it to "Running" only when the container successfully starts. Reworking "GetContainer" to "GetRunningContainer" seemed to be the least invasive change, which allows us to limit updates in the affected places. If "GetContainer" is left unchanged, then handling of containers in status "Creating" needs to take place and this requires handling cases when (e.g.) a modification request is sent to a container which isn't yet running. Signed-off-by: Maksim An <maksiman@microsoft.com>

Prior to this change, GCS allowed only one CreateContainer operation at a time. This isn't an issue in general case, however this doesn't work properly with synchronization via OCI runtime hook. Synchronization via runtime hook was introduced in: #1258 It injects a `CreateRuntime` OCI hook, if security policy provides wait paths. This allows container-A to run after container-B, where container-B writes to an empty directory volume shared between the two containers to signal that it's done some setup container-A depends on. In general case, container-A can be started before container-B which results in a deadlock, because `CreateContainer` request holds a lock to a map, which keeps track of running containers. To resolve the issue, the code has been updated to do a more granular locking when reading/updating the containers map: - Add a new "status" field to Container object and atomic setter/getter, which can be either "Created" or "Creating". New `uint32` type alias and constants were added to represent the values (`containerCreated` and `containerCreating`) - Remove locking from `CreateContainer` function - Rework `GetContainer` to `GetCreatedContainer`, which returns the container object only when it's in `containerCreated` state, otherwise either `gcserr.HrVmcomputeSystemNotFound` or `gcserr.HrVmcomputeInvalidState` error returned. - Add new `AddContainer(id, container)` function, which updates the containers map with new container instances. - Rework `CreateContainer` to initially add new container objects into the containers map and set the "status" to `containerCreating` at the start of the function and set it to `containerCreated` only when the container is successfully created in runtime. Reworking `GetContainer` to `GetCreatedContainer` seemed to be the least invasive change, which allows us to limit updates in the affected places. If `GetContainer` is left unchanged, then handling of containers in status "Creating" needs to take place and this requires handling cases when (e.g.) a modification request is sent to a container which isn't yet running. Additionally update synchronization CRI tests to use go routines to properly reproduce the scenario. Signed-off-by: Maksim An <maksiman@microsoft.com>

Follow up PR to add tests for wait-paths after initial PR microsoft#1258 was merged. Signed-off-by: Maksim An <maksiman@microsoft.com>

Follow up PR to add tests for wait-paths after initial PR #1258 was merged. Signed-off-by: Maksim An <maksiman@microsoft.com>

Related work items: #1240, #1241, #1258, #1260, #1270, #1271, #1272, #1279, #1286, #1288, #1289, #1292, #1293, #1294, #1295, #1297, #1298, #1299, #1301, #1305, #1307, #1308, #1309, #1310, #1313, #1314, #1315, #1318, #1320, #1321, #1322, #1324, #1326, #1328, #1332, #1333, #1334, #1335, #1337

Sync ADO to 671ec23 Related work items: #1258, #1273, #1304, #1359, #1360, #1362, #1363, #1367, #1372, #1374, #1377, #1380, #1381, #1382, #1384, #1387, #1388, #1389, #1390, #1391, #1393, #1394, #1395, #1396, #1398, #1400, #1401, #1402, #1404, #1406

…1258) Introduce a new `wait-paths` binary, which polls file system until requested paths are available or a timeout is reached. Security policy has been updated to have `ExpectedMounts` entries, which will be used in conjunction with "wait-paths" hook for synchronization purposes. Refactor oci-hook logic into its own internal package and update existing code to use that package. Copy runc HookName and constants definitions to break dependency on runc Introduce `ExpectedMounts` as part of security policy language and the logic to enforce the policy, which resolves the expected mounts in the UVM and adds a wait-paths hook to the spec. Add positive and negative CRI tests. Signed-off-by: Maksim An <maksiman@microsoft.com>

…t#1355) Prior to this change, GCS allowed only one CreateContainer operation at a time. This isn't an issue in general case, however this doesn't work properly with synchronization via OCI runtime hook. Synchronization via runtime hook was introduced in: microsoft#1258 It injects a `CreateRuntime` OCI hook, if security policy provides wait paths. This allows container-A to run after container-B, where container-B writes to an empty directory volume shared between the two containers to signal that it's done some setup container-A depends on. In general case, container-A can be started before container-B which results in a deadlock, because `CreateContainer` request holds a lock to a map, which keeps track of running containers. To resolve the issue, the code has been updated to do a more granular locking when reading/updating the containers map: - Add a new "status" field to Container object and atomic setter/getter, which can be either "Created" or "Creating". New `uint32` type alias and constants were added to represent the values (`containerCreated` and `containerCreating`) - Remove locking from `CreateContainer` function - Rework `GetContainer` to `GetCreatedContainer`, which returns the container object only when it's in `containerCreated` state, otherwise either `gcserr.HrVmcomputeSystemNotFound` or `gcserr.HrVmcomputeInvalidState` error returned. - Add new `AddContainer(id, container)` function, which updates the containers map with new container instances. - Rework `CreateContainer` to initially add new container objects into the containers map and set the "status" to `containerCreating` at the start of the function and set it to `containerCreated` only when the container is successfully created in runtime. Reworking `GetContainer` to `GetCreatedContainer` seemed to be the least invasive change, which allows us to limit updates in the affected places. If `GetContainer` is left unchanged, then handling of containers in status "Creating" needs to take place and this requires handling cases when (e.g.) a modification request is sent to a container which isn't yet running. Additionally update synchronization CRI tests to use go routines to properly reproduce the scenario. Signed-off-by: Maksim An <maksiman@microsoft.com>

anmaxvl marked this pull request as ready for review December 30, 2021 18:50

anmaxvl requested a review from a team as a code owner December 30, 2021 18:50

katiewasnothere reviewed Jan 4, 2022

View reviewed changes

cmd/hooks/wait-paths/main.go Show resolved Hide resolved

katiewasnothere reviewed Jan 4, 2022

View reviewed changes

internal/constants/constants.go Outdated Show resolved Hide resolved

katiewasnothere self-assigned this Jan 4, 2022

katiewasnothere reviewed Jan 4, 2022

View reviewed changes

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

katiewasnothere reviewed Jan 4, 2022

View reviewed changes

internal/hooks/spec.go Show resolved Hide resolved

katiewasnothere reviewed Jan 4, 2022

View reviewed changes

pkg/securitypolicy/securitypolicy.go Show resolved Hide resolved

katiewasnothere reviewed Jan 4, 2022

View reviewed changes

pkg/securitypolicy/securitypolicy.go Show resolved Hide resolved

dcantah self-assigned this Jan 5, 2022

dcantah mentioned this pull request Jan 5, 2022

Fix ReadDMVeritySuperBlock function #1257

Merged

anmaxvl force-pushed the sync-hooks branch 2 times, most recently from 263fca3 to b8c51a7 Compare January 12, 2022 21:18

katiewasnothere reviewed Jan 12, 2022

View reviewed changes

internal/hcsoci/create.go Outdated Show resolved Hide resolved

anmaxvl force-pushed the sync-hooks branch from 46f07ba to 6da0f47 Compare January 13, 2022 00:29