-
Notifications
You must be signed in to change notification settings - Fork 23
SECURESIGN-3167 CTLog recovery #1406
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
4e85d48
324a518
beeb631
3599d6f
456b7f3
0520fcd
3e1cde2
018dfb5
e37c908
78a664f
0ab0b7f
e17aa9c
14b3b9c
4453c05
8cb95ce
c021042
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import ( | |
| "fmt" | ||
| "maps" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| rhtasv1alpha1 "github.com/securesign/operator/api/v1alpha1" | ||
| "github.com/securesign/operator/internal/action" | ||
|
|
@@ -17,6 +18,7 @@ import ( | |
| "github.com/securesign/operator/internal/utils/kubernetes/ensure" | ||
| corev1 "k8s.io/api/core/v1" | ||
| "k8s.io/apimachinery/pkg/api/equality" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| "k8s.io/apimachinery/pkg/api/meta" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| labels2 "k8s.io/apimachinery/pkg/labels" | ||
|
|
@@ -51,8 +53,11 @@ func (i serverConfig) CanHandle(_ context.Context, instance *rhtasv1alpha1.CTlog | |
| return true | ||
| case instance.Spec.ServerConfigRef != nil: | ||
| return !equality.Semantic.DeepEqual(instance.Spec.ServerConfigRef, instance.Status.ServerConfigRef) | ||
| case c.ObservedGeneration != instance.Generation: | ||
| return true | ||
| default: | ||
| return instance.Generation != c.ObservedGeneration | ||
| // Always run Handle() to validate the secret: exists and is valid | ||
| return true | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When default is true than some of prev validation are not needed. |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -62,6 +67,29 @@ func (i serverConfig) Handle(ctx context.Context, instance *rhtasv1alpha1.CTlog) | |
| ) | ||
|
|
||
| if instance.Spec.ServerConfigRef != nil { | ||
| // Validate that the custom secret is accessible | ||
| secret, err := kubernetes.GetSecret(i.Client, instance.Namespace, instance.Spec.ServerConfigRef.Name) | ||
| if err != nil { | ||
| return i.Error(ctx, fmt.Errorf("error accessing custom server config secret: %w", err), instance, | ||
| metav1.Condition{ | ||
| Type: ConfigCondition, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: constants.Failure, | ||
| Message: fmt.Sprintf("Error accessing custom server config secret: %s", instance.Spec.ServerConfigRef.Name), | ||
| ObservedGeneration: instance.Generation, | ||
| }) | ||
| } | ||
| if secret.Data == nil || secret.Data[ctlogUtils.ConfigKey] == nil { | ||
| return i.Error(ctx, fmt.Errorf("custom server config secret is invalid"), instance, | ||
| metav1.Condition{ | ||
| Type: ConfigCondition, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: constants.Failure, | ||
| Message: fmt.Sprintf("Custom server config secret is missing '%s' key: %s", ctlogUtils.ConfigKey, instance.Spec.ServerConfigRef.Name), | ||
| ObservedGeneration: instance.Generation, | ||
| }) | ||
| } | ||
|
|
||
| instance.Status.ServerConfigRef = instance.Spec.ServerConfigRef | ||
| i.Recorder.Event(instance, corev1.EventTypeNormal, "CTLogConfigUpdated", "CTLog config updated") | ||
| meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ | ||
|
|
@@ -74,6 +102,7 @@ func (i serverConfig) Handle(ctx context.Context, instance *rhtasv1alpha1.CTlog) | |
| return i.StatusUpdate(ctx, instance) | ||
| } | ||
|
|
||
| // Validate prerequisites and normalize Trillian address before validation | ||
| switch { | ||
| case instance.Status.TreeID == nil: | ||
| return i.Error(ctx, fmt.Errorf("%s: %v", i.Name(), ctlogUtils.ErrTreeNotSpecified), instance) | ||
|
|
@@ -87,6 +116,59 @@ func (i serverConfig) Handle(ctx context.Context, instance *rhtasv1alpha1.CTlog) | |
|
|
||
| trillianUrl := fmt.Sprintf("%s:%d", instance.Spec.Trillian.Address, *instance.Spec.Trillian.Port) | ||
|
|
||
| c := meta.FindStatusCondition(instance.Status.Conditions, ConfigCondition) | ||
| isSpecChange := c != nil && c.ObservedGeneration != instance.Generation | ||
|
|
||
| // Validate existing secret before attempting recreation (only for hot updates, not spec changes) | ||
| if !isSpecChange && instance.Status.ServerConfigRef != nil && instance.Status.ServerConfigRef.Name != "" { | ||
| secret, err := kubernetes.GetSecret(i.Client, instance.Namespace, instance.Status.ServerConfigRef.Name) | ||
|
|
||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| i.Logger.Info("Server config secret is missing, will recreate", | ||
| "secret", instance.Status.ServerConfigRef.Name) | ||
| i.Recorder.Event(instance, corev1.EventTypeWarning, "CTLogConfigMissing", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please provide to all logs and events the name of the secret map. The name is generated.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is on multiple places so please fix it for all of them. |
||
| "Config secret is missing, will recreate") | ||
| } else { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for k8s api error other than not found will be better to return failure reconcilation. The reason is that there could be a lot of different api error like access rejection which creating a new object will not solve the problem and it cause other issues. |
||
| i.Logger.Error(err, "Error accessing server config secret, will attempt to recreate", | ||
| "secret", instance.Status.ServerConfigRef.Name) | ||
| i.Recorder.Event(instance, corev1.EventTypeWarning, "CTLogConfigError", | ||
| "Error accessing config secret, will recreate") | ||
| } | ||
| } else { | ||
| // Secret exists and is accessible - validate it (for hot updates only) | ||
| if !ctlogUtils.IsSecretDataValid(secret.Data, trillianUrl) { | ||
| // Secret has wrong Trillian configuration, will recreate | ||
| i.Logger.Info("Server config secret is invalid, will recreate", | ||
| "secret", secret.Name, | ||
| "reason", "Trillian configuration mismatch") | ||
| i.Recorder.Event(instance, corev1.EventTypeWarning, "CTLogConfigInvalid", | ||
| "Config secret has invalid Trillian configuration, will recreate") | ||
| } else { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why checking list if root certificate is not handled by IsSecretDataValid and checking only size of list is insufficient it will need to compare that exactly correct certificates are used from status.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is possible to solve that by adding annotation which will contains information based on which data it has been generated. We have it for example in Fulcio's For that you can use CRDs generation or compare exactly spec. |
||
| // Check if root certificates match (for hot updates) | ||
| // Count fulcio-* keys in the secret | ||
| actualRootCertCount := 0 | ||
| for key := range secret.Data { | ||
| if strings.HasPrefix(key, "fulcio-") { | ||
| actualRootCertCount++ | ||
| } | ||
| } | ||
|
|
||
| // Compare with expected count from status | ||
| expectedRootCertCount := len(instance.Status.RootCertificates) | ||
| if actualRootCertCount == expectedRootCertCount && expectedRootCertCount > 0 { | ||
| // Everything matches - no need to recreate | ||
| return i.Continue() | ||
| } | ||
| // Root certificates changed - need to recreate for hot update | ||
| i.Logger.Info("Server config secret needs update for root certificate change", | ||
| "secret", secret.Name, | ||
| "expected_certs", expectedRootCertCount, | ||
| "actual_certs", actualRootCertCount) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| configLabels := labels.ForResource(ComponentName, DeploymentName, instance.Name, serverConfigResourceName) | ||
|
|
||
| rootCerts, err := i.handleRootCertificates(instance) | ||
|
|
@@ -151,7 +233,7 @@ func (i serverConfig) Handle(ctx context.Context, instance *rhtasv1alpha1.CTlog) | |
|
|
||
| instance.Status.ServerConfigRef = &rhtasv1alpha1.LocalObjectReference{Name: newConfig.Name} | ||
|
|
||
| i.Recorder.Eventf(instance, corev1.EventTypeNormal, "CTLogConfigCreated", "Secret with ctlog configuration created: %s", newConfig.Name) | ||
| i.Recorder.Event(instance, corev1.EventTypeNormal, "CTLogConfigCreated", "Config secret created successfully") | ||
| meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ | ||
| Type: ConfigCondition, | ||
| Status: metav1.ConditionTrue, | ||
|
|
@@ -186,11 +268,11 @@ func (i serverConfig) cleanup(ctx context.Context, instance *rhtasv1alpha1.CTlog | |
| err = i.Client.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: partialConfig.Name, Namespace: partialConfig.Namespace}}) | ||
| if err != nil { | ||
| i.Logger.Error(err, "unable to delete secret", "namespace", instance.Namespace, "name", partialConfig.Name) | ||
| i.Recorder.Eventf(instance, corev1.EventTypeWarning, "CTLogConfigDeleted", "Unable to delete secret: %s", partialConfig.Name) | ||
| i.Recorder.Event(instance, corev1.EventTypeWarning, "CTLogConfigCleanupFailed", "Unable to delete old config secret") | ||
| continue | ||
| } | ||
| i.Logger.Info("Remove invalid Secret with ctlog configuration", "Name", partialConfig.Name) | ||
| i.Recorder.Eventf(instance, corev1.EventTypeNormal, "CTLogConfigDeleted", "Secret with ctlog configuration deleted: %s", partialConfig.Name) | ||
| i.Recorder.Event(instance, corev1.EventTypeNormal, "CTLogConfigCleanedUp", "Old config secret deleted successfully") | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If custom ServerConfigRef is unchanged, generation changes are ignored and periodic validation doesn't run.