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.

Sign up for GitHub

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

feat: revise EdDSA mechanism to support optional params #244

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: revise EdDSA mechanism to support optional params #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8d4fca9
feat: revise EdDSA mechanism to support optional params
mcaneris Jan 18, 2025
282cbdf
refactor: derive Default for EddsaParams
mcaneris Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions cryptoki/src/mechanism/eddsa.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
//! EdDSA mechanism types

use cryptoki_sys::*;
use std::marker::PhantomData;

/// EdDSA parameters.
///
/// The EdDSA mechanism, denoted CKM_EDDSA, is a mechanism for
/// single-part and multipart signatures and verification for
/// EdDSA. This mechanism implements the five EdDSA signature
/// schemes defined in RFC 8032 and RFC 8410.
///
/// For curves according to RFC 8032, this mechanism has an
/// optional parameter, a CK_EDDSA_PARAMS structure.
///
/// The absence or presence of the parameter as well as its
/// content is used to identify which signature scheme is to be
/// used.
///
/// | Signature Scheme | Mechanism Param | phFlag | Context Data |
/// |------------------|-----------------|--------|--------------|
/// | Ed25519 | Not Required | N/A | N/A |
/// | Ed25519ctx | Required | False | Optional |
/// | Ed25519ph | Required | True | Optional |
/// | Ed448 | Required | False | Optional |
/// | Ed448ph | Required | True | Optional |
Comment on lines +20 to +26
Copy link
Member

Choose a reason for hiding this comment

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

As I understood the spec one has two ways to choose its curve:

  • either use the RFC 8032 curves (which are the ones you listed) and use the CK_EDDSA_PARAMS
  • or use a RFC 8410 curve and directly select it in the CKK_EC_EDWARDS type. Seems to be Eddsa in pure mode.

In order to remove the raw CK_EDDSA_PARAMS from new, would it work to define an enum in this file listing those 5 schemes + the pure mode option and then remove the Option in the Mechanism enum?

So something like:

pub enum EddsaSignatureScheme {
    Ed25519,
    Ed25519ctx(&[u8]),
    Ed25519ph(&[u8]),
    Ed448(&[u8]),
    Ed448ph(&[u8]),
    /// Curves according to RFC 8410, the mechanism is implicitly given by the curve, which is EdDSA in pure mode.
    PureMode,
}

and then the new method below would take this enum as argument and set the right fields!

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I think this can be much more expressive.

On @wiktor-k's suggestion, I tried to test the schemes with SoftHSM, but the signature produced does not seem to differ based on the params (between Ed25519, Ed25519ctx, and Ed25519ph, for example).

Let me try with your suggestion, and I'll get back.

Copy link
Author

@mcaneris mcaneris Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@hug-dev I implemented the enum variants, and it removes the need to pass the raw CK_EDDSA_PARAMS struct to the EddsaParams constructor.

But I'm thinking it still doesn't remove the need to pass an Option<EddsaParams> to the mechanism at the final stage, and match on None case specifically to set the pParameter of the mechanism itself to be null, and ulParameterLen to be 0.

Because at the end, make_mechanism calculates the ulParameterLen based on the type, which will never be 0 so long as EddsaParams or CK_EDDSA_PARAMS is passed as an argument.

  • None case should either pass null_mut() or generate CK_MECHANISM directly with null_mut() similar to HkdfKeyGen
  • Some case should pass EddsaParams.

Or we could set inner field of EddsaParams to hold an Option<CK_EDDSA_PARAMS> and match on that directly generating a CK_MECHANISM rather than using make_mechanism, which seems to be the best solution, removing the need to pass an Option to the mechanism, and setting the inner Option using the EddsaSignatureScheme.

I don't see anyway around it, do you have any further suggestions?

///
/// This structure wraps a `CK_EDDSA_PARAMS` structure.
#[derive(Copy, Debug, Clone, Default)]
#[repr(transparent)]
pub struct EddsaParams<'a> {
inner: CK_EDDSA_PARAMS,
_marker: PhantomData<&'a [u8]>,
}

impl EddsaParams<'_> {
/// Construct EdDSA parameters.
///
/// # Arguments
///
/// * `params` - The CK_EDDSA_PARAMS structure.
pub fn new(params: CK_EDDSA_PARAMS) -> Self {
Self {
inner: params,
_marker: PhantomData,
}
}
}
24 changes: 18 additions & 6 deletions cryptoki/src/mechanism/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
//! Data types for mechanisms

