Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed ring dependency #127

Merged
merged 2 commits into from
Oct 7, 2022
Merged

Conversation

Xynnn007
Copy link
Member

@Xynnn007 Xynnn007 commented Sep 13, 2022

Summary

Due to #27. This pr removed ring crate, and use RustCrypto crates instead.

Release Note

  • replaced ring crate with RustCrypto
  • refactored CosignVerificationKey implementation and interface
  • fixed the code which references CosignVerificationKey
  • added RSA keys and signing algorithms support (RSA PSS & RSA PKCS#1 v1.5)
API Changes
  • Changed all the from_XXX functions for CosignVerificationKey's SignatureDigestAlgorithm parameter into SigningScheme
  • Deleted the enum SignatureDigestAlgorithm
  • Replace struct CosignVerificationKey into enum.

Documentation

@Xynnn007 Xynnn007 force-pushed the refactor-remove-ring branch 2 times, most recently from fe13148 to 553f90e Compare September 13, 2022 08:46
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

Overall I like the change.

There are however two main things that have to be addressed:

  • Be able to use RSA keys to verify signatures
  • Given a PEM encoded key, have the code automatically figure out the encryption and signing algorithm. Or provide the user a helper function that does that

@@ -399,16 +398,16 @@ mod tests {
let pubkey = key
.public_key_to_pem()
.expect("export private key to PEM format failed.");
println!("{}", pubkey);
Copy link
Member

Choose a reason for hiding this comment

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

nit, probably a leftover?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, sry for that. It was used for test.

verification_algorithm,
data,
})
Self::from_der(key_pem.contents.as_slice(), signing_scheme)
Copy link
Member

Choose a reason for hiding this comment

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

I miss the usability of the old implementation. Now, as a user I would have to specify the signing scheme when importing the key. The signing scheme is made by encryption algorithm + digest algorithm.

The old code leveraged a functionality of ring that provided back the type of encryption algorithm being used; see lines 69-95.

Thanks to that, the verify example was able to handle different kind of public keys without having the user to specify the encryption type.

Moreover, the current code lost the ability to handle RSA keys. I happen to have some container images that have been signed with a RSA key. This change breaks their verification.

In case you are interested:

curl -O https://ftp.suse.com/pub/projects/security/keys/container–key.pem
cargo run --example verify -- -k ./container–key.pem registry.suse.com/suse/sle-micro/5.0/toolbox

This will fail with the following error:

2022-09-13T12:21:07.095762Z  INFO sigstore::cosign::client_builder: Rekor public key not provided. Rekor integration disabled
2022-09-13T12:21:07.095804Z  INFO sigstore::cosign::client_builder: No Fulcio cert has been provided. Fulcio integration disabled
Image verification failed: Cannot create public key verifier: Pkcs8 spki error : Ecdsa-P256 from der bytes to public key failed: unknown/unsupported algorithm OID: 1.2.840.10045.2.1

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, the next release of the rsa crate will include some pretty major overhauls of signature support, including dedicated RSASSA-PSS and RSASSA-PKCS#1v1.5 signing/verifying keys which use the traits from the signature crate and hopefully support for things like parsing PSS params via PKCS#1/PKCS#8.

The current low-level signing APIs will also continue to work with some minor changes as well.

Copy link
Member Author

@Xynnn007 Xynnn007 Sep 13, 2022

Choose a reason for hiding this comment

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

Thanks for your example, it will be very helpful.

I miss the usability of the old implementation. Now, as a user I would have to specify the signing scheme when importing the key. The signing scheme is made by encryption algorithm + digest algorithm.

The old code leveraged a functionality of ring that provided back the type of encryption algorithm being used; see lines 69-95.

In fact, the padding scheme used in the old implementation of RSA-related schemes only included PSS, so the API could only receive one parameter DigestAlgorithm. There are other padding schemes, like OAEP, PKCS#1v1.5. Thus EC keys may not share the same from_pem/der API with RSA keys.

Also, the public key file usually only contains information of the key itself. For example an EC public key only contains information of its type (ec public key, by OID) and curve (by OID), but not its signing usage.

So I think it is hard to do that, (TBH, I think the old implementation for the API is not reasonable)

Moreover, the current code lost the ability to handle RSA keys. I happen to have some container images that have been signed with a RSA key. This change breaks their verification.

Maybe we can delay this pr. When the RSA key-interface is done, I'll rebase and go on doing this.

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW, the next release of the rsa crate will include some pretty major overhauls of signature support, including dedicated RSASSA-PSS and RSASSA-PKCS#1v1.5 signing/verifying keys which use the traits from the signature crate and hopefully support for things like parsing PSS params via PKCS#1/PKCS#8.

The current low-level signing APIs will also continue to work with some minor changes as well.

Thanks for the information! I wonder when the next release will be published. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

We're trying to wrap it up soon. This is probably the last major PR we'd like to get merged first prior to another release: RustCrypto/RSA#183

Copy link
Member Author

@Xynnn007 Xynnn007 Sep 14, 2022

Choose a reason for hiding this comment

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

Ok, I got your idea about the developer convenience. I added two interfaces try_from_der() and try_from_pem(). The two interfaces receive only one parameter data: &[u8], and will decide the verification algorithm (digest, padding ,...) with a default value. For example, by default ECDSA_P256_SHA256_ASN1 for a ec-p256 public key and RSA_PSS_2048_8192_SHA256 for a RSA public key (in future)
Does this make sense?

  • Go ahead with the SignatureDigestAlgorithm refactoring, they are pretty nice

What do you mean? Should SignatureDigestAlgorithm be kept or deleted? Sry I can not understand this ;)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I got your idea about the developer convenience. I added two interfaces try_from_der() and try_from_pem(). The two interfaces receive only one parameter data: &[u8], and will decide the verification algorithm (digest, padding ,...) with a default value. For example, by default ECDSA_P256_SHA256_ASN1 for a ec-p256 public key and RSA_PSS_2048_8192_SHA256 for a RSA public key (in future)
Does this make sense?

Yes, it does make sense. I like the UX

What do you mean? Should SignatureDigestAlgorithm be kept or deleted? Sry I can not understand this ;)

My fault, I like the cleaning/refactoring done by this PR. That's the way to go 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the PR. If possible, I would like RSA support to be in before merging it. Otherwise, we can merge it in the main branch, but not tag a new release until this is added back.

Thanks. Let me finish RSA support in this PR then.

Copy link
Contributor

@tarcieri tarcieri Sep 16, 2022

Choose a reason for hiding this comment

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

New rsa crate prerelease here:

https://docs.rs/rsa/0.7.0-rc.0/

It includes new PSS-specific SigningKey/VerifyingKey types:

https://docs.rs/rsa/0.7.0-rc.0/rsa/pss/struct.SigningKey.html

It'd be good to give us feedback on anything that might require a breaking change soon, as we intend to cut a final release in the next week or so.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, for support for PKCS#1/PKCS#8 PSS keys, this is a tracking issue specifically for supporting them:

RustCrypto/RSA#177

@Xynnn007 Xynnn007 force-pushed the refactor-remove-ring branch 3 times, most recently from b160821 to 76b1b9d Compare September 14, 2022 02:33
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

I like the PR. If possible, I would like RSA support to be in before merging it. Otherwise, we can merge it in the main branch, but not tag a new release until this is added back.

verification_algorithm,
data,
})
Self::from_der(key_pem.contents.as_slice(), signing_scheme)
Copy link
Member

Choose a reason for hiding this comment

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

Ok, I got your idea about the developer convenience. I added two interfaces try_from_der() and try_from_pem(). The two interfaces receive only one parameter data: &[u8], and will decide the verification algorithm (digest, padding ,...) with a default value. For example, by default ECDSA_P256_SHA256_ASN1 for a ec-p256 public key and RSA_PSS_2048_8192_SHA256 for a RSA public key (in future)
Does this make sense?

Yes, it does make sense. I like the UX

What do you mean? Should SignatureDigestAlgorithm be kept or deleted? Sry I can not understand this ;)

My fault, I like the cleaning/refactoring done by this PR. That's the way to go 👍

@Xynnn007
Copy link
Member Author

Please do not review the newly push-ed commit now, because it uses a modified version of rsa crate. I've made a PR for the change. After the PR is merged I'll change this commit.

Copy link
Member

@lukehinds lukehinds left a comment

Choose a reason for hiding this comment

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

great work thanks @Xynnn007

@Xynnn007
Copy link
Member Author

