Initial KBKDF implementation #108

TheBestTvarynka · 2025-01-27T10:48:31Z

Hello,
I implemented KBKDF and added tests in this PR. The implementation is based on the previously opened draft PR: #87. I fixed merge conflicts, made a few small corrections, and added more tests based on the NIST test vectors: https://csrc.nist.gov/Projects/Cryptographic-Algorithm-Validation-Program/Key-Derivation.

Specification: https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-108r1.pdf.

Take a look at the comments below for more details.

TheBestTvarynka · 2025-01-27T10:49:28Z

kbkdf/tests/kbkdf/counter_mode.rs

Please, do not be afraid of big file sizes 😅. It's a generated code. counter_mode.rs and feedback_mode_iv.rs are generated files.

NIST test vectors contain a lot of samples. It'd take an enormous compilation time to parse test data files, generate Rust code, and compile generated tests. So, I tried other approaches.

Instead of parsing data files in proc macro, I wrote a separate script that generates Rust code. The idea is to compile the already generated code instead of compile time code generation. This script isn't included in this PR, but if this approach is approved, I'll refactor it and add it to the repo.

Although, you can notice that my code still has macros. It's because I wanted to abstract generated tests from the crate API. If the crate API changes, we will not need to regenerate test files but only change the macro.

During my experiments, I realized that the test compilation takes more time than code generation. So, the fewer tests we have the faster it compiles. So, I reduced the test amount by grouping samples with the same KBKDF configuration.

cargo clean && cargo test takes around 15-20 seconds (Inter Core i5-10300H 16 Gb Ram). They cover the following KBKDF configurations:

Counter and feedback modes.

BEFORE_FIXED and AFTER_ITER counter locations.

HMAC-SHA1/224/256/384/512 and CMAC-AES128/192/256 PRFs.

R-len: 8/16/24/32 bits.

And various Ls defined in test vectors.

I don't know if that was the approach I would have recommended. That still produces a ton of code for the compiler to work through.

Because we have the test vector parser implemented, can't we just run it at runtime and not at compile time instead?

can't we just run it at runtime and not at compile time instead?

We can 🙂 Somewhat I didn't think about it. I'll try it

UDP: I rewrote tests. See this comment for details: #108 (comment)

One problem with large file sizes is they impact the size of the released crate.

It would be good to exclude such files and the tests that depend on them so tests against the released crate still pass. You can look here for an example:

https://github.com/RustCrypto/MACs/blob/master/cmac/Cargo.toml#L14

done. I also added a simple README.md file because cargu publish --dry-run fails to work without it. Currently, I'm working on a proper README.md page

TheBestTvarynka · 2025-01-27T10:51:04Z

kbkdf/src/lib.rs

+            if Self::DOUBLE_PIPELINE {
+                h.update(a.as_slice());
+            } else {
+                // counter encoded as big endian u32
+                // Type parameter R encodes how large the value is to be (either U8, U16, U24, or U32)
+                //
+                // counter = 1u32 ([0, 0, 0, 1])
+                //                     \-------/
+                //                      R = u24
+                h.update(&counter.to_be_bytes()[(4 - R::USIZE / 8)..]);
+            }
+
+            // Fixed input data


According to the specification, the counter is hashed before the fixed data. But NIST test vectors have samples for AFTER_FIXED, MIDDLE_FIXED, and BEFORE_ITER counter locations. I'm wondering why is that.

I checked KBKDF implementation in other languages. For example, a CounterLocation is implemented in Python's implementation. Should I implement it too or leave it as it is?

TheBestTvarynka · 2025-01-27T10:51:32Z

kbkdf/src/lib.rs

+                // counter encoded as big endian u32
+                // Type parameter R encodes how large the value is to be (either U8, U16, U24, or U32)
+                //
+                // counter = 1u32 ([0, 0, 0, 1])
+                //                     \-------/
+                //                      R = u24
+                h.update(&counter.to_be_bytes()[(4 - R::USIZE / 8)..]);


I noticed that the counter hashing is marked as optional in the specification. Moreover, test vectors have samples for KBKDF with and without a counter. I think the use_counter: bool flag should be added. What do you think?

Yes, we should make it optional!

If anything for TPM support. KBKDF is used for KDFa in the TPM world: https://trustedcomputinggroup.org/wp-content/uploads/TPM-2.0-1.83-Part-1-Architecture.pdf#page=79

I'm working on the implementation of it, I'll pull this PR to try it out.

