Skip to content
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

Added alg_version details to test output #2080

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pablogf-uma
Copy link
Contributor

This PR aims to fix #1786 by including the implementation path in the test_kem.c file for Kyber implementations. Before moving forward with other algorithms, I would like to confirm if this approach is correct and if the referenced implementation path aligns with the information requested in the issue.

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

Signed-off-by: Pablo Gutiérrez <pablogf@uma.es>
@dstebila
Copy link
Member

Hi Pablo! The alg_version field of the OQS_KEM data structure is meant to convey some of this information, though it doesn't exactly match what you're outputting. If there is more information you think should be included, I think my preference would be to have this information populated in an additional field of the OQS_KEM data structure so that it's available to be used in other places as well, if need be.

@baentsch
Copy link
Member

In addition to the comments by @dstebila allow me to state that OQS is a collection of many different algorithms and the amount of code catering to only some should be minimized as it makes maintenance of the library more and more difficult. Created #2081 to document this recurring issue for good.

Signed-off-by: Pablo Gutiérrez <pablogf@uma.es>
@pablogf-uma
Copy link
Contributor Author

Thank you for your feedback! I included the alg_version field instead, as suggested by @dstebila. Let me know if you would like it to be added to any other tests as well. Also, @baentsch, would this information cover what was requested in the original issue?

@dstebila
Copy link
Member

Thank you for your feedback! I included the alg_version field instead, as suggested by @dstebila. Let me know if you would like it to be added to any other tests as well. Also, @baentsch, would this information cover what was requested in the original issue?

Looks good to me! If you convert the PR to "Ready for review" I will give it an approving review.

@pablogf-uma pablogf-uma marked this pull request as ready for review February 21, 2025 14:09
@pablogf-uma pablogf-uma changed the title Added Kyber implementation details to test_kem.c Added alg_version details to test output Feb 24, 2025
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.

Enhance test output
3 participants