pub mod aead;
pub mod eddsa;
pub mod ekdf;
pub mod elliptic_curve;
pub mod hkdf;
Expand Down Expand Up @@ -902,11 +903,16 @@ pub enum Mechanism<'a> {
EcdsaSha512,
/// EDDSA mechanism
///
/// This mechanism has an optional parameter, a CK_EDDSA_PARAMS
/// structure. The absence or presence of the parameter as well
/// as its content is used to identify which signature scheme
/// is to be used.
///
/// Note: EdDSA is not part of the PKCS#11 v2.40 standard and as
/// such may not be understood by the backend. It is included here
/// because some vendor implementations support it through the
/// v2.40 interface.
Eddsa,
/// such may not be understood by some backends. It is included
/// here because some vendor implementations support it through
/// the v2.40 interface.
Eddsa(Option<eddsa::EddsaParams<'a>>),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm... here None and Some(Default::default()) is exactly the same state... I wonder if it's too ugly to require Default::default() (remove the Option)... @hug-dev ?

Copy link
Author

@mcaneris mcaneris Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I agree the passing of an Option made the code more ugly, but I think the None case is not same as the Default case.

In the Default case, although CK_EDDSA_PARAMS is a nullish struct (phFlag is false, pContextData is null, and pContextDataLen is 0), params passed to Eddsa mechanism is no longer null (in other words, pParameter is not null, and ulParameterLen is no longer 0).

Reading the specification, I understand that for selection of the Ed25519 curve, pParameter must be null, and ulParameterLen must be 0.

That said, I'm open to suggestions for a more elegant solution, I agree the Option made it ugly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree the passing of an Option made the code more ugly, but I think the None case is not same as the Default case.

I don't mind None that much but just for the sake of my understanding I'd like to get bottom to the None non-equal to Default. I think one thing that tripped me is the manual conversion from CK_MECHANISM below. 👇

Copy link
Member

Choose a reason for hiding this comment

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

Replied in my other comment on a way to fix this, with an enum 🤓!


// SHA-n
/// SHA-1 mechanism
Expand Down Expand Up @@ -1001,7 +1007,7 @@ impl Mechanism<'_> {
Mechanism::EccKeyPairGen => MechanismType::ECC_KEY_PAIR_GEN,
Mechanism::EccEdwardsKeyPairGen => MechanismType::ECC_EDWARDS_KEY_PAIR_GEN,
Mechanism::EccMontgomeryKeyPairGen => MechanismType::ECC_MONTGOMERY_KEY_PAIR_GEN,
Mechanism::Eddsa => MechanismType::EDDSA,
Mechanism::Eddsa(_) => MechanismType::EDDSA,
Mechanism::Ecdh1Derive(_) => MechanismType::ECDH1_DERIVE,
Mechanism::Ecdsa => MechanismType::ECDSA,
Mechanism::EcdsaSha1 => MechanismType::ECDSA_SHA1,
Expand Down Expand Up @@ -1073,6 +1079,13 @@ impl From<&Mechanism<'_>> for CK_MECHANISM {
| Mechanism::Sha512RsaPkcsPss(params) => make_mechanism(mechanism, params),
Mechanism::RsaPkcsOaep(params) => make_mechanism(mechanism, params),
Mechanism::Ecdh1Derive(params) => make_mechanism(mechanism, params),
Mechanism::Eddsa(params) => match params {
Some(params) => make_mechanism(mechanism, params),
None => CK_MECHANISM {
mechanism,
..Default::default()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Continued:

This line made me suspicious. Maybe it's just me but I think this would be more readable (basically the same code as for Mechanism::HkdfKeyGen):

Suggested change
..Default::default()
pParameter: null_mut(),
ulParameterLen: 0,

I wonder if this is just me, though, and yeah, I'm aware that this makes the code longer 😅

},
},
Mechanism::HkdfDerive(params) | Mechanism::HkdfData(params) => {
make_mechanism(mechanism, params)
}
Expand All @@ -1098,7 +1111,6 @@ impl From<&Mechanism<'_>> for CK_MECHANISM {
| Mechanism::EccKeyPairGen
| Mechanism::EccEdwardsKeyPairGen
| Mechanism::EccMontgomeryKeyPairGen
| Mechanism::Eddsa
| Mechanism::Ecdsa
| Mechanism::EcdsaSha1
| Mechanism::EcdsaSha224
Expand Down
53 changes: 51 additions & 2 deletions cryptoki/tests/basic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use common::init_pins;
use cryptoki::context::Function;
use cryptoki::error::{Error, RvError};
use cryptoki::mechanism::aead::GcmParams;
use cryptoki::mechanism::eddsa::EddsaParams;
use cryptoki::mechanism::rsa::{PkcsMgfType, PkcsOaepParams, PkcsOaepSource};
use cryptoki::mechanism::{Mechanism, MechanismType};
use cryptoki::object::{
Expand Down Expand Up @@ -99,9 +100,57 @@ fn sign_verify_ed25519() -> TestResult {

let data = [0xFF, 0x55, 0xDD];

let signature = session.sign(&Mechanism::Eddsa, private, &data)?;
let signature = session.sign(&Mechanism::Eddsa(None), private, &data)?;

session.verify(&Mechanism::Eddsa, public, &data, &signature)?;
session.verify(&Mechanism::Eddsa(None), public, &data, &signature)?;

session.destroy_object(public)?;
session.destroy_object(private)?;

Ok(())
}

#[test]
#[serial]
fn sign_verify_ed25519_with_eddsa_params() -> TestResult {
let (pkcs11, slot) = init_pins();

let session = pkcs11.open_rw_session(slot)?;

session.login(UserType::User, Some(&AuthPin::new(USER_PIN.into())))?;

let mechanism = Mechanism::EccEdwardsKeyPairGen;

let pub_key_template = vec![
Attribute::Token(true),
Attribute::Private(false),
Attribute::Verify(true),
// Ed25519 OID
// See: https://github.com/opendnssec/SoftHSMv2/blob/ac70dc398b236e4522101930e790008936489e2d/src/lib/test/SignVerifyTests.cpp#L173
Attribute::EcParams(vec![
0x13, 0x0c, 0x65, 0x64, 0x77, 0x61, 0x72, 0x64, 0x73, 0x32, 0x35, 0x35, 0x31, 0x39,
]),
];

let priv_key_template = vec![Attribute::Token(true)];

let (public, private) =
session.generate_key_pair(&mechanism, &pub_key_template, &priv_key_template)?;

let data = [0xFF, 0x55, 0xDD];

let signature = session.sign(
&Mechanism::Eddsa(Some(EddsaParams::default())),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does SoftHSM support any other parameters than the defaults? If so it'd be good to test it.

private,
&data,
)?;

session.verify(
&Mechanism::Eddsa(Some(EddsaParams::default())),
public,
&data,
&signature,
)?;

session.destroy_object(public)?;
session.destroy_object(private)?;
Expand Down
Loading