[heartbeat][libbeat][metricbeat][filebeat] Pass TLS options to forward proxies #15516
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewvc
added
bug
in progress
|
Pinging @elastic/uptime (:uptime) |
andrewvc
added
review
and removed
in progress
urso
reviewed
Jan 24, 2020
| @@ -138,6 +138,7 @@ func newRoundTripper(config *Config, tls *transport.TLSConfig) (*http.Transport, | |||
| Proxy: proxy, | |||
| Dial: dialer.Dial, | |||
| DialTLS: tlsDialer.Dial, | |||
| TLSClientConfig: tls.ToConfig(), | |||
There was a problem hiding this comment.
Can you add this change to some other packages, please?
libbeat/output/elasticsearchmetricbeat/helper
andrewvc
changed the title
|
@urso should be ready for re-review, I added this setting to every other spot I saw |
andrewvc
added a commit
to andrewvc/beats
that referenced
this pull request
Jan 27, 2020
…d proxies (elastic#15516) Fixes a bug where TLS options would not be passed to forward proxies. This fixes a reported issue where x509 client cert auth doesn't work due to the cert parameters not being passed in. Since this doesn't give us a full dialer we can't easily record TLS certificate expiry with proxies in heartbeat with this approach, so those fields would be missing. I think it makes sense to merge this change as-is and fix that secondary bug in a subsequent PR. I've opened a separate issue ( elastic#15797 ) to cover that. I've tested this manually against with the following config: https://github.com/elastic/uptime-contrib/tree/master/testing/configs/client-auth , note that you'll need the included client certs in that directory, and will need to alter the YAML to point at their actual path. I think in this scenario I'm fine with only having unit tests because the complexity of setting of a forward proxy + a custom TLS backend is very high. Fixes elastic#15524 (cherry picked from commit 0e57f6b)
5 tasks
andrewvc
added a commit
to andrewvc/beats
that referenced
this pull request
Jan 27, 2020
…d proxies (elastic#15516) Fixes a bug where TLS options would not be passed to forward proxies. This fixes a reported issue where x509 client cert auth doesn't work due to the cert parameters not being passed in. Since this doesn't give us a full dialer we can't easily record TLS certificate expiry with proxies in heartbeat with this approach, so those fields would be missing. I think it makes sense to merge this change as-is and fix that secondary bug in a subsequent PR. I've opened a separate issue ( elastic#15797 ) to cover that. I've tested this manually against with the following config: https://github.com/elastic/uptime-contrib/tree/master/testing/configs/client-auth , note that you'll need the included client certs in that directory, and will need to alter the YAML to point at their actual path. I think in this scenario I'm fine with only having unit tests because the complexity of setting of a forward proxy + a custom TLS backend is very high. Fixes elastic#15524 (cherry picked from commit 0e57f6b)
5 tasks
simitt
added a commit
to simitt/apm-server
that referenced
this pull request
Feb 3, 2020
Follow up on elastic/beats#15516 to pass TLS options to forward proxies.
simitt
added a commit
to elastic/apm-server
that referenced
this pull request
Feb 4, 2020
Follow up on elastic/beats#15516 to pass TLS options to forward proxies.
simitt
added a commit
to simitt/apm-server
that referenced
this pull request
Feb 4, 2020
Follow up on elastic/beats#15516 to pass TLS options to forward proxies.
simitt
added a commit
to elastic/apm-server
that referenced
this pull request
Feb 4, 2020
Follow up on elastic/beats#15516 to pass TLS options to forward proxies.
simitt
added a commit
to simitt/apm-server
that referenced
this pull request
Feb 4, 2020
Follow up on elastic/beats#15516 to pass TLS options to forward proxies.
simitt
added a commit
to elastic/apm-server
that referenced
this pull request
Feb 10, 2020
* Update beats framework to 32f1a9e * Update TLS config for elasticsearch client (#3278) Follow up on elastic/beats#15516 to pass TLS options to forward proxies.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a bug where TLS options would not be passed to forward proxies. This fixes a reported issue where x509 client cert auth doesn't work due to the cert parameters not being passed in.
Since this doesn't give us a full dialer we can't easily record TLS certificate expiry with proxies with this approach, so those fields would be missing. I think it makes sense to merge this change as-is and fix that secondary bug in a subsequent PR. I've opened a separate issue ( #15797 ) to cover that.
I've tested this manually against with the following config: https://github.com/elastic/uptime-contrib/tree/master/testing/configs/client-auth , note that you'll need the included client certs in that directory, and will need to alter the YAML to point at their actual path.
I think in this scenario I'm fine with only having unit tests because the complexity of setting of a forward proxy + a custom TLS backend is very high.
Fixes #15524