[Heartbeat] Add tls fields when connecting through proxy #22190

adriansr · 2020-10-27T13:49:35Z

What does this PR do?

This updates Heartbeat to enrich an event with TLS information when the connection has been established via an HTTP proxy.

Why is it important?

Because tls fields are missing from events when an HTTPS connection is performed through a proxy.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Tested with tinyproxy (HTTP) and stunnel+tinyproxy (HTTPS)

diff --git a/heartbeat/heartbeat.yml b/heartbeat/heartbeat.yml
index 19c3f79e96..3ed6e835e1 100644
--- a/heartbeat/heartbeat.yml
+++ b/heartbeat/heartbeat.yml
@@ -27,7 +27,9 @@ heartbeat.monitors:
   # Human readable display name for this service in Uptime UI and elsewhere
   name: My Monitor
   # List or urls to query
-  urls: ["http://localhost:9200"]
+  urls: ["https://elastic.co/"]
+  proxy_url: "https://localhost:3333/"
+  ssl.verification_mode: none
   # Configure task schedule
   schedule: '@every 10s'
   # Total test connection and data exchange timeout

Related issues

Closes #15797

This updates Heartbeat to enrich an event with TLS information when the connection has been established via an HTTP proxy. Closes elastic#15797

elasticmachine · 2020-10-27T14:29:13Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: [Pull request #22190 updated]
Start Time: 2020-11-23T10:28:17.431+0000
Duration: 34 min 58 sec

Test stats 🧪

Test	Results
Failed	0
Passed	929
Skipped	13
Total	942

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	929
Skipped	13
Total	942

andrewvc · 2020-10-27T20:31:46Z

Thanks for the contribution! Out of curiosity (since setting up a forward proxy is such a pain), did you confirm that this records the TLS info for the site being tested and not the proxy itself? It'd be great if we could write a test around this, we do have some existing tests that setup forward proxies using go itself.

adriansr · 2020-10-30T16:40:29Z

Yes, it always records the TLS info for the target site, not the HTTPS proxy, and it also works when the connection to the proxy is HTTP. I'll see if I can add a test

adriansr

Added those tests. @andrewvc please have a look

adriansr · 2020-10-30T18:09:40Z

heartbeat/monitors/active/http/http_test.go

+	// When connecting through a proxy, the following fields are missing.
+	if _, isProxy := reqExtraConfig["proxy_url"]; isProxy {
+		missing := map[string]interface{}{
+			"http.rtt.response_header.us": time.Duration(0),
+			"http.rtt.content.us":         time.Duration(0),
+			"monitor.ip":                  "127.0.0.1",
+			"tcp.rtt.connect.us":          time.Duration(0),
+			"http.rtt.validate.us":        time.Duration(0),
+			"http.rtt.write_request.us":   time.Duration(0),
+			"tls.rtt.handshake.us":        time.Duration(0),
+		}


I'm not sure why these fields are missing when proxy_url is defined. monitor.ip worries me.

IIRC we intentionally drop monitor.ip because the IP we've connected to is masked by the proxy. There may be ways to pull that out of special HTTP headers however.

I haven't looked at that code in a while TBH, and it's never been a concern from users (maybe till now). IIRC there were issues with the timings being relevant.

TCP connection time is not really as relevant either as that would measure TCP connection time to the proxy, for instance. If this is an area where you'd like to go deep and add relevant test cases we'd be glad to accept a patch, however, as far as I'm aware there isn't huge demand.

elasticmachine · 2020-10-30T18:35:03Z

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	917
Skipped	13
Total	930

elasticmachine · 2020-11-02T07:50:02Z

Pinging @elastic/uptime (Team:Uptime)

adriansr · 2020-11-23T10:52:32Z

@andrewvc are you OK with merging this?

andrewvc

LGTM. Apologies for the delayed review . This is a fantastic improvement

This updates Heartbeat to enrich an event with TLS information when the connection has been established via an HTTP proxy. Closes elastic#15797 (cherry picked from commit cd16ca0)

…2881) This updates Heartbeat to enrich an event with TLS information when the connection has been established via an HTTP proxy. Closes #15797 (cherry picked from commit cd16ca0)

Heartbeat: Add tls fields when connecting through proxy

c9ff368

This updates Heartbeat to enrich an event with TLS information when the connection has been established via an HTTP proxy. Closes elastic#15797

adriansr added enhancement Heartbeat labels Oct 27, 2020

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 27, 2020

adriansr added the Team:Security-External Integrations label Oct 27, 2020

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 27, 2020

adriansr added Team:obs-ds-hosted-services Label for the Observability Hosted Services team and removed Team:Security-External Integrations labels Oct 27, 2020

adriansr requested a review from andrewvc October 27, 2020 15:26

Add test

bc75a04

adriansr commented Oct 30, 2020

View reviewed changes

adriansr marked this pull request as ready for review November 2, 2020 07:50

adriansr requested a review from a team as a code owner November 2, 2020 07:50

adriansr added the review label Nov 2, 2020

adriansr added 2 commits November 2, 2020 08:53

Changelog entry

68418e5

Merge branch 'master' into hb_tls_proxy

942598b

andrewvc approved these changes Dec 2, 2020

View reviewed changes

adriansr merged commit cd16ca0 into elastic:master Dec 3, 2020

adriansr mentioned this pull request Dec 3, 2020

Cherry-pick #22190 to 7.x: [Heartbeat] Add tls fields when connecting through proxy #22881

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Heartbeat] Add tls fields when connecting through proxy #22190

[Heartbeat] Add tls fields when connecting through proxy #22190

adriansr commented Oct 27, 2020 •

edited

Loading

elasticmachine commented Oct 27, 2020 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

Test stats 🧪

andrewvc commented Oct 27, 2020

adriansr commented Oct 30, 2020

adriansr left a comment

adriansr Oct 30, 2020

andrewvc Nov 2, 2020 •

edited

Loading

elasticmachine commented Oct 30, 2020

Test stats 🧪

elasticmachine commented Nov 2, 2020

adriansr commented Nov 23, 2020

andrewvc left a comment

[Heartbeat] Add tls fields when connecting through proxy #22190

[Heartbeat] Add tls fields when connecting through proxy #22190

Conversation

adriansr commented Oct 27, 2020 • edited Loading

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

elasticmachine commented Oct 27, 2020 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

Test stats 🧪

andrewvc commented Oct 27, 2020

adriansr commented Oct 30, 2020

adriansr left a comment

Choose a reason for hiding this comment

adriansr Oct 30, 2020

Choose a reason for hiding this comment

andrewvc Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Oct 30, 2020

💚 Flaky test report

Test stats 🧪

elasticmachine commented Nov 2, 2020

adriansr commented Nov 23, 2020

andrewvc left a comment

Choose a reason for hiding this comment

adriansr commented Oct 27, 2020 •

edited

Loading

elasticmachine commented Oct 27, 2020 •

edited by jenkins-beats-ci bot

Loading

andrewvc Nov 2, 2020 •

edited

Loading