Implement image-rs basic features #4

arronwy · 2022-01-19T08:59:37Z

This PR implements image-rs basic features including OCI image download/decryption/decompression/unpack/mount.

Image signing verification is not included.

jiangliu · 2022-01-21T03:07:30Z

As we are early in the development stages, let's merge the PR so others may contribute to it? @sameo

stevenhorsman

I tried to check out the code and run the tests but hit some problems and had some questions.

stevenhorsman · 2022-01-21T10:27:09Z

Cargo.toml

+oci-distribution = { git = "https://github.com/arronwy/oci-distribution", branch = "export_pull_layer" }
+oci-spec = { git = "https://github.com/containers/oci-spec-rs" }
+ocicrypt-rs = { git = "https://github.com/arronwy/ocicrypt-rs", branch = "oci_distribution" }


Hey @arronwy - I notice that these dependencies are pulled from a branch of your fork of the projects, is that something you're planning to resolve soon by getting those changes merged upstream and then using the main versions?

Yes, there are two pending PR in oci-distribution to support manifest list and public pull layer API, after these two PRs are merged, we'll use the upstream repo.

Any progress about the oci-distribution upstream?

Upstream PR for image manifest list support have beed merged, they basicly agree public pull_layer and auth API, the PR should be merged soon, these two API may face refactoring when oci-distribution targeting 1.0 release.

stevenhorsman · 2022-01-21T10:42:25Z

src/snapshots/overlay.rs

+// SPDX-License-Identifier: Apache-2.0
+
+use anyhow::{anyhow, Result};
+use nix::mount::MsFlags;


When I run the cargo build on my mac I get:

unresolved import `nix::mount` --> src/snapshots/overlay.rs:6:10 | 6 | use nix::mount::MsFlags; | ^^^^^ could not find `mount` in `nix`

It may well be that mac (x86_64, not darwin) isn't supported, but I thought I'd let you know.

src/decrypt.rs

stevenhorsman · 2022-01-21T11:06:08Z

data/generate_test_data.md

@@ -0,0 +1,16 @@
+## Generate keypair and container image for unit and integration tests
+


I think it might be worth adding some more instructions here, for example getting the user to log into their container registry then set a shell variable to their user account. e.g. export cr_user="<your container reg account>
then updating the skopeo commands to replace user with cr_user.
I'm not sure if this is supposed to support more that just Docker Hub either?

Yes, we can add more detail info for the setup. It should support different registries, I tried with a local registry and Docker Hub, encrypted container image both works, but the server side may change the dest-compress-format.

stevenhorsman · 2022-01-21T11:08:26Z

data/generate_test_data.md

@@ -0,0 +1,16 @@
+## Generate keypair and container image for unit and integration tests


I wonder if it is worth linking this file on the main README.md as part of a set up guide as it's easy to miss?

Signed-off-by: Arron Wang <[email protected]>

arronwy · 2022-01-21T14:45:34Z

I tried to check out the code and run the tests but hit some problems and had some questions.

Hi @stevenhorsman, Thanks for your try, I just added the keyprovider config to the repo and the CI passed:
https://github.com/arronwy/image-rs/actions/runs/1729173936

BTW for test_pull_image since the final mount operation need root privilege, we need test with sudo command.

stevenhorsman · 2022-01-21T15:08:26Z

I tried to check out the code and run the tests but hit some problems and had some questions.

Hi @stevenhorsman, Thanks for your try, I just added the keyprovider config to the repo and the CI passed: https://github.com/arronwy/image-rs/actions/runs/1729173936

BTW for test_pull_image since the final mount operation need root privilege, we need test with sudo command.

Thanks for the info - and adding the GHA CI output link. I'm seeing better behaviour locally now, but still not quite right.
If I run the test_pull_image on it's own it works:

sudo -E PATH=$PATH -s cargo test image::tests::test_pull_image
    Finished test [unoptimized + debuginfo] target(s) in 0.19s
     Running unittests (/root/go/src/github.com/confidential-containers/image-rs/target/debug/deps/image_rs-9b1dcdc702e8e38c)

running 1 test
test image::tests::test_pull_image ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 8 filtered out; finished in 16.18s

but if I run all the tests it fails (differently from before):

sudo -E PATH=$PATH -s cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.18s
     Running unittests (/root/go/src/github.com/confidential-containers/image-rs/target/debug/deps/image_rs-9b1dcdc702e8e38c)

