WFSSL-111 Add the ability to use a custom OpenSSL engine #127

heyuanliu-intel · 2022-12-21T02:30:05Z

Add the ability to use a custom OpenSSL engine.
This PR depends on wildfly-security/wildfly-openssl-natives#16

fjuma

Thanks very much for the PR @heyuanliu-intel!

We'll be taking a closer look at this and the corresponding wildfly-openssl-natives PR in January.

One thing I am wondering though is if we can get some basic tests for configuring a custom engine added to this PR. It seems like using the QAT_Engine in the tests might be tricky if there are hardware pre-requisites needed. If that's the case, is it possible to test with some other engine instead so we have the custom engine use case covered in the testsuite?

fjuma · 2022-12-22T22:00:57Z

pom.xml

-        <version.org.wildfly.openssl.natives>2.2.2.Final</version.org.wildfly.openssl.natives>
-        <version.org.wildfly.openssl.wildfly-openssl-linux-x86_64>2.2.2.SP01</version.org.wildfly.openssl.wildfly-openssl-linux-x86_64>
-        <version.org.wildfly.openssl.wildfly-openssl-linux-s390x>2.2.2.SP01</version.org.wildfly.openssl.wildfly-openssl-linux-s390x>
+        <version.org.wildfly.openssl.natives>2.2.3.CR1-SNAPSHOT</version.org.wildfly.openssl.natives>


Just to note, because this PR depends on wildfly-security/wildfly-openssl-natives#16, the way we'd handle this is to merge the wildfly-openssl-natives PR first, do a wildfly-openssl-natives 2.2.3.Final release, then this PR would need to be updated with the released version.

Yes. Because it depends on wildfly-openssl-natives. So we should merge the wildfly-openssl-natives PR first.
For the QAT_engine part, it provides two implementations : software one and hardware one. For my testcase, I uses the software implementation do the verification. So it doesn't have hardware pre-requisites needed.

Ok that's great. Sounds like it should be possible to add some tests here then. For reference, the existing tests are found here: https://github.com/wildfly-security/wildfly-openssl/tree/main/java/src/test/java/org/wildfly/openssl

Although it doesn't have hardware pre-requisites but it needs to install a suite of software to make the test successful. From the program logic, it supports all the openssl custom engine. Let me think about how to add a testcase.

I use Intel RDRAND as OpenSSL custom engine to verify the function. Please review this commit.

Thanks very much for working on that! We'll take a look.

heyuanliu-intel · 2023-01-04T07:39:44Z

I find the Intel RDRAND is a custom OpenSSL engine, so I use it to write a testcase.

fjuma · 2023-01-04T23:23:32Z

java/src/test/java/org/wildfly/openssl/BasicOpenSSLEngineTest.java

