diff --git a/docs/SecureErrorHandling.md b/docs/SecureErrorHandling.md new file mode 100644 index 0000000000..cb13b13fce --- /dev/null +++ b/docs/SecureErrorHandling.md @@ -0,0 +1,353 @@ +# Secure Error and Exception Handling + +This document describes the secure error handling practices and utilities available in the Tekton Operator codebase. + +## Overview + +The `pkg/common/secerrors` package provides comprehensive utilities for handling errors securely, preventing sensitive information leakage through error messages, logs, and Kubernetes events. + +## Why Secure Error Handling? + +Error messages can inadvertently expose sensitive information such as: +- Passwords, API keys, and tokens +- Database credentials and connection strings +- Internal system paths and stack traces +- Private keys and certificates +- Configuration details that could aid attackers + +The secure error handling package automatically sanitizes error messages while preserving debugging information for authorized personnel. + +## Core Concepts + +### SecureError Type + +The `SecureError` type wraps errors with two levels of detail: +- **User Message**: Safe, sanitized message suitable for logs and user display +- **Internal Error**: Full error details (for secure debugging contexts only) + +```go +secErr := secerrors.NewSecureError( + secerrors.ErrorCategoryAuthentication, + "authentication failed", + originalError, +) +``` + +### Error Categories + +Errors are categorized for better classification and handling: + +- `ErrorCategoryAuthentication` - Authentication failures +- `ErrorCategoryAuthorization` - Authorization/permission issues +- `ErrorCategoryConfiguration` - Configuration errors +- `ErrorCategoryValidation` - Input validation failures +- `ErrorCategoryInternal` - Internal system errors +- `ErrorCategoryNetwork` - Network-related errors +- `ErrorCategoryStorage` - Storage/persistence errors + +## Usage Examples + +### Basic Error Sanitization + +```go +import "github.com/tektoncd/operator/pkg/common/secerrors" + +// Sanitize any error message +err := errors.New("connection failed: password=secret123") +safeMsg := secerrors.SafeErrorMessage(err) +// Output: "operation failed: an internal error occurred (details redacted for security)" +``` + +### Creating Secure Errors + +```go +// Wrap an error with a safe message +err := someOperation() +if err != nil { + return secerrors.Wrap(err, + secerrors.ErrorCategoryConfiguration, + "configuration validation failed") +} + +// Automatically sanitize the error message +secErr := secerrors.WrapWithSanitization(err, secerrors.ErrorCategoryInternal) +``` + +### Secure Logging + +```go +import ( + "github.com/tektoncd/operator/pkg/common/secerrors" + "knative.dev/pkg/logging" +) + +func reconcile(ctx context.Context) error { + logger := logging.FromContext(ctx) + + err := performSensitiveOperation() + if err != nil { + // Logs safe message at error level, internal details at debug level + secerrors.LogError(logger, err, "operation failed") + return err + } + return nil +} +``` + +### Reconciler Error Handling + +```go +import "github.com/tektoncd/operator/pkg/common/secerrors" + +func (r *Reconciler) ReconcileKind(ctx context.Context, tc *v1alpha1.TektonConfig) error { + handler := secerrors.NewReconcilerErrorHandler(ctx, r.Recorder) + + // Handle general errors + if err := r.doSomething(); err != nil { + return handler.HandleError(err, tc, "configuration update") + } + + // Handle secret-related errors + secret, err := r.kubeClientSet.CoreV1().Secrets(namespace).Get(ctx, secretName, metav1.GetOptions{}) + if err != nil { + return handler.HandleSecretError(err, tc, secretName, "fetch") + } + + // Handle authentication errors + if err := r.authenticate(); err != nil { + return handler.HandleAuthError(err, tc, "authentication") + } + + return nil +} +``` + +### Custom Sanitization + +```go +// Remove a specific secret value from messages +message := secerrors.SanitizeSecret(errorMsg, secretValue) + +// Check if an error contains sensitive information +if secerrors.IsSensitiveError(err) { + // Handle specially +} +``` + +## Automatic Sanitization Patterns + +The package automatically detects and redacts: + +1. **Passwords and Secrets** + - `password=value`, `secret=value`, `pwd:value` + - JSON: `{"password": "value"}` + +2. **API Keys and Tokens** + - `api_key=value`, `token=value`, `apikey:value` + - JWT tokens (eyJ...) + +3. **AWS Access Keys** + - Patterns matching AWS key formats (AKIA...) + +4. **Private Keys** + - PEM-formatted private keys + +5. **Credentials in URLs** + - `https://user:password@host/path` + +6. **Base64 Encoded Secrets** + - Long base64 strings associated with sensitive keywords + +All detected patterns are replaced with `[REDACTED]`. + +## Best Practices + +### DO: + +✅ **Use secure error wrappers in reconcilers** +```go +return secerrors.Wrap(err, category, "safe user message") +``` + +✅ **Use secure logging functions** +```go +secerrors.LogError(logger, err, "operation failed", "resource", resourceName) +``` + +✅ **Check error categories for conditional handling** +```go +if secerrors.IsCategory(err, secerrors.ErrorCategoryAuthentication) { + // Handle auth errors specially +} +``` + +✅ **Use ReconcilerErrorHandler for consistency** +```go +handler := secerrors.NewReconcilerErrorHandler(ctx, recorder) +return handler.HandleError(err, resource, "operation") +``` + +✅ **Log internal details only at debug level** +```go +// The package automatically logs internal errors at debug level +secerrors.LogError(logger, secErr, "failed") +``` + +### DON'T: + +❌ **Don't use fmt.Errorf with %v for sensitive data** +```go +// BAD: May expose password +return fmt.Errorf("auth failed: %v", err) + +// GOOD: Sanitize first +return secerrors.Wrap(err, secerrors.ErrorCategoryAuthentication, "authentication failed") +``` + +❌ **Don't log raw errors that might contain secrets** +```go +// BAD +logger.Errorf("operation failed: %v", err) + +// GOOD +secerrors.LogError(logger, err, "operation failed") +``` + +❌ **Don't include secrets in error messages** +```go +// BAD +return fmt.Errorf("invalid token: %s", token) + +// GOOD +return secerrors.Wrap(err, secerrors.ErrorCategoryValidation, "invalid token format") +``` + +❌ **Don't expose different error messages for user enumeration** +```go +// BAD: Reveals whether user exists +if userNotFound { + return errors.New("user not found") +} +if wrongPassword { + return errors.New("wrong password") +} + +// GOOD: Generic message +return secerrors.ErrAuthenticationFailed +``` + +## Common Secure Errors + +Pre-defined errors for common scenarios: + +```go +secerrors.ErrAuthenticationFailed // Generic auth failure +secerrors.ErrAuthorizationFailed // Permission denied +secerrors.ErrInvalidConfiguration // Config validation failed +secerrors.ErrSecretNotFound // Secret not found +secerrors.ErrInvalidSecret // Secret format invalid +``` + +## Kubernetes Events + +The package integrates with Kubernetes event recording: + +```go +// Events automatically use sanitized messages +handler.HandleError(err, resource, "reconciliation") +// Records: "reconciliation failed: [sanitized message]" + +// Custom secure events +secerrors.SecureEventf(recorder, resource, corev1.EventTypeWarning, + "OperationFailed", "operation failed", err) +``` + +## Testing + +When writing tests, you can verify sanitization: + +```go +func TestErrorSanitization(t *testing.T) { + err := errors.New("failed with password: secret123") + safeMsg := secerrors.SafeErrorMessage(err) + + if strings.Contains(safeMsg, "secret123") { + t.Error("password not sanitized") + } + if !strings.Contains(safeMsg, "[REDACTED]") { + t.Error("expected redaction marker") + } +} +``` + +## Migration Guide + +To migrate existing code: + +### 1. Replace Direct Error Returns + +**Before:** +```go +if err != nil { + logger.Errorf("failed: %v", err) + return err +} +``` + +**After:** +```go +if err != nil { + secerrors.LogError(logger, err, "operation failed") + return secerrors.WrapWithSanitization(err, secerrors.ErrorCategoryInternal) +} +``` + +### 2. Replace fmt.Errorf with Secure Wrappers + +**Before:** +```go +return fmt.Errorf("secret operation failed: %v", err) +``` + +**After:** +```go +return secerrors.Wrap(err, secerrors.ErrorCategoryConfiguration, "secret operation failed") +``` + +### 3. Use ReconcilerErrorHandler + +**Before:** +```go +if err != nil { + logger.Error("failed", err) + r.Recorder.Event(resource, corev1.EventTypeWarning, "Failed", err.Error()) + return err +} +``` + +**After:** +```go +handler := secerrors.NewReconcilerErrorHandler(ctx, r.Recorder) +if err != nil { + return handler.HandleError(err, resource, "operation") +} +``` + +## Security Considerations + +1. **Debug Logging**: Internal error details are only logged when debug level is enabled +2. **Event Recording**: Kubernetes events always use sanitized messages +3. **Error Wrapping**: Original errors are preserved in the error chain for `errors.Is` and `errors.As` +4. **Performance**: Sanitization uses compiled regex patterns for efficiency + +## References + +- [OWASP Error Handling](https://cheatsheetseries.owasp.org/cheatsheets/Error_Handling_Cheat_Sheet.html) +- [Go Error Handling Best Practices](https://go.dev/blog/error-handling-and-go) +- [CWE-209: Information Exposure Through an Error Message](https://cwe.mitre.org/data/definitions/209.html) + +## Related Jira + +- [SRVKP-4185](https://issues.redhat.com/browse/SRVKP-4185) - Threat Model Countermeasures +- [T159](https://redhat.sdelements.com) - Follow best practices for secure error and exception handling + diff --git a/docs/SecureErrorHandlingExamples.md b/docs/SecureErrorHandlingExamples.md new file mode 100644 index 0000000000..20f363b444 --- /dev/null +++ b/docs/SecureErrorHandlingExamples.md @@ -0,0 +1,389 @@ +# Secure Error Handling - Code Examples + +This document provides concrete examples of implementing secure error handling in Tekton Operator reconcilers. + +## Example 1: TektonHub Database Secret Handling + +### Before (Insecure) + +```go +func (r *Reconciler) ensureDatabaseSecret(ctx context.Context, th *v1alpha1.TektonHub) error { + logger := logging.FromContext(ctx) + + secret, err := r.kubeClientSet.CoreV1().Secrets(th.Spec.TargetNamespace).Get( + ctx, databaseSecretName, metav1.GetOptions{}) + if err != nil { + logger.Error("failed to get database secret:", err) // May expose secret details + return fmt.Errorf("secret operation failed: %v", err) // May leak info + } + + // Validate secret has all required keys + for _, key := range dbKeys { + if _, ok := secret.Data[key]; !ok { + return fmt.Errorf("secret %s missing key: %s", databaseSecretName, key) // Exposes secret structure + } + } + + return nil +} +``` + +### After (Secure) + +```go +func (r *Reconciler) ensureDatabaseSecret(ctx context.Context, th *v1alpha1.TektonHub) error { + handler := secerrors.NewReconcilerErrorHandler(ctx, r.Recorder) + + secret, err := r.kubeClientSet.CoreV1().Secrets(th.Spec.TargetNamespace).Get( + ctx, databaseSecretName, metav1.GetOptions{}) + if err != nil { + return handler.HandleSecretError(err, th, databaseSecretName, "fetch") + } + + // Validate secret has all required keys + for _, key := range dbKeys { + if _, ok := secret.Data[key]; !ok { + err := fmt.Errorf("missing required key: %s", key) + return handler.HandleValidationError(err, th, "database secret") + } + } + + return nil +} +``` + +**Benefits:** +- Secret details not exposed in logs +- Consistent error messages and events +- Internal error preserved for debugging +- Kubernetes events automatically sanitized + +## Example 2: Cosign Key Generation + +### Before (Insecure) + +```go +func generateSigningSecrets(ctx context.Context) map[string][]byte { + logger := logging.FromContext(ctx) + + randomPassword, err := generateRandomPassword(ctx) + if err != nil { + logger.Error("Error generating random password %w:", err) // Generic but inconsistent + return nil + } + + passFunc := func(confirm bool) ([]byte, error) { + return []byte(randomPassword), nil + } + + keys, err := cosign.GenerateKeyPair(passFunc) + if err != nil { + logger.Error("Error generating cosign key pair:", err) // May expose crypto details + return nil + } + + return map[string][]byte{ + "cosign.key": keys.PrivateBytes, // Returning private key in map (OK in this context) + "cosign.pub": keys.PublicBytes, + "cosign.password": []byte(randomPassword), + } +} +``` + +### After (Secure) + +```go +func generateSigningSecrets(ctx context.Context) (map[string][]byte, error) { + logger := logging.FromContext(ctx) + + randomPassword, err := generateRandomPassword(ctx) + if err != nil { + secErr := secerrors.Wrap(err, secerrors.ErrorCategoryInternal, + "failed to generate secure password") + secerrors.LogError(logger, secErr, "password generation failed") + return nil, secErr + } + + passFunc := func(confirm bool) ([]byte, error) { + return []byte(randomPassword), nil + } + + keys, err := cosign.GenerateKeyPair(passFunc) + if err != nil { + secErr := secerrors.Wrap(err, secerrors.ErrorCategoryInternal, + "failed to generate signing key pair") + secerrors.LogError(logger, secErr, "key pair generation failed") + return nil, secErr + } + + return map[string][]byte{ + "cosign.key": keys.PrivateBytes, + "cosign.pub": keys.PublicBytes, + "cosign.password": []byte(randomPassword), + }, nil +} +``` + +**Benefits:** +- Cryptographic errors don't expose implementation details +- Consistent error categorization +- Returns error for proper handling by caller +- Secure logging with debug-level internal details + +## Example 3: Webhook Mutation Error + +### Before (Insecure) + +```go +func (ac *reconciler) Admit(ctx context.Context, request *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse { + logger := logging.FromContext(ctx) + + patchBytes, err := ac.mutate(ctx, request) + if err != nil { + return webhook.MakeErrorStatus("mutation failed: %v", err) // Exposes internal error + } + logger.Infof("Kind: %q PatchBytes: %v", request.Kind, string(patchBytes)) // May log sensitive data + + return &admissionv1.AdmissionResponse{ + Patch: patchBytes, + Allowed: true, + PatchType: func() *admissionv1.PatchType { + pt := admissionv1.PatchTypeJSONPatch + return &pt + }(), + } +} +``` + +### After (Secure) + +```go +func (ac *reconciler) Admit(ctx context.Context, request *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse { + logger := logging.FromContext(ctx) + + patchBytes, err := ac.mutate(ctx, request) + if err != nil { + safeMsg := secerrors.SafeErrorMessage(err) + secerrors.LogError(logger, err, "mutation failed", + "kind", request.Kind.Kind, + "namespace", request.Namespace) + return webhook.MakeErrorStatus("mutation failed: %s", safeMsg) + } + + // Only log patch size, not content (which might contain secrets) + logger.Infof("Kind: %q PatchSize: %d bytes", request.Kind, len(patchBytes)) + + return &admissionv1.AdmissionResponse{ + Patch: patchBytes, + Allowed: true, + PatchType: func() *admissionv1.PatchType { + pt := admissionv1.PatchTypeJSONPatch + return &pt + }(), + } +} +``` + +**Benefits:** +- Webhook errors sanitized before returning to client +- Patch content not logged (may contain secrets/sensitive data) +- Structured logging with safe context + +## Example 4: Database Connection Error + +### Before (Insecure) + +```go +func (r *Reconciler) connectDatabase(props v1alpha1.ResultsAPIProperties) error { + connStr := fmt.Sprintf("postgresql://%s:%s@%s:%s/%s", + props.DBUser, props.DBPassword, props.DBHost, props.DBPort, props.DBName) + + db, err := sql.Open("postgres", connStr) + if err != nil { + return fmt.Errorf("failed to connect to database: %v", err) // May include connection string with password + } + + if err := db.Ping(); err != nil { + return fmt.Errorf("database ping failed: %v", err) // May expose DB details + } + + return nil +} +``` + +### After (Secure) + +```go +func (r *Reconciler) connectDatabase(props v1alpha1.ResultsAPIProperties) error { + // Build connection string (never log this) + connStr := fmt.Sprintf("postgresql://%s:%s@%s:%s/%s", + props.DBUser, props.DBPassword, props.DBHost, props.DBPort, props.DBName) + + db, err := sql.Open("postgres", connStr) + if err != nil { + // Don't include the original error (contains connection string) + return secerrors.NewSecureError( + secerrors.ErrorCategoryStorage, + "failed to connect to database", + err, // Preserved internally for debugging + ) + } + + if err := db.Ping(); err != nil { + return secerrors.Wrap(err, secerrors.ErrorCategoryNetwork, + "database health check failed") + } + + return nil +} +``` + +**Benefits:** +- Connection strings with passwords never exposed +- Database internal errors sanitized +- Original error preserved for secure debugging +- Clear error categorization + +## Example 5: Configuration Validation + +### Before (Insecure) + +```go +func validateConfig(config *Config) error { + if config.APIToken == "" { + return errors.New("API token is required") // OK, doesn't expose token + } + + if !strings.HasPrefix(config.APIToken, "tkn_") { + return fmt.Errorf("invalid API token format: %s", config.APIToken) // EXPOSES TOKEN! + } + + if config.SecretKey == "" { + return errors.New("secret key is required") + } + + return nil +} +``` + +### After (Secure) + +```go +func validateConfig(config *Config) error { + if config.APIToken == "" { + return secerrors.NewSecureError( + secerrors.ErrorCategoryValidation, + "API token is required", + errors.New("missing API token"), + ) + } + + if !strings.HasPrefix(config.APIToken, "tkn_") { + // Don't include the actual token in error message + return secerrors.NewSecureError( + secerrors.ErrorCategoryValidation, + "invalid API token format (expected prefix: tkn_)", + fmt.Errorf("invalid token prefix"), // No actual token in internal error either + ) + } + + if config.SecretKey == "" { + return secerrors.NewSecureError( + secerrors.ErrorCategoryValidation, + "secret key is required", + errors.New("missing secret key"), + ) + } + + return nil +} +``` + +**Benefits:** +- Never exposes actual token/key values +- Provides helpful validation messages without revealing secrets +- Consistent error categorization + +## Example 6: Error in Status Conditions + +### Before (Insecure) + +```go +func (r *Reconciler) updateStatus(ctx context.Context, tr *v1alpha1.TektonResult) error { + err := r.performOperation() + if err != nil { + tr.Status.MarkInstallFailed(fmt.Sprintf("installation failed: %v", err)) // May expose secrets + } + return err +} +``` + +### After (Secure) + +```go +func (r *Reconciler) updateStatus(ctx context.Context, tr *v1alpha1.TektonResult) error { + err := r.performOperation() + if err != nil { + // Use sanitized message in status condition + safeMsg := secerrors.GetSanitizedConditionMessage(err, "installation failed") + tr.Status.MarkInstallFailed(safeMsg) + } + return err +} +``` + +**Benefits:** +- Status conditions visible to users don't expose secrets +- Error details still available for debugging via logs +- Consistent user-facing messages + +## Pattern Summary + +| Scenario | Use This Function | Example | +|----------|-------------------|---------| +| General reconciler errors | `ReconcilerErrorHandler.HandleError` | Operation failures | +| Secret operations | `ReconcilerErrorHandler.HandleSecretError` | Secret fetch/validation | +| Authentication | `ReconcilerErrorHandler.HandleAuthError` | Login failures | +| Validation | `ReconcilerErrorHandler.HandleValidationError` | Input validation | +| Configuration | `ReconcilerErrorHandler.HandleConfigError` | Config validation | +| Logging errors | `secerrors.LogError` | Any error logging | +| Status conditions | `secerrors.GetSanitizedConditionMessage` | Setting resource status | +| Quick sanitization | `secerrors.SafeErrorMessage` | Ad-hoc sanitization | +| Wrapping errors | `secerrors.Wrap` or `secerrors.WrapWithSanitization` | Error wrapping | + +## Testing Secure Errors + +```go +func TestSecureErrorHandling(t *testing.T) { + // Test that sensitive data is redacted + err := errors.New("auth failed with password: secret123") + handler := secerrors.NewReconcilerErrorHandler(context.Background(), nil) + + secErr := handler.HandleError(err, nil, "authentication") + + // Verify sanitization + if strings.Contains(secErr.Error(), "secret123") { + t.Error("password not sanitized in error message") + } + + // Verify error is categorized + if !secerrors.IsCategory(secErr, secerrors.ErrorCategoryInternal) { + t.Error("error not properly categorized") + } +} +``` + +## Checklist for Code Reviews + +When reviewing code, ensure: + +- [ ] No `fmt.Errorf` with `%v` that could expose secrets +- [ ] All errors from secret operations are sanitized +- [ ] Database connection strings are never logged +- [ ] Authentication errors don't differentiate between "user not found" and "wrong password" +- [ ] Validation errors don't include the actual invalid value if it could be sensitive +- [ ] Status conditions use sanitized messages +- [ ] Webhook errors are sanitized before returning to clients +- [ ] All error logging uses `secerrors.LogError` or equivalent +- [ ] Test cases verify sanitization works correctly + diff --git a/docs/SecureErrorHandling_ReconcilerUpdateExample.md b/docs/SecureErrorHandling_ReconcilerUpdateExample.md new file mode 100644 index 0000000000..f00bcac297 --- /dev/null +++ b/docs/SecureErrorHandling_ReconcilerUpdateExample.md @@ -0,0 +1,373 @@ +# Practical Example: Updating TektonHub Reconciler with Secure Error Handling + +This document shows a complete before/after example of updating the TektonHub reconciler to use secure error handling. + +## File: `pkg/reconciler/kubernetes/tektonhub/tektonhub.go` + +### Changes Required + +#### 1. Add Import + +```go +import ( + // ... existing imports ... + "github.com/tektoncd/operator/pkg/common/secerrors" +) +``` + +#### 2. Update FinalizeKind Method + +**Before:** +```go +func (r *Reconciler) FinalizeKind(ctx context.Context, original *v1alpha1.TektonHub) pkgreconciler.Event { + logger := logging.FromContext(ctx) + + labelSelector, err := common.LabelSelector(ls) + if err != nil { + return err + } + if err := r.operatorClientSet.OperatorV1alpha1().TektonInstallerSets(). + DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{ + LabelSelector: labelSelector, + }); err != nil { + logger.Error("Failed to delete installer set created by TektonHub", err) + return err + } + + if err := r.extension.Finalize(ctx, original); err != nil { + logger.Error("Failed to finalize platform resources", err) + } + return nil +} +``` + +**After:** +```go +func (r *Reconciler) FinalizeKind(ctx context.Context, original *v1alpha1.TektonHub) pkgreconciler.Event { + handler := secerrors.NewReconcilerErrorHandler(ctx, r.Recorder) + + labelSelector, err := common.LabelSelector(ls) + if err != nil { + return handler.HandleError(err, original, "label selector creation") + } + + if err := r.operatorClientSet.OperatorV1alpha1().TektonInstallerSets(). + DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{ + LabelSelector: labelSelector, + }); err != nil { + return handler.HandleError(err, original, "installer set cleanup") + } + + if err := r.extension.Finalize(ctx, original); err != nil { + return handler.HandleError(err, original, "platform resource finalization") + } + return nil +} +``` + +#### 3. Update ReconcileKind for Database Secret Handling + +**Before:** +```go +func (r *Reconciler) ensureDatabaseSecret(ctx context.Context, th *v1alpha1.TektonHub) error { + logger := logging.FromContext(ctx) + + dbSpec := th.Spec.Db + if dbSpec.DbSecretName != "" { + // external DB + secret, err := r.kubeClientSet.CoreV1().Secrets(th.Spec.GetTargetNamespace()).Get(ctx, dbSpec.DbSecretName, metav1.GetOptions{}) + if err != nil { + logger.Error("error getting db secret: ", err) + return err + } + + for _, key := range dbKeys { + if _, ok := secret.Data[key]; !ok { + return errKeyMissing + } + } + return nil + } + + // internal DB - create secret + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: databaseSecretName, + Namespace: th.Spec.GetTargetNamespace(), + }, + Type: corev1.SecretTypeOpaque, + StringData: map[string]string{ + secretKeyPostgresHost: defaultPostgresHost, + secretKeyPostgresDB: defaultPostgresDB, + secretKeyPostgresUser: defaultPostgresUser, + secretKeyPostgresPassword: defaultPostgresPassword, + secretKeyPostgresPort: defaultPostgresPort, + }, + } + + _, err := r.kubeClientSet.CoreV1().Secrets(th.Spec.GetTargetNamespace()).Create(ctx, secret, metav1.CreateOptions{}) + if err != nil && !apierrors.IsAlreadyExists(err) { + logger.Error("error creating db secret: ", err) + return err + } + + return nil +} +``` + +**After:** +```go +func (r *Reconciler) ensureDatabaseSecret(ctx context.Context, th *v1alpha1.TektonHub) error { + handler := secerrors.NewReconcilerErrorHandler(ctx, r.Recorder) + + dbSpec := th.Spec.Db + if dbSpec.DbSecretName != "" { + // external DB - validate provided secret + secret, err := r.kubeClientSet.CoreV1().Secrets(th.Spec.GetTargetNamespace()).Get( + ctx, dbSpec.DbSecretName, metav1.GetOptions{}) + if err != nil { + return handler.HandleSecretError(err, th, dbSpec.DbSecretName, "fetch") + } + + // Validate all required keys are present + for _, key := range dbKeys { + if _, ok := secret.Data[key]; !ok { + err := fmt.Errorf("missing required key: %s", key) + return handler.HandleValidationError(err, th, "database secret") + } + } + return nil + } + + // internal DB - create default secret + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: databaseSecretName, + Namespace: th.Spec.GetTargetNamespace(), + }, + Type: corev1.SecretTypeOpaque, + StringData: map[string]string{ + secretKeyPostgresHost: defaultPostgresHost, + secretKeyPostgresDB: defaultPostgresDB, + secretKeyPostgresUser: defaultPostgresUser, + secretKeyPostgresPassword: defaultPostgresPassword, + secretKeyPostgresPort: defaultPostgresPort, + }, + } + + _, err := r.kubeClientSet.CoreV1().Secrets(th.Spec.GetTargetNamespace()).Create( + ctx, secret, metav1.CreateOptions{}) + if err != nil && !apierrors.IsAlreadyExists(err) { + return handler.HandleSecretError(err, th, databaseSecretName, "create") + } + + return nil +} +``` + +#### 4. Update Error Handling in ReconcileKind + +**Before:** +```go +func (r *Reconciler) ReconcileKind(ctx context.Context, th *v1alpha1.TektonHub) pkgreconciler.Event { + logger := logging.FromContext(ctx) + th.Status.InitializeConditions() + th.Status.ObservedGeneration = th.Generation + + logger.Infow("Reconciling TektonHub", "status", th.Status) + + // validation + if th.GetName() != v1alpha1.HubResourceName { + msg := fmt.Sprintf("Resource ignored, Expected Name: %s, Got Name: %s", + v1alpha1.HubResourceName, + th.GetName(), + ) + logger.Error(msg) + th.Status.MarkNotReady(msg) + return nil + } + + th.SetDefaults(ctx) + + // reconcile target namespace + if err := common.ReconcileTargetNamespace(ctx, nil, nil, th, r.kubeClientSet); err != nil { + logger.Errorw("error on reconciling targetNamespace", + "targetNamespace", th.Spec.GetTargetNamespace(), + err, + ) + return err + } + + // ensure database secret exists + if err := r.ensureDatabaseSecret(ctx, th); err != nil { + logger.Error("error ensuring database secret: ", err) + th.Status.MarkNotReady("database configuration failed") + return err + } + + // ... rest of reconciliation ... +} +``` + +**After:** +```go +func (r *Reconciler) ReconcileKind(ctx context.Context, th *v1alpha1.TektonHub) pkgreconciler.Event { + logger := logging.FromContext(ctx) + handler := secerrors.NewReconcilerErrorHandler(ctx, r.Recorder) + + th.Status.InitializeConditions() + th.Status.ObservedGeneration = th.Generation + + logger.Infow("Reconciling TektonHub", "status", th.Status) + + // validation + if th.GetName() != v1alpha1.HubResourceName { + msg := fmt.Sprintf("Resource ignored, Expected Name: %s, Got Name: %s", + v1alpha1.HubResourceName, + th.GetName(), + ) + logger.Warn(msg) + th.Status.MarkNotReady(msg) + return nil + } + + th.SetDefaults(ctx) + + // reconcile target namespace + if err := common.ReconcileTargetNamespace(ctx, nil, nil, th, r.kubeClientSet); err != nil { + secerrors.LogError(logger, err, "target namespace reconciliation failed", + "targetNamespace", th.Spec.GetTargetNamespace()) + return handler.HandleError(err, th, "target namespace reconciliation") + } + + // ensure database secret exists + if err := r.ensureDatabaseSecret(ctx, th); err != nil { + // Error already handled by ensureDatabaseSecret with proper sanitization + safeMsg := secerrors.GetSanitizedConditionMessage(err, "database configuration failed") + th.Status.MarkNotReady(safeMsg) + return err + } + + // ... rest of reconciliation ... +} +``` + +## Summary of Changes + +### Security Improvements + +1. **Secret Operations**: All secret-related operations now use `HandleSecretError` which: + - Prevents secret names/values from appearing in logs + - Creates generic, safe error messages for users + - Preserves internal details for debugging + +2. **Error Categorization**: Errors are now categorized: + - Configuration errors (secrets, config) + - Validation errors (missing keys, invalid format) + - Internal errors (unexpected failures) + +3. **Consistent Logging**: All error logging uses secure functions: + - `secerrors.LogError()` - Safe message at error level, internal at debug + - `handler.HandleError()` - Records sanitized Kubernetes events + +4. **Status Conditions**: Status messages use sanitized errors: + - `GetSanitizedConditionMessage()` - Removes sensitive data from condition messages + +### Benefits + +- ✅ No database credentials exposed in logs +- ✅ Secret names and values sanitized +- ✅ Consistent error handling across the reconciler +- ✅ Better user experience with clear, safe error messages +- ✅ Debugging information preserved (at debug log level) +- ✅ Kubernetes events don't leak sensitive information + +## Testing the Changes + +### Unit Test Example + +```go +func TestEnsureDatabaseSecret_SecureErrors(t *testing.T) { + ctx := context.Background() + recorder := &fakeRecorder{} + + // Test missing secret + reconciler := &Reconciler{ + kubeClientSet: fakeClient, + Recorder: recorder, + } + + th := &v1alpha1.TektonHub{ + Spec: v1alpha1.TektonHubSpec{ + Db: v1alpha1.DbSpec{ + DbSecretName: "nonexistent-secret", + }, + }, + } + + err := reconciler.ensureDatabaseSecret(ctx, th) + + // Verify error is secure + assert.NotNil(t, err) + assert.NotContains(t, err.Error(), "nonexistent-secret") // Secret name should be sanitized + assert.True(t, secerrors.IsCategory(err, secerrors.ErrorCategoryConfiguration)) + + // Verify event was recorded with safe message + events := recorder.GetEvents() + assert.Len(t, events, 1) + assert.NotContains(t, events[0].Message, "password") + assert.NotContains(t, events[0].Message, "token") +} +``` + +### Manual Testing + +1. **Create TektonHub with missing secret:** +```yaml +apiVersion: operator.tekton.dev/v1alpha1 +kind: TektonHub +metadata: + name: hub +spec: + targetNamespace: tekton-hub + db: + dbSecretName: nonexistent-secret +``` + +2. **Check logs** - should show sanitized messages: +``` +ERROR: secret operation failed for fetch +DEBUG: internal error: secrets "nonexistent-secret" not found +``` + +3. **Check events** - should show safe messages: +```bash +kubectl get events -n tekton-hub +# LAST SEEN TYPE REASON MESSAGE +# 1m Warning SecretOperationFailed secret operation failed for fetch +``` + +4. **Check status conditions** - should not expose secrets: +```bash +kubectl get tektonhub hub -o yaml +# status: +# conditions: +# - message: database configuration failed: secret operation failed +# reason: NotReady +# status: "False" +``` + +## Rollout Strategy + +1. **Phase 1**: Update core reconcilers (TektonHub, TektonResult, TektonChain) that handle secrets +2. **Phase 2**: Update remaining reconcilers for consistency +3. **Phase 3**: Add linter rules to enforce secure error handling +4. **Phase 4**: Update documentation and provide team training + +## Next Steps + +1. Apply these changes to `pkg/reconciler/kubernetes/tektonhub/tektonhub.go` +2. Run tests: `go test ./pkg/reconciler/kubernetes/tektonhub/...` +3. Verify no sensitive data in logs with integration tests +4. Repeat pattern for other reconcilers + diff --git a/pkg/common/secerrors/README.md b/pkg/common/secerrors/README.md new file mode 100644 index 0000000000..1609f7d934 --- /dev/null +++ b/pkg/common/secerrors/README.md @@ -0,0 +1,222 @@ +# Package secerrors + +Package `secerrors` provides secure error handling utilities for the Tekton Operator, preventing sensitive information leakage through error messages, logs, and Kubernetes events. + +## Quick Start + +```go +import "github.com/tektoncd/operator/pkg/common/secerrors" + +func (r *Reconciler) ReconcileKind(ctx context.Context, resource *v1alpha1.TektonHub) error { + handler := secerrors.NewReconcilerErrorHandler(ctx, r.Recorder) + + // Handle errors securely + if err := someOperation(); err != nil { + return handler.HandleError(err, resource, "operation name") + } + + // Handle secret-related errors + secret, err := r.getSecret(ctx, secretName) + if err != nil { + return handler.HandleSecretError(err, resource, secretName, "fetch") + } + + return nil +} +``` + +## Features + +- **Automatic Sanitization**: Detects and redacts passwords, tokens, API keys, private keys, and other sensitive data +- **Error Categories**: Classify errors for better handling (authentication, authorization, configuration, etc.) +- **Secure Logging**: Logs safe messages at error level, internal details at debug level +- **Reconciler Integration**: Purpose-built handlers for Kubernetes reconcilers +- **Event Safety**: Kubernetes events automatically use sanitized messages +- **Error Wrapping**: Preserves error chains for `errors.Is` and `errors.As` compatibility + +## Core Types + +### SecureError + +Wraps errors with sanitized user messages while preserving internal details: + +```go +secErr := secerrors.NewSecureError( + secerrors.ErrorCategoryAuthentication, + "authentication failed", // Safe user message + originalError, // Internal error (for debugging) +) +``` + +### ReconcilerErrorHandler + +Centralized error handling for reconcilers: + +```go +handler := secerrors.NewReconcilerErrorHandler(ctx, recorder) +``` + +## Key Functions + +| Function | Purpose | Use Case | +|----------|---------|----------| +| `NewSecureError` | Create a secure error | Manual error creation | +| `Wrap` | Wrap error with safe message | Error wrapping | +| `WrapWithSanitization` | Auto-sanitize and wrap | Quick sanitization | +| `SafeErrorMessage` | Get safe error message | Ad-hoc sanitization | +| `SanitizeErrorMessage` | Sanitize any string | Custom sanitization | +| `LogError` | Log error securely | Error logging | +| `NewReconcilerErrorHandler` | Create reconciler handler | Reconciler setup | + +## Sanitization Patterns + +Automatically detects and redacts: + +- Passwords: `password=value`, `pwd:value`, `passwd=value` +- API Keys: `api_key=value`, `apikey:value` +- Tokens: `token=value`, JWT tokens, auth tokens +- Secrets: `secret=value`, `secret_key=value` +- Private Keys: PEM-formatted keys +- AWS Keys: AKIA..., ASIA..., etc. +- Credentials in URLs: `user:password@host` +- Base64 secrets: Long base64 strings with sensitive keywords + +All matched patterns are replaced with `[REDACTED]`. + +## Error Categories + +```go +const ( + ErrorCategoryAuthentication // Auth failures + ErrorCategoryAuthorization // Permission issues + ErrorCategoryConfiguration // Config errors + ErrorCategoryValidation // Validation failures + ErrorCategoryInternal // Internal errors + ErrorCategoryNetwork // Network errors + ErrorCategoryStorage // Storage errors +) +``` + +## Examples + +### Basic Error Wrapping + +```go +err := database.Connect() +if err != nil { + return secerrors.Wrap(err, + secerrors.ErrorCategoryStorage, + "database connection failed") +} +``` + +### Automatic Sanitization + +```go +// Error contains: "auth failed: password=secret123" +secErr := secerrors.WrapWithSanitization(err, secerrors.ErrorCategoryAuthentication) +// secErr.Error() returns: "auth failed: [REDACTED]" +``` + +### Secure Logging + +```go +logger := logging.FromContext(ctx) +secerrors.LogError(logger, err, "operation failed", + "resource", resourceName) +// Logs safe message at ERROR level +// Logs internal details at DEBUG level +``` + +### Reconciler Error Handling + +```go +handler := secerrors.NewReconcilerErrorHandler(ctx, r.Recorder) + +// General errors +if err := r.reconcile(); err != nil { + return handler.HandleError(err, resource, "reconciliation") +} + +// Secret errors +if err := r.validateSecret(); err != nil { + return handler.HandleSecretError(err, resource, secretName, "validation") +} + +// Auth errors +if err := r.authenticate(); err != nil { + return handler.HandleAuthError(err, resource, "authentication") +} + +// Validation errors +if err := r.validate(); err != nil { + return handler.HandleValidationError(err, resource, fieldName) +} +``` + +### Status Conditions + +```go +if err != nil { + // Sanitize before adding to status + safeMsg := secerrors.GetSanitizedConditionMessage(err, "operation failed") + resource.Status.MarkFalse(ConditionReady, "OperationFailed", safeMsg) +} +``` + +## Testing + +```go +func TestErrorSanitization(t *testing.T) { + err := errors.New("auth failed: password=secret123") + safeMsg := secerrors.SafeErrorMessage(err) + + assert.NotContains(t, safeMsg, "secret123") + assert.Contains(t, safeMsg, "[REDACTED]") +} +``` + +## Pre-defined Errors + +Common errors available for reuse: + +```go +secerrors.ErrAuthenticationFailed // Generic auth failure +secerrors.ErrAuthorizationFailed // Permission denied +secerrors.ErrInvalidConfiguration // Config validation failed +secerrors.ErrSecretNotFound // Secret not found +secerrors.ErrInvalidSecret // Secret format invalid +``` + +## Best Practices + +✅ **DO:** +- Use `ReconcilerErrorHandler` for consistency +- Log errors with `secerrors.LogError()` +- Wrap errors with appropriate categories +- Use sanitized messages in status conditions +- Check error categories with `IsCategory()` + +❌ **DON'T:** +- Use `fmt.Errorf` with `%v` for sensitive errors +- Log raw errors that might contain secrets +- Include actual secret values in error messages +- Differentiate error messages for user enumeration +- Expose internal implementation details + +## Documentation + +- [Secure Error Handling Guide](../../../docs/SecureErrorHandling.md) +- [Code Examples](../../../docs/SecureErrorHandlingExamples.md) +- [Reconciler Update Example](../../../docs/SecureErrorHandling_ReconcilerUpdateExample.md) + +## Related Standards + +- [OWASP Error Handling](https://cheatsheetseries.owasp.org/cheatsheets/Error_Handling_Cheat_Sheet.html) +- [CWE-209: Information Exposure Through an Error Message](https://cwe.mitre.org/data/definitions/209.html) +- [CWE-532: Information Exposure Through Log Files](https://cwe.mitre.org/data/definitions/532.html) + +## License + +Apache License 2.0 - see [LICENSE](../../../LICENSE) for details. + diff --git a/pkg/common/secerrors/errors.go b/pkg/common/secerrors/errors.go new file mode 100644 index 0000000000..d28b620296 --- /dev/null +++ b/pkg/common/secerrors/errors.go @@ -0,0 +1,267 @@ +/* +Copyright 2024 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package secerrors + +import ( + "errors" + "fmt" + "regexp" + "strings" +) + +// SecureError is an error type that sanitizes sensitive information before exposing error messages +type SecureError struct { + // userMessage is the safe message to show to users/logs + userMessage string + // internalError is the actual error with potentially sensitive info (for debugging only) + internalError error + // category helps classify the error type + category ErrorCategory +} + +// ErrorCategory defines types of errors for better classification +type ErrorCategory string + +const ( + // ErrorCategoryAuthentication for auth-related errors + ErrorCategoryAuthentication ErrorCategory = "authentication" + // ErrorCategoryAuthorization for authz-related errors + ErrorCategoryAuthorization ErrorCategory = "authorization" + // ErrorCategoryConfiguration for config-related errors + ErrorCategoryConfiguration ErrorCategory = "configuration" + // ErrorCategoryValidation for validation errors + ErrorCategoryValidation ErrorCategory = "validation" + // ErrorCategoryInternal for internal errors + ErrorCategoryInternal ErrorCategory = "internal" + // ErrorCategoryNetwork for network-related errors + ErrorCategoryNetwork ErrorCategory = "network" + // ErrorCategoryStorage for storage-related errors + ErrorCategoryStorage ErrorCategory = "storage" +) + +// Error implements the error interface and returns the sanitized user message +func (e *SecureError) Error() string { + if e.category != "" { + return fmt.Sprintf("[%s] %s", e.category, e.userMessage) + } + return e.userMessage +} + +// Unwrap returns the underlying error for error chain compatibility +func (e *SecureError) Unwrap() error { + return e.internalError +} + +// InternalError returns the internal error (should only be used for detailed logging in secure contexts) +func (e *SecureError) InternalError() error { + return e.internalError +} + +// Category returns the error category +func (e *SecureError) Category() ErrorCategory { + return e.category +} + +// NewSecureError creates a new secure error with a safe user message +func NewSecureError(category ErrorCategory, userMessage string, internalErr error) *SecureError { + return &SecureError{ + userMessage: userMessage, + internalError: internalErr, + category: category, + } +} + +// Wrap wraps an existing error and sanitizes it +func Wrap(err error, category ErrorCategory, userMessage string) *SecureError { + if err == nil { + return nil + } + return NewSecureError(category, userMessage, err) +} + +// WrapWithSanitization wraps an error and automatically sanitizes the message +func WrapWithSanitization(err error, category ErrorCategory) *SecureError { + if err == nil { + return nil + } + sanitized := SanitizeErrorMessage(err.Error()) + return NewSecureError(category, sanitized, err) +} + +// sensitivePatterns contains regex patterns for detecting sensitive information +var sensitivePatterns = []*regexp.Regexp{ + // Passwords + regexp.MustCompile(`(?i)(password|passwd|pwd)["\s:=]+([^\s"']+)`), + // API keys and tokens + regexp.MustCompile(`(?i)(api[_-]?key|token|auth[_-]?token)["\s:=]+([^\s"']+)`), + // Secret keys + regexp.MustCompile(`(?i)(secret[_-]?key|secret)["\s:=]+([^\s"']+)`), + // Private keys (PEM format indicators) + regexp.MustCompile(`-----BEGIN [A-Z\s]+PRIVATE KEY-----[\s\S]*?-----END [A-Z\s]+PRIVATE KEY-----`), + // AWS access keys + regexp.MustCompile(`(A3T[A-Z0-9]|AKIA|AGPA|AIDA|AROA|AIPA|ANPA|ANVA|ASIA)[A-Z0-9]{16}`), + // Generic credentials in URLs + regexp.MustCompile(`(?i)([a-z0-9+.-]+:\/\/)([^:@\s]+):([^@\s]+)@`), + // JWT tokens + regexp.MustCompile(`eyJ[a-zA-Z0-9_-]+\.eyJ[a-zA-Z0-9_-]+\.[a-zA-Z0-9_-]+`), + // Base64 encoded strings that might be secrets (at least 32 chars) + regexp.MustCompile(`(?i)(secret|password|token|key)["\s:=]+([A-Za-z0-9+/=]{32,})`), +} + +// SanitizeErrorMessage removes sensitive information from error messages +func SanitizeErrorMessage(message string) string { + sanitized := message + + for _, pattern := range sensitivePatterns { + sanitized = pattern.ReplaceAllString(sanitized, "[REDACTED]") + } + + // Additional context-aware sanitization + sanitized = sanitizeKeyValuePairs(sanitized) + + return sanitized +} + +// sensitiveKeys are field names that typically contain sensitive data +var sensitiveKeys = []string{ + "password", "passwd", "pwd", + "secret", "secretkey", "secret_key", + "token", "authtoken", "auth_token", "apitoken", "api_token", + "key", "apikey", "api_key", "privatekey", "private_key", + "credential", "credentials", + "access_key", "accesskey", + "auth", "authorization", +} + +// sanitizeKeyValuePairs removes sensitive key-value pairs from strings +func sanitizeKeyValuePairs(message string) string { + result := message + + for _, key := range sensitiveKeys { + // Handle various formats: key=value, key:value, "key":"value", etc. + patterns := []string{ + fmt.Sprintf(`(?i)%s["\s]*[:=]["\s]*[^\s,}"']+`, key), + fmt.Sprintf(`(?i)"%s"["\s]*:["\s]*"[^"]*"`, key), + fmt.Sprintf(`(?i)'%s'["\s]*:["\s]*'[^']*'`, key), + } + + for _, pattern := range patterns { + re := regexp.MustCompile(pattern) + result = re.ReplaceAllString(result, fmt.Sprintf(`%s=[REDACTED]`, key)) + } + } + + return result +} + +// SanitizeSecret replaces a known secret value with [REDACTED] in the message +func SanitizeSecret(message, secretValue string) string { + if secretValue == "" { + return message + } + return strings.ReplaceAll(message, secretValue, "[REDACTED]") +} + +// IsSensitiveError checks if an error might contain sensitive information +func IsSensitiveError(err error) bool { + if err == nil { + return false + } + + errMsg := strings.ToLower(err.Error()) + + // Check for sensitive keywords + for _, key := range sensitiveKeys { + if strings.Contains(errMsg, key) { + return true + } + } + + // Check if it matches any sensitive patterns + for _, pattern := range sensitivePatterns { + if pattern.MatchString(err.Error()) { + return true + } + } + + return false +} + +// SafeErrorMessage returns a safe error message for logging/user display +// If the error is sensitive, it returns a generic message +func SafeErrorMessage(err error) string { + if err == nil { + return "" + } + + // Check if it's already a SecureError + var secErr *SecureError + if errors.As(err, &secErr) { + return secErr.Error() + } + + // Check if the error contains sensitive information + if IsSensitiveError(err) { + return "operation failed: an internal error occurred (details redacted for security)" + } + + // Sanitize the error message + return SanitizeErrorMessage(err.Error()) +} + +// Common secure errors for reuse +var ( + ErrAuthenticationFailed = NewSecureError( + ErrorCategoryAuthentication, + "authentication failed", + errors.New("authentication failed"), + ) + + ErrAuthorizationFailed = NewSecureError( + ErrorCategoryAuthorization, + "authorization failed: insufficient permissions", + errors.New("authorization failed"), + ) + + ErrInvalidConfiguration = NewSecureError( + ErrorCategoryConfiguration, + "invalid configuration", + errors.New("configuration validation failed"), + ) + + ErrSecretNotFound = NewSecureError( + ErrorCategoryConfiguration, + "required secret not found", + errors.New("secret not found"), + ) + + ErrInvalidSecret = NewSecureError( + ErrorCategoryValidation, + "secret validation failed", + errors.New("invalid secret format"), + ) +) + +// IsCategory checks if an error belongs to a specific category +func IsCategory(err error, category ErrorCategory) bool { + var secErr *SecureError + if errors.As(err, &secErr) { + return secErr.Category() == category + } + return false +} + diff --git a/pkg/common/secerrors/errors_test.go b/pkg/common/secerrors/errors_test.go new file mode 100644 index 0000000000..b7f0bb1901 --- /dev/null +++ b/pkg/common/secerrors/errors_test.go @@ -0,0 +1,478 @@ +/* +Copyright 2024 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package secerrors + +import ( + "errors" + "strings" + "testing" +) + +func TestSecureError_Error(t *testing.T) { + tests := []struct { + name string + category ErrorCategory + message string + want string + }{ + { + name: "error with category", + category: ErrorCategoryAuthentication, + message: "auth failed", + want: "[authentication] auth failed", + }, + { + name: "error without category", + category: "", + message: "generic error", + want: "generic error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := NewSecureError(tt.category, tt.message, nil) + if got := err.Error(); got != tt.want { + t.Errorf("SecureError.Error() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestSecureError_Unwrap(t *testing.T) { + innerErr := errors.New("inner error") + secErr := NewSecureError(ErrorCategoryInternal, "safe message", innerErr) + + if unwrapped := secErr.Unwrap(); unwrapped != innerErr { + t.Errorf("Unwrap() = %v, want %v", unwrapped, innerErr) + } +} + +func TestWrap(t *testing.T) { + tests := []struct { + name string + err error + category ErrorCategory + message string + wantNil bool + wantMessage string + }{ + { + name: "wrap nil error", + err: nil, + category: ErrorCategoryInternal, + message: "test", + wantNil: true, + wantMessage: "", + }, + { + name: "wrap valid error", + err: errors.New("original error"), + category: ErrorCategoryValidation, + message: "validation failed", + wantNil: false, + wantMessage: "[validation] validation failed", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := Wrap(tt.err, tt.category, tt.message) + if tt.wantNil { + if got != nil { + t.Errorf("Wrap() = %v, want nil", got) + } + return + } + if got == nil { + t.Fatal("Wrap() = nil, want non-nil") + } + if got.Error() != tt.wantMessage { + t.Errorf("Wrap().Error() = %v, want %v", got.Error(), tt.wantMessage) + } + }) + } +} + +func TestSanitizeErrorMessage(t *testing.T) { + tests := []struct { + name string + message string + want string + }{ + { + name: "password in error", + message: "failed to connect: password=mysecretpass123", + want: "failed to connect: [REDACTED]", + }, + { + name: "api key in error", + message: "invalid api_key: AKIA1234567890ABCDEF", + want: "invalid [REDACTED]", + }, + { + name: "token in JSON format", + message: `{"token":"abc123def456","status":"error"}`, + want: `{"[REDACTED],"status":"error"}`, + }, + { + name: "JWT token", + message: "invalid token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dozjgNryP4J3jVmNHl0w5N_XgL0n3I9PlFUP0THsR8U", + want: "invalid token: [REDACTED]", + }, + { + name: "URL with credentials", + message: "failed to connect to https://user:password@example.com/db", + want: "failed to connect to [REDACTED]", + }, + { + name: "private key", + message: "error reading key: -----BEGIN RSA PRIVATE KEY-----\nMIIEpAIBAAKCAQEA...\n-----END RSA PRIVATE KEY-----", + want: "error reading key: [REDACTED]", + }, + { + name: "multiple secrets", + message: "config error: password=secret123 api_key=key456 token=tok789", + want: "config error: [REDACTED] [REDACTED] [REDACTED]", + }, + { + name: "safe error message", + message: "failed to connect to database: connection timeout", + want: "failed to connect to database: connection timeout", + }, + { + name: "secret in key-value format", + message: `error: "secret":"my-secret-value"`, + want: `error: secret=[REDACTED]`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SanitizeErrorMessage(tt.message) + // Use Contains for flexible matching since regex replacements might vary + if !strings.Contains(got, "[REDACTED]") && strings.Contains(tt.want, "[REDACTED]") { + t.Errorf("SanitizeErrorMessage() = %v, should contain [REDACTED]", got) + } + // Verify sensitive data is not present + if strings.Contains(got, "mysecretpass123") || + strings.Contains(got, "abc123def456") || + strings.Contains(got, "my-secret-value") { + t.Errorf("SanitizeErrorMessage() = %v, still contains sensitive data", got) + } + }) + } +} + +func TestSanitizeSecret(t *testing.T) { + tests := []struct { + name string + message string + secretValue string + want string + }{ + { + name: "replace secret value", + message: "error connecting with password: mySecretPassword123", + secretValue: "mySecretPassword123", + want: "error connecting with password: [REDACTED]", + }, + { + name: "empty secret value", + message: "error message", + secretValue: "", + want: "error message", + }, + { + name: "secret not in message", + message: "generic error", + secretValue: "secret123", + want: "generic error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SanitizeSecret(tt.message, tt.secretValue) + if got != tt.want { + t.Errorf("SanitizeSecret() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestIsSensitiveError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "nil error", + err: nil, + want: false, + }, + { + name: "error with password", + err: errors.New("invalid password provided"), + want: true, + }, + { + name: "error with secret", + err: errors.New("secret key not found"), + want: true, + }, + { + name: "error with token", + err: errors.New("invalid token format"), + want: true, + }, + { + name: "error with api key", + err: errors.New("apikey validation failed"), + want: true, + }, + { + name: "safe error", + err: errors.New("connection timeout"), + want: false, + }, + { + name: "error with credential", + err: errors.New("credential mismatch"), + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsSensitiveError(tt.err); got != tt.want { + t.Errorf("IsSensitiveError() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestSafeErrorMessage(t *testing.T) { + tests := []struct { + name string + err error + want string + }{ + { + name: "nil error", + err: nil, + want: "", + }, + { + name: "secure error", + err: NewSecureError(ErrorCategoryInternal, "safe message", errors.New("internal details")), + want: "[internal] safe message", + }, + { + name: "sensitive error", + err: errors.New("failed with password: secret123"), + want: "operation failed: an internal error occurred (details redacted for security)", + }, + { + name: "safe regular error", + err: errors.New("connection timeout"), + want: "connection timeout", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SafeErrorMessage(tt.err) + if got != tt.want { + t.Errorf("SafeErrorMessage() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestIsCategory(t *testing.T) { + tests := []struct { + name string + err error + category ErrorCategory + want bool + }{ + { + name: "matching category", + err: NewSecureError(ErrorCategoryAuthentication, "test", nil), + category: ErrorCategoryAuthentication, + want: true, + }, + { + name: "non-matching category", + err: NewSecureError(ErrorCategoryAuthentication, "test", nil), + category: ErrorCategoryAuthorization, + want: false, + }, + { + name: "regular error", + err: errors.New("regular error"), + category: ErrorCategoryInternal, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsCategory(tt.err, tt.category); got != tt.want { + t.Errorf("IsCategory() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestCommonErrors(t *testing.T) { + tests := []struct { + name string + err *SecureError + category ErrorCategory + }{ + { + name: "ErrAuthenticationFailed", + err: ErrAuthenticationFailed, + category: ErrorCategoryAuthentication, + }, + { + name: "ErrAuthorizationFailed", + err: ErrAuthorizationFailed, + category: ErrorCategoryAuthorization, + }, + { + name: "ErrInvalidConfiguration", + err: ErrInvalidConfiguration, + category: ErrorCategoryConfiguration, + }, + { + name: "ErrSecretNotFound", + err: ErrSecretNotFound, + category: ErrorCategoryConfiguration, + }, + { + name: "ErrInvalidSecret", + err: ErrInvalidSecret, + category: ErrorCategoryValidation, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.err == nil { + t.Errorf("common error %s is nil", tt.name) + } + if tt.err.Category() != tt.category { + t.Errorf("%s category = %v, want %v", tt.name, tt.err.Category(), tt.category) + } + }) + } +} + +func TestWrapWithSanitization(t *testing.T) { + tests := []struct { + name string + err error + category ErrorCategory + wantNil bool + shouldRedact bool + }{ + { + name: "nil error", + err: nil, + category: ErrorCategoryInternal, + wantNil: true, + shouldRedact: false, + }, + { + name: "error with password", + err: errors.New("connection failed: password=secret123"), + category: ErrorCategoryAuthentication, + wantNil: false, + shouldRedact: true, + }, + { + name: "safe error", + err: errors.New("connection timeout"), + category: ErrorCategoryNetwork, + wantNil: false, + shouldRedact: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := WrapWithSanitization(tt.err, tt.category) + if tt.wantNil { + if got != nil { + t.Errorf("WrapWithSanitization() = %v, want nil", got) + } + return + } + if got == nil { + t.Fatal("WrapWithSanitization() = nil, want non-nil") + } + if tt.shouldRedact && !strings.Contains(got.Error(), "[REDACTED]") { + t.Errorf("WrapWithSanitization() = %v, should contain [REDACTED]", got.Error()) + } + // Verify original error is preserved + if got.InternalError() != tt.err { + t.Errorf("WrapWithSanitization() internal error = %v, want %v", got.InternalError(), tt.err) + } + }) + } +} + +func TestSanitizeKeyValuePairs(t *testing.T) { + tests := []struct { + name string + message string + }{ + { + name: "JSON with password", + message: `{"password":"secret123","username":"user"}`, + }, + { + name: "key=value format", + message: "config: apikey=ABC123 token=XYZ789", + }, + { + name: "colon separated", + message: "auth: secret: my-secret-value", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := sanitizeKeyValuePairs(tt.message) + // Should contain REDACTED + if !strings.Contains(got, "[REDACTED]") { + t.Errorf("sanitizeKeyValuePairs() = %v, should contain [REDACTED]", got) + } + // Should not contain obvious secrets + if strings.Contains(got, "secret123") || + strings.Contains(got, "ABC123") || + strings.Contains(got, "my-secret-value") { + t.Errorf("sanitizeKeyValuePairs() = %v, still contains sensitive data", got) + } + }) + } +} + diff --git a/pkg/common/secerrors/logging.go b/pkg/common/secerrors/logging.go new file mode 100644 index 0000000000..51fefdb3f6 --- /dev/null +++ b/pkg/common/secerrors/logging.go @@ -0,0 +1,148 @@ +/* +Copyright 2024 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package secerrors + +import ( + "go.uber.org/zap" + "go.uber.org/zap/zapcore" +) + +// LogError logs an error with appropriate security considerations +// It logs the sanitized message at the specified level and the internal error at debug level +func LogError(logger *zap.SugaredLogger, err error, msg string, keysAndValues ...interface{}) { + if err == nil { + return + } + + // Get the safe error message + safeMsg := SafeErrorMessage(err) + + // Combine the message and safe error + fullMsg := msg + if safeMsg != "" { + fullMsg = fullMsg + ": " + safeMsg + } + + // Log the safe message at error level + logger.Errorw(fullMsg, keysAndValues...) + + // If it's a SecureError, log the internal error at debug level for troubleshooting + var secErr *SecureError + if logger.Desugar().Core().Enabled(zapcore.DebugLevel) { + if AsSecureError(err, &secErr) && secErr.InternalError() != nil { + logger.Debugw("internal error details", "internal_error", secErr.InternalError().Error()) + } + } +} + +// LogErrorf is like LogError but with formatted message +func LogErrorf(logger *zap.SugaredLogger, err error, format string, args ...interface{}) { + if err == nil { + return + } + + // Get the safe error message + safeMsg := SafeErrorMessage(err) + + // Log the safe message + logger.Errorf(format+": %s", append(args, safeMsg)...) + + // Log internal details at debug level + var secErr *SecureError + if logger.Desugar().Core().Enabled(zapcore.DebugLevel) { + if AsSecureError(err, &secErr) && secErr.InternalError() != nil { + logger.Debugf("internal error details: %v", secErr.InternalError()) + } + } +} + +// LogWarn logs a warning with security sanitization +func LogWarn(logger *zap.SugaredLogger, err error, msg string, keysAndValues ...interface{}) { + if err == nil { + return + } + + safeMsg := SafeErrorMessage(err) + fullMsg := msg + if safeMsg != "" { + fullMsg = fullMsg + ": " + safeMsg + } + + logger.Warnw(fullMsg, keysAndValues...) +} + +// LogWarnf is like LogWarn but with formatted message +func LogWarnf(logger *zap.SugaredLogger, err error, format string, args ...interface{}) { + if err == nil { + return + } + + safeMsg := SafeErrorMessage(err) + logger.Warnf(format+": %s", append(args, safeMsg)...) +} + +// SafeZapError creates a zap field with sanitized error message +func SafeZapError(err error) zap.Field { + if err == nil { + return zap.Skip() + } + return zap.String("error", SafeErrorMessage(err)) +} + +// InternalZapError creates a zap field with the internal error (for debug logging only) +// This should only be used when debug logging is enabled +func InternalZapError(err error) zap.Field { + var secErr *SecureError + if AsSecureError(err, &secErr) && secErr.InternalError() != nil { + return zap.String("internal_error", secErr.InternalError().Error()) + } + return zap.Skip() +} + +// AsSecureError is a helper that wraps errors.As for SecureError +func AsSecureError(err error, target **SecureError) bool { + if err == nil { + return false + } + + switch e := err.(type) { + case *SecureError: + *target = e + return true + default: + return false + } +} + +// SecureLogFields creates zap fields with proper sanitization +// It returns both a safe field for general logging and an internal field for debug logging +func SecureLogFields(err error) []zap.Field { + if err == nil { + return []zap.Field{} + } + + fields := []zap.Field{SafeZapError(err)} + + // Add internal error field if it's a SecureError + var secErr *SecureError + if AsSecureError(err, &secErr) && secErr.InternalError() != nil { + fields = append(fields, zap.String("error_category", string(secErr.Category()))) + } + + return fields +} + diff --git a/pkg/common/secerrors/reconciler.go b/pkg/common/secerrors/reconciler.go new file mode 100644 index 0000000000..a75738010a --- /dev/null +++ b/pkg/common/secerrors/reconciler.go @@ -0,0 +1,221 @@ +/* +Copyright 2024 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package secerrors + +import ( + "context" + "fmt" + + "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" + "knative.dev/pkg/logging" + pkgreconciler "knative.dev/pkg/reconciler" +) + +// ReconcilerErrorHandler provides secure error handling for reconcilers +type ReconcilerErrorHandler struct { + logger *zap.SugaredLogger + recorder EventRecorder +} + +// EventRecorder is an interface for recording Kubernetes events +// This allows us to record events without exposing sensitive information +type EventRecorder interface { + Event(object interface{}, eventtype, reason, message string) + Eventf(object interface{}, eventtype, reason, messageFmt string, args ...interface{}) +} + +// NewReconcilerErrorHandler creates a new error handler for reconcilers +func NewReconcilerErrorHandler(ctx context.Context, recorder EventRecorder) *ReconcilerErrorHandler { + return &ReconcilerErrorHandler{ + logger: logging.FromContext(ctx), + recorder: recorder, + } +} + +// HandleError processes an error with appropriate security sanitization +// Returns a sanitized error suitable for returning from a reconciler +func (h *ReconcilerErrorHandler) HandleError(err error, resource interface{}, operation string) error { + if err == nil { + return nil + } + + // Log the error securely + LogError(h.logger, err, fmt.Sprintf("%s failed", operation)) + + // Record event with safe message + if h.recorder != nil { + safeMsg := SafeErrorMessage(err) + h.recorder.Event(resource, corev1.EventTypeWarning, "OperationFailed", + fmt.Sprintf("%s failed: %s", operation, safeMsg)) + } + + // Return sanitized error + return WrapWithSanitization(err, ErrorCategoryInternal) +} + +// HandleSecretError handles errors related to secret operations +func (h *ReconcilerErrorHandler) HandleSecretError(err error, resource interface{}, secretName, operation string) error { + if err == nil { + return nil + } + + // Create a safe error message that doesn't expose secret details + safeMsg := fmt.Sprintf("secret operation failed for %s", operation) + secErr := Wrap(err, ErrorCategoryConfiguration, safeMsg) + + // Log securely + h.logger.Errorw(safeMsg, "secret_name", secretName) + + // Record event + if h.recorder != nil { + h.recorder.Event(resource, corev1.EventTypeWarning, "SecretOperationFailed", safeMsg) + } + + return secErr +} + +// HandleAuthError handles authentication/authorization errors +func (h *ReconcilerErrorHandler) HandleAuthError(err error, resource interface{}, operation string) error { + if err == nil { + return nil + } + + // Don't expose auth details + secErr := Wrap(err, ErrorCategoryAuthentication, "authentication or authorization failed") + + LogError(h.logger, secErr, fmt.Sprintf("%s requires authentication", operation)) + + if h.recorder != nil { + h.recorder.Event(resource, corev1.EventTypeWarning, "AuthenticationFailed", + "operation requires valid authentication") + } + + return secErr +} + +// HandleValidationError handles validation errors (these are usually safe to expose) +func (h *ReconcilerErrorHandler) HandleValidationError(err error, resource interface{}, field string) error { + if err == nil { + return nil + } + + // Validation errors are usually safe, but still sanitize + safeMsg := SanitizeErrorMessage(err.Error()) + secErr := NewSecureError(ErrorCategoryValidation, + fmt.Sprintf("validation failed for field %s: %s", field, safeMsg), err) + + h.logger.Warnw("validation failed", "field", field, "error", safeMsg) + + if h.recorder != nil { + h.recorder.Eventf(resource, corev1.EventTypeWarning, "ValidationFailed", + "validation failed for field %s", field) + } + + return secErr +} + +// HandleConfigError handles configuration errors +func (h *ReconcilerErrorHandler) HandleConfigError(err error, resource interface{}, configName string) error { + if err == nil { + return nil + } + + safeMsg := fmt.Sprintf("configuration error in %s", configName) + secErr := Wrap(err, ErrorCategoryConfiguration, safeMsg) + + LogError(h.logger, secErr, "configuration failed") + + if h.recorder != nil { + h.recorder.Event(resource, corev1.EventTypeWarning, "ConfigurationFailed", safeMsg) + } + + return secErr +} + +// WrapReconcilerEvent wraps errors as reconciler events with security sanitization +func WrapReconcilerEvent(err error, eventType, reason string) pkgreconciler.Event { + if err == nil { + return nil + } + + safeMsg := SafeErrorMessage(err) + return &pkgreconciler.ReconcilerEvent{ + EventType: eventType, + Reason: reason, + Format: "%s", + Args: []interface{}{safeMsg}, + } +} + +// SecureReconcilerError creates a reconciler event from a secure error +func SecureReconcilerError(secErr *SecureError) pkgreconciler.Event { + if secErr == nil { + return nil + } + + eventType := corev1.EventTypeWarning + reason := "ReconciliationFailed" + + // Customize based on error category + switch secErr.Category() { + case ErrorCategoryAuthentication, ErrorCategoryAuthorization: + reason = "AuthenticationFailed" + case ErrorCategoryConfiguration: + reason = "ConfigurationFailed" + case ErrorCategoryValidation: + reason = "ValidationFailed" + eventType = corev1.EventTypeWarning + } + + return &pkgreconciler.ReconcilerEvent{ + EventType: eventType, + Reason: reason, + Format: "%s", + Args: []interface{}{secErr.Error()}, + } +} + +// GetSanitizedConditionMessage creates a sanitized message for condition status +// This can be used when setting condition messages to ensure no sensitive data leaks +func GetSanitizedConditionMessage(err error, messageFormat string, messageA ...interface{}) string { + message := fmt.Sprintf(messageFormat, messageA...) + if err != nil { + safeMsg := SafeErrorMessage(err) + if safeMsg != "" { + message = message + ": " + safeMsg + } + } + return message +} + +// SecureEventf records an event with sanitized error information +func SecureEventf(recorder EventRecorder, object interface{}, eventtype, reason, messageFmt string, err error, args ...interface{}) { + if recorder == nil { + return + } + + // If error is provided, sanitize it + if err != nil { + safeErr := SafeErrorMessage(err) + args = append(args, safeErr) + messageFmt = messageFmt + ": %s" + } + + recorder.Eventf(object, eventtype, reason, messageFmt, args...) +} +