[Heartbeat] Fix expiration of cert chains calculation #33231

andrewvc · 2022-09-30T03:05:48Z

This is a bit WIP in that I'm not certain this is the final logic we want. It appears the elastic agent libs override the default golang TLS implementation's verification and only set the cert chains attribute on the connection when strict validation is enabled, where the default is full. See https://www.elastic.co/guide/en/beats/heartbeat/current/configuration-ssl.html for definition of the various validation options.

I suspect we'd be best off making strict the default since this is what chrome does nowadays, requiring a SAN to be set, which is the only distinction between the checks of strict and full.

To test this use the following config, which returns a too-early expiration date without this patch:

- type: http
  enabled: true
  id: wyportal
  name: WYPortal
  schedule: "@every 1m"
  ssl.verification_mode: strict
  urls: "https://portals.edu.wyoming.gov/"

You can try playing with the verification mode as well

What does this PR do?

Why is it important?

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.

Author's Checklist

[ ]

How to test this PR locally

Use the above config and run and build with the following command to prune the output. Note this requires your heartbeat yaml to not have an output section set.

mage build && ./heartbeat -e  -E output.console={} | jq '{"id": .monitor.id, "na": .tls.server.x509.not_after}'

Fixes elastic#33215 This is a bit WIP in that I'm not certain this is the final logic we want. It appears the elastic agent libs override the default golang TLS implementation's verification and only set the cert chains attribute on the connection when strict validation is enabled, where the default is `full`. See https://www.elastic.co/guide/en/beats/heartbeat/current/configuration-ssl.html for definition of the various validation options. I suspect we'd be best off making strict the default since this is what chrome does nowadays, requiring a SAN to be set, which is the only other distinction. To test this use the following config, which returns a too-early expiration date without this patch: ``` - type: http enabled: true id: wyportal name: WYPortal schedule: "@every 1m" ssl.verification_mode: strict urls: "https://portals.edu.wyoming.gov/" ``` You can try playing with the verification mode as well

elasticmachine · 2022-09-30T03:05:51Z

Pinging @elastic/uptime (Team:Uptime)

mergify · 2022-09-30T03:05:52Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @andrewvc? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2022-09-30T03:32:40Z

💚 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

Start Time: 2022-10-11T14:15:49.652+0000
Duration: 46 min 44 sec

Test stats 🧪

Test	Results
Failed	0
Passed	1869
Skipped	25
Total	1894

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

emilioalvap

On the strict by default discussion, it would seem the actual config that drives chain behaviour is InsecureSkipVerify, not verification_mode itself. Could we add this as a default for all https monitors? It would still make possible to get the correct result with full verification mode.

emilioalvap · 2022-10-05T14:49:29Z

heartbeat/monitors/active/dialchain/tlsmeta/tlsmeta.go

