Merge pull request #8977 from rook/mergify/bp/release-1.7/pr-8867

ceph: fix kms auto-detection when full TLS (backport #8867)

pkg/daemon/ceph/osd/kms/kms.go

@@ -202,7 +202,7 @@ func ValidateConnectionDetails(clusterdContext *clusterd.Context, securitySpec *
 		case VaultKVSecretEngineKey:
 			// Append Backend Version if not already present
 			if GetParam(securitySpec.KeyManagementService.ConnectionDetails, vault.VaultBackendKey) == "" {
-				backendVersion, err := BackendVersion(securitySpec.KeyManagementService.ConnectionDetails)
+				backendVersion, err := BackendVersion(clusterdContext, ns, securitySpec.KeyManagementService.ConnectionDetails)
 				if err != nil {
 					return errors.Wrap(err, "failed to get backend version")
 				}

pkg/daemon/ceph/osd/kms/kms_test.go

@@ -91,7 +91,7 @@ func TestValidateConnectionDetails(t *testing.T) {
 	securitySpec.KeyManagementService.ConnectionDetails["VAULT_CACERT"] = "vault-ca-secret"
 	err = ValidateConnectionDetails(context, securitySpec, ns)
 	assert.Error(t, err, "")
-	assert.EqualError(t, err, "failed to validate vault connection details: failed to find TLS connection details k8s secret \"VAULT_CACERT\"")
+	assert.EqualError(t, err, "failed to validate vault connection details: failed to find TLS connection details k8s secret \"vault-ca-secret\"")
 
 	// Error: TLS secret exists but empty key
 	tlsSecret := &v1.Secret{
@@ -122,7 +122,9 @@ func TestValidateConnectionDetails(t *testing.T) {
 		vault.TestWaitActive(t, core)
 		client := cluster.Cores[0].Client
 		// Mock the client here
-		vaultClient = func(secretConfig map[string]string) (*api.Client, error) { return client, nil }
+		vaultClient = func(clusterdContext *clusterd.Context, namespace string, secretConfig map[string]string) (*api.Client, error) {
+			return client, nil
+		}
 		if err := client.Sys().Mount("rook/", &api.MountInput{
 			Type:    "kv-v2",
 			Options: map[string]string{"version": "2"},

pkg/daemon/ceph/osd/kms/vault.go

@@ -19,6 +19,7 @@ package kms
 import (
 	"context"
 	"io/ioutil"
+	"os"
 	"strings"
 
 	"github.com/hashicorp/vault/api"
@@ -45,6 +46,14 @@ var (
 	vaultMandatoryConnectionDetails = []string{api.EnvVaultAddress}
 )
 
+// Used for unit tests mocking too as well as production code
+var (
+	createTmpFile      = ioutil.TempFile
+	getRemoveCertFiles = getRemoveCertFilesFunc
+)
+
+type removeCertFilesFunction func()
+
 /* VAULT API INTERNAL VALUES
 // Refer to https://pkg.golangclub.com/github.com/hashicorp/vault/api?tab=doc#pkg-constants
    const EnvVaultAddress = "VAULT_ADDR"
@@ -77,10 +86,11 @@ func InitVault(context *clusterd.Context, namespace string, config map[string]st
 	}
 
 	// Populate TLS config
-	newConfigWithTLS, err := configTLS(context, namespace, oriConfig)
+	newConfigWithTLS, removeCertFiles, err := configTLS(context, namespace, oriConfig)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to initialize vault tls configuration")
 	}
+	defer removeCertFiles()
 
 	// Populate TLS config
 	for key, value := range newConfigWithTLS {
@@ -96,8 +106,31 @@ func InitVault(context *clusterd.Context, namespace string, config map[string]st
 	return v, nil
 }
 
-func configTLS(clusterdContext *clusterd.Context, namespace string, config map[string]string) (map[string]string, error) {
+// configTLS returns a map of TLS config that map physical files for the TLS library to load
+// Also it returns a function to remove the temporary files (certs, keys)
+// The signature has named result parameters to help building 'defer' statements especially for the
+// content of removeCertFiles which needs to be populated by the files to remove if no errors and be
+// nil on errors
+func configTLS(clusterdContext *clusterd.Context, namespace string, config map[string]string) (newConfig map[string]string, removeCertFiles removeCertFilesFunction, retErr error) {
 	ctx := context.TODO()
+	var filesToRemove []*os.File
+
+	defer func() {
+		// Build the function that the caller should use to remove the temp files here
+		// create it when this function is returning based on the currently-recorded files
+		removeCertFiles = getRemoveCertFiles(filesToRemove)
+		if retErr != nil {
+			// If we encountered an error, remove the temp files
+			removeCertFiles()
+
+			// Also return an empty function to remove the temp files
+			// It's fine to use nil here since the defer from the calling functions is only
+			// triggered after evaluating any error, if on error the defer is not triggered since we
+			// have returned already
+			removeCertFiles = nil
+		}
+	}()
+
 	for _, tlsOption := range cephv1.VaultTLSConnectionDetails {
 		tlsSecretName := GetParam(config, tlsOption)
 		if tlsSecretName == "" {
@@ -107,31 +140,52 @@ func configTLS(clusterdContext *clusterd.Context, namespace string, config map[s
 		if !strings.Contains(tlsSecretName, EtcVaultDir) {
 			secret, err := clusterdContext.Clientset.CoreV1().Secrets(namespace).Get(ctx, tlsSecretName, v1.GetOptions{})
 			if err != nil {
-				return nil, errors.Wrapf(err, "failed to fetch tls k8s secret %q", tlsSecretName)
+				return nil, removeCertFiles, errors.Wrapf(err, "failed to fetch tls k8s secret %q", tlsSecretName)
 			}
-
 			// Generate a temp file
-			file, err := ioutil.TempFile("", "")
+			file, err := createTmpFile("", "")
 			if err != nil {
-				return nil, errors.Wrapf(err, "failed to generate temp file for k8s secret %q content", tlsSecretName)
+				return nil, removeCertFiles, errors.Wrapf(err, "failed to generate temp file for k8s secret %q content", tlsSecretName)
 			}
 
 			// Write into a file
 			err = ioutil.WriteFile(file.Name(), secret.Data[tlsSecretKeyToCheck(tlsOption)], 0444)
 			if err != nil {
-				return nil, errors.Wrapf(err, "failed to write k8s secret %q content to a file", tlsSecretName)
+				return nil, removeCertFiles, errors.Wrapf(err, "failed to write k8s secret %q content to a file", tlsSecretName)
 			}
 
 			logger.Debugf("replacing %q current content %q with %q", tlsOption, config[tlsOption], file.Name())
 
-			// update the env var with the path
+			// Update the env var with the path
 			config[tlsOption] = file.Name()
+
+			// Add the file to the list of files to remove
+			filesToRemove = append(filesToRemove, file)
 		} else {
 			logger.Debugf("value of tlsOption %q tlsSecretName is already correct %q", tlsOption, tlsSecretName)
 		}
 	}
 
-	return config, nil
+	return config, removeCertFiles, nil
+}
+
+func getRemoveCertFilesFunc(filesToRemove []*os.File) removeCertFilesFunction {
+	return removeCertFilesFunction(func() {
+		for _, file := range filesToRemove {
+			logger.Debugf("closing %q", file.Name())
+			err := file.Close()
+			if err != nil {
+				logger.Errorf("failed to close file %q. %v", file.Name(), err)
+			}
+			logger.Debugf("closed %q", file.Name())
+			logger.Debugf("removing %q", file.Name())
+			err = os.Remove(file.Name())
+			if err != nil {
+				logger.Errorf("failed to remove file %q. %v", file.Name(), err)
+			}
+			logger.Debugf("removed %q", file.Name())
+		}
+	})
 }
 
 func put(v secrets.Secrets, secretName, secretValue string, keyContext map[string]string) error {
@@ -215,7 +269,7 @@ func validateVaultConnectionDetails(clusterdContext *clusterd.Context, ns string
 			// Fetch the secret
 			s, err := clusterdContext.Clientset.CoreV1().Secrets(ns).Get(ctx, tlsSecretName, v1.GetOptions{})
 			if err != nil {
-				return errors.Errorf("failed to find TLS connection details k8s secret %q", tlsOption)
+				return errors.Errorf("failed to find TLS connection details k8s secret %q", tlsSecretName)
 			}
 
 			// Check the Secret key and its content

pkg/daemon/ceph/osd/kms/vault_api.go

@@ -23,6 +23,7 @@ import (
 	"github.com/libopenstorage/secrets/vault"
 	"github.com/libopenstorage/secrets/vault/utils"
 	"github.com/pkg/errors"
+	"github.com/rook/rook/pkg/clusterd"
 
 	"github.com/hashicorp/vault/api"
 )
@@ -38,16 +39,35 @@ var vaultClient = newVaultClient
 
 // newVaultClient returns a vault client, there is no need for any secretConfig validation
 // Since this is called after an already validated call InitVault()
-func newVaultClient(secretConfig map[string]string) (*api.Client, error) {
+func newVaultClient(clusterdContext *clusterd.Context, namespace string, secretConfig map[string]string) (*api.Client, error) {
 	// DefaultConfig uses the environment variables if present.
 	config := api.DefaultConfig()
 
+	// Always use a new map otherwise the map will mutate and subsequent calls will fail since the
+	// TLS content has been altered by the TLS config in vaultClient()
+	localSecretConfig := make(map[string]string)
+	for k, v := range secretConfig {
+		localSecretConfig[k] = v
+	}
+
 	// Convert map string to map interface
 	c := make(map[string]interface{})
-	for k, v := range secretConfig {
+	for k, v := range localSecretConfig {
 		c[k] = v
 	}
 
+	// Populate TLS config
+	newConfigWithTLS, removeCertFiles, err := configTLS(clusterdContext, namespace, localSecretConfig)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to initialize vault tls configuration")
+	}
+	defer removeCertFiles()
+
+	// Populate TLS config
+	for key, value := range newConfigWithTLS {
+		c[key] = string(value)
+	}
+
 	// Configure TLS
 	if err := utils.ConfigureTLS(config, c); err != nil {
 		return nil, err
@@ -64,15 +84,15 @@ func newVaultClient(secretConfig map[string]string) (*api.Client, error) {
 	client.SetToken(strings.TrimSuffix(os.Getenv(api.EnvVaultToken), "\n"))
 
 	// Set Vault address, was validated by ValidateConnectionDetails()
-	err = client.SetAddress(strings.TrimSuffix(secretConfig[api.EnvVaultAddress], "\n"))
+	err = client.SetAddress(strings.TrimSuffix(localSecretConfig[api.EnvVaultAddress], "\n"))
 	if err != nil {
 		return nil, err
 	}
 
 	return client, nil
 }
 
-func BackendVersion(secretConfig map[string]string) (string, error) {
+func BackendVersion(clusterdContext *clusterd.Context, namespace string, secretConfig map[string]string) (string, error) {
 	v1 := "v1"
 	v2 := "v2"
 
@@ -91,7 +111,7 @@ func BackendVersion(secretConfig map[string]string) (string, error) {
 		return v2, nil
 	default:
 		// Initialize Vault client
-		vaultClient, err := vaultClient(secretConfig)
+		vaultClient, err := vaultClient(clusterdContext, namespace, secretConfig)
 		if err != nil {
 			return "", errors.Wrap(err, "failed to initialize vault client")
 		}

pkg/daemon/ceph/osd/kms/vault_api_test.go

@@ -17,13 +17,20 @@ limitations under the License.
 package kms
 
 import (
+	"context"
 	"testing"
 
 	kv "github.com/hashicorp/vault-plugin-secrets-kv"
 	"github.com/hashicorp/vault/api"
 	vaulthttp "github.com/hashicorp/vault/http"
 	"github.com/hashicorp/vault/sdk/logical"
 	"github.com/hashicorp/vault/vault"
+	"github.com/libopenstorage/secrets/vault/utils"
+	"github.com/rook/rook/pkg/clusterd"
+	"github.com/rook/rook/pkg/operator/test"
+	"github.com/stretchr/testify/assert"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestBackendVersion(t *testing.T) {
@@ -35,7 +42,9 @@ func TestBackendVersion(t *testing.T) {
 	client := cluster.Cores[0].Client
 
 	// Mock the client here
-	vaultClient = func(secretConfig map[string]string) (*api.Client, error) { return client, nil }
+	vaultClient = func(clusterdContext *clusterd.Context, namespace string, secretConfig map[string]string) (*api.Client, error) {
+		return client, nil
+	}
 
 	// Set up the kv store
 	if err := client.Sys().Mount("rook/", &api.MountInput{
@@ -67,7 +76,7 @@ func TestBackendVersion(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := BackendVersion(tt.args.secretConfig)
+			got, err := BackendVersion(&clusterd.Context{}, "ns", tt.args.secretConfig)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("BackendVersion() error = %v, wantErr %v", err, tt.wantErr)
 				return
@@ -91,3 +100,102 @@ func fakeVaultServer(t *testing.T) *vault.TestCluster {
 
 	return cluster
 }
+
+func TestTLSConfig(t *testing.T) {
+	ns := "rook-ceph"
+	ctx := context.TODO()
+	context := &clusterd.Context{Clientset: test.New(t, 3)}
+	secretConfig := map[string]string{
+		"foo":                "bar",
+		"KMS_PROVIDER":       "vault",
+		"VAULT_ADDR":         "1.1.1.1",
+		"VAULT_BACKEND_PATH": "vault",
+		"VAULT_CACERT":       "vault-ca-cert",
+		"VAULT_CLIENT_CERT":  "vault-client-cert",
+		"VAULT_CLIENT_KEY":   "vault-client-key",
+	}
+
+	// DefaultConfig uses the environment variables if present.
+	config := api.DefaultConfig()
+
+	// Convert map string to map interface
+	c := make(map[string]interface{})
+	for k, v := range secretConfig {
+		c[k] = v
+	}
+
+	sCa := &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "vault-ca-cert",
+			Namespace: ns,
+		},
+		Data: map[string][]byte{"cert": []byte(`-----BEGIN CERTIFICATE-----
+MIIBJTCB0AIJAPNFNz1CNlDOMA0GCSqGSIb3DQEBCwUAMBoxCzAJBgNVBAYTAkZS
+MQswCQYDVQQIDAJGUjAeFw0yMTA5MzAwODAzNDBaFw0yNDA2MjYwODAzNDBaMBox
+CzAJBgNVBAYTAkZSMQswCQYDVQQIDAJGUjBcMA0GCSqGSIb3DQEBAQUAA0sAMEgC
+QQDHeZ47hVBcryl6SCghM8Zj3Q6DQzJzno1J7EjPXef5m+pIVAEylS9sQuwKtFZc
+vv3qS/OVFExmMdbrvfKEIfbBAgMBAAEwDQYJKoZIhvcNAQELBQADQQAAnflLuUM3
+4Dq0v7If4cgae2mr7jj3U/lIpHVtFbF7kVjC/eqmeN1a9u0UbRHKkUr+X1mVX3rJ
+BvjQDN6didwQ
+-----END CERTIFICATE-----`)},
+	}
+
+	sClCert := &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "vault-client-cert",
+			Namespace: ns,
+		},
+		Data: map[string][]byte{"cert": []byte(`-----BEGIN CERTIFICATE-----
+MIIBEDCBuwIBATANBgkqhkiG9w0BAQUFADAaMQswCQYDVQQGEwJGUjELMAkGA1UE
+CAwCRlIwHhcNMjEwOTMwMDgwNDA1WhcNMjQwNjI2MDgwNDA1WjANMQswCQYDVQQG
+EwJGUjBcMA0GCSqGSIb3DQEBAQUAA0sAMEgCQQCpWJqKhSES3BiFkt2M82xy3tkB
+plDS8DM0s/+VkqfZlVG18KbbIVDHi1lsPjjs/Aja7lWymw0ycV4KGEcqxdmNAgMB
+AAEwDQYJKoZIhvcNAQEFBQADQQC5esmoTqp4uEWyC+GKbTTFp8ngMUywAtZJs4nS
+wdoF3ZJJzo4ps0saP1ww5LBdeeXUURscxyaFfCFmGODaHJJn
+-----END CERTIFICATE-----`)},
+	}
+
+	sClKey := &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "vault-client-key",
+			Namespace: ns,
+		},
+		Data: map[string][]byte{"key": []byte(`-----BEGIN PRIVATE KEY-----
+MIIBVgIBADANBgkqhkiG9w0BAQEFAASCAUAwggE8AgEAAkEAqViaioUhEtwYhZLd
+jPNsct7ZAaZQ0vAzNLP/lZKn2ZVRtfCm2yFQx4tZbD447PwI2u5VspsNMnFeChhH
+KsXZjQIDAQABAkARlCv+oxEq1wQIoZUz83TXe8CFBlGvg9Wc6+5lBWM9F7K4by7i
+IB5hQ2oaTNN+1Kxzf+XRM9R7sMPP9qFEp0LhAiEA0PzsQqbvNUVEx8X16Hed6V/Z
+yvL1iZeHvc2QIbGjZGkCIQDPcM7U0frsFIPuMY4zpX2b6w4rpxZN7Kybp9/3l0tX
+hQIhAJVWVsGeJksLr4WNuRYf+9BbNPdoO/rRNCd2L+tT060ZAiEAl0uontITl9IS
+s0yTcZm29lxG9pGkE+uVrOWQ1W0Ud10CIQDJ/L+VCQgjO+SviUECc/nMwhWDMT+V
+cjLxGL8tcZjHKg==
+-----END PRIVATE KEY-----`)},
+	}
+
+	for _, s := range []*v1.Secret{sCa, sClCert, sClKey} {
+		if secret, err := context.Clientset.CoreV1().Secrets(ns).Create(ctx, s, metav1.CreateOptions{}); err != nil {
+			t.Fatal(err)
+		} else {
+			defer func() {
+				err := context.Clientset.CoreV1().Secrets(ns).Delete(ctx, secret.Name, metav1.DeleteOptions{})
+				if err != nil {
+					logger.Errorf("failed to delete secret %s: %v", secret.Name, err)
+				}
+			}()
+		}
+	}
+
+	// Populate TLS config
+	newConfigWithTLS, removeCertFiles, err := configTLS(context, ns, secretConfig)
+	assert.NoError(t, err)
+	defer removeCertFiles()
+
+	// Populate TLS config
+	for key, value := range newConfigWithTLS {
+		c[key] = string(value)
+	}
+
+	// Configure TLS
+	err = utils.ConfigureTLS(config, c)
+	assert.NoError(t, err)
+}