This repository has been archived by the owner on Jul 11, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 276
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
ref(certs): refactor k8s root ca secret access (#4657)
Refactors the k8s secret access for root ca's. This is done strictly to make subsequent PR's smaller, and easier to review. This is essentially just moving code around for the purpose of avoiding cyclic dependencies, with a couple renames, but almost 0 functionality change. The one change is abstracting NewFromPem, to work for both tresor and certmanager to accept a nil privatekey. It is now up to the caller to validate whether the private key is present if it is required. Signed-off-by: Sean Teeling <[email protected]>
- Loading branch information
Showing
17 changed files
with
383 additions
and
385 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| package k8s | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "github.com/rs/zerolog/log" | ||
| "k8s.io/client-go/kubernetes" | ||
|
|
||
| corev1 "k8s.io/api/core/v1" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
|
|
||
| "github.com/openservicemesh/osm/pkg/certificate" | ||
| "github.com/openservicemesh/osm/pkg/constants" | ||
| "github.com/openservicemesh/osm/pkg/errcode" | ||
| "github.com/openservicemesh/osm/pkg/version" | ||
| ) | ||
|
|
||
| // GetCertFromKubernetes is a helper function that loads a certificate from a Kubernetes secret | ||
| func GetCertFromKubernetes(ns string, secretName string, kubeClient kubernetes.Interface) (*certificate.Certificate, error) { | ||
| certSecret, err := kubeClient.CoreV1().Secrets(ns).Get(context.Background(), secretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| // TODO(#3962): metric might not be scraped before process restart resulting from this error | ||
| log.Error().Str(errcode.Kind, errcode.GetErrCodeWithMetric(errcode.ErrFetchingCertSecret)). | ||
| Msgf("Could not retrieve certificate secret %q from namespace %q", secretName, ns) | ||
| return nil, certificate.ErrSecretNotFound | ||
| } | ||
|
|
||
| pemCert, ok := certSecret.Data[constants.KubernetesOpaqueSecretCAKey] | ||
| if !ok { | ||
| // TODO(#3962): metric might not be scraped before process restart resulting from this error | ||
| log.Error().Err(certificate.ErrInvalidCertSecret).Str(errcode.Kind, errcode.GetErrCodeWithMetric(errcode.ErrObtainingCertFromSecret)). | ||
| Msgf("Opaque k8s secret %s/%s does not have required field %q", ns, secretName, constants.KubernetesOpaqueSecretCAKey) | ||
| return nil, certificate.ErrInvalidCertSecret | ||
| } | ||
|
|
||
| pemKey, ok := certSecret.Data[constants.KubernetesOpaqueSecretRootPrivateKeyKey] | ||
| if !ok { | ||
| // TODO(#3962): metric might not be scraped before process restart resulting from this error | ||
| log.Error().Err(certificate.ErrInvalidCertSecret).Str(errcode.Kind, errcode.GetErrCodeWithMetric(errcode.ErrObtainingPrivateKeyFromSecret)). | ||
| Msgf("Opaque k8s secret %s/%s does not have required field %q", ns, secretName, constants.KubernetesOpaqueSecretRootPrivateKeyKey) | ||
| return nil, certificate.ErrInvalidCertSecret | ||
| } | ||
|
|
||
| cert, err := certificate.NewFromPEM(pemCert, pemKey) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("Failed to create new Certificate from PEM") | ||
| return nil, err | ||
| } | ||
|
|
||
| return cert, nil | ||
| } | ||
|
|
||
| // 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.Certificate, kubeClient kubernetes.Interface) (*certificate.Certificate, 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.KubernetesOpaqueSecretRootPrivateKeyKey: cert.GetPrivateKey(), | ||
| } | ||
|
|
||
| secret := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: secretName, | ||
| Namespace: ns, | ||
| Labels: map[string]string{ | ||
| constants.OSMAppNameLabelKey: constants.OSMAppNameLabelValue, | ||
| constants.OSMAppVersionLabelKey: version.Version, | ||
| }, | ||
| }, | ||
| Data: secretData, | ||
| } | ||
|
|
||
| if _, err := kubeClient.CoreV1().Secrets(ns).Create(context.TODO(), secret, metav1.CreateOptions{}); err == nil { | ||
| log.Info().Msgf("Secret %s/%s created in kubernetes", ns, secretName) | ||
| } else if apierrors.IsAlreadyExists(err) { | ||
| log.Info().Msgf("Secret %s/%s already exists in kubernetes, loading.", ns, secretName) | ||
| } else { | ||
| // TODO(#3962): metric might not be scraped before process restart resulting from this error | ||
| log.Error().Err(err).Str(errcode.Kind, errcode.GetErrCodeWithMetric(errcode.ErrCreatingCertSecret)). | ||
| Msgf("Error creating/retrieving certificate secret %s/%s", ns, secretName) | ||
| return nil, err | ||
| } | ||
|
|
||
| // For simplicity, we will load the certificate for all of them, this way the instance 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 | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| package k8s | ||
|
|
||
| import ( | ||
| "context" | ||
| "sync" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/google/uuid" | ||
| tassert "github.com/stretchr/testify/assert" | ||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "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/constants" | ||
| "github.com/openservicemesh/osm/pkg/tests" | ||
| ) | ||
|
|
||
| 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: metav1.ObjectMeta{ | ||
| Name: secretName, | ||
| Namespace: ns, | ||
| }, | ||
| Data: map[string][]byte{ | ||
| constants.KubernetesOpaqueSecretCAKey: certPEM, | ||
| 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: metav1.ObjectMeta{ | ||
| Name: secretName, | ||
| Namespace: ns, | ||
| }, | ||
| Data: map[string][]byte{ | ||
| constants.KubernetesOpaqueSecretRootPrivateKeyKey: keyPEM, | ||
| }, | ||
| }, | ||
| expectError: true, | ||
| expectNilVal: true, | ||
| }, | ||
| { | ||
| // Error when Private Key is missing | ||
| secret: &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: secretName, | ||
| Namespace: ns, | ||
| }, | ||
| Data: map[string][]byte{ | ||
| constants.KubernetesOpaqueSecretCAKey: certPEM, | ||
| }, | ||
| }, | ||
| 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, metav1.CreateOptions{}) | ||
| assert.NoError(err) | ||
| } | ||
|
|
||
| cert, err := GetCertFromKubernetes(ns, secretName, kubeClient) | ||
|
|
||
| assert.Equal(testElement.expectError, err != nil) | ||
| assert.Equal(testElement.expectNilVal, cert == nil) | ||
| } | ||
| } | ||
|
|
||
| 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.Certificate, 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]) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.