[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #7565: Enable mTLS for gRPC channel

[GitBox <gi...@apache.org>]

kezhenxu94 commented on a change in pull request #7565:
URL: https://github.com/apache/skywalking/pull/7565#discussion_r696385780



##########
File path: CHANGES.md
##########
@@ -18,6 +18,7 @@ Release Notes.
 * Fix kafka-reporter-plugin shade package conflict
 * Add all config items to `agent.conf` file for convenient containerization use cases.
 * Advanced Kafka Producer configuration enhancement.
+* Enable mTLS for gRPC channel.

Review comment:
       Maybe change `Enable` to `Support`? Because we don't enable it by default

##########
File path: oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/grpc/ssl/DynamicSslContext.java
##########
@@ -59,18 +63,25 @@ protected void updateContext(String caFile) {
         }
     }
 
-    @Override
-    protected void updateContext(final String privateKeyFile, final String certChainFile) {
+    protected void updateContext(final String privateKeyFile, final String certChainFile, final String trustedCAsFile) {
         try {
-            setCtx(GrpcSslContexts
-                .configure(SslContextBuilder
-                    .forServer(
-                        new FileInputStream(Paths.get(certChainFile).toFile()),
-                        PrivateKeyUtil.loadDecryptionKey(privateKeyFile)),
-                    SslProvider.OPENSSL)
-                .build());
+            SslContextBuilder builder = GrpcSslContexts.configure(
+                SslContextBuilder.forServer(
+                    new FileInputStream(Paths.get(certChainFile).toFile()),
+                    PrivateKeyUtil.loadDecryptionKey(privateKeyFile)
+                ),
+                SslProvider.OPENSSL
+            );
+
+            if (StringUtil.isNotEmpty(trustedCAsFile)) {
+                builder.trustManager(new FileInputStream(Paths.get(trustedCAsFile).toFile()))

Review comment:
       Same here

##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/remote/TLSChannelBuilder.java
##########
@@ -23,31 +23,59 @@
 import io.grpc.netty.NettyChannelBuilder;
 import io.netty.handler.ssl.SslContextBuilder;
 import java.io.File;
-import javax.net.ssl.SSLException;
+import java.io.FileInputStream;
+import java.io.IOException;
 import org.apache.skywalking.apm.agent.core.boot.AgentPackageNotFoundException;
 import org.apache.skywalking.apm.agent.core.boot.AgentPackagePath;
 import org.apache.skywalking.apm.agent.core.conf.Config;
-import org.apache.skywalking.apm.agent.core.conf.Constants;
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+import org.apache.skywalking.apm.agent.core.util.PrivateKeyUtil;
+import org.apache.skywalking.apm.util.StringUtil;
 
 /**
- * Detect the `/ca` folder in agent package, if `ca.crt` exists, start TLS (no mutual auth).
+ * If only ca.crt exists, start TLS. If cert, key and ca files exist, enable mTLS.
  */
 public class TLSChannelBuilder implements ChannelBuilder<NettyChannelBuilder> {
-    private static String CA_FILE_NAME = "ca" + Constants.PATH_SEPARATOR + "ca.crt";
+    private static final ILog LOGGER = LogManager.getLogger(TLSChannelBuilder.class);
 
     @Override
     public NettyChannelBuilder build(
-        NettyChannelBuilder managedChannelBuilder) throws AgentPackageNotFoundException, SSLException {
-        File caFile = new File(AgentPackagePath.getPath(), CA_FILE_NAME);
-        boolean isCAFileExist = caFile.exists() && caFile.isFile();
-        if (Config.Agent.FORCE_TLS || isCAFileExist) {
+        NettyChannelBuilder managedChannelBuilder) throws AgentPackageNotFoundException, IOException {
+
+        File caFile = new File(toAbsolutePath(Config.Agent.SSL_TRUSTED_CA_PATH));
+        if (Config.Agent.FORCE_TLS || caFile.isFile()) {
             SslContextBuilder builder = GrpcSslContexts.forClient();
-            if (isCAFileExist) {
+
+            if (caFile.isFile()) {
+                String certPath = Config.Agent.SSL_CERT_CHAIN_PATH;
+                String keyPath = Config.Agent.SSL_KEY_PATH;
+                if (StringUtil.isNotBlank(certPath) && StringUtil.isNotBlank(keyPath)) {
+                    File keyFile = new File(toAbsolutePath(keyPath));
+                    File certFile = new File(toAbsolutePath(certPath));
+                    if (certFile.isFile() && keyFile.isFile()) {
+                        builder.keyManager(new FileInputStream(certFile), PrivateKeyUtil.loadDecryptionKey(keyPath));

Review comment:
       Seems `builder.keyManager` doesn't close the passed-in `InputStream`? If so, should close it




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org