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 oqs-kem-info.md #311

Closed

Conversation

alexw91
Copy link

@alexw91 alexw91 commented May 26, 2021

I think there may have been a bug in the script that was used to generate this table. The algorithm identifiers in this table match those in this previous spreadsheet which had all Round 2 labels.

This table should match the data that the old spreadsheet had, so I manually changed the labels on this table to use Round 2 labels so that when new Round 3 labels are added in the future they won't conflict.

@baentsch
Copy link
Member

If memory serves, we intentionally didn't change KEM alg IDs when integrating round 3 code as a) those IDs don't create persistent manifestations (unlike sig-alg OIDs in certs) and b) didn't want to maintain round 2 algorithm support.

@dstebila, @christianpaquin @xvzcf : Is this also your recollection? If so, this PR needs to be closed. If not, we should instead bump the NIDs of all KEMs where we're at R3 level (all?) -- modifying both this PR as well as generate.yml (and do a new 0.6.0 RC, removing reference to the stale spreadsheet, making https://github.com/open-quantum-safe/openssl/blob/OQS-OpenSSL_1_1_1-stable/oqs-template/generate.yml the "authoritative NID source").

@dstebila
Copy link
Member

If memory serves, we intentionally didn't change KEM alg IDs when integrating round 3 code as a) those IDs don't create persistent manifestations (unlike sig-alg OIDs in certs) and b) didn't want to maintain round 2 algorithm support.

@dstebila, @christianpaquin @xvzcf : Is this also your recollection? If so, this PR needs to be closed. If not, we should instead bump the NIDs of all KEMs where we're at R3 level (all?) -- modifying both this PR as well as generate.yml (and do a new 0.6.0 RC, removing reference to the stale spreadsheet, making https://github.com/open-quantum-safe/openssl/blob/OQS-OpenSSL_1_1_1-stable/oqs-template/generate.yml the "authoritative NID source").

No, my recollection is that we did change NIDs when there was an algorithmically incompatible change between Round 2 and Round 3 versions.

@baentsch
Copy link
Member

No, my recollection is that we did change NIDs when there was an algorithmically incompatible change between Round 2 and Round 3 versions.

If this were so, this PR would be moot, right? Would it make sense to compare an old NID spreadsheet with the current generate.yml to ascertain this? Or did you already do this @alexw91 (causing you to create the PR in the first place)?

@alexw91
Copy link
Author

alexw91 commented May 27, 2021

I'm only familiar with BIKE, SIKE, and Kyber. SIKE Round 3 is backwards compatible with it's Round 2 version, while BIKE and Kyber are not. I'm not sure if FrodoKEM or SABER are backwards compatible.

I also agree that we should create new algorithm ID's for each new round that doesn't have backwards compatibility. AWS has already released client SDK's that can negotiate Round 2 versions of BIKE, SIKE, and Kyber. We don't want to break those clients by changing the algorithm ID's to mean the Round 3 variants on the server side, while the clients that haven't migrated yet still think those ID's mean Round 2.

Since Kyber Round 3 is not backwards compatible with Round 2, this means that the changes in this pull request for Kyber at least are applicable, and should be applied. 0x2F0F meant secp256r1_kyber-512-r2 according to the previous spreadsheet, while this table now indicates that it means secp256r1_kyber-512-r3. New ID's for Kyber Round 3 should be created since we don't want to change the meaning of the ID after it was released and break clients.

I am not sure if FrodoKEM or SABER are cross compatible between their Round 2 and Round 3 versions, so I'm not certain if we can reuse their Round 2 ID's for Round 3.

@dstebila
Copy link
Member

I'm only familiar with BIKE, SIKE, and Kyber. SIKE Round 3 is backwards compatible with it's Round 2 version, while BIKE and Kyber are not. I'm not sure if FrodoKEM or SABER are backwards compatible.

FrodoKEM Round 3 is unchanged from Round 2.

SABER KATs haven't changed in the past 2 years (1, 2, 3).

@alexw91
Copy link
Author

alexw91 commented May 27, 2021

Just realized that I missed the NTRU algorithm.

Does anyone know if NTRU ntru_hps2048509, ntru_hps2048677, ntru_hps4096821, and ntru_hrss701 are backwards compatible between Round 2 and Round 3?

https://github.com/open-quantum-safe/openssl/blame/9e342427ead2945c7a757e8497d3d1cb7ae6d578/oqs-template/oqs-kem-info.md#L24-L27

@dstebila
Copy link
Member

Just realized that I missed the NTRU algorithm.

Does anyone know if NTRU ntru_hps2048509, ntru_hps2048677, ntru_hps4096821, and ntru_hrss701 are backwards compatible between Round 2 and Round 3?

https://github.com/open-quantum-safe/openssl/blame/9e342427ead2945c7a757e8497d3d1cb7ae6d578/oqs-template/oqs-kem-info.md#L24-L27

Tagging @jschanck

@jschanck
Copy link

Does anyone know if NTRU ntru_hps2048509, ntru_hps2048677, ntru_hps4096821, and ntru_hrss701 are backwards compatible between Round 2 and Round 3?

There were no changes to NTRU in Round 3.

@alexw91
Copy link
Author

alexw91 commented May 27, 2021

Updated the table to list which NIST Rounds correspond to which identifiers. This change only updates the existing rows in the table, and does not add any new rows yet. Once this is merged, I can open a 2nd PR to add the new Round 3 identifiers for BIKE and Kyber.

@baentsch
Copy link
Member

Once this is merged, I can open a 2nd PR to add the new Round 3 identifiers for BIKE and Kyber.

Why not do the update in this PR? By merging this PR as-is we'd have a documented inconsistency in the system for BIKE and Kyber: This new markdown file version contradicts the generate.yml (as well as the actual, running --R3-- code) and might indicate that oqs-openssl maintains both R2 and R3 code versions (which I doubt we want to do). Second question: If you already have R3-NIDs for BIKE and Kyber defined what speaks against adding them here (and in generate.yml to ensure consistency)? Whatever the decision where to fix it (in this PR or another one), #312 should be used to track/document we bring IDs in line with code bases before a next release.

@alexw91
Copy link
Author

alexw91 commented May 28, 2021

Why not do the update in this PR?

I'm happy to include the new NID's for Round 3 BIKE and Kyber in this same PR if everyone is okay with that. The reason I originally didn't do both changes in this PR was to separate the concerns that people might bring up during review around fixing the values already in the table, versus adding new proposed values to the table. But if there aren't separate concerns, I can do both here.

If you already have R3-NIDs for BIKE and Kyber defined what speaks against adding them here

We picked temporary placeholder ID's in this s2n Pull Request by manually counting past the current maximum values defined by OQS. These ID's have not been merged into s2n yet, and we wanted to make sure that both OQS and s2n agree on the new ID's that we're proposing before merging them into s2n. We have been treating the spreadsheet as the source of truth and originally attempted to add them there, but were redirected to this markdown table.

@dstebila dstebila mentioned this pull request May 28, 2021
2 tasks
@dstebila
Copy link
Member

I've proposed a way of encoding this in the .yml file and generating an updated Markdown file in #313. Please take a look, @alexw91 @baentsch @xvzcf.

@baentsch
Copy link
Member

Please take a look

Done; feedback provided there.

We picked temporary placeholder ID's in this s2n Pull Request by manually counting past the current maximum values defined by OQS. These ID's have not been merged into s2n yet, and we wanted to make sure that both OQS and s2n agree on the new ID's that we're proposing before merging them into s2n.

Thanks for the pointer: Very much appreciated. open-quantum-safe/oqs-demos#83 created to ensure the interop server will reference the right file as and when created.

@dstebila
Copy link
Member

dstebila commented Jun 1, 2021

@alexw91 Regarding the NIDs in the s2n pull request, do you intend to keep different NIDs for SIKE R2 and R3? I thought those were interoperable.

@alexw91
Copy link
Author

alexw91 commented Jun 3, 2021

We are planning to re-use the SIKE Round 2 NID as the SIKE Round 3 NID. However, that change is not reflected in the s2n Pull Request yet, and the PR is still currently using a new placeholder NID for SIKE Round 3.

We're working on removing SIKE Round 2 and move to SIKE Round 3 in this PR: aws/s2n-tls#2864

@dstebila
Copy link
Member

dstebila commented Jun 3, 2021

We are planning to re-use the SIKE Round 2 NID as the SIKE Round 3 NID. However, that change is not reflected in the s2n Pull Request yet, and the PR is still currently using a new placeholder NID for SIKE Round 3.

We're working on removing SIKE Round 2 and move to SIKE Round 3 in this PR: aws/s2n-tls#2864

Okay. Do you consider the other numbers in your PR to be the ones you want to use? Or are you waiting for us to pick numbers? I'm fine either way, just don't want both of us thinking the other will make the choice.

@alexw91
Copy link
Author

alexw91 commented Jun 5, 2021

Let's use the ID's that we have in our s2n PR.

#define TLS_PQ_KEM_GROUP_ID_X25519_BIKE_L1_R3       0x2F37
#define TLS_PQ_KEM_GROUP_ID_SECP256R1_BIKE_L1_R3    0x2F38
#define TLS_PQ_KEM_GROUP_ID_X25519_KYBER_512_R3     0x2F39
#define TLS_PQ_KEM_GROUP_ID_SECP256R1_KYBER_512_R3  0x2F3A

@baentsch
Copy link
Member

baentsch commented Jun 5, 2021

Are these really the only IDs/algorithms you support? No Kyber768 or 1024? No KyberAES variants? No other hybrids? Or if there's more, could you please point to the pq-s2n github location(s) where one could find them all? Our full list here. Thanks in advance.

@alexw91
Copy link
Author

alexw91 commented Jun 7, 2021

Yes, s2n only supports these combinations when using TLS 1.3:

  • EC Curve: {x25519, secp256r1}
  • PQ KEM: {sikep434, bike1_l1, kyber512}
  • NIST Round: {Round 2, Round 3} (Round 3 currently in development)

s2n also supports Rounds 1-3 when using TLS 1.2 to keep backwards compatibility from when we launched Round 1 BIKE and SIKE support in TLS 1.2.

@dstebila
Copy link
Member

dstebila commented Jun 7, 2021

Let's use the ID's that we have in our s2n PR.

#define TLS_PQ_KEM_GROUP_ID_X25519_BIKE_L1_R3       0x2F37
#define TLS_PQ_KEM_GROUP_ID_SECP256R1_BIKE_L1_R3    0x2F38
#define TLS_PQ_KEM_GROUP_ID_X25519_KYBER_512_R3     0x2F39
#define TLS_PQ_KEM_GROUP_ID_SECP256R1_KYBER_512_R3  0x2F3A

Okay, I've added these to the table in #313. I'm going to close this PR, please do further discussion on #313.

@dstebila dstebila closed this Jun 7, 2021
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.

4 participants