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.

Sign up for GitHub

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

Jump to bottom

AssociatedAlgorithmIdentifier implementation #278

Merged
merged 2 commits into from
Apr 5, 2023
Merged

AssociatedAlgorithmIdentifier implementation #278

merged 2 commits into from
Apr 5, 2023

Conversation

lumag
Copy link
Contributor

@lumag lumag commented Mar 27, 2023

This is an implementation of a trait defined at RustCrypto/formats#954

@lumag lumag mentioned this pull request Mar 27, 2023
Closed
@rozbb
Copy link

rozbb commented Mar 27, 2023

Thanks! Could you possibly 1) make the ID string consts at the top of the file, and 2) put some links in the comments for where the strings come from?

tarcieri
tarcieri reviewed Mar 27, 2023
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
baloo
baloo reviewed Apr 1, 2023
View reviewed changes
src/pkcs1v15.rs Outdated Show resolved Hide resolved
@lumag lumag force-pushed the kai branch 2 times, most recently from 5d63864 to 3ca392b Compare April 2, 2023 22:55
lumag
lumag commented Apr 2, 2023
View reviewed changes
src/pkcs1v15.rs Outdated Show resolved Hide resolved
@lumag lumag force-pushed the kai branch 2 times, most recently from 5fb4f89 to ceb637c Compare April 3, 2023 14:30
@lumag lumag marked this pull request as ready for review April 3, 2023 14:31
@lumag
Copy link
Contributor Author

lumag commented Apr 3, 2023

@baloo this PR has support for PKCS1v15. Could you please check whether it works for you?

@baloo
Copy link
Member

baloo commented Apr 3, 2023

Yeah, I was missing the blanket impl for SignatureAlgorithmIdentifier. That works for me.

@baloo baloo mentioned this pull request Apr 3, 2023
Closed
baloo
baloo reviewed Apr 3, 2023
View reviewed changes
src/pkcs1v15.rs
#[cfg(feature = "sha2")]
impl RsaSignatureAssociatedOid for sha2::Sha224 {
const OID: ObjectIdentifier =
const_oid::ObjectIdentifier::new_unwrap("1.2.840.113549.1.1.14");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const_oid::ObjectIdentifier::new_unwrap("1.2.840.113549.1.1.14");
const_oid::db::rfc5912::SHA_224_WITH_RSA_ENCRYPTION

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was bugging me too. But I think the overall design now is not to add a dependency on const-oid/db.
@tarcieri ?

Copy link
Member

Choose a reason for hiding this comment

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

At some point we could probably consider using the db in this crate, though it does add to compile times a little bit.

@tarcieri
Copy link
Member

tarcieri commented Apr 3, 2023

Which of these (#278 vs #284) should I be reviewing? They have many of the same changes.

@lumag
Copy link
Contributor Author

lumag commented Apr 3, 2023

Which of these (#278 vs #284) should I be reviewing? They have many of the same changes.

This ons (#278). #284 was a spin-off w/o pkcs1v15 changes which were controvercial.

tarcieri
tarcieri reviewed Apr 3, 2023
View reviewed changes
src/pss.rs Outdated Show resolved Hide resolved
@lumag lumag force-pushed the kai branch 2 times, most recently from c389a1e to 4170b19 Compare April 4, 2023 02:14
@lumag
Copy link
Contributor Author

lumag commented Apr 4, 2023

@tarcieri rsa crate doesn't have direct dependency on spki/der. Would it be possible to cut a dummy point release for pkcs8 to bump its internaly dependecy? Or we'd better declare rsa -> spki dependency?

@tarcieri
Copy link
Member

tarcieri commented Apr 4, 2023

@lumag already in progress: RustCrypto/formats#983

@lumag
Copy link
Contributor Author

lumag commented Apr 4, 2023

Thanks! didn't notice it.

@lumag
Copy link
Contributor Author

lumag commented Apr 5, 2023

Ugh. @tarcieri Another request, but now for pkcs1, to get RsaPssParams::SALT_LEN_DEFAULT

@lumag
feat: implement associated AlgorithmIdentifier traits for PKCS1v15 an…
bd0c090 
…d PSS keys

Allow getting the algorithm identifiers for signing keys

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
@lumag
pkcs1v15: implement SignatureAlgorithmIdentifier for PKCS1v15 structs
4cf8531 
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
@tarcieri tarcieri merged commit cf90255 into RustCrypto:master Apr 5, 2023
9 checks passed
@lumag lumag deleted the kai branch April 5, 2023 06:59
@lumag
Copy link
Contributor Author

lumag commented Apr 5, 2023

@tarcieri thank you!

@tarcieri tarcieri mentioned this pull request Apr 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baloo baloo baloo left review comments

@tarcieri tarcieri tarcieri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lumag @rozbb @baloo @tarcieri