Updated to support OpenSSL 3 #295

ctjhai · 2023-01-19T22:42:12Z

This PR aims to address issue #259.

ctjhai · 2023-01-24T01:08:57Z

Hi @bifurcation, as a follow up of our discussion in #259, I've been looking at OpenSSL 3.x source code, and found that the flag EVP_MD_CTX_FLAG_NON_FIPS_ALLOW is not used anymore, see:
https://github.com/openssl/openssl/blob/3147785eb23bb27080a0b7accbbff46ac471e86c/include/openssl/evp.h#L208-L209

The discussion around this can be found here: openssl/openssl#17484.

I also found that in some OpenSSL versions (1.1.1) that I've tested, the flag EVP_MD_CTX_FLAG_NON_FIPS_ALLOW is only used to load non-FIPS implementation, e.g. loading MD5 implementation in FIPS mode; it doesn't really validate the constraint on the key size. I guess some custom implementation of the FIPS object module may be stricter, so perhaps this flag has been used to override this behaviour?

Anyway, with OpenSSL 3.x, perhaps we need to ensure that both FIPS and default providers are loaded, so that we can fallback to the default implementation to allow a more relaxed key size. So there is no need to force the use OpenSSL 1.1.1 for the FIPS mode. Having said that, just like version 1.1.1, it looks like OpenSSL 3.x is happy with HMAC key size < 112 bits even in FIPS mode. I tried 0-length HMAC key size with OpenSSL 3.0 FIPS provider, there is no issue; I arrive at the expected HMAC digest.

There are some test cases that fail in FIPS mode though, but I think this is related to using the likes of X25519 and X448 for key-agreement or Ed25519 in the X.509. I'll update the test cases as part of this pull-request.

bifurcation

This is a great start, thanks @ctjhai. A few suggestions for refactoring, and it looks like you have some CI issues to clear, but overall this seems like the right direction.

Given the depth of the changes in group.cpp and digest.cpp, I wonder if it would be simpler just to have two versions of the files (one with the v1 implementation and one with v3), and switch between them in CMake. One of my concerns here is how easy it's going to be to revert this change once we're more in an all-v3 world, and that would certainly make that reversion simple.

It would also be good to add a CI run here that builds with OpenSSL v3. Might need an alternate vcpkg.json that gets swapped in at the right moment.

bifurcation · 2023-01-24T18:54:45Z

lib/hpke/src/certificate.cpp

+    , not_before(asn1_time_to_chrono(X509_getm_notBefore(x509.get())))
+    , not_after(asn1_time_to_chrono(X509_getm_notAfter(x509.get())))


IIUC, the getm versions return mutable references, which seems unnecessary here?

My bad, this should be X509_get0_XXX.

lib/hpke/src/digest.cpp

lib/hpke/src/group.cpp

bifurcation · 2023-01-24T19:16:37Z

lib/hpke/src/group.cpp

+      }
+    }
+
+    auto bld = make_typed_unique(OSSL_PARAM_BLD_new());


I might make this name a little more descriptive, say builder

lib/hpke/src/group.cpp

ctjhai · 2023-01-25T16:04:16Z

Given the depth of the changes in group.cpp and digest.cpp, I wonder if it would be simpler just to have two versions of the files (one with the v1 implementation and one with v3), and switch between them in CMake. One of my concerns here is how easy it's going to be to revert this change once we're more in an all-v3 world, and that would certainly make that reversion simple.

The changes in digest.cpp are pretty small compared to group.cpp, so I am not entirely sure about creating a separately file for this. Additionally, another advantage of not creating two files is we can easily access the trails of changes of the files (from git blame perspective). In terms of reversion, we can use tools like unifdef to pick the code for OpenSSL 3 only.

Having said that, if you insist, I am happy to use two separate files for these. But we still need to cater for the WITH_OPENSSL3 flag in openssl_common.cpp. So would you also like to have two files for openssl_common.cpp to maintain consistency?

* Refactored group.cpp * Added Github actions for OpenSSL 3 * Added OpenSSL vcpkg.json for MLS library and interop binary

bifurcation

Thanks @ctjhai, this is on the right track. A couple of minor things, then this is good to go.

bifurcation · 2023-01-30T16:01:47Z

.github/workflows/compat_openssl3.yml

@@ -0,0 +1,43 @@
+name: Build for older MacOS


I don't think we need to run this combination in CI. Support for older macOS is mainly a question of variant and optional, which is orthogonal to OpenSSL versions.

bifurcation · 2023-01-30T16:08:30Z

.github/workflows/interop_openssl3.yml

@@ -0,0 +1,42 @@
+name: Build the interop harness (OpenSSL 3)


Likewise, the interop harness's issues are separate from any OpenSSL issues.

bifurcation · 2023-01-30T16:15:06Z

lib/hpke/src/digest.cpp

+  auto mac =
+    make_typed_unique(EVP_MAC_fetch(nullptr, OSSL_MAC_NAME_HMAC, nullptr));
+  auto ctx = make_typed_unique(EVP_MAC_CTX_new(mac.get()));
+  auto digest_name = openssl_digest_name(id);
+  std::array<OSSL_PARAM, 2> params = {
+    OSSL_PARAM_construct_utf8_string(
+      OSSL_ALG_PARAM_DIGEST, digest_name.data(), 0),
+    OSSL_PARAM_construct_end()
+  };


I would switch the order of initialization here, so that if (ctx == nullptr) immediately follows the declaration of ctx. So:

auto digest_name = ... auto params = std::array<OSSL_PARAM, 2>{ ... }; auto mac = ... auto ctx = ...

(const auto if possible)

bifurcation · 2023-02-06T18:22:08Z

.github/workflows/compat_openssl3.yml

@@ -0,0 +1,43 @@
+name: Build for older MacOS (OpenSSL 3)


We don't need this extra build. The MacOS compat issues are orthogonal to OpenSSL 3.

bifurcation · 2023-02-06T18:23:08Z

CMakeLists.txt

+# External libraries
+find_package(OpenSSL REQUIRED)
+if ( OPENSSL_FOUND )
+    if (${OPENSSL_VERSION} VERSION_GREATER 1.1.1)


Suggested change

if (${OPENSSL_VERSION} VERSION_GREATER 1.1.1)

if (${OPENSSL_VERSION} VERSION_GREATER_EQUAL 3)

(This avoids spurious errors if the 1.X line evolves)

lib/hpke/src/digest.cpp

bifurcation · 2023-02-06T18:33:37Z

lib/hpke/test/common.cpp

+static int
+FIPS_mode()
+{
+  if (OSSL_PROVIDER_available(nullptr, "fips") == 1) {
+    return 1;
+  }
+  return EVP_default_properties_is_fips_enabled(nullptr);
+}


Rather than emulate the old OpenSSL FIPS API, I would pull the contents of fips.cpp/h into this file and its header, and make variants of fips() and enable_fips() for the two versions. Where in the latter case, I mean either make a new function enable_fips() that varies between the versions, or just branch internally to ensure_fips_if_required().

Not a problem

bifurcation · 2023-02-06T18:37:30Z

lib/mls_vectors/test/fips.h

+bool
+fips();
+
+bool
+is_fips_approved(mls::CipherSuite::ID id);


These methods seem duplicative of fips and fips_disable?

This method is no longer needed, so removed in the current commit.

bifurcation · 2023-02-06T18:39:44Z

test/fips.h

+bool
+fips();
+
+bool
+is_fips_approved(mls::CipherSuite::ID id);


If we need these methods in test, let's just make them a public API of the hpke library. We shouldn't be directly including OpenSSL headers outside of HPKE.

This method is no longer needed. Initially, I needed it when I tested my changes on an Oracle Linux 8 with FIPS enabled. The OS came with OpenSSL 1.1.1k FIPS and it had a smaller set of FIPS approved algorithms compared to OpenSSL 3.0 FIPS. So I had to use the above method to skip certain algorithm choices. Given that we are moving towards the later version of OpenSSL, I don't see there is a need to skip any test so I think we can remove this method.

bifurcation

Thanks for all the work on this @ctjhai !

bifurcation · 2023-02-21T22:58:24Z

@ctjhai - Some CI runs are failing due to deficiencies in the macOS GitHub Actions runners. These were fixed in #314, but your PR is older than that. If you could please do a quick git checkout main && git pull && git checkout openssl3 && git merge main, that should turn the CI green and I'll merge.

ctjhai · 2023-02-22T09:38:37Z

@bifurcation Thanks Richard. Unfortunately, you will need to re-approve the CI run again. On my fork repo, all of the CI runs are all good now.

Updated to support OpenSSL 3

4d052a9

bifurcation requested changes Jan 24, 2023

View reviewed changes

Merge branch 'cisco:main' into openssl3

629d121

ctjhai added 2 commits January 27, 2023 23:34

Addressed reviewer's comments

54ffe0b

* Refactored group.cpp * Added Github actions for OpenSSL 3 * Added OpenSSL vcpkg.json for MLS library and interop binary

clang-tidy conformance and updated tests for FIPS mode

5d5ea5b

ctjhai marked this pull request as ready for review January 27, 2023 23:41

ctjhai force-pushed the openssl3 branch from 20bd9cf to 5d5ea5b Compare January 30, 2023 15:09

ctjhai added 2 commits January 31, 2023 11:27

Fixed broken GitHub actions

0764209

Updated built-baseline to allow Windows to find OpenSSL 3.0.7

f933250

ctjhai marked this pull request as draft February 1, 2023 09:14

ctjhai force-pushed the openssl3 branch from 08ecd47 to f933250 Compare February 1, 2023 12:23

ctjhai marked this pull request as ready for review February 1, 2023 13:07

ctjhai requested a review from bifurcation February 6, 2023 07:26

bifurcation requested changes Feb 6, 2023

View reviewed changes

ctjhai added 2 commits February 9, 2023 14:54

Merge branch 'main' into openssl3

7cd0a1c

Addressed reviewer's comments

9df638a

ctjhai requested a review from bifurcation February 9, 2023 16:14

Removed unused OpenSSL 3 vcpkg.json for interop

101ac70

bifurcation approved these changes Feb 21, 2023

View reviewed changes

ctjhai added 2 commits February 21, 2023 23:13

Merge branch 'main' into openssl3

ec8db19

Added missing pkg-config to OpenSSL 3 CI build

dbcdb86

bifurcation merged commit ebc2d63 into cisco:main Feb 22, 2023

ctjhai mentioned this pull request Feb 23, 2023

OpenSSL 3 integration ctjhai/mlspp#1

Closed

bifurcation mentioned this pull request Jul 17, 2023

Enable linking against BoringSSL #349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated to support OpenSSL 3 #295

Updated to support OpenSSL 3 #295

ctjhai commented Jan 19, 2023

ctjhai commented Jan 24, 2023

bifurcation left a comment

bifurcation Jan 24, 2023

ctjhai Jan 25, 2023

bifurcation Jan 24, 2023

ctjhai commented Jan 25, 2023

bifurcation left a comment

bifurcation Jan 30, 2023

bifurcation Jan 30, 2023

bifurcation Jan 30, 2023

bifurcation Feb 6, 2023

bifurcation Feb 6, 2023

bifurcation Feb 6, 2023

bifurcation Feb 6, 2023

bifurcation Feb 6, 2023

ctjhai Feb 9, 2023

bifurcation Feb 6, 2023

ctjhai Feb 9, 2023

bifurcation Feb 6, 2023

ctjhai Feb 9, 2023 •

edited

Loading

bifurcation left a comment

bifurcation commented Feb 21, 2023

ctjhai commented Feb 22, 2023

		, not_before(asn1_time_to_chrono(X509_getm_notBefore(x509.get())))
		, not_after(asn1_time_to_chrono(X509_getm_notAfter(x509.get())))

	if (${OPENSSL_VERSION} VERSION_GREATER 1.1.1)
	if (${OPENSSL_VERSION} VERSION_GREATER_EQUAL 3)

Updated to support OpenSSL 3 #295

Updated to support OpenSSL 3 #295

Conversation

ctjhai commented Jan 19, 2023

ctjhai commented Jan 24, 2023

bifurcation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctjhai commented Jan 25, 2023

bifurcation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctjhai Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

bifurcation left a comment

Choose a reason for hiding this comment

bifurcation commented Feb 21, 2023

ctjhai commented Feb 22, 2023

ctjhai Feb 9, 2023 •

edited

Loading