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

Compile fails with openssl binary build mode #2376

Closed
snps-janick opened this issue Sep 27, 2023 · 8 comments · Fixed by #2386
Closed

Compile fails with openssl binary build mode #2376

snps-janick opened this issue Sep 27, 2023 · 8 comments · Fixed by #2386
Assignees
Labels
bug Something isn't working
Milestone

Comments

@snps-janick
Copy link

snps-janick commented Sep 27, 2023

The openssl stub uses a function (evp_pkey_copy_downgraded) that is no longer public in OpenSSL 3.0 (see openssl/openssl#16088 (comment))

My build command:

(cd build \
      && cmake -DARCH=x64 -DTOOLCHAIN=GCC -DTARGET=Debug \
               -DCRYPTO=openssl -DENABLE_BINARY_BUILD=1 -DCOMPILED_LIBCRYPTO_PATH=${OPENSSL}/lib64/libcrypto.so -DCOMPILED_LIBSSL_PATH=${OPENSSL}/lib64/libssl.so \
               .. \
      && make copy_sample_key \
      && make )

The error:

[ 31%] Linking C executable ../../bin/test_spdm_requester
/tmp/test_spdm_requester.XOo3q2.ltrans0.ltrans.o: In function `libspdm_ecd_set_pub_key':
<artificial>:(.text+0xd5118): undefined reference to `evp_pkey_copy_downgraded'
/tmp/test_spdm_requester.XOo3q2.ltrans0.ltrans.o: In function `libspdm_ecd_set_pri_key':
<artificial>:(.text+0xd520a): undefined reference to `evp_pkey_copy_downgraded'

Note that commenting out both places where it is called (which obviously does not fix the problem) compiles successfully, which indicates it is the only such no-longer-public symbol.

@steven-bellock steven-bellock added the bug Something isn't working label Sep 27, 2023
@steven-bellock steven-bellock added this to the Q3 2023 milestone Sep 27, 2023
@steven-bellock
Copy link
Contributor

Related pull requests: #2314 and #2330.

It sounds like, currently, if ENABLE_BINARY_BUILD=1 and the cryptography library is OpenSSL then EDDSA must be disabled at compile time. If that is the case then DISABLE_EDDSA can be removed in place of this logic. @alistair23 do you agree?

@steven-bellock
Copy link
Contributor

In addition we should add ENABLE_BINARY_BUILD=1 to CI/CD. I'll file a separate issue for that.

@snps-janick
Copy link
Author

Looking at the openSSL source code, I think this is the correct fix:

@@ -128,7 +128,7 @@ bool libspdm_ecd_set_pub_key(void *ecd_context, const uint8_t *public_key,
         return false;
     }
 
-    if (evp_pkey_copy_downgraded(&evp_key, new_evp_key) != 1) {
+    if (EVP_PKEY_copy_parameters(evp_key, new_evp_key) != 0) {
         EVP_PKEY_free(new_evp_key);
         return false;
     }
@@ -185,7 +185,7 @@ bool libspdm_ecd_set_pri_key(void *ecd_context, const uint8_t *private_key,
         return false;
     }
 
-    if (evp_pkey_copy_downgraded(&evp_key, new_evp_key) != 1) {
+    if (EVP_PKEY_copy_parameters(evp_key, new_evp_key) != 0) {
         EVP_PKEY_free(new_evp_key);
         return false;
     }

@steven-bellock
Copy link
Contributor

I think this is the correct fix:

Even better. Feel free to make a pull request or @alistair23 or @Wenxing-hou can comment.

@alistair23
Copy link
Contributor

The EVP_PKEY_copy_parameters() doesn't fix the issue unfortunately. It builds but a variety of test cases will fail. You are welcome to try and get it to work though. You can see what I tried at: #2314 and #2310. Unfortunatley Github doesn't track history, but I'm sure I tried EVP_PKEY_copy_parameters() and didn't get it to work.

As for building libspdm with openssl, you can see the configs that work here: https://git.buildroot.net/buildroot/tree/package/libspdm/libspdm.mk#n20. The key one is DISABLE_EDDSA as libspdm can't be built against public OpenSSL APIs with EDDSA support enabled.

@alistair23
Copy link
Contributor

I think disabling EDDSA when ENABLE_BINARY_BUILD=1 is enabled makes sense as well

@jyao1
Copy link
Member

jyao1 commented Sep 28, 2023

I think disabling EDDSA when ENABLE_BINARY_BUILD=1 is enabled makes sense as well

Agree with @alistair23.

It is not obvious today, that we must disable EdDSA to make binary build work.

We should either update readme, or disable EdDSA when ENABLE_BINARY_BUILD=1.

Note: Install OpenSSL with command `sudo make install` before build libspdm.
cmake -DARCH=x64 -DTOOLCHAIN=GCC -DTARGET=Release -DCRYPTO=openssl -DENABLE_BINARY_BUILD=1 -DCOMPILED_LIBCRYPTO_PATH=<OPENSSL_PATH>/libcrypto.a -DCOMPILED_LIBSSL_PATH=<OPENSSL_PATH>/libssl.a ..

@steven-bellock steven-bellock self-assigned this Oct 2, 2023
@steven-bellock
Copy link
Contributor

Note this works when using the .a library rather than .so.

steven-bellock added a commit to steven-bellock/libspdm that referenced this issue Oct 3, 2023
jyao1 pushed a commit that referenced this issue Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants