[GitHub] [skywalking-java] hanahmily commented on a change in pull request #15: Support mTLS for gRPC channel

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

hanahmily commented on a change in pull request #15:
URL: https://github.com/apache/skywalking-java/pull/15#discussion_r698096140



##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/remote/TLSChannelBuilder.java
##########
@@ -23,31 +23,64 @@
 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 java.io.InputStream;
 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()) {
+                        try (InputStream cert = new FileInputStream(certFile);
+                             InputStream key = PrivateKeyUtil.loadDecryptionKey(keyPath)) {
+                            builder.keyManager(cert, key);
+                        }
+                    }
+                    else if (!certFile.isFile() || !keyFile.isFile()) {
+                        LOGGER.warn("Failed to enable mTLS caused by cert or key cannot be found.");
+                    }
+                }
+
                 builder.trustManager(caFile);
             }
-            managedChannelBuilder = managedChannelBuilder.negotiationType(NegotiationType.TLS)
-                                                         .sslContext(builder.build());
+            managedChannelBuilder.negotiationType(NegotiationType.TLS).sslContext(builder.build());
         }
         return managedChannelBuilder;
     }
+
+    private static String toAbsolutePath(final String path) throws AgentPackageNotFoundException {
+        if (path.startsWith("/")) {
+            return path;
+        } else if (path.startsWith("./")) {

Review comment:
       How to handle other relative paths, for instance, `../`, `.././..` and etc?
   
   What I mean is we don't have to parse relative paths, just appending the agent package path to it looks like a tidier way

##########
File path: docs/en/setup/service-agent/java-agent/TLS.md
##########
@@ -17,10 +16,23 @@ Only support **no mutual auth**.
 ## Open and config TLS
 
 ### Agent config
-- Place `ca.crt` into `/ca` folder in agent package. Notice, `/ca` is not created in distribution, please create it by yourself.
-
-- Agent open TLS automatically after the `/ca/ca.crt` file detected.
+- Agent open TLS automatically after the `ca.crt`(by default `/ca` folder in agent package) file detected.
 - TLS with no CA mode could be activated by this setting.
 ```
-agent.force_tls=${SW_AGENT_FORCE_TLS:false}
+agent.force_tls=${SW_AGENT_FORCE_TLS:true}
+```
+
+## Enable mutual TLS
+
+- Sharing gRPC server must be started with enabled mTLS. More details see `receiver-sharing-server` section in `application.yaml` of SkyWalking OAP Server.  
+- Configure Client-side SSL/TLS in `agent.conf`.
+- Change `SW_AGENT_COLLECTOR_BACKEND_SERVICES` targeting to host and port of `receiver-sharing-server`.
+
+For example:
 ```
+agent.force_tls=${SW_AGENT_FORCE_TLS:true}
+agent.ssl_trusted_ca_path=${SW_AGENT_SSL_TRUSTED_CA_PATH:/path/to/ca.crt}
+agent.ssl_key_path=${SW_AGENT_SSL_KEY_PATH:/path/to/client.pem}

Review comment:
       We should document which private key format is supported just like the server-side.




-- 
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