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

Merge integrate-pq into main #897

Merged
merged 76 commits into from
May 16, 2023
Merged

Conversation

WillChilds-Klein
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein commented Mar 22, 2023

Issues:

Partially addresses CryptoAlg-1665

Notes

There were a few issues in the merge that I had to manually resolve:

  1. NID conflict on 973 for Dilithium3 and Kyber768. I resolved this by
    giving Dilithium3 an NID of 975 instead (Kyber1024 is 974).
    • After updating nid.h, I re-ran util/generate_build_files.py.
    • After updating files in crypto/obj, I reran the objects.go script.
  2. Dilithium3's EVP_PKEY_ASN1_METHOD struct definition was slightly
    outdated and didn't have a priv_encode_v2 implementation. So, I gave
    it a trivial implementation that returns error, specifically EVP err
    EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE. We may want to come
    back and implement this method which "encodes |key| as a
    OneAsymmetricKey (RFC 5958)".

To simplify this PR's diff and facilitate reviewing, some portions of the merge
contents have been deleted in discrete commits. These deletions are temporary,
and will be reverted immediately post-merge in a two fast-follow PRs:

  1. Reinstate Dilithium KATs and fuzzing corpus
  2. Reinstate Dilithium documentation, and adjust the example code per
    @dkostic's guidance

Regarding the code remaining in this PR, much of it has been reviewed
previously. Here's a summary of the PRs for Dilithium-related code:

Testing

  • CI checks
  • local testing for the feature guard

Against test binary built with default build flags (i.e. ENABLE_DILITHIUM=OFF):