+    @Test
+    public void tesRDRANDEngine() throws IOException, InterruptedException {
+        System.setProperty(SSL.ORG_WILDFLY_OPENSSL_ENGINE, "rdrand");
+        SSL ssl = SSL.getInstance();


Just tried running this test but noticed that the configured custom engine doesn't actually get used since SSL#getInstance() is already called during the test setup so SSL#init doesn't get run again.

To make sure that SSL#getInstance actually makes use of the configured property, I think we'd need to create a new test class with this test case and update the maven-surefire-plugin configuration in the java/pom.xml file so that it sets the org.wildfly.openssl.engine property to rdrand for this new test class only instead of trying to set the property programmatically.

As an example, we updated the maven-surefire-plugin configuration for a different testsuite recently, where we needed to specify some properties using the -D option when a certain test was being run:

wildfly/wildfly@d23461e#diff-16653cf23553258b76452eb02be79fc3d93668abe53bd164f0f3dd10a09a9fe7R292-R323

I think we should be able to do something similar here.

Feel free to let us know if you have any questions about this or would like some help with testing this.

okay. I will take a look at this and try to refine the testcase.

I have modified the code in wildfly-openssl-native, please review the code wildfly-security/wildfly-openssl-natives@e879191

The main change is to throw exceptions if any error occurs during initializing the custom engine.

So I also add the testcase to verify this function.
I think adding a new testcase class is simple than using maven-surefire-plugin method.

@heyuanliu-intel Great! Thanks very much for the updates here and on the wildfly-openssl-natives PR. We will take a look this week. Thanks!

fjuma

@heyuanliu-intel Thanks for the updates! I've added some comments.

fjuma · 2023-01-10T20:58:21Z

java/src/test/java/org/wildfly/openssl/BasicOpenSSLCustomEngineTest.java

+ */
+public class BasicOpenSSLCustomEngineTest {
+
+    @Test


Something that I noticed was that the outcome of the tests depends on the order in which the test cases are run.

Because the second test below is expected to successfully use the system property and call SSL#getInstance(), this means that if the second test happens to be run before the first test, then SSL#init won't be run again and the first test will fail on line 33.

To fix this, you could specify that the first test must be run before the second test. This could be done by adding @InSequence(1) and @InSequence(2) annotations to the test methods (see org.jboss.arquillian.junit.InSequence for more details).

Thanks for your comments. I will fix this.

Arquillian isn't in project's maven dependency. Should I add this dependency or upgrade the Junit version from Junit 4 to Junit 5 (maybe there is API change to upgrade Junit5)?

Or I separate those test methods to two test classes ?
Any comments about this?

Good question, we haven't checked yet if it's easy to upgrade to JUnit 5. For now, it's fine to add the following dependency:

<dependency> <groupId>org.jboss.arquillian.junit</groupId> <artifactId>arquillian-junit-core</artifactId> <version>1.6.0.Final</version> <scope>test</scope> </dependency>

I still see random order issues if I launch the new test alone in my env:

mvn clean install -Dorg.wildfly.openssl.path=/home/rmartinc/apps/openssl-1.1.1f/lib -Dorg.wildfly.openssl.engine=qatengine -Dtest=org.wildfly.openssl.BasicOpenSSLCustomEngineTest ... [INFO] Running org.wildfly.openssl.BasicOpenSSLCustomEngineTest [ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.093 s <<< FAILURE! - in org.wildfly.openssl.BasicOpenSSLCustomEngineTest [ERROR] testUnknownEngine(org.wildfly.openssl.BasicOpenSSLCustomEngineTest) Time elapsed: 0.003 s <<< FAILURE! java.lang.AssertionError: Expected ExceptionInInitializerError not thrown

My feeling is that only junit4 is being used and the arquillian InSequence annotation is doing nothing. In junit4 (but since 4.11) we can use @FixMethodOrder(MethodSorters.NAME_ASCENDING) and call the methods test1UnknownEngine and test2RDRANDEngine (or similar) to have the order we want. But that needs an upgrade of junit to last 4.13.2 (which is not bad because this project is using a very old junit 4.8.2). I just see a problem in TestAllMethodsImplemented.java with the upgrade, but is easily fixable using MatcherAssert.assertThat.

@heyuanliu-intel @fjuma WDYT about using FixMethodOrder and upgrading junit4?

The rest of the PR LGTM. When I reviewed the natives PR I used this one too and everything was OK for me.

Okay. I will do a test and fix this issue.

Since @rmartinc has the changes for upgrading to JUnit 4.13.2 done locally, he's going to submit a PR for that and we'll get that merged.

@heyuanliu-intel You'll then be able to rebase and use the @FixMethodOrder annotations as @rmartinc mentioned.

Thank you both!

@heyuanliu-intel We just merged @rmartinc's PR that upgrades to JUnit 4.13.2 (#130) so you should be able to rebase now.

fjuma · 2023-01-10T20:58:50Z

java/src/test/java/org/wildfly/openssl/BasicOpenSSLCustomEngineTest.java

+
+    @Test
+    public void testUnknownEngine() {
+        System.setProperty(SSL.ORG_WILDFLY_OPENSSL_ENGINE, "unknown");


Each test case should also reset this system property to its original value in a finally block.

Okay. I will fix this.

@fjuma When I set the engine to qat, I found a performance issue caused by ByteBuffer. You can get the detail info from this project ByteBufferPerformance.

And this is the Thread Dump File.

So my fix is change from the direct byte buffer allocation to byte buffer pool.
ByteBuffer buf = ByteBuffer.allocateDirect(len);
to
DefaultByteBufferPool.PooledByteBuffer pool = DefaultByteBufferPool.DIRECT_POOL.allocate(); ByteBuffer buf = pool.getBuffer(); buf.limit(len);

So the CPU utilization can boost from 10% to 50%, and the performance can boost 5X.

Should I open a new PR or just add this code change commit to this PR?

ByteBuffer buf = ByteBuffer.allocateDirect(len); try { final long addr = memoryAddress(buf); src.limit(pos + len); buf.put(src); src.limit(limit); sslWrote = SSL.getInstance().writeToSSL(ssl, addr, len); if (sslWrote > 0) { src.position(pos + sslWrote); return sslWrote; } else { src.position(pos); } } finally { buf.clear(); ByteBufferUtils.cleanDirectBuffer(buf); }

DefaultByteBufferPool.PooledByteBuffer pool = DefaultByteBufferPool.DIRECT_POOL.allocate(); ByteBuffer buf = pool.getBuffer(); buf.limit(len); try { final long addr = memoryAddress(buf); src.limit(pos + len); buf.put(src); src.limit(limit); sslWrote = SSL.getInstance().writeToSSL(ssl, addr, len); if (sslWrote > 0) { src.position(pos + sslWrote); return sslWrote; } else { src.position(pos); } } finally { pool.close(); }

The same code change should be applied to all the ByteBuffer allocateDirect.

@heyuanliu-intel Please create a separate WFSSL issue and a separate PR for this and we can review that more closely and discuss there. Thanks!

fjuma · 2023-01-12T15:22:19Z

java/src/test/java/org/wildfly/openssl/BasicOpenSSLCustomEngineTest.java

    public void testUnknownEngine() {
+        String engine = System.getProperty(SSL.ORG_WILDFLY_OPENSSL_ENGINE);


System.setProperty returns the previous value of the property so calling getProperty isn't needed.

okay. I will fix this.

fjuma · 2023-01-12T15:23:17Z

java/src/test/java/org/wildfly/openssl/BasicOpenSSLCustomEngineTest.java

        } catch (ExceptionInInitializerError expected) {
            Assert.assertNotNull(expected);
+        } finally {
+            if (engine != null) {


If the previous value was null, System.clearProperty can be called to unset the value.

I got it. I will unset the value I set if there is no value setting before.

fjuma · 2023-01-12T15:23:58Z

java/src/test/java/org/wildfly/openssl/BasicOpenSSLCustomEngineTest.java

-        AbstractOpenSSLTest.setup();
-        SSL ssl = SSL.getInstance();
-        Assert.assertNotNull(ssl.version());
+        String engine = System.getProperty(SSL.ORG_WILDFLY_OPENSSL_ENGINE);


Same here, the getProperty call isn't needed since the value is returned from setProperty.

fjuma · 2023-01-12T15:24:16Z

java/src/test/java/org/wildfly/openssl/BasicOpenSSLCustomEngineTest.java

+            SSL ssl = SSL.getInstance();
+            Assert.assertNotNull(ssl.version());
+        } finally {
+            if (engine != null) {


Same here, should clear the property if it was null before.

fjuma · 2023-01-13T18:42:36Z

java/src/test/java/org/wildfly/openssl/BasicOpenSSLCustomEngineTest.java

+
+    @Test
+    public void testUnknownEngine() {
+        System.setProperty(SSL.ORG_WILDFLY_OPENSSL_ENGINE, "unknown");


@heyuanliu-intel Please create a separate WFSSL issue and a separate PR for this and we can review that more closely and discuss there. Thanks!

fjuma · 2023-01-17T16:55:31Z

@heyuanliu-intel Thanks very much for the updates! This PR is looking good. Would you be able to squash the commits into a single commit?

fjuma · 2023-01-17T16:55:52Z

@rmartinc When you get a chance, please review. Thanks!

heyuanliu-intel · 2023-01-18T03:20:14Z

I have squashed the commits into a single commit. @fjuma

I have another question. I think we should change the dependency version after wildfly-openssl-natives is published.

   <version.org.wildfly.openssl.natives>2.2.3.CR1-SNAPSHOT</version.org.wildfly.openssl.natives>
        <version.org.wildfly.openssl.wildfly-openssl-linux-x86_64>2.2.3.CR1-SNAPSHOT</version.org.wildfly.openssl.wildfly-openssl-linux-x86_64>
        <version.org.wildfly.openssl.wildfly-openssl-linux-s390x>2.2.3.CR1-SNAPSHOT</version.org.wildfly.openssl.wildfly-openssl-linux-s390x>

fjuma · 2023-01-18T16:32:50Z

I have squashed the commits into a single commit. @fjuma

I have another question. I think we should change the dependency version after wildfly-openssl-natives is published.
   <version.org.wildfly.openssl.natives>2.2.3.CR1-SNAPSHOT</version.org.wildfly.openssl.natives>
        <version.org.wildfly.openssl.wildfly-openssl-linux-x86_64>2.2.3.CR1-SNAPSHOT</version.org.wildfly.openssl.wildfly-openssl-linux-x86_64>
        <version.org.wildfly.openssl.wildfly-openssl-linux-s390x>2.2.3.CR1-SNAPSHOT</version.org.wildfly.openssl.wildfly-openssl-linux-s390x>

@heyuanliu-intel Thank you very much for squashing the changes! Yes, once the wildfly-openssl-natives PR is merged, we will release WildFly OpenSSL Natives 2.3.0.Final. I've created WFSSL-115 to track upgrading to the new Natives version once released.

For now, you could remove the above dependency changes from the WFSSL-111 commit and introduce a new commit that references WFSSL-115 instead. For now you'd have to use the SNAPSHOT versions to successfully build locally.

heyuanliu-intel · 2023-01-19T03:45:16Z

@fjuma @rmartinc I have used the new Junit way to change the testcase. Please review it again.

Should I remove the dependency about Arquillian from pom.xml because it isn't used by the project now ?

<dependency>
    <groupId>org.jboss.arquillian.junit</groupId>
    <artifactId>arquillian-junit-core</artifactId>
    <version>1.6.0.Final</version>
    <scope>test</scope>
</dependency>

rmartinc · 2023-01-19T07:37:35Z

@heyuanliu-intel Yes, that part is not needed now, please remove that part.

heyuanliu-intel · 2023-01-19T12:02:02Z

@fjuma @rmartinc I have removed all the changes about maven pom.xml and squash the commits into a single commit. Please review it again.

rmartinc

@heyuanliu-intel I see the changes OK but I need to rebase the branch to run the tests. I think that you missed that point. Nevertheless I'm going to approve the PR cos it also needs the upgrade of the natives part to run OK. So a new rebase will be needed for sure in the future.

bjvetter · 2023-08-07T19:31:26Z

What is the status of this PR? Was it ever accepted/merged into the main branch or did it get left lingering in PR/review?

fjuma · 2023-08-08T18:40:13Z

@bjvetter Thanks very much for the reminder about this one!

@heyuanliu-intel When you get a chance, would you be able to rebase your branch (to include @rmartinc's changes that upgrade JUnit to 4.13.2)? Thanks!

heyuanliu-intel requested review from fjuma and Skyllarr as code owners December 21, 2022 02:30

heyuanliu-intel mentioned this pull request Dec 21, 2022

SSLNTV-29 Add the ability to use a custom OpenSSL engine wildfly-security/wildfly-openssl-natives#16

Open

fjuma reviewed Dec 22, 2022

View reviewed changes

fjuma reviewed Jan 4, 2023

View reviewed changes

fjuma requested changes Jan 10, 2023

View reviewed changes

fjuma reviewed Jan 13, 2023

View reviewed changes

heyuanliu-intel force-pushed the WFSSL-111 branch from 2b49cbc to 59e2a6a Compare January 18, 2023 03:06

heyuanliu-intel force-pushed the WFSSL-111 branch from 42ea470 to 5adf8a0 Compare January 19, 2023 11:57

WFSSL-111 Add the ability to use a custom OpenSSL engine

5adf8a0

rmartinc approved these changes Jan 20, 2023

View reviewed changes

		public void testUnknownEngine() {
		String engine = System.getProperty(SSL.ORG_WILDFLY_OPENSSL_ENGINE);

WFSSL-111 Add the ability to use a custom OpenSSL engine #127

Are you sure you want to change the base?

WFSSL-111 Add the ability to use a custom OpenSSL engine #127

Conversation

heyuanliu-intel commented Dec 21, 2022 • edited Loading

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

heyuanliu-intel commented Jan 4, 2023

fjuma Jan 4, 2023 • 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

fjuma 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

fjuma Jan 18, 2023 • 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

heyuanliu-intel Jan 13, 2023 • 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

fjuma commented Jan 17, 2023

fjuma commented Jan 17, 2023

heyuanliu-intel commented Jan 18, 2023

fjuma commented Jan 18, 2023

heyuanliu-intel commented Jan 19, 2023

rmartinc commented Jan 19, 2023

heyuanliu-intel commented Jan 19, 2023

rmartinc left a comment

Choose a reason for hiding this comment

bjvetter commented Aug 7, 2023

fjuma commented Aug 8, 2023

heyuanliu-intel commented Dec 21, 2022 •

edited

Loading

fjuma left a comment •

edited

Loading

fjuma Jan 4, 2023 •

edited

Loading

fjuma Jan 18, 2023 •

edited

Loading

heyuanliu-intel Jan 13, 2023 •

edited

Loading