Skip to content

evp_test (ML_KEM): IKME instead of entropy name, bug fixes & a small refactor#26082

Closed
paulidale wants to merge 2 commits intoopenssl:feature/ml-kemfrom
paulidale:ikme-entropy-name
Closed

evp_test (ML_KEM): IKME instead of entropy name, bug fixes & a small refactor#26082
paulidale wants to merge 2 commits intoopenssl:feature/ml-kemfrom
paulidale:ikme-entropy-name

Conversation

@paulidale
Copy link
Contributor

Use the existing IKME param instead of a new one for the key generation input.
Avoid using param builder.
Fix some mistakes in the return code.

  • documentation is added or updated
  • tests are added or updated

@paulidale paulidale added the approval: review pending This pull request needs review by a committer label Nov 29, 2024
@paulidale paulidale self-assigned this Nov 29, 2024
@paulidale paulidale changed the title Ikme entropy name, bug fixes & a small refactor evp_test (ML_KEM): IKME instead of entropy name, bug fixes & a small refactor Nov 29, 2024
Copy link

@vdukhovni vdukhovni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally fine, but please rebase.

Comment on lines 128 to 132

if ((p = OSSL_PARAM_locate_const(params, OSSL_KEM_PARAM_MLKEM_ENC_ENTROPY)) != NULL
&& (p->data_size != MLKEM_ENCAP_ENTROPY
if ((p = OSSL_PARAM_locate_const(params, OSSL_KEM_PARAM_IKME)) != NULL
&& (p->data_size < MLKEM_ENCAP_ENTROPY
|| (ctx->entropy = OPENSSL_memdup(p->data, MLKEM_ENCAP_ENTROPY)) == NULL))
return 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire file goes away in the upcoming refactor. So it likely best if you rebase on the ML-KEM refactor PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. CI failures are relevant but don't reproduce locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might it be easier for you to pull the commits here into the refactor branch?
Otherwise, we're going to encounter an ordering conflict which serialises PRs.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which CI failures? As for the conflict, I am happy to pull your changes in, but this PR will become obsolete and should be closed. Works for you?

The recent additon of EVP KEM tests for ML-KEM used a new parameter to pass
the input entropy for the key.  The ECX tests already had a different
parameter for this purpose, so the ML-KEM code is updated.
@vdukhovni
Copy link

Integrated as fixup into the ML-KEM PR. Closing.

@vdukhovni vdukhovni closed this Nov 29, 2024
@paulidale
Copy link
Contributor Author

Thanks.

@paulidale paulidale deleted the ikme-entropy-name branch November 29, 2024 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: review pending This pull request needs review by a committer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants