rgw: use insecure TLS for bucket health check

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 <seb@redhat.com>

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

@@ -24,6 +24,7 @@ import (
 	"os"
 	"path/filepath"
 	"strconv"
+	"strings"
 	"testing"
 	"time"
 
@@ -35,9 +36,15 @@ import (
 	"github.com/stretchr/testify/require"
 	"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 (
+	// #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"
@@ -183,6 +190,16 @@ func assertObjectStoreDeletion(t *testing.T, k8sh *utils.K8sHelper, namespace, s
 
 	err := k8sh.WaitUntilResourceIsDeleted("CephObjectStore", namespace, storeName)
 	assert.NoError(t, err)
+
+	if strings.Contains(storeName, "tls") {
+		err = k8sh.Clientset.CoreV1().Secrets(namespace).Delete(context.TODO(), objectTLSSecretName, metav1.DeleteOptions{})
+		if err != nil {
+			if !errors.IsNotFound(err) {
+				logger.Errorf("failed to deleted store %q TLS secret", storeName)
+			}
+		}
+		logger.Infof("successfully deleted store %q TLS secret", storeName)
+	}
 }
 
 func createCephObjectUser(

tests/integration/ceph_object_test.go

@@ -34,6 +34,15 @@ import (
 	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 +101,26 @@ func (s *ObjectSuite) TestWithTLS() {
 	}
 
 	tls := true
+	objectStoreServicePrefix = objectStoreServicePrefixUniq
+	runObjectE2ETest(s.helper, s.k8sh, s.Suite, s.settings.Namespace, tls)
+	s.T().Run("delete tls object store", func(t *testing.T) {
+		deleteObjectStore(t, s.k8sh, s.settings.Namespace, objectStoreTLSName)
+		assertObjectStoreDeletion(t, s.k8sh, s.settings.Namespace, objectStoreTLSName)
+	})
+}
+
+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)
+	s.T().Run("delete broken tls object store", func(t *testing.T) {
+		deleteObjectStore(t, s.k8sh, s.settings.Namespace, objectStoreTLSName)
+		assertObjectStoreDeletion(t, s.k8sh, s.settings.Namespace, objectStoreTLSName)
+	})
 }
 
 func (s *ObjectSuite) TestWithoutTLS() {
@@ -101,6 +129,7 @@ func (s *ObjectSuite) TestWithoutTLS() {
 	}
 
 	tls := false
+	objectStoreServicePrefix = objectStoreServicePrefixUniq
 	runObjectE2ETest(s.helper, s.k8sh, s.Suite, s.settings.Namespace, tls)
 }
 
@@ -111,7 +140,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 +218,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)
 		}