io,grpc,netty: Add support for IBMJSSE2 provider #5374

ankitshubham97 · 2019-02-20T00:02:30Z

This commit addresses the issue: https://github.com/grpc/grpc-java/issues/5369
This commit improves gRPC to run gRPC server with TLS enabled on
AIX machines with IBMJSSE2 as their JDK SSL provider.

thelinuxfoundation · 2019-02-20T00:02:31Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

One or more commits are not associated with a GitHub user. The email used in the Git commit must match a verified email address in a GitHub user profile. For more information, please see https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/

Regards,
CLA GitHub bot

thelinuxfoundation · 2019-02-20T00:07:16Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @ankitshubham97 isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
CLA GitHub bot

ankitshubham97 · 2019-02-20T00:27:23Z

Filled in the CLA form.

This commit improves gRPC to run gRPC server with TLS enabled on AIX machines with IBMJSSE2 as their JDK SSL provider.

carl-mastrangelo

Code LGTM.

I am curious if this can get a test. We don't have an IBM JDK testbed set up so it might be unknowingly broken in the future.

sanjaypujare · 2019-02-20T06:21:26Z

The link in the PR comment doesn't work - the text is correct but the actual link is https://github.com/grpc/grpc-java/pull/issue#5369 which obviously is wrong.

netty/src/main/java/io/grpc/netty/GrpcSslContexts.java

sanjaypujare · 2019-02-20T06:47:29Z

netty/src/main/java/io/grpc/netty/GrpcSslContexts.java

@@ -305,3 +324,4 @@ static void ensureAlpnAndH2Enabled(
        alpnNegotiator.protocols());
  }
 }
+


Is the newline needed? I prefer to see only changes that are needed in the PR.

sanjaypujare · 2019-02-20T06:53:08Z

netty/src/main/java/io/grpc/netty/GrpcSslContexts.java

