image-rs: Support simple signing with X-R-S-S #372

mattarnoatibm · 2023-10-03T07:42:15Z

Support getting simple signing signatures from
registry that supports X-R-S-S extension.

Refactor into separate tests for XRSS and others.

Fixes: #12

Xynnn007

Thanks @mattarnoatibm for the contribution! Could you do some refactoring to the code for better maintainance in future?

Xynnn007 · 2023-10-07T01:48:48Z

image-rs/src/signature/mechanism/simple/mod.rs

+#[cfg(feature = "signature-simple")]
+use base64::Engine;
+
+#[cfg(feature = "signature-simple-xrss")]
+use oci_distribution::Reference;
+#[cfg(feature = "signature-simple-xrss")]
+use reqwest::Client;
+
+#[cfg(feature = "signature-simple-xrss")]
+use crate::signature::image::digest::Digest;
+


There are many #[cfg(feature = "signature-simple-xrss")] sentences in this file. If these feature-specific sentences can be put into a separate file, it can be better for reading and maintaining. We only need to mark the mod xrss; sentence with #[cfg(feature = "signature-simple-xrss")].

Hi @Xynnn007 I moved most of the code in simple/mod.rs that was behind the signature-simple-xrss feature to a new file, xrss.rs. I marked 2 sentences with #[cfg(feature = "signature-simple-xrss")] in addition to the mod xrss; sentence in mod.rs:

If the XRSS feature is used then crate::resource::get_resource(&config.sigstore_config).await can return an error, otherwise we cannot allow it to return an error, so I left if sig_store_config_file.is_err() { return Ok(()); } marked with #[cfg(feature = "signature-simple-xrss")].

If XRSS is not used then we don't want to create an xrss::RegistryClient and try to get signatures from the registry. So I created a code block where the xrss::RegistryClient is created and used to try and get signatures from the registry, and I marked that code block with #[cfg(feature = "signature-simple-xrss")].

arronwy · 2023-10-07T02:39:31Z

image-rs/src/signature/mechanism/simple/mod.rs

+        }
+    }
+
+    pub async fn get_signatures_from_registry(


maybe we can send the PR in parallel to https://github.com/krustlet/oci-distribution to extend the Client API to support pull signatures from registry

I'll take at look at the OCI Distribution library.

image-rs/src/signature/mechanism/simple/mod.rs

image-rs/src/signature/mechanism/simple/xrss.rs

image-rs/tests/signature_verification.rs

Xynnn007

One last step for the integration test from myside :-)

Xynnn007 · 2023-10-11T02:14:00Z

image-rs/tests/credential.rs

