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

Rename sphincs algs to follow upstream #6576

Merged
merged 3 commits into from
Jul 11, 2023
Merged

Conversation

iyanmv
Copy link
Contributor

@iyanmv iyanmv commented Jul 5, 2023

Description

liboqs 0.8 renamed sphincs-shake256-X to sphincs-shake-X.

Testing

I tried to compile wolfSSL with --with-liboqs and it failed. After these changes, it compiles again.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

@iyanmv
Copy link
Contributor Author

iyanmv commented Jul 5, 2023

I cannot compile with cmake (passing -DWOLFSSL_OQS=ON), so perhaps I missed something else. Will have another look later today.

@anhu
Copy link
Member

anhu commented Jul 5, 2023

Hi @iyanmv ,

Thank you so much for creating this pull request. I must admit to having lost track of liboqs updates. Its great news that they have had a new release.

Can I ask you about your project and what you are doing with wolfSSL and liboqs?

I've had a look at your changes and they look fine to me! Before we can continue with the process, we will need you sign a contributor agreement. Would you be willing to read over and sign the contributor agreement?

If so, please send a message to support@wolfssl.com requesting to become a wolfSSL contributor. Please also reference this PR in your message to support@wolfssl.com

Warm regards, Anthony

@anhu
Copy link
Member

anhu commented Jul 5, 2023

By the way, I don't think wolfSSL library's cmake infrastructure supports -DWOLFSSL_OQS=ON .

@anhu
Copy link
Member

anhu commented Jul 5, 2023

Oh, my colleague has corrected me. We do have it in there. I apologize for misleading you.

@iyanmv
Copy link
Contributor Author

iyanmv commented Jul 5, 2023

Hi @anhu

Thank you so much for creating this pull request. I must admit to having lost track of liboqs updates. Its great news that they have had a new release.

Can I ask you about your project and what you are doing with wolfSSL and liboqs?

I'm running some benchmarks using the NIST PQC candidates, and I learned about wolfSSL from this paper. I was curious so I wanted to try to replicate some of their results.

I've had a look at your changes and they look fine to me! Before we can continue with the process, we will need you sign a contributor agreement. Would you be willing to read over and sign the contributor agreement?

If so, please send a message to support@wolfssl.com requesting to become a wolfSSL contributor. Please also reference this PR in your message to support@wolfssl.com

Alright! I will send the email now.

@anhu
Copy link
Member

anhu commented Jul 5, 2023

Hi @iyanmv ,

Ah, yes, I know the authors of that paper quite well.

You might be interested in our integration with the PQM4 library on STM32. You can have a look here:

https://github.com/wolfSSL/wolfssl-examples/tree/master/pq/stm32

If visuals are better for you, please have a look at this YouTube video that talk about it in depth:

https://www.youtube.com/watch?v=OK6MKXYiVBY

Warm regards, Anthony

@embhorn
Copy link
Member

embhorn commented Jul 7, 2023

@iyanmv has been approved as a wolfSSL contributor

@embhorn embhorn requested a review from anhu July 7, 2023 14:48
Copy link
Member

@anhu anhu left a comment

Choose a reason for hiding this comment

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

I have tested this change and it works great!!

@anhu
Copy link
Member

anhu commented Jul 7, 2023

Oooh. Just ran wolfcrypt/benchmark/benchmark and verification fails:

wc_sphincs_verify_msg failed -229, verify 0

@anhu anhu self-requested a review July 7, 2023 16:02
Copy link
Member

@anhu anhu left a comment

Choose a reason for hiding this comment

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

verification failures. See previous comments.

@iyanmv
Copy link
Contributor Author

iyanmv commented Jul 7, 2023

Falcon also fails, not only sphincs. Let me have a look.

@iyanmv
Copy link
Contributor Author

iyanmv commented Jul 10, 2023

I think verification fails because the keys saved and defined in certs_test.h that are used for the benchmark are incompatible with the latest implementation of SPHINCS+ by the PQClean, which was updated in this PR in liboqs.

If you point me in the right direction (maybe you have a script to generate those keys?) I can update certs/sphincs/ and wolfssl/certs_test.h in this PR.

I quickly tried replacing one with this small program:

#include<oqs/oqs.h>

void cleanup_heap(uint8_t *public_key, uint8_t *secret_key, OQS_SIG *sig)
{
    if (sig != NULL) {
        OQS_MEM_secure_free(secret_key, sig->length_secret_key);
    }
    OQS_MEM_insecure_free(public_key);
    OQS_SIG_free(sig);
}

