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

Pull FIPS draft versions of Kyber/Dilithium #1537

Closed
wants to merge 5 commits into from
Closed

Pull FIPS draft versions of Kyber/Dilithium #1537

wants to merge 5 commits into from

Conversation

bhess
Copy link
Member

@bhess bhess commented Aug 30, 2023

Pulls standard-version of pq-crystals repo (fips203/204 drafts).

TODOs:

Fixes #1521

  • 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 oqs-provider, OQS-OpenSSL, OQS-BoringSSL, and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

@baentsch
Copy link
Member

stay with kyber/dilithium until FIPS graduates from draft status

+1

@baentsch
Copy link
Member

doesn't include optimized aarch64 versions: either wait for it to land in pqclean or merge without

"merge without" would be my preference to move forward with oqsprovider & interop testing

@bhess bhess force-pushed the bhe-fips204 branch 2 times, most recently from 0c68ac4 to cea8d5f Compare August 30, 2023 17:17
@dstebila
Copy link
Member

Regarding the aarch64 implementation, @mkannwischer is working on that and can provide an update on his path to that.

As for naming, yes, I guess it makes sense to stick with the name Kyber for now and then switch to ML-KEM once the FIPS documents are finalized. @cryptojedi, any opinion?

@cryptojedi
Copy link
Contributor

cryptojedi commented Aug 31, 2023 via email

@baentsch
Copy link
Member

Could also wait until the name is fixed for TLS and/or SSH, right?

As well as the OIDs, yes. (Only) The reference code (with old names/proprietary OIDs) is waited for (integration) e.g., for interop testing.

@baentsch
Copy link
Member

baentsch commented Sep 8, 2023

What about merging them into a(n equally named) "standard" branch in liboqs? That would allow us to ("test-")integrate them in downstream projects (most notably oqsprovider to make progress in interop testing): Arguably, (optimized ARM or actually any) performance doesn't matter there, just functionality. Integration to "main" could be done as and when the upstream projects catch up.

@bhess
Copy link
Member Author

bhess commented Sep 8, 2023

Sounds good to me @baentsch.

For the full PR there are two open points:

@baentsch
Copy link
Member

baentsch commented Sep 8, 2023

Sounds good to me @baentsch.

Alright then -- can you re-target this PR to a different branch ("standard") then? Or just create a separate PR (or branch) for that (that we then maintain until this PR closes for good with the upstream updates merged)?

@bhess
Copy link
Member Author

bhess commented Sep 8, 2023

Alright then -- can you re-target this PR to a different branch ("standard") then? Or just create a separate PR (or branch) for that (that we then maintain until this PR closes for good with the upstream updates merged)?

See #1543. I think the PR here could stay open until completed.

@dstebila
Copy link
Member

dstebila commented Sep 8, 2023

Thanks Basil for getting this ready so quickly. I guess this means we should re-question whether to include this in 0.9.0 or save for 0.10.0. I think it would be nice to get upstream approval of the fixes in the pq-crystals repository, but hopefully that's straightforward. So that would leave us in the position of deciding whether to push this 0.10.0 due the lack of aarch64 optimized implementation. Thoughts? Tagging @cryptojedi and @mkannwischer for input as well.

@baentsch
Copy link
Member

baentsch commented Sep 9, 2023

So that would leave us in the position of deciding whether to push this 0.10.0 due the lack of aarch64 optimized implementation. Thoughts?

In the interest to have release "0.9.0" support all "final" algorithms (and use that in interop testing with other implementations scheduled for end September) I would very much support including this in 0.9.0 (assuming we do this before end September) -- even with the consequence of dropping aarch64 optimizations for now:
a) We still support aarch64 (via ref)
b) This would substantially reduce effort in several projects: Maintaining a separate branch in liboqs (see #1543) as well as all downstreams interested in using this code base having to introduce the same "standard" branch maintenance duplicates effort and
c) creates confusion: "Why are standard algorithms not the default in liboqs?"

What worries me more in this PR though, is that the upstreams do not seem to have a notion of version. This has been updated only locally in #1543: I'd think this either needs to be added also to this PR (with liboqs assuming responsibility for algorithm versioning) -- or ideally upstream.

@baentsch
Copy link
Member

@SWilson4 Please discuss in the next team meeting whether to heed the above (include in 0.9.0 --properly versioned-- or not).

@bhess
Copy link
Member Author

bhess commented Sep 13, 2023

I'd think this either needs to be added also to this PR (with liboqs assuming responsibility for algorithm versioning)

This PR and #1543 have the same source branch, the update is also applied here.

Vadim posted a message that highlights a few inconsistencies/typos in the FIPS 204 draft, compared to the pq-crystals/standard implementation. The algorithms appear to match apart from one minor difference, which can lead to different KAT and signatures in a "literal" implementation of the draft. This might be relevant for interop. I'd therefore tend to state, for example, "FIPS 204 (Draft, August 24, 2023), implemented by pq-crystals/standard" (for spec-version, if we do this in liboqs). Any thoughts?

@dstebila
Copy link
Member

I'd think this either needs to be added also to this PR (with liboqs assuming responsibility for algorithm versioning)

This PR and #1543 have the same source branch, the update is also applied here.

Vadim posted a message that highlights a few inconsistencies/typos in the FIPS 204 draft, compared to the pq-crystals/standard implementation. The algorithms appear to match apart from one minor difference, which can lead to different KAT and signatures in a "literal" implementation of the draft. This might be relevant for interop. I'd therefore tend to state, for example, "FIPS 204 (Draft, August 24, 2023), implemented by pq-crystals/standard" (for spec-version, if we do this in liboqs). Any thoughts?

Do we know if NIST is expected to make this change?

@baentsch
Copy link
Member

I'd therefore tend to state, for example, "FIPS 204 (Draft, August 24, 2023), implemented by pq-crystals/standard" (for spec-version, if we do this in liboqs). Any thoughts?

I'm personally no big fan of version strings with more letters than numbers, let alone some code exegesis. I also wouldn't refer to FIPS if there is a possibility that the FIPS standard could be implemented differently. If pqcrystals doesn't assign a version, what about simply "pqcrystals 200823"?

@dstebila
Copy link
Member

I talked with @cryptojedi this afternoon, and he tells me that there are incompatibilities between the current FIPS drafts and PQCrystals standard branch for both Kyber and Dilithium. He says that they've told NIST of the changes that they think should be made in the FIPS drafts, but there's no confirmation yet whether those changes will be made. And for Dilithium, there's still a change that he thinks should be made that isn't yet in the PQCrystals standard branch.

His suggestion was that it would be better not to include the Kyber and Dilithium updates in the 0.9.0 release, and instead keep them in a separate branch for now which can be tagged for those wanting to use it in the IETF PQC PKI hackathon.

@baentsch
Copy link
Member

His suggestion was that it would be better not to include the Kyber and Dilithium updates in the 0.9.0 release, and instead keep them in a separate branch for now which can be tagged for those wanting to use it in the IETF PQC PKI hackathon.

OK -- then this PR may be kept in Draft until the discussion with NIST concludes. Given there's no date for this and the PR's anyway already in need of re-base, closing it seems the prudent thing to do. Please re-open if you disagree, @bhess .

#1543 just merged to separate (arguably somewhat inaccurately named) "standard" branch. Will follow up with a PR for oqsprovider using that so that can be used in interop testing.

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.

Update Kyber and Dilithium to FIPS versions
4 participants