wu-sheng commented on a change in pull request #15:
URL: https://github.com/apache/skywalking-java/pull/15#discussion_r698491065
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/remote/TLSChannelBuilder.java
##########
@@ -23,31 +23,61 @@
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 AgentPackagePath.getPath() + path;
+ } else {
+ return AgentPackagePath.getPath() + "/" + path;
+ }
Review comment:
You deleted line `File caFile = new File(AgentPackagePath.getPath(),
CA_FILE_NAME);` is for avoiding this complex `if/else`. Could you share way
removing that line?
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/remote/TLSChannelBuilder.java
##########
@@ -23,31 +23,61 @@
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();
Review comment:
Also, why remove this? It seems you are copying `caFile.isFile()` in 2
places instead. I am confused about the principle of code change.
##########
File path: docs/en/setup/service-agent/java-agent/TLS.md
##########
@@ -8,19 +8,34 @@ at the same time, the SkyWalking backend is in another region
(VPC).
> Because of that, security requirement is very obvious.
## Authentication Mode
-Only support **no mutual auth**.
- Use this [script](../../../../../tools/TLS/tls_key_generate.sh) if you are
not familiar with how to generate key files.
-- Find `ca.crt`, and use it at client side
-- Find `server.crt` ,`server.pem` and `ca.crt`. Use them at server side.
Please refer to `gRPC SSL` of the OAP server doc.
+- Find `ca.crt`, and use it at client side. In `mTLS` mode, `client.crt` and
`client.pem` are required at client side.
+- Find `server.crt` ,`server.pem` and `ca.crt`. Use them at server side.
Please refer to `gRPC Security` of the OAP server doc.
for more details.
## 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 enables TLS automatically after the `ca.crt`(by default `/ca` folder
in agent package) file is 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 mTLS enabled. More details can be
found in `receiver-sharing-server` section in `application.yaml`. Please refer
to `gRPC Security` and `gRPC/HTTP server for receiver`.
+- Copy CA certificate, certificate and private key of client into `agent/ca`.
+- 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:/ca/ca.crt}
+agent.ssl_key_path=${SW_AGENT_SSL_KEY_PATH:/ca/client.pem}
Review comment:
It seems this
https://github.com/apache/skywalking-java/pull/15#discussion_r698096325 still
gets unresolved, could you share why?
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/remote/TLSChannelBuilder.java
##########
@@ -23,31 +23,61 @@
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();
Review comment:
The previous logic only calls the method once, even check twice. The
second API call is unnecessary.
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/remote/TLSChannelBuilder.java
##########
@@ -23,31 +23,61 @@
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 AgentPackagePath.getPath() + path;
+ } else {
+ return AgentPackagePath.getPath() + "/" + path;
+ }
Review comment:
Then your logic is back to
https://github.com/apache/skywalking-java/pull/15#discussion_r698096140. This
is not what we need to deal with, and it is endless.
And I am not meaning about configurable. My point is `File caFile = new
File(AgentPackagePath.getPath(), CA_FILE_NAME);`. This is usually the standard
way to process a file/path of a parent path. Let's keep in this way.
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/remote/TLSChannelBuilder.java
##########
@@ -23,31 +23,61 @@
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 AgentPackagePath.getPath() + path;
+ } else {
+ return AgentPackagePath.getPath() + "/" + path;
+ }
Review comment:
Once you did this in the codebase, we could have to face pull requests
or requests to change how to process file path in the whole project, which
could be confused.
##########
File path:
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/Config.java
##########
@@ -135,6 +135,21 @@
* Force open TLS for gRPC channel if true.
*/
public static boolean FORCE_TLS = false;
+
+ /**
+ * SSL trusted ca file. If it exists, will enable TLS for gRPC channel.
+ */
+ public static String SSL_TRUSTED_CA_PATH = "ca" +
Constants.PATH_SEPARATOR + "ca.crt";
+
+ /**
+ * Key cert chain file. If ssl_cert_chain and ssl_key exist, will
enable mTLS for gRPC channel.
+ */
+ public static String SSL_CERT_CHAIN_PATH;
+
+ /**
+ * Private key file. If ssl_cert_chain and ssl_key exist, will enable
mTLS for gRPC channel.
+ */
+ public static String SSL_KEY_PATH;
Review comment:
According to recent requirements, all configuration should be added in
default `agent.config`, with all system env variable names should be documents
in agent doc.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
