Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OCPBUGS-38335: certrotation: avoid using applySecret to catch racy cert updates #1772

Merged
merged 2 commits into from
Aug 16, 2024
Merged

OCPBUGS-38335: certrotation: avoid using applySecret to catch racy cert updates #1772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a042827
resourcehelper: expose ReportCreateEvent/ReportUpdateEvent
vrutkovs Aug 14, 2024
3a8d8c5
certrotation: avoid using applySecret to catch racy cert updates
vrutkovs Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions pkg/operator/certrotation/cabundle.go
View file
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and we could modify line 70 to:

updateRequired = needsOwnerUpdate || needsMetadataUpdate

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
Open in desktop
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
35 changes: 23 additions & 12 deletions pkg/operator/certrotation/signer.go
View file
Open in desktop
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
// run Update if metadata needs changing
needsMetadataUpdate := ensureMetadataUpdate(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations)
needsTypeChange := ensureSecretTLSTypeSet(signingCertKeyPairSecret)
modified = needsMetadataUpdate || needsTypeChange || modified
updateRequired = needsMetadataUpdate || needsTypeChange

// run Update if signer content needs changing
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need this var ?

I think it could be replaced by creationRequired || updateRequired, right ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. updateRequired means "any part of the secret has changed", but signerUpdated is specific to certificate part (we need to pass it on to trigger CA bundle / target cert updates)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I think that the following code:

	// at this point, the secret has the correct signer, so we should read that signer to be able to sign
	signingCertKeyPair, err := crypto.GetCAFromBytes(signingCertKeyPairSecret.Data["tls.crt"], signingCertKeyPairSecret.Data["tls.key"])
	if err != nil {
		return nil, signerUpdated, err
	}

	return signingCertKeyPair, signerUpdated, nil

could be replaced be:

	// at this point, the secret has the correct signer, so we should read that signer to be able to sign
	signingCertKeyPair, err := crypto.GetCAFromBytes(signingCertKeyPairSecret.Data["tls.crt"], signingCertKeyPairSecret.Data["tls.key"])
	if err != nil {
		return nil, creationRequired || updateRequired, err
	}

	return signingCertKeyPair, creationRequired || updateRequired, nil

and we could remove signerUpdated, am I right ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, updateRequired || creationRequired is not equivalent to signerUpdated. We set updateRequired = true when annotations change too.

This PR is already growing too large - can we optimize this in a followup?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, do you also want to move the refactor commit to a new PR ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets leave this version as is and experiment if signerUpdated can be removed in a separate PR.

I think we want more specific messages for signer/cabundle/target too, i.e. Updated contents and Updated metadata probably

}

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