-
Notifications
You must be signed in to change notification settings - Fork 81
Add more setters for EcKdf. #292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
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. |
Thanks for this, looks good! Agree with @Jakuje about adding two simple tests where this is available in Fedora! |
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? |
Regarding the ANSI variant, I think its ok to skip it as it is mostly the same (for the rust-cryptoki). |
4bf4ba6
to
797edfa
Compare
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 <[email protected]>
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? |
Signed-off-by: Neal H. Walfield <[email protected]>
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 <[email protected]>
Adds setters for the ANSI X9.63 KDFs (
EcKdf::sha1
, etc) and the NIST SP800-56A KDFs (EcKdf::sha1_sp800
) toEcKdf
.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.