int create_keys(const char *method_name)
{
    OQS_SIG *sig = NULL;
    uint8_t *public_key = NULL;
    uint8_t *secret_key = NULL;
    OQS_STATUS rc;

    sig = OQS_SIG_new(method_name);
    if (sig == NULL) {
        fprintf(stderr, "OQS_SIG_new failed!\n");
        return OQS_ERROR;
    }

    public_key = malloc(sig->length_public_key);
    secret_key = malloc(sig->length_secret_key);
    if ((public_key == NULL) || (secret_key == NULL)) {
        fprintf(stderr, "ERROR: malloc failed!\n");
        cleanup_heap(public_key, secret_key, sig);
        return OQS_ERROR;
    }

    rc = OQS_SIG_keypair(sig, public_key, secret_key);
    if (rc != OQS_SUCCESS) {
        fprintf(stderr, "ERROR: OQS_SIG_keypair failed!\n");
        cleanup_heap(public_key, secret_key, sig);
        return OQS_ERROR;
    }

    printf("Key:\n");
    for (size_t i=0; i<sig->length_secret_key; ++i) {
        printf("0x%02X, ", secret_key[i]);
    }
    for (size_t i=0; i<sig->length_public_key; ++i) {
       printf("0x%02X, ", public_key[i]);
    }
    printf("\n");

    printf("%s keypair generation succeeded!\n", method_name);
    cleanup_heap(public_key, secret_key, sig);
    return OQS_SUCCESS;
}


int main()
{
    OQS_STATUS ret;
    ret = create_keys("SPHINCS+-SHAKE-128f-simple");
    if (ret != OQS_SUCCESS) {
        fprintf(stderr, "ERROR!\n");
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
}

But now I get a wc_sphincs_import_private_key failed -140.

@iyanmv
Copy link
Contributor Author

iyanmv commented Jul 10, 2023

Oh, I think I understood now. wc_sphincs_import_private_key does not expect the keys directly but it should be in DER. And the wolfssl/certs_test.h probably you generated with xxd -i -c 10?

I will include new keys in this PR in a bit.

@iyanmv
Copy link
Contributor Author

iyanmv commented Jul 10, 2023

Why aren't there any methods in the WOLFSSL_API to generate key pairs? Something like

WOLFSSL_API
int wc_sphincs_generate_key(sphincs_key* key);

(Sorry if this is a stupid question, I'm not familiar with wolfSSL. Are users supposed to generate keys with other libraries?)

@iyanmv iyanmv requested a review from anhu July 10, 2023 14:07
@anhu
Copy link
Member

anhu commented Jul 10, 2023

Hi @iyanmv ,

Why aren't there any methods in the WOLFSSL_API to generate key pairs?

Its not a stupid question at all. Generally, for signature schemes we use OQS's fork of OpenSSL to generate the certificates.

Please see this script: https://github.com/wolfSSL/osp/blob/master/oqs/generate_sphincs_chains.sh

The main reason is that no one has requested generation as a feature yet.

Warm regards, Anthony

@iyanmv
Copy link
Contributor Author

iyanmv commented Jul 10, 2023

Please see this script: https://github.com/wolfSSL/osp/blob/master/oqs/generate_sphincs_chains.sh

Oh! That would have been so useful. I guess I did a similar thing, but using OpenSSL 3 and the oqs-provider instead of the OQS OpenSSL 1.1.1 fork. But I also had to manually enable some algs and re-run the generate.py script.

The main reason is that no one has requested generation as a feature yet.

I see. I have implemented a quick function in this branch. If you think it makes sense, I can open a new PR and work on it after we are done with this. I don't think it would take a lot of time. Also, I guess it would help me to get more familiar with wolfSSL.

Cheers

@anhu
Copy link
Member

anhu commented Jul 10, 2023

Thank you for this:

8f5692d

It was a copy and paste failure on my part!

anhu
anhu previously approved these changes Jul 10, 2023
Copy link
Member

@anhu anhu left a comment

Choose a reason for hiding this comment

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

Took it for a spin and things look good to me!! Can you please squash all your commits and then do a force push? After that I will assign to one more member of our engineering team for final review and merge.

This also adds new keys for SPHINCS+. The reason is that SPHINCS+
was updated to 3.1 in liboqs (open-quantum-safe/liboqs/pull/1420),
and old keys are incompatible with the new implementation.

Keys were generated using the oqs-provider for OpenSSL 3

openssl genpkey \
    -provider default -provider oqsprovider \
    -algorithm sphincsshake128fsimple \
    -outform der \
    -out bench_sphincs_fast_level1_key.der

And certs_test.h was updated using xxd

xxd -i -c 10 -u bench_sphincs_fast_level1_key.der

This was repeated for the 6 variants of SPHINCS+ that wolfSSL supports.
@anhu
Copy link
Member

anhu commented Jul 10, 2023

Hi @iyanmv , I will now assign this ticket to @dgarske for final review and merge.

@anhu anhu requested a review from dgarske July 10, 2023 17:24
wolfssl/certs_test.h Outdated Show resolved Hide resolved
0xCC, 0xF4, 0x2F, 0xF2, 0xAC, 0x74, 0xDF, 0x0E, 0x20, 0x9D,
0xC2, 0x9E, 0xD1, 0xB4, 0x12
0X30, 0X71, 0X02, 0X01, 0X00, 0X30, 0X08, 0X06, 0X06, 0X2B,
0XCE, 0X0F, 0X06, 0X07, 0X0D, 0X04, 0X62, 0X04, 0X60, 0XD8,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did these keys change? Seems like a change to Shake would not require the test keys to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I have no idea. But I can replicate the issue using liboqs 0.7.2 to generate some keys, and then trying to verify with liboqs 0.8.0.

dgarske
dgarske previously approved these changes Jul 10, 2023
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Thanks. @anhu please review the reason for the test key change.

@dgarske
Copy link
Contributor

dgarske commented Jul 10, 2023

OK to test

@anhu
Copy link
Member

anhu commented Jul 10, 2023

Apparently there was a change/update to the round 3 submission.
Original Round 3 submission: https://sphincs.org/data/sphincs+-round3-specification.pdf
2022-06-10 update to the Round 3 submission: https://sphincs.org/data/sphincs+-r3.1-specification.pdf

@iyanmv
Copy link
Contributor Author

iyanmv commented Jul 10, 2023

@anhu I forgot to squash the last commit with the suggested change. Should I do it or wait until checks are completed?

@anhu
Copy link
Member

anhu commented Jul 10, 2023

@iyanmv ,

I think no action is required from you. a single extra commit is fine.
Warm regards, Anthony

@anhu anhu self-requested a review July 10, 2023 20:20
anhu
anhu previously approved these changes Jul 10, 2023
@dgarske
Copy link
Contributor

dgarske commented Jul 10, 2023

Retest this please

@dgarske dgarske assigned dgarske and unassigned anhu and iyanmv Jul 10, 2023
@dgarske
Copy link
Contributor

dgarske commented Jul 10, 2023

Our liboqs CI test is failing now. @anhu or @bandi13 seems like we will need to update the liboqs version in our CI to get this to pass then merge?

In file included from wolfcrypt/benchmark/benchmark.c:172:
./wolfssl/wolfcrypt/sphincs.h:70:37: error: ‘OQS_SIG_sphincs_shake_256f_simple_length_public_key’ undeclared here (not in a function); did you mean ‘OQS_SIG_sphincs_sha256_256f_simple_length_public_key’?
   70 | #define SPHINCS_LEVEL5_PUB_KEY_SIZE OQS_SIG_sphincs_shake_256f_simple_length_public_key
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./wolfssl/wolfcrypt/sphincs.h:70:37: note: in definition of macro ‘SPHINCS_LEVEL5_PUB_KEY_SIZE’
   70 | #define SPHINCS_LEVEL5_PUB_KEY_SIZE OQS_SIG_sphincs_shake_256f_simple_length_public_key
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./wolfssl/wolfcrypt/sphincs.h:89:12: note: in expansion of macro ‘SPHINCS_MAX_PUB_KEY_SIZE’
   89 |     byte p[SPHINCS_MAX_PUB_KEY_SIZE];
      |            ^~~~~~~~~~~~~~~~~~~~~~~~
./wolfssl/wolfcrypt/sphincs.h:69:37: error: ‘OQS_SIG_sphincs_shake_256f_simple_length_secret_key’ undeclared here (not in a function); did you mean ‘OQS_SIG_sphincs_sha256_256f_simple_length_secret_key’?
   69 | #define SPHINCS_LEVEL5_KEY_SIZE     OQS_SIG_sphincs_shake_256f_simple_length_secret_key
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./wolfssl/wolfcrypt/sphincs.h:69:37: note: in definition of macro ‘SPHINCS_LEVEL5_KEY_SIZE’
   69 | #define SPHINCS_LEVEL5_KEY_SIZE     OQS_SIG_sphincs_shake_256f_simple_length_secret_key
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...

@dgarske dgarske assigned anhu and bandi13 and unassigned dgarske Jul 10, 2023
@anhu
Copy link
Member

anhu commented Jul 11, 2023

Jenkins retest this please.

@iyanmv
Copy link
Contributor Author

iyanmv commented Jul 11, 2023

I cannot compile with cmake (passing -DWOLFSSL_OQS=ON), so perhaps I missed something else. Will have another look later today.

Regarding this. Is this tested in the CI pipeline? If so, how is it done? I'm trying to run this (basically just the PKGBUILD from Arch Linux modified with -DWOLFSSL_OQS=ON)

build() {
    local cmake_options=(
        -DCMAKE_INSTALL_PREFIX=/usr
        -DCMAKE_BUILD_TYPE=None
        -DWOLFSSL_CURVE25519=ON
        -DWOLFSSL_CURVE448=ON
        -DWOLFSSL_ED25519=ON
        -DWOLFSSL_ED448=ON
        -DWOLFSSL_REPRODUCIBLE_BUILD=ON
        -DWOLFSSL_OQS=ON
        -DWARNING_C_FLAGS="$CFLAGS"
        -Wno-dev
        -B build
        -S $_pkgname-$pkgver-stable
    )

    cmake "${cmake_options[@]}"
    cmake --build build --verbose
}

But I get multiple undefined reference errors.

/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_Init'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_DecodePublicKey'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_dilithium_verify_msg'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_CipherTextSize'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_Dilithium_PrivateKeyDecode'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_sphincs_import_public'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_sphincs_init'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_sphincs_verify_msg'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_dilithium_get_level'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_PublicKeySize'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_Free'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_dilithium_import_private_only'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_dilithium_free'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `ext_kyber_enabled'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_Decapsulate'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_dilithium_set_level'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_SharedSecretSize'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_dilithium_sign_msg'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_dilithium_import_public'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_dilithium_check_key'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_dilithium_init'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_sphincs_check_key'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_PrivateKeySize'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_sphincs_free'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_EncodePublicKey'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_DecodePrivateKey'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_sphincs_set_level_and_optim'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_Encapsulate'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_MakeKey'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_Sphincs_PrivateKeyDecode'
/usr/bin/ld: libwolfssl.so.35.5.1: undefined reference to `wc_KyberKey_EncodePrivateKey'

@iyanmv
Copy link
Contributor Author

iyanmv commented Jul 11, 2023

I think some logic is missing in cmake/functions.cmake. Having a quick look when this functionality was originally added to CMake (#5407), I tried adding some extra list(APPEND LIB_SOURCES ...). Here is my diff that makes previous command work.

diff --git a/cmake/functions.cmake b/cmake/functions.cmake
index e77991ea1..f0fc48c2e 100644
--- a/cmake/functions.cmake
+++ b/cmake/functions.cmake
@@ -195,6 +195,9 @@ function(generate_build_flags)
     endif()
     if(WOLFSSL_OQS OR WOLFSSL_USER_SETTINGS)
         set(BUILD_FALCON "yes" PARENT_SCOPE)
+        set(BUILD_SPHINCS "yes" PARENT_SCOPE)
+        set(BUILD_DILITHIUM "yes" PARENT_SCOPE)
+        set(BUILD_EXT_KYBER "yes" PARENT_SCOPE)
     endif()
     set(BUILD_INLINE ${WOLFSSL_INLINE} PARENT_SCOPE)
     if(WOLFSSL_OCSP OR WOLFSSL_USER_SETTINGS)
@@ -804,6 +807,18 @@ function(generate_lib_src_list LIB_SOURCES)
               list(APPEND LIB_SOURCES wolfcrypt/src/falcon.c)
          endif()
 
+         if(BUILD_SPHINCS)
+              list(APPEND LIB_SOURCES wolfcrypt/src/sphincs.c)
+         endif()
+
+         if(BUILD_DILITHIUM)
+              list(APPEND LIB_SOURCES wolfcrypt/src/dilithium.c)
+         endif()
+
+         if(BUILD_EXT_KYBER)
+              list(APPEND LIB_SOURCES wolfcrypt/src/ext_kyber.c)
+         endif()
+
          if(BUILD_LIBZ)
               list(APPEND LIB_SOURCES wolfcrypt/src/compress.c)
          endif()

@anhu
Copy link
Member

anhu commented Jul 11, 2023

@iyanmv ,

Ha! You beat me to it. I just noticed that the list of files being built did not include the post-quantum algorithm files.
Thanks for the patch above. Can you add it to this PR?

Anthony

wolfSSLGH-5407 already included falcon.c, but now we also add sphincs.c,
dilithium.c and ext_kyber.c to avoid undefined reference errors.
@iyanmv
Copy link
Contributor Author

iyanmv commented Jul 11, 2023

Ha! You beat me to it. I just noticed that the list of files being built did not include the post-quantum algorithm files. Thanks for the patch above. Can you add it to this PR?

Sure! Will do in a minute.

@iyanmv iyanmv dismissed stale reviews from anhu and dgarske via fd091a7 July 11, 2023 16:03
@anhu anhu self-requested a review July 11, 2023 18:27
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.

6 participants