From b69881441275dffed88d09df340118e184b79222 Mon Sep 17 00:00:00 2001 From: clyang82 Date: Mon, 13 Dec 2021 22:39:46 +0800 Subject: [PATCH 1/4] Add custom certificate CA support for s3 Signed-off-by: clyang82 --- pkg/objstore/s3/s3.go | 25 +++++++++++++---- pkg/objstore/s3/s3_test.go | 57 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/pkg/objstore/s3/s3.go b/pkg/objstore/s3/s3.go index 610ac6cf5c..2c4f210bb6 100644 --- a/pkg/objstore/s3/s3.go +++ b/pkg/objstore/s3/s3.go @@ -6,7 +6,6 @@ package s3 import ( "context" - "crypto/tls" "fmt" "io" "io/ioutil" @@ -25,6 +24,7 @@ import ( "github.com/minio/minio-go/v7/pkg/credentials" "github.com/minio/minio-go/v7/pkg/encrypt" "github.com/pkg/errors" + commoncfg "github.com/prometheus/common/config" "github.com/prometheus/common/model" "github.com/prometheus/common/version" "gopkg.in/yaml.v2" @@ -117,13 +117,24 @@ type HTTPConfig struct { // Allow upstream callers to inject a round tripper Transport http.RoundTripper `yaml:"-"` + + TLSConfig commoncfg.TLSConfig `yaml:"tls_config,omitempty"` } // DefaultTransport - this default transport is based on the Minio // DefaultTransport up until the following commit: // https://github.com/minio/minio-go/commit/008c7aa71fc17e11bf980c209a4f8c4d687fc884 // The values have since diverged. -func DefaultTransport(config Config) *http.Transport { +func DefaultTransport(config Config) (*http.Transport, error) { + tlsConfig, err := commoncfg.NewTLSConfig(&config.HTTPConfig.TLSConfig) + if err != nil { + return nil, err + } + + if config.HTTPConfig.InsecureSkipVerify { + tlsConfig.InsecureSkipVerify = true + } + return &http.Transport{ Proxy: http.ProxyFromEnvironment, DialContext: (&net.Dialer{ @@ -148,8 +159,8 @@ func DefaultTransport(config Config) *http.Transport { // // Refer: https://golang.org/src/net/http/transport.go?h=roundTrip#L1843. DisableCompression: true, - TLSClientConfig: &tls.Config{InsecureSkipVerify: config.HTTPConfig.InsecureSkipVerify}, - } + TLSClientConfig: tlsConfig, + }, nil } // Bucket implements the store.Bucket interface against s3-compatible APIs. @@ -241,7 +252,11 @@ func NewBucketWithConfig(logger log.Logger, config Config, component string) (*B if config.HTTPConfig.Transport != nil { rt = config.HTTPConfig.Transport } else { - rt = DefaultTransport(config) + var err error + rt, err = DefaultTransport(config) + if err != nil { + return nil, err + } } client, err := minio.New(config.Endpoint, &minio.Options{ diff --git a/pkg/objstore/s3/s3_test.go b/pkg/objstore/s3/s3_test.go index 78f2adca92..abcd17e071 100644 --- a/pkg/objstore/s3/s3_test.go +++ b/pkg/objstore/s3/s3_test.go @@ -163,6 +163,63 @@ http_config: } } +func TestParseConfig_CustomHTTPConfigWithTLS(t *testing.T) { + input := []byte(`bucket: abcd +insecure: false +http_config: + tls_config: + ca_file: /certs/ca.crt + cert_file: /certs/cert.crt + key_file: /certs/key.key + server_name: server + insecure_skip_verify: false + `) + cfg, err := parseConfig(input) + testutil.Ok(t, err) + + if cfg.HTTPConfig.TLSConfig.CAFile != "/certs/ca.crt" { + t.Errorf("parsing of ca_file failed: got %v, expected %v", + cfg.HTTPConfig.TLSConfig.CAFile, "/certs/ca.crt") + } + + if cfg.HTTPConfig.TLSConfig.CertFile != "/certs/cert.crt" { + t.Errorf("parsing of cert_file failed: got %v, expected %v", + cfg.HTTPConfig.TLSConfig.CertFile, "/certs/cert.crt") + } + + if cfg.HTTPConfig.TLSConfig.KeyFile != "/certs/key.key" { + t.Errorf("parsing of key_file failed: got %v, expected %v", + cfg.HTTPConfig.TLSConfig.KeyFile, "/certs/key.key") + } + + if cfg.HTTPConfig.TLSConfig.ServerName != "server" { + t.Errorf("parsing of server_name failed: got %v, expected %v", + cfg.HTTPConfig.TLSConfig.ServerName, "server") + } + + if cfg.HTTPConfig.TLSConfig.InsecureSkipVerify { + t.Errorf("parsing of insecure_skip_verify failed: got %v, expected %v", cfg.HTTPConfig.InsecureSkipVerify, false) + } +} + +func TestParseConfig_CustomLegacyInsecureSkipVerify(t *testing.T) { + input := []byte(`bucket: abcd +insecure: false +http_config: + insecure_skip_verify: true + tls_config: + insecure_skip_verify: false + `) + cfg, err := parseConfig(input) + testutil.Ok(t, err) + transport, err := DefaultTransport(cfg) + testutil.Ok(t, err) + + if !transport.TLSClientConfig.InsecureSkipVerify { + t.Errorf("parsing of insecure_skip_verify failed: got %v, expected %v", transport.TLSClientConfig.InsecureSkipVerify, true) + } +} + func TestValidate_OK(t *testing.T) { input := []byte(`bucket: "bucket-name" endpoint: "s3-endpoint" From f00ae19a5fd8b9df94583b0aaae237154561b77f Mon Sep 17 00:00:00 2001 From: clyang82 Date: Thu, 16 Dec 2021 16:10:32 +0800 Subject: [PATCH 2/4] Update CHANGELOG.md and address review comments Signed-off-by: clyang82 --- CHANGELOG.md | 1 + pkg/objstore/s3/s3_test.go | 33 ++++++--------------------------- 2 files changed, 7 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 284281b5c1..ddb675098c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re - [#4942](https://github.com/thanos-io/thanos/pull/4942) Tracing: add `traceid_128bit` support for jaeger. - [#4917](https://github.com/thanos-io/thanos/pull/4917) Query: add initial query pushdown for a subset of aggregations. Can be enabled with `--enable-feature=query-pushdown` on Thanos Query. - [#4888](https://github.com/thanos-io/thanos/pull/4888) Cache: support redis cache backend. +- [#4946](https://github.com/thanos-io/thanos/pull/4946) Store: Support tls_config configuration for the s3 minio client. ### Fixed diff --git a/pkg/objstore/s3/s3_test.go b/pkg/objstore/s3/s3_test.go index abcd17e071..2b3f879a1b 100644 --- a/pkg/objstore/s3/s3_test.go +++ b/pkg/objstore/s3/s3_test.go @@ -177,29 +177,11 @@ http_config: cfg, err := parseConfig(input) testutil.Ok(t, err) - if cfg.HTTPConfig.TLSConfig.CAFile != "/certs/ca.crt" { - t.Errorf("parsing of ca_file failed: got %v, expected %v", - cfg.HTTPConfig.TLSConfig.CAFile, "/certs/ca.crt") - } - - if cfg.HTTPConfig.TLSConfig.CertFile != "/certs/cert.crt" { - t.Errorf("parsing of cert_file failed: got %v, expected %v", - cfg.HTTPConfig.TLSConfig.CertFile, "/certs/cert.crt") - } - - if cfg.HTTPConfig.TLSConfig.KeyFile != "/certs/key.key" { - t.Errorf("parsing of key_file failed: got %v, expected %v", - cfg.HTTPConfig.TLSConfig.KeyFile, "/certs/key.key") - } - - if cfg.HTTPConfig.TLSConfig.ServerName != "server" { - t.Errorf("parsing of server_name failed: got %v, expected %v", - cfg.HTTPConfig.TLSConfig.ServerName, "server") - } - - if cfg.HTTPConfig.TLSConfig.InsecureSkipVerify { - t.Errorf("parsing of insecure_skip_verify failed: got %v, expected %v", cfg.HTTPConfig.InsecureSkipVerify, false) - } + testutil.Equals(t, "/certs/ca.crt", cfg.HTTPConfig.TLSConfig.CAFile) + testutil.Equals(t, "/certs/cert.crt", cfg.HTTPConfig.TLSConfig.CertFile) + testutil.Equals(t, "/certs/key.key", cfg.HTTPConfig.TLSConfig.KeyFile) + testutil.Equals(t, "server", cfg.HTTPConfig.TLSConfig.ServerName) + testutil.Equals(t, false, cfg.HTTPConfig.TLSConfig.InsecureSkipVerify) } func TestParseConfig_CustomLegacyInsecureSkipVerify(t *testing.T) { @@ -214,10 +196,7 @@ http_config: testutil.Ok(t, err) transport, err := DefaultTransport(cfg) testutil.Ok(t, err) - - if !transport.TLSClientConfig.InsecureSkipVerify { - t.Errorf("parsing of insecure_skip_verify failed: got %v, expected %v", transport.TLSClientConfig.InsecureSkipVerify, true) - } + testutil.Equals(t, true, transport.TLSClientConfig.InsecureSkipVerify) } func TestValidate_OK(t *testing.T) { From b2cf577a701eea14c1b3419d5778242d041572cd Mon Sep 17 00:00:00 2001 From: clyang82 Date: Thu, 16 Dec 2021 16:58:59 +0800 Subject: [PATCH 3/4] fix storage document Signed-off-by: clyang82 --- docs/storage.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/storage.md b/docs/storage.md index f40ed976b1..c9ad7f6182 100644 --- a/docs/storage.md +++ b/docs/storage.md @@ -74,6 +74,12 @@ config: idle_conn_timeout: 1m30s response_header_timeout: 2m insecure_skip_verify: false + tls_config: + ca_file: /certs/ca.crt + cert_file: /certs/cert.crt + key_file: /certs/key.key + server_name: server + insecure_skip_verify: false tls_handshake_timeout: 10s expect_continue_timeout: 1s max_idle_conns: 100 @@ -105,6 +111,8 @@ Please refer to the documentation of [the Transport type](https://golang.org/pkg Set `list_objects_version: "v1"` for S3 compatible APIs that don't support ListObjectsV2 (e.g. some versions of Ceph). Default value (`""`) is equivalent to `"v2"`. +`http_config.tls_config` allows configuring TLS connections. Please refer to the document of [tls_config](https://prometheus.io/docs/prometheus/latest/configuration/configuration/#tls_config) for detailed information on what each option does. + For debug and testing purposes you can set * `insecure: true` to switch to plain insecure HTTP instead of HTTPS From 26f0750a1fd1a096c29fa24005a0403df10004be Mon Sep 17 00:00:00 2001 From: clyang82 Date: Fri, 17 Dec 2021 14:25:31 +0800 Subject: [PATCH 4/4] define TLSConfig struct Signed-off-by: clyang82 --- docs/storage.md | 12 +++---- pkg/objstore/s3/s3.go | 83 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 86 insertions(+), 9 deletions(-) diff --git a/docs/storage.md b/docs/storage.md index c9ad7f6182..68f215996a 100644 --- a/docs/storage.md +++ b/docs/storage.md @@ -74,17 +74,17 @@ config: idle_conn_timeout: 1m30s response_header_timeout: 2m insecure_skip_verify: false - tls_config: - ca_file: /certs/ca.crt - cert_file: /certs/cert.crt - key_file: /certs/key.key - server_name: server - insecure_skip_verify: false tls_handshake_timeout: 10s expect_continue_timeout: 1s max_idle_conns: 100 max_idle_conns_per_host: 100 max_conns_per_host: 0 + tls_config: + ca_file: "" + cert_file: "" + key_file: "" + server_name: "" + insecure_skip_verify: false trace: enable: false list_objects_version: "" diff --git a/pkg/objstore/s3/s3.go b/pkg/objstore/s3/s3.go index 2c4f210bb6..50c9441292 100644 --- a/pkg/objstore/s3/s3.go +++ b/pkg/objstore/s3/s3.go @@ -6,6 +6,8 @@ package s3 import ( "context" + "crypto/tls" + "crypto/x509" "fmt" "io" "io/ioutil" @@ -24,7 +26,6 @@ import ( "github.com/minio/minio-go/v7/pkg/credentials" "github.com/minio/minio-go/v7/pkg/encrypt" "github.com/pkg/errors" - commoncfg "github.com/prometheus/common/config" "github.com/prometheus/common/model" "github.com/prometheus/common/version" "gopkg.in/yaml.v2" @@ -118,7 +119,83 @@ type HTTPConfig struct { // Allow upstream callers to inject a round tripper Transport http.RoundTripper `yaml:"-"` - TLSConfig commoncfg.TLSConfig `yaml:"tls_config,omitempty"` + TLSConfig TLSConfig `yaml:"tls_config"` +} + +// NewTLSConfig creates a new tls.Config from the given TLSConfig. +func NewTLSConfig(cfg *TLSConfig) (*tls.Config, error) { + tlsConfig := &tls.Config{InsecureSkipVerify: cfg.InsecureSkipVerify} + + // If a CA cert is provided then let's read it in. + if len(cfg.CAFile) > 0 { + b, err := readCAFile(cfg.CAFile) + if err != nil { + return nil, err + } + if !updateRootCA(tlsConfig, b) { + return nil, fmt.Errorf("unable to use specified CA cert %s", cfg.CAFile) + } + } + + if len(cfg.ServerName) > 0 { + tlsConfig.ServerName = cfg.ServerName + } + // If a client cert & key is provided then configure TLS config accordingly. + if len(cfg.CertFile) > 0 && len(cfg.KeyFile) == 0 { + return nil, fmt.Errorf("client cert file %q specified without client key file", cfg.CertFile) + } else if len(cfg.KeyFile) > 0 && len(cfg.CertFile) == 0 { + return nil, fmt.Errorf("client key file %q specified without client cert file", cfg.KeyFile) + } else if len(cfg.CertFile) > 0 && len(cfg.KeyFile) > 0 { + // Verify that client cert and key are valid. + if _, err := cfg.getClientCertificate(nil); err != nil { + return nil, err + } + tlsConfig.GetClientCertificate = cfg.getClientCertificate + } + + return tlsConfig, nil +} + +// readCAFile reads the CA cert file from disk. +func readCAFile(f string) ([]byte, error) { + data, err := ioutil.ReadFile(f) + if err != nil { + return nil, fmt.Errorf("unable to load specified CA cert %s: %s", f, err) + } + return data, nil +} + +// updateRootCA parses the given byte slice as a series of PEM encoded certificates and updates tls.Config.RootCAs. +func updateRootCA(cfg *tls.Config, b []byte) bool { + caCertPool := x509.NewCertPool() + if !caCertPool.AppendCertsFromPEM(b) { + return false + } + cfg.RootCAs = caCertPool + return true +} + +// getClientCertificate reads the pair of client cert and key from disk and returns a tls.Certificate. +func (c *TLSConfig) getClientCertificate(*tls.CertificateRequestInfo) (*tls.Certificate, error) { + cert, err := tls.LoadX509KeyPair(c.CertFile, c.KeyFile) + if err != nil { + return nil, fmt.Errorf("unable to use specified client cert (%s) & key (%s): %s", c.CertFile, c.KeyFile, err) + } + return &cert, nil +} + +// TLSConfig configures the options for TLS connections. +type TLSConfig struct { + // The CA cert to use for the targets. + CAFile string `yaml:"ca_file"` + // The client cert file for the targets. + CertFile string `yaml:"cert_file"` + // The client key file for the targets. + KeyFile string `yaml:"key_file"` + // Used to verify the hostname for the targets. + ServerName string `yaml:"server_name"` + // Disable target certificate validation. + InsecureSkipVerify bool `yaml:"insecure_skip_verify"` } // DefaultTransport - this default transport is based on the Minio @@ -126,7 +203,7 @@ type HTTPConfig struct { // https://github.com/minio/minio-go/commit/008c7aa71fc17e11bf980c209a4f8c4d687fc884 // The values have since diverged. func DefaultTransport(config Config) (*http.Transport, error) { - tlsConfig, err := commoncfg.NewTLSConfig(&config.HTTPConfig.TLSConfig) + tlsConfig, err := NewTLSConfig(&config.HTTPConfig.TLSConfig) if err != nil { return nil, err }