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

Fix client certificate requests when no client certificate is specified #1123

Merged
merged 1 commit into from Jul 13, 2023

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Jul 13, 2023

Only set GetClientCertificate if client certificate is configured.

In docs for GetClientCertificate it specifies:

GetClientCertificate must return a non-nil Certificate. If
Certificate.Certificate is empty then no certificate will be sent to the
server.

If a nil certificate is sent when the server requests a client
certificate, the client will return an error. Instead, only configure
GetClientCertificate if certificates are provided and the server may
choose to how to handle the lack of a client certificate.

This is needed primarily for when the server is using RequestClientCert,
which requests a certificate, but does not require the client to send
one.

Previously, you would see this log message:

transport: authentication handshake failed: mTLS client certificate requested, but not provided

From the client when the server requested a client cert, even if it was not required.

@chancez chancez requested a review from a team as a code owner July 13, 2023 00:19
@chancez chancez requested review from kaworu and removed request for a team July 13, 2023 00:19
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label PR is blocked until the release note is set label Jul 13, 2023
@chancez chancez self-assigned this Jul 13, 2023
@chancez chancez added the 🐛 kind/bug This is a bug in the Hubble logic. label Jul 13, 2023
@chancez chancez force-pushed the pr/chancez/main/client_certs branch from 1e08c17 to 2f0276d Compare July 13, 2023 00:20
@chancez chancez changed the title Use non-nil certificate in GetClientCertificate Fix client certificate requests when no client certificate is specified Jul 13, 2023
@chancez chancez requested review from rolinh and a team July 13, 2023 00:21
@chancez chancez added the release-note/bug This PR fixes an issue in a previous release of Hubble. label Jul 13, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label PR is blocked until the release note is set label Jul 13, 2023
cmd/common/conn/tls.go Outdated Show resolved Hide resolved
@chancez chancez marked this pull request as draft July 13, 2023 00:37
@chancez chancez force-pushed the pr/chancez/main/client_certs branch from 2f0276d to 6e85e84 Compare July 13, 2023 00:39
@chancez chancez marked this pull request as ready for review July 13, 2023 00:39
Only set GetClientCertificate if client certificate is configured.

In docs for `GetClientCertificate` it specifies:

  GetClientCertificate must return a non-nil Certificate. If
  Certificate.Certificate is empty then no certificate will be sent to the
  server.

If a nil certificate is sent when the server requests a client
certificate, the client will return an error. Instead, only configure
GetClientCertificate if certificates are provided and the server may
choose to how to handle the lack of a client certificate.

This is needed primarily for when the server is using RequestClientCert,
which requests a certificate, but does not require the client to send
one.

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez chancez force-pushed the pr/chancez/main/client_certs branch from 6e85e84 to d218c72 Compare July 13, 2023 00:40
@rolinh rolinh added the needs-backport/0.12 This PR needs backporting to the v0.12 branch label Jul 13, 2023
@chancez chancez merged commit 8409224 into master Jul 13, 2023
5 checks passed
@chancez chancez deleted the pr/chancez/main/client_certs branch July 13, 2023 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 kind/bug This is a bug in the Hubble logic. needs-backport/0.12 This PR needs backporting to the v0.12 branch release-note/bug This PR fixes an issue in a previous release of Hubble.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants