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

Use GetClientCertificate to allow client cert to be reloaded #537

Merged
merged 9 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@
* [ENHANCEMENT] memberlist: use separate queue for broadcast messages that are result of local updates, and prioritize locally-generated messages when sending broadcasts. On stopping, only wait for queue with locally-generated messages to be empty. #539
* [ENHANCEMENT] memberlist: Added `-<prefix>memberlist.broadcast-timeout-for-local-updates-on-shutdown` option to set timeout for sending locally-generated updates on shutdown, instead of previously hardcoded 10s (which is still the default). #539
* [ENHANCEMENT] tracing: add ExtractTraceSpanID function.
* [EHNANCEMENT] crypto/tls: Support reloading client certificates #537
* [CHANGE] Backoff: added `Backoff.ErrCause()` which is like `Backoff.Err()` but returns the context cause if backoff is terminated because the context has been canceled. #538
* [BUGFIX] spanlogger: Support multiple tenant IDs. #59
* [BUGFIX] Memberlist: fixed corrupted packets when sending compound messages with more than 255 messages or messages bigger than 64KB. #85
Expand Down
6 changes: 5 additions & 1 deletion crypto/tls/test/tls_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import (
"github.com/grafana/dskit/crypto/tls"
)

const mismatchCAAndCerts = "remote error: tls: unknown certificate authority"

type tcIntegrationClientServer struct {
name string
tlsGrpcEnabled bool
Expand Down Expand Up @@ -363,6 +365,8 @@ func TestTLSServerWithLocalhostCertWithClientCertificateEnforcementUsingClientCA
// bad certificate from the server side and just see connection
// closed/reset instead
badCertErr := errorContainsString(badCertificateErrorMessage)
mismatchCAAndCertsErr := errorContainsString(mismatchCAAndCerts)

newIntegrationClientServer(
t,
cfg,
Expand Down Expand Up @@ -411,7 +415,7 @@ func TestTLSServerWithLocalhostCertWithClientCertificateEnforcementUsingClientCA
CertPath: certs.client2CertFile,
KeyPath: certs.client2KeyFile,
},
httpExpectError: badCertErr,
httpExpectError: mismatchCAAndCertsErr,
grpcExpectError: unavailableDescErr,
},
},
Expand Down
31 changes: 21 additions & 10 deletions crypto/tls/tls.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,15 +109,7 @@ func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) {
config.RootCAs = caCertPool
}

// Read Client Certificate
if cfg.CertPath != "" || cfg.KeyPath != "" {
if cfg.CertPath == "" {
return nil, errCertMissing
}
if cfg.KeyPath == "" {
return nil, errKeyMissing
}

loadCert := func() (*tls.Certificate, error) {
cert, err := reader.ReadSecret(cfg.CertPath)
if err != nil {
return nil, errors.Wrapf(err, "error loading client cert: %s", cfg.CertPath)
Expand All @@ -131,7 +123,26 @@ func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) {
if err != nil {
return nil, errors.Wrapf(err, "failed to load TLS certificate %s,%s", cfg.CertPath, cfg.KeyPath)
}
config.Certificates = []tls.Certificate{clientCert}
return &clientCert, nil

}

// Read Client Certificate
if cfg.CertPath != "" || cfg.KeyPath != "" {
if cfg.CertPath == "" {
return nil, errCertMissing
}
if cfg.KeyPath == "" {
return nil, errKeyMissing
}
// Confirm that certificate and key paths are valid.
if _, err := loadCert(); err != nil {
return nil, err
}

config.GetClientCertificate = func(_ *tls.CertificateRequestInfo) (*tls.Certificate, error) {
return loadCert()
}
}

if cfg.MinVersion != "" {
Expand Down
5 changes: 4 additions & 1 deletion crypto/tls/tls_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,10 @@ func TestGetTLSConfig_ClientCerts(t *testing.T) {
tlsConfig, err := c.GetTLSConfig()
assert.NoError(t, err)
assert.Equal(t, false, tlsConfig.InsecureSkipVerify, "make sure we default to not skip verification")
assert.Equal(t, 1, len(tlsConfig.Certificates), "ensure a certificate is returned")
require.NotNil(t, tlsConfig.GetClientCertificate, "ensure GetClientCertificate is set")
cert, err := tlsConfig.GetClientCertificate(nil)
require.NoError(t, err)
assert.NotNil(t, cert, "ensure GetClientCertificate returns a certificate")

// expect error with key and cert swapped passed along
c = &ClientConfig{
Expand Down