feat: revise EdDSA mechanism to support optional params

[mcaneris]

This revises the Mechanism::Eddsa struct to receive a EddsaParams struct as an optional argument in line with the PKCS#11 3.0 specification.

According to RFC 8032, CKM_EDDSA 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. The following table enumerates the five signature schemes defined in RFC 8032 and all supported permutations of the mechanism parameter and its content.

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

The binding for CK_EDDSA_PARAMS is already present under the cryptoki-sys crate. However, there is no way currently to pass these params to the Mechanism::Eddsa struct.

As the specification places significance on the absence of the parameters (resulting in curve selection), I elected to pass EddsaParams as an Option to Mechanism::Eddsa. Added and modified tests pass for Ed25519 signing with no parameters, as well as default CK_EDDSA_PARAMS.

I think the capability to pass these params is necessary to use the various curves allowed under the mechanism according to the specification.

[wiktor-k]

Generally looks very nice 👌

[wiktor-k]

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

[wiktor-k]

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 ?

[mcaneris]

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.

[wiktor-k]

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. 👇

[hug-dev]

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

[wiktor-k]

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. 👇

[wiktor-k]

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 😅

[hug-dev]

Thanks for the PR!! Really appreciate the effort in the description you added and the comments too, made it easy to review!

Added a suggestion on types, would also appreciate @ionut-arm eyes to make sure all of this is correct :)

[hug-dev]

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?

[mcaneris]

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.

[mcaneris]

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

[hug-dev]

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