Openssl 3.1.3

[disa6302]

Issue #, if available:

What was changed?
Added support for OpenSSL 3.x version in the SDK

Why was it changed?
OpenSSL 1.1.1x went EOL in September 2023 and some of the APIs that the SDK uses are deprecated which mostly show up as warnings. This means these APIs could be removed in the future. To ensure we are up to date, this PR introduces support for OpenSSL 3.x, while also maintaining support for older OpenSSL versions.

How was it changed?

• Used OPENSSL_VERSION_NUMBER define to switch the APIs used where relevant.
• Added a new cmake property: BUILD_OLD_OPENSSL_VERSION to allow building older version from source (1.1.1w) if 3.1.3 causes issues. System dependencies could also be used instead if applications/platforms require using older version, however, this property covers situations for applications that build the dependencies from source in our SDK
• Added CI jobs for different platforms to test of older OpenSSL version

What testing was done for the changes?

• Local testing was done for RSA certificate generation and EC certificate generation to confirm the new APIs work
• For MD5, the hash was logged to compare the values between 1.1.1x and 3.x version
• Existing benchmark was used to verify no performance degradation in the handshake process.
• CI was used to fix build failures on Windows and ARM cross compilation that came with the upgrade

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (feba4d3) 74.84% compared to head (7027a1c) 74.87%.

❗ Current head 7027a1c differs from pull request most recent head 90ba931. Consider uploading reports for the commit 90ba931 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1840      +/-   ##
===========================================
+ Coverage    74.84%   74.87%   +0.03%     
===========================================
  Files           48       48              
  Lines        12844    12844              
===========================================
+ Hits          9613     9617       +4     
+ Misses        3231     3227       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[niyatim23]

Nice!

[hassanctech]

Need to properly handle failure return on EVP_DigestInit_ex() or else we will have undefined behavior:

Ignoring failure returns of EVP_DigestInit_ex(), EVP_DigestInit_ex2(), or EVP_DigestInit() can lead to undefined behavior on subsequent calls updating or finalizing the EVP_MD_CTX such as the EVP_DigestUpdate() or EVP_DigestFinal() functions. The only valid calls on the EVP_MD_CTX when initialization fails are calls that attempt another initialization of the context or release the context.

From: https://www.openssl.org/docs/man3.0/man3/EVP_DigestInit_ex.html

Undefined behavior needs to be handled

[hassanctech]

Ok so I see that you're using the new API SSL_CTX_set1_groups_list for open ssl >= 3.0.0. But where the diff now is compared to our existing code is for all versions higher than 1.0.2 (so 1.0.2 <= x < 3.0.0), really we did not enforce the upper bound before but for all intents in purposes for theses versions we were previously using SSL_CTX_set_ecdh_auto. That I want to understand why are we changing the behavior for existing versions of openssl that's already been tried and tested, was there a bug with this or what is the reason for this change? IMO unless there is a good reason this change should only impact when the version is >= 3.0.0 and otherwise it should not touch the existing behavior at all, unless there is a good reason to do that. I'm not following what that reason is.

[disa6302]

SSL_CTX_set_ecdh_auto is primarily used for automatic curve selection during SSL/TLS handshakes, making it convenient for situations where you want to delegate curve selection to OpenSSL. On the other hand, EC_KEY_new_by_curve_name is used when you need to explicitly specify the ECC curve for ECC key generation and want more control over the curve selection process. I am not sure what SSL_CTX_set_ecdh_auto selects by default, but given we are controlling the curve selection pre 1.0.2, it only made sense to do the same post 1.0.2 since the API is available. Another reason for the change was I wanted to avoid having multiple ifdefs that make the code buggy when it is unnecessary. I do understand the point of the existing code already be tested, but the functions I have included for versions between 1.0.2 and 3.0.0 are also tested as being part of version <1.0.2. Without this change, the ifdefs look like:

#if (OPENSSL_VERSION_NUMBER >= 0x10002000L && OPENSSL_VERSION_NUMBER < 0x3000000L)
    SSL_CTX_set_ecdh_auto(pSslCtx, TRUE);
#elif (OPENSSL_VERSION_NUMBER < 0x10002000L)
    CHK((ecdh = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1)) != NULL, STATUS_SSL_CTX_CREATION_FAILED);
    CHK(SSL_CTX_set_tmp_ecdh(pSslCtx, ecdh) == 1, STATUS_SSL_CTX_CREATION_FAILED);
#elif (OPENSSL_VERSION_NUMBER >= 0x3000000L)
    CHK(SSL_CTX_set1_groups_list(pSslCtx, "prime256v1") == 1, STATUS_SSL_CTX_CREATION_FAILED);
#endif

Let me know if you still feel strongly about this change and I can switch it to this block. But, given EC_KEY_new_by_curve_name and SSL_CTX_set_tmp_ecdh are already tested for versions less than 1.0.2, I did not see the harm in using the same for 1.0.2 to 3.0.0.

[hassanctech]

I did some digging and I found SSL_CTX_set_ecdh_auto was entirely removed in openssl 1.1.0 [https://github.com/openssl/openssl/issues/1437#issuecomment-238828306]. Then it was added back as a dummy no-op macro for backwards compat like a week later in 1.1.0a (openssl/openssl@2ecb9f2). So I suppose we've actually been getting lucky if someone was using this against 1.1.0 it wouldn't compile.

I cannot seem to find the manpages for openssl 1.0.0 to verify if EC_KEY_new_by_curve_name existed then, and also what about the curve itself, has NID_X9_62_prime256v1 always existed. Let me just see if I can find some official references if not I'll look through the source.

[disa6302]

Yeah. There doesnt seem to be official man pages pre-1.0.2. But I just checked out the branch to check if the API existed.

[hassanctech]

but given we are controlling the curve selection pre 1.0.2, it only made sense to do the same post 1.0.2 since the API is available.

Could it be it was done that way because better/more optimal curves might be selected post 1.0.2 that were not available previously which is why the auto option was used? What I want to make sure is we're not regressing for post 1.0.2 where a better curve (if available) would have been selected.

let me know if you still feel strongly about this change and I can switch it to this block.
No it's not that, I just want to make sure that we don't inadvertently do something worse now than before. I think this just requires some investigation.

With the change in this PR for openssl < 3 we do:

EC_KEY_new_by_curve_name(NID_X9_62_prime256v1)

What happens if NID_X9_62_prime256v1 is not available (is that even possible) or is there something better than NID_X9_62_prime256v1 that we might want to use? I just want to definitely know the answer to that, are we losing anything by pinning to this specific curve?

For openssl3 we do:

SSL_CTX_set1_groups_list(pSslCtx, "prime256v1")

What if "prime256v1" is not available but an alternative is, can that happen on certain platforms that we may support? Also this string, should probably be declared as a macro. What is the guidance here should there be a few of these named strings we should be trying in some type of priority order? I'm sure on standard linux/max/windows all of this will be just fine, but what about other platforms (which are not covered in our CI), I just want to make sure we won't error out because we pinned it to a specific curve. I'm not saying we should use the deprecated API what I'm saying is we should due our due diligence to make sure this will work across all platforms and if there are other curve names we need to check for then we should do that.

[github-actions]

This is a very old issue. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to open a new one.

[github-actions]

This is a very old issue. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to open a new one.