feat: revise EdDSA mechanism to support optional params #244
Conversation
cryptoki/tests/basic.rs
Outdated
| let data = [0xFF, 0x55, 0xDD]; | ||
|
|
||
| let signature = session.sign( | ||
| &Mechanism::Eddsa(Some(EddsaParams::default())), |
There was a problem hiding this comment.
Does SoftHSM support any other parameters than the defaults? If so it'd be good to test it.
cryptoki/src/mechanism/mod.rs
Outdated
| /// 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>>), |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 👇
There was a problem hiding this comment.
Replied in my other comment on a way to fix this, with an enum 🤓!
cryptoki/src/mechanism/mod.rs
Outdated
| /// 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>>), |
There was a problem hiding this comment.
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. 👇
cryptoki/src/mechanism/mod.rs
Outdated
| Some(params) => make_mechanism(mechanism, params), | ||
| None => CK_MECHANISM { | ||
| mechanism, | ||
| ..Default::default() |
There was a problem hiding this comment.
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):
| ..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 😅
There was a problem hiding this comment.
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 :)
cryptoki/src/mechanism/mod.rs
Outdated
| /// 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>>), |
There was a problem hiding this comment.
Replied in my other comment on a way to fix this, with an enum 🤓!
mcaneris
force-pushed
the
main
branch
2 times, most recently
from
aaa07e0 to
bad849d
Compare
This revises the `Mechanism::Eddsa` struct to receive a `EddsaParams` struct as an optional argument. Signed-off-by: Mete Can Eriş <can.eris@feyn.dev>
This revises the
Mechanism::Eddsastruct to receive aEddsaParamsstruct 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.
The binding for
CK_EDDSA_PARAMSis already present under thecryptoki-syscrate. However, there is no way currently to pass these params to theMechanism::Eddsastruct.As the specification places significance on the absence of the parameters (resulting in curve selection), I elected to pass
EddsaParamsas anOptiontoMechanism::Eddsa. Added and modified tests pass for Ed25519 signing with no parameters, as well as defaultCK_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.