-
Notifications
You must be signed in to change notification settings - Fork 134
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
PasswordRecipientInfoBuilder for CMS #1273
base: master
Are you sure you want to change the base?
Conversation
@tarcieri Regarding the security alert: I assume, I have to wait for the |
cms/src/builder.rs
Outdated
/// Provided password encryptor | ||
pub key_encryptor: P, | ||
/// Random number generator | ||
pub rng: &'r mut R, |
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.
It could be worth changing the signature of RecipientInfoBuilder::build
trait function to build_with_rng
, just so we don't have to bind the rng like that.
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.
Or maybe not, it's not object-safe anymore.
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.
So we leave it as it is? Or what kind of binding would you prefer?
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'm not too fan of the &mut R
in an object. I think that makes it harder to use. You need two instances of the RNG: for example in the context of HSM, you might not have two references to the RNG easily (kough pkcs11 kough).
If we could bring in the rng "late" (it's already in the EnvelopedDataBuilder::build_with_rng
context), that be great. But the Vec<Box<dyn RecipientInfoBuilder>>
requires it to be object-safe, which makes it impossible.
I don't have a good solution to offer, but that's my concern here (not a blocker obviously).
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.
the only solution I could come up with is this one: bkstein#1
But this introduces some API breakage in the builder (but we can just push that in a cms-0.3)
Co-authored-by: Arthur Gautier <superbaloo+registrations.github@superbaloo.net>
Co-authored-by: Arthur Gautier <superbaloo+registrations.github@superbaloo.net>
(@bkstein if you rebase, the security audit error should go away. This is not actionable for now and we'll ignore it until RSA 0.10 is released) |
Obviously, this should not be forgotten. Co-authored-by: Arthur Gautier <superbaloo+registrations.github@superbaloo.net>
This requires two rng to be made available, which might not necessarily be available in some cases.
@baloo First, I apologize for the long time beeing absent. We were busy working on our project and we had this PR as a local patch. But I'm interested in making this official and would like to resume. I started by updating to the master branch and experienced problems with the new crate versions. The clippy error is due to incompatible versioning of some Dependencies (digest is referenced by ecdsa with version v0.11.0-pre.8, but I introduced pkdf2, which references digest v0.10.7):
At the moment, I don't see how to resolve this. |
Ha, this is an oversight. I've bumped a bunch of dependencies in the ecosystem to pre-release versions but forgot pkcs5. See; #1391 (also: no need to apologize, we all have various priorities :)) |
@baloo I updated this PR to the latest master and dependencies. As for me, this can be merged. |
@baloo Reverted to draft, because I think, it's better to modify the rng handling. You mentioned above, that 2 rng's are now necessary for the pwri-builder. But it should be possible to get by with one rng, if I pass it from the builder to the encryption method. |
@baloo I tried to write a test for the |
You can grab it back in anycase:
Yes! Please grab it! |
@baloo I think, this PR is ready. However, there is a problem in CI with cargo-audit which is not related to my changes, I think. |
This provides a builder for CMS'
PasswordRecipientInfo
according to RFC 3211.In contrast to the
KeyTransRecipientInfoBuilder
the key-encryption algorithm must be provided by the user (who has to provide an implementation ofPwriEncryptor
). This allows for more flexibility in choosing allgorithms for key derivation and encryption.An example for using the implementation is in the new test.