Add support for OCI Image signing (spec v1.0) #158

Xynnn007 · 2022-11-12T11:44:18Z

Summary

This PR helps to implement OCI Image Signing due to #125.

But please notice that this PR only uses the OCI Spec v1.0 (instead of v1.1, which brings referrer mechinism)

Release Note

Added push related functions for ClientCapabilities and related impls
Fixed a bug when converting sigstore::ClientConfig into oci_distribution::ClientConfig
Fixed a bug when parsing a private key file RSA was not considered.
Added some Debug, ToString, Eq, PartialEq macros and functions to enums and structs of crypto mod.
Refactored SimpleSigning as a sub module for payloads
Added OCI Image signing support together with some unit tests and examples.
Changed signature field of SignatureLayer into an Option, which helps when we meet a SignatureLayer that is to be signed.
Added a switch of http/https when access a registry for examples/cosign/verify

Documentation

Please see the examples/cosign/sign/README.md for the guide of OCI Image Signing.

Xynnn007 · 2022-11-12T11:55:36Z

The Clippy CI error is not caused by this PR. It can be fixed in another PR.

lukehinds · 2022-11-12T17:04:40Z

awesome, will take a look early next week @Xynnn007 !

What's the reasoning behind 1.0 support over 1.1?

Xynnn007 · 2022-11-12T18:12:46Z

What's the reasoning behind 1.0 support over 1.1?

The reason may be three I think:

It is still in rc stage.
I did not find related spec in sigstore/cosign to apply cosign signing to v1.1 registry
upstream oci-distribution is not ready for referrers api of v1.1 spec

For first: #125 (comment).
I've read some of the specs in opencontainers/images and opencontainers/distribution-spec. I think that v1.1 spec is still in rc stage, as https://github.com/opencontainers/image-spec/releases shows.

For second, I checked code and spec in sigstore/cosign roughly, and did not find the guide that defines how we store signatures in an oci registry v1.1. If any please tell me the link please. If not, I think we should make a new spec to apply the in-coming registry if v1.1 spec with referrer API. And I'd like to join the discussion of the spec :-)

For third, https://github.com/krustlet/oci-distribution still needs to be developed.

Besides, current implementation may have extensibility to support v2 spec, s.t. might not need much refactor.

lukehinds · 2022-11-13T13:14:42Z

makes sense.

@imjasonh is a good source for OCI specs

Xynnn007 · 2022-11-13T13:42:29Z

makes sense.

@imjasonh is a good source for OCI specs

Thanks for introducing!

For second, I checked code and spec in sigstore/cosign roughly, and did not find the guide that defines how we store signatures in an oci registry v1.1. If any please tell me the link please. If not, I think we should make a new spec to apply the in-coming registry if v1.1 spec with referrer API. And I'd like to join the discussion of the spec :-)

Got it here sigstore/cosign#1397. But as @imjasonh mentioned, the work for golang version is going on upsteam google/go-containerregistry#1455. Maybe the work on the rust side should also start. Let me propose an issue first ;-).

FYI oras-project/rust-oci-client#51

flavio

Fantastic job! 👏

Overall LGTM, I left some minor comments

src/cosign/client.rs

src/cosign/constraint.rs

src/cosign/mod.rs

src/cosign/payload/simple_signing.rs

flavio · 2022-11-15T10:18:29Z

src/lib.rs

@@ -235,5 +235,4 @@ pub mod fulcio;
 pub mod oauth;
 pub mod registry;
 pub mod rekor;
-pub mod simple_signing;


reminder: because of this and other refactors the public API changed, hence we will have to do a minor release

Xynnn007 · 2022-11-16T05:44:32Z

BTW, this pr includes a new feature "test-registry", which is not triggered by CI now. Do we need to need to add a switch for this feature, or directly enable it in GitHub CI?

flavio

Thanks for the changes you've made!

src/cosign/mod.rs

flavio · 2022-11-16T08:59:25Z

BTW, this pr includes a new feature "test-registry", which is not triggered by CI now. Do we need to need to add a switch for this feature, or directly enable it in GitHub CI?

I would be fine making the test-trigger a default feature, like we're doing with oci-distribution

This would ensure these tests are run also inside of GitHub's CI

Before the From trait of sigstore::ClientConfig does not spicify the `protocol` field of the resulted oci_distribution::ClientConfig. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

This interface helps to convert a SigStoreKeyPair to SigStoreSigner due to the given SigningScheme Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Add Debug, Eq, PartialEq, ToString traits and macros for crypto structs and enums, which will is helpful for tests Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

push function helps to push image manifest, layers to the target registry Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

SimpleSigning is a kind of signed payload. To bring it out aims to support more potential different payload formats. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Xynnn007 · 2022-11-16T10:51:36Z

BTW, this pr includes a new feature "test-registry", which is not triggered by CI now. Do we need to need to add a switch for this feature, or directly enable it in GitHub CI?

I would be fine making the test-trigger a default feature, like we're doing with oci-distribution

This would ensure these tests are run also inside of GitHub's CI

Agreed, and have fixed it

Xynnn007 · 2022-11-16T11:01:57Z

Another significant problem I meet:
Given that upstream oci_distribution has not support OCI Image Configuration, I use an empty json here to fill the Configuration. https://github.com/sigstore/sigstore-rs/pull/158/files#diff-3974fb3df6df5c9e9da01fb27e58de8f8681f4c14898cf0d4dfe2e08e638f9d0R34

I have tried with test registry, but no error had occurred. I wonder whether it is ok now?

And we surely need to fix it then, though.

cc @imjasonh

flavio

Thanks a lot for having incorporated all the feedback.

LGTM

lukehinds · 2022-11-18T10:33:49Z

This looks good to me, but I am not able to test this as registries typically need auth

Do we need to code in auth mechanisms in this or a follow up patch?

docker login
Authenticating with existing credentials...
Login Succeeded

cargo run --example sign -- \
    --key cosign.key \
    --image lukehinds/cosign-test:latest \
    --signing-scheme ECDSA_P256_SHA256_ASN1 \
    --password p6ssw0rd \
    --http \
    --annotations a=1

Image signing failed: Cannot fetch manifest of docker.io/lukehinds/cosign-test:latest: Not authorized: url http://registry-1.docker.io/v2/lukehinds/cosign-test/manifests/latest

Xynnn007 · 2022-11-18T16:39:17Z

Do we need to code in auth mechanisms in this or a follow up patch?

I can add something to support BasicAuth in this pr.

For upstream oci_distribution does not support identity token now, we cannot automatically use identity token in ~/.docker/config.json. We can promote it in the future, when the upstream supports identity token.

Xynnn007 · 2022-11-19T12:35:03Z

Hi @lukehinds I've added some code to retrieve credential to access a registry, which will help the command that you'd tried. Still, is it proper to include in this PR? If not, I can split it into another.

src/registry/auth.rs

Cargo.toml

Close sigstore#125 Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Now use --http parameter can let the verification use HTTP instead of HTTPS. This is helpful when doing tests with local registry. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

flavio

LGTM. I would like to merge it

flavio · 2022-11-24T09:44:19Z

Cargo.toml

@@ -57,18 +59,24 @@ anyhow = { version = "1.0", features = ["backtrace"] }
 assert-json-diff = "2.0.2"
 chrono = "0.4.20"
 clap = { version = "4.0.8", features = ["derive"] }
+docker_credential = { git = "https://github.com/Xynnn007/docker_credential", rev = "f350805"}
 openssl = "0.10.38"
 rstest = "0.15.0"
 tempfile = "3.3.0"


Please file a PR against the upstream project.

@danbev

Fixes ===== * Fix typo in cosign/mod.rs doc comment by @danbev in sigstore#148 * Fix typo in KeyPair trait doc comment by @danbev in sigstore#149 * Update cached requirement from 0.39.0 to 0.40.0 by @dependabot in sigstore#154 * Fix typos in PublicKeyVerifier doc comments by @danbev in sigstore#155 * Fix: CI error for auto deref by @Xynnn007 in sigstore#160 * Fix typo and grammar in signature_layers.rs by @danbev in sigstore#161 * Remove unused imports in examples/rekor by @danbev in sigstore#162 * Update link to verification example by @danbev in sigstore#156 * Fix typos in from_encrypted_pem doc comments by @danbev in sigstore#164 * Fix typos in doc comments by @danbev in sigstore#163 * Update path to fulcio-cert in verify example by @danbev in sigstore#168 Enhancements ============ * Add getter functions for LogEntry fields by @lkatalin in sigstore#147 * Add TreeSize alias to Rekor by @avery-blanchard in sigstore#151 * Updates for parsing hashedrekord LogEntry by @lkatalin in sigstore#152 * Add certificate based verification by @flavio in sigstore#159 * Add support for OCI Image signing (spec v1.0) by @Xynnn007 in sigstore#158 Contributors ============ * Avery Blanchard (@avery-blanchardmade) * Daniel Bevenius (@danbev) * Flavio Castelli (@flavio) * Lily Sturmann (@lkatalin) * Xynnn (@Xynnn007) Signed-off-by: Flavio Castelli <fcastelli@suse.com>

Xynnn007 force-pushed the feat-cosign-signing branch from 882f8e1 to 76edbb2 Compare November 12, 2022 11:45

flavio reviewed Nov 15, 2022

View reviewed changes

Xynnn007 force-pushed the feat-cosign-signing branch 2 times, most recently from c81113e to 7e7d203 Compare November 16, 2022 04:11

flavio reviewed Nov 16, 2022

View reviewed changes