+
+		// If this chain expires sooner than a previously seen chain we don't
+		// set any fields
+		if chainNotAfter.Before(maxNotAfterFloor) {


maxNotAfterFloor is not updated in the loop, so all chains go through overriding each other. Is that expected? ~~Shouldn't this be initialiazed to hostCert.NotAfter if available?~~ Edit: nvm, chain expiration can pre-date peer certificate expiration

Good catch, will fix

mergify · 2022-10-10T21:47:27Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fix-cert-chains upstream/fix-cert-chains
git merge upstream/main
git push upstream fix-cert-chains

andrewvc · 2022-10-11T03:25:08Z

@emilioalvap this is now ready for review again, I spent probably a bit too much effort improving the logic and test cases in aa35bb0 , while I think it could be cleaned up further, given that this requires strict, I think it covers more than can probably be justified given the effort of digging up the right certs.

paulb-elastic · 2022-10-11T11:44:43Z

Taking the PR off the board as the parent issue is on there

emilioalvap

LGTM, reviewed test scenarios locally and it holds up. Still some linting to fix, though

…cert-chains

andrewvc · 2022-10-11T20:57:05Z

@Mergifyio backport 8.5

mergify · 2022-10-11T20:57:39Z

backport 8.5

✅ Backports have been created

#33316 [Heartbeat] Fix expiration of cert chains calculation (backport #33231) has been created for branch 8.5

* [Heartbeat] Fix expiration of cert chains calculation Fixes #33215 This is a bit WIP in that I'm not certain this is the final logic we want. It appears the elastic agent libs override the default golang TLS implementation's verification and only set the cert chains attribute on the connection when strict validation is enabled, where the default is `full`. See https://www.elastic.co/guide/en/beats/heartbeat/current/configuration-ssl.html for definition of the various validation options. I suspect we'd be best off making strict the default since this is what chrome does nowadays, requiring a SAN to be set, which is the only other distinction. To test this use the following config, which returns a too-early expiration date without this patch: ``` - type: http enabled: true id: wyportal name: WYPortal schedule: "@every 1m" ssl.verification_mode: strict urls: "https://portals.edu.wyoming.gov/" ``` You can try playing with the verification mode as well * Cleanup work * Add changelog * Fix cert calculations * Fix lint (cherry picked from commit bd081b6)

andrewvc · 2022-10-11T20:57:55Z

@Mergifyio backport 7.17

* [Heartbeat] Fix expiration of cert chains calculation Fixes #33215 This is a bit WIP in that I'm not certain this is the final logic we want. It appears the elastic agent libs override the default golang TLS implementation's verification and only set the cert chains attribute on the connection when strict validation is enabled, where the default is `full`. See https://www.elastic.co/guide/en/beats/heartbeat/current/configuration-ssl.html for definition of the various validation options. I suspect we'd be best off making strict the default since this is what chrome does nowadays, requiring a SAN to be set, which is the only other distinction. To test this use the following config, which returns a too-early expiration date without this patch: ``` - type: http enabled: true id: wyportal name: WYPortal schedule: "@every 1m" ssl.verification_mode: strict urls: "https://portals.edu.wyoming.gov/" ``` You can try playing with the verification mode as well * Cleanup work * Add changelog * Fix cert calculations * Fix lint (cherry picked from commit bd081b6) # Conflicts: # heartbeat/hbtest/hbtestutil.go # heartbeat/monitors/active/dialchain/tlsmeta/tlsmeta.go # heartbeat/monitors/active/dialchain/tlsmeta/tlsmeta_test.go # heartbeat/monitors/active/http/task.go # heartbeat/monitors/active/tcp/tcp.go

mergify · 2022-10-11T20:59:07Z

backport 7.17

✅ Backports have been created

#33317 [Heartbeat] Fix expiration of cert chains calculation (backport #33231) has been created for branch 7.17

* [Heartbeat] Fix expiration of cert chains calculation Fixes #33215 This is a bit WIP in that I'm not certain this is the final logic we want. It appears the elastic agent libs override the default golang TLS implementation's verification and only set the cert chains attribute on the connection when strict validation is enabled, where the default is `full`. See https://www.elastic.co/guide/en/beats/heartbeat/current/configuration-ssl.html for definition of the various validation options. I suspect we'd be best off making strict the default since this is what chrome does nowadays, requiring a SAN to be set, which is the only other distinction. To test this use the following config, which returns a too-early expiration date without this patch: ``` - type: http enabled: true id: wyportal name: WYPortal schedule: "@every 1m" ssl.verification_mode: strict urls: "https://portals.edu.wyoming.gov/" ``` You can try playing with the verification mode as well * Cleanup work * Add changelog * Fix cert calculations * Fix lint (cherry picked from commit bd081b6) Co-authored-by: Andrew Cholakian <[email protected]>

* [Heartbeat] Fix expiration of cert chains calculation Fixes #33215 This is a bit WIP in that I'm not certain this is the final logic we want. It appears the elastic agent libs override the default golang TLS implementation's verification and only set the cert chains attribute on the connection when strict validation is enabled, where the default is `full`. See https://www.elastic.co/guide/en/beats/heartbeat/current/configuration-ssl.html for definition of the various validation options. I suspect we'd be best off making strict the default since this is what chrome does nowadays, requiring a SAN to be set, which is the only other distinction. To test this use the following config, which returns a too-early expiration date without this patch: ``` - type: http enabled: true id: wyportal name: WYPortal schedule: "@every 1m" ssl.verification_mode: strict urls: "https://portals.edu.wyoming.gov/" ``` You can try playing with the verification mode as well * Cleanup work * Add changelog * Fix cert calculations * Fix lint

andrewvc added bug Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Sep 30, 2022

andrewvc self-assigned this Sep 30, 2022

andrewvc requested a review from a team as a code owner September 30, 2022 03:05

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 30, 2022

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 30, 2022

andrewvc requested a review from emilioalvap September 30, 2022 03:06

andrewvc marked this pull request as draft September 30, 2022 03:06

andrewvc added 2 commits October 3, 2022 17:40

Merge remote-tracking branch 'origin/main' into fix-cert-chains

687810d

Cleanup work

2b65fce

andrewvc marked this pull request as ready for review October 4, 2022 01:17

andrewvc added v8.5.0 and removed v8.5.0 labels Oct 4, 2022

Add changelog

b5c745a

emilioalvap reviewed Oct 5, 2022

View reviewed changes

andrewvc added 2 commits October 10, 2022 22:19

Fix cert calculations

aa35bb0

Merge branch 'main' into fix-cert-chains

62202fd

emilioalvap approved these changes Oct 11, 2022

View reviewed changes

andrewvc added 2 commits October 11, 2022 09:14

Fix lint

40b161b

Merge branch 'fix-cert-chains' of github.com:andrewvc/beats into fix-…

ce91997

…cert-chains

andrewvc merged commit bd081b6 into elastic:main Oct 11, 2022

andrewvc deleted the fix-cert-chains branch October 11, 2022 20:56

andrewvc added the v8.5.0 label Oct 11, 2022

mergify bot mentioned this pull request Oct 11, 2022

[Heartbeat] Fix expiration of cert chains calculation (backport #33231) #33316

Merged

mergify bot mentioned this pull request Oct 11, 2022

[Heartbeat] Fix expiration of cert chains calculation (backport #33231) #33317

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Heartbeat] Fix expiration of cert chains calculation #33231

[Heartbeat] Fix expiration of cert chains calculation #33231

andrewvc commented Sep 30, 2022 •

edited

Loading

elasticmachine commented Sep 30, 2022

mergify bot commented Sep 30, 2022

elasticmachine commented Sep 30, 2022 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

emilioalvap left a comment •

edited

Loading

emilioalvap Oct 5, 2022 •

edited

Loading

andrewvc Oct 6, 2022

mergify bot commented Oct 10, 2022

andrewvc commented Oct 11, 2022

paulb-elastic commented Oct 11, 2022

emilioalvap left a comment

andrewvc commented Oct 11, 2022

mergify bot commented Oct 11, 2022

andrewvc commented Oct 11, 2022

mergify bot commented Oct 11, 2022

[Heartbeat] Fix expiration of cert chains calculation #33231

[Heartbeat] Fix expiration of cert chains calculation #33231

Conversation

andrewvc commented Sep 30, 2022 • edited Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

elasticmachine commented Sep 30, 2022

mergify bot commented Sep 30, 2022

elasticmachine commented Sep 30, 2022 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

emilioalvap left a comment • edited Loading

Choose a reason for hiding this comment

emilioalvap Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

andrewvc Oct 6, 2022

Choose a reason for hiding this comment

mergify bot commented Oct 10, 2022

andrewvc commented Oct 11, 2022

paulb-elastic commented Oct 11, 2022

emilioalvap left a comment

Choose a reason for hiding this comment

andrewvc commented Oct 11, 2022

mergify bot commented Oct 11, 2022

✅ Backports have been created

andrewvc commented Oct 11, 2022

mergify bot commented Oct 11, 2022

✅ Backports have been created

andrewvc commented Sep 30, 2022 •

edited

Loading

elasticmachine commented Sep 30, 2022 •

edited by jenkins-beats-ci bot

Loading

emilioalvap left a comment •

edited

Loading

emilioalvap Oct 5, 2022 •

edited

Loading