Use TLS config from dskit (#2407)

* Adjust s3 config for dskit/crypto/tls common configuration Signed-off-by: Zach Leslie <zach.leslie@grafana.com> * Update changelog Signed-off-by: Zach Leslie <zach.leslie@grafana.com> * Add nil config check Signed-off-by: Zach Leslie <zach.leslie@grafana.com> * Restore default tls min version from default minio client Signed-off-by: Zach Leslie <zach.leslie@grafana.com> * Update config manifest Signed-off-by: Zach Leslie <zach.leslie@grafana.com> * Include docs to the other supported tls configurations Signed-off-by: Zach Leslie <zach.leslie@grafana.com> * Update tls_min_version flag name to be consistent Signed-off-by: Zach Leslie <zach.leslie@grafana.com> --------- Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
grafana · May 2, 2023 · e809ae6 · e809ae6
1 parent 86759c3
commit e809ae6
Show file tree

Hide file tree

Showing 7 changed files with 69 additions and 18 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,14 @@
 * [ENHANCEMENT] Add synchronous read mode to vParquet and vParquet2 optionally enabled by env vars  [#2165](https://github.com/grafana/tempo/pull/2165) (@mdisibio)
 * [ENHANCEMENT] Add option to override metrics-generator ring port  [#2399](https://github.com/grafana/tempo/pull/2399) (@mdisibio)
 * [BUGFIX] tempodb integer divide by zero error [#2167](https://github.com/grafana/tempo/issues/2167) (@kroksys)
+* [CHANGE] **Breaking Change** Rename s3.insecure_skip_verify [#???](https://github.com/grafana/tempo/pull/???) (@zalegrala)
+```yaml
+storage:
+    trace:
+        s3:
+            insecure_skip_verify: true   // renamed to tls_insecure_skip_verify
+
+```
 
 ## v2.1.1 / 2023-04-28
 * [BUGFIX] Fix issue where Tempo sometimes flips booleans from false->true at storage time. [#2400](https://github.com/grafana/tempo/issues/2400) (@joe-elliott)

diff --git a/docs/sources/tempo/configuration/_index.md b/docs/sources/tempo/configuration/_index.md
@@ -686,8 +686,32 @@ storage:
             [insecure: <bool>]
 
             # optional.
-            # Set to true to disable verification of an TLS endpoint.  The default value is false.
-            [insecure_skip_verify: <bool>]
+            # Path to the client certificate file.
+            [tls_cert_path: <string>]
+
+            # optional.
+            # Path to the private client key file.
+            [tls_key_path: <string>]
+
+            # optional.
+            # Path to the CA certificate file.
+            [tls_ca_path: <string>]
+
+            # optional.
+            # Path to the CA certificate file.
+            [tls_server_name: <string>]
+
+            # optional.
+            # Set to true to disable verification of a TLS endpoint.  The default value is false.
+            [tls_insecure_skip_verify: <bool>]
+
+            # optional.
+            # Override the default cipher suite list, separated by commas.
+            [tls_cipher_suites: <string>]
+
+            # optional.
+            # Override the default minimum TLS version. The default value is VersionTLS12.  Allowed values: VersionTLS10, VersionTLS11, VersionTLS12, VersionTLS13
+            [tls_min_version: <string>]
 
             # optional.
             # enable to use path-style requests.

diff --git a/docs/sources/tempo/configuration/manifest.md b/docs/sources/tempo/configuration/manifest.md
@@ -537,7 +537,7 @@ storage:
             secret_key: ""
             session_token: ""
             insecure: false
-            insecure_skip_verify: false
+            tls_insecure_skip_verify: false
             part_size: 0
             hedge_requests_at: 0s
             hedge_requests_up_to: 2

diff --git a/modules/storage/config.go b/modules/storage/config.go
@@ -5,7 +5,6 @@ import (
 	"time"
 
 	"github.com/grafana/tempo/pkg/cache"
-
 	"github.com/grafana/tempo/pkg/util"
 	"github.com/grafana/tempo/tempodb"
 	"github.com/grafana/tempo/tempodb/backend"
@@ -79,6 +78,7 @@ func (cfg *Config) RegisterFlagsAndApplyDefaults(prefix string, f *flag.FlagSet)
 	f.StringVar(&cfg.Trace.S3.Prefix, util.PrefixConfig(prefix, "trace.s3.prefix"), "", "s3 root directory to store blocks in.")
 	f.StringVar(&cfg.Trace.S3.Endpoint, util.PrefixConfig(prefix, "trace.s3.endpoint"), "", "s3 endpoint to push blocks to.")
 	f.StringVar(&cfg.Trace.S3.AccessKey, util.PrefixConfig(prefix, "trace.s3.access_key"), "", "s3 access key.")
+	f.StringVar(&cfg.Trace.S3.MinVersion, util.PrefixConfig(prefix, "trace.s3.tls_min_version"), "VersionTLS12", "minimum version of TLS to use when connecting to s3.")
 	f.Var(&cfg.Trace.S3.SecretKey, util.PrefixConfig(prefix, "trace.s3.secret_key"), "s3 secret key.")
 	f.Var(&cfg.Trace.S3.SessionToken, util.PrefixConfig(prefix, "trace.s3.session_token"), "s3 session token.")
 	cfg.Trace.S3.HedgeRequestsUpTo = 2

diff --git a/tempodb/backend/s3/config.go b/tempodb/backend/s3/config.go
@@ -3,22 +3,24 @@ package s3
 import (
 	"time"
 
+	"github.com/grafana/dskit/crypto/tls"
 	"github.com/grafana/dskit/flagext"
 )
 
 type Config struct {
-	Bucket             string         `yaml:"bucket"`
-	Prefix             string         `yaml:"prefix"`
-	Endpoint           string         `yaml:"endpoint"`
-	Region             string         `yaml:"region"`
-	AccessKey          string         `yaml:"access_key"`
-	SecretKey          flagext.Secret `yaml:"secret_key"`
-	SessionToken       flagext.Secret `yaml:"session_token"`
-	Insecure           bool           `yaml:"insecure"`
-	InsecureSkipVerify bool           `yaml:"insecure_skip_verify"`
-	PartSize           uint64         `yaml:"part_size"`
-	HedgeRequestsAt    time.Duration  `yaml:"hedge_requests_at"`
-	HedgeRequestsUpTo  int            `yaml:"hedge_requests_up_to"`
+	tls.ClientConfig `yaml:",inline"`
+
+	Bucket            string         `yaml:"bucket"`
+	Prefix            string         `yaml:"prefix"`
+	Endpoint          string         `yaml:"endpoint"`
+	Region            string         `yaml:"region"`
+	AccessKey         string         `yaml:"access_key"`
+	SecretKey         flagext.Secret `yaml:"secret_key"`
+	SessionToken      flagext.Secret `yaml:"session_token"`
+	Insecure          bool           `yaml:"insecure"`
+	PartSize          uint64         `yaml:"part_size"`
+	HedgeRequestsAt   time.Duration  `yaml:"hedge_requests_at"`
+	HedgeRequestsUpTo int            `yaml:"hedge_requests_up_to"`
 	// SignatureV2 configures the object storage to use V2 signing instead of V4
 	SignatureV2      bool              `yaml:"signature_v2"`
 	ForcePathStyle   bool              `yaml:"forcepathstyle"`

diff --git a/tempodb/backend/s3/s3.go b/tempodb/backend/s3/s3.go
@@ -74,6 +74,10 @@ func New(cfg *Config) (backend.RawReader, backend.RawWriter, backend.Compactor,
 }
 
 func internalNew(cfg *Config, confirm bool) (backend.RawReader, backend.RawWriter, backend.Compactor, error) {
+	if cfg == nil {
+		return nil, nil, nil, fmt.Errorf("config is nil")
+	}
+
 	l := log.Logger
 
 	core, err := createCore(cfg, false)
@@ -372,8 +376,13 @@ func createCore(cfg *Config, hedge bool) (*minio.Core, error) {
 		return nil, errors.Wrap(err, "create minio.DefaultTransport")
 	}
 
-	if cfg.InsecureSkipVerify {
-		customTransport.TLSClientConfig.InsecureSkipVerify = true
+	tlsConfig, err := cfg.GetTLSConfig()
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to create TLS config")
+	}
+
+	if tlsConfig != nil {
+		customTransport.TLSClientConfig = tlsConfig
 	}
 
 	// add instrumentation

diff --git a/tempodb/backend/s3/s3_test.go b/tempodb/backend/s3/s3_test.go
@@ -96,6 +96,14 @@ func TestHedge(t *testing.T) {
 	}
 }
 
+func TestNilConfig(t *testing.T) {
+	_, _, _, err := New(nil)
+	require.Error(t, err)
+
+	_, _, _, err = NewNoConfirm(nil)
+	require.Error(t, err)
+}
+
 func fakeServer(t *testing.T, returnIn time.Duration, counter *int32) *httptest.Server {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		time.Sleep(returnIn)