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

Convert oqs-kem-info.md code points to hex #188

Merged
merged 3 commits into from
Jun 23, 2023

Conversation

WillChilds-Klein
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein commented Jun 19, 2023

Please give a brief explanation of the purpose of this pull request.

Minor documentation update to use hexadecimal notation for KEM code points.

Does this PR resolve any issue? If so, please reference it using automatic-closing keywords like "Fixes #123."

Not that I'm aware of.


Notes

The initial motivation for this PR was a small documentation change. That change uncovered an unexpected warning message. It appears that the erroneous warning message motivating this commit is due to some vestigial code special-casing x-curves. The nid_ecx_hyybrid kem field was only used to generate the duplicate ecx nid warning message, so we track duplicates in a local set instead. Additionally, we expand the warning message to all curve NIDs, not just x-curves and treat duplicates as fatal because extra curve NIDs should be unique for each KEM.

Duplicate NID detection is tested below:

$ git diff
diff --git a/oqs-template/generate.yml b/oqs-template/generate.yml
index 4865eb7..fa3ddfe 100644
--- a/oqs-template/generate.yml
+++ b/oqs-template/generate.yml
@@ -114,6 +114,8 @@ kems:
           nid: '0x2F90'
         - hybrid_group: "x25519"
           nid: '0x6399'
+        - hybrid_group: "p521"
+          nid: '0x6399'
         - hybrid_group: "p256"
           nid: '0x639A'
       old:

$ LIBOQS_SRC_DIR=$(pwd)/../liboqs python3 oqs-template/generate.py
ERROR: duplicate hybrid NID for kyber768 : 0x6399 in generate.yml. Curve NIDs may only be specified once per KEM.

$ echo $?
1

I'm not sure what's up with the CI; I don't appear to have permissions to view the build errors in CircleCI. I ran the tests locally on a linux box:

$ cmake --build build && ( cd build; ctest -V )
...
100% tests passed, 0 tests failed out of 5

Total Test time (real) =  29.85 sec

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

This is a change to an automatically generated file and as such won't persist. Please change at the master file (oqs-template/generate.yml), re-generate (python3 oqs-template/generate.py) and re-run all tests.

@WillChilds-Klein
Copy link
Contributor Author

I'm not able to see the CircleCI results. I'll try to re-run the tests to reproduce the CI failures locally.

Also, I've noticed that both before and after this PR's changeset, the generate.py script emits (what appears to be) an erroneous warning. I'll try to update the script to fix this, but I may need some guidance on that warning's original intent.

@baentsch
Copy link
Member

Thanks for the update: This is now perfect to retain.

some guidance on that warning's original intent

This is basically a question/reminder to @bhess to re-evaluate the original ECX logic (via an extra-YML file) he originally introduced. IMO it could be removed as #177 completely changed the build logic to permit more than a single X-hybrid for the same PQ alg.

@WillChilds-Klein WillChilds-Klein marked this pull request as ready for review June 21, 2023 15:30
It appears that the erroneous warning message motivating this commit is
due to some vestigial code special-casing x-curves. The
`nid_ecx_hyybrid` kem field was only used to generate the duplicate ecx
nid warning message, so we track duplicates in a local `set` instead.
Additionally, we expand the warning message to all curve NIDs, not just
x-curves and treat duplicates as fatal because extra curve NIDs should
be unique for each KEM.
@baentsch
Copy link
Member

@WillChilds-Klein Thanks again for the contribution! CI has run successfully so we'll merge this tomorrow. As this is more than a trivial change, please consider amending this PR before merge by adding yourself to the Contributors list in the README.

@baentsch baentsch merged commit 71b6042 into open-quantum-safe:main Jun 23, 2023
3 of 4 checks passed
@WillChilds-Klein WillChilds-Klein deleted the patch-1 branch June 26, 2023 14:41
feventura pushed a commit to EntrustCorporation/oqs-provider that referenced this pull request Mar 16, 2024
* Convert Kyber768 code points to hex

* Fix generate.py duplicate hybrid NID warning

It appears that the erroneous warning message motivating this commit is
due to some vestigial code special-casing x-curves. The
`nid_ecx_hyybrid` kem field was only used to generate the duplicate ecx
nid warning message, so we track duplicates in a local `set` instead.
Additionally, we expand the warning message to all curve NIDs, not just
x-curves and treat duplicates as fatal because extra curve NIDs should
be unique for each KEM.

* Update contributors section of README

Signed-off-by: Felipe Ventura <felipe.ventura@entrust.com>
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.

3 participants