Xynnn007 commented Sep 19, 2022

great work thanks @Xynnn007

Hi @lukehinds Thanks for your approval.

However there is some significant things to clarify. This pr now involves non-official crypto crate version and must not be merged until the upstream of rsa merges the pr (The current version does not provide Clone and Debug trait implementation for signing keys).

I pushed the second commit to collect suggestions about the rsa key length. There are two design schemes:

  • Add a member usize for RSA-related signing schemes enums to specify the key length (which is implemented in the commit). It is convenient to generate new keypair, but may seem weird when call RSAKeyPair::to_verification_key() to carry a usize.
  • Another choice is to set a default value of the key size when generating the RSAKeyPair (like 2048 or 4096 bits). This may sacrifice the flexibility, but can avoid confusing users and avoid letting users to set it manually.

Please share some views about which one is better, thanks.

cc @flavio @tarcieri

@lukehinds
Copy link
Member

Got you @Xynnn007 you have the dependency pinned to your fork until upstream changes merge. Thanks for pointing that out, I will change this to a hold.

Regarding the two options. I am more inclined to allow users to pass a usize in. How about we allow users to set key length, yet if the key length is None, we default to 4096?

Copy link
Member

@lukehinds lukehinds left a comment

Choose a reason for hiding this comment

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

hold until RSA upstream patch is merged.

@tarcieri
Copy link
Contributor

Note: the RSA patch is merged

@Xynnn007
Copy link
Member Author

Note: the RSA patch is merged

Great, thanks! When the new version of rsa is published I will import it, instead of {git = "xxx", rev = "xxx"}

@lukehinds
Copy link
Member

@tarcieri do you have any insight into when v0.7.0 moves from rc to a release?

@flavio
Copy link
Member

flavio commented Sep 29, 2022

We can already consume the rc from crates.io: https://crates.io/crates/rsa/0.7.0-rc.0

@lukehinds
Copy link
Member

We can already consume the rc from crates.io: https://crates.io/crates/rsa/0.7.0-rc.0

Is this sufficient @Xynnn007 ?

flavio
flavio previously approved these changes Sep 29, 2022
@flavio
Copy link
Member

flavio commented Sep 29, 2022

I tried building using the released crate and the code doesn't compile.

Aside from that, I tried the current code (the one that builds)::

curl -O https://ftp.suse.com/pub/projects/security/keys/container–key.pem
cargo run --example verify -- -k ./container–key.pem registry.suse.com/suse/sle-micro/5.0/toolbox

and the verification fails. I didn't look too much into that, maybe @Xynnn007 has a better idea about why this RSA key is not working anymore 🤔

@Xynnn007
Copy link
Member Author

Yea, I pushed a pr on 0.7.0-rc.0 to help, so we should wait for the newest tag after 0.7.0-rc.0
@lukehinds @flavio

@tarcieri
Copy link
Contributor

@lukehinds we'd like to get a final release out soon, although it'd be good to know if there are any remaining issues.

@lukehinds
Copy link
Member

lukehinds commented Sep 30, 2022

I think we should merge this, and hold on the release for 0.7.0 , at which point we can just update cargo.toml, that way it will save folks managing rebases when this later lands. We can cut the release then.

How does that sound @Xynnn007 ?

@Xynnn007
Copy link
Member Author

I think we should merge this, and hold on the release for 0.7.0 , at which point we can just update cargo.toml, that way it will save folks managing rebases when this later lands. We can cut the release then.

How does that sound @Xynnn007 ?

Ok, I've just change the crate ref. Also, current implementation may not be perfect as I see, and we can make it better over time.

lukehinds
lukehinds previously approved these changes Sep 30, 2022
@flavio
Copy link
Member

flavio commented Sep 30, 2022

@Xynnn007 Please, before merging it, can you figure out what is wrong with the RSA signature I've pointed out here?

This is a regression

@Xynnn007
Copy link
Member Author

@Xynnn007 Please, before merging it, can you figure out what is wrong with the RSA signature I've pointed out here?

This is a regression

Ah, please wait for some time. Let me do this

@flavio
Copy link
Member

flavio commented Sep 30, 2022

Ah, please wait for some time. Let me do this

Sure, no hurries! 😄

I just wanted to ensure this PR wouldn't get accidentally merged with this open issue.

@Xynnn007
Copy link
Member Author

@flavio Hey, there is a trick. The image you've given is sign by RSA_PKCS1_SHA256, and I set the default signing scheme of RSA is RSA_PSS_SHA256, because the padding scheme PSS is safer than PKCS1

You can use an extra parameter to tell the signing scheme

cargo run --example verify -- -k ./container–key.pem --signing-scheme RSA_PKCS1_SHA256 registry.suse.com/suse/sle-micro/5.0/toolbox

And will result

    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `target/debug/examples/verify -k './container–key.pem' --signing-scheme RSA_PKCS1_SHA256 registry.suse.com/suse/sle-micro/5.0/toolbox`
2022-09-30T12:08:50.238132Z  INFO sigstore::cosign::client_builder: Rekor public key not provided. Rekor integration disabled
2022-09-30T12:08:50.238193Z  INFO sigstore::cosign::client_builder: No Fulcio cert has been provided. Fulcio integration disabled
Image successfully verified

Still hold on, please. I found some bugs for notes, let me fix it in a few minutes.

@Xynnn007
Copy link
Member Author

Done, please take a look whether there is problems left
@flavio @lukehinds

@lukehinds
Copy link
Member

could you rebase with #136 pls

@Xynnn007
Copy link
Member Author

@lukehinds hey done

@lukehinds
Copy link
Member

thanks @Xynnn007

@flavio is your comment still applicable?

@lukehinds
Copy link
Member

I should have held off 0.5.0 for this one, my bad. @flavio as above, are you OK with this one to merge?

Cargo.toml Outdated Show resolved Hide resolved
@flavio
Copy link
Member

flavio commented Oct 5, 2022

@flavio Hey, there is a trick. The image you've given is sign by RSA_PKCS1_SHA256, and I set the default signing scheme of RSA is RSA_PSS_SHA256, because the padding scheme PSS is safer than PKCS1

You can use an extra parameter to tell the signing scheme

cargo run --example verify -- -k ./container–key.pem --signing-scheme RSA_PKCS1_SHA256 registry.suse.com/suse/sle-micro/5.0/toolbox

And will result

    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `target/debug/examples/verify -k './container–key.pem' --signing-scheme RSA_PKCS1_SHA256 registry.suse.com/suse/sle-micro/5.0/toolbox`
2022-09-30T12:08:50.238132Z  INFO sigstore::cosign::client_builder: Rekor public key not provided. Rekor integration disabled
2022-09-30T12:08:50.238193Z  INFO sigstore::cosign::client_builder: No Fulcio cert has been provided. Fulcio integration disabled
Image successfully verified

Please, forgive me if I bring this topic up again. Is there a way to understand if a RSA key is using the pkcs v1 1.5 or the pss signature algorithm? I tried searching for it, but I couldn't find anything about that. Maybe @Xynnn007 or @tarcieri know about that...

I'm so obsessed by this topic because I would like the consumers of the sigstore crate to be able to build tools that behave like cosign. With cosign, I don't have to specify what kind of algorithms are used by an input key, it will try to guess it using a list of supported algorithms.

Now, this PR is doing something in this direction too with the PublicKeyVerifier::try_from method. It would be great to extend this method to cover also RSA_PKCS1 verification keys.

@tarcieri
Copy link
Contributor

tarcieri commented Oct 5, 2022

Is there a way to understand if a RSA key is using the pkcs v1 1.5 or the pss signature algorithm?

I'm not 100% clear on this, but I believe PKCS#1 v2.2 as defined in RFC8017 makes it possible to mark a key as intended for use with PSS, and also provide the additional PSS parameters (e.g. hash function, MGF, and salt length) needed to instantiate the algorithm. See:

https://datatracker.ietf.org/doc/html/rfc8017#appendix-A.2.3

Note that this isn't yet supported by the pkcs1 crate. We have a tracking issue for it here: RustCrypto/formats#694

@tarcieri
Copy link
Contributor

tarcieri commented Oct 5, 2022

I released rsa v0.7.0-rc.1 which may fix the compile errors.

We also have a tracking issue for a final v0.7.0 release here: RustCrypto/RSA#205

@flavio
Copy link
Member

flavio commented Oct 6, 2022

I released rsa v0.7.0-rc.1 which may fix the compile errors.

Yes, that fixes the compilation errors. Thanks!

