cms: make KeyWrapAlgorithm a trait#3
Conversation
cms/src/builder/utils/kw.rs
Outdated
| /// Return the Object Identifier (OID) of the algorithm. | ||
|
|
||
| /// Represents key wrap algorithms methods. | ||
| pub trait KeyWrapAlgorithm { |
There was a problem hiding this comment.
I think you can make it use AssociatedOid:
| pub trait KeyWrapAlgorithm { | |
| pub trait KeyWrapAlgorithm: AssociatedOid { |
and then drop the parameter() below.
There was a problem hiding this comment.
We can make it use AssociateOid, but we currently still need the KeyWrap<AesWrap>: AssociatedOid bound in the implementation:
impl<AesWrap> KeyWrapAlgorithm for KeyWrap<AesWrap>
where
AesWrap: KeyInit
+ BlockSizeUser<BlockSize = U16>
+ BlockCipherEncrypt
+ BlockCipherDecrypt
+ KeySizeUser,
KeyWrap<AesWrap>: AssociatedOid,
Or actually Kek<AesWrap>: AssociatedOid:
impl<AesWrap> KeyWrapAlgorithm for KeyWrap<AesWrap>
where
AesWrap: KeyInit
+ BlockSizeUser<BlockSize = U16>
+ BlockCipherEncrypt
+ BlockCipherDecrypt
+ KeySizeUser,
Kek<AesWrap>: AssociatedOid,
The reason is that aes-kw implements AssociateOid for Kek<Aes>, so I am leveraging it to implement AssociateOid for KeyWrapAlgorithm.
Indeed as the implementation is specifically done for AES here, I guess, parameters() does not really bring much.
Adressed in 649c3f6
There was a problem hiding this comment.
As for algorithm_identifier(), I think it may still be necessary, as we need it to build the KeyAgreeRecipientInfoBuilder: it is used in EccCmsSharedInfo and in DER form for the Key Encryption Algorithm Identifier.
I do not think we can directly call a From/Into trait on the non-instanciated generic and so we need an associated function (unless we define our own AlgorithmIdentifier trait maybe?).
There was a problem hiding this comment.
If we want to remove algorithm_identifer(), we could potentially do something like:
- Define a trait for AlgorithmIdentifierOwned:
pub trait AssociateAlgorithmIdentifier {
const ALGORITHM_IDENTIFIER: AlgorithmIdentifierOwned;
}
- However I think we would still need another trait for "constructed"
AlgorithmIdentifierOwned(as const_try is not stable):
pub trait ConstructedAlgorithmIdentifier {
fn algorithm_identifier() -> Result<AlgorithmIdentifierOwned>;
}
- And then impl for
KeyWrapand a new generic structKeyEncryptionrespectively:
impl<Aes> AssociateAlgorithmIdentifier for KeyWrap<Aes>
where
KeyWrap<Aes>: AssociatedOid,
{
const ALGORITHM_IDENTIFIER: AlgorithmIdentifierOwned = AlgorithmIdentifierOwned {
oid: <Self as AssociatedOid>::OID,
parameters: None,
};
}
pub struct KeyEncryptionAlgorithm<KA, KW>
where
KA: KeyAgreementAlgorithm,
KW: KeyWrapAlgorithm,
{
_phantom: PhantomData<(KA, KW)>,
}
impl<KA, KW> ConstructedAlgorithmIdentifier for KeyEncryptionAlgorithm<KA, KW>
where
KA: KeyAgreementAlgorithm,
KW: KeyWrapAlgorithm + Encode,
{
fn algorithm_identifier() -> Result<AlgorithmIdentifierOwned> {
Ok(AlgorithmIdentifierOwned {
oid: KA::OID,
parameters: Some(Any::from_der(&KW::ALGORITHM_IDENTIFIER.to_der()?)?),
})
}
}
Essentially, as we need to construct the AlgorithmIdentifier for KeyEncryptionAlgorithm to build the Kari, I think we have to keep an associated function like algorithm_identifer() somewhere.
Let me know if that looks better to you, and let me know if I missed a better way of doing things.
Note that we would need to merge your branch with KeyAgreement as a trait before we implement this though.
There was a problem hiding this comment.
I had in mind something like #4 (look at the last commit only, the branch needs a rebase to clear up the noise).
cms/src/builder/utils/kw.rs
Outdated
| pub(crate) fn as_mut(&mut self) -> &mut [u8] { | ||
| self.inner.as_mut_slice() |
There was a problem hiding this comment.
isn't that redundant with the impl<T> AsMut<[u8]> for WrappedKey<T> just below?
eee4c5a to
2828b7e
Compare
|
@baloo hello, sorry for the delay, had less time to work on this due to end of year. I'll fix the conflicts, and I think this should be more or less ready? |
|
This will need a rebase to be reviewable, at the moment, this includes 35 commits |
5a7a6ca to
aca8958
Compare
|
Hello, did a rebase - although some checks are now failling. I'll need to have a look at that too. |
|
Ok, it seems that the issue comes from a change in the RecipientInfoBuilder trait. Do you want to adress this here, or maybe first merge this one and then use a dedicated feature branch? |
f49f856 to
7d57e32
Compare
|
Hello, I rebased against latest upstream as well as the latest feature branch which now includes:
It should now pass the CI. I think it may be best to merge this one with a squash once its ready, as it may be more manageable? |
No description provided.