[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS

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

kezhenxu94 commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081116842


##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##########
@@ -70,6 +78,21 @@ public void initialize() {
             })
             .decorator(LoggingService.newDecorator());
 
+        if (config.isEnableTLS()) {
+            sb.https(config.getHttpsPort());
+            if (config.isEnableTlsSelfSigned()) {
+                sb.tlsSelfSigned();

Review Comment:
   It's impossible to use self signed certificate for services that are exposed to external consumer, i.e. aws won't definitely skip verify certificate like you do in the unit tests, so I think this is useless in reality.



##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##########
@@ -39,4 +39,14 @@ public class HTTPServerConfig {
     private int acceptQueueSize = 0;
     @Builder.Default
     private int maxRequestHeaderSize = 8192;
+
+    @Builder.Default
+    private boolean enableTLS = false;
+    @Builder.Default
+    private boolean enableTlsSelfSigned = false;
+
+    private int httpsPort;
+    private String tlsKeyPath;
+    private String tlsCertChainPath;

Review Comment:
   All these configurations are domain-name:port specific, i.e. every domain name:port should have the corresponding https port, tlsKeyPath, tlsCertChainPath. 



##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##########
@@ -70,6 +78,21 @@ public void initialize() {
             })
             .decorator(LoggingService.newDecorator());
 
+        if (config.isEnableTLS()) {
+            sb.https(config.getHttpsPort());
+            if (config.isEnableTlsSelfSigned()) {
+                sb.tlsSelfSigned();
+            } else {
+                try (InputStream cert =
+                             Files.newInputStream(Paths.get(config.getTlsCertChainPath())
+                                     .toFile().toPath());
+                     InputStream key = PrivateKeyUtil.loadDecryptionKey(config.getTlsKeyPath())) {
+                    sb.tls(cert, key);

Review Comment:
   Https certificates are bound to a domain name so you have to add a configuration item to configure virtual host for that cert. 



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