+      } else if (IBM_PROVIDER_NAME.equals(provider.getName())) {
+        if (JettyTlsUtil.isJettyAlpnConfigured()
+            || JettyTlsUtil.isJettyNpnConfigured()
+            || JettyTlsUtil.isJava9AlpnAvailable()) {


lines 285-287 are identical to lines 279-281. The checks seem to be independent of provider (the for loop iterator) and loop invariant. Can we create a helper method and also initialize the value outside the loop?

sanjaypujare · 2019-02-20T07:03:24Z

I agree with @carl-mastrangelo : it will be very desirable to have a couple of tests. One for SunJSSE (with the Sun JDK) and another one for IBMJSSE2 (with the IBM JDK?). If there are problems adding tests I would like to know

ankitshubham97 · 2019-02-25T09:04:18Z

@sanjaypujare I would be happy to add the tests. May I know what exactly should we be covering in the tests? Also, as @carl-mastrangelo earlier said that you do not have IBM test bed set up, how am I supposed to verify if the tests pass?

carl-mastrangelo · 2019-02-25T17:52:17Z

@ankitshubham97 Were you able to run our tests on this JVM? (I assume you have access to it). You should be able to run ./gradlew build and see the tests pass, making sure your JAVA_HOME is set properly.

As for what we want to see, the coverage numbers from codecov would be good.

Lastly, since we don't have access to this JVM ourselves, it would have to be clear up front that we may break support (accidentally, or from technical compromise) for it. We do want gRPC to run everywhere, but short of regular testing it would be best effort. Is that okay with you?

sanjaypujare · 2019-02-25T18:17:52Z

@ankitshubham97 I was thinking you could add tests that are enabled only for the IBM JVM and you are able to verify those tests. So you can verify that findProvider returns the right provider and configure returns the right SslContextBuilder. It will be really nice to be able to mock the SslContextBuilder so unit testing becomes easier.

ejona86

As far as tests, TlsTest could be extended to try out this IBM provider. It'd probably be easiest to have a big if in the case JDK section and have two code paths (within the case) depending on whether using SunJSSE or IBMJSSE2. (I say that mainly because I don't believe Jetty ALPN works with IBMJSSE2.)

ejona86 · 2019-03-01T05:13:14Z

netty/src/main/java/io/grpc/netty/GrpcSslContexts.java

@@ -225,6 +226,18 @@ public static SslContextBuilder configure(SslContextBuilder builder, Provider jd
        throw new IllegalArgumentException(
            SUN_PROVIDER_NAME + " selected, but Jetty NPN/ALPN unavailable");
      }
+    } else if (IBM_PROVIDER_NAME.equals(jdkProvider.getName())) {
+      // Jetty ALPN/NPN only supports one of NPN or ALPN
+      if (JettyTlsUtil.isJettyAlpnConfigured()) {


Does it actually support Jetty ALPN? I don't see anything about Jetty ALPN working for IBM and based on the IBM documentation, it seems like Jetty ALPN only works if using Oracle's JDK on IBM hardware. It looks like it has its own API for Java 8 (which is very similar to Jetty NPN/ALPN).

I would really like to know which of these is being selected when it works for you. I hope it is the isJava9AlpnAvailable() case.

My eventual goal in this part of the code was to use the Java 9+ API, when available, and check whether this specific provider supports ALPN by creating an sslEngine and calling getApplicationProtocol() (or similar); if it throws UnsupportedOperationException, then it doesn't support ALPN. That way we don't have to hard-code provider names (for the Java 9 ALPN API).

SunJSSE is hard-coded today because it must be hard-coded for Jetty ALPN/NPN. Java 9+ support was a community contribution and just followed most of the current flow. Unfortunately, just being on Java 9+ doesn't mean the provider supports ALPN, thus the need for eventually adding a getApplicationProtocol() check. But this had been "good enough" up until now since we know Java 9+'s SunJSSE implementation supports ALPN.

ejona86 · 2019-03-01T05:27:14Z

It also looks like we can add ibmjdk8 to the list of JDKs in .travis.yml. That way TlsTest could actually run against the IBM JRE.

sanjaypujare · 2020-04-09T17:19:23Z

What's the status of this PR? Any interest?

cmuchinsky · 2020-05-29T12:22:01Z

I just stumbled upon this and have a need for it as I'm using the IBM JDK, it seems like the solution might simply be to use SunJSSE and IBMJSSE2 interchangeably. I will try that out and if successful will open another PR.

ejona86 · 2020-09-18T17:56:26Z

Closing in favor of #7422

ankitshubham97 force-pushed the IBMJSSE2_support_branch branch from 4a8af86 to b1819c5 Compare February 20, 2019 00:07

io,grpc,netty: Add support for IBMJSSE2 provider

c0235e7

This commit improves gRPC to run gRPC server with TLS enabled on AIX machines with IBMJSSE2 as their JDK SSL provider.

ankitshubham97 force-pushed the IBMJSSE2_support_branch branch from b1819c5 to c0235e7 Compare February 20, 2019 00:34

carl-mastrangelo approved these changes Feb 20, 2019

View reviewed changes

carl-mastrangelo added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 20, 2019

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 20, 2019

sanjaypujare reviewed Feb 20, 2019

View reviewed changes

netty/src/main/java/io/grpc/netty/GrpcSslContexts.java Show resolved Hide resolved

sanjaypujare reviewed Feb 20, 2019

View reviewed changes

ejona86 reviewed Mar 1, 2019

View reviewed changes

kiwi1969 mentioned this pull request Sep 13, 2020

Test that newly-added Java 8 ALPN works #6997

Open

ejona86 mentioned this pull request Sep 15, 2020

Add support for IBMJSSE2 into grpc-java #7422

Merged

ejona86 closed this Sep 18, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

io,grpc,netty: Add support for IBMJSSE2 provider #5374

io,grpc,netty: Add support for IBMJSSE2 provider #5374

ankitshubham97 commented Feb 20, 2019

thelinuxfoundation commented Feb 20, 2019

thelinuxfoundation commented Feb 20, 2019

ankitshubham97 commented Feb 20, 2019

carl-mastrangelo left a comment

sanjaypujare commented Feb 20, 2019

sanjaypujare Feb 20, 2019

sanjaypujare Feb 20, 2019

sanjaypujare commented Feb 20, 2019

ankitshubham97 commented Feb 25, 2019

carl-mastrangelo commented Feb 25, 2019

sanjaypujare commented Feb 25, 2019

ejona86 left a comment

ejona86 Mar 1, 2019 •

edited

ejona86 commented Mar 1, 2019

sanjaypujare commented Apr 9, 2020

cmuchinsky commented May 29, 2020

ejona86 commented Sep 18, 2020

io,grpc,netty: Add support for IBMJSSE2 provider #5374

io,grpc,netty: Add support for IBMJSSE2 provider #5374

Conversation

ankitshubham97 commented Feb 20, 2019

thelinuxfoundation commented Feb 20, 2019

thelinuxfoundation commented Feb 20, 2019

ankitshubham97 commented Feb 20, 2019

carl-mastrangelo left a comment

Choose a reason for hiding this comment

sanjaypujare commented Feb 20, 2019

sanjaypujare Feb 20, 2019

Choose a reason for hiding this comment

sanjaypujare Feb 20, 2019

Choose a reason for hiding this comment

sanjaypujare commented Feb 20, 2019

ankitshubham97 commented Feb 25, 2019

carl-mastrangelo commented Feb 25, 2019

sanjaypujare commented Feb 25, 2019

ejona86 left a comment

Choose a reason for hiding this comment

ejona86 Mar 1, 2019 • edited

Choose a reason for hiding this comment

ejona86 commented Mar 1, 2019

sanjaypujare commented Apr 9, 2020

cmuchinsky commented May 29, 2020

ejona86 commented Sep 18, 2020

ejona86 Mar 1, 2019 •

edited