Skip to content

Add more setters for EcKdf.#292

Merged
hug-dev merged 3 commits into
parallaxsecond:mainfrom
nwalfield:eckdf
Jul 8, 2025
Merged

Add more setters for EcKdf.#292
hug-dev merged 3 commits into
parallaxsecond:mainfrom
nwalfield:eckdf

Conversation

@nwalfield

Copy link
Copy Markdown
Contributor

Adds setters for the ANSI X9.63 KDFs (EcKdf::sha1, etc) and the NIST SP800-56A KDFs (EcKdf::sha1_sp800) to EcKdf.

Fixes EcKdf::sha256.

Fixes #281.

The SP800 variants were recently added to Kryoptic.

I've tested this using Sequoia, but I haven't included any unit tests. First, the unit tests will fail, because the current released version of Kryoptic doesn't include support for the SP800 yet. Second, creating test vectors will be a huge amount of work. Given how simple the implementation is, do you want unit tests for these functions?

Thanks for taking a look.

@Jakuje

Jakuje commented Jun 25, 2025

Copy link
Copy Markdown
Collaborator

Code wise looks good. I would prefer to have some test prepared so we can enable it once the kryoptic version will get updated in Fedora, if nothing to prevent the case of having mechanisms like the sha256 added without any testing.

I do not think it needs to be full coverage of all combinations, but at least one test with NIST SP800 and one with ANSI with known answer test to make sure it works.

@hug-dev

hug-dev commented Jun 26, 2025

Copy link
Copy Markdown
Member

Thanks for this, looks good! Agree with @Jakuje about adding two simple tests where this is available in Fedora!

@nwalfield

Copy link
Copy Markdown
Contributor Author

I've updated the MR to include a test that uses known good values. I created the test vector by instrumenting Sequoia OpenPGP and dumping the relevant values. The test vector is for SHA256-SP800. It would be straightforward (although still some work) to create test vectors for SHA384-SP800 and SHA512-SP800.

As Sequoia does not use the ANSI functions, I cannot create test vectors for them without a lot of work. That said, I'm not sure of the value of additional test vectors: I think the test suite is about testing the library, not the underlying token implementation. The test demonstrates that the shape of the API is correct, and that the shared value is passed correctly.

Thoughts?

Comment thread cryptoki/tests/basic.rs Outdated
@Jakuje

Jakuje commented Jun 27, 2025

Copy link
Copy Markdown
Collaborator

Regarding the ANSI variant, I think its ok to skip it as it is mostly the same (for the rust-cryptoki).

Comment thread cryptoki/tests/basic.rs Outdated
@nwalfield nwalfield force-pushed the eckdf branch 4 times, most recently from 4bf4ba6 to 797edfa Compare June 30, 2025 09:29
@nwalfield

Copy link
Copy Markdown
Contributor Author

The MR is currently failing, because this relies on Kryoptic support the SP 800 KDF, which has been added to Kryoptic, but has not yet been released.

@Jakuje

Jakuje commented Jun 30, 2025

Copy link
Copy Markdown
Collaborator

The MR is currently failing, because this relies on Kryoptic support the SP 800 KDF, which has been added to Kryoptic, but has not yet been released.

I think the cleanest way would be now to expect it to fail now (with appropriate comment or even ideally in separate commit that can be reverted later) and change it (revert that separate commit) once the new kryoptict will be released.

The other option would be to start building kryoptic from source, but that is also outside of the scope of this PR.

Signed-off-by: Neal H. Walfield <neal@sequoia-pgp.org>
@nwalfield

Copy link
Copy Markdown
Contributor Author

The MR is currently failing, because this relies on Kryoptic support the SP 800 KDF, which has been added to Kryoptic, but has not yet been released.

I think the cleanest way would be now to expect it to fail now (with appropriate comment or even ideally in separate commit that can be reverted later) and change it (revert that separate commit) once the new kryoptict will be released.

The other option would be to start building kryoptic from source, but that is also outside of the scope of this PR.

I added two helper functions: one to check if we're using Kryoptic and one to get the token's version. I then modified the test to check if we're using at least Kryopic 1.3, which I assume will be the next version of Kryoptic. What do you think?

Comment thread cryptoki/tests/basic.rs
Signed-off-by: Neal H. Walfield <neal@sequoia-pgp.org>
Adds setters for the ANSI X9.63 KDFs (`EcKdf::sha1`, etc) and the NIST
SP800-56A KDFs (`EcKdf::sha1_sp800`) to `EcKdf`.

Fixes `EcKdf::sha256`.

Fixes parallaxsecond#281.

Signed-off-by: Neal H. Walfield <neal@sequoia-pgp.org>
@Jakuje Jakuje requested review from hug-dev and wiktor-k June 30, 2025 10:16
@hug-dev hug-dev merged commit 92382a3 into parallaxsecond:main Jul 8, 2025
39 checks passed
@nwalfield

Copy link
Copy Markdown
Contributor Author

@hug-dev, thanks for taking a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EcKdf documentation unclear

3 participants