Treat CK*_VENDOR_DEFINED correctly

[vkkoskie]

All enum types with an associated VENDOR_DEFINED constant describe that value in roughly the same way in the standard. Using key types as an example: "Key types CKK_VENDOR_DEFINED and above are permanently reserved for token vendors. For interoperability, vendors should register their key types through the PKCS process."

The intent is that vendors who aren't concerned with interoperability or standardization are permitted to safely use that entire range of values. This crate largely omits these values, but not consistently. The two places where it does use them, they aren't treated properly:

1. CKM_VENDOR_DEFINED is recognized for the Display trait, but only as a single value. All other in-range values are treated as errors.
2. CKR_VENDOR_DEFINED is reported as an error return value. All other in-range values are converted to GeneralError.

In both cases, the underlying value is made visible in printed strings but lost within the type system and to the user.

Proposed changes:

• All enums documented to support vendor extensions should at least recognize them properly, even if full support isn't provided.
• Where defined, all values x that satisfy VENDOR_DEFINED <= x <= ULONG::MAX should be propagated back up to the user to respond to. For Rv, this would likely be a sibling type to Ok and Error(RvError). For all others it would be an additional discriminant containing the non-standard value (i.e., VendorDefined(Ulong))
• All stringification sites should include the value associated with vendor-defined values (e,g. "CKM_VENDOR_DEFINED(0x{%08x})"

[hug-dev]

Hi 👋 !

Thanks for the issue! You are perfectly right, we do not support those _VENDOR_DEFINED values correctly at all at the moment. I think it was because we overlooked the part where it says that the vendor defined values are a range of value, as opposed as a single one.

For your proposed changes:

• All enums documented to support vendor extensions should at least recognize them properly, even if full support isn't provided.

Agree!

• Where defined, all values x that satisfy VENDOR_DEFINED <= x <= ULONG::MAX should be propagated back up to the user to respond to. For Rv, this would likely be a sibling type to Ok and Error(RvError). For all others it would be an additional discriminant containing the non-standard value (i.e., VendorDefined(Ulong))

I think that even for CKR_VENDOR_DEFINED this could be put in a RvError::VendorDefined(Ulong)? Unless the CKR_VENDOR_DEFINED value could represent another success state than CKR_OK?

This is all fine when converting from C types to Rust, but when doing the reverse for some of the types when the variants are defined as const, we might have to add a new method vendor_defined(val: Ulong) to allow people to create their vendor defined values. We should also check that it belongs to the allowed range.
I think using Ulong is fine because the specification typedef the C types to CK_ULONG.

• All stringification sites should include the value associated with vendor-defined values (e,g. "CKM_VENDOR_DEFINED(0x{%08x})"

Agree!

[vkkoskie]

I think that even for CKR_VENDOR_DEFINED this could be put in a RvError::VendorDefined(Ulong)? Unless the CKR_VENDOR_DEFINED value could represent another success state than CKR_OK?

I was actually proposing that it be a third member of Rv. The interpretation of the contained value is neither a success nor a failure from the perspective of the standard. Its interpretation is entirely for the vendor to decide. This has mild ergonomics impacts by adding a third case to a typically binary concept. One possible solution to this is to feature gate vendor extension values and make them non-default. This has at least three advantages off the top of my head:

• It doesn't breaking Rv pattern matching in existing user code.
• It would permit treating vendor-range values as errors (correctly) when client code is assuming only definitions from the standard.
• It would allow the Rv enum to avoid being marked non-exhaustive (which may just be my personal taste)

This is all fine when converting from C types to Rust, but when doing the reverse for some of the types when the variants are defined as const, we might have to add a new method vendor_defined(val: Ulong) to allow people to create their vendor defined values. We should also check that it belongs to the allowed range.

I did actually make an attempt at this and found this to be very difficult in practice. Rust doesn't currently provide an error response mechanism for const and compile-time contexts. There is a static_assertions crate that provides some interesting ways around this, but all involve exporting macros as part of the API, which found a bit clunky. I'd recommend taking a look at it, though, to see if you think it's worth it. And again, these can be behind the same opt-in feature gate.

I think using Ulong is fine because the specification typedef the C types to CK_ULONG.

Correct. In all cases where a *_VENDOR_DEFINED variant is mentioned in the standard, it's a CK_ULONG type. All other vendor-defined entities are constructed by the vendor. E.g., a vendor-defined object would be composed out of a combination of pre-defined attributes and those in the CKA_VENDOR_DEFINED range.

[hug-dev]

The interpretation of the contained value is neither a success nor a failure from the perspective of the standard. Its interpretation is entirely for the vendor to decide.

That makes sense then with adding a new variant to Rv catering for this. The only problem then would be for the into_result function which transforms a Rv into a Rust Result. Might be easier to put aside the use case of multiple successful return values and add the VendorDefined in RvError. There are even some cases with existing error values which do not represent real errors but just indications. For example:

Note that the error codes CKR_ATTRIBUTE_SENSITIVE, CKR_ATTRIBUTE_TYPE_INVALID, and CKR_BUFFER_TOO_SMALL do not denote true errors for C_GeAttributeValue.

We should also check that it belongs to the allowed range.

I was thinking of doing the checks dynamically (and then having a real function and not a const)

[mike-boquard]

As an aside, a possible future item that should be discussed is whether or not this crate should be updated to support PKCS#11 v3.0 (or 3.1, whose release is supposedly imminent). Many vendors had to implement quite a few common crypto algorithms as VENDOR_DEFINED because they were missing in v2.4 but were required (e.g. supporting FIPS 140).

Not sure if it should be done with this crate or if another crate (rust-crypto-3 or something) should be forked from here.

[vkkoskie]

Re: const. I was imagining the case in which a vendor/user of the crate wanted to define their own enum values extending standard ones (i.e., types defined here). In order to do that the range check would have to happen at declaration/compile time, which requires const-friendly error support that's not in the language today except through unattractive hacks. That's all a long way of saying I agree. :)

Re: v3.0. I think feature gates are the ideal way to do that, especially if 2.4 and 3.0 (which I haven't looked at closely) overlap a great deal.

[hug-dev]

Support for vendored mechanisms was implemented in #232 . Feel free to re-open if there remains issues!

[thgoebel]

Vendor defined error codes are still not handled nicely. As a vendor, you probably do something like this:

#define CKR_MY_ERROR_A (CKR_VENDOR_DEFINED+1)
#define CKR_MY_ERROR_B (CKR_VENDOR_DEFINED+2)
#define CKR_MY_ERROR_C (CKR_VENDOR_DEFINED+3)

In cryptoki 0.10.0, these will be mapped to RvError::GeneralError. Therefore, as a cryptoki user I cannot distinguish between the vendor-defined errors.

From Section 3.6 of the standard v3.1:

Return values CKR_VENDOR_DEFINED and above are permanently reserved for token vendors.

So it should be safe to map these to something like RvError::VendorDefined(code).

[hug-dev]

Good one, thanks for noticing :) I created a new issue just for that. As you say, I think we could just do a similar approach than for vendor defined key types!