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 2021/11/24 03:34:11 UTC
[GitHub] [skywalking] chanjarster opened a new pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
chanjarster opened a new pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179
<!--
⚠️ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
<!-- ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👇 ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking/blob/master/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👆 ==== -->
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [x] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #8154.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/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] chanjarster commented on pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
chanjarster commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-977475470
Sure, it's an insecure way. But skip TLS host verify is always an option for TLS client, Netty, HttpClient, [OpenTelemetry exporters][1] all support it.
In consider the secure issue, so I make this option `false` by default. One should turn it on explicitly if really want to.
[1]: https://opentelemetry.io/docs/collector/configuration/
--
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] tirelibirefe commented on pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
tirelibirefe commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-1004635056
Hello,
Is there a way using ElasticSearch as backend if a self-signed certificate is used with ES or may we say "Skywalking doesn't support ElasticSearch with self-signed certificates?"
(I don't know there is anyone using ElasticSearch with ssl gotten from a public cert authority in Kubernetes.)
Thanks
--
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] chanjarster commented on pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
chanjarster commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-981013127
I explained before. Below is the use case:
Deploy SkyWalking in K8S by using [chart][1], using an existing ElasticSearch instance which is https enabled and using self signed key.
So how can you give SkyWalking a trust store which containing the ElasticSearch's CA by using environment?
3 Options:
1. Easy way: just tell SkyWalking to skip TLS host verify.
2. Middle way: give SkyWalking a PEM encoded X.509 certificate to trust by using an env variable.
3. Hardest way, and also SkyWalking currently support: make a trust store with the CA certificate, put that trust store into SkyWalking OAP server container. Apparently this way is not suitable for K8S deploy.
So I implement Option 1.
Option 2 is good too, I'll implement in another PR.
[1]: https://github.com/apache/skywalking-kubernetes/blob/master/chart/skywalking/README.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] chanjarster commented on pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
chanjarster commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-981054716
> Maybe some blogs could be written.
> I would like to recommend you using sidecar injection. It is really good.
I'm looking forward to 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] wu-sheng commented on pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-981050900
>> We didn't know you think accessing inside k8s is a challenge, all our deployments are in k8s, and nothing blocks us.
Where is the doc about the situation I've mentioned? I struggled on this for several days.
There isn't. This is not SkyWalking's scope of guiding how to use K8s to manage security. Sorry. Maybe some blogs could be written.
> Yes. And let me correct it: put the base64 encoded content of the cert as the value of system env.
Even with sidecar injection, do you still want this? From my side, this is not a cloud native style solution. I may need to think more about whether this is a security leak. Besides that, I would like to recommend you using sidecar injection. It is really good.
--
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 #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-981020834
> I explained before. Below is the use case:
>
> Deploy SkyWalking in K8S by using [chart](https://github.com/apache/skywalking-kubernetes/blob/master/chart/skywalking/README.md), using an existing ElasticSearch instance which is https enabled and using self signed key.
>
> So how can you give SkyWalking a trust store which containing the ElasticSearch's CA by using environment?
>
> 3 Options:
>
> 1. Easy way: just tell SkyWalking to skip TLS host verify.
> 2. Middle way: give SkyWalking a PEM encoded X.509 certificate to trust by using an env variable.
> 3. Hardest way, and also SkyWalking currently support: make a trust store with the CA certificate, put that trust store into SkyWalking OAP server container. Apparently this way is not suitable for K8S deploy.
>
> So I implement Option 1.
>
> Option 2 is good too, I'll implement in another PR.
This is not accurate, I am afraid. There are plenty of sidecar tech could inject files into docker inside, such as Vault(just an example). I can't see why this is hard. But you should know, this is correct and real TLS should be done.
--
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 #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-977473377
OK, now I can understand your proposal. But still not sure why. This kind of TLS ignore seems a security leak.
cc @apache/skywalking-committers
--
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 change in pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#discussion_r757822397
##########
File path: oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/StorageModuleElasticsearchConfig.java
##########
@@ -101,6 +101,12 @@
* @since 7.0.0 This could be managed inside {@link #secretsManagementFile}
*/
private String trustStorePass;
+
+ /**
+ * @since 8.8.2 Skip tls host verify on elasticsearch server
Review comment:
```suggestion
* @since 8.9.0 Skip tls host verify on ElasticSearch server host
```
##########
File path: oap-server/server-library/library-client/src/main/java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java
##########
@@ -67,6 +67,9 @@
@Setter
private volatile String trustStorePass;
+ @Setter
+ private boolean skipHostVerify;
Review comment:
```suggestion
private volatile boolean skipHostVerify;
```
--
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 #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-980808027
I want to discuss is, why need this configuration. Could you explain the use case?
--
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 #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-981017652
Considering you don't provide any case after you submitted the PR, even I asked for several times.
I have a talk with @hanahmily.
The only case I got is you are trying to access ES server through unofficial domain name.
But this seems insecurity way, which violates the original purpose of TLS.
We are a deliverable package, rather than a library. Once we have this, this could be reported as a CVE, even as optional, because it doesn't fit the TLS purpose.
So, I think we need to reject this feature. If you have more cases to provide, we could continue from there.
--
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] chanjarster commented on pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
chanjarster commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-981042774
> The only case I got is you are trying to access ES server through unofficial domain name.
Why I need a OFFICIAL domain name for a ElasticSearch for internal usage?
> There are plenty of sidecar tech could inject files into docker inside, such as Vault(just an example). I can't see why this is hard.
Well, I won't argue about it.
So, please look at Option 2, is that easier than sidecar injection?
> give SkyWalking a PEM encoded X.509 certificate to trust by using an env variable.
--
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 edited a comment on pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng edited a comment on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-977497875
> But skip TLS host verify is always an option for TLS client, Netty, HttpClient, OpenTelemetry exporters all support it.
I can NOT represent other open source projects' authors. I believe they have their reasons. I just don't feel right from SkyWalking's perspective. Anyway, let's wait for others' idea.
--
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 #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-977494219
Personally I don't like it, no matter default on or off. If you want to skip the security, HTTP proxy is there for you.(You could set up HTTP proxy between SkyWalking OAP and ElasticSearch server).
A security check is open at the server side, but client skips it because of?
--
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 edited a comment on pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng edited a comment on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-981020834
> I explained before. Below is the use case:
>
> Deploy SkyWalking in K8S by using [chart](https://github.com/apache/skywalking-kubernetes/blob/master/chart/skywalking/README.md), using an existing ElasticSearch instance which is https enabled and using self signed key.
>
> So how can you give SkyWalking a trust store which containing the ElasticSearch's CA by using environment?
>
> 3 Options:
>
> 1. Easy way: just tell SkyWalking to skip TLS host verify.
> 2. Middle way: give SkyWalking a PEM encoded X.509 certificate to trust by using an env variable.
> 3. Hardest way, and also SkyWalking currently support: make a trust store with the CA certificate, put that trust store into SkyWalking OAP server container. Apparently this way is not suitable for K8S deploy.
>
> So I implement Option 1.
>
> Option 2 is good too, I'll implement in another PR.
This is not accurate, I am afraid. There are plenty of sidecar tech could inject files into docker inside, such as Vault(just an example). I can't see why this is hard. More importantly you should know, this is correct and real TLS should be done.
--
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 closed pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng closed pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179
--
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 #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-977497875
> But skip TLS host verify is always an option for TLS client, Netty, HttpClient, OpenTelemetry exporters all support it.
I can represent other open source projects' authors. I believe they have their reasons. I just don't feel right from SkyWalking's perspective. Anyway, let's wait for others' idea.
--
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 #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-977495567
I am going to wait for @kezhenxu94 @hanahmily @dmsolr @JaredTan95 or any other committers' idea.
--
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] chanjarster commented on pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
chanjarster commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-981050354
> We didn't know you think accessing inside k8s is a challenge, all our deployments are in k8s, and nothing blocks us.
Where is the doc about the situation I've mentioned? I struggled on this for several days.
> Do you mean put the content of certification as the value of system env?
Yes. And let me correct it: put the base64 encoded content of the cert as the value of system env.
--
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 #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-981046208
> Why I need a OFFICIAL domain name for a ElasticSearch for internal usage?
This was what we are guessing why you need this. You could skip this. We didn't know you think accessing inside k8s is a challenge, all our deployments are in k8s, and nothing blocks us.
--
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] chanjarster commented on pull request #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
chanjarster commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-981025050
What happened? The use case provided:
> Deploy SkyWalking in K8S by using chart, using an existing ElasticSearch instance which is https enabled and using self signed key.
In this case SkyWalking OAP server CANNOT connect to ElasticSearch and failed to start.
--
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 #8179: [Feature] Support skip TLS host verify on ElasticSearch
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8179:
URL: https://github.com/apache/skywalking/pull/8179#issuecomment-981047754
> So, please look at Option 2, is that easier than sidecar injection?
give SkyWalking a PEM encoded X.509 certificate to trust by using an env variable.
Do you mean put the content of certification as the value of system env?
--
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