Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/tls.ClientConfig: Set Config.Certificates for backwards compat #552

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

aknuds1
Copy link
Collaborator

@aknuds1 aknuds1 commented Aug 1, 2024

What this PR does:
A recent change in crypto/tls.ClientConfig.GetTLSConfig that allows for reloading of client certs, turned out to break memberlist/kv.NewTCPTransport. Apparently the latter uses ClientConfig for a server, which then fails since there's no longer any certificate config for servers.

I propose working around the problem by again setting crypto/tls.Config.Certificates, as it used to be prior to the aforementioned change. However, since we now also set GetClientCertificate, clients will prefer the latter:

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

I've tested that this change fixes the broken integration test in Mimir (TestSingleBinaryWithMemberlist/tls).

Which issue(s) this PR fixes:

Checklist

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

@aknuds1 aknuds1 force-pushed the arve/tls-client-config-backwards-compat branch from cd67538 to 4c8bc01 Compare August 1, 2024 15:42
@aknuds1 aknuds1 marked this pull request as ready for review August 1, 2024 15:43
@aknuds1 aknuds1 added the bug Something isn't working label Aug 1, 2024
@aknuds1 aknuds1 changed the title crypto/tls: Set Config.Certificates for backwards compat crypto/tls.ClientConfig: Set Config.Certificates for backwards compat Aug 1, 2024
@zalegrala
Copy link
Contributor

Looks good to me.

@aknuds1 aknuds1 merged commit 736c44c into main Aug 1, 2024
6 checks passed
@aknuds1 aknuds1 deleted the arve/tls-client-config-backwards-compat branch August 1, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants