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 f1a15f7f375b1869a2dd1a437c69d3eff791eee2, 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 8ac48ed215e1ce056ae34321fcb0d84bcf677966, 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!

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

[jacobprudhomme]

@wiktor-k sorry to ping again, but do you know when you might have time for a re-review?

[wiktor-k]

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?

The accessors there are for providing safe access to internals of the struct. I don't think you need to add them. If anyone feels the need they can always expand the current code, it's open-source! :)

The Endianness enum is quite general, should I place this somewhere else?

I'm not sure since the use is currently limited but I'm also okay with moving it.

The tests I wrote are quite big, I'd be happy to break them up if deemed necessary

They are quite readable. I'm more worried with special-casing more and more of our testing backends but it's not a problem for this PR.

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?

I think you made the right choice. They're readable and thanks to doc comments you've inserted if someone searches for the ugly name they'd find it too. :+1:

As you can see in commit https://github.com/parallaxsecond/rust-cryptoki/commit/8ac48ed215e1ce056ae34321fcb0d84bcf677966, 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

I think safety is more important than the 1-1 mapping. I wonder what does @hug-dev think about it?

[jacobprudhomme]

@wiktor-k thanks for the answers! I agree that maybe it's best to address any comments in follow-up PRs since it'd be nice to get this one through

[hug-dev]

I think safety is more important than the 1-1 mapping.

Agree! Trust you with making the right choices here, since you are from all of us the one knowing the best those parts :sweat_smile: We can always adapt in the future if new contributors/users have diverging opinions!

[jacobprudhomme]

Only small changes for you to review this time, thankfully!

Regarding the 1-to-1 mapping with the spec vs. being more type-safe, I decided in the end to stick with the first option. With the 2nd option, the end-user would still be able to make mistakes (i.e. initializing a KbkdfParams without a PrfDataParam::IterationVariable, which is always required) without a more significant refactor, while also making things more verbose and ugly imo. As a tradeoff, the doc comment on PrfDataParam is quite complete and describes these rules, so developers will at least have the relevant information accessible right away.