Skip to content

Commit

Permalink
certrotation: avoid using applySecret to catch racy cert updates
Browse files Browse the repository at this point in the history
  • Loading branch information
@vrutkovs
vrutkovs committed Aug 14, 2024
1 parent a042827 commit 3ac47e0
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 331 deletions.
29 changes: 18 additions & 11 deletions pkg/operator/certrotation/cabundle.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"github.com/openshift/library-go/pkg/certs"
"github.com/openshift/library-go/pkg/crypto"
"github.com/openshift/library-go/pkg/operator/events"
"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
"github.com/openshift/library-go/pkg/operator/resource/resourcehelper"
)

// CABundleConfigMap maintains a CA bundle config map, by adding new CA certs coming from RotatedSigningCASecret, and by removing expired old ones.
Expand All @@ -44,7 +44,8 @@ type CABundleConfigMap struct {
func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingCertKeyPair *crypto.CA, signingCertKeyPairLocation string) ([]*x509.Certificate, error) {
// by this point we have current signing cert/key pair. We now need to make sure that the ca-bundle configmap has this cert and
// doesn't have any expired certs
modified := false
updateRequired := false
creationRequired := false

originalCABundleConfigMap, err := c.Lister.ConfigMaps(c.Namespace).Get(c.Name)
if err != nil && !apierrors.IsNotFound(err) {
Expand All @@ -58,23 +59,23 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC
c.Namespace,
c.AdditionalAnnotations,
)}
modified = true
creationRequired = true
}

needsOwnerUpdate := false
if c.Owner != nil {
needsOwnerUpdate = ensureOwnerReference(&caBundleConfigMap.ObjectMeta, c.Owner)
}
needsMetadataUpdate := c.AdditionalAnnotations.EnsureTLSMetadataUpdate(&caBundleConfigMap.ObjectMeta)
modified = needsOwnerUpdate || needsMetadataUpdate || modified
updateRequired = needsOwnerUpdate || needsMetadataUpdate

updatedCerts, err := manageCABundleConfigMap(caBundleConfigMap, signingCertKeyPair.Config.Certs[0])
if err != nil {
return nil, err
}
if originalCABundleConfigMap == nil || originalCABundleConfigMap.Data == nil || !equality.Semantic.DeepEqual(originalCABundleConfigMap.Data, caBundleConfigMap.Data) {
reason := ""
if originalCABundleConfigMap == nil {
if creationRequired {
reason = "configmap doesn't exist"
} else if originalCABundleConfigMap.Data == nil {
reason = "configmap is empty"
Expand All @@ -84,18 +85,24 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC
c.EventRecorder.Eventf("CABundleUpdateRequired", "%q in %q requires a new cert: %s", c.Name, c.Namespace, reason)
LabelAsManagedConfigMap(caBundleConfigMap, CertificateTypeCABundle)

modified = true
updateRequired = true
}

if modified {

actualCABundleConfigMap, updated, err := resourceapply.ApplyConfigMap(ctx, c.Client, c.EventRecorder, caBundleConfigMap)
if creationRequired {
actualCABundleConfigMap, err := c.Client.ConfigMaps(c.Namespace).Create(ctx, caBundleConfigMap, metav1.CreateOptions{})
resourcehelper.ReportCreateEvent(c.EventRecorder, actualCABundleConfigMap, err)
if err != nil {
return nil, err
}
if updated {
klog.V(2).Infof("Updated ca-bundle.crt configmap %s/%s with:\n%s", certs.CertificateBundleToString(updatedCerts), caBundleConfigMap.Namespace, caBundleConfigMap.Name)
klog.V(2).Infof("Created ca-bundle.crt configmap %s/%s with:\n%s", certs.CertificateBundleToString(updatedCerts), caBundleConfigMap.Namespace, caBundleConfigMap.Name)
caBundleConfigMap = actualCABundleConfigMap
} else if updateRequired {
actualCABundleConfigMap, err := c.Client.ConfigMaps(c.Namespace).Update(ctx, caBundleConfigMap, metav1.UpdateOptions{})
resourcehelper.ReportUpdateEvent(c.EventRecorder, actualCABundleConfigMap, err)
if err != nil {
return nil, err
}
klog.V(2).Infof("Updated ca-bundle.crt configmap %s/%s with:\n%s", certs.CertificateBundleToString(updatedCerts), caBundleConfigMap.Namespace, caBundleConfigMap.Name)
caBundleConfigMap = actualCABundleConfigMap
}

Expand Down
40 changes: 18 additions & 22 deletions pkg/operator/certrotation/cabundle_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,14 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
initialConfigMapFn: func() *corev1.ConfigMap { return nil },
verifyActions: func(t *testing.T, client *kubefake.Clientset) {
actions := client.Actions()
if len(actions) != 2 {
if len(actions) != 1 {
t.Fatal(spew.Sdump(actions))
}

if !actions[0].Matches("get", "configmaps") {
if !actions[0].Matches("create", "configmaps") {
t.Error(actions[0])
}
if !actions[1].Matches("create", "configmaps") {
t.Error(actions[1])
}

actual := actions[1].(clienttesting.CreateAction).GetObject().(*corev1.ConfigMap)
actual := actions[0].(clienttesting.CreateAction).GetObject().(*corev1.ConfigMap)
if certType, _ := CertificateTypeFromObject(actual); certType != CertificateTypeCABundle {
t.Errorf("expected certificate type 'ca-bundle', got: %v", certType)
}
Expand All @@ -73,7 +69,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
},
initialConfigMapFn: func() *corev1.ConfigMap {
caBundleConfigMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle"},
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle", ResourceVersion: "10"},
Data: map[string]string{},
}
certs, err := newTestCACertificate(pkix.Name{CommonName: "signer-tests"}, int64(1), metav1.Duration{Duration: time.Hour * 24 * 60}, time.Now)
Expand All @@ -89,15 +85,15 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
},
verifyActions: func(t *testing.T, client *kubefake.Clientset) {
actions := client.Actions()
if len(actions) != 2 {
if len(actions) != 1 {
t.Fatal(spew.Sdump(actions))
}

if !actions[1].Matches("update", "configmaps") {
t.Error(actions[1])
if !actions[0].Matches("update", "configmaps") {
t.Error(actions[0])
}

actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap)
actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap)
if len(actual.Data["ca-bundle.crt"]) == 0 {
t.Error(actual.Data)
}
Expand All @@ -120,7 +116,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
},
initialConfigMapFn: func() *corev1.ConfigMap {
caBundleConfigMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle"},
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle", ResourceVersion: "10"},
Data: map[string]string{},
}
certs, err := newTestCACertificate(pkix.Name{CommonName: "signer-tests"}, int64(1), metav1.Duration{Duration: time.Hour * 24 * 60}, time.Now)
Expand All @@ -136,15 +132,15 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
},
verifyActions: func(t *testing.T, client *kubefake.Clientset) {
actions := client.Actions()
if len(actions) != 2 {
if len(actions) != 1 {
t.Fatal(spew.Sdump(actions))
}

if !actions[1].Matches("update", "configmaps") {
t.Error(actions[1])
if !actions[0].Matches("update", "configmaps") {
t.Error(actions[0])
}

actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap)
actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap)
if len(actual.Data["ca-bundle.crt"]) == 0 {
t.Error(actual.Data)
}
Expand All @@ -167,7 +163,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
},
initialConfigMapFn: func() *corev1.ConfigMap {
caBundleConfigMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle"},
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle", ResourceVersion: "10"},
Data: map[string]string{},
}
certBytes, err := os.ReadFile("./testfiles/tls-expired.crt")
Expand All @@ -187,15 +183,15 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
},
verifyActions: func(t *testing.T, client *kubefake.Clientset) {
actions := client.Actions()
if len(actions) != 2 {
if len(actions) != 1 {
t.Fatal(spew.Sdump(actions))
}

if !actions[1].Matches("update", "configmaps") {
t.Error(actions[1])
if !actions[0].Matches("update", "configmaps") {
t.Error(actions[0])
}

actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap)
actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap)
if len(actual.Data["ca-bundle.crt"]) == 0 {
t.Error(actual.Data)
}
Expand Down
31 changes: 21 additions & 10 deletions pkg/operator/certrotation/signer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ import (

"github.com/openshift/library-go/pkg/crypto"
"github.com/openshift/library-go/pkg/operator/events"
"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
"github.com/openshift/library-go/pkg/operator/resource/resourcehelper"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
corev1informers "k8s.io/client-go/informers/core/v1"
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
corev1listers "k8s.io/client-go/listers/core/v1"
"k8s.io/klog/v2"
)

// RotatedSigningCASecret rotates a self-signed signing CA stored in a secret. It creates a new one when
Expand Down Expand Up @@ -57,12 +58,12 @@ type RotatedSigningCASecret struct {
// EnsureSigningCertKeyPair manages the entire lifecycle of a signer cert as a secret, from creation to continued rotation.
// It always returns the currently used CA pair, a bool indicating whether it was created/updated within this function call and an error.
func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*crypto.CA, bool, error) {
modified := false
creationRequired := false
updateRequired := false
originalSigningCertKeyPairSecret, err := c.Lister.Secrets(c.Namespace).Get(c.Name)
if err != nil && !apierrors.IsNotFound(err) {
return nil, false, err
}
var signerExists = true
signingCertKeyPairSecret := originalSigningCertKeyPairSecret.DeepCopy()
if apierrors.IsNotFound(err) {
// create an empty one
Expand All @@ -74,18 +75,18 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
),
Type: corev1.SecretTypeTLS,
}
signerExists = false
creationRequired = true
}

// apply necessary metadata (possibly via delete+recreate) if secret exists
// this is done before content update to prevent unexpected rollouts
needsMetadataUpdate := ensureMetadataUpdate(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations)
needsTypeChange := ensureSecretTLSTypeSet(signingCertKeyPairSecret)
modified = needsMetadataUpdate || needsTypeChange || modified
updateRequired = needsMetadataUpdate || needsTypeChange

signerUpdated := false
if needed, reason := needNewSigningCertKeyPair(signingCertKeyPairSecret, c.Refresh, c.RefreshOnlyWhenExpired); needed || !signerExists {
if !signerExists {
if needed, reason := needNewSigningCertKeyPair(signingCertKeyPairSecret, c.Refresh, c.RefreshOnlyWhenExpired); needed || creationRequired {
if creationRequired {
reason = "secret doesn't exist"
}
c.EventRecorder.Eventf("SignerUpdateRequired", "%q in %q requires a new signing cert/key pair: %v", c.Name, c.Namespace, reason)
Expand All @@ -95,15 +96,25 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*

LabelAsManagedSecret(signingCertKeyPairSecret, CertificateTypeSigner)

modified = true
updateRequired = true
signerUpdated = true
}

if modified {
actualSigningCertKeyPairSecret, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)
if creationRequired {
actualSigningCertKeyPairSecret, err := c.Client.Secrets(c.Namespace).Create(ctx, signingCertKeyPairSecret, metav1.CreateOptions{})
resourcehelper.ReportCreateEvent(c.EventRecorder, actualSigningCertKeyPairSecret, err)
if err != nil {
return nil, false, err
}
klog.V(2).Infof("Created secret %s/%s", actualSigningCertKeyPairSecret.Namespace, actualSigningCertKeyPairSecret.Name)
signingCertKeyPairSecret = actualSigningCertKeyPairSecret
} else if updateRequired {
actualSigningCertKeyPairSecret, err := c.Client.Secrets(c.Namespace).Update(ctx, signingCertKeyPairSecret, metav1.UpdateOptions{})
resourcehelper.ReportUpdateEvent(c.EventRecorder, actualSigningCertKeyPairSecret, err)
if err != nil {
return nil, false, err
}
klog.V(2).Infof("Updated secret %s/%s", actualSigningCertKeyPairSecret.Namespace, actualSigningCertKeyPairSecret.Name)
signingCertKeyPairSecret = actualSigningCertKeyPairSecret
}

Expand Down
Loading

0 comments on commit 3ac47e0

Please sign in to comment.