-
Notifications
You must be signed in to change notification settings - Fork 52
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
Conversation
fe13148
to
553f90e
Compare
There was a problem hiding this 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
src/crypto/signing_key/ed25519.rs
Outdated
@@ -399,16 +398,16 @@ mod tests { | |||
let pubkey = key | |||
.public_key_to_pem() | |||
.expect("export private key to PEM format failed."); | |||
println!("{}", pubkey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, probably a leftover?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 thesignature
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. :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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 👍
There was a problem hiding this 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.
Thanks. Let me finish RSA support in this PR then.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
b160821
to
76b1b9d
Compare
There was a problem hiding this 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) |
There was a problem hiding this comment.
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 👍
Please do not review the newly push-ed commit now, because it uses a modified version of |
a5974ac
to
460de25
Compare
There was a problem hiding this 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
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 I pushed the second commit to collect suggestions about the rsa key length. There are two design schemes:
Please share some views about which one is better, thanks. |
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 |
There was a problem hiding this 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.
Note: the RSA patch is merged |
Great, thanks! When the new version of |
@tarcieri do you have any insight into when v0.7.0 moves from rc to a release? |
We can already consume the rc from |
Is this sufficient @Xynnn007 ? |
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 🤔 |
Yea, I pushed a pr on |
@lukehinds we'd like to get a final release out soon, although it'd be good to know if there are any remaining issues. |
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 ? |
460de25
to
534cf4e
Compare
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. |
Sure, no hurries! 😄 I just wanted to ensure this PR wouldn't get accidentally merged with this open issue. |
@flavio Hey, there is a trick. The image you've given is sign by 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
Still hold on, please. I found some bugs for notes, let me fix it in a few minutes. |
534cf4e
to
447c89f
Compare
Done, please take a look whether there is problems left |
could you rebase with #136 pls |
447c89f
to
5b17f87
Compare
@lukehinds hey done |
I should have held off 0.5.0 for this one, my bad. @flavio as above, are you OK with this one to merge? |
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 Now, this PR is doing something in this direction too with the |
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 |
I released We also have a tracking issue for a final v0.7.0 release here: RustCrypto/RSA#205 |
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 |
6f266b7
to
4afde59
Compare
- 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>
Great, thanks
Hi, I'll rebase the newest version and fix the errors.
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 The Because of that, I propose to change the code that guesses the RSA key to give priority to what 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 :) |
4afde59
to
21b4e5c
Compare
Close sigstore#27 Signed-off-by: Xynnn007 <mading.ma@alibaba-inc.com>
21b4e5c
to
5c14f24
Compare
There was a problem hiding this 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! 👏 ❤️
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>
FYI |
Yea, thanks! |
Summary
Due to #27. This pr removed
ring
crate, and use RustCrypto crates instead.Release Note
RSA PSS
&RSA PKCS#1 v1.5
)API Changes
from_XXX
functions forCosignVerificationKey
'sSignatureDigestAlgorithm
parameter intoSigningScheme
SignatureDigestAlgorithm
CosignVerificationKey
into enum.Documentation