$ /build/crypto/crypto_test --gtest_filter='Dilithium3Test.*'
...
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from Dilithium3Test
[ RUN      ] Dilithium3Test.EvpDisabled
[       OK ] Dilithium3Test.EvpDisabled (0 ms)
[----------] 1 test from Dilithium3Test (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (0 ms total)
[  PASSED  ] 1 test.

Against test binary built with -DENABLE_DILITHIUM=ON set in cmake invocation:

$ /build/crypto/crypto_test --gtest_filter='Dilithium3Test.*'
...
[==========] Running 7 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 7 tests from Dilithium3Test
[ RUN      ] Dilithium3Test.KeyGeneration
[       OK ] Dilithium3Test.KeyGeneration (0 ms)
[ RUN      ] Dilithium3Test.KeyComparison
[       OK ] Dilithium3Test.KeyComparison (2 ms)
[ RUN      ] Dilithium3Test.NewKeyFromBytes
[       OK ] Dilithium3Test.NewKeyFromBytes (0 ms)
[ RUN      ] Dilithium3Test.KeySize
[       OK ] Dilithium3Test.KeySize (1 ms)
[ RUN      ] Dilithium3Test.Encoding
[       OK ] Dilithium3Test.Encoding (1 ms)
[ RUN      ] Dilithium3Test.Decoding
[       OK ] Dilithium3Test.Decoding (0 ms)
[ RUN      ] Dilithium3Test.SIGOperations
[       OK ] Dilithium3Test.SIGOperations (5 ms)
[----------] 7 tests from Dilithium3Test (9 ms total)

[----------] Global test environment tear-down
[==========] 7 tests from 1 test suite ran. (9 ms total)
[  PASSED  ] 7 tests.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and
the ISC license.

jylama99 and others added 30 commits March 11, 2022 14:09
* rebase and added pq-crypto folder(CRLF/LF issue)

* local clean up

* clean up for PR

* codebuild CMakeLists.txt issue

* codebuild correct cmake issue2

* updated pq CMakeLists file

* fix bracket issues

* fix bracket issues attempt 2

* fix bracket issues attempt 3

* cmake updated and braces fixed

* API integration and windows codebuild fix

* API integration and windows codebuild fix

* clean up

* apply PR feedback part 1

* Update PR

* Updated PR

* Updated PR

* Resolved all PR reviews to date

* PR update

* replace ct with SIKE version

* fix SIKE ct function

* This will fail CodeBuild

* Back to working verison

* Added helpful comments

* Add helpful TODO comments
clean up comments and remove AWS license from SIKE.
This PR imports the source code of SIKE from the official repository:
https://github.com/microsoft/PQCrypto-SIDH.

The new EVP APIs for PQ KEMs are also refactored to be more in line
with the existing EVP APIs in the library.

A basic unit test is added to verify the functionality
of SIKE's implementation and the newly added PQ KEM APIs.

Co-authored-by: Dusan Kostic <[email protected]>
…#343)

* Add PQCrystals-Kyber512, Remove SIKE, relocate pq_kem interface.
* Use a runtime switch to enable the DRBG, similar to how it is done in fork_detect.c.
* Modify Kyber's verify.c logic to avoid a warning-turned-error on MSVC builds.
* Increase OID length to accommodate longer OID value for Kyber-R3.
* Remove PQ_KEM API and replace with EVP_KEM API. Update KAT tests to match new API.

* Address memory issues found during testing.

* Update comment, braces, and function parameter style

* Update test names to match convention
The NID and other identifiers of an algorithm in AWS-LC are generated
by a script rather than adding the NID by hand. This commit fixes
the Kyber512 NID previously added by hand.
The prefix build doesn't work with the Kyber source code because Kyber
is doing it's own prefixing that clashes with the one we do in aws-lc.

This will be fixed before moving Kyber to the main aws-lc branch.
Merge main into integrate-pq branch
- Removed the OpenSSL 3.0-like structures and APIs from the implementation of PQ KEMs.
- Added two new methods in EVP_PKEY, encapsulate and decapsulate, to be used for KEMs.
- Renamed some macro and variable names to be consistent with NIST's notation.
Internalize the custom randomness generation done for PQ algorithms testing.
This PR introduces a new signature algorithm CRYSTALS Dilithium that has been selected for standardization by NIST as part of the PQC project. We import the Dilithium source code from this commit of the official Dilithium repository pq-crystals/dilithium@3e9b9f1 as of May 2022.

This PR includes only the Dilithium source implementation and does not include higher level API integration. The interface to the EVP APIs will be included within a following PR.
This PR introduces the EVP_PKEY APIs for Dilithium3, utilizing key generation, sign and verify.

**Addition to EVP interface**
This PR also makes Dilithium3 keys available to the EVP PKEY interface including a subset of methods for key management. As the Dilithium signing procedure is a single-shot signing algorithm that does not use a pre-hash, we follow much of the same structure as the EVP interface for Ed25519. For example, to sign and verify we use the EVP operations `EVP_DigestSign` and `EVP_DigestVerify` with hash `|type|` set to NULL.

There are two pairs of sign/verify APIs provided by Dilithium, `pqcrystals_dilithium3_ref_signature` and `pqcrystals_dilithium3_ref_verify` which sign and verify (resp.) a Dilithium3 signature generated for some message m, and `pqcrystals_dilithium3_ref` and `pqcrystals_dilithium3_ref_open`, which sign and verify (resp.) a Dilithium3 signed message (which is the composition of a Dilithium3 and the message m that was signed) for some message m. We have chosen to select the two APIs `pqcrystals_dilithium3_ref_signature` and `pqcrystals_dilithium3_ref_verify` to be consistent with existing EVP signature functionality. This is also consistent with other implementations, such as within [LibOQS](https://github.com/open-quantum-safe/liboqs/blob/main/src/sig/dilithium/sig_dilithium_3.c#L33-L34).

**DRBG**
To support KAT testing, a deterministic random number generator was needed. This PR introduces the same DRBG (AES-CTR) used by the Dilithium source code and uses the same randombytes interface to seed and get random bytes from it. The DRBG only gets built when the testing flag is enabled. Otherwise, the existing RNG is used.

**Testing**
Unit tests were added which exercise the use of Dilithium3 in the EVP interface. KAT tests from the Dilithium submission to NIST's Round 3 Post-Quantum Cryptography selection process are also included. To support running the KAT tests, an additional random number generator was added which is deterministic and only built-in test mode.

The KAT tests are built by the Dilithium team using the `pqcrystals_dilithium3_ref` and `pqcrystals_dilithium3_ref_open` APIs (which include the message within the signature). Since we are using the APIs that do not include the message within the signature we truncate the full signature in the KAT to only the signature part to exclude the copy of the message.
Still need to:

- change `DILITHIUM3` to `DILITHIUM3_R3` everywhere
- move `crypto/dilithium/` to `crypto/dilithium3/r3`?
- update CI to set this feature guard
- perhaps move NID up to higher numerical value in an "experimental"
  namespace where it won't conflict with newer algos/NIDs?
- #ifdef out the dilithium NID definition if possible
WillChilds-Klein and others added 8 commits May 5, 2023 16:48
This change qualifies the public DILITHIUM3 identifiers with a "round 3"
(`_R3`) suffix. Internal function, variable, etc. are left as-is because
they are not exposed to callers and can be updated if/when a materially
different round 4 comes down the pipeline.

Ideally, we'd be able to exclude this NID/OID entirely in non-dilithium
builds, but this is not currently possible with the current
`crypto/obj/` code. A next-best option would have been to put the
Dilithium3 NID into an "experimental" NID namespace _well_ above the
number of anticipated algorithms over the library's lifetime. I
prototyped this out with an NID of 10,001 but as it turns out the
tooling will expand NUM_NID and the `kObjects[]` in `obj_dat.h` and
backfill all ~9000 ivervening array values with NULL/0-valued entries.
This space tradeoff seemed unacceptable, so this implementation presents
the `DILITHIUM3_R3` NID in all builds.

The next commit will add a test to assert the behavior of trying to
specify `DILITHIUM3_R3` in non-`DILITHIUM_ENABLED=ON` builds.
This was only possible by setting OPENSSL_armcap via static defines in order to avoid reading from MIDR_EL1.
Valgrind was outputting an illegal opcode error on 
__asm__ volatile("mrs %0, MIDR_EL1" : "=r" (val));

This commit avoids compiling cpu_aarch64_linux.c in valgrind build by passing -DOPENSSL_STATIC_ARMCAP to it and setting all capabilities via the STATIC_ARMCAP macros (except SHA512 which wasn't used before with valgrind and was also causing an "illegal opcode" error when set).
Note: Setting -DOPENSSL_STATIC_ARMCAP without specifying any capabilities disables all of them as in the second
set of tests.

The ssl runner valgrind tests are here made to run in parallel to the valgrind tests (only with the build where all capabilities are set).
@WillChilds-Klein
Copy link
Contributor Author

WillChilds-Klein commented May 8, 2023

Ok, here are the requested changes:

  • change NID name to DILITHIUM3 to DILITHIUM3_R3
  • a build-time feature guard for Dilithium (covers source, test, and benchmark)
    • see updated Testing section in PR overview for how the build flag affects Dilithium testing
  • add Dilithium3 test CI run to the POSIX tests to ensure we always have full coverage with and without Dilithium enabled

Here's a log of commits I've made since responding to @torben-hansen's last round of feedback (the API version bump is unrelated to this PR and has already been merged):

$ git log --author childw d88c2ee..HEAD
commit 9f662afeec19cd7cdf8a9e65d013451177bd345d (HEAD -> cleaup-pq)
Merge: df834beba c48902790
Author: Will Childs-Klein <[email protected]>
Date:   Mon May 8 19:06:29 2023 +0000

    Merge branch 'main' into cleaup-pq

commit 314c50d0054b52a71bab0227154809ea6ce5c206
Author: Will Childs-Klein <[email protected]>
Date:   Mon May 8 17:57:56 2023 +0000

    Add disablement test when ENABLE_DILITHIUM=OFF

commit 7d08364fe91c11b491ee48c59d4a3cfe8ce6edd6
Author: Will Childs-Klein <[email protected]>
Date:   Mon May 8 17:15:10 2023 +0000

    Rename DILITHIUM3 OID/NID to DILITHIUM3_R3

    This change qualifies the public DILITHIUM3 identifiers with a "round 3"
    (`_R3`) suffix. Internal function, variable, etc. are left as-is because
    they are not exposed to callers and can be updated if/when a materially
    different round 4 comes down the pipeline.

    Ideally, we'd be able to exclude this NID/OID entirely in non-dilithium
    builds, but this is not currently possible with the current
    `crypto/obj/` code. A next-best option would have been to put the
    Dilithium3 NID into an "experimental" NID namespace _well_ above the
    number of anticipated algorithms over the library's lifetime. I
    prototyped this out with an NID of 10,001 but as it turns out the
    tooling will expand NUM_NID and the `kObjects[]` in `obj_dat.h` and
    backfill all ~9000 ivervening array values with NULL/0-valued entries.
    This space tradeoff seemed unacceptable, so this implementation presents
    the `DILITHIUM3_R3` NID in all builds.

    The next commit will add a test to assert the behavior of trying to
    specify `DILITHIUM3_R3` in non-`DILITHIUM_ENABLED=ON` builds.

commit 51494fa7226e2080ebf2097960c8b64ca0a532bc
Merge: f3aa0ed9e 14daff9d3
Author: Will Childs-Klein <[email protected]>
Date:   Fri May 5 17:05:27 2023 +0000

    Merge branch 'main' into cleaup-pq

commit f3aa0ed9e7b87d36869f5e9e294dc2f495776d25
Author: Will Childs-Klein <[email protected]>
Date:   Fri May 5 17:04:10 2023 +0000

    Add Dilithium3 case to POSIX tests, enable dilithium in benchmarks

commit 8eeb737d15123fde4b6d213a82edf79690cae891
Author: Will Childs-Klein <[email protected]>
Date:   Fri May 5 16:44:06 2023 +0000

    Fix speed.cc to work with new dilithium guard

commit ce5eda46933e9eae06425c721abe817c43f50a54
Author: Will Childs-Klein <[email protected]>
Date:   Thu May 4 21:32:52 2023 +0000

    Add opt-in build feature guard for Dilithium

    Still need to:

    - change `DILITHIUM3` to `DILITHIUM3_R3` everywhere
    - move `crypto/dilithium/` to `crypto/dilithium3/r3`?
    - update CI to set this feature guard
    - perhaps move NID up to higher numerical value in an "experimental"
      namespace where it won't conflict with newer algos/NIDs?
    - #ifdef out the dilithium NID definition if possible

commit 5b88f4a09867612dc1aa040dcf2f52ee0a6fd2c8
Merge: b907f6b9c d723334d7
Author: Will Childs-Klein <[email protected]>
Date:   Thu May 4 20:50:29 2023 +0000

    Merge branch 'main' into cleaup-pq

commit b907f6b9cd648085eb9b216fe4584f295de09e3a
Merge: 5d320e117 33741f13c
Author: Will Childs-Klein <[email protected]>
Date:   Thu May 4 20:50:14 2023 +0000

    Merge branch 'main' into cleaup-pq

commit 28c85a29a45a69f37f8becb7cefe5faed96eb422
Author: Will Childs-Klein <[email protected]>
Date:   Wed Apr 5 16:30:54 2023 -0400

    Bump AWS-LC API version (#900)

commit 5d320e117d74b547c7f944f9e159e78ad4fe545f
Merge: 0d0b0b937 a962ee3f3
Author: Will Childs-Klein <[email protected]>
Date:   Tue Apr 4 19:13:05 2023 +0000

    Merge branch 'main' into cleaup-pq

As mentioned previously, once this PR has been merged there will be fast-follow PRs to:

  1. Reinstate Dilithium KATs and fuzzing corpus
  2. Reinstate Dilithium documentation, and adjust the example code per @dkostic's guidance

torben-hansen
torben-hansen previously approved these changes May 12, 2023
tests/ci/run_posix_tests.sh Show resolved Hide resolved
Copy link
Contributor

@torben-hansen torben-hansen left a comment

Choose a reason for hiding this comment

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

Prefer to squash this, otherwise the commit history would be mighty polluted.

For the squash commit message. Write something that makes sense. A high-level summary of changes maybe?

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.