You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2023/01/19 10:39:05 UTC
[GitHub] [skywalking] yswdqz opened a new pull request, #10296: Add https
yswdqz opened a new pull request, #10296:
URL: https://github.com/apache/skywalking/pull/10296
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [] Update the documentation to include this new feature.
- [x] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
--
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
[GitHub] [skywalking] wu-sheng commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398122150
> > From my experience when users want to enable https they usually need a gateway to configure domain name and the port. Adding this to OAP makes it troublesome, for example, professional gateway can refresh expired cert without reboot but this is not possible in OAP for now as of your implementation.
>
> This is still my main concern, it doesn't look to be a production-ready implementation when you have to restart all the OAP instances when you want to renew the certificate
You are correct. Certification update thing should be considered if you want to take security very seriously. But do we have a good way to do in Java and armeria?
But I think this should not be a block. If users want to use a Gateway(we could document this for firehole-receiver), I am totally fine, and this feature would not block them to do so, right?
In another way, our gRPC service seems to have the same issue(can't update cert in the runtime), right? We would not call that not product-ready.
This reminds me, of when I wrote the security notice, https://skywalking.apache.org/docs/main/next/en/security/readme/. How many people would really put a Gateway before the SkyWalking OAP in the product? AFAIK, very few.
--
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
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081223732
##########
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:
<img width="808" alt="image" src="https://user-images.githubusercontent.com/5441976/213448204-3d859dc6-690c-4532-b51c-27f3732c9bc6.png">
I mean we have relative/similar configurations in gRPC part.
--
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
[GitHub] [skywalking] yswdqz commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081345389
##########
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:
So I don't need to change configuration item now ?
--
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
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by 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
[GitHub] [skywalking] kezhenxu94 commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398106425
> From my experience when users want to enable https they usually need a gateway to configure domain name and the port. Adding this to OAP makes it troublesome, for example, professional gateway can refresh expired cert without reboot but this is not possible in OAP for now as of your implementation.
This is still my main concern, it doesn't look to be a production-ready implementation when you have to restart all the OAP instances when you want to renew the certificate
--
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
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081347271
##########
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:
We don't need to add a domain name for now. But `enableTlsSelfSigned` seems to need to be removed.
--
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
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081209069
##########
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:
One question, today's gRPC seems not setting the domain name?
--
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
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081322512
##########
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:
We are actually using the server host here
https://github.com/apache/skywalking/blob/289427990aa857298e498782ad97a1d8db68ae4a/oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/grpc/GRPCServer.java#L86
After thinking twice I think I was wrong, we don't need to configure the host/domain name, as long as the CN field in the cert is the same as the hostname that are used to access this endpoint, we are good to go.
--
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
[GitHub] [skywalking] pg-yang commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398001600
> @pg-yang Have you tested this?
I'm using gateway(nginx) to test firehouse https, rather than OAP.
BTW, @yswdqz if you need domain,pulibc certificate, I could provide 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
[GitHub] [skywalking] yswdqz commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1396765973
I have not found docs about httpserver. Is there a doc I need to edit?
--
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
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081338049
##########
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:
> But still, we need to make it clear, do we support https for all other http server like Prometheus, Zipkin receiver, or do we want to only support that for AWS receiver? If we want to support for all, we need these configurations for each server respectively, otherwise we should move these configurations to that plugin.
For now, we plan to build a new HTTPS receiver inside `aws-firehose-receiver` separate from the existing core and sharing server HTTP server, also separate from Prometheus or Zipkin server.
We don't need to support all in HTTPS, at least not for now.
--
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
[GitHub] [skywalking] yswdqz commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1397909868
Is there anything else I should change?
--
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
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1082245628
##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##########
@@ -70,6 +74,22 @@ public void initialize() {
})
.decorator(LoggingService.newDecorator());
+ if (config.isEnableTLS()) {
+ sb.https(config.getPort());
Review Comment:
```suggestion
sb.https(new InetSocketAddress(
config.getHost(),
config.getPort()));
```
##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##########
@@ -70,6 +74,22 @@ public void initialize() {
})
.decorator(LoggingService.newDecorator());
+ if (config.isEnableTLS()) {
+ sb.https(config.getPort());
+ try (InputStream cert =
+ Files.newInputStream(Paths.get(config.getTlsCertChainPath())
+ .toFile().toPath());
Review Comment:
```suggestion
try (InputStream cert = new FileInputStream(config.getTlsCertChainPath());
```
--
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
[GitHub] [skywalking] wu-sheng commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398330679
> > > > In another way, our gRPC service seems to have the same issue(can't update cert in the runtime), right? We would not call that not product-ready.
> > >
> > >
> > > gRPC can reload the certs in about every 10 seconds.
> >
> >
> > How is `armeria`? Could it do that? If so, polish is good. If it still can't, I feel still fine.
>
> It’s not built in for gRPC, it’s skywalking’s implementation does the reloading work. Armeria doesn’t do that out of the box either. Feel free to merge
Got it. Saw `AbstractSslContext`. Yes, it is SkyWalking's implementation.
--
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
[GitHub] [skywalking] yswdqz commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by "yswdqz (via GitHub)" <gi...@apache.org>.
yswdqz commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1399180731
> @yswdqz I think we could refer gRPC to do similar in HTTP
>
> > Unfortunately, there is no easy way to update a SSL context at the moment.
> > One possible workaround is to reconfigure a whole Server with new certs.
> > https://github.com/line/armeria/blob/871d87297e4d051241589cb1ae95641cbc83f880/core/src/test/java/com/linecorp/armeria/client/ReconfigurableServerTest.java#L120
>
> This is from Armeria officially. I think we could follow this.
Got 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
[GitHub] [skywalking] yswdqz commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by "yswdqz (via GitHub)" <gi...@apache.org>.
yswdqz commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1399296647
@wu-sheng We also need to be like grpc, reload the certs in every 10 seconds?
--
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
[GitHub] [skywalking] wu-sheng commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1396810825
> I have not found docs about httpserver. Is there a doc I need to edit?
No, there isn't doc for a class.
But there are core and sharing server modules using this, if you are going to expose configurations(currently you seem not), you need to update relative docs.
--
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
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081327623
##########
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:
But still, we need to make it clear, do we support https for all other http server like Prometheus, Zipkin receiver, or do we want to only support that for AWS receiver? If we want to support for all, we need these configurations for each server respectively, otherwise we should move these configurations to that plugin.
--
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
[GitHub] [skywalking] yswdqz commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1082120917
##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##########
@@ -39,4 +39,12 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+ @Builder.Default
+ private boolean enableTLS = false;
+
+ private int httpsPort;
Review Comment:
For now I think use port is enough, because only `aws-receriver` need https, and it only support https.
So I modify now?
--
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
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1082121473
##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##########
@@ -39,4 +39,12 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+ @Builder.Default
+ private boolean enableTLS = false;
+
+ private int httpsPort;
Review Comment:
Yes, please.
--
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
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1082107524
##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServerConfig.java:
##########
@@ -39,4 +39,12 @@ public class HTTPServerConfig {
private int acceptQueueSize = 0;
@Builder.Default
private int maxRequestHeaderSize = 8192;
+
+ @Builder.Default
+ private boolean enableTLS = false;
+
+ private int httpsPort;
Review Comment:
I am wondering are we going to share tls and no tls server together? If not, we could use port, rather than using a new port.
By this, I feel we open an insecure port for http.
It is better we don't expose that.
--
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
[GitHub] [skywalking] kezhenxu94 commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398246733
> > > In another way, our gRPC service seems to have the same issue(can't update cert in the runtime), right? We would not call that not product-ready.
> >
> >
> > gRPC can reload the certs in about every 10 seconds.
>
> How is `armeria`? Could it do that? If so, polish is good. If it still can't, I feel still fine.
It’s not built in for gRPC, it’s skywalking’s implementation does the reloading work. Armeria doesn’t do that either out of the box either. Feel free to merge
--
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
[GitHub] [skywalking] wu-sheng merged pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #10296:
URL: https://github.com/apache/skywalking/pull/10296
--
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
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081217976
##########
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:
> One question, today's gRPC seems not setting the domain name?
We have no built-in https support in today's gRPC server inside OAP. In another word, users have to setup a gateway to support https for gRPC port 11800.
--
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
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#discussion_r1081123339
##########
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 tlsKeyPath, and tlsCertChainPath.
--
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
[GitHub] [skywalking] wu-sheng commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1397912275
@pg-yang Have you tested this?
--
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
[GitHub] [skywalking] kezhenxu94 commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398137826
> In another way, our gRPC service seems to have the same issue(can't update cert in the runtime), right? We would not call that not product-ready.
gRPC can reload the certs in about every 10 seconds.
--
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
[GitHub] [skywalking] wu-sheng commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1398144197
> > In another way, our gRPC service seems to have the same issue(can't update cert in the runtime), right? We would not call that not product-ready.
>
> gRPC can reload the certs in about every 10 seconds.
How is `armeria`? Could it do that? If so, polish is good. If it still can't, I feel still fine.
--
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
[GitHub] [skywalking] wu-sheng commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1399178286
@yswdqz I think we could refer gRPC to do similar in HTTP
> Unfortunately, there is no easy way to update a SSL context at the moment.
One possible workaround is to reconfigure a whole Server with new certs.
https://github.com/line/armeria/blob/871d87297e4d051241589cb1ae95641cbc83f880/core/src/test/java/com/linecorp/armeria/client/ReconfigurableServerTest.java#L120
This is from Armeria officially. I think we could follow this.
--
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
[GitHub] [skywalking] wu-sheng commented on pull request #10296: Enhance OAP HTTP server to support HTTPS
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10296:
URL: https://github.com/apache/skywalking/pull/10296#issuecomment-1399350011
Like we discussed, it is better to be. Because in real prod env, cert file has expired time, and we should not have to reboot OAP for it, like Zhenxu mentioned.
--
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