I've updated this PR to make use of this pre-release from crates.io instead of obtaining the code from the git repository

- replace ring crate with RustCrypto
- refactor CosignVerificationKey implementation and interface
- fix the code which references CosignVerificationKey

Signed-off-by: Xynnn007 <mading.ma@alibaba-inc.com>
@Xynnn007
Copy link
Member Author

Xynnn007 commented Oct 6, 2022

I released rsa v0.7.0-rc.1 which may fix the compile errors.

We also have a tracking issue for a final v0.7.0 release here: RustCrypto/RSA#205

Great, thanks

I released rsa v0.7.0-rc.1 which may fix the compile errors.

Yes, that fixes the compilation errors. Thanks!

I've updated this PR to make use of this pre-release from crates.io instead of obtaining the code from the git repository

Hi, I'll rebase the newest version and fix the errors.

Now, this PR is doing something in this direction too with the PublicKeyVerifier::try_from method. It would be great to extend this method to cover also RSA_PKCS1 verification keys.

Hi agreed. However as @tarcieri said, the related functions of RSA Crypto is still not prepared. I think we can wait for the functions to be prepared and then make a new pr to fix this. Before that, we still should need to manually set the algorithms for the key. Does it make sense? @flavio

@flavio
Copy link
Member

flavio commented Oct 6, 2022

Hi agreed. However as @tarcieri said, the related functions of RSA Crypto is still not prepared. I think we can wait for the functions to be prepared and then make a new pr to fix this. Before that, we still should need to manually set the algorithms for the key. Does it make sense? @flavio

I've looked at how cosign works. Currently users who want to use keys not generated by cosign must import them, see this document.

The cosign import-key-pair does NOT support the pps padding, I did a quick test with the latest code built from the main branch. Also, looking at the verify code, the RSA key is expected to be using pkcs11.

Because of that, I propose to change the code that guesses the RSA key to give priority to what cosign supports.
That translates to do the following change:

diff --git a/src/crypto/verification_key.rs b/src/crypto/verification_key.rs
index 93845e5897..2a0c470724 100644
--- a/src/crypto/verification_key.rs
+++ b/src/crypto/verification_key.rs
@@ -161,7 +161,9 @@ impl CosignVerificationKey {
                 ed25519bytes.as_ref(),
             )?))
         } else if let Ok(rsapk) = rsa::RsaPublicKey::from_public_key_der(der_data) {
-            Ok(Self::RSA_PSS_SHA256(pss::VerifyingKey::new(rsapk)))
+            Ok(Self::RSA_PKCS1_SHA256(
+                pkcs1v15::VerifyingKey::new_with_prefix(rsapk),
+            ))
         } else {
             Err(SigstoreError::InvalidKeyFormat {
                 error: "Failed to parse the public key.".to_string(),

I think that gives the UX I'm looking for :)

Close sigstore#27

Signed-off-by: Xynnn007 <mading.ma@alibaba-inc.com>
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Xynnn007 for the awesome work and for your patience during the review process! 👏 ❤️

@flavio flavio merged commit a0ebf6b into sigstore:main Oct 7, 2022
flavio added a commit to flavio/sigstore-rs that referenced this pull request Oct 7, 2022
Enhancements
============

* update user-agent value to be specific to sigstore-rs (sigstore#122)
* remove /api/v1/version from client by (sigstore#121)
* crate async fulcio client (sigstore#132)
* Removed ring dependency (sigstore#127)

Others
======

* Update dependencies
* Refactoring and examples for key interface (sigstore#123)
* Fix doc test failures (sigstore#136)

Contributors
============

* Bob Callaway (@bobcallaway)
* Bob McWhirter (@bobmcwhirter)
* Flavio Castelli (@flavio)
* Luke Hinds (@lukehinds)
* Xynnn (@Xynnn007)

Signed-off-by: Flavio Castelli <fcastelli@suse.com>
@Xynnn007 Xynnn007 deleted the refactor-remove-ring branch October 8, 2022 01:22
@tarcieri
Copy link
Contributor

FYI rsa v0.7.0 has been released: https://crates.io/crates/rsa/0.7.0

@Xynnn007
Copy link
Member Author

FYI rsa v0.7.0 has been released: https://crates.io/crates/rsa/0.7.0

Yea, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants