Feat: Add NIST SP800-108 KDF mechanisms

[jacobprudhomme]

Author: Jacob Prud'homme
Email: [email protected]

Description

This PR adds support for the NIST SP800-108 KDF (aka KBKDF) in counter-, feedback- and double pipeline-modes

Summary of Changes

• Added SP800_108_{COUNTER,FEEDBACK,DOUBLE_PIPELINE}_KDF mechanism types
  • Also, their corresponding Mechanism variants and associated parameters (KbkdfParams for counter- and double pipeline-mode, KbkdfFeedbackParams for feedback-mode)
  • Also, the types used within these parameter types (PrfDataParam, KbkdfDkmLengthFormat, KbkdfDkmLengthType, KbkdfCounterFormat, DerivedKey)
  • Also, tests for each mechanism (don't be afraid of the size of the diff! This is where the largest part of the changes comes from)
• Updated warning on pointer casts in mechanism::make_mechanism()
• Unrelated lint errors that popped up (not sure where they came from...)

Design Justifications

Since there's a decent amount of changes in this PR, I figured I'd start by justifying some of the choices I made, to preemptively answer any questions that might pop up.

1. I didn't make KbkdfParams/KbkdfFeedbackParams transparent wrappers around the corresponding PKCS#11 struct, because it simply wasn't possible. Since we need to pass a &[Attribute] to the constructor for DerivedKey (unless we want to expose the end-user to the internal CK_ATTRIBUTE struct), and Attribute itself isn't a transparent wrapper around CK_ATTRIBUTE, we have to convert to a slice of CK_ATTRIBUTEs and store it internally, so that that contiguous section of memory stays valid for the needed duration. The consequences of this choice bubble up to the top level, and requires that the Kbkdf[Feedback]Params themselves can not be transparent for the same reason. Because of this, I had to add an accessor for the wrapped PKCS#11 type to get things to work with make_mechanism(). I have suggestions below about making this more flexible for the future
2. I've settled on mutating the original DerivedKey structs the end-user creates to update them with the handle of the created key, and so it is up to that user to go back and collect these ObjectHandles for the additional derived keys after calling derive_key() (see this test for an example). While slightly less convenient, this more closely matches the paradigm of the spec itself (where pointers to handles are passed in via the params, and these are then mutated when the corresponding key is derived). The additional_derived_keys argument is also correspondingly marked mut, to signal that it will be mutated on the PKCS#11 backend
   1. As you can see in commit f1a15f7, my prior solution involved creating a new derive_keys() function, not present in the spec, as well as a HasAdditionalDerivedKeys trait to allow accessing such data on arbitrary param types. This clunky solution was significantly less attractive too
3. I wrapped the internal boxed slices in DerivedKey and KbkdfParams/KbkdfFeedbackParams in a Pin. While pinning a box does nothing, I figured it was important to mark that this data should have a stable address, since the C side relies on pointers to it remaining valid

Questions/Feedback Wanted

General Questions

• When looking at the HKDF implementation as an example, there are a bunch of accessors on HkdfParams that return different parts of the internal data. I'm unsure of their purpose, however. Should I also add these to Kbkdf[Feedback]Params?

Coding Style Questions

• The Endianness enum is quite general, should I place this somewhere else?
• The tests I wrote are quite big, I'd be happy to break them up if deemed necessary

Design Questions

• I named all external-facing types as Kbkdf(...) instead of something like Sp800108(...), because the naming of the latter is atrocious. However, the term KBKDF is never used in the spec and may lead to people not easily discovering the functionality they need. How do you feel about this naming?
• As you can see in commit 8ac48ed, I previously had an implementation with separate PrfCounterDataParam and PrfDataParam types, as there are some differences between the data parameters allowed by the KDF in counter-mode vs the other modes. Notably, in counter-mode, IterationVariable must take a KbkdfCounterFormat argument, and the Counter data parameter type is not allowed. While distinguishing these two types of params ensures the end-user could not make mistakes when configuring the KDF, it goes against the spec, which has just a single CK_PRF_DATA_PARAM type. Would the type-safety focus of the original design be preferred over the current 1-to-1 mapping with the spec? In the latter case, the backend should return CKR_MECHANISM_PARAM_INVALID for these types of errors anyway, but it may be nice to stop these errors at the type system-level

Suggestions for Future

Both of these came up while trying to work around the limitations I ran into on Design Justifications (1).

1. What if we made Attribute a transparent wrapper around CK_ATTRIBUTE? That way we could get rid of the inconsistencies between constructing a Kbkdf[Feedback]Params vs other parameter types, i.e. have a tree of just transparent wrappers all the way to the root params object, which we can directly pass as a pointer to the C side, as was done up until now
2. Solving part of the same issue as above, what if we made some sort of trait AsPkcsParams defining a as_pkcs_params_ptr() method with the following default implementation that covers 99% of the cases so far (i.e. all cases except this one):

fn as_pkcs_params_ptr(&self) -> *mut c_void {
    self as *const T as *mut c_void
}

While this partially overlaps with (1) in terms of solving some inconsistencies that I've had to introduce to make my changes fit, I think it has other benefits. While not so important because make_mechanism() is internal, it would still prevent developers from making a mistake and enforce that the function is only used with parameter objects that explicitly implement the trait, instead of arbitrary types. As well, the impact of implementing this is quite small. I could even make that a part of this PR, if you're interested!

[wiktor-k]

I think it looks good. It's quite a sizable change 😅

The make_mechanism made me think of #225 🤔

[wiktor-k]

Does width_in_bits have any value constraints? Maybe using NonZeroUsize would be a good idea? 🤔

[jacobprudhomme]

The spec doesn't explicitly say anything: https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/os/pkcs11-curr-v3.0-os.html#_Toc30061446. But I think it's logical that if you didn't want a Counter or DkmLength format object to be present in the input data, you would just leave it out instead of providing one with length 0. I'll make this change

[wiktor-k]

Just out of curiosity why this is pinned?

[jacobprudhomme]

Essentially I just wanted to include some visual hint to the programmer that it's crucial that the address of this data doesn't change (because of the fact that we pass pointers into it to the backend). I know this is redundant since Box guarantees this anyway, but I thought it would be nice to make this explicit. If you think it's just noise or could be confusing, I'm happy to remove it

[wiktor-k]

Why the underscore?

[jacobprudhomme]

Since we don't use this member, the lint check complains without it. It's just an internal member needed to hold the slice of additional key PKCS#11 structs so that it remains valid for the lifetime of the params object (this is a consequence of what I explained in the Design Justifications section of the PR body)

[wiktor-k]

I think it'd be good to move all these unsupported cases into a separate test and mark them with #[ignore = "unsupported by both softhsm and kryoptic"].

[jacobprudhomme]

Done!

[jacobprudhomme]

@wiktor-k glad to know I was on the same wavelength as previous work using mutable parameters! I was worried I was doing something weird.

Also, did you have any comments/remarks on what I've written in the PR body?

[wiktor-k]

Also, did you have any comments/remarks on what I've written in the PR body?

Tbh I want to calmly re-read it as I didn't have much time for open-source reviews today. I'll add a more concrete comment soon.