So I pulled it in the TPM implementation (parallaxsecond/rust-tss-esapi#563) and got it to work successfully!

I had to add a single commit to it though: baloo@4ce305a

This just use the pre-release version of the rust-crypto crates.

I implemented the use_counter: bool flag. The implementation passed all NIST tests.

Rebased on your latest branch: baloo@35c5fa1

TheBestTvarynka · 2025-01-27T10:57:24Z

@baloo Can you take a look at this PR and my comments? What do you think about this approach?

Thank you 🙏

…t countermode testing;

…v tests;

… mode without counter tests;

TheBestTvarynka · 2025-01-29T16:16:52Z

kbkdf/tests/kbkdf/parser.rs

As suggested in this comment, now we parse NIST test vectors directly in tests and test our KBKDF implementation against them.

I tried to make the parser and executor implementation simple and readable. I included data files in the repo and I use them in the code using the include_str macro.

Moving from dynamic KBKDF parameters (parsed from the file) to generic type parameters was the biggest issue I faced. I wrote a macro to overcome it and explained it in the code. See this comment: https://github.com/RustCrypto/KDFs/pull/108/files#diff-0eee564f4af3593aaeb6be4eeab84a3889b59ceaadb1f54810f31acb4ec8279aR329-R333

// This function tests KBKDF implementation against test vectors parsed from the file. // // All KDF parameters are obtained in runtime, but the implementation relies on generic type parameters which are compile-time. // Macros inside this function generate all possible KDF parameter configurations. A suitable KDF configuration will be selected during runtime.

cargo clean && cargo test takes around 50-53 seconds (Inter Core i5-10300H 16 Gb Ram). Is it acceptable for RustCrypto? If not, then I guess we have two ways:

Replace macros with the code they generate (it's around 8 /* number of PRFs */ * 4 /* number of possible R values */ * 14 /* number of different Ls */ = 448 if statements).

Change the implementation design and move some type parameters to regular function parameters.

I think it's acceptable for now, even if not great.

That at least gets us through the door. We can always improve or change strategy later as needed.

TheBestTvarynka · 2025-01-29T16:29:06Z

kbkdf/src/sealed.rs

I changed the sealing method because I need the R trait to be exported. I use it in integration tests.

kbkdf/Cargo.toml

README.md

baloo · 2025-01-29T17:09:43Z

The last bit would be TheBestTvarynka#2, otherwise it looks good to me. Thanks a lot for your work!

baloo · 2025-01-29T17:24:03Z

and a readme: TheBestTvarynka#3

TheBestTvarynka · 2025-01-31T13:06:24Z

@tarcieri What else do I need to do for this PR to be merged?

newpavlov · 2025-01-31T13:13:59Z

kbkdf/README.md

@@ -0,0 +1,72 @@
+# RustCrypto: KBKDF
+
+[![crate][crate-image]][crate-link] [![Docs][docs-image]][docs-link] ![Apache2/MIT licensed][license-image] ![Rust Version][rustc-image] [![Project Chat][chat-image]][chat-link] [![Build Status][build-image]][build-link]


For better readability it's better to split the badges into multiple lines, e.g. see here.

TheBestTvarynka#5

I was planning to address comments tomorrow morning 😅 anyway, thank you! 🤗
done

newpavlov · 2025-01-31T13:18:59Z

kbkdf/src/lib.rs

+                "
+                241C3FBAABEDE87601B1C778B24F9A32 742A14FE34DA61D77E8352EF9D6C7FC8
+                E335E32344E21D7DC0CD627D7E2FF973 992611F372C5D3DD91C100F2C6DB2CAF
+            "


You can write these constants like this:

&hex!( "241C3FBAABEDE87601B1C778B24F9A32" "742A14FE34DA61D77E8352EF9D6C7FC8" "E335E32344E21D7DC0CD627D7E2FF973" "992611F372C5D3DD91C100F2C6DB2CAF" )

TheBestTvarynka#4

newpavlov · 2025-01-31T13:19:40Z

kbkdf/src/lib.rs

+}
+
+#[cfg(test)]
+mod tests {


These tests are relatively big, so it may be worth to move them into a separate file, e.g. counter_tests.rs.

TheBestTvarynka#6

newpavlov · 2025-01-31T13:49:04Z

kbkdf/Cargo.toml

+categories = ["cryptography", "no-std"]
+readme = "README.md"
+rust-version = "1.81"
+exclude = ["/tests/data/*"]


Excluding the test data would break all integration tests, no? If so, we should just exclude tests/ completely.

TheBestTvarynka#7

newpavlov · 2025-01-31T13:58:26Z

kbkdf/src/lib.rs

+        kin: &[u8],
+        use_l: bool,
+        use_separator: bool,
+        use_counter: bool,


API of this function is somewhat easy to misuse. Maybe it's worth to do something like this?

let key = k.derive(Params { secret, label: b"label", context: b"ctx", use_l: true, ..Default::default(), });

We can't provide a default for the boolean as they would usually default to true and can't provide a default for the key, label or context, but we can make a param builder: TheBestTvarynka#8

baloo and others added 11 commits January 22, 2025 14:24

Initial commit for KBKDF

5357d80

cleanup

7617a2f

add CI

2018242

cleanup

42af5fc

double-pipeline support

342e132

fixup cargo.toml

3c1e97e

feat(kbkdf): add counter mode tests;

d7664e9

feat(kbkdf): add counter mode tests: cmacaes;

a050281

feat(kbkdf): format tests;

7aeede2

feat(kbkdf): add feedback mode with iv tests;

0cb72fe

fix(kbkdf): double pipeline implementation: do not hash separator;

7f211fa

TheBestTvarynka commented Jan 27, 2025

View reviewed changes

TheBestTvarynka added 7 commits January 29, 2025 12:57

feat(kbkdf): tests: implement simple test vactors parser and implemen…

af72de5

…t countermode testing;

feat(kbkdf): tests: refactor tests and implement feedback mode with i…

2bf73b5

…v tests;

feat(kbkdf): tests: implement double pipeline without counter tests;

6f05fd4

feat(kbkdf): implement use_counter flag;

b9517fc

feat(kbkdf): tests: implement double pipeline with counter tests;

77059cb

feat(kbkdf): tests: implement feedback mode with zero iv and feedback…

560d134

… mode without counter tests;

feat(kbkdf): fix broken sealing;

8bb2873

TheBestTvarynka commented Jan 29, 2025

View reviewed changes

baloo and others added 2 commits January 29, 2025 18:35

feat(kbkdf): use pre-releases

bb86eab

chore: update MSRV for kbkdf;

b56a967

TheBestTvarynka marked this pull request as ready for review January 29, 2025 16:48

tarcieri reviewed Jan 29, 2025

View reviewed changes

kbkdf/Cargo.toml Outdated Show resolved Hide resolved

tarcieri reviewed Jan 29, 2025

View reviewed changes

kbkdf/Cargo.toml Outdated Show resolved Hide resolved

chore: kbkdf: remove devrem and update hex-literal and rust versions;

4886b3b

chore: kbkdf: exclude NIST test vectors from publishing. add simple

289c98e

README.md

kbkdf: std is no longer required as core::error::Error stabilized (#2)

737aaa0

baloo approved these changes Jan 29, 2025

View reviewed changes

kbkdf: provide a README (#3)

fa2c25e

baloo mentioned this pull request Jan 29, 2025

Initial commit for KBKDF #87

Closed

2 tasks

newpavlov reviewed Jan 31, 2025

View reviewed changes

baloo added 5 commits January 31, 2025 22:48

kbkdf: cleanup tests (#4)

ad79ff9

kbkdf: fixup readme (#5)

2c59f3a

kbkdf: split off unit tests (#6)

a5c1b21

kbkdf: integration tests are not functional without data (#7)

0715721

kbkdf: provide a param builder (#8)

b9e3645

		@@ -0,0 +1,72 @@
		# RustCrypto: KBKDF

		[![crate][crate-image]][crate-link] [![Docs][docs-image]][docs-link] ![Apache2/MIT licensed][license-image] ![Rust Version][rustc-image] [![Project Chat][chat-image]][chat-link] [![Build Status][build-image]][build-link]

Initial KBKDF implementation #108

Are you sure you want to change the base?

Initial KBKDF implementation #108

Conversation

TheBestTvarynka commented Jan 27, 2025

TheBestTvarynka Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baloo Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBestTvarynka commented Jan 27, 2025

TheBestTvarynka Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baloo commented Jan 29, 2025

baloo commented Jan 29, 2025

TheBestTvarynka commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBestTvarynka Jan 27, 2025 •

edited

Loading

baloo Jan 27, 2025 •

edited

Loading

TheBestTvarynka Jan 29, 2025 •

edited

Loading