Add PKCS#11 3.2 bindings + plug them into the cryptoki

[Jakuje]

The PKCS#11 3.2 headers are available and considered final:

https://github.com/latchset/pkcs11-headers/tree/main/public-domain/3.2

This pulls the new headers, regenerates bindings and adjusts the initialization so they can be used.

I took also the chance to update bindgen to 0.71.1 once we generate new binding version

[Jakuje]

Oh, lovely! The CI does not like the generated binding as it is too complex.

error: very complex type used. Consider factoring parts into `type` definitions
    --> /home/runner/work/rust-cryptoki/rust-cryptoki/cryptoki-sys/src/bindings/x86_64-unknown-linux-gnu.rs:6801:35
     |
6801 |       pub C_UnwrapKeyAuthenticated: Result<
     |  ___________________________________^
6802 | |         unsafe extern "C" fn(
6803 | |             arg1: CK_SESSION_HANDLE,
6804 | |             arg2: *mut CK_MECHANISM,
...    |
6814 | |         ::libloading::Error,
6815 | |     >,
     | |_____^
     |

Who would say it for a function with 10 arguments:

    pub C_UnwrapKeyAuthenticated: Result<
        unsafe extern "C" fn(
            arg1: CK_SESSION_HANDLE,
            arg2: *mut CK_MECHANISM,
            arg3: CK_OBJECT_HANDLE,
            arg4: *mut CK_BYTE,
            arg5: CK_ULONG,
            arg6: *mut CK_ATTRIBUTE,
            arg7: CK_ULONG,
            arg8: *mut CK_BYTE,
            arg9: CK_ULONG,
            arg10: *mut CK_OBJECT_HANDLE,
        ) -> CK_RV,
        ::libloading::Error,
    >,  

Is this something we should fix in the generated bindings or open an issue for the bindgen? Not sure if this is a new issue with the new bindgen, but more likely the issue of the new PKCS#11 API having that insane number of arguments ....

[wiktor-k]

Hmm fortunately this is just a clippy issue. I guess suppressing it somewhere would be an okayish workaround. (maybe in lib.rs of -sys?)

I'd still report it to bindgen. This may improve their code generation or at least make them consider adding the lint suppression in the generated code.

[Jakuje]

Thanks for the pointer! I see the lib.rs has already some more warnings ignored. I am not sure if there is some way how to improve the code generation to avoid this. I can think of using some temporary named type for the function, but I think it just moves the problem one more function argument away.

I think this is inherent issue of the PKCS#11 specification, introducing this complex functions.

[wiktor-k]

I am not sure if there is some way how to improve the code generation to avoid this.

FWIW I didn't mean that. Slapping the lint suppression there is sufficient enough for me.

[Jakuje]

Kryoptic tests now fail as there are few bugs in the last release that were already fixed in main. I think this is already good for first reviews, but I would probably wait for the pkcs11 3.2 to get released officially and I will likely implement also the SLH-DSA (they look quite simple and similar to the ML-DSA, but I do not have an implementation to test against yet).

[hug-dev]

Feel free to ping us when you want us to review this again!

[Jakuje]

I think this should be ready for reviews! Rebased and updated links to the final headers.

[teythoon]

I found a small issue. When I ask for an AttributeType::MlDsaParameterSet, the token returns an Attribute::ParameterSet.

I think the patch introduces AttributeType::MlDsaParameterSet as an alias for AttributeType::ParameterSet. Both MlDsaParameterSet and ParameterSet map to CKA_PARAMETER_SET, and therefore the distinction is lost, and the response will be a ParameterSet.

[Jakuje]

I found a small issue. When I ask for an AttributeType::MlDsaParameterSet, the token returns an Attribute::ParameterSet.

I think the patch introduces AttributeType::MlDsaParameterSet as an alias for AttributeType::ParameterSet. Both MlDsaParameterSet and ParameterSet map to CKA_PARAMETER_SET, and therefore the distinction is lost, and the response will be a ParameterSet.

Yeah, I was aware of this issue, but I am not sure how this could be easily handled given that this attribute has different values for different key types. Any thoughts?

I think it would require some logic in the GetAttributeValue mapping the type to the correct aliased "subtype" based on the key type, but I really did not look into that yet.

[teythoon]

I think the aliasing is most surprising. The Rust model is a refinement of the interface model, i.e. it is more expressive. I think that is a mistake at this point. It is also surprising for people coming from the PKCS#11 spec.

Instead, I think we should offer explicit conversion functions from ParameterSetType (doesn't exist yet, and should hold a u64, and that is what Attribute::ParameterSet should use) to MlDsaParameterSet.

[wiktor-k]

Looks good 👌 Thanks for attaching tests for various combinations of the new API.

[wiktor-k]

Suggested change

	let (p_context, ul_context_len) = match context {
	Some(c) => (
	c.as_ptr() as *mut _,
	c.len().try_into().expect("usize can not fit in CK_ULONG"),
	),
	None => (null_mut(), 0),
	};
	let (p_context, ul_context_len) = if let Some(c) = context {
	(c.as_ptr() as *mut _,
	c.len().try_into().expect("usize can not fit in CK_ULONG")),
	} else {
	(null_mut(), 0),
	};
	};

I tend to avoid matches which then destructure into Some/None as I think if let is more readable... but it's not a blocker :)

[wiktor-k]

Is that still needed?

[Jakuje]

warning: function `is_softhsm` is never used
  --> cryptoki/tests/common/mod.rs:19:8
   |
19 | pub fn is_softhsm() -> bool {
   |        ^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

warning: `cryptoki` (test "ml_kem") generated 1 warning
warning: `cryptoki` (test "ml_dsa") generated 1 warning (1 duplicate)

I did some restructure of the tests and this reports failure for the two now (mlkem and mldsa) as they do not use this public function.

I think the whole test structure can be improved as now it builds separate test binaries, which is not very effective.

So as a after-thought I think we should really have one test binary and source code split to separate files, probably something similar as we have in kryoptic (but that lives under src/tests, which I understand is not a standard place to push tests according to rust.

[wiktor-k]

which is not very effective.

Oh yeah, this is definitely an annoying aspect of cargo test and part of the reason why in other projects we're instead using cargo nextest run instead of bare cargo test.

[hug-dev]

Thank you for pushing for this!

[Jakuje]

I still want to resolve the ParameterSetType issue reported by @teythoon, but I needed some clarification what exactly he had in mind. I agree that the current solution is wrong, but I was not able to make better one working so far :)

[Jakuje]

@teythoon I did rehash the ParameterSetType a bit in the last commit. Is it something you had in your mind or is there something missing that you would add or change?

[teythoon]

Yes, that was what I had in mind.

But, I remember Simo suggesting to keep the aliases, always make sure to request the key type as well, and when wrapping the response in Rust types, convert the generic parameter set response to a specific one based on the reported key type. If that is feasible to implement, that'd be most convenient for the downstream users.

[Jakuje]

But, I remember Simo suggesting to keep the aliases, always make sure to request the key type as well, and when wrapping the response in Rust types, convert the generic parameter set response to a specific one based on the reported key type. If that is feasible to implement, that'd be most convenient for the downstream users.

I guess I will have to think about that a bit more (and will probably not get back to that before I will get back from holiday). It will either have to be some ad-hoc change in the C_GetAttribute to do both of the pulling of the key type and converting the parameter set to the key-specific one. Or some new more generic logic to do this. But as far as I know the ParameterSet of the three new PQC algorithms is the only one so the ad-hoc solution might be enough.

[Jakuje]

@teythoon I though about implementing the aliases, but though it would be more confusing now. With the .into() conversion, the inner types are just one step away. If we decide to do this later, it can be done though.

@hug-dev @wiktor-k I think this is ready for final reviews.

[wiktor-k]

LGTM 👍 a couple of nits/questions.

[Jakuje]

Applied all the suggestions. Thanks for prompt review @wiktor-k !

[hug-dev]

Thank you!!