cms: make KeyWrapAlgorithm a trait

[nemynm]

No description provided.

[baloo]

I think you can make it use AssociatedOid:

Suggested change

	pub trait KeyWrapAlgorithm {
	pub trait KeyWrapAlgorithm: AssociatedOid {

and then drop the parameter() below.

[baloo]

and fn algorithm_identifier() too?

[nemynm]

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

[nemynm]

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?).

[nemynm]

If we want to remove algorithm_identifer(), we could potentially do something like:

1. Define a trait for AlgorithmIdentifierOwned:

pub trait AssociateAlgorithmIdentifier {
    const ALGORITHM_IDENTIFIER: AlgorithmIdentifierOwned;
}

2. 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>;
}

3. And then impl for KeyWrap and a new generic struct KeyEncryption respectively:

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.

[baloo]

I had in mind something like #4 (look at the last commit only, the branch needs a rebase to clear up the noise).

[baloo]

isn't that redundant with the impl<T> AsMut<[u8]> for WrappedKey<T> just below?

[nemynm]

indeed, fixed in ffad593

[nemynm]

@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?

[baloo]

This will need a rebase to be reviewable, at the moment, this includes 35 commits

[nemynm]

Hello, did a rebase - although some checks are now failling. I'll need to have a look at that too.

[nemynm]

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?