Use GetClientCertificate to allow client cert to be reloaded

[rubenvp8510]

What this PR does:
Use GetClientCertificate to reload the client certificates from the disk, similar to what has been done with GetCertificate in this library https://github.com/prometheus/exporter-toolkit/blob/master/web/tls_config.go#L204 which is used here: https://github.com/grafana/dskit/blob/main/server/server.go#L317 by the server.

Which issue(s) this PR fixes:

This is useful because the certificates can rotate, and with this change we will reload the certificate from the disk without need to restart the application for it.

Fixes #549

Checklist

• Tests updated
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[zalegrala]

This looks reasonable to me. Adhere to the lint check.

Is the expectation here that the server certificate will be reloaded by some other means?

[rubenvp8510]

This looks reasonable to me. Adhere to the lint check.

Is the expectation here that the serer certificate will be reloaded by some other means?

The expectation is that at some point the certificate expired, and usually in some platforms there is a logic for cert renewal. But if the certificate is loaded only one time in memory, even if the file changed this won't take effect. Unless you reload it as this PR does. may be an additional improvement will be to not reload each time

[aknuds1]

The idea makes sense to me, but the current implementation doesn't really. I think it changes too much basically, without clear rationale.

[aknuds1]

Looks pretty good now, but validateCertificatePaths is now dead and should be removed. Plus, some other simplifications should be made.

[aknuds1]

LGTM, thanks for addressing my feedback!

[charleskorn]

This change has broken the TestSingleBinaryWithMemberlist/tls integration test in Mimir (sample logs), it is failing with:

failed to create transport: failed to start TLS TCP listener on "0.0.0.0" port 8000: tls: neither Certificates, GetCertificate, nor GetConfigForClient set in Config

Could you please take a look @rubenvp8510? You should be able to pull the branch from grafana/mimir#8833 to reproduce this.

[rubenvp8510]

Hello @charleskorn I'll take a look today, it seems like the test is expecting one of these to be defined: Certificates, GetCertificate, or GetConfigForClient . Why is not valid to expect a GetClientCertificate ? My understanding if that if you set that you don't need to set Certificates. Also this is for clients., so GetCertificate won't work on this case.

Other thing I can do is set GetConfigForClient, but wondering why this is needed as I only want to make the part of the certificate to be reloaded (which is why I'm using `GetClientCertificate)

[zalegrala]

How goes the testing? I'm seeing the above error in Tempo also after the dskit update.

[aknuds1]

I'm trying to reproduce the test error on my side.

[aknuds1]

Strange, on my side the memberlist-kv module doesn't fail for the reason Charles linked to in CI. For me it fails with a non-informative error:

16:43:20 mimir-1: ts=2024-08-01T14:43:20.847969237Z caller=mimir.go:907 level=error msg="module failed" module=memberlist-kv err="starting module memberlist-kv: invalid service state: Stopping, expected: Running"

[zalegrala]

I'm seeing this in Tempo.

level=error ts=2024-08-01T15:06:58.78706799Z caller=app.go:223 msg="module failed" module=memberlist-kv err="service memberlist_kv failed: failed to create transport: failed to start TLS TCP listener on \"::\" port 7946: tls: neither Certificates, GetCertificate, nor GetConfigForClient set in Config"

[rubenvp8510]

It seems like in order to not break anything we will need to provide a GetConfigForClient instead, not sure why GetClientCertificate is not enough I'll do the change and send the PR , I expected to have something between today and tomorrow.

[aknuds1]

@charleskorn @rubenvp8510 @zalegrala I think the problem is that memberlist/kv uses a client certificate also for server purposes, that's the reason for the error. As a fix, shall we also set the Certificates field? It shouldn't hurt, since GetClientCertificate overrides it for client purposes. I'm going to test and see if this works.

[rubenvp8510]

@aknuds1 that make sense

According to go documentation

Server configurations must set one of Certificates, GetCertificate or
GetConfigForClient. Clients doing client-authentication may set either
Certificates or GetClientCertificate.

This is confusing because the struture is named ClientConfig.

About setting certificates, What I don't know is , if I set certificates then GetClientCertificate will be called? or if this is also used for server, we can set algo GetCertificates method.

[aknuds1]

@rubenvp8510 I agree it's confusing, I don't know yet why memberlist/kv appears to use ClientConfig for a server. Maybe it shouldn't?

About setting certificates, What I don't know is , if I set certificates then GetClientCertificate will be called? or if this is also used for server, we can set algo GetCertificates method.

The GetClientCertificate documentation makes it clear that it masks Certificates for clients. However, it doesn't matter to servers.

// GetClientCertificate, if not nil, is called when a server requests a
// certificate from a client. If set, the contents of Certificates will
// be ignored.

[aknuds1]

I've tested also setting Certificates for backwards compat purposes, that makes the integration test pass. Making a PR.

[rubenvp8510]

I've tested also setting Certificates for backwards compat purposes, that makes the integration test pass. Making a PR.

Great! then we can set it with confidence :)

[aknuds1]

I've made a PR to fix the issue, please review. Tried the broken Mimir integration test with it, it passes.