Skip to content

cms: make KeyWrapAlgorithm a trait#3

Merged
nemynm merged 17 commits intocms-ecc-karifrom
keywrap-trait
Feb 18, 2025
Merged

cms: make KeyWrapAlgorithm a trait#3
nemynm merged 17 commits intocms-ecc-karifrom
keywrap-trait

Conversation

@nemynm
Copy link
Owner

@nemynm nemynm commented Nov 5, 2024

No description provided.

/// Return the Object Identifier (OID) of the algorithm.

/// Represents key wrap algorithms methods.
pub trait KeyWrapAlgorithm {
Copy link

Choose a reason for hiding this comment

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

I think you can make it use AssociatedOid:

Suggested change
pub trait KeyWrapAlgorithm {
pub trait KeyWrapAlgorithm: AssociatedOid {

and then drop the parameter() below.

Copy link

Choose a reason for hiding this comment

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

and fn algorithm_identifier() too?

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Comment on lines +156 to +157
pub(crate) fn as_mut(&mut self) -> &mut [u8] {
self.inner.as_mut_slice()
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

indeed, fixed in ffad593

@nemynm
Copy link
Owner Author

nemynm commented Jan 13, 2025

@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
Copy link

baloo commented Jan 22, 2025

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

@nemynm nemynm force-pushed the cms-ecc-kari branch 3 times, most recently from 5a7a6ca to aca8958 Compare January 26, 2025 02:41
@nemynm
Copy link
Owner Author

nemynm commented Jan 26, 2025

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

@nemynm
Copy link
Owner Author

nemynm commented Jan 26, 2025

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?

@nemynm
Copy link
Owner Author

nemynm commented Feb 17, 2025

Hello, I rebased against latest upstream as well as the latest feature branch which now includes:

  • KeyAgreementAlgoritm as a trait
  • Fix for latest build_with_rng()

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?

@nemynm nemynm marked this pull request as ready for review February 17, 2025 14:13
@nemynm nemynm merged commit 3331518 into cms-ecc-kari Feb 18, 2025
165 checks passed
@nemynm nemynm deleted the keywrap-trait branch April 21, 2025 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants