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

Update KEM code points to IANA compliance (#561) #644

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RodriM11
Copy link
Contributor

PR to update KEM code points to "Reserved for private use" values in https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8, as specified in #561.
Also, generate.py includes modifications to perform future additions of code points automatically, i.e., nids for pure PQC as well as any hybrid desired are automatically filled with the next available code point, when left blank.

@RodriM11
Copy link
Contributor Author

Failing/skipped CI tests are due to the need of installing the additional dependency needed to run generate.py.

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.

Thanks for this update/fix @RodriM11 ! See some comments/questions whether the formatting can be improved for readability. Also, is it correct that all SIG code points are not in need of updating? Finally, what needs to be done to allow CI to pass? Please rebase first to check whether the problem then is gone.

'code_point': '0xfea2'}]
-
name: 'dilithium3'
mix_with: [{'name': 'p256', 'pretty_name': 'ECDSA p256', 'oid': '1.3.9999.2.7.1',
Copy link
Member

Choose a reason for hiding this comment

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

why this reformatting? Makes it hard to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is part of the formatting that happens when automatizing NID assignment.

'security': '128', 'oid': '2.16.840.1.114027.80.8.1.3', 'code_point': '0x090a'},
{'name': 'p256', 'pretty_name': 'ECDSA p256', 'security': '128', 'oid': '2.16.840.1.114027.80.8.1.4',
'code_point': '0x0907'}, {'name': 'bp256', 'pretty_name': 'ECDSA brainpoolP256r1',
'security': '256', 'oid': '2.16.840.1.114027.80.8.1.5', 'code_point': '0xfee5'}]
Copy link
Member

Choose a reason for hiding this comment

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

These code points are all in an OK range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, SIG code points were already onto the "Reserved for private use" range (0xfe00 - 0xffff), except those pertaining to DSA and certain combiners, which follow https://datatracker.ietf.org/doc/draft-reddy-tls-composite-mldsa/01/.

What I didn't do though was to automatize these assignments also. I can proceed to add it in the same way as KEMs, if the development for KEMs of this PR is acceptable.

'security': '128', 'oid': '2.16.840.1.114027.80.8.1.8', 'code_point': '0x0908'},
{'name': 'bp256', 'pretty_name': 'ECDSA brainpoolP256r1', 'security': '256',
'oid': '2.16.840.1.114027.80.8.1.9', 'code_point': '0xfee9'}, {'name': 'ed25519',
'pretty_name': 'ED25519', 'security': '128', 'oid': '2.16.840.1.114027.80.8.1.10',
Copy link
Member

Choose a reason for hiding this comment

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

very hard to read -- is this due to some automated reformatting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same. Part of the formatting of the automatic procedures. I was unable to avoid those modifications.

@RodriM11
Copy link
Contributor Author

For CI tests to run, the Python dependency I used for modifying generate.yml with minimal disruption would need to be installed (i.e. ruamel.yaml)

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.

2 participants