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

kyber/dilithium aarch64 pull from pqclean + patches #1512

Merged
merged 10 commits into from Aug 4, 2023
Merged

Conversation

bhess
Copy link
Member

@bhess bhess commented Jul 19, 2023

  • 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.)

TODOs:

  • KATs of 2 out of 3 Kyber versions (768, 1024) seem to fail with the updated upstream code.
    @Martyrshot did you face any similar issues when including the previous code?
  • Update license infos

@bhess
Copy link
Member Author

bhess commented Jul 19, 2023

I think I found the cause for the KAT mismatch, will create a patch here and open an issue upstream..

@mczraf
Copy link

mczraf commented Jul 19, 2023

@bhess Thank you for working on the KAT mismatch issue. Quick question: would you have any rough time estimate for the integration of this pull-request? I suspect that the KAT mismatch needs to be fixed first but is there any other blocking issues for this integration?

@bhess
Copy link
Member Author

bhess commented Jul 20, 2023

KATs are now matching and the license infos updated, so the PR seems ready from my side @mczraf . The constant-time tests mentioned in #1320 (comment) are now also passing (tested locally, there isn't a CI CT-run for arm at the moment).

The update appears to include the changes noted in #1245 (comment) (i.e. feat.s), so it should close #1320.

@dstebila There's a small Falcon update as part in this pqclean-pull you wanted to check if it is ok to include.

@baentsch
Copy link
Member

LGTM. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

We should be able to delete this file, the PQClean Makefiles are not needed nor used in liboqs.

Copy link
Member

Choose a reason for hiding this comment

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

As well as the other Makefiles in the PQClean directories.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a commit that removes them.

Copy link
Member

@dstebila dstebila left a comment

Choose a reason for hiding this comment

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

Please remove superfluous Makefiles

@bhess
Copy link
Member Author

bhess commented Jul 23, 2023

Please remove superfluous Makefiles

The makefiles are removed now, updated the copy_from_upstream script to handle this case when there are arch-specific upstreams.

with copy_from_upstream patches
spdx-license-identifier: CC0-1.0
spdx-license-identifier: (CC0-1.0 or Apache-2.0) and (CC0-1.0 or MIT) and MIT
Copy link
Member

Choose a reason for hiding this comment

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

A question regarding this SPDX documentation: Am I assuming right that this has been manually created from the single source code LICENSE files? In addition, is it intentional (and legal) to not include the actual LICENSE file(s) from PQClean? When looking at the contents of those (example: https://github.com/PQClean/PQClean/blob/master/crypto_kem/kyber1024/aarch64/LICENSE) the documented upstream license does not seem to be in line with the SPDX statement in this PR -- or am I overlooking where Apache-2.0 is granted for the whole of the aarch64 kyber code base? Or did PQClean/PQClean#488 simply fail to update the main LICENSE file?

Copy link
Member Author

Choose a reason for hiding this comment

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

The spdx-license-identifier was manually curated looking at the license information in the headers of the individual files. It would be nicer if there was a single LICENSE or NOTICE file upstream containing all this information. The main LICENSE indeed seems out of sync, I don't know if this is intentional. @dstebila, since you reviewed PQClean/PQClean#488, do you have an insight here?

Copy link
Member

Choose a reason for hiding this comment

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

No insight; the PR only modified the individual files, rather than touching the related licence file. Probably an oversight.

Copy link
Member Author

@bhess bhess Jul 26, 2023

Choose a reason for hiding this comment

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

Ok, thx. I'd then add CC0 to the SPDX-"and-chain" until this is potentially updated upstream (or until this is automized).

@dstebila dstebila merged commit be67811 into main Aug 4, 2023
22 checks passed
@dstebila dstebila deleted the bhe-pqclean-pull branch August 4, 2023 20:52
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.

macOS M1: dilithium/aarch64 not compiling with gcc
4 participants