From abb7b1423e45f97ccf1dafc31ad6d0cd25c5618a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Han?= Date: Mon, 27 Sep 2021 16:03:18 +0200 Subject: [PATCH] rgw: use insecure TLS for bucket health check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have seen cases where the signed certificate used for the RGW does not contain the internal DNS endpoint, resulting in the health check to fail since the certificate is not valid for this domain. People consuming the gateways by external clients and for specific domains do not necessarily have the internal DNS configured in the certificate. So let's be a bit more flexible and simply ensure a connectivity check and bypass the certificate validation. Also, this is fixing the tls code in newS3Agent and adds unit tests. Closes: #8663 Signed-off-by: Sébastien Han --- .../ceph/object/bucket/provisioner.go | 3 +- pkg/operator/ceph/object/health.go | 3 +- pkg/operator/ceph/object/rgw.go | 3 +- pkg/operator/ceph/object/s3-handlers.go | 26 +++--- pkg/operator/ceph/object/s3-handlers_test.go | 80 +++++++++++++++++++ tests/integration/ceph_base_object_test.go | 5 ++ tests/integration/ceph_object_test.go | 40 +++++++++- 7 files changed, 140 insertions(+), 20 deletions(-) create mode 100644 pkg/operator/ceph/object/s3-handlers_test.go diff --git a/pkg/operator/ceph/object/bucket/provisioner.go b/pkg/operator/ceph/object/bucket/provisioner.go index 7743f3218bc0f..a1d773c9eef3c 100644 --- a/pkg/operator/ceph/object/bucket/provisioner.go +++ b/pkg/operator/ceph/object/bucket/provisioner.go @@ -621,7 +621,8 @@ func (p *Provisioner) setAdminOpsAPIClient() error { Timeout: cephObject.HttpTimeOut, } if p.tlsCert != nil { - httpClient.Transport = cephObject.BuildTransportTLS(p.tlsCert) + insecure := false + httpClient.Transport = cephObject.BuildTransportTLS(p.tlsCert, insecure) } // Fetch the ceph object store diff --git a/pkg/operator/ceph/object/health.go b/pkg/operator/ceph/object/health.go index c2963902ae2ed..996686d24c855 100644 --- a/pkg/operator/ceph/object/health.go +++ b/pkg/operator/ceph/object/health.go @@ -159,14 +159,13 @@ func (c *bucketChecker) checkObjectStoreHealth() error { } // Set access and secret key - tlsCert := c.objContext.TlsCert s3endpoint := c.objContext.Endpoint s3AccessKey := user.Keys[0].AccessKey s3SecretKey := user.Keys[0].SecretKey // Initiate s3 agent logger.Debugf("initializing s3 connection for object store %q", c.namespacedName.Name) - s3client, err := NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, "", false, tlsCert) + s3client, err := NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, "", false) if err != nil { return errors.Wrap(err, "failed to initialize s3 connection") } diff --git a/pkg/operator/ceph/object/rgw.go b/pkg/operator/ceph/object/rgw.go index 3c9c0248823e9..7e89c290f7d92 100644 --- a/pkg/operator/ceph/object/rgw.go +++ b/pkg/operator/ceph/object/rgw.go @@ -361,7 +361,8 @@ func genObjectStoreHTTPClient(objContext *Context, spec *cephv1.ObjectStoreSpec) if err != nil { return nil, tlsCert, errors.Wrapf(err, "failed to fetch CA cert to establish TLS connection with object store %q", nsName) } - c.Transport = BuildTransportTLS(tlsCert) + insecure := false + c.Transport = BuildTransportTLS(tlsCert, insecure) } return c, tlsCert, nil } diff --git a/pkg/operator/ceph/object/s3-handlers.go b/pkg/operator/ceph/object/s3-handlers.go index 74b8b76c1ae95..a76d40879ee7e 100644 --- a/pkg/operator/ceph/object/s3-handlers.go +++ b/pkg/operator/ceph/object/s3-handlers.go @@ -1,5 +1,5 @@ /* -Copyright 2018 The Kubernetes Authors. +Copyright 2018 The Rook Authors. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -40,7 +40,7 @@ func NewS3Agent(accessKey, secretKey, endpoint, region string, debug bool, tlsCe return newS3Agent(accessKey, secretKey, endpoint, region, debug, tlsCert, false) } -func NewTestOnlyS3Agent(accessKey, secretKey, endpoint, region string, debug bool) (*S3Agent, error) { +func NewInsecureS3Agent(accessKey, secretKey, endpoint, region string, debug bool) (*S3Agent, error) { return newS3Agent(accessKey, secretKey, endpoint, region, debug, nil, true) } @@ -60,14 +60,7 @@ func newS3Agent(accessKey, secretKey, endpoint, region string, debug bool, tlsCe tlsEnabled := false if len(tlsCert) > 0 || insecure { tlsEnabled = true - if len(tlsCert) > 0 { - client.Transport = BuildTransportTLS(tlsCert) - } else if insecure { - client.Transport = &http.Transport{ - // #nosec G402 is enabled only for testing - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, - } - } + client.Transport = BuildTransportTLS(tlsCert, insecure) } sess, err := session.NewSession( aws.NewConfig(). @@ -205,11 +198,16 @@ func (s *S3Agent) DeleteObjectInBucket(bucketname string, key string) (bool, err return true, nil } -func BuildTransportTLS(tlsCert []byte) *http.Transport { - caCertPool := x509.NewCertPool() - caCertPool.AppendCertsFromPEM(tlsCert) +func BuildTransportTLS(tlsCert []byte, insecure bool) *http.Transport { + // #nosec G402 is enabled only for testing + tlsConfig := &tls.Config{MinVersion: tls.VersionTLS12, InsecureSkipVerify: insecure} + if len(tlsCert) > 0 { + caCertPool := x509.NewCertPool() + caCertPool.AppendCertsFromPEM(tlsCert) + tlsConfig.RootCAs = caCertPool + } return &http.Transport{ - TLSClientConfig: &tls.Config{RootCAs: caCertPool, MinVersion: tls.VersionTLS12}, + TLSClientConfig: tlsConfig, } } diff --git a/pkg/operator/ceph/object/s3-handlers_test.go b/pkg/operator/ceph/object/s3-handlers_test.go new file mode 100644 index 0000000000000..0417d7eb2549e --- /dev/null +++ b/pkg/operator/ceph/object/s3-handlers_test.go @@ -0,0 +1,80 @@ +/* +Copyright 2021 The Rook Authors. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package object + +import ( + "net/http" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/stretchr/testify/assert" +) + +func TestNewS3Agent(t *testing.T) { + accessKey := "accessKey" + secretKey := "secretKey" + endpoint := "endpoint" + region := "region" + + t.Run("test without tls/debug", func(t *testing.T) { + debug := false + insecure := false + s3Agent, err := newS3Agent(accessKey, secretKey, endpoint, region, debug, nil, insecure) + assert.NoError(t, err) + assert.NotEqual(t, aws.LogDebug, s3Agent.Client.Config.LogLevel) + assert.Equal(t, nil, s3Agent.Client.Config.HTTPClient.Transport) + assert.True(t, *s3Agent.Client.Config.DisableSSL) + }) + t.Run("test with debug without tls", func(t *testing.T) { + debug := true + logLevel := aws.LogDebug + insecure := false + s3Agent, err := newS3Agent(accessKey, secretKey, endpoint, region, debug, nil, insecure) + assert.NoError(t, err) + assert.Equal(t, &logLevel, s3Agent.Client.Config.LogLevel) + assert.Nil(t, s3Agent.Client.Config.HTTPClient.Transport) + assert.True(t, *s3Agent.Client.Config.DisableSSL) + }) + t.Run("test without tls client cert but insecure tls", func(t *testing.T) { + debug := true + insecure := true + s3Agent, err := newS3Agent(accessKey, secretKey, endpoint, region, debug, nil, insecure) + assert.NoError(t, err) + assert.Nil(t, s3Agent.Client.Config.HTTPClient.Transport.(*http.Transport).TLSClientConfig.RootCAs) + assert.True(t, s3Agent.Client.Config.HTTPClient.Transport.(*http.Transport).TLSClientConfig.InsecureSkipVerify) + assert.False(t, *s3Agent.Client.Config.DisableSSL) + }) + t.Run("test with secure tls client cert", func(t *testing.T) { + debug := true + insecure := false + tlsCert := []byte("tlsCert") + s3Agent, err := newS3Agent(accessKey, secretKey, endpoint, region, debug, tlsCert, insecure) + assert.NoError(t, err) + assert.NotNil(t, s3Agent.Client.Config.HTTPClient.Transport.(*http.Transport).TLSClientConfig.RootCAs) + assert.False(t, *s3Agent.Client.Config.DisableSSL) + }) + t.Run("test with insesure tls client cert", func(t *testing.T) { + debug := true + insecure := true + tlsCert := []byte("tlsCert") + s3Agent, err := newS3Agent(accessKey, secretKey, endpoint, region, debug, tlsCert, insecure) + assert.NoError(t, err) + assert.NotNil(t, s3Agent.Client.Config.HTTPClient.Transport) + assert.True(t, s3Agent.Client.Config.HTTPClient.Transport.(*http.Transport).TLSClientConfig.InsecureSkipVerify) + assert.False(t, *s3Agent.Client.Config.DisableSSL) + }) +} diff --git a/tests/integration/ceph_base_object_test.go b/tests/integration/ceph_base_object_test.go index 61fb518ec152e..4ae3a46870140 100644 --- a/tests/integration/ceph_base_object_test.go +++ b/tests/integration/ceph_base_object_test.go @@ -38,6 +38,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +const ( + // #nosec G101 since this is not leaking any hardcoded credentials, it's just the secret name + objectTLSSecretName = "rook-ceph-rgw-tls-test-store-csr" +) + var ( userid = "rook-user" userdisplayname = "A rook RGW user" diff --git a/tests/integration/ceph_object_test.go b/tests/integration/ceph_object_test.go index ef1eff90a9507..510da505bc173 100644 --- a/tests/integration/ceph_object_test.go +++ b/tests/integration/ceph_object_test.go @@ -31,9 +31,19 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +const ( + objectStoreServicePrefixUniq = "rook-ceph-rgw-" + objectStoreTLSName = "tls-test-store" +) + +var ( + objectStoreServicePrefix = "rook-ceph-rgw-" +) + func TestCephObjectSuite(t *testing.T) { if installer.SkipTestSuite(installer.CephTestSuite) { t.Skip() @@ -92,7 +102,32 @@ func (s *ObjectSuite) TestWithTLS() { } tls := true + objectStoreServicePrefix = objectStoreServicePrefixUniq + runObjectE2ETest(s.helper, s.k8sh, s.Suite, s.settings.Namespace, tls) + err := s.k8sh.Clientset.CoreV1().Secrets(s.settings.Namespace).Delete(context.TODO(), objectTLSSecretName, metav1.DeleteOptions{}) + if err != nil { + if !errors.IsNotFound(err) { + logger.Fatal("failed to deleted store TLS secret") + } + } + logger.Info("successfully deleted store TLS secret") +} + +func (s *ObjectSuite) TestWithBrokenTLS() { + if utils.IsPlatformOpenShift() { + s.T().Skip("object store tests skipped on openshift") + } + + tls := true + objectStoreServicePrefix = "broken" runObjectE2ETest(s.helper, s.k8sh, s.Suite, s.settings.Namespace, tls) + err := s.k8sh.Clientset.CoreV1().Secrets(s.settings.Namespace).Delete(context.TODO(), objectTLSSecretName, metav1.DeleteOptions{}) + if err != nil { + if !errors.IsNotFound(err) { + logger.Fatal("failed to deleted store TLS secret") + } + } + logger.Info("successfully deleted store TLS secret") } func (s *ObjectSuite) TestWithoutTLS() { @@ -101,6 +136,7 @@ func (s *ObjectSuite) TestWithoutTLS() { } tls := false + objectStoreServicePrefix = objectStoreServicePrefixUniq runObjectE2ETest(s.helper, s.k8sh, s.Suite, s.settings.Namespace, tls) } @@ -111,7 +147,7 @@ func (s *ObjectSuite) TestWithoutTLS() { func runObjectE2ETest(helper *clients.TestClient, k8sh *utils.K8sHelper, s suite.Suite, namespace string, tlsEnable bool) { storeName := "test-store" if tlsEnable { - storeName = "tls-test-store" + storeName = objectStoreTLSName } logger.Infof("Running on Rook Cluster %s", namespace) @@ -189,7 +225,7 @@ func testObjectStoreOperations(s suite.Suite, helper *clients.TestClient, k8sh * s3AccessKey, _ := helper.BucketClient.GetAccessKey(obcName) s3SecretKey, _ := helper.BucketClient.GetSecretKey(obcName) if objectStore.Spec.IsTLSEnabled() { - s3client, err = rgw.NewTestOnlyS3Agent(s3AccessKey, s3SecretKey, s3endpoint, region, true) + s3client, err = rgw.NewInsecureS3Agent(s3AccessKey, s3SecretKey, s3endpoint, region, true) } else { s3client, err = rgw.NewS3Agent(s3AccessKey, s3SecretKey, s3endpoint, region, true, nil) }