Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: mTLS server and client setup and docs #1781

Merged
merged 12 commits into from
May 5, 2023
Merged

Conversation

johanandren
Copy link
Member

@johanandren johanandren commented May 2, 2023

Includes fix for #1728

johanandren and others added 2 commits May 3, 2023 11:14
Co-authored-by: Patrik Nordwall <patrik.nordwall@gmail.com>
@johanandren
Copy link
Member Author

Caveat: not sure the added SSL context setup doesn't break all kinds of things when not doing mTLS, hoping CI will tell me

@johanandren johanandren marked this pull request as ready for review May 3, 2023 15:36
@johanandren johanandren changed the title doc: WIP mTLS server setup in Scala and certs prepared doc: mTLS server and client setup May 4, 2023
@johanandren johanandren changed the title doc: mTLS server and client setup fix: mTLS server and client setup and docs May 4, 2023
@johanandren
Copy link
Member Author

Some more testing/digging, the client SSLContext translation only happens when a custom SSLContext is defined, so regular use without that is completely unaffected by this, and I think all such customisation was broken based on what @patriknw noticed when trying to set up a custom context to accept a self-signed cert, which then worked with this patch.

So: ready for final review/merge

@johanandren
Copy link
Member Author

I'll add a test covering this as well, so we notice if we break TLS setup again

@patriknw
Copy link
Member

patriknw commented May 5, 2023

Caveat: not sure the added SSL context setup doesn't break all kinds of things when not doing mTLS

I tested this with TLS but without mTLS and it works.

@johanandren
Copy link
Member Author

We could add an additional test for custom SSL only, but I think what that would catch is already covered by the mTLS test I wrote yesterday.

Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, after something minor in the samples

try {
BufferedInputStream in = new BufferedInputStream(MtlsGreeterServer.class.getResourceAsStream(path));
ByteArrayOutputStream bao = new ByteArrayOutputStream();
for (int result = in.read(); result != -1; result = in.read()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is text, shouldn't it use a Reader instead? Random example https://www.baeldung.com/convert-input-stream-to-string

Also, this is missing close() on the in. I think they are Closable so placing in try (...) { should automatically close.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Server cert for localhost signed by rootCA in localhost-server.*, no password for private key
Client cert for a client to connect in localhost-client.*, no password for private key

Certs used by `MtlsGreeterServer`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, should we include the commands for how the test certs were created?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -19,8 +19,8 @@ object Dependencies {
// https://doc.akka.io//docs/akka/current/project/downstream-upgrade-strategy.html
val akka = "2.7.0"
val akkaBinary = "2.7"
val akkaHttp = "10.5.0-M1"
val akkaHttpBinary = "10.4"
val akkaHttp = "10.5.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure the release notes includes this change

@johanandren johanandren merged commit 6133650 into main May 5, 2023
@johanandren johanandren deleted the wip-document-mtls branch May 5, 2023 09:37
@ihostage
Copy link
Contributor

@patriknw @johanandren
Hi, friends! 👋
Any chance that this fix will be backported to 2.1.x and released as 2.1.7 with APL 2.0?

Context: Play gRPC stucks on 2.1.5 and can't bump to 2.1.6 by this problem 😞

@johanandren
Copy link
Member Author

I'm afraid we have no plans to backport any functionality added in 2.2 and later to the 2.1 branch

@ihostage
Copy link
Contributor

I'm afraid we have no plans to backport any functionality added in 2.2 and later to the 2.1 branch

Will release be possible if I will do the small (without docs and samples) backport itself?

@ihostage
Copy link
Contributor

I tested locally and it looks as if we need only this piece of changes, nothing more

Subject: [PATCH] Backport #1781
---
Index: runtime/src/main/scala/akka/grpc/internal/NettyClientUtils.scala
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/runtime/src/main/scala/akka/grpc/internal/NettyClientUtils.scala b/runtime/src/main/scala/akka/grpc/internal/NettyClientUtils.scala
--- a/runtime/src/main/scala/akka/grpc/internal/NettyClientUtils.scala	(revision accf37ccc97205d588742ced4bcc7d1ed7499142)
+++ b/runtime/src/main/scala/akka/grpc/internal/NettyClientUtils.scala	(revision 1943f6dc29418f3332203fa3143b959aa60fd512)
@@ -5,7 +5,6 @@
 package akka.grpc.internal
 
 import java.util.concurrent.TimeUnit
-
 import javax.net.ssl.SSLContext
 import akka.{ Done, NotUsed }
 import akka.annotation.InternalApi
@@ -16,7 +15,17 @@
 import io.grpc.netty.shaded.io.grpc.netty.GrpcSslContexts
 import io.grpc.netty.shaded.io.grpc.netty.NegotiationType
 import io.grpc.netty.shaded.io.grpc.netty.NettyChannelBuilder
-import io.grpc.netty.shaded.io.netty.handler.ssl.{ SslContext, SslContextBuilder }
+import io.grpc.netty.shaded.io.netty.handler.ssl.ApplicationProtocolConfig.{
+  Protocol,
+  SelectedListenerFailureBehavior,
+  SelectorFailureBehavior
+}
+import io.grpc.netty.shaded.io.netty.handler.ssl.{
+  ApplicationProtocolConfig,
+  ApplicationProtocolNames,
+  SslContext,
+  SslContextBuilder
+}
 
 import scala.annotation.nowarn
 import scala.concurrent.duration.FiniteDuration
@@ -172,23 +181,24 @@
    */
   @InternalApi
   private def createNettySslContext(javaSslContext: SSLContext): SslContext = {
-    import io.grpc.netty.shaded.io.netty.handler.ssl.{
-      ApplicationProtocolConfig,
-      ClientAuth,
-      IdentityCipherSuiteFilter,
-      JdkSslContext
-    }
+    import io.grpc.netty.shaded.io.netty.handler.ssl.{ ClientAuth, IdentityCipherSuiteFilter, JdkSslContext }
     // See
     // https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java#L229-L309
-    new JdkSslContext(
+    val apn = new ApplicationProtocolConfig(
+      Protocol.ALPN,
+      SelectorFailureBehavior.NO_ADVERTISE,
+      SelectedListenerFailureBehavior.ACCEPT,
+      ApplicationProtocolNames.HTTP_2)
+    val context = new JdkSslContext(
       javaSslContext,
       /* boolean isClient */ true,
       /* Iterable<String> ciphers */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
       IdentityCipherSuiteFilter.INSTANCE,
-      /* ApplicationProtocolConfig apn */ ApplicationProtocolConfig.DISABLED, // use JDK default (null would also be acceptable, DISABLED config will select the NONE protocol and thus the JdkDefaultApplicationProtocolNegotiator)
-      ClientAuth.NONE, // server-only option, which is ignored as isClient=true (as indicated in constructor Javadoc)
+      /* ApplicationProtocolConfig apn */ apn,
+      ClientAuth.OPTIONAL, // server-only option, which is ignored as isClient=true (as indicated in constructor Javadoc)
       /* String[] protocols */ null, // use JDK defaults (null is accepted as indicated in constructor Javadoc)
       /* boolean startTls */ false)
+    context
   }
 
   /**

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants