Update copy_from_pqclean / copy_from_upstream

[bhess]

Updates copy_from_pqclean / copy_from_upstream (#880)

• Renames copy_from_pqclean to copy_from_upstream.

• Adds 'upstreams' field to copy_from_upstream.yml, allowing to specify upstream repositories, branches and commits.

upstreams:
  -
    name: pqclean
    git_url: https://github.com/PQClean/PQClean.git
    git_branch: master
    git_commit: 3d7d2024fa892bd7f00dca3fff4122175f4a26dc
    kem_meta_path: 'crypto_kem/{pqclean_scheme}/META.yml'
    sig_meta_path: 'crypto_sign/{pqclean_scheme}/META.yml'
    kem_scheme_path: 'crypto_kem/{pqclean_scheme}'
    sig_scheme_path: 'crypto_sign/{pqclean_scheme}'
  -
    name: pqcrystals-kyber
    git_url: https://github.com/pq-crystals/kyber.git
    git_branch: master
    git_commit: 8c8d7da74ee92808eebb6128ca209fd5851f48dc
    kem_meta_path: '{pretty_name_full}_META.yml'
    kem_scheme_path: '.'
  -
    name: pqcrystals-dilithium
    git_url: https://github.com/pq-crystals/dilithium.git
    git_branch: master
    git_commit: bda9689704262b53f0942efdda6340b922acb58b
    sig_meta_path: '{pretty_name_full}_META.yml'
    sig_scheme_path: '.'
...

• Modifies "copy" command in copy_from_upstream: sources are pulled from specified git-repositories. Implementation folders in $LIBOQS_DIR/src will be prefixed with the upstream-name.

• Adds "verify" command in copy_from_upstream: Implementations in $LIBOQS_DIR/src are compared with expected upstream versions. Implementations that differ from upstream will be hightlighted.

• Prepares for copying pqclean, pqcrystals-kyber and pqcrystals-dilithium (Round 3 versions) from upstream.

How to use copy_from_upstream:

$ ./copy_from_upstream.py --help
usage: copy_from_upstream.py [-h] [-v VERBOSITY] [-k] {copy,verify}

positional arguments:
  {copy,verify}

optional arguments:
  -h, --help            show this help message and exit
  -v VERBOSITY, --verbosity VERBOSITY
  -k, --keep_data

Assumed that pqclean_saber_avx2 differs from upstream, the output will be as follows:

$ ./copy_from_upstream.py verify
-----
Total schemes: 151 - 150 match with upstream, 1 differ
-----
Schemes that differ from upstream:
Name: pqclean_saber_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 3d7d2024fa892bd7f00dca3fff4122175f4a26dc
-----

If any verification failed, the script returns with an exit code (1).

• 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 the list of algorithms available -- either adding or removing? (If so, PRs in OQS-OpenSSL, OQS-BoringSSL, and OQS-OpenSSH will also be required by the time this is merged.)

-> This PR doesn't tick the two boxes above, but after running ./copy_from_upstream, the two boxes will be ticked because of new Dilithium Round 3 schemes.

[bhess]

Some more tweaks:

• test_spdx.sh and run_astyle.sh are modified to exclude checks from all implementation folders in $LIBOQS_DIR/kem and $LIBOQS_DIR/kem
• test_portability.py, which only runs on Ubuntu, checks for "Ubuntu" in platform.platform(). On my machine (Ubuntu 24.04LTS), the string appears in platform.version() instead.

[baentsch]

First impression: Looks good. I assume you also already tested this after importing the current upstreams? Could you/did you run CCI local (say, circleci local execute --job ubuntu-bionic-clang9) successfully after such execution?

I'd further recommend ticking the checkboxes as a way to warn us that running this script will necessitate a new version id: New algs (dil5) and new naming is introduced with this.

[bhess]

Thanks for the feedback. I edited the checkboxes to clarify that they apply after the script is run with the proposed configuration.

CCI local (circleci local execute --job ubuntu-bionic-clang9) runs successfully when importing the upstreams. Will need to revert the test_portability.py script to let them run on Bionic. Only Kyber and Dilithium will be updated after the new import. I'd suggest to do this in another PR. The pqclean-implementations already match the configuration.

[baentsch]

I'd suggest to do this in another PR.

Very much agreed.

CCI local (circleci local execute --job ubuntu-bionic-clang9) runs successfully

Very good. Although, ubuntu-bionic-clang9 is just one of the battery of CI tests... CCI will run them all and local testing helps minimize iterations to get ready for review.

@dstebila : Would you (agree to) add @bhess to the access group for running CCI?

[dstebila]

@dstebila : Would you (agree to) add @bhess to the access group for running CCI?

I've sent an invitation to add @bhess to the Core team on the GitHub OQS organization. I think that will be enough to get CircleCI running on Basil's PRs, but I'm not certain of the configuration.

[baentsch]

Thanks for getting CCI to pass. Please click "Ready for review" if you don't want to change sth else. I'd suggest requesting @dstebila @xvzcf @baentsch as possible reviewers.

[bhess]

I am still checking a few issues with the new upstreams when building liboqs as a shared library, especially if the upstream uses common code under the same namespace.

It could make sense to let the upstreams define what their common code is, and to build this part separately.

For common code such as AES, FIPS202, etc., I see that liboqs provides common code and shim API for pqclean, which is nice. Perhaps these shims could be extended to support other upstreams if necessary.

[dstebila]

For common code such as AES, FIPS202, etc., I see that liboqs provides common code and shim API for pqclean, which is nice. Perhaps these shims could be extended to support other upstreams if necessary.

Yes, ideally we'd shim all the common code on to liboqs providers (which in some cases pass through to OpenSSL).

[bhess]

I've pushed another commit that adds the possibility to declare common code used across implementations in an upstream location. This will avoid issues with duplicate symbols if the code is replicated for each implementation. It also reduces code duplication in the imported sources.

I think that ideally, common code would be provided centrally by liboqs as shims. But I also see that can be beneficial for upstreams to define their own common code without having to change the liboqs core code (shims). So I see this as complementary to the code provided in src/common.

An example for pqcrystals-dilithium:

The upstream can define a .yml-file to define common code. This file is defined in a field common_meta_path in copy_from_upstream.yml. If there is no such field defined, the import behavior is the same as before:

upstreams:
...
  -
    name: pqcrystals-dilithium
...
    common_meta_path: 'Common_META.yml'
...

The Common_META.yml can look as follows:

commons:
  - name: common_ref
    folder_name: ref
    sources: aes256ctr.c aes256ctr.h fips202.c fips202.h
  - name: common_aes
    folder_name: avx2
    sources: aes256ctr.c aes256ctr.h
    supported_platforms:
      - architecture: x86_64
        operating_systems:
          - Darwin
          - Linux
        required_flags:
          - aes
          - sse2
          - ssse3
  - name: common_avx2
    folder_name: avx2
    sources: f1600x4.S fips202.c fips202.h fips202x4.c fips202x4.h
    supported_platforms:
      - architecture: x86_64
        operating_systems:
          - Darwin
          - Linux
        required_flags:
          - avx2
          - popcnt

The implementation .yml can then define its dependencies:

name: Dilithium2-AES
...
  - name: avx2
...
    common_dep: common_avx2 common_aes
...

In this case, copy_from_upstream will copy common dependencies to src/sig/dilithium/pqcrystals-dilithium_<name> for <name> as defined in Common_META.yml.

CMake definitions for the common code will be added to src/sig/dilithium/CMakeLists.txt:

if(OQS_ENABLE_SIG_dilithium_2_aes_avx2 OR OQS_ENABLE_SIG_dilithium_3_aes_avx2 OR OQS_ENABLE_SIG_dilithium_5_aes_avx2)
    add_library(dilithium_common_aes OBJECT pqcrystals-dilithium_common_aes/aes256ctr.c)
    target_include_directories(dilithium_common_aes PRIVATE ${CMAKE_CURRENT_LIST_DIR}/pqcrystals-dilithium_common_aes)
    target_compile_options(dilithium_common_aes PRIVATE -maes -msse2 -mssse3)
    set(_DILITHIUM_OBJS ${_DILITHIUM_OBJS} $<TARGET_OBJECTS:dilithium_common_aes>)
endif()

The if-guard ensures that the common code is only compiled if an implementation is selected that uses it.

Note: for now the pqcrystals upstream-locations in copy_from_upstream.yml point to my fork. Running copy_from_upstream should be functional with this configuration, local circleci builds run fine with the imports. I will open a PR to pqcrystals after feedback here.

[baentsch]

Upstream common code does make sense. It both allows code-sharing and (e.g., CPU feature) optimization to be done across several algorithms and further can help reduce the number of runtime portability guards. Using the same .yml definition mechanism also eases understanding and further re-use.

It would be ideal if the goal for such code would be to eventually be of interest to more QSC algorithms than one "family" (say pqcrystals); if you'd agree, I'd then suggest such common upstream code to follow both contributing standards: Those for new algorithms and those for OQS common code.

[bhess]

I'm not sure if the "OQS common code" standards are easy to ensure with the automatic import, especially the code formatting rules.

Would the following make sense? the OQS common code and new algorithms standards should apply to code contributed to src/common. For code automatically imported to src/<family>/<algorithm>/<upstream-common-code-dir>, only the "new algorithms" standards should apply.

[dstebila]

I'm a bit confused here. Are you talking about having different implementations of AES, SHA, ... in src/common that are pulled from upstream sources automatically? Or are you talking about having implementations of AES, SHA, ... pulled into subdirectories of src/<family>/...?

[baentsch]

Would the following make sense? the OQS common code and new algorithms standards should apply to code contributed to src/common. For code automatically imported to src///, only the "new algorithms" standards should apply.

The current wording for common code (requirements) explicitly refers only to stuff in src/common and excludes code automatically added. The statements in my comment above were mere suggestions trying to help make "family-common code" become more attractive to wider audiences but in no way a request for that.

[bhess]

Sorry if there was a confusion. I only refer to common code pulled into subdirectories of src/<family>/....
@baentsch Then I fully agree.

[bhess]

Squashed the commits. In the current state, it is ready to pull from pqclean, pqcrystals-kyber and pqcrystals-dilithium.

[baentsch]

LGTM -- particularly considering the "test" via #891