src/cosign/mod.rs Outdated Show resolved Hide resolved

Xynnn007 added 6 commits November 16, 2022 18:49

fix: From sigstore ClientConfig to oci_distribution ClientConfig

6a170a1

Before the From trait of sigstore::ClientConfig does not spicify the `protocol` field of the resulted oci_distribution::ClientConfig. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

feat: Add to_sigstore_signer to SigStoreKeyPair

a3e77b9

This interface helps to convert a SigStoreKeyPair to SigStoreSigner due to the given SigningScheme Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

fix: add RSA case when trying to parse a key pair

c0b1e2e

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

feat: Add useful macros for crypto module

b488af5

Add Debug, Eq, PartialEq, ToString traits and macros for crypto structs and enums, which will is helpful for tests Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

feat: add push trait func for oci_client

13ac0b6

push function helps to push image manifest, layers to the target registry Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

refactor: refactor SimpleSigning to a submodule

cefd239

SimpleSigning is a kind of signed payload. To bring it out aims to support more potential different payload formats. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Xynnn007 force-pushed the feat-cosign-signing branch from 7e7d203 to ff9df41 Compare November 16, 2022 10:49

flavio previously approved these changes Nov 16, 2022

View reviewed changes

Xynnn007 dismissed flavio’s stale review via 375ddd1 November 19, 2022 12:32

flavio reviewed Nov 24, 2022

View reviewed changes

src/registry/auth.rs Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

Xynnn007 force-pushed the feat-cosign-signing branch from 375ddd1 to 1c340dd Compare November 24, 2022 09:41

Xynnn007 added 2 commits November 24, 2022 17:43

feat: Add OCI Image signing support

6f1bd05

Close sigstore#125 Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

feat: add HTTP/HTTPS switch for verify example

36629c2

Now use --http parameter can let the verification use HTTP instead of HTTPS. This is helpful when doing tests with local registry. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Xynnn007 force-pushed the feat-cosign-signing branch from 1c340dd to 36629c2 Compare November 24, 2022 09:43

flavio approved these changes Nov 24, 2022

View reviewed changes

flavio merged commit 453b91a into sigstore:main Nov 24, 2022

flavio mentioned this pull request Nov 24, 2022

Tag v0.6.0 #169

Merged

Xynnn007 deleted the feat-cosign-signing branch November 25, 2022 02:12

Xynnn007 mentioned this pull request Feb 1, 2023

[RFC] Proposal for KBS Resource URL confidential-containers/documentation#85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for OCI Image signing (spec v1.0) #158

Add support for OCI Image signing (spec v1.0) #158

Xynnn007 commented Nov 12, 2022

Xynnn007 commented Nov 12, 2022

lukehinds commented Nov 12, 2022 •

edited

Loading

Xynnn007 commented Nov 12, 2022 •

edited

Loading

lukehinds commented Nov 13, 2022

Xynnn007 commented Nov 13, 2022

flavio left a comment

flavio Nov 15, 2022

Xynnn007 commented Nov 16, 2022

flavio left a comment

flavio commented Nov 16, 2022

Xynnn007 commented Nov 16, 2022

Xynnn007 commented Nov 16, 2022

flavio left a comment

lukehinds commented Nov 18, 2022 •

edited

Loading

Xynnn007 commented Nov 18, 2022 •

edited

Loading

Xynnn007 commented Nov 19, 2022

flavio left a comment

flavio Nov 24, 2022

Add support for OCI Image signing (spec v1.0) #158

Add support for OCI Image signing (spec v1.0) #158

Conversation

Xynnn007 commented Nov 12, 2022

Summary

Release Note

Documentation

Xynnn007 commented Nov 12, 2022

lukehinds commented Nov 12, 2022 • edited Loading

Xynnn007 commented Nov 12, 2022 • edited Loading

lukehinds commented Nov 13, 2022

Xynnn007 commented Nov 13, 2022

flavio left a comment

Choose a reason for hiding this comment

flavio Nov 15, 2022

Choose a reason for hiding this comment

Xynnn007 commented Nov 16, 2022

flavio left a comment

Choose a reason for hiding this comment

flavio commented Nov 16, 2022

Xynnn007 commented Nov 16, 2022

Xynnn007 commented Nov 16, 2022

flavio left a comment

Choose a reason for hiding this comment

lukehinds commented Nov 18, 2022 • edited Loading

Xynnn007 commented Nov 18, 2022 • edited Loading

Xynnn007 commented Nov 19, 2022

flavio left a comment

Choose a reason for hiding this comment

flavio Nov 24, 2022

Choose a reason for hiding this comment

lukehinds commented Nov 12, 2022 •

edited

Loading

Xynnn007 commented Nov 12, 2022 •

edited

Loading

lukehinds commented Nov 18, 2022 •

edited

Loading

Xynnn007 commented Nov 18, 2022 •

edited

Loading