Check if TLS certificate and key file have been modified #345

LeviHarrison · 2021-11-24T16:30:35Z

This PR adds the capability to check the hashes of the client certificate and key files to see if they've been modified, as is already done with the CA file.

Fixes prometheus/prometheus#9512

roidelapluie · 2021-11-25T21:34:30Z

I think we should only store the cert, you can not change the key without changing the cert.

LeviHarrison · 2021-11-26T17:45:17Z

The issue with that would be if someone were to change only the key, yes, the TLS configuration would be invalid, but instead of getting (what I think would be) a private key does not match public key error, they would continue to get whatever error they had before or nothing, because the configuration wouldn't be updated.

config/http_config.go

roidelapluie · 2021-12-01T23:45:29Z

config/http_config.go

+	}
+	h1 := sha256.Sum256(b1)
+
+	b2, b3, err := readCertAndKey(t.certFile, t.keyFile)


Where do we deal with unset certFile and t.keyFile?

Could you please elaborate on what you mean by unset?

I mean empty strings

Good point. A round tripper just isn't created if the CAFile is unset, but we can't do the same with the certFile and keyFile.

common/config/http_config.go

Lines 452 to 455 in 102c12a

if len(cfg.TLSConfig.CAFile) == 0 {

// No need for a RoundTripper that reloads the CA file automatically.

return newRT(tlsConfig)

}

Should be fixed.

fpetkovski · 2022-03-01T08:02:17Z

We are also hitting this problem, anything we can help with to move the PR forward?

LeviHarrison · 2022-03-01T14:03:06Z

Apologies, I just need to find time to work on this.

simonpasquier · 2022-04-12T08:36:42Z

@LeviHarrison do you want me to pick up the work? This bug is becoming a significant issue for us right now.

roidelapluie · 2022-04-12T08:58:44Z

@LeviHarrison do you want me to pick up the work? This bug is becoming a significant issue for us right now.

Yes, I think you can review&rebase

simonpasquier · 2022-07-08T12:57:48Z

Rebased on top of main, I didn't change anything to the PR except for resolving the conflicts. Thanks a lot @LeviHarrison!

Signed-off-by: Levi Harrison <git@leviharrison.dev> Signed-off-by: Simon Pasquier <spasquie@redhat.com>

This reverts commit c63387b. Signed-off-by: Levi Harrison <git@leviharrison.dev> Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Signed-off-by: Levi Harrison <git@leviharrison.dev> Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier · 2022-07-21T10:39:27Z

@roidelapluie PTAL

simonpasquier · 2022-10-17T10:50:30Z

@roidelapluie friendly ping :)

When the TLS certificate (used by Prometheus to authenticate to the scraped targets) gets rotated, Prometheus doesn't pick up the new certificate until the connection to the target is re-established. Because Prometheus uses keep-alive HTTP connections, the consequence is that the scrapes start failing after about 1 day and the TargetDown alert fires. There's an upstream pull request [1] to address the issue but it isn't merged yet. This commit pulls the changes from [1] into our downstream fork by adding a replace directive to go.mod for the github.com/prometheus/common. The replacement code is under patches/github.com/prometheus/common which is the same version as upstream (v0.37.0) + the upstream PR applied on top of it. As soon as upstream Prometheus depends on a version of github.com/prometheus/common that fixes the issue, the replace directive in go.mod and the code under the patches/ directory can be removed. [1] prometheus/common#345 Signed-off-by: Simon Pasquier <spasquie@redhat.com>

roidelapluie · 2022-11-03T11:38:31Z

Thanks!

When the TLS certificate (used by Prometheus to authenticate to the scraped targets) gets rotated, Prometheus doesn't pick up the new certificate until the connection to the target is re-established. Because Prometheus uses keep-alive HTTP connections, the consequence is that the scrapes start failing after about 1 day and the TargetDown alert fires. There's an upstream pull request [1] to address the issue but it isn't merged yet. This commit pulls the changes from [1] into our downstream fork by adding a replace directive to go.mod for the github.com/prometheus/common. The replacement code is under patches/github.com/prometheus/common which is the same version as upstream (v0.37.0) + the upstream PR applied on top of it. As soon as upstream Prometheus depends on a version of github.com/prometheus/common that fixes the issue, the replace directive in go.mod and the code under the patches/ directory can be removed. [1] prometheus/common#345 Signed-off-by: Simon Pasquier <spasquie@redhat.com>

- Check if TLS certificate and key file have been modified prometheus/common#345 - Add the ability to specify the maximum acceptable TLS version prometheus/common#414 Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>

) * Check hash of cert and key file Signed-off-by: Levi Harrison <git@leviharrison.dev> Signed-off-by: Simon Pasquier <spasquie@redhat.com>

LeviHarrison requested a review from roidelapluie November 24, 2021 16:30

LeviHarrison force-pushed the tls-roundtripper-check-cert-key branch from 4419fa0 to 546ae0d Compare November 24, 2021 16:32

roidelapluie reviewed Nov 30, 2021

View reviewed changes

config/http_config.go Outdated Show resolved Hide resolved

LeviHarrison force-pushed the tls-roundtripper-check-cert-key branch from de05404 to 102c12a Compare November 30, 2021 01:14

LeviHarrison requested a review from roidelapluie November 30, 2021 01:14

roidelapluie reviewed Dec 1, 2021

View reviewed changes

LeviHarrison requested a review from roidelapluie March 13, 2022 17:06

simonpasquier force-pushed the tls-roundtripper-check-cert-key branch from e06f022 to 9294f56 Compare July 8, 2022 12:39

LeviHarrison and others added 8 commits July 13, 2022 18:01

Check hash of cert and key file

bbca57c

Signed-off-by: Levi Harrison <git@leviharrison.dev> Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Added tests

f87a29c

Signed-off-by: Levi Harrison <git@leviharrison.dev> Signed-off-by: Simon Pasquier <spasquie@redhat.com>

os.ReadFile -> ioutil.ReadFile (for older versions)

a91bc27

Signed-off-by: Levi Harrison <git@leviharrison.dev> Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Add error messages in tests

2ab8d3c

Signed-off-by: Levi Harrison <git@leviharrison.dev> Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Only create new client if ca file changed

fda87d4

Signed-off-by: Levi Harrison <git@leviharrison.dev> Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Revert "Only create new client if ca file changed"

a1f5419

This reverts commit c63387b. Signed-off-by: Levi Harrison <git@leviharrison.dev> Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Handle unset cert and key

84e7ee9

Signed-off-by: Levi Harrison <git@leviharrison.dev> Signed-off-by: Simon Pasquier <spasquie@redhat.com>

config: fix imports

d3fa312

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier force-pushed the tls-roundtripper-check-cert-key branch from 9294f56 to d3fa312 Compare July 13, 2022 16:02

simonpasquier mentioned this pull request Oct 26, 2022

OCPBUGS-2873: fix certificate reloads after rotation openshift/prometheus#145

Merged

roidelapluie merged commit 1c0fa3e into prometheus:main Nov 3, 2022

lucacome mentioned this pull request Nov 11, 2022

Remove deprecated ioutil #410

Closed

roidelapluie mentioned this pull request Dec 8, 2022

Update Prometheus/common prometheus/prometheus#11685

Merged

lpetrazickisupgrade mentioned this pull request Mar 6, 2024

Prometheus failing to reload Probe TLS cert and key from disk #598

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if TLS certificate and key file have been modified #345

Check if TLS certificate and key file have been modified #345

LeviHarrison commented Nov 24, 2021

roidelapluie commented Nov 25, 2021

LeviHarrison commented Nov 26, 2021 •

edited

roidelapluie Dec 1, 2021

LeviHarrison Dec 26, 2021

roidelapluie Jan 29, 2022

LeviHarrison Feb 3, 2022

LeviHarrison Mar 13, 2022

fpetkovski commented Mar 1, 2022 •

edited

LeviHarrison commented Mar 1, 2022

simonpasquier commented Apr 12, 2022

roidelapluie commented Apr 12, 2022

simonpasquier commented Jul 8, 2022

simonpasquier commented Jul 21, 2022

simonpasquier commented Oct 17, 2022

roidelapluie commented Nov 3, 2022

	if len(cfg.TLSConfig.CAFile) == 0 {
	// No need for a RoundTripper that reloads the CA file automatically.
	return newRT(tlsConfig)
	}

Check if TLS certificate and key file have been modified #345

Check if TLS certificate and key file have been modified #345

Conversation

LeviHarrison commented Nov 24, 2021

roidelapluie commented Nov 25, 2021

LeviHarrison commented Nov 26, 2021 • edited

roidelapluie Dec 1, 2021

Choose a reason for hiding this comment

LeviHarrison Dec 26, 2021

Choose a reason for hiding this comment

roidelapluie Jan 29, 2022

Choose a reason for hiding this comment

LeviHarrison Feb 3, 2022

Choose a reason for hiding this comment

LeviHarrison Mar 13, 2022

Choose a reason for hiding this comment

fpetkovski commented Mar 1, 2022 • edited

LeviHarrison commented Mar 1, 2022

simonpasquier commented Apr 12, 2022

roidelapluie commented Apr 12, 2022

simonpasquier commented Jul 8, 2022

simonpasquier commented Jul 21, 2022

simonpasquier commented Oct 17, 2022

roidelapluie commented Nov 3, 2022

LeviHarrison commented Nov 26, 2021 •

edited

fpetkovski commented Mar 1, 2022 •

edited