From bcfd8c4836886b61ab404bd6a27a9e73c4b2440a Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Mon, 1 Jul 2024 22:03:16 -0600 Subject: [PATCH 1/9] Use GetClientCertificate to allow client cert to be reloaded Signed-off-by: Ruben Vargas --- CHANGELOG.md | 1 + .../tls/test/tls_integration_go_1_20_test.go | 1 + .../tls/test/tls_integration_go_1_21_test.go | 1 + crypto/tls/test/tls_integration_test.go | 4 +- crypto/tls/tls.go | 62 +++++++++++++------ crypto/tls/tls_test.go | 2 +- 6 files changed, 51 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b8865269..0e3531228 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 `-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] Supporting reloading client certificates #549 * [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 diff --git a/crypto/tls/test/tls_integration_go_1_20_test.go b/crypto/tls/test/tls_integration_go_1_20_test.go index 9728a0dee..7a5317cc1 100644 --- a/crypto/tls/test/tls_integration_go_1_20_test.go +++ b/crypto/tls/test/tls_integration_go_1_20_test.go @@ -4,3 +4,4 @@ package test // The error message changed in Go 1.21. const badCertificateErrorMessage = "remote error: tls: bad certificate" +const mismatchCAAndCerts = "remote error: tls: unknown certificate authority" diff --git a/crypto/tls/test/tls_integration_go_1_21_test.go b/crypto/tls/test/tls_integration_go_1_21_test.go index 4fd1aac24..d2372a223 100644 --- a/crypto/tls/test/tls_integration_go_1_21_test.go +++ b/crypto/tls/test/tls_integration_go_1_21_test.go @@ -4,3 +4,4 @@ package test // The error message changed in Go 1.21. const badCertificateErrorMessage = "remote error: tls: certificate required" +const mismatchCAAndCerts = "remote error: tls: unknown certificate authority" diff --git a/crypto/tls/test/tls_integration_test.go b/crypto/tls/test/tls_integration_test.go index 57b08d8b2..684a503c3 100644 --- a/crypto/tls/test/tls_integration_test.go +++ b/crypto/tls/test/tls_integration_test.go @@ -363,6 +363,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, @@ -411,7 +413,7 @@ func TestTLSServerWithLocalhostCertWithClientCertificateEnforcementUsingClientCA CertPath: certs.client2CertFile, KeyPath: certs.client2KeyFile, }, - httpExpectError: badCertErr, + httpExpectError: mismatchCAAndCertsErr, grpcExpectError: unavailableDescErr, }, }, diff --git a/crypto/tls/tls.go b/crypto/tls/tls.go index 7ed818f39..1c505f02a 100644 --- a/crypto/tls/tls.go +++ b/crypto/tls/tls.go @@ -83,6 +83,19 @@ func (cfg *ClientConfig) GetTLSCipherSuitesLongDescription() string { return text } +func (cfg *ClientConfig) validateCertificatePaths() (error, bool) { + if cfg.CertPath != "" || cfg.KeyPath != "" { + if cfg.CertPath == "" { + return errCertMissing, true + } + if cfg.KeyPath == "" { + return errKeyMissing, true + } + return nil, true + } + return nil, false +} + // GetTLSConfig initialises tls.Config from config options func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) { config := &tls.Config{ @@ -109,29 +122,42 @@ 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) { + // not used boolean, assumed if this is called is because we already configured TLS Client certificates. + err, _ := cfg.validateCertificatePaths() + if err == nil { + cert, err := reader.ReadSecret(cfg.CertPath) + if err != nil { + return nil, errors.Wrapf(err, "error loading client cert: %s", cfg.CertPath) + } + key, err := reader.ReadSecret(cfg.KeyPath) + if err != nil { + return nil, errors.Wrapf(err, "error loading client key: %s", cfg.KeyPath) + } + + clientCert, err := tls.X509KeyPair(cert, key) + if err != nil { + return nil, errors.Wrapf(err, "failed to load TLS certificate %s,%s", cfg.CertPath, cfg.KeyPath) + } + return &clientCert, nil } + return nil, err + } - cert, err := reader.ReadSecret(cfg.CertPath) - if err != nil { - return nil, errors.Wrapf(err, "error loading client cert: %s", cfg.CertPath) - } - key, err := reader.ReadSecret(cfg.KeyPath) - if err != nil { - return nil, errors.Wrapf(err, "error loading client key: %s", cfg.KeyPath) + err, useClientCerts := cfg.validateCertificatePaths() + if err != nil { + return nil, err + } + + if useClientCerts { + // Confirm that certificate and key paths are valid. + if _, err := loadCert(); err != nil { + return nil, err } - clientCert, err := tls.X509KeyPair(cert, key) - if err != nil { - return nil, errors.Wrapf(err, "failed to load TLS certificate %s,%s", cfg.CertPath, cfg.KeyPath) + config.GetClientCertificate = func(_ *tls.CertificateRequestInfo) (*tls.Certificate, error) { + return loadCert() } - config.Certificates = []tls.Certificate{clientCert} } if cfg.MinVersion != "" { diff --git a/crypto/tls/tls_test.go b/crypto/tls/tls_test.go index 292307ddf..1674841d6 100644 --- a/crypto/tls/tls_test.go +++ b/crypto/tls/tls_test.go @@ -109,7 +109,7 @@ 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") + assert.NotNil(t, tlsConfig.GetClientCertificate, "ensure a get certificate function is returned") // expect error with key and cert swapped passed along c = &ClientConfig{ From 3d7557cb718d93b04e738b61936d79bc3fcb74e1 Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Fri, 19 Jul 2024 10:54:39 -0600 Subject: [PATCH 2/9] Fix linting issues Signed-off-by: Ruben Vargas --- crypto/tls/tls.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/crypto/tls/tls.go b/crypto/tls/tls.go index 1c505f02a..0c949403d 100644 --- a/crypto/tls/tls.go +++ b/crypto/tls/tls.go @@ -83,17 +83,17 @@ func (cfg *ClientConfig) GetTLSCipherSuitesLongDescription() string { return text } -func (cfg *ClientConfig) validateCertificatePaths() (error, bool) { +func (cfg *ClientConfig) validateCertificatePaths() (bool, error) { if cfg.CertPath != "" || cfg.KeyPath != "" { if cfg.CertPath == "" { - return errCertMissing, true + return true, errCertMissing } if cfg.KeyPath == "" { - return errKeyMissing, true + return true, errKeyMissing } - return nil, true + return true, nil } - return nil, false + return false, nil } // GetTLSConfig initialises tls.Config from config options @@ -124,7 +124,7 @@ func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) { loadCert := func() (*tls.Certificate, error) { // not used boolean, assumed if this is called is because we already configured TLS Client certificates. - err, _ := cfg.validateCertificatePaths() + _, err := cfg.validateCertificatePaths() if err == nil { cert, err := reader.ReadSecret(cfg.CertPath) if err != nil { @@ -144,7 +144,7 @@ func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) { return nil, err } - err, useClientCerts := cfg.validateCertificatePaths() + useClientCerts, err := cfg.validateCertificatePaths() if err != nil { return nil, err } From 4ced848a42ce07d6bcac39c30ecf1229e1a6e3d3 Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Mon, 22 Jul 2024 21:13:27 -0600 Subject: [PATCH 3/9] Update crypto/tls/tls.go Co-authored-by: Arve Knudsen --- crypto/tls/tls.go | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/crypto/tls/tls.go b/crypto/tls/tls.go index 0c949403d..34f1a0bb4 100644 --- a/crypto/tls/tls.go +++ b/crypto/tls/tls.go @@ -123,25 +123,21 @@ func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) { } loadCert := func() (*tls.Certificate, error) { - // not used boolean, assumed if this is called is because we already configured TLS Client certificates. - _, err := cfg.validateCertificatePaths() - if err == nil { - cert, err := reader.ReadSecret(cfg.CertPath) - if err != nil { - return nil, errors.Wrapf(err, "error loading client cert: %s", cfg.CertPath) - } - key, err := reader.ReadSecret(cfg.KeyPath) - if err != nil { - return nil, errors.Wrapf(err, "error loading client key: %s", cfg.KeyPath) - } - - clientCert, err := tls.X509KeyPair(cert, key) - if err != nil { - return nil, errors.Wrapf(err, "failed to load TLS certificate %s,%s", cfg.CertPath, cfg.KeyPath) - } - return &clientCert, nil + cert, err := reader.ReadSecret(cfg.CertPath) + if err != nil { + return nil, errors.Wrapf(err, "error loading client cert: %s", cfg.CertPath) } - return nil, err + key, err := reader.ReadSecret(cfg.KeyPath) + if err != nil { + return nil, errors.Wrapf(err, "error loading client key: %s", cfg.KeyPath) + } + + clientCert, err := tls.X509KeyPair(cert, key) + if err != nil { + return nil, errors.Wrapf(err, "failed to load TLS certificate %s,%s", cfg.CertPath, cfg.KeyPath) + } + return &clientCert, nil + } useClientCerts, err := cfg.validateCertificatePaths() From ccb79f2459e91e07aa9ce10ed98bb48f684239ed Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Mon, 22 Jul 2024 21:13:37 -0600 Subject: [PATCH 4/9] Update crypto/tls/tls.go Co-authored-by: Arve Knudsen --- crypto/tls/tls.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crypto/tls/tls.go b/crypto/tls/tls.go index 34f1a0bb4..bfb0fe55e 100644 --- a/crypto/tls/tls.go +++ b/crypto/tls/tls.go @@ -140,12 +140,14 @@ func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) { } - useClientCerts, err := cfg.validateCertificatePaths() - if err != nil { - return nil, err - } - - if useClientCerts { + // 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 From 0275cffd9bce8202d438cdd66a5d22d91b6a9f0c Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Mon, 22 Jul 2024 21:13:43 -0600 Subject: [PATCH 5/9] Update CHANGELOG.md Co-authored-by: Arve Knudsen --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e3531228..9422d5826 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -218,7 +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 `-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] Supporting reloading client certificates #549 +* [EHNANCEMENT] Supporting 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 From 313d2e2fe7e000322edc9c3527fe8274799628aa Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Tue, 23 Jul 2024 09:27:49 -0600 Subject: [PATCH 6/9] Update crypto/tls/tls.go Co-authored-by: Arve Knudsen --- crypto/tls/tls.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/crypto/tls/tls.go b/crypto/tls/tls.go index bfb0fe55e..95b19852b 100644 --- a/crypto/tls/tls.go +++ b/crypto/tls/tls.go @@ -83,18 +83,6 @@ func (cfg *ClientConfig) GetTLSCipherSuitesLongDescription() string { return text } -func (cfg *ClientConfig) validateCertificatePaths() (bool, error) { - if cfg.CertPath != "" || cfg.KeyPath != "" { - if cfg.CertPath == "" { - return true, errCertMissing - } - if cfg.KeyPath == "" { - return true, errKeyMissing - } - return true, nil - } - return false, nil -} // GetTLSConfig initialises tls.Config from config options func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) { From f5440fb9b5924c91a5b8f909dd02c76396f7733e Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Tue, 23 Jul 2024 09:27:56 -0600 Subject: [PATCH 7/9] Update CHANGELOG.md Co-authored-by: Arve Knudsen --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9422d5826..f21aaf6fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -218,7 +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 `-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] Supporting reloading client certificates #537 +* [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 From d67878e18c72bfc164cca3b81d76decde4abf0a0 Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Tue, 23 Jul 2024 20:01:50 -0600 Subject: [PATCH 8/9] Update crypto/tls/tls_test.go Co-authored-by: Arve Knudsen --- crypto/tls/tls_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crypto/tls/tls_test.go b/crypto/tls/tls_test.go index 1674841d6..8d2a8c185 100644 --- a/crypto/tls/tls_test.go +++ b/crypto/tls/tls_test.go @@ -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.NotNil(t, tlsConfig.GetClientCertificate, "ensure a get certificate function 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{ From 3c149d86b4af315e79daad288d32d8c31bfa6bd4 Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Tue, 23 Jul 2024 20:04:47 -0600 Subject: [PATCH 9/9] Move constants to main TLS test Signed-off-by: Ruben Vargas --- crypto/tls/test/tls_integration_go_1_20_test.go | 1 - crypto/tls/test/tls_integration_go_1_21_test.go | 1 - crypto/tls/test/tls_integration_test.go | 2 ++ crypto/tls/tls.go | 1 - 4 files changed, 2 insertions(+), 3 deletions(-) diff --git a/crypto/tls/test/tls_integration_go_1_20_test.go b/crypto/tls/test/tls_integration_go_1_20_test.go index 7a5317cc1..9728a0dee 100644 --- a/crypto/tls/test/tls_integration_go_1_20_test.go +++ b/crypto/tls/test/tls_integration_go_1_20_test.go @@ -4,4 +4,3 @@ package test // The error message changed in Go 1.21. const badCertificateErrorMessage = "remote error: tls: bad certificate" -const mismatchCAAndCerts = "remote error: tls: unknown certificate authority" diff --git a/crypto/tls/test/tls_integration_go_1_21_test.go b/crypto/tls/test/tls_integration_go_1_21_test.go index d2372a223..4fd1aac24 100644 --- a/crypto/tls/test/tls_integration_go_1_21_test.go +++ b/crypto/tls/test/tls_integration_go_1_21_test.go @@ -4,4 +4,3 @@ package test // The error message changed in Go 1.21. const badCertificateErrorMessage = "remote error: tls: certificate required" -const mismatchCAAndCerts = "remote error: tls: unknown certificate authority" diff --git a/crypto/tls/test/tls_integration_test.go b/crypto/tls/test/tls_integration_test.go index 684a503c3..9a8730b1a 100644 --- a/crypto/tls/test/tls_integration_test.go +++ b/crypto/tls/test/tls_integration_test.go @@ -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 diff --git a/crypto/tls/tls.go b/crypto/tls/tls.go index 95b19852b..a5b3805b7 100644 --- a/crypto/tls/tls.go +++ b/crypto/tls/tls.go @@ -83,7 +83,6 @@ func (cfg *ClientConfig) GetTLSCipherSuitesLongDescription() string { return text } - // GetTLSConfig initialises tls.Config from config options func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) { config := &tls.Config{