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.

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

cms: ECC KeyAgreementRecipientInfo initial support #1579

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

nemynm
Copy link
Contributor

@nemynm nemynm commented Oct 20, 2024

This PR intends to bring initial support for Elliptic Curve Cryptography (ECC) for CMS - addressing #1544.

  • Following rfc5753, it implements KeyAgreementRecipientInfoBuilder for ECC. For now only EnvelopedData using (ephemeral-static) ECDH is supported.
  • It does not include reader/decryption logic (it could be part of another PR if there is interest).
  • It does not support SHA1 and 3DES schemes (For SHA1, it could maybe be introduced and gated behind a feature to parse/read/decrypt older/legacy incoming CMS message that use them. However RustCrypto does not seem to support 3DES key wrap)
  1. Utils functions:

    • KDF and key-wrapping operation are hosted in their own submodules.
    • Key-derivation uses an internal HashDigest enum to select the hash function to use during key-derivation
    • KeyWrapper is a struct that aims at abstracting the key-wrapping logic for different AES algorithm and different incoming key size.
  2. Key Agreement Recipient Info:

    • KeyAgreementRecipientInfoBuilder is hosted in its own submodule.
    • Supported schemes and algorithm are represented using enums. At least three are needed for the builder:
      • EcKeyEncryptionInfo - recipient public key (generic over RustCrypto elliptic-curve).
        While EcKeyEncryptionInfo is essentially the ECC equivalent of the existing KeyEncryptionInfo, it has been introduced to limit API breakage - as it introduces a generic over the chosen elliptic-curve.
      • KeyAgreementAlgorithm - key agreement as per RFC (SHA1 schemes are not supported on purpose, can be amended if needed).
      • KeyWrapAlgorithm - key-wrap algorithm to use.
        For the sake of simplicity From<ContentEncryptionAlgorithm> for KeyWrapAlgorithm trait has been implemented.
  3. Testing:
    Unit tests have been written in the relevant files. One integration test showcasing KeyAgreeRecipientInfoBuilder is available, leveraging the existing test P256 key material. Obtained message can be decrypted using:

    openssl cms -decrypt -inkey cms/tests/examples/p256-priv.der -inform PEM

nemynm added 23 commits October 20, 2024 09:33
- Re-organize imports
- Adjust comments
- Add KeyAgreeRecipientInfoBuilder build logic
- Add tests for KeyAgreementAlgorithm and EcKeyEncryptionInfo
@nemynm nemynm marked this pull request as ready for review October 20, 2024 16:14
cms/Cargo.toml Outdated
@@ -22,9 +22,12 @@ x509-cert = { version = "=0.3.0-pre.0", default-features = false }

# optional dependencies
aes = { version = "=0.9.0-pre.2", optional = true }
aes-kw = { version ="0.2.1", optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

This duplicates the dependency on aes and cipher, we should bump aes-kw to use "aes 0.9.0-pre.2" & "cipher 0.5.0-pre.7".

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll merge once it's ready

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged

/// [RFC 5753 Section 8]: https://datatracker.ietf.org/doc/html/rfc5753#section-8
#[allow(clippy::enum_variant_names)]
#[derive(Clone, Copy)]
pub enum KeyAgreementAlgorithm {
Copy link
Member

Choose a reason for hiding this comment

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

Could we convert that to a trait instead:

pub trait KeyAgreementAlgorithm: AssociatedOid {
    fn kdf(secret: &[u8]);
}

And then make a struct like:

struct DhSinglePassStdDhKdf<D> where D:Digest + {
    digest: PhantomData<D>,
}

impl<D> KeyAgreementAlgorithm for DhSinglePassStdDhKdf<D> where D:Digest {
    fn kdf(secret: &[u8]) {
        ansi_x963_kdf::derive_key_into::<D>(secret)
    }
}

impl AssociatedOid for DhSinglePassStdDhKdf<sha2::Sha224> {
   const OID: ObjectIdentifier = rfc5753::DH_SINGLE_PASS_STD_DH_SHA_224_KDF_SCHEME;
}

Copy link
Member

Choose a reason for hiding this comment

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

I had something like that in mind:
nemynm#2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!
Indeed I was not particularly convinced by the current KDF handling, but I thought it was a fair solution that was easily extensible for other schemes/algo (maybe with the help of a quick macro to derive try_ansi_x963_kdf).

At first I actually went the trait route in my code (close-ish to what you proposed), but then settled on the enum and dispatch function. Rationales were:

  1. This snippet in EnvelopedDataBuilder at builder.rs#L890 which caught my eye:
    // TODO bk Not good to offer both, `content_encryptor` and `content_encryption_algorithm`.
    // We should
    // (1) either derive `content_encryption_algorithm` from `content_encryptor` (but this is not
    //            yet supported by RustCrypto),
    // (2) or     pass `content_encryption_algorithm` and create an encryptor for it.
    // In the first case, we might need a new trait here, e.g. `DynEncryptionAlgorithmIdentifier` in
    // analogy to `DynSignatureAlgorithmIdentifier`.
    // Going for (2)
    //  content_encryptor: E,
    content_encryption_algorithm: ContentEncryptionAlgorithm,

Even if its not the same situation, I interprated it as "work with enum rather than trait", at least until some is finalized. That was somewhat re-inforced by the current get_hasher() function.

  1. Consequently, just passing an enum looked closer to the current API for other builders (having to specify the KA generic in KeyAgreeRecipientInfoBuilder seemed a tad less user-friendly).

  2. I thought that if we ended up supporting the other key agreement algorithm (e.g. cofactor ECDH and 1-Pass ECMQV ), we may want to have a more complete trait like:

pub trait KeyAgreementAlgorithm: AssociatedOid {
    fn shared_secret()
    fn try_kdf(secret: &[u8], other_info: &[u8], key: &mut impl AsMut<[u8]>) -> Result<()>;
}

-> So essentially folding most of the build() logic behind a trait - so to speak.

However not exactly knowing yet:

  • what shared_secret() signature should look like for all.
  • the input to try_kdf() would be with other schemes (would that always be a SharedSecret<C> or something more generic like &[u8]).
  • if we should have three structs (DhSinglePassStdDhKdf, DhSinglePassCoFactorDhKdf, MqvSinglePassKdf) or maybe just one generic for the three schemes.

I figured that a trait was maybe not the best solution yet.

Anyway as you can see, these are more generic thoughts than strong reasons not to, so I am also ok to go the trait route now too.

Comment on lines +69 to +71
Self::Aes128 => const_oid::db::rfc5911::ID_AES_128_WRAP,
Self::Aes192 => const_oid::db::rfc5911::ID_AES_192_WRAP,
Self::Aes256 => const_oid::db::rfc5911::ID_AES_256_WRAP,
Copy link
Member

Choose a reason for hiding this comment

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

TODO: We should implement AssociatedOid in https://github.com/RustCrypto/key-wraps/tree/master/aes-kw

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks will merge once its merged upstream

Copy link
Member

Choose a reason for hiding this comment

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

we will need to move that to a generic parameter as well

Copy link
Contributor Author

@nemynm nemynm Nov 1, 2024

Choose a reason for hiding this comment

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

I could add a trait for Key Wrapping too. Do you want the trait to only implement AssociateOid, or the key wrapping logic itself as well (akin to kdf for the KeyAgreement trait)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we will need to move that to a generic parameter as well

@baloo, I came up with a trait-based approach for key-wrapping, a first draft is available here nemynm#3, let me know your thoughts and if this is the kind of things you had in mind.

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