From f93ea93da7e1c50218b0339f7396f7895fe37c0b Mon Sep 17 00:00:00 2001 From: Eduard Serra Date: Fri, 26 Feb 2021 15:59:00 -0800 Subject: [PATCH] Add generic SynchronizeCertificate helper API Moved the individual tresor-only logic to synchronize a secret to a broadly available helper API, to be reused later by necessary other parts/packages. This API will be used shortly in injector, to synchronize webhook certificate. There was an entire file explicitely testing the related cert functions in cli/osm-controller, I've moved the tests to a more suitable location (plus rebased it to use testing cascade). Signed-off-by: Eduard Serra --- cmd/osm-controller/certificates_test.go | 180 ----------------------- pkg/certificate/providers/config.go | 103 +++++++------ pkg/certificate/providers/config_test.go | 156 ++++++++++++++++++++ pkg/certificate/providers/errors.go | 1 + 4 files changed, 214 insertions(+), 226 deletions(-) delete mode 100644 cmd/osm-controller/certificates_test.go diff --git a/cmd/osm-controller/certificates_test.go b/cmd/osm-controller/certificates_test.go deleted file mode 100644 index 506701cc4a..0000000000 --- a/cmd/osm-controller/certificates_test.go +++ /dev/null @@ -1,180 +0,0 @@ -package main - -import ( - "context" - "time" - - "github.com/google/uuid" - corev1 "k8s.io/api/core/v1" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - testclient "k8s.io/client-go/kubernetes/fake" - - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" - - "github.com/openservicemesh/osm/pkg/certificate/pem" - "github.com/openservicemesh/osm/pkg/certificate/providers" - "github.com/openservicemesh/osm/pkg/certificate/providers/tresor" - "github.com/openservicemesh/osm/pkg/constants" - "github.com/openservicemesh/osm/pkg/tests" -) - -var _ = Describe("Test CMD tools", func() { - certPEM, err := tests.GetPEMCert() - It("should have resulted in no errors", func() { - Expect(err).ToNot(HaveOccurred()) - }) - - keyPEM, err := tests.GetPEMPrivateKey() - It("should have resulted in no errors", func() { - Expect(err).ToNot(HaveOccurred()) - }) - - Context("Testing getCertFromKubernetes", func() { - It("obtained root cert from k8s", func() { - kubeClient := testclient.NewSimpleClientset() - - ns := uuid.New().String() - secretName := uuid.New().String() - - certProviderConfig := providers.NewCertificateProviderConfig(kubeClient, nil, nil, providers.Kind(certProviderKind), ns, - secretName, tresorOptions, vaultOptions, certManagerOptions) - Expect(err).ToNot(HaveOccurred()) - - secret := &corev1.Secret{ - ObjectMeta: v1.ObjectMeta{ - Name: secretName, - Namespace: ns, - }, - Data: map[string][]byte{ - constants.KubernetesOpaqueSecretCAKey: certPEM, - constants.KubernetesOpaqueSecretCAExpiration: []byte("2020-05-07T14:25:18.677Z"), - constants.KubernetesOpaqueSecretRootPrivateKeyKey: keyPEM, - }, - } - - _, err := kubeClient.CoreV1().Secrets(ns).Create(context.Background(), secret, v1.CreateOptions{}) - Expect(err).ToNot(HaveOccurred()) - - actual, err := certProviderConfig.GetCertFromKubernetes() - Expect(err).ToNot(HaveOccurred()) - - expectedCert := pem.Certificate(certPEM) - expectedKey := pem.PrivateKey(keyPEM) - expiration, err := time.Parse(constants.TimeDateLayout, "2020-05-07T14:25:18.677Z") - Expect(err).ToNot(HaveOccurred()) - - expected, err := tresor.NewCertificateFromPEM(expectedCert, expectedKey, expiration) - Expect(err).ToNot(HaveOccurred()) - - Expect(actual).To(Equal(expected)) - }) - - It("should not error when the root certificate secret is not found", func() { - kubeClient := testclient.NewSimpleClientset() - - ns := uuid.New().String() - secretName := uuid.New().String() - - certProviderConfig := providers.NewCertificateProviderConfig(kubeClient, nil, nil, providers.Kind(certProviderKind), ns, - secretName, tresorOptions, vaultOptions, certManagerOptions) - Expect(err).ToNot(HaveOccurred()) - - rootCert, err := certProviderConfig.GetCertFromKubernetes() - Expect(err).ToNot(HaveOccurred()) - Expect(rootCert).To(BeNil()) - }) - - It("should return an error when the root cert CA is missing in the secret", func() { - kubeClient := testclient.NewSimpleClientset() - - ns := uuid.New().String() - secretName := uuid.New().String() - - certProviderConfig := providers.NewCertificateProviderConfig(kubeClient, nil, nil, providers.Kind(certProviderKind), ns, - secretName, tresorOptions, vaultOptions, certManagerOptions) - Expect(err).ToNot(HaveOccurred()) - - keyPEM := []byte(uuid.New().String()) - - secret := &corev1.Secret{ - ObjectMeta: v1.ObjectMeta{ - Name: secretName, - Namespace: ns, - }, - Data: map[string][]byte{ - constants.KubernetesOpaqueSecretCAExpiration: []byte("2020-05-07T14:25:18.677Z"), - constants.KubernetesOpaqueSecretRootPrivateKeyKey: keyPEM, - }, - } - - _, err := kubeClient.CoreV1().Secrets(ns).Create(context.Background(), secret, v1.CreateOptions{}) - Expect(err).ToNot(HaveOccurred()) - - rootCert, err := certProviderConfig.GetCertFromKubernetes() - Expect(err).To(HaveOccurred()) - Expect(rootCert).To(BeNil()) - }) - - It("should return an error when the root cert private key is missing in the secret", func() { - kubeClient := testclient.NewSimpleClientset() - - ns := uuid.New().String() - secretName := uuid.New().String() - - certProviderConfig := providers.NewCertificateProviderConfig(kubeClient, nil, nil, providers.Kind(certProviderKind), ns, - secretName, tresorOptions, vaultOptions, certManagerOptions) - Expect(err).ToNot(HaveOccurred()) - - secret := &corev1.Secret{ - ObjectMeta: v1.ObjectMeta{ - Name: secretName, - Namespace: ns, - }, - Data: map[string][]byte{ - constants.KubernetesOpaqueSecretCAKey: certPEM, - constants.KubernetesOpaqueSecretCAExpiration: []byte("2020-05-07T14:25:18.677Z"), - }, - } - - _, err := kubeClient.CoreV1().Secrets(ns).Create(context.Background(), secret, v1.CreateOptions{}) - Expect(err).ToNot(HaveOccurred()) - - rootCert, err := certProviderConfig.GetCertFromKubernetes() - Expect(err).To(HaveOccurred()) - Expect(rootCert).To(BeNil()) - }) - - It("should return an error when the root cert expiration time is missing in the secret", func() { - kubeClient := testclient.NewSimpleClientset() - - ns := uuid.New().String() - secretName := uuid.New().String() - - certProviderConfig := providers.NewCertificateProviderConfig(kubeClient, nil, nil, providers.Kind(certProviderKind), ns, - secretName, tresorOptions, vaultOptions, certManagerOptions) - Expect(err).ToNot(HaveOccurred()) - - certPEM := []byte(uuid.New().String()) - keyPEM := []byte(uuid.New().String()) - - secret := &corev1.Secret{ - ObjectMeta: v1.ObjectMeta{ - Name: secretName, - Namespace: ns, - }, - Data: map[string][]byte{ - constants.KubernetesOpaqueSecretCAKey: certPEM, - constants.KubernetesOpaqueSecretRootPrivateKeyKey: keyPEM, - }, - } - - _, err := kubeClient.CoreV1().Secrets(ns).Create(context.Background(), secret, v1.CreateOptions{}) - Expect(err).ToNot(HaveOccurred()) - - rootCert, err := certProviderConfig.GetCertFromKubernetes() - Expect(err).To(HaveOccurred()) - Expect(rootCert).To(BeNil()) - }) - }) -}) diff --git a/pkg/certificate/providers/config.go b/pkg/certificate/providers/config.go index 6464ec8bc9..e35c81777d 100644 --- a/pkg/certificate/providers/config.go +++ b/pkg/certificate/providers/config.go @@ -154,6 +154,45 @@ func (c *Config) GetCertificateManager() (certificate.Manager, debugger.Certific } } +// GetCertificateFromSecret is a helper function that ensures creation and synchronization of a certificate +// using Kubernetes Secrets backend and API atomicity. +func GetCertificateFromSecret(ns string, secretName string, cert certificate.Certificater, kubeClient kubernetes.Interface) (certificate.Certificater, error) { + // Attempt to create it in Kubernetes. When multiple agents attempt to create, only one of them will succeed. + // All others will get "AlreadyExists" error back. + secretData := map[string][]byte{ + constants.KubernetesOpaqueSecretCAKey: cert.GetCertificateChain(), + constants.KubernetesOpaqueSecretCAExpiration: []byte(cert.GetExpiration().Format(constants.TimeDateLayout)), + constants.KubernetesOpaqueSecretRootPrivateKeyKey: cert.GetPrivateKey(), + } + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: ns, + }, + Data: secretData, + } + + if _, err := kubeClient.CoreV1().Secrets(ns).Create(context.TODO(), secret, metav1.CreateOptions{}); err == nil { + log.Info().Msg("CA created in kubernetes") + } else if apierrors.IsAlreadyExists(err) { + log.Info().Msg("CA already exists in kubernetes, loading.") + } else { + log.Error().Err(err).Msgf("Error creating/retrieving root certificate from secret %s/%s", ns, secretName) + return nil, err + } + + // For simplicity, we will load the certificate for all of them, this way the intance which created it + // and the ones that didn't share the same code. + cert, err := GetCertFromKubernetes(ns, secretName, kubeClient) + if err != nil { + log.Error().Err(err).Msg("Failed to fetch certificate from Kubernetes") + return nil, err + } + + return cert, nil +} + // getTresorOSMCertificateManager returns a certificate manager instance with Tresor as the certificate provider func (c *Config) getTresorOSMCertificateManager() (certificate.Manager, debugger.CertificateManagerDebugger, error) { var err error @@ -178,38 +217,11 @@ func (c *Config) getTresorOSMCertificateManager() (certificate.Manager, debugger return nil, nil, errors.Errorf("Root cert does not have a private key") } - // Attempt to create it in K8s. When creating, only one of the creates will succeed. All others will get - secretData := map[string][]byte{ - constants.KubernetesOpaqueSecretCAKey: rootCert.GetCertificateChain(), - constants.KubernetesOpaqueSecretCAExpiration: []byte(rootCert.GetExpiration().Format(constants.TimeDateLayout)), - constants.KubernetesOpaqueSecretRootPrivateKeyKey: rootCert.GetPrivateKey(), - } - - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: c.caBundleSecretName, - Namespace: c.providerNamespace, - }, - Data: secretData, - } - - _, err = c.kubeClient.CoreV1().Secrets(c.providerNamespace).Create(context.TODO(), secret, metav1.CreateOptions{}) - - if err == nil { - log.Info().Msgf("CA created in kubernetes") - } else if apierrors.IsAlreadyExists(err) { - log.Info().Msgf("CA already exists in kubernetes, loading.") - } else { - log.Error().Err(err).Msgf("Error creating/retrieving root certificate from secret %s/%s", c.providerNamespace, c.caBundleSecretName) - return nil, nil, err - } - - // For simplicity, we will load the CA for all of them, this way the intance which created it - // and the ones that didn't share the same code. - rootCert, err = c.GetCertFromKubernetes() + rootCert, err = GetCertificateFromSecret(c.providerNamespace, c.caBundleSecretName, rootCert, c.kubeClient) if err != nil { - return nil, nil, errors.Errorf("Failed to load CA from kubernetes") + return nil, nil, errors.Errorf("Failed to synchronize certificate on Secrets API : %v", err) } + certManager, err := tresor.NewCertManager(rootCert, rootCertOrganization, c.cfg) if err != nil { return nil, nil, errors.Errorf("Failed to instantiate Tresor as a Certificate Manager") @@ -218,47 +230,46 @@ func (c *Config) getTresorOSMCertificateManager() (certificate.Manager, debugger return certManager, certManager, nil } -// GetCertFromKubernetes returns a Certificater type corresponding to the root certificate. +// GetCertFromKubernetes is a helper function that loads a certificate from a Kubernetes secret // The function returns an error only if a secret is found with invalid data. -func (c *Config) GetCertFromKubernetes() (certificate.Certificater, error) { - rootCertSecret, err := c.kubeClient.CoreV1().Secrets(c.providerNamespace).Get(context.Background(), c.caBundleSecretName, metav1.GetOptions{}) +func GetCertFromKubernetes(ns string, secretName string, kubeClient kubernetes.Interface) (certificate.Certificater, error) { + certSecret, err := kubeClient.CoreV1().Secrets(ns).Get(context.Background(), secretName, metav1.GetOptions{}) if err != nil { - // It is okay for this secret to be missing, in which case a new CA will be created along with a k8s secret - log.Debug().Msgf("Could not retrieve root certificate secret %q from namespace %q", c.caBundleSecretName, c.providerNamespace) - return nil, nil + log.Debug().Msgf("Could not retrieve certificate secret %q from namespace %q", secretName, ns) + return nil, errSecretNotFound } - pemCert, ok := rootCertSecret.Data[constants.KubernetesOpaqueSecretCAKey] + pemCert, ok := certSecret.Data[constants.KubernetesOpaqueSecretCAKey] if !ok { - log.Error().Err(errInvalidCertSecret).Msgf("Opaque k8s secret %s/%s does not have required field %q", c.providerNamespace, c.caBundleSecretName, constants.KubernetesOpaqueSecretCAKey) + log.Error().Err(errInvalidCertSecret).Msgf("Opaque k8s secret %s/%s does not have required field %q", ns, secretName, constants.KubernetesOpaqueSecretCAKey) return nil, errInvalidCertSecret } - pemKey, ok := rootCertSecret.Data[constants.KubernetesOpaqueSecretRootPrivateKeyKey] + pemKey, ok := certSecret.Data[constants.KubernetesOpaqueSecretRootPrivateKeyKey] if !ok { - log.Error().Err(errInvalidCertSecret).Msgf("Opaque k8s secret %s/%s does not have required field %q", c.providerNamespace, c.caBundleSecretName, constants.KubernetesOpaqueSecretRootPrivateKeyKey) + log.Error().Err(errInvalidCertSecret).Msgf("Opaque k8s secret %s/%s does not have required field %q", ns, secretName, constants.KubernetesOpaqueSecretRootPrivateKeyKey) return nil, errInvalidCertSecret } - expirationBytes, ok := rootCertSecret.Data[constants.KubernetesOpaqueSecretCAExpiration] + expirationBytes, ok := certSecret.Data[constants.KubernetesOpaqueSecretCAExpiration] if !ok { - log.Error().Err(errInvalidCertSecret).Msgf("Opaque k8s secret %s/%s does not have required field %q", c.providerNamespace, c.caBundleSecretName, constants.KubernetesOpaqueSecretCAExpiration) + log.Error().Err(errInvalidCertSecret).Msgf("Opaque k8s secret %s/%s does not have required field %q", ns, secretName, constants.KubernetesOpaqueSecretCAExpiration) return nil, errInvalidCertSecret } expiration, err := time.Parse(constants.TimeDateLayout, string(expirationBytes)) if err != nil { - log.Error().Err(err).Msgf("Error parsing CA expiration %q from Kubernetes rootCertSecret %q from namespace %q", string(expirationBytes), c.caBundleSecretName, c.providerNamespace) + log.Error().Err(err).Msgf("Error parsing cert expiration %q from Kubernetes rootCertSecret %q from namespace %q", string(expirationBytes), secretName, ns) return nil, err } - rootCert, err := tresor.NewCertificateFromPEM(pemCert, pemKey, expiration) + cert, err := tresor.NewCertificateFromPEM(pemCert, pemKey, expiration) if err != nil { - log.Error().Err(err).Msgf("Failed to create new Certificate Authority with cert issuer %s", c.providerKind) + log.Error().Err(err).Msgf("Failed to create new Certificate from PEM") return nil, err } - return rootCert, nil + return cert, nil } // getHashiVaultOSMCertificateManager returns a certificate manager instance with Hashi Vault as the certificate provider diff --git a/pkg/certificate/providers/config_test.go b/pkg/certificate/providers/config_test.go index ca6edd13d6..75a1089c56 100644 --- a/pkg/certificate/providers/config_test.go +++ b/pkg/certificate/providers/config_test.go @@ -3,14 +3,23 @@ package providers import ( "context" "fmt" + "sync" "testing" + "time" "github.com/golang/mock/gomock" + "github.com/google/uuid" tassert "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" + "github.com/openservicemesh/osm/pkg/certificate" + "github.com/openservicemesh/osm/pkg/certificate/providers/tresor" "github.com/openservicemesh/osm/pkg/configurator" + "github.com/openservicemesh/osm/pkg/constants" + "github.com/openservicemesh/osm/pkg/tests" ) func TestGetCertificateManager(t *testing.T) { @@ -55,3 +64,150 @@ func TestGetCertificateManager(t *testing.T) { }) } } + +func TestSynchronizeCertificate(t *testing.T) { + assert := tassert.New(t) + kubeClient := fake.NewSimpleClientset() + + // Create some cert, using tresor's api for simplicity + cert, err := tresor.NewCA("common-name", time.Hour, "test-country", "test-locality", "test-org") + assert.NoError(err) + + wg := sync.WaitGroup{} + wg.Add(10) + certResults := make([]certificate.Certificater, 10) + + // Test synchronization, expect all routines end up with the same cert + for i := 0; i < 10; i++ { + go func(num int) { + defer wg.Done() + + resCert, err := GetCertificateFromSecret("test", "test", cert, kubeClient) + assert.NoError(err) + + certResults[num] = resCert + }(i) + } + wg.Wait() + + // Verifiy all of them loaded the exact same cert + for i := 0; i < 9; i++ { + assert.Equal(certResults[i], certResults[i+1]) + } +} + +func TestGetCertificateFromKubernetes(t *testing.T) { + assert := tassert.New(t) + + certPEM, err := tests.GetPEMCert() + assert.NoError(err) + keyPEM, err := tests.GetPEMPrivateKey() + assert.NoError(err) + + ns := uuid.New().String() + secretName := uuid.New().String() + + testCases := []struct { + secret *corev1.Secret + expectError bool + expectNilVal bool + }{ + { + // Valid cert, valid test + secret: &corev1.Secret{ + ObjectMeta: v1.ObjectMeta{ + Name: secretName, + Namespace: ns, + }, + Data: map[string][]byte{ + constants.KubernetesOpaqueSecretCAKey: certPEM, + constants.KubernetesOpaqueSecretCAExpiration: []byte("2020-05-07T14:25:18.677Z"), + constants.KubernetesOpaqueSecretRootPrivateKeyKey: keyPEM, + }, + }, + expectError: false, + expectNilVal: false, + }, + { + // Error when cert fetch is not present + secret: nil, + expectError: true, + expectNilVal: true, + }, + { + // Error when CA key is missing + secret: &corev1.Secret{ + ObjectMeta: v1.ObjectMeta{ + Name: secretName, + Namespace: ns, + }, + Data: map[string][]byte{ + constants.KubernetesOpaqueSecretCAExpiration: []byte("2020-05-07T14:25:18.677Z"), + constants.KubernetesOpaqueSecretRootPrivateKeyKey: keyPEM, + }, + }, + expectError: true, + expectNilVal: true, + }, + { + // Error when Private Key is missing + secret: &corev1.Secret{ + ObjectMeta: v1.ObjectMeta{ + Name: secretName, + Namespace: ns, + }, + Data: map[string][]byte{ + constants.KubernetesOpaqueSecretCAKey: certPEM, + constants.KubernetesOpaqueSecretCAExpiration: []byte("2020-05-07T14:25:18.677Z"), + }, + }, + expectError: true, + expectNilVal: true, + }, + { + // Error when Expiration is missing + secret: &corev1.Secret{ + ObjectMeta: v1.ObjectMeta{ + Name: secretName, + Namespace: ns, + }, + Data: map[string][]byte{ + constants.KubernetesOpaqueSecretCAKey: certPEM, + constants.KubernetesOpaqueSecretRootPrivateKeyKey: keyPEM, + }, + }, + expectError: true, + expectNilVal: true, + }, + { + // Error when Parsing expiration date + secret: &corev1.Secret{ + ObjectMeta: v1.ObjectMeta{ + Name: secretName, + Namespace: ns, + }, + Data: map[string][]byte{ + constants.KubernetesOpaqueSecretCAKey: certPEM, + constants.KubernetesOpaqueSecretCAExpiration: []byte("Invalid expiration date"), + constants.KubernetesOpaqueSecretRootPrivateKeyKey: keyPEM, + }, + }, + expectError: true, + expectNilVal: true, + }, + } + + for _, testElement := range testCases { + kubeClient := fake.NewSimpleClientset() + + if testElement.secret != nil { + _, err = kubeClient.CoreV1().Secrets(ns).Create(context.Background(), testElement.secret, v1.CreateOptions{}) + assert.NoError(err) + } + + cert, err := GetCertFromKubernetes(ns, secretName, kubeClient) + + assert.Equal(testElement.expectError, err != nil) + assert.Equal(testElement.expectNilVal, cert == nil) + } +} diff --git a/pkg/certificate/providers/errors.go b/pkg/certificate/providers/errors.go index 82eea2a464..7e5d41d251 100644 --- a/pkg/certificate/providers/errors.go +++ b/pkg/certificate/providers/errors.go @@ -6,4 +6,5 @@ import ( var ( errInvalidCertSecret = errors.New("Invalid secret for certificate") + errSecretNotFound = errors.Errorf("Secret not found") )