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

tls: fixes for wolfssl + openssl combo builds #10322

Closed
wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jan 19, 2023

  1. Add USE_WOLFSSL to the TLS backend priority list in lib/curl_ntlm_core.c.

  2. Fix lib/curl_ntlm_core.h to respect TLS backend priority, bringing this in sync with the above list and lib/curl_ntlm_core.c itself.

    Reported-by: Mark Roszko
    Ref: openssl + wolfssl combo backend build broken #10321

  3. Allow enabling both wolfSSL and OpenSSL at the same time in lib/Makefile.mk. Update the logic to select the crypto-specific lib for ngtcp2, where only one TLS backend is supported at the same time.

Closes #10322

@vszakats vszakats changed the title tls: fixes for wolfssl and openssl combo builds tls: fixes for wolfssl + openssl combo builds Jan 19, 2023
1. Add USE_WOLFSSL to the priority list in `lib/curl_ntlm_core.c`

2. Fix `lib/curl_ntlm_core.h` to respect backend priority, bringing
   this in sync with the above list and `lib/curl_ntlm_core.c` itself.

3. Allow enabling both wolfSSL and OpenSSL at the same time in
   `lib/Makefile.mk`. Update the logic to select the crypto-specific
   lib for ngtcp2.

Ref: curl#10321

Closes curl#10322
@jay
Copy link
Member

jay commented Jan 20, 2023

The reason it was the other way around though was because if OpenSSL came first (like it does in this PR) then include <openssl/ssl.h> could include a wolfSSL include with that name (depending on build order; if OpenSSL and wolfSSL include paths are both used) if I understand @elms' commit right.

@vszakats
Copy link
Member Author

vszakats commented Jan 20, 2023

I tend to think that if this combo isn't supported in all cases (which it isn't), we can hardly say it this is a supported combination. It also doesn't seem to be something much tested before. @elms commit says it adds wolfSSL support, but there's no mention of building in parallel with OpenSSL, so this was maybe something not tested or considered. To me the order picked seems to be the result of keeping changes at minimum while extending existing code. As for header confusion (picking the same header from the wrong project): if the header paths are setup correctly this should not happen because wolfSSL ones are referenced under wolfssl/openssl/.

(All of this is mostly guessing, my build attempts always failed with this combo.)

@bagder
Copy link
Member

bagder commented Jan 29, 2023

wolfSSL provides an OpenSSL API, that curl partially uses to do some things.

The only way we can build curl to use both OpenSSL and wolfSSL simultaneously is if they the two libraries do not share any symbols/function names, which would mean that wolfSSL's OpenSSL API would have to be disabled. That would then make the wolfSSL backend less feature rich.

If someone insists on making that work, fine, but I'm not going to personally lose any sleep over us not supporting that.

@vszakats
Copy link
Member Author

vszakats commented Jan 31, 2023

I plan to commit this as-is. This should help adding further fixes if someone is interested in doing them. It also brings .mk builds on par with cmake/autotools, which also don't explicitly block this combination.

@vszakats vszakats closed this in 48eb71a Feb 1, 2023
@vszakats vszakats deleted the wolfssl-openssl-1 branch February 1, 2023 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants