Repository: grafana/dskit (go)

Issue:
Supporting reloading client certificates
Currently I see some logic with the use of GetCertificates that allows to reload the certificate when it rotates. But there is no option for the client certificates.


Pull request description:
Use GetClientCertificate to allow client cert to be reloaded
<!--  Thanks for sending a pull request!  Before submitting:

1. Read our CONTRIBUTING.md guide
2. Rebase your PR if it gets out of sync with main
-->

**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**
- [x] Tests updated
- [x] `CHANGELOG.md` updated - the order of entries should be `[CHANGE]`, `[FEATURE]`, `[ENHANCEMENT]`, `[BUGFIX]`


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5b88652..f21aaf6 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 `-<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
diff --git a/cache/memcached_server_selector.go b/cache/memcached_server_selector.go
index 60c9afe..2ef5316 100644
--- a/cache/memcached_server_selector.go
+++ b/cache/memcached_server_selector.go
@@ -7,7 +7,7 @@ import (
 	"net"
 	"sync"
 
-	"github.com/cespare/xxhash"
+	"github.com/cespare/xxhash/v2"
 	"github.com/facette/natsort"
 	"github.com/grafana/gomemcache/memcache"
 )
diff --git a/cache/redis_client.go b/cache/redis_client.go
index 1f0bb25..cd9efb4 100644
--- a/cache/redis_client.go
+++ b/cache/redis_client.go
@@ -311,12 +311,7 @@ func (c *RedisClient) Name() string {
 	return c.name
 }
 
-// stringToBytes converts string to byte slice (copied from vendor/github.com/go-redis/redis/v8/internal/util/unsafe.go).
+// stringToBytes converts string to byte slice.
 func stringToBytes(s string) []byte {
-	return *(*[]byte)(unsafe.Pointer(
-		&struct {
-			string
-			Cap int
-		}{s, len(s)},
-	))
+	return unsafe.Slice(unsafe.StringData(s), len(s))
 }
diff --git a/crypto/tls/tls.go b/crypto/tls/tls.go
index 7ed818f..a5b3805 100644
--- a/crypto/tls/tls.go
+++ b/crypto/tls/tls.go
@@ -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)
@@ -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 != "" {
diff --git a/go.mod b/go.mod
index 1b84ceb..c18d118 100644
--- a/go.mod
+++ b/go.mod
@@ -7,7 +7,7 @@ require (
 	github.com/alicebob/miniredis v2.5.0+incompatible
 	github.com/armon/go-metrics v0.3.10
 	github.com/aws/aws-sdk-go v1.44.321
-	github.com/cespare/xxhash v1.1.0
+	github.com/cespare/xxhash/v2 v2.2.0
 	github.com/cristalhq/hedgedhttp v0.9.1
 	github.com/davecgh/go-spew v1.1.1
 	github.com/facette/natsort v0.0.0-20181210072756-2cd4dd1e2dcb
@@ -57,10 +57,8 @@ require (
 )
 
 require (
-	github.com/OneOfOne/xxhash v1.2.6 // indirect
 	github.com/alicebob/gopher-json v0.0.0-20200520072559-a9ecdc9d1d3a // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
-	github.com/cespare/xxhash/v2 v2.2.0 // indirect
 	github.com/codahale/hdrhistogram v0.0.0-20161010025455-3a0bb77429bd // indirect
 	github.com/coreos/go-semver v0.3.0 // indirect
 	github.com/coreos/go-systemd/v22 v22.5.0 // indirect
@@ -89,7 +87,6 @@ require (
 	github.com/onsi/gomega v1.24.0 // indirect
 	github.com/prometheus/procfs v0.9.0 // indirect
 	github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529 // indirect
-	github.com/spaolacci/murmur3 v1.1.0 // indirect
 	github.com/stretchr/objx v0.5.0 // indirect
 	github.com/yuin/gopher-lua v0.0.0-20210529063254-f4c35e4016d9 // indirect
 	go.uber.org/multierr v1.6.0 // indirect
diff --git a/go.sum b/go.sum
index f068598..a05b5ef 100644
--- a/go.sum
+++ b/go.sum
@@ -2,9 +2,6 @@ cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMT
 cloud.google.com/go v0.34.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
 github.com/DataDog/datadog-go v3.2.0+incompatible/go.mod h1:LButxg5PwREeZtORoXG3tL4fMGNddJ+vMq1mwgfaqoQ=
-github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU=
-github.com/OneOfOne/xxhash v1.2.6 h1:U68crOE3y3MPttCMQGywZOLrTeF5HHJ3/vDBCJn9/bA=
-github.com/OneOfOne/xxhash v1.2.6/go.mod h1:eZbhyaAYD41SGSSsnmcpxVoRiQ/MPUTjUdIIOT9Um7Q=
 github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
 github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
 github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
@@ -31,8 +28,6 @@ github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
 github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
 github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kBD4zp0CCIs=
 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
-github.com/cespare/xxhash v1.1.0 h1:a6HrQnmkObjyL+Gs60czilIUGqrzKutQD6XZog3p+ko=
-github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc=
 github.com/cespare/xxhash/v2 v2.1.1/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
 github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44=
 github.com/cespare/xxhash/v2 v2.2.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
@@ -321,9 +316,6 @@ github.com/sercand/kuberesolver/v5 v5.1.1/go.mod h1:Fs1KbKhVRnB2aDWN12NjKCB+RgYM
 github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo=
 github.com/sirupsen/logrus v1.4.2/go.mod h1:tLMulIdttU9McNUspp0xgXVQah82FyeX6MwdIuYE2rE=
 github.com/sirupsen/logrus v1.6.0/go.mod h1:7uNnSEd1DgxDLC74fIahvMZmmYsHGZGEOFrfsX/uA88=
-github.com/spaolacci/murmur3 v0.0.0-20180118202830-f09979ecbc72/go.mod h1:JwIasOWyU6f++ZhiEuf87xNszmSA2myDM2Kzu9HwQUA=
-github.com/spaolacci/murmur3 v1.1.0 h1:7c1g84S4BPRrfL5Xrdp6fOJ206sU9y293DDHaoy0bLI=
-github.com/spaolacci/murmur3 v1.1.0/go.mod h1:JwIasOWyU6f++ZhiEuf87xNszmSA2myDM2Kzu9HwQUA=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
diff --git a/ring/shard/shard.go b/ring/shard/shard.go
index 1d70eb6..26d6955 100644
--- a/ring/shard/shard.go
+++ b/ring/shard/shard.go
@@ -41,5 +41,5 @@ func ShuffleShardExpectedInstances(shardSize, numZones int) int {
 
 // yoloBuf will return an unsafe pointer to a string, as the name yolo.yoloBuf implies use at your own risk.
 func yoloBuf(s string) []byte {
-	return *((*[]byte)(unsafe.Pointer(&s)))
+	return unsafe.Slice(unsafe.StringData(s), len(s))
 }


Test patch:
diff --git a/crypto/tls/test/tls_integration_test.go b/crypto/tls/test/tls_integration_test.go
index 57b08d8..9a8730b 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
@@ -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,
@@ -411,7 +415,7 @@ func TestTLSServerWithLocalhostCertWithClientCertificateEnforcementUsingClientCA
 					CertPath: certs.client2CertFile,
 					KeyPath:  certs.client2KeyFile,
 				},
-				httpExpectError: badCertErr,
+				httpExpectError: mismatchCAAndCertsErr,
 				grpcExpectError: unavailableDescErr,
 			},
 		},
diff --git a/crypto/tls/tls_test.go b/crypto/tls/tls_test.go
index 292307d..8d2a8c1 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.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{