diff --git a/pkg/daemon/ceph/osd/kms/kms.go b/pkg/daemon/ceph/osd/kms/kms.go index 018143da77cc6..3217d98ac98a9 100644 --- a/pkg/daemon/ceph/osd/kms/kms.go +++ b/pkg/daemon/ceph/osd/kms/kms.go @@ -83,7 +83,14 @@ func (c *Config) PutSecret(secretName, secretValue string) error { } if c.IsVault() { // Store the secret in Vault - v, err := InitVault(c.context, c.clusterInfo.Namespace, c.clusterSpec.Security.KeyManagementService.ConnectionDetails) + removeCertFiles := make(chan bool, 1) + // Remove cert files from operator's filesystem + defer func() { + removeCertFiles <- true + close(removeCertFiles) + }() + + v, err := InitVault(c.context, c.clusterInfo.Namespace, c.clusterSpec.Security.KeyManagementService.ConnectionDetails, removeCertFiles) if err != nil { return errors.Wrap(err, "failed to init vault kms") } @@ -102,7 +109,14 @@ func (c *Config) GetSecret(secretName string) (string, error) { var value string if c.IsVault() { // Store the secret in Vault - v, err := InitVault(c.context, c.clusterInfo.Namespace, c.clusterSpec.Security.KeyManagementService.ConnectionDetails) + removeCertFiles := make(chan bool, 1) + // Remove cert files from operator's filesystem + defer func() { + removeCertFiles <- true + close(removeCertFiles) + }() + + v, err := InitVault(c.context, c.clusterInfo.Namespace, c.clusterSpec.Security.KeyManagementService.ConnectionDetails, removeCertFiles) if err != nil { return "", errors.Wrap(err, "failed to get secret in vault") } @@ -121,7 +135,14 @@ func (c *Config) GetSecret(secretName string) (string, error) { func (c *Config) DeleteSecret(secretName string) error { if c.IsVault() { // Store the secret in Vault - v, err := InitVault(c.context, c.clusterInfo.Namespace, c.clusterSpec.Security.KeyManagementService.ConnectionDetails) + removeCertFiles := make(chan bool, 1) + // Remove cert files from operator's filesystem + defer func() { + removeCertFiles <- true + close(removeCertFiles) + }() + + v, err := InitVault(c.context, c.clusterInfo.Namespace, c.clusterSpec.Security.KeyManagementService.ConnectionDetails, removeCertFiles) if err != nil { return errors.Wrap(err, "failed to delete secret in vault") } @@ -202,7 +223,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") } diff --git a/pkg/daemon/ceph/osd/kms/kms_test.go b/pkg/daemon/ceph/osd/kms/kms_test.go index 957f153fea23f..7213b209e49ea 100644 --- a/pkg/daemon/ceph/osd/kms/kms_test.go +++ b/pkg/daemon/ceph/osd/kms/kms_test.go @@ -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"}, diff --git a/pkg/daemon/ceph/osd/kms/vault.go b/pkg/daemon/ceph/osd/kms/vault.go index 5948c2fe3d8bb..d127d8fcbb720 100644 --- a/pkg/daemon/ceph/osd/kms/vault.go +++ b/pkg/daemon/ceph/osd/kms/vault.go @@ -19,7 +19,9 @@ package kms import ( "context" "io/ioutil" + "os" "strings" + "time" "github.com/hashicorp/vault/api" "github.com/libopenstorage/secrets" @@ -66,7 +68,7 @@ var ( */ // InitVault inits the secret store -func InitVault(context *clusterd.Context, namespace string, config map[string]string) (secrets.Secrets, error) { +func InitVault(context *clusterd.Context, namespace string, config map[string]string, removeCertFiles chan bool) (secrets.Secrets, error) { c := make(map[string]interface{}) // So that we don't alter the content of c.config for later iterations @@ -77,7 +79,7 @@ func InitVault(context *clusterd.Context, namespace string, config map[string]st } // Populate TLS config - newConfigWithTLS, err := configTLS(context, namespace, oriConfig) + newConfigWithTLS, err := configTLS(context, namespace, oriConfig, removeCertFiles) if err != nil { return nil, errors.Wrap(err, "failed to initialize vault tls configuration") } @@ -96,8 +98,9 @@ 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) { +func configTLS(clusterdContext *clusterd.Context, namespace string, config map[string]string, removeCertFiles chan bool) (map[string]string, error) { ctx := context.TODO() + var filesToRemove []*os.File for _, tlsOption := range cephv1.VaultTLSConnectionDetails { tlsSecretName := GetParam(config, tlsOption) if tlsSecretName == "" { @@ -124,13 +127,50 @@ func configTLS(clusterdContext *clusterd.Context, namespace string, config map[s 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) } } + // Run a goroutine that waits for a confirmation to remove the certificates from the filesystem + go func(filesToRemove []*os.File) { + logger.Debugf("files to remove: %+v", filesToRemove) + rmCertFiles := 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()) + } + } + for { + select { + case <-removeCertFiles: + logger.Debug("received confirmation from channel to remove vault certificates") + rmCertFiles() + logger.Info("successfully removed secret files") + return + case <-time.After(time.Second * 5): + logger.Info("never received anything from the channel removing vault cert files anyway") + rmCertFiles() + return + } + } + }(filesToRemove) + return config, nil } @@ -215,7 +255,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 diff --git a/pkg/daemon/ceph/osd/kms/vault_api.go b/pkg/daemon/ceph/osd/kms/vault_api.go index 2e2cdb2a7788c..0d0276b569981 100644 --- a/pkg/daemon/ceph/osd/kms/vault_api.go +++ b/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,7 +39,7 @@ 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() @@ -48,6 +49,18 @@ func newVaultClient(secretConfig map[string]string) (*api.Client, error) { c[k] = v } + // Populate TLS config + removeCertFiles := make(chan bool, 1) + newConfigWithTLS, err := configTLS(clusterdContext, namespace, secretConfig, removeCertFiles) + if err != nil { + return nil, errors.Wrap(err, "failed to initialize vault tls configuration") + } + + // 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 @@ -72,7 +85,7 @@ func newVaultClient(secretConfig map[string]string) (*api.Client, error) { 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 +104,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") } diff --git a/pkg/daemon/ceph/osd/kms/vault_api_test.go b/pkg/daemon/ceph/osd/kms/vault_api_test.go index 774298271a7eb..82874a15954d5 100644 --- a/pkg/daemon/ceph/osd/kms/vault_api_test.go +++ b/pkg/daemon/ceph/osd/kms/vault_api_test.go @@ -24,6 +24,7 @@ import ( vaulthttp "github.com/hashicorp/vault/http" "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/vault" + "github.com/rook/rook/pkg/clusterd" ) func TestBackendVersion(t *testing.T) { @@ -35,7 +36,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 +70,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 diff --git a/pkg/daemon/ceph/osd/kms/vault_test.go b/pkg/daemon/ceph/osd/kms/vault_test.go index c7c8e1ffac406..9b2948e7809bb 100644 --- a/pkg/daemon/ceph/osd/kms/vault_test.go +++ b/pkg/daemon/ceph/osd/kms/vault_test.go @@ -19,6 +19,7 @@ package kms import ( "context" "testing" + "time" "github.com/rook/rook/pkg/clusterd" "github.com/rook/rook/pkg/operator/test" @@ -51,109 +52,156 @@ func Test_tlsSecretKeyToCheck(t *testing.T) { func Test_configTLS(t *testing.T) { ctx := context.TODO() - config := map[string]string{ - "foo": "bar", - "KMS_PROVIDER": "vault", - "VAULT_ADDR": "1.1.1.1", - "VAULT_BACKEND_PATH": "vault", - } ns := "rook-ceph" context := &clusterd.Context{Clientset: test.New(t, 3)} - // No tls config - _, err := configTLS(context, ns, config) - assert.NoError(t, err) + t.Run("no TLS config", func(t *testing.T) { + config := map[string]string{ + "foo": "bar", + "KMS_PROVIDER": "vault", + "VAULT_ADDR": "1.1.1.1", + "VAULT_BACKEND_PATH": "vault", + } + // No tls config + removeCertFiles := make(chan bool, 1) + _, err := configTLS(context, ns, config, removeCertFiles) + assert.NoError(t, err) + close(removeCertFiles) + }) - // TLS config with correct values - config = map[string]string{ - "foo": "bar", - "KMS_PROVIDER": "vault", - "VAULT_ADDR": "1.1.1.1", - "VAULT_BACKEND_PATH": "vault", - "VAULT_CACERT": "/etc/vault/cacert", - "VAULT_SKIP_VERIFY": "false", - } - config, err = configTLS(context, ns, config) - assert.NoError(t, err) - assert.Equal(t, "/etc/vault/cacert", config["VAULT_CACERT"]) + t.Run("TLS config with already populated cert path", func(t *testing.T) { + removeCertFiles := make(chan bool, 1) + config := map[string]string{ + "foo": "bar", + "KMS_PROVIDER": "vault", + "VAULT_ADDR": "1.1.1.1", + "VAULT_BACKEND_PATH": "vault", + "VAULT_CACERT": "/etc/vault/cacert", + "VAULT_SKIP_VERIFY": "false", + } + config, err := configTLS(context, ns, config, removeCertFiles) + assert.NoError(t, err) + assert.Equal(t, "/etc/vault/cacert", config["VAULT_CACERT"]) + close(removeCertFiles) + }) - // TLS config but no secret - config = map[string]string{ - "foo": "bar", - "KMS_PROVIDER": "vault", - "VAULT_ADDR": "1.1.1.1", - "VAULT_BACKEND_PATH": "vault", - "VAULT_CACERT": "vault-ca-cert", - "VAULT_SKIP_VERIFY": "false", - } - _, err = configTLS(context, ns, config) - assert.Error(t, err) - assert.EqualError(t, err, "failed to fetch tls k8s secret \"vault-ca-cert\": secrets \"vault-ca-cert\" not found") + t.Run("TLS config but no secret", func(t *testing.T) { + removeCertFiles := make(chan bool, 1) + config := map[string]string{ + "foo": "bar", + "KMS_PROVIDER": "vault", + "VAULT_ADDR": "1.1.1.1", + "VAULT_BACKEND_PATH": "vault", + "VAULT_CACERT": "vault-ca-cert", + "VAULT_SKIP_VERIFY": "false", + } + _, err := configTLS(context, ns, config, removeCertFiles) + assert.Error(t, err) + assert.EqualError(t, err, "failed to fetch tls k8s secret \"vault-ca-cert\": secrets \"vault-ca-cert\" not found") + close(removeCertFiles) + }) - // TLS config success! - config = map[string]string{ - "foo": "bar", - "KMS_PROVIDER": "vault", - "VAULT_ADDR": "1.1.1.1", - "VAULT_BACKEND_PATH": "vault", - "VAULT_CACERT": "vault-ca-cert", - "VAULT_SKIP_VERIFY": "false", - } - s := &v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "vault-ca-cert", - Namespace: ns, - }, - Data: map[string][]byte{"cert": []byte("bar")}, - } - _, err = context.Clientset.CoreV1().Secrets(ns).Create(ctx, s, metav1.CreateOptions{}) - assert.NoError(t, err) - config, err = configTLS(context, ns, config) - assert.NoError(t, err) - assert.NotEqual(t, "vault-ca-cert", config["VAULT_CACERT"]) - err = context.Clientset.CoreV1().Secrets(ns).Delete(ctx, s.Name, metav1.DeleteOptions{}) - assert.NoError(t, err) + t.Run("TLS config success!", func(t *testing.T) { + removeCertFiles := make(chan bool, 1) + config := map[string]string{ + "foo": "bar", + "KMS_PROVIDER": "vault", + "VAULT_ADDR": "1.1.1.1", + "VAULT_BACKEND_PATH": "vault", + "VAULT_CACERT": "vault-ca-cert", + "VAULT_SKIP_VERIFY": "false", + } + s := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vault-ca-cert", + Namespace: ns, + }, + Data: map[string][]byte{"cert": []byte("bar")}, + } + _, err := context.Clientset.CoreV1().Secrets(ns).Create(ctx, s, metav1.CreateOptions{}) + assert.NoError(t, err) + config, err = configTLS(context, ns, config, removeCertFiles) + assert.NoError(t, err) + assert.NotEqual(t, "vault-ca-cert", config["VAULT_CACERT"]) + err = context.Clientset.CoreV1().Secrets(ns).Delete(ctx, s.Name, metav1.DeleteOptions{}) + assert.NoError(t, err) + removeCertFiles <- true + close(removeCertFiles) + }) - // All TLS success! - config = 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", - } - sCa := &v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "vault-ca-cert", - Namespace: ns, - }, - Data: map[string][]byte{"cert": []byte("bar")}, - } - sClCert := &v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "vault-client-cert", - Namespace: ns, - }, - Data: map[string][]byte{"cert": []byte("bar")}, - } - sClKey := &v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "vault-client-key", - Namespace: ns, - }, - Data: map[string][]byte{"key": []byte("bar")}, - } - _, err = context.Clientset.CoreV1().Secrets(ns).Create(ctx, sCa, metav1.CreateOptions{}) - assert.NoError(t, err) - _, err = context.Clientset.CoreV1().Secrets(ns).Create(ctx, sClCert, metav1.CreateOptions{}) - assert.NoError(t, err) - _, err = context.Clientset.CoreV1().Secrets(ns).Create(ctx, sClKey, metav1.CreateOptions{}) - assert.NoError(t, err) - config, err = configTLS(context, ns, config) - assert.NoError(t, err) - assert.NotEqual(t, "vault-ca-cert", config["VAULT_CACERT"]) - assert.NotEqual(t, "vault-client-cert", config["VAULT_CLIENT_CERT"]) - assert.NotEqual(t, "vault-client-key", config["VAULT_CLIENT_KEY"]) + t.Run("advanced TLS config success!", func(t *testing.T) { + removeCertFiles := make(chan bool, 1) + config := 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", + } + sCa := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vault-ca-cert", + Namespace: ns, + }, + Data: map[string][]byte{"cert": []byte("bar")}, + } + sClCert := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vault-client-cert", + Namespace: ns, + }, + Data: map[string][]byte{"cert": []byte("bar")}, + } + sClKey := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vault-client-key", + Namespace: ns, + }, + Data: map[string][]byte{"key": []byte("bar")}, + } + _, err := context.Clientset.CoreV1().Secrets(ns).Create(ctx, sCa, metav1.CreateOptions{}) + assert.NoError(t, err) + _, err = context.Clientset.CoreV1().Secrets(ns).Create(ctx, sClCert, metav1.CreateOptions{}) + assert.NoError(t, err) + _, err = context.Clientset.CoreV1().Secrets(ns).Create(ctx, sClKey, metav1.CreateOptions{}) + assert.NoError(t, err) + config, err = configTLS(context, ns, config, removeCertFiles) + assert.NoError(t, err) + assert.NotEqual(t, "vault-ca-cert", config["VAULT_CACERT"]) + assert.NotEqual(t, "vault-client-cert", config["VAULT_CLIENT_CERT"]) + assert.NotEqual(t, "vault-client-key", config["VAULT_CLIENT_KEY"]) + assert.FileExists(t, config["VAULT_CACERT"]) + removeCertFiles <- true + close(removeCertFiles) + time.Sleep(time.Second) + assert.NoFileExists(t, config["VAULT_CACERT"]) + assert.NoFileExists(t, config["VAULT_CLIENT_CERT"]) + assert.NoFileExists(t, config["VAULT_CLIENT_KEY"]) + }) + + t.Run("advanced TLS config success with timeout!", func(t *testing.T) { + removeCertFiles := make(chan bool, 1) + config := 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", + } + config, err := configTLS(context, ns, config, removeCertFiles) + assert.NoError(t, err) + assert.NotEqual(t, "vault-ca-cert", config["VAULT_CACERT"]) + assert.NotEqual(t, "vault-client-cert", config["VAULT_CLIENT_CERT"]) + assert.NotEqual(t, "vault-client-key", config["VAULT_CLIENT_KEY"]) + assert.FileExists(t, config["VAULT_CACERT"]) + time.Sleep(time.Second * 6) + assert.NoFileExists(t, config["VAULT_CACERT"]) + assert.NoFileExists(t, config["VAULT_CLIENT_CERT"]) + assert.NoFileExists(t, config["VAULT_CLIENT_KEY"]) + close(removeCertFiles) + }) } diff --git a/tests/manifests/test-kms-vault-spec.yaml b/tests/manifests/test-kms-vault-spec.yaml index d9541f960533b..6848fe48d69be 100644 --- a/tests/manifests/test-kms-vault-spec.yaml +++ b/tests/manifests/test-kms-vault-spec.yaml @@ -7,4 +7,7 @@ spec: VAULT_BACKEND_PATH: rook/ver1 VAULT_SECRET_ENGINE: kv VAULT_SKIP_VERIFY: "true" + VAULT_CLIENT_KEY: "vault-client-key" + VAULT_CLIENT_CERT: "vault-client-cert" + VAULT_CACERT: "vault-ca-cert" tokenSecretName: rook-vault-token