Merge pull request #8712 from leseb/fix-8663

rgw: use insecure TLS for bucket health check

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

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")
 	}

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
 }

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,
 	}
 }

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)
+	})
+}

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"

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,15 @@ 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) TestWithoutTLS() {
@@ -101,6 +119,7 @@ func (s *ObjectSuite) TestWithoutTLS() {
 	}
 
 	tls := false
+	objectStoreServicePrefix = objectStoreServicePrefixUniq
 	runObjectE2ETest(s.helper, s.k8sh, s.Suite, s.settings.Namespace, tls)
 }
 
@@ -111,7 +130,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)
@@ -125,6 +144,18 @@ func runObjectE2ETest(helper *clients.TestClient, k8sh *utils.K8sHelper, s suite
 		deleteStore := true
 		runObjectE2ETestLite(t, helper, k8sh, namespace, otherStoreName, 1, deleteStore, tlsEnable)
 	})
+	if tlsEnable {
+		// test that a third object store can be created (and deleted) while the first exists
+		s.T().Run("run a third object store with broken tls", func(t *testing.T) {
+			otherStoreName := "broken-" + storeName
+			// The lite e2e test is perfect, as it only creates a cluster, checks that it is healthy,
+			// and then deletes it.
+			deleteStore := true
+			objectStoreServicePrefix = objectStoreServicePrefixUniq
+			runObjectE2ETestLite(t, helper, k8sh, namespace, otherStoreName, 1, deleteStore, tlsEnable)
+			objectStoreServicePrefix = objectStoreServicePrefixUniq
+		})
+	}
 
 	// now test operation of the first object store
 	testObjectStoreOperations(s, helper, k8sh, namespace, storeName)
@@ -189,7 +220,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)
 		}