@@ -19,7 +19,7 @@ pub mod common;
 #[tokio::test]
 #[serial]
 async fn test_use_credential(#[case] image_ref: &str, #[case] auth_file_uri: &str) {
-    common::prepare_test().await;
+    common::prepare_test("aa-offline_fs_kbc-resources.json").await;


I think it would be better to declare this as a const, or we add the parameter to #[case] macro. How do you like?

Done. I went with adding some constants to common/mod.rs as the signature verification tests do not use the #[case] macro.

Xynnn007 · 2023-10-11T02:14:12Z

image-rs/tests/image_decryption.rs

@@ -30,7 +30,7 @@ const OCICRYPT_CONFIG: &str = "test_data/ocicrypt_keyprovider_ttrpc.conf";
 #[tokio::test]
 #[serial]
 async fn test_decrypt_layers(#[case] image: &str) {
-    common::prepare_test().await;
+    common::prepare_test("aa-offline_fs_kbc-resources.json").await;


I think it would be better to declare this as a const, or we add the parameter to #[case] macro. How do you like?

Xynnn007 · 2023-10-11T02:14:26Z

image-rs/tests/signature_verification.rs

@@ -93,13 +118,53 @@ const SIGSTORE_CONFIG_URI: &str = "kbs:///default/sigstore-config/test";
 #[tokio::test]
 #[serial]
 async fn signature_verification() {
-    common::prepare_test().await;
+    do_signature_verification_tests(&_TESTS, "aa-offline_fs_kbc-resources.json", &None).await;


I think it would be better to declare this as a const, or we add the parameter to #[case] macro. How do you like?

Xynnn007 · 2023-10-11T02:14:36Z

image-rs/tests/signature_verification.rs

+                let auth_info = &Some(auth.as_str());
+                do_signature_verification_tests(
+                    &_TESTS_XRSS,
+                    "aa-offline_fs_kbc-resources-for-icr.json",


I think it would be better to declare this as a const, or we add the parameter to #[case] macro. How do you like?

Support getting simple signing signatures from registry that supports X-R-S-S extension. Refactor into separate tests for XRSS and others. Fixes: confidential-containers#12 Signed-off-by: Matthew Arnold <[email protected]>

Xynnn007

lgtm. Thanks!

stevenhorsman · 2023-10-12T10:23:18Z

@arronwy - hey Arron, we are keen to try and get this feature into the 0.8 release before CCv0 gets archived, so are you okay with this being merged now as Matt has created oras-project/rust-oci-client#97 to get the opinions of the oci-distribution community about moving it to there, which we can do as a separate follow-up activity? Thanks!

arronwy · 2023-10-13T01:22:48Z

@arronwy - hey Arron, we are keen to try and get this feature into the 0.8 release before CCv0 gets archived, so are you okay with this being merged now as Matt has created krustlet/oci-distribution#97 to get the opinions of the oci-distribution community about moving it to there, which we can do as a separate follow-up activity? Thanks!

sure, I though we already code freeze for v0.8.0 and merge after the release, it's ok from my side to merge for v0.8.0

arronwy

Thanks @mattarnoatibm

mattarnoatibm requested review from sameo, jiangliu, arronwy, lumjjb, jialez0 and Xynnn007 as code owners October 3, 2023 07:42

mattarnoatibm force-pushed the simple-signing-xrss branch 3 times, most recently from 0625429 to 92c94fa Compare October 3, 2023 12:25

Xynnn007 reviewed Oct 7, 2023

View reviewed changes

arronwy reviewed Oct 7, 2023

View reviewed changes

mattarnoatibm force-pushed the simple-signing-xrss branch from 55b8d5b to 4ea27d6 Compare October 9, 2023 10:42

Xynnn007 reviewed Oct 10, 2023

View reviewed changes

mattarnoatibm force-pushed the simple-signing-xrss branch 2 times, most recently from a731302 to c28adf0 Compare October 10, 2023 14:57

Xynnn007 reviewed Oct 11, 2023

View reviewed changes

image-rs: Support simple signing with X-R-S-S

43227e4

Support getting simple signing signatures from registry that supports X-R-S-S extension. Refactor into separate tests for XRSS and others. Fixes: confidential-containers#12 Signed-off-by: Matthew Arnold <[email protected]>

mattarnoatibm force-pushed the simple-signing-xrss branch from 1812db1 to 43227e4 Compare October 11, 2023 10:02

Xynnn007 approved these changes Oct 11, 2023

View reviewed changes

arronwy approved these changes Oct 13, 2023

View reviewed changes

arronwy merged commit 99694c7 into confidential-containers:main Oct 13, 2023
6 checks passed

mattarnoatibm deleted the simple-signing-xrss branch October 13, 2023 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image-rs: Support simple signing with X-R-S-S #372

image-rs: Support simple signing with X-R-S-S #372

mattarnoatibm commented Oct 3, 2023

Xynnn007 left a comment

Xynnn007 Oct 7, 2023

mattarnoatibm Oct 9, 2023

arronwy Oct 7, 2023

mattarnoatibm Oct 9, 2023

Xynnn007 left a comment

Xynnn007 Oct 11, 2023

mattarnoatibm Oct 11, 2023

Xynnn007 Oct 11, 2023

Xynnn007 Oct 11, 2023

Xynnn007 Oct 11, 2023

Xynnn007 left a comment

stevenhorsman commented Oct 12, 2023

arronwy commented Oct 13, 2023

arronwy left a comment

image-rs: Support simple signing with X-R-S-S #372

image-rs: Support simple signing with X-R-S-S #372

Conversation

mattarnoatibm commented Oct 3, 2023

Xynnn007 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

Xynnn007 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

Xynnn007 left a comment

Choose a reason for hiding this comment

stevenhorsman commented Oct 12, 2023

arronwy commented Oct 13, 2023

arronwy left a comment

Choose a reason for hiding this comment