Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

vc: make host shared path readonly #2713

Merged
merged 1 commit into from
Jun 3, 2020

Conversation

bergwolf
Copy link
Member

We need to make sure containers cannot modify host path unless it is explicitly shared to it. Right now we expose an additional top level shared directory to the guest and allow it to be modified. This is less ideal and can be enhanced by following method:

  1. create two directories for each sandbox:
    -. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/, a directory to hold all host/guest shared mounts
    -. /run/kata-containers/shared/sandboxes/$sbx_id/shared/, a host/guest shared directory (9pfs/virtiofs source dir)
  2. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ is bind mounted readonly to /run/kata-containers/shared/sandboxes/$sbx_id/shared/, so guest cannot modify it
  3. host-guest shared files/directories are mounted one-level under /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ and thus present to guest at one level under /run/kata-containers/shared/sandboxes/$sbx_id/shared/

Fixes: #2712

@bergwolf bergwolf force-pushed the ro-shared-dir branch 2 times, most recently from 2b5a529 to a2269c7 Compare May 30, 2020 16:35
@codecov
Copy link

codecov bot commented May 30, 2020

Codecov Report

Merging #2713 into master will increase coverage by 4.92%.
The diff coverage is 53.06%.

@@            Coverage Diff             @@
##           master    #2713      +/-   ##
==========================================
+ Coverage   45.57%   50.50%   +4.92%     
==========================================
  Files         118      118              
  Lines       17261    17291      +30     
==========================================
+ Hits         7867     8732     +865     
+ Misses       8517     7486    -1031     
- Partials      877     1073     +196     

@amshinde
Copy link
Member

amshinde commented Jun 1, 2020

@bergwolf CI seems to be failing. Can you take a look?

@jodh-intel
Copy link
Contributor

Yeah, it's slightly ironic 😄:

21:38:04 # time="2020-05-31 20:38:01.436721357Z" level=error msg="Container creation error: rpc error: code = Internal desc = Could not create destination mount point: /run/kata-containers/shared/containers/2aa56755a0a1442e543722b07b8c51e88e880856296cc0ee3b39bb9043e1c5a3: mkdir /run/kata-containers/shared/containers/2aa56755a0a1442e543722b07b8c51e88e880856296cc0ee3b39bb9043e1c5a3: read-only file system\n" file="oci/runtime_oci.go:210"

@bergwolf
Copy link
Member Author

bergwolf commented Jun 1, 2020

/test

@bergwolf
Copy link
Member Author

bergwolf commented Jun 1, 2020

Thanks @jodh-intel ! I wasn't expecting dm based rootfs to still use the shared fs path but it turns out that we do reply on shared fs path even if there is no file system to share.

We need to make sure containers cannot modify host path unless it is explicitly shared to it. Right now we expose an additional top level shared directory to the guest and allow it to be modified. This is less ideal and can be enhanced by following method:
1. create two directories for each sandbox:
  -. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/, a directory to hold all host/guest shared mounts
  -. /run/kata-containers/shared/sandboxes/$sbx_id/shared/, a host/guest shared directory (9pfs/virtiofs source dir)
2. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ is bind mounted readonly to /run/kata-containers/shared/sandboxes/$sbx_id/shared/, so guest cannot modify it
3. host-guest shared files/directories are mounted one-level under /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ and thus present to guest at one level under /run/kata-containers/shared/sandboxes/$sbx_id/shared/

Fixes: kata-containers#2712
Signed-off-by: Peng Tao <bergwolf@hyper.sh>
@bergwolf
Copy link
Member Author

bergwolf commented Jun 2, 2020

The two failed ci are unrelated and tracked at kata-containers/tests#2594 and kata-containers/tests#2592

@GabyCT
Copy link
Contributor

GabyCT commented Jun 2, 2020

@jcvenegas and @likebreath there is an issue with the ci of clh-k8s could you please take a look?

@likebreath
Copy link
Contributor

@GabyCT I believe the clh-k8s-crio is still work-in-progress, and we should be able to ignore its failing for now. @jcvenegas Please correct me if I am mistaken on it.

Copy link
Member

@gnawux gnawux left a comment

Choose a reason for hiding this comment

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

LGTM

@teawater teawater merged commit 7a4925f into kata-containers:master Jun 3, 2020
@bergwolf bergwolf deleted the ro-shared-dir branch June 3, 2020 02:21
// Ensure container mount destination exists
// TODO: remove dependency on shared fs path. shared fs is just one kind of storage sources.
// we should not always use shared fs path for all kinds of storage. Stead, all storage
// should be bind mounted to a tmpfs path for containers to use.

Choose a reason for hiding this comment

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

typo? "Instead"

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

host shared directory should be readonly
8 participants