running 9 tests
test config::tests::test_image_config ... ok
test decoder::tests::test_uncompressed_decode ... ok
test config::tests::test_image_config_from_file ... ok
test decoder::tests::test_zstd_decode ... ok
test decoder::tests::test_gzip_decode ... ok
test bundle::tests::test_bundle_create_config ... ok
test unpack::tests::test_unpack ... ok
test pull::tests::test_pull_client ... ok
test image::tests::test_pull_image ... FAILED

failures:

---- image::tests::test_pull_image stdout ----
thread 'image::tests::test_pull_image' panicked at 'assertion failed: image_client.pull_image(image, bundle_dir.path(), &None, &None).await.is_ok()', src/image.rs:222:13

I tried single threading the tests to see if that helped, but it still failed, so it doesn't look like it was a synchronous issue.

It is super late for you, so I'm not expecting anything now, I just wanted to share what I saw. Thanks!

jiangliu · 2022-01-21T16:05:57Z

I tried to check out the code and run the tests but hit some problems and had some questions.

Hi @stevenhorsman, Thanks for your try, I just added the keyprovider config to the repo and the CI passed: https://github.com/arronwy/image-rs/actions/runs/1729173936
BTW for test_pull_image since the final mount operation need root privilege, we need test with sudo command.

Thanks for the info - and adding the GHA CI output link. I'm seeing better behaviour locally now, but still not quite right. If I run the test_pull_image on it's own it works:
sudo -E PATH=$PATH -s cargo test image::tests::test_pull_image
    Finished test [unoptimized + debuginfo] target(s) in 0.19s
     Running unittests (/root/go/src/github.com/confidential-containers/image-rs/target/debug/deps/image_rs-9b1dcdc702e8e38c)

running 1 test
test image::tests::test_pull_image ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 8 filtered out; finished in 16.18s
but if I run all the tests it fails (differently from before):
sudo -E PATH=$PATH -s cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.18s
     Running unittests (/root/go/src/github.com/confidential-containers/image-rs/target/debug/deps/image_rs-9b1dcdc702e8e38c)

running 9 tests
test config::tests::test_image_config ... ok
test decoder::tests::test_uncompressed_decode ... ok
test config::tests::test_image_config_from_file ... ok
test decoder::tests::test_zstd_decode ... ok
test decoder::tests::test_gzip_decode ... ok
test bundle::tests::test_bundle_create_config ... ok
test unpack::tests::test_unpack ... ok
test pull::tests::test_pull_client ... ok
test image::tests::test_pull_image ... FAILED

failures:

---- image::tests::test_pull_image stdout ----
thread 'image::tests::test_pull_image' panicked at 'assertion failed: image_client.pull_image(image, bundle_dir.path(), &None, &None).await.is_ok()', src/image.rs:222:13
I tried single threading the tests to see if that helped, but it still failed, so it doesn't look like it was a synchronous issue.

It is super late for you, so I'm not expecting anything now, I just wanted to share what I saw. Thanks!

Most code is still assuming Linux. We may need a plan to support other OS .

stevenhorsman · 2022-01-21T16:09:41Z

Most code is still assuming Linux. We may need a plan to support other OS .

Sorry, I wasn't clear - this result in on my Ubuntu 20.04 system, not macOS

arronwy · 2022-01-22T01:47:36Z

I tried single threading the tests to see if that helped, but it still failed, so it doesn't look like it was a synchronous issue.
It is super late for you, so I'm not expecting anything now, I just wanted to share what I saw. Thanks!

Most code is still assuming Linux. We may need a plan to support other OS .

Yes, we can add other OS support to the TODO list and add this info to the README

arronwy · 2022-01-22T02:09:06Z

Yes, we can add other OS support to the TODO list and add this info to the README

Most code is still assuming Linux. We may need a plan to support other OS .

Sorry, I wasn't clear - this result in on my Ubuntu 20.04 system, not macOS

The root cause maybe image-rs store the layers data to default work dir, during pull image it will check whether image layers data already downloaded, and remove_image API is not implemented yet:

const DEFAULT_WORK_DIR: &str = "/var/lib/image-rs/";

I updated to use tempdir for the work dir, it works with multiple times cargo test now.

jiangliu · 2022-01-22T02:15:24Z

Yes, we can add other OS support to the TODO list and add this info to the README

Most code is still assuming Linux. We may need a plan to support other OS .

Sorry, I wasn't clear - this result in on my Ubuntu 20.04 system, not macOS

The root cause maybe image-rs store the layers data to default work dir, during pull image it will check whether image layers data already downloaded, and remove_image API is not implemented yet:
const DEFAULT_WORK_DIR: &str = "/var/lib/image-rs/";
I updated to use tempdir for the work dir, it works with multiple times cargo test now.

It would be better to use tempfile crate to create tmpdir for tests, but be careful that the downloaded image may cost too much memory when tmpfs is used.

arronwy · 2022-01-22T05:51:43Z

The root cause maybe image-rs store the layers data to default work dir, during pull image it will check whether image layers data already downloaded, and remove_image API is not implemented yet:
const DEFAULT_WORK_DIR: &str = "/var/lib/image-rs/";
I updated to use tempdir for the work dir, it works with multiple times cargo test now.
It would be better to use tempfile crate to create tmpdir for tests, but be careful that the downloaded image may cost too much memory when tmpfs is used.

Yes, we use tempfile crate for all file related tests now. Thanks for the reminder, we use small size container image like busybox for current test cases. The following benchmark test will use bigger container image, we'll ensure it works with current CI.

stevenhorsman · 2022-01-24T09:23:37Z

Yes, we use tempfile crate for all file related tests now. Thanks for the reminder, we use small size container image like busybox for current test cases. The following benchmark test will use bigger container image, we'll ensure it works with current CI.

Thanks for the changed. Just to confirm that I can run sudo -E PATH=$PATH -s cargo test multiple times now and the tests all pass! 👍

src/snapshots/overlay.rs

src/unpack.rs

fitzthum

General comments. Looks good overall.

fitzthum · 2022-01-28T16:29:31Z

src/pull.rs

+                let mut media_type_str: &str = layer.media_type.as_str();
+
+                let mut decryptor = Decryptor::default();
+                if decryptor.is_encrypted(&layer.media_type) {


One of the issues we ran into with skopeo was that if a decryption key was provided but the image was not encrypted, the image would be unpacked anyway. For CCv0 this meant that if the CSP intercepted an encrypted image and replaced it with an unencrypted image, skopeo would unpack the unencrypted image inside the enclave without warning the user.

Does image-rs do something similar? Here it looks like the format of the image determines whether the image is decrypted or not. Do we check whether or not encryption is expected from by the client?

Yes, current PR for image-rs do the same way as skopeo, and after we pulled the image, we will return the image and layer metadata info like whether the image is signed and layer is encrypted.

The next step work for integrating signature crate which also provide the policy info, we will do these verifications to ensure the image/layer info is comply with the policy.

Ok, let's just make sure we enforce the guest owner's policy at some point, even for unsigned images.

data/ocicrypt_keyprovider.conf

dubek

small comment:

data/private_key.pem

jodh-intel

Thanks @arronwy. A few comments...

jodh-intel · 2022-02-10T12:49:08Z

.github/workflows/ci.yml

+          command: build
+          args: --all-features
+
+      - name: Run cargo test with root privilege


Why is root needed?

The snapshots module will do mount operation which require the root privilege.

I'd add a comment to make that clear. Also, is it worth running the unit tests twice as we do in Kata, once as root and once as non-root? That would allow you to assert that the mount tests (correctly) failed when not running as root for example.

src/bundle.rs

jodh-intel · 2022-02-10T13:33:17Z

src/config.rs

+#[derive(Clone, Debug, Deserialize)]
+pub struct ImageConfig {
+    /// The location for `image-rs` to store data.
+    pub work_dir: PathBuf,


Should this path exist or will it be created?

Should it be an absolute path or can it be relative?

Are sym links allowed?

work_dir will be created if not existed, it can be relative or sym links, we did not enforce any policy or check for this directory.

jodh-intel · 2022-02-10T13:33:37Z

src/config.rs

+    // Construct a default instance of `ImageConfig`
+    fn default() -> ImageConfig {
+        let work_dir = PathBuf::from(
+            std::env::var("IMAGE_WORK_DIR").unwrap_or_else(|_| DEFAULT_WORK_DIR.to_string()),


Why is this needed / who uses this variable? Is it for testing / debug only?

This looks potentially dangerous as no validation is being done on this env var value.

The name of the variable is rather generic. If this is really needed what about prefixing with CC_ or CONFIDENTIAL_CONTAINERS_?

This variable allow the user to easy set the work dir without create the config file.
Prefix with CC_ will be much better.

src/config.rs

src/unpack.rs

sameo

Most of my comments are minor. This PR is very well laid out and easy to review, thanks a lot
@arronwy.

One overall comment: Could you please add some more detailed commit messages?

src/config.rs

src/decoder/mod.rs

src/unpack.rs

src/decrypt.rs

src/meta_store.rs

sameo · 2022-02-10T17:14:52Z

src/meta_store.rs

+            .map_err(|e| anyhow!("failed to open metastore file {}", e.to_string()))?;
+        serde_json::from_reader::<File, MetaStore>(file)
+            .map_err(|e| anyhow!("failed to parse metastore file {}", e.to_string()))
+    }


How do we store/serialize the store?

This will be in to todo list since current API only cover pull_image API.

src/image.rs

sameo · 2022-02-10T17:21:30Z

src/image.rs

+        image: &str,
+        bundle_dir: &Path,
+        auth_info: &Option<&str>,
+        decrypt_config: &Option<&str>,


Here again, this unstructured decrypt_config argument is very opaque. What kind of strings are we expecting here?

src/image.rs

Signed-off-by: Samuel Ortiz <[email protected]>

arronwy

Thanks @sameo @jodh-intel for your reviewing. I'll add more detailed commit message and resolve all your comments.

arronwy · 2022-02-11T05:27:04Z

.github/workflows/ci.yml

+          command: build
+          args: --all-features
+
+      - name: Run cargo test with root privilege


The snapshots module will do mount operation which require the root privilege.

src/bundle.rs

src/config.rs

src/unpack.rs

src/snapshots/mod.rs

src/image.rs

src/decrypt.rs

jodh-intel · 2022-02-11T10:03:48Z

src/unpack.rs

+    use tempfile;
+
+    #[test]
+    fn test_unpack() {


Given the comments about the current limitations of tar-rs, shouldn't this also test sym-links? Also, what about other file types like char+block devices? I'm just wondering if we need tests for those here or can we rely on tar-rs for those tests?

The reason we did not test sym-links is current tar-rs crate packing feature will not preserve the file time info, we may need a sepeate tool to crate the test file, I have add it as TODO for sym-links and other file types.

Current support gzip and zstd compression algorithm type. Signed-off-by: Arron Wang <[email protected]>

Unpack module will unpack contents of tarball to the destination path. Signed-off-by: Arron Wang <[email protected]>

src/decrypt.rs

sameo · 2022-02-16T21:43:36Z

@jodh-intel @stevenhorsman If you're ok with that new version, I think we could merge it.

The decrypt module will use https://github.com/containers/ocicrypt-rs project to parse/retrive decrypt key and decrypt the data. Signed-off-by: Arron Wang <[email protected]>

jodh-intel

Thanks @arronwy. A few nits and a small bug (I think)...

src/bundle.rs

src/config.rs

test_data/generate_test_data.md

Provide the API to create bundle runtime config file which mainly add the config from container images: https://github.com/opencontainers/image-spec/blob/main/specs-go/v1/config.go to the annotation part of container runtime spec: https://github.com/opencontainers/runtime-spec/blob/main/specs-go/config.go Signed-off-by: Arron Wang <[email protected]>

Current we implement overlay snapshot as the default type. Signed-off-by: Arron Wang <[email protected]>

Signed-off-by: Arron Wang <[email protected]>

pull module will export the API to pull container image manifest, config and layers. it utilize oci-distribution crate to handle the real download process. Signed-off-by: Arron Wang <[email protected]>

Signed-off-by: Arron Wang <[email protected]>

The image module will provide CRI image service compatiable API, currently we implemented pull_image API. The unit test will focuse on different container registries to ensure the compalitibility with remote registries. Signed-off-by: Arron Wang <[email protected]>

Currently we use a hashmap to store the container image related metadata info, we may switch to a light database if needed. Signed-off-by: Arron Wang <[email protected]>

jodh-intel

Thanks @arronwy.

lgtm

stevenhorsman reviewed Jan 21, 2022

View reviewed changes

ci: Add github actions for PR check

d76b52b

Signed-off-by: Arron Wang <[email protected]>

dubek reviewed Jan 27, 2022

View reviewed changes

src/snapshots/overlay.rs Outdated Show resolved Hide resolved

src/unpack.rs Show resolved Hide resolved

fitzthum reviewed Jan 28, 2022

View reviewed changes

dubek reviewed Jan 28, 2022

View reviewed changes

data/private_key.pem Outdated Show resolved Hide resolved

jodh-intel reviewed Feb 10, 2022

View reviewed changes

sameo reviewed Feb 10, 2022

View reviewed changes

gh: Create dco.yml

ff88ab4

Signed-off-by: Samuel Ortiz <[email protected]>

arronwy commented Feb 11, 2022

View reviewed changes

jodh-intel reviewed Feb 11, 2022

View reviewed changes

arronwy added 2 commits February 16, 2022 16:25

decoder: Add the decoder module

8f97b3c

Current support gzip and zstd compression algorithm type. Signed-off-by: Arron Wang <[email protected]>

unpack: Add the unpack module

dc64c28

Unpack module will unpack contents of tarball to the destination path. Signed-off-by: Arron Wang <[email protected]>

fitzthum approved these changes Feb 16, 2022

View reviewed changes

src/decrypt.rs Outdated Show resolved Hide resolved

sameo approved these changes Feb 16, 2022

View reviewed changes

decrypt: Add the decrypt module

f218b3a

The decrypt module will use https://github.com/containers/ocicrypt-rs project to parse/retrive decrypt key and decrypt the data. Signed-off-by: Arron Wang <[email protected]>

stevenhorsman approved these changes Feb 17, 2022

View reviewed changes

jodh-intel reviewed Feb 17, 2022

View reviewed changes

src/bundle.rs Outdated Show resolved Hide resolved

src/bundle.rs Show resolved Hide resolved

src/bundle.rs Show resolved Hide resolved

src/config.rs Outdated Show resolved Hide resolved

test_data/generate_test_data.md Outdated Show resolved Hide resolved

arronwy added 2 commits February 18, 2022 11:18

snapshots: Add the snapshots module

8492a2e

Current we implement overlay snapshot as the default type. Signed-off-by: Arron Wang <[email protected]>

arronwy added 5 commits February 18, 2022 11:19

test_data: Generate keypair and container image for tests

94df310

Signed-off-by: Arron Wang <[email protected]>

pull: Add the pull module

29a4835

pull module will export the API to pull container image manifest, config and layers. it utilize oci-distribution crate to handle the real download process. Signed-off-by: Arron Wang <[email protected]>

config: Add the config module

2ba9de3

Signed-off-by: Arron Wang <[email protected]>

meta_store: Add the meta_store module

f8d6eda

Currently we use a hashmap to store the container image related metadata info, we may switch to a light database if needed. Signed-off-by: Arron Wang <[email protected]>

jodh-intel approved these changes Feb 18, 2022

View reviewed changes

sameo merged commit 8dc4d9d into confidential-containers:main Feb 21, 2022

arronwy deleted the basic_features branch April 29, 2022 05:37

		@@ -0,0 +1,16 @@
		## Generate keypair and container image for unit and integration tests

Implement image-rs basic features #4

Implement image-rs basic features #4

Conversation

arronwy commented Jan 19, 2022

jiangliu commented Jan 21, 2022 • edited Loading

stevenhorsman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arronwy commented Jan 21, 2022

stevenhorsman commented Jan 21, 2022

jiangliu commented Jan 21, 2022

stevenhorsman commented Jan 21, 2022

arronwy commented Jan 22, 2022

arronwy commented Jan 22, 2022

jiangliu commented Jan 22, 2022

arronwy commented Jan 22, 2022 • edited Loading

stevenhorsman commented Jan 24, 2022

fitzthum left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubek left a comment

Choose a reason for hiding this comment

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arronwy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameo commented Feb 16, 2022

jodh-intel left a comment

Choose a reason for hiding this comment

jodh-intel left a comment

Choose a reason for hiding this comment

jiangliu commented Jan 21, 2022 •

edited

Loading

arronwy commented Jan 22, 2022 •

edited

Loading

fitzthum left a comment •

edited

Loading