SSLNTV-29 Add the ability to use a custom OpenSSL engine #16

heyuanliu-intel · 2022-10-09T05:17:07Z

Add support custom engine to fix the issue #15

Intel QAT can boost SSL/TLS performance and has been integrated into many popular open source frameworks.
Such as envoy and nginx. Please refer to envoyproxy/envoy#21984

And for the async mode nginx, it can get 4X performance boost with QAT.
Please refer to https://01.org/sites/default/files/downloads/intelr-quickassist-technology/intelquickassisttechnologyopensslperformance.pdf

So many applications can get benefit from QAT custom engine and other custom engines.

fjuma · 2022-12-13T21:11:10Z

Thanks for the PR, @heyuanliu-intel!

Tests for WildFly OpenSSL are included in the wildfly-openssl project that depends on this wildfly-openssl-natives project. Do you think it would be possible to add a test case that makes use of a custom engine? Or is it possible to provide steps that can be used for testing this?

heyuanliu-intel · 2022-12-15T01:20:05Z

I have verified this using Intel QAT engine and using Airlift sample as a testcase. Please refer to this project.
https://github.com/heyuanliu-intel/AirliftPerformance

fjuma · 2022-12-19T20:04:06Z

Thanks, took a look at https://github.com/heyuanliu-intel/AirliftPerformance. I noticed the -Dorg.wildfly.openssl.engine being specified in run_server.sh. Do you have corresponding changes against the wildfly-openssl repo that adds support for this new org.wildfly.openssl.engine option?

In particular, it looks like corresponding changes would be needed in SSL.java to call the updated initialize method with the customEngine parameter.

heyuanliu-intel · 2022-12-20T02:23:58Z

Yes. Below is the git commit

diff --git a/java/src/main/java/org/wildfly/openssl/SSLImpl.java b/java/src/main/java/org/wildfly/openssl/SSLImpl.java
index 2dbe55e..c7ab32a 100644
--- a/java/src/main/java/org/wildfly/openssl/SSLImpl.java
+++ b/java/src/main/java/org/wildfly/openssl/SSLImpl.java
@@ -27,10 +27,10 @@ public class SSLImpl extends SSL {
public SSLImpl() {
}

static native void initialize0(String libCryptoPath, String libSslPath);

static native void initialize0(String libCryptoPath, String libSslPath, String customEngine);

protected void initialize(String libCryptoPath, String libSslPath) {

   SSLImpl.initialize0(libCryptoPath, libSslPath);

protected void initialize(String libCryptoPath, String libSslPath, String customEngine) {

   SSLImpl.initialize0(libCryptoPath, libSslPath, customEngine);

}

diff --git a/java/src/main/java/org/wildfly/openssl/SSL.java b/java/src/main/java/org/wildfly/openssl/SSL.java
index d103474..534c7d3 100644
--- a/java/src/main/java/org/wildfly/openssl/SSL.java
+++ b/java/src/main/java/org/wildfly/openssl/SSL.java
@@ -41,6 +41,7 @@ public abstract class SSL {
public static final String MAC_HOMEBREW_OPENSSL_PATH = "/usr/local/opt/openssl/lib/";
private static SSL instance;

public static final String ORG_WILDFLY_OPENSSL_ENGINE = "org.wildfly.openssl.engine";
public static final String ORG_WILDFLY_OPENSSL_PATH = "org.wildfly.openssl.path";
public static final String ORG_WILDFLY_OPENSSL_PATH_LIBSSL = "org.wildfly.openssl.path.ssl";
public static final String ORG_WILDFLY_OPENSSL_PATH_LIBCRYPTO = "org.wildfly.openssl.path.crypto";
@@ -191,7 +192,8 @@ public abstract class SSL {
if (cryptoPath == null) {
throw new RuntimeException(Messages.MESSAGES.couldNotFindLibCrypto(ORG_WILDFLY_OPENSSL_PATH, attemptedCrypto.toString()));
}

               instance.initialize(cryptoPath, sslPath);

               String sslEngine = System.getProperty(ORG_WILDFLY_OPENSSL_ENGINE);

               instance.initialize(cryptoPath, sslPath, sslEngine);
               String version = instance.version();
               logger.info(Messages.MESSAGES.openSSLVersion(version));

@@ -289,7 +291,7 @@ public abstract class SSL {
}
}

protected abstract void initialize(String libCryptoPath, String libSslPath);

protected abstract void initialize(String libCryptoPath, String libSslPath, String customEngine);

fjuma · 2022-12-20T13:36:17Z

@heyuanliu-intel Thanks! Would you be able to submit a PR against wildfly-openssl and indicate in the description that it depends on this PR?

fjuma · 2022-12-20T13:39:05Z

When creating the PR against wildfly-openssl, please reference WFSSL-111 in the commit message. Thanks!

heyuanliu-intel · 2022-12-21T02:33:01Z

Create a new PR on wildfly-openssl.
The PR is wildfly-security/wildfly-openssl#127

heyuanliu-intel · 2023-01-06T05:54:43Z

Throw exceptions when error occurs during initialize custom engine.

fjuma

@heyuanliu-intel Thanks for the updates! I've added some comments, feel free to let me know if you have any questions.

One more small thing, please update the commits in this PR so they reference SSLNTV-29.

Thanks!

fjuma · 2023-01-10T19:31:59Z

libwfssl/src/ssl.c

-        return 0;
+        TCN_FREE_CSTRING(customEngine);
+        throwIllegalStateException(e, "Could not load openssl dynamic methods.");
+        goto init_failed;


I don't see any changes made to load_openssl_dynamic_methods. Is there a reason for introducing the above two lines? These change the behaviour compared to what was done before so am curious about the need for this.

Yes. Previous logic doesn't contains this. I added this exception to keep the consistent with the other error processing. As you know, I added the logic to throw exception when the error occurs during loading custom engine. So If you think this isn't necessary, I can remove this line.

Because this would mean a change in behaviour compared to what was happening before, for backwards compatibility, I think we should remove these two lines.

fjuma · 2023-01-10T19:44:28Z

libwfssl/src/ssl.c

    }
    TCN_FREE_CSTRING(libCryptoPath);
    TCN_FREE_CSTRING(libSSLPath);

    /* Check if already initialized */
    if (ssl_initialized++) {
+        TCN_FREE_CSTRING(customEngine);


It looks like we are missing a call to TCN_FREE_CSTRING(customEngine) after the changes below.

Yes. You are right. I can move TCN_FREE_CSTRING(customEngine) to init_failed.

fjuma · 2023-01-10T20:03:42Z

libwfssl/src/ssl.c

@@ -548,6 +587,9 @@ WF_OPENSSL(jint, initialize) (JNIEnv *e, jobject o, jstring libCryptoPath, jstri
    session_init(e);

    return (jint)0;
+
+    init_failed:
+        return 0;


I think there should be a call to TCN_FREE_CSTRING(customEngine) before returning.

I think ssl_initialized should also be set to 0 here.

Yes. You are right. I will do the code change.

fjuma · 2023-01-10T20:30:02Z

libwfssl/src/ssl.c

+                }
+            }
+
+            if (ssl_engine) {


For this case, it looks like both the Netty and Tomcat OpenSSL implementations also do this before attempting to set the default:

#ifdef ENGINE_CTRL_CHIL_SET_FORKCHECK if (strcmp(J2S(engine), "chil") == 0) ENGINE_ctrl(tcn_ssl_engine, ENGINE_CTRL_CHIL_SET_FORKCHECK, 1, 0, 0); #endif

See https://github.com/netty/netty-tcnative/blob/main/openssl-dynamic/src/main/c/ssl.c#L804-L810 and https://github.com/apache/tomcat-native/blob/main/native/src/ssl.c#L540-L547.

Do you think we could do something similar here?

I don't know the real reason to add this logic. So I need to check.

This logic is specifically for the "chil" engine. It's not strictly required to add this for your changes but since both the Netty and Tomcat OpenSSL implementations have this, I think we might as well add the same logic here as well.

The ENGINE_CTRL_CHIL_SET_FORKCHECK is never going to be defined, so adding the lines like they are now is doing nothing. Note that wildfly-openssl works using dlopen without any openssl include at compilation. If we really want to set this option for chil like in the other implementations we need to add the define in wfssl.h:

+/* Flags specific to the nCipher "chil" engine */ +# define ENGINE_CTRL_CHIL_SET_FORKCHECK 100

And always execute the ENGINE_ctrl in case of the chil engine.

Nit-picky request, please add the brackets to the if. 😄

@heyuanliu-intel The #ifdef is not needed now, it's going to be always defined. All the rest LGTM now. Thanks!

okay. I will remove it.

fjuma · 2023-01-10T20:48:26Z

Note that the test failures in this CI job are expected because the corresponding changes to SSL.java and SSLImpl.java from wildfly-security/wildfly-openssl#127 are needed for things to work properly.

rmartinc

@heyuanliu-intel I have added a few more comments.

Thanks!

rmartinc · 2023-01-12T10:56:13Z

libwfssl/src/ssl.c

+                    if (!crypto_methods.ENGINE_ctrl_cmd_string(ssl_engine, "SO_PATH", engine, 0) || !crypto_methods.ENGINE_ctrl_cmd_string(ssl_engine, "LOAD", NULL, 0)) {
+                        crypto_methods.ENGINE_free(ssl_engine);
+                        ssl_engine = NULL;
+                        tcn_Throw(e, "Could not load openssl custom engine (%s) .so library file", engine);


@heyuanliu-intel What do you think about returning the openssl error too?
Something like the following:

char err[2048]; crypto_methods.ENGINE_free(ssl_engine); ssl_engine = NULL; generate_openssl_stack_error(e, err, sizeof(err)); tcn_Throw(e, "Could not load openssl custom engine (%s) .so library file: %s", engine, err);

Good suggestion. It will help software developer get more detailed error message. I will add this to the code change.

rmartinc · 2023-01-12T10:57:15Z

libwfssl/src/ssl.c

+                if (!crypto_methods.ENGINE_set_default(ssl_engine, ENGINE_METHOD_ALL)) {
+                    crypto_methods.ENGINE_free(ssl_engine);
+                    ssl_engine = NULL;
+                    tcn_Throw(e, "Could not set custom engine (%s) to default engine.", engine);


Same here, I would add the openssl error like in the previous throw.

rmartinc · 2023-01-12T10:58:02Z

libwfssl/src/ssl.c

+                }
+            }
+        }
+    }



The customEngine is not freed in non-error return, you can add TCN_FREE_CSTRING(customEngine); here after the if.

You are right and I will fix this.

rmartinc · 2023-01-12T11:06:45Z

libwfssl/src/ssl.c

+                }
+            }
+
+            if (ssl_engine) {


The ENGINE_CTRL_CHIL_SET_FORKCHECK is never going to be defined, so adding the lines like they are now is doing nothing. Note that wildfly-openssl works using dlopen without any openssl include at compilation. If we really want to set this option for chil like in the other implementations we need to add the define in wfssl.h:

+/* Flags specific to the nCipher "chil" engine */ +# define ENGINE_CTRL_CHIL_SET_FORKCHECK 100

And always execute the ENGINE_ctrl in case of the chil engine.

Nit-picky request, please add the brackets to the if. 😄

rmartinc

LGTM now, thanks @heyuanliu-intel !

fjuma · 2023-01-17T17:08:37Z

Thanks for the updates @heyuanliu-intel! Would you be able to squash the commits into a single commit?

heyuanliu-intel · 2023-01-18T02:38:42Z

I have squashed the commits into a singe commit, please review it again.

fjuma

Looks good, thanks @heyuanliu-intel!

fjuma · 2023-02-03T17:08:18Z

@heyuanliu-intel Just to keep you updated, we are planning on doing a WildFly OpenSSL release next month so will get this merged in the next few weeks (we are currently working on some updates to our CI environments). Will keep you posted on the release date. Thanks.

heyuanliu-intel · 2023-02-04T00:34:02Z

Great! Thanks.

heyuanliu-intel mentioned this pull request Dec 21, 2022

WFSSL-111 Add the ability to use a custom OpenSSL engine wildfly-security/wildfly-openssl#127

Open

heyuanliu-intel requested review from fjuma and Skyllarr as code owners January 6, 2023 05:52

fjuma requested changes Jan 10, 2023

View reviewed changes

heyuanliu-intel force-pushed the custom_engine branch 2 times, most recently from 6a17ec9 to e8914f5 Compare January 12, 2023 04:48

rmartinc suggested changes Jan 12, 2023

View reviewed changes

heyuanliu-intel changed the title ~~support custom engine~~ SSLNTV-29 Add the ability to use a custom OpenSSL engine Jan 13, 2023

rmartinc approved these changes Jan 16, 2023

View reviewed changes

heyuanliu-intel force-pushed the custom_engine branch from 5ec0da9 to bd6365a Compare January 18, 2023 02:34

SSLNTV-29 Add the ability to use a custom OpenSSL engine

bd6365a

fjuma approved these changes Jan 18, 2023

View reviewed changes

fjuma mentioned this pull request Aug 8, 2023

[SSLNTV-33] Add Ubuntu workflow that uses OpenSSL 3.0.7 #25

Closed

SSLNTV-29 Add the ability to use a custom OpenSSL engine #16

Are you sure you want to change the base?

SSLNTV-29 Add the ability to use a custom OpenSSL engine #16

Conversation

heyuanliu-intel commented Oct 9, 2022 • edited Loading

fjuma commented Dec 13, 2022 • edited Loading

heyuanliu-intel commented Dec 15, 2022

fjuma commented Dec 19, 2022

heyuanliu-intel commented Dec 20, 2022

fjuma commented Dec 20, 2022

fjuma commented Dec 20, 2022

heyuanliu-intel commented Dec 21, 2022

heyuanliu-intel commented Jan 6, 2023

fjuma left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma commented Jan 10, 2023

rmartinc 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

rmartinc left a comment

Choose a reason for hiding this comment

fjuma commented Jan 17, 2023

heyuanliu-intel commented Jan 18, 2023

fjuma left a comment

Choose a reason for hiding this comment

fjuma commented Feb 3, 2023 • edited Loading

heyuanliu-intel commented Feb 4, 2023

heyuanliu-intel commented Oct 9, 2022 •

edited

Loading

fjuma commented Dec 13, 2022 •

edited

Loading

fjuma left a comment •

edited

Loading

fjuma commented Feb 3, 2023 •

edited

Loading