kyber/dilithium aarch64 pull from pqclean + patches#1512
Conversation
|
I think I found the cause for the KAT mismatch, will create a patch here and open an issue upstream.. |
|
@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? |
|
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. |
|
LGTM. Thanks! |
There was a problem hiding this comment.
We should be able to delete this file, the PQClean Makefiles are not needed nor used in liboqs.
There was a problem hiding this comment.
As well as the other Makefiles in the PQClean directories.
There was a problem hiding this comment.
Added a commit that removes them.
dstebila
left a comment
There was a problem hiding this comment.
Please remove superfluous Makefiles
…ript to handle this case
The makefiles are removed now, updated the copy_from_upstream script to handle this case when there are arch-specific upstreams. |
| source: https://github.com/PQClean/PQClean/commit/66e50172055aaf1b9a16d8f35fe03b0807f2723e | ||
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
No insight; the PR only modified the individual files, rather than touching the related licence file. Probably an oversight.
There was a problem hiding this comment.
Ok, thx. I'd then add CC0 to the SPDX-"and-chain" until this is potentially updated upstream (or until this is automized).
TODOs:
@Martyrshot did you face any similar issues when including the previous code?