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

Support more NIDs in KEM table #313

Merged
merged 9 commits into from
Jul 8, 2021
Merged

Conversation

dstebila
Copy link
Member

@dstebila dstebila commented May 28, 2021

This adds extra fields in the generate.yml file to track

  • extra hybrid curves (e.g., x25519)
  • past NID identifiers

Replaces #311. Fixes #312.

If we are happy with this general approach, then before this is merged we will need to:

  • Fill in the remaining Kyber Round 2 previous NIDs
  • Set new NIDs for Kyber Round 3 (using values from s2n, if they've been set)

@dstebila dstebila self-assigned this May 28, 2021
@dstebila dstebila requested a review from xvzcf May 28, 2021 19:58
@dstebila dstebila mentioned this pull request May 28, 2021
oqs-template/generate.yml Outdated Show resolved Hide resolved
oqs-template/generate.yml Show resolved Hide resolved
oqs-template/generate-oid-nid-table.py Show resolved Hide resolved
oqs-template/generate.yml Outdated Show resolved Hide resolved
@alexw91
Copy link

alexw91 commented Jun 7, 2021

BIKE has changed their naming scheme in their Round 3 submission. Before it's scheme was bike1l1fo, now it is just bikel1. Also, I'm not sure if you want to include the BIKE version number of 4.1 or leave it as NIST Round 3 submission.

| Family         | Implementation Version   | Variant         |   Claimed NIST Level | PQ-only Code Point   | Hybrid Elliptic Curve   | Hybrid Code Point   |
| BIKE           | 4.1 (NIST Round 3)       | bikel1          |                    1 | 0x0237               | x25519                  | 0x2F37              |
| BIKE           | 4.1 (NIST Round 3)       | bikel1          |                    1 | 0x0238               | secp256_r1              | 0x2F38              |
| CRYSTALS-Kyber | NIST Round 3 submission  | kyber512        |                    1 | 0x0239               | x25519                  | 0x2F39              |
| CRYSTALS-Kyber | NIST Round 3 submission  | kyber512        |                    1 | 0x023A               | secp256_r1              | 0x2F3A              |

@dstebila
Copy link
Member Author

dstebila commented Jun 7, 2021

BIKE has changed their naming scheme in their Round 3 submission. Before it's scheme was bike1l1fo, now it is just bikel1. Also, I'm not sure if you want to include the BIKE version number of 4.1 or leave it as NIST Round 3 submission.

| Family         | Implementation Version   | Variant         |   Claimed NIST Level | PQ-only Code Point   | Hybrid Elliptic Curve   | Hybrid Code Point   |
| BIKE           | 4.1 (NIST Round 3)       | bikel1          |                    1 | 0x0237               | x25519                  | 0x2F37              |
| BIKE           | 4.1 (NIST Round 3)       | bikel1          |                    1 | 0x0238               | secp256_r1              | 0x2F38              |
| CRYSTALS-Kyber | NIST Round 3 submission  | kyber512        |                    1 | 0x0239               | x25519                  | 0x2F39              |
| CRYSTALS-Kyber | NIST Round 3 submission  | kyber512        |                    1 | 0x023A               | secp256_r1              | 0x2F3A              |

@xvzcf How would you handle variant renaming in the .yml file?

@baentsch
Copy link
Member

baentsch commented Jun 8, 2021

@xvzcf How would you handle variant renaming in the .yml file?

Can I bring up the idea (as already tried in my comment above) to also give a "readable" name (say 'implementation_version') to the current algorithm variant and not just to the "old" ones? But more important I find this question:

Also, I'm not sure if you want to include the BIKE version number of 4.1 or leave it as NIST Round 3 submission

I'd vote to include the algorithm version (first and foremost: I somehow have the feeling that there may be more versions than just one for the same set of NIST R3 KATs)

@baentsch
Copy link
Member

baentsch commented Jun 8, 2021

@dstebila Thanks for adding the handful of s2n NIDs. But this doesn't solve the issue for R3 algs in general: Do we now "make up" our own NIDs for all R3 algs (i.e., those that neither we bumped nor anyone else supports)? How do we keep track of these IDs given we apparently don't have a way to keep them contiguous within alg families?

@xvzcf
Copy link

xvzcf commented Jun 9, 2021

Let me think about this some more.

@alexw91
Copy link

alexw91 commented Jun 14, 2021

Any updates? I wanted to confirm whether or not the OQS project is in favor of merging in the ID's I listed here: #313 (comment) or wanted to have the Python script generate ID's that s2n would then use.

@baentsch
Copy link
Member

One common "source of truth" (for all NIDs, past and present) for OQS-OpenSSL and s2n would be great. OK with YML as the representation? If so, I'll volunteer to "manually curate" the YML (retaining already used IDs as well as creating new ones where R3 changed KATs).

@dstebila
Copy link
Member Author

Any updates? I wanted to confirm whether or not the OQS project is in favor of merging in the ID's I listed here: #313 (comment) or wanted to have the Python script generate ID's that s2n would then use.

Sorry, was on vacation for a few days. I'm fine with using the IDs you've provided in that comment.

@dstebila
Copy link
Member Author

One common "source of truth" (for all NIDs, past and present) for OQS-OpenSSL and s2n would be great. OK with YML as the representation? If so, I'll volunteer to "manually curate" the YML (retaining already used IDs as well as creating new ones where R3 changed KATs).

Are you saying that this would be in the YML file in this PR, or in a different YML file? If in a different file, would the NIDs still be present (duplicated) in this file, or removed from this file and present only in the new file?

@baentsch
Copy link
Member

Are you saying that this would be in the YML file in this PR

Yes, that would be the proposal. We could regularly update/maintain it and oqs-openssl, oqs-provider and (pq-)s2n could extract from it what it needs (& wants to maintain in terms of backwards compatibility).

@dstebila
Copy link
Member Author

Are you saying that this would be in the YML file in this PR

Yes, that would be the proposal. We could regularly update/maintain it and oqs-openssl, oqs-provider and (pq-)s2n could extract from it what it needs (& wants to maintain in terms of backwards compatibility).

Makes sense, go for it.

@baentsch
Copy link
Member

Makes sense, go for it.

All right, that then begs the immediate question to @alexw91: Are you also OK taking code points from this YML or do you want/need to retain the 4 you listed above (i.e., did you already release those)? If the former, keeping numbers contiguous (i.e., change whole "number blocks" for algorithms with different R2+R3 KATs) would seem sensible, but requiring a change in pq-s2n. If the latter, open-quantum-safe/liboqs#887 must close before tackling this issue (so we have BIKE code matching the above in liboqs).

"Implementation" question: Do we consider it sufficient to (now, once) manually check the KEM algorithms as to whether KATs changed between R2 (i.e., liboqs v0.4.0, right?) and R3 (current code) to decide whether we need new code points/IDs -- and think about updating code points at the next "interop breaking" liboqs update? Or shall we decide that "by program", e.g., adding a hash of an algorithm's KATs to the openssl-generate.yml to always be made aware of the need to update code points in the future? If the latter, does someone have a better idea than adding & checking liboqs' (current) KATs when running generate.py?

@baentsch
Copy link
Member

Answering my own question above: Manual checking is no fun, so I added logic to automatically check KATs. It can be triggered as follows: python3 oqs-template/generate.py ../liboqs/tests/KATs/kem/kats.json oqs-template/v040kemkats.json resulting in

Different KATs for kyber512: Code point update needed
Different KATs for kyber768: Code point update needed
Different KATs for kyber1024: Code point update needed
Different KATs for ntru_hps2048509: Code point update needed
Different KATs for ntru_hps2048677: Code point update needed
Different KATs for ntru_hps4096821: Code point update needed
No KAT for KEM sidhp434: New code point needed
No KAT for KEM sidhp503: New code point needed
No KAT for KEM sidhp610: New code point needed
No KAT for KEM sidhp751: New code point needed
Different KATs for kyber90s512: Code point update needed
Different KATs for kyber90s768: Code point update needed
Different KATs for kyber90s1024: Code point update needed
No KAT for KEM hqc128: New code point needed
No KAT for KEM hqc192: New code point needed
No KAT for KEM hqc256: New code point needed
No KAT for KEM ntrulpr653: New code point needed
No KAT for KEM ntrulpr761: New code point needed
No KAT for KEM ntrulpr857: New code point needed
No KAT for KEM sntrup653: New code point needed
No KAT for KEM sntrup761: New code point needed
No KAT for KEM sntrup857: New code point needed

--> Does this look reasonable? OK with the approach to add liboqs KAT hashes to generate.yml to warn about incompatibilities? Why don't we have KATs for SIDH? How then do we decide whether or not a code point update is warranted for SIDH?

@dstebila
Copy link
Member Author

Answering my own question above: Manual checking is no fun, so I added logic to automatically check KATs. It can be triggered as follows: python3 oqs-template/generate.py ../liboqs/tests/KATs/kem/kats.json oqs-template/v040kemkats.json resulting in

Different KATs for kyber512: Code point update needed
Different KATs for kyber768: Code point update needed
Different KATs for kyber1024: Code point update needed
Different KATs for ntru_hps2048509: Code point update needed
Different KATs for ntru_hps2048677: Code point update needed
Different KATs for ntru_hps4096821: Code point update needed
No KAT for KEM sidhp434: New code point needed
No KAT for KEM sidhp503: New code point needed
No KAT for KEM sidhp610: New code point needed
No KAT for KEM sidhp751: New code point needed
Different KATs for kyber90s512: Code point update needed
Different KATs for kyber90s768: Code point update needed
Different KATs for kyber90s1024: Code point update needed
No KAT for KEM hqc128: New code point needed
No KAT for KEM hqc192: New code point needed
No KAT for KEM hqc256: New code point needed
No KAT for KEM ntrulpr653: New code point needed
No KAT for KEM ntrulpr761: New code point needed
No KAT for KEM ntrulpr857: New code point needed
No KAT for KEM sntrup653: New code point needed
No KAT for KEM sntrup761: New code point needed
No KAT for KEM sntrup857: New code point needed

--> Does this look reasonable?

Approach is reasonable, though not all KAT updates necessarily mean incompatibility, as it could be the implementation changed how it uses randomness internally (resulting in different KATs) but is still interoperable with the previous version.

OK with the approach to add liboqs KAT hashes to generate.yml to warn about incompatibilities?

Yes, great idea.

Why don't we have KATs for SIDH? How then do we decide whether or not a code point update is warranted for SIDH?

SIDH is the IND-CPA version of SIKE. At this point we are preferring IND-CCA over IND-CPA versions when available, so that would speak to including only SIKE, not SIDH.

@alexw91
Copy link

alexw91 commented Jun 16, 2021

@alexw91: Are you also OK taking code points from this YML or do you want/need to retain the 4 you listed above (i.e., did you already release those)?

I'd prefer to use the ones we proposed, but we haven't merged those ID's into s2n yet so we can still change them if necessary.

@baentsch
Copy link
Member

I'd prefer to use the ones we proposed

OK, will do my best to retain them; will let you know if it's getting too tedious.

@baentsch
Copy link
Member

but is still interoperable with the previous version.

OK, we could leave it as-is above: Just warnings output. But then again, which of these "alerts" now warrant a real change of code point? I kind of would prefer as much automation as possible.

so that would speak to including only SIKE, not SIDH.

So shall we remove SIDH from generate.yml entirely? Or comment out? Retain code points "reserved"?

@dstebila
Copy link
Member Author

@alexw91: Are you also OK taking code points from this YML or do you want/need to retain the 4 you listed above (i.e., did you already release those)?

I'd prefer to use the ones we proposed, but we haven't merged those ID's into s2n yet so we can still change them if necessary.

Hi Alex, Eric said that this is blocking you on your end. Please go ahead with your IDs and we will match them.

@dstebila
Copy link
Member Author

but is still interoperable with the previous version.

OK, we could leave it as-is above: Just warnings output. But then again, which of these "alerts" now warrant a real change of code point? I kind of would prefer as much automation as possible.

so that would speak to including only SIKE, not SIDH.

So shall we remove SIDH from generate.yml entirely? Or comment out? Retain code points "reserved"?

I'd want to keep documentation that we used those code points for SIDH in the past and not reuse, but am not sufficiently aware of the semantics of the generate.yml to know what keeping unused SIDH entries in there would imply.

@mrbluecoat
Copy link

Also, any reason you've only chosen to implement the Level 1 variant?

defs.h

// UNCOMMENT TO SELECT THE NIST SECURITY LEVEL 1, 3 OR 5:
#define PARAM64 // NIST LEVEL 1
//#define PARAM96 // NIST LEVEL 3
//#define PARAM128 // NIST LEVEL 5

@baentsch
Copy link
Member

baentsch commented Jul 1, 2021

Hi Alex, Eric said that this is blocking you on your end. Please go ahead with your IDs and we will match them.

@dstebila I know we said in the call this week again that we keep waiting for s2n; however, with the removal of BIKE R2 code, all OQS subprojects now fail in CI. Thus, the downstream "logjam" gets bigger and bigger: No demos aligned with liboqs v0.6.0 can be built since its release nearly a month ago; no test server update is possible; docker images are now pretty much outdated; all app builds now fail.

@alexw91, can you make a statement when you plan to merge the new IDs so we can match as per the above and move forward, too? If this wouldn't be in the next few days (?), I'd like to propose doing a separate PR to this one to move forward here. Rationale: We've stalled on this for a month now and in 2 weeks time I'll probably not be the only one to go on vacation for some time. I'd personally feel bad not having done my best to ensure all OQS "downstream" projects are not in "red build status" for all summer.

If OK for both of you (?), we can then leave this PR "lingering" until s2n settles and/or a standard document gets written.

@alexw91
Copy link

alexw91 commented Jul 1, 2021

@alexw91, can you make a statement when you plan to merge the new IDs

I am hoping to have these ID's merged in s2n today or tomorrow.

@dstebila
Copy link
Member Author

dstebila commented Jul 1, 2021 via email

@baentsch
Copy link
Member

baentsch commented Jul 2, 2021

Great! I'll get things rolling on our side over the weekend or on Monday.

Thanks! FWIW (as I just did (forget) that initially in openssh) please remember to remove all references to OQS_SIG_alg_default and OQS_KEM_alg_default too.

@alexw91
Copy link

alexw91 commented Jul 2, 2021

s2n has merged the new algorithm ID's for Round 3 KEM's that we plan to use: aws/s2n-tls#2842

Algorithm ID's are located at: https://github.com/aws/s2n-tls/pull/2842/files#diff-1dd84e416725429d8eaf7eb4b5feaa0221afd308fe5e4027cf28b2b1ef9c2882R84-R87

@dstebila
Copy link
Member Author

dstebila commented Jul 6, 2021

I've updated the NIDs based on the values in the landed s2n code. This is just for the table, I haven't run the full code generation. I also haven't updated based on the algorithm changes in upstream liboqs. I'm feeling a bit overwhelmed on this and unfortunately have another project on my plate for the rest of the week, @xvzcf and @baentsch can you help?

@baentsch baentsch assigned baentsch and unassigned dstebila Jul 6, 2021
@baentsch
Copy link
Member

baentsch commented Jul 6, 2021

AFAIK all required changes are now done; failure is with boringssl interop -- not surprisingly: @xvzcf Please check open-quantum-safe/boringssl#65 : I think all required changes there are also done, but I keep getting failures generating the crypto objects... :-(

@dstebila dstebila marked this pull request as ready for review July 6, 2021 14:03
@dstebila
Copy link
Member Author

dstebila commented Jul 6, 2021

That looks great, Michael! Only thing I can think of that's left is to sort out the Kyber round 2 versus round 3 NIDs. Since Kyber changed in an incompatible way for Round 3, we would bump new NIDs for the current Kyber and archive the previous NIDs as being for round 2.

@baentsch
Copy link
Member

baentsch commented Jul 7, 2021

bump new NIDs for the current Kyber and archive the previous NIDs as being for round 2

Is it really worth while archiving something that doesn't have (nor need, except for a handful of legacy s2n code points) to be remembered? For all I know this is a range of pretty much unmaintained IDs without a supported code base implementing them nor a specification defining them; further, this might make people think we'd be willing to maintain also old code as --unlike OIDS-- that's the only place for old KEM code points to be of relevance (if we disregard NSA logging TLS session setups for future decipherment :-)

Whatever: I chose to merge boringssl first to see that we got it all right then updated all Kyber code points as per your request with the latest commit. If you're all OK with the IDs chosen, please approve first open-quantum-safe/boringssl#66, then re-run only the last step of the now again failing interop test and if it then passes as expected, please approve and merge this PR.

@baentsch
Copy link
Member

baentsch commented Jul 7, 2021

@dstebila @xvzcf Please check and approve: CI passed as per this. I just can't seem to trigger a re-run for the proper github job to make this visible here. Thanks in advance.

oqs-template/oqs-sig-info.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@baentsch
Copy link
Member

baentsch commented Jul 8, 2021

@xvzcf You seem to be the most suitable person that can approve this PR now that both @dstebila and @baentsch contributed and vacation time is on: So could you please review (and mark the questions above resolved if you agree with my assessment and fix)?

@dstebila dstebila merged commit cbdb793 into OQS-OpenSSL_1_1_1-stable Jul 8, 2021
@dstebila dstebila deleted the more-nids branch July 8, 2021 14:15
@dstebila
Copy link
Member Author

dstebila commented Jul 8, 2021

Thanks @baentsch! I've gone ahead with merging since you and I have both looked it over.

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.

Validate/ensure OID/NID consistency
5 participants