FIPS: Check user provided ctlog crypto for fips compliance

[JasonPowr]

PR Type

Enhancement, Tests

---

Description

• Add FIPS compliance validation for CTLog cryptographic materials

• Validate private/public keys and certificates against FIPS standards

• Reject non-FIPS-compliant keys (e.g., EC P224) and enforce minimum key sizes

• Add comprehensive unit and E2E tests for FIPS validation scenarios

---

Diagram Walkthrough

flowchart LR
  A["FIPS Detection"] -->|FIPSEnabled flag| B["Crypto Validation Module"]
  B -->|ValidatePrivateKeyPEM| C["Private Key Checks"]
  B -->|ValidatePublicKeyPEM| D["Public Key Checks"]
  B -->|ValidateCertificatePEM| E["Certificate Checks"]
  B -->|ValidateTLS| F["TLS Material Checks"]
  C -->|RSA/ECDSA validation| G["Key Size & Curve Validation"]
  D -->|RSA/ECDSA validation| G
  E -->|Signature algorithm| G
  G -->|Reject non-FIPS| H["Requeue with error"]
  G -->|Accept FIPS-compliant| I["Continue deployment"]
  J["CTLog Actions"] -->|Call validators| B

Loading

File Walkthrough

	Relevant files
Enhancement	7 files main.go Add FIPS TLS configuration to webhook server                          +10/-1    deployment.go Validate trusted CA certificates for FIPS compliance          +14/-0    handle_keys.go Validate private/public keys and add error conditions        +27/-2    server_config.go Validate server config and crypto materials for FIPS          +117/-1  tls.go Add FIPS validation for TLS certificates and keys                +15/-0    fips.go Implement FIPS compliance validation for cryptographic materials +297/-0  ctlog.go Add helper function for creating custom CTLog secrets        +10/-0
Tests	5 files handle_keys_test.go Add FIPS validation tests for key handling                              +193/-0  server_config_test.go Add comprehensive FIPS server config validation tests        +390/-1  fips_test.go Add unit tests for FIPS validation functions                          +436/-0  helpers.go Add test helpers for generating FIPS/non-FIPS crypto materials +141/-0  ctlog_test.go Add end-to-end FIPS compliance tests for CTLog                      +250/-0

---

[qodo-code-review]

PR Code Suggestions ✨

Latest suggestions up to 4577f1b

Category	Suggestion	Impact
Possible issue	Verify certificate matches private key In ValidateTLS, add a check to verify that the public key in the certificate matches the provided private key. This prevents runtime failures caused by mismatched TLS key pairs. internal/utils/crypto/fips.go [173-195] func ValidateTLS(client client.Client, namespace string, tls v1alpha1.TLS) error { + var certPEM, keyPEM []byte + var err error + if tls.CertRef != nil { - cert, err := kubernetes.GetSecretData(client, namespace, tls.CertRef) + certPEM, err = kubernetes.GetSecretData(client, namespace, tls.CertRef) if err != nil { return fmt.Errorf("failed to get certificate: %w", err) } - if err := ValidateCertificatePEM(cert); err != nil { + if err := ValidateCertificatePEM(certPEM); err != nil { return fmt.Errorf("certificate validation failed: %w", err) } } if tls.PrivateKeyRef != nil { - key, err := kubernetes.GetSecretData(client, namespace, tls.PrivateKeyRef) + keyPEM, err = kubernetes.GetSecretData(client, namespace, tls.PrivateKeyRef) if err != nil { return fmt.Errorf("failed to get private key: %w", err) } - if err := ValidatePrivateKeyPEM(key, nil); err != nil { + if err := ValidatePrivateKeyPEM(keyPEM, nil); err != nil { return fmt.Errorf("private key validation failed: %w", err) + } + } + + // If both are present, ensure they form a valid keypair. + if len(certPEM) != 0 && len(keyPEM) != 0 { + certBlock, _ := pem.Decode(certPEM) + if certBlock == nil || certBlock.Type != "CERTIFICATE" { + return fmt.Errorf("failed to parse certificate PEM") + } + cert, err := x509.ParseCertificate(certBlock.Bytes) + if err != nil { + return fmt.Errorf("failed to parse certificate: %w", err) + } + + keyBlock, _ := pem.Decode(keyPEM) + if keyBlock == nil { + return fmt.Errorf("failed to parse private key PEM") + } + der := keyBlock.Bytes + if x509.IsEncryptedPEMBlock(keyBlock) { //nolint:staticcheck + return ErrNoPassword + } + + var priv any + if k, e := x509.ParsePKCS8PrivateKey(der); e == nil { + priv = k + } else if k, e := x509.ParsePKCS1PrivateKey(der); e == nil { + priv = k + } else if k, e := x509.ParseECPrivateKey(der); e == nil { + priv = k + } else { + return ErrUnsupportedKey + } + + if !publicKeysEqual(cert.PublicKey, priv) { + return fmt.Errorf("certificate does not match private key") } } return nil } Apply / Chat Suggestion importance[1-10]: 8 __ Why: This is a valuable suggestion that addresses a potential runtime failure by adding a check to ensure the provided TLS certificate and private key form a valid pair. This "fail-fast" approach improves the operator's robustness and provides clearer, earlier feedback on misconfigurations.	Medium
	Reject trailing key PEM data Improve PEM validation for private and public keys by rejecting any trailing data after the PEM block and explicitly checking for expected PEM block types. This makes the validation stricter and consistent with certificate validation. internal/utils/crypto/fips.go [67-121] func ValidatePrivateKeyPEM(pemBytes []byte, password []byte) error { if !FIPSEnabled { return nil } - block, _ := pem.Decode(pemBytes) + block, rest := pem.Decode(pemBytes) if block == nil { return ErrInvalidPEM + } + if len(bytes.TrimSpace(rest)) != 0 { + return fmt.Errorf("%w: trailing data after PEM block", ErrInvalidPEM) + } + switch block.Type { + case "PRIVATE KEY", "RSA PRIVATE KEY", "EC PRIVATE KEY": + default: + return fmt.Errorf("%w: unexpected PEM block %q", ErrInvalidPEM, block.Type) } der := block.Bytes if x509.IsEncryptedPEMBlock(block) { //nolint:staticcheck if len(password) == 0 { return ErrNoPassword } decrypted, err := x509.DecryptPEMBlock(block, password) //nolint:staticcheck if err != nil { return fmt.Errorf("%w: %v", ErrFailedToDecrypt, err) } der = decrypted } if key, err := x509.ParsePKCS8PrivateKey(der); err == nil { return validatePrivateKeyType(key) } if key, err := x509.ParsePKCS1PrivateKey(der); err == nil { return validatePrivateKeyType(key) } if key, err := x509.ParseECPrivateKey(der); err == nil { return validatePrivateKeyType(key) } return ErrUnsupportedKey } func ValidatePublicKeyPEM(pemBytes []byte) error { if !FIPSEnabled { return nil } - block, _ := pem.Decode(pemBytes) + block, rest := pem.Decode(pemBytes) if block == nil { return ErrInvalidPEM + } + if len(bytes.TrimSpace(rest)) != 0 { + return fmt.Errorf("%w: trailing data after PEM block", ErrInvalidPEM) } der := block.Bytes if key, err := x509.ParsePKIXPublicKey(der); err == nil { return validatePublicKeyType(key) } if key, err := x509.ParsePKCS1PublicKey(der); err == nil { return validatePublicKeyType(key) } return ErrUnsupportedKey } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that the PEM validation for keys is less strict than for certificates, as it ignores trailing data, which could mask misconfigurations. Implementing this would improve input validation robustness and align the key validation logic with the stricter certificate validation already present in the PR.	Medium
	Validate derived secret key names Add validation to ensure that filenames extracted from paths in the server config are not empty, ., or / before using them to look up data in a secret. This hardens the parsing of custom server configurations. internal/controller/ctlog/actions/server_config.go [432-453] privateKeyName := path.Base(pemKey.Path) +if privateKeyName == "" || privateKeyName == "." || privateKeyName == "/" { + return fmt.Errorf("server config private key path %q does not resolve to a valid filename", pemKey.Path) +} privateKey, ok := secretData[privateKeyName] if !ok { return fmt.Errorf("private key %s is missing from secret", privateKeyName) } if err := cryptoutil.ValidatePrivateKeyPEM(privateKey, []byte(pemKey.Password)); err != nil { return fmt.Errorf("private key is not FIPS-compliant: %w", err) } for _, rootPath := range logConfig.RootsPemFile { if rootPath == "" { return fmt.Errorf("root certificate path is empty") } rootName := path.Base(rootPath) + if rootName == "" || rootName == "." || rootName == "/" { + return fmt.Errorf("root certificate path %q does not resolve to a valid filename", rootPath) + } rootData, ok := secretData[rootName] if !ok { return fmt.Errorf("root certificate %s is missing from secret", rootName) } if err := cryptoutil.ValidateCertificatePEM(rootData); err != nil { return fmt.Errorf("root certificate %s is not FIPS-compliant: %w", rootName, err) } } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies an edge case where a malformed path in a custom server config could lead to confusing behavior. Adding validation for the extracted filename from path.Base() makes the configuration parsing more robust and provides clearer error messages to the user.	Low
Update

---

Previous suggestions

Suggestions up to commit 33fd58d

Category	Suggestion	Impact
High-level	Consider a more robust FIPS detection The current FIPS detection by reading /proc/sys/crypto/fips_enabled is not portable. A more robust approach using a build-time tag or an explicit configuration flag should be considered for predictable behavior. Examples: internal/utils/crypto/fips.go [52-65] func init() { FIPSEnabled = IsFIPS() } func IsFIPS() bool { const fipsPath = "/proc/sys/crypto/fips_enabled" data, err := os.ReadFile(fipsPath) if err != nil { return false ... (clipped 4 lines) Solution Walkthrough: Before: // internal/utils/crypto/fips.go var FIPSEnabled bool func init() { FIPSEnabled = IsFIPS() } func IsFIPS() bool { const fipsPath = "/proc/sys/crypto/fips_enabled" data, err := os.ReadFile(fipsPath) if err != nil { return false } return strings.TrimSpace(string(data)) == "1" } After: // internal/utils/crypto/fips.go var FIPSEnabled bool // This function can be defined in a separate file with a build tag, e.g., fips.go // +build fips func init() { FIPSEnabled = true } // cmd/main.go var enableFIPS = flag.Bool("enable-fips", false, "Enable FIPS mode.") func main() { flag.Parse() // ... // FIPSEnabled can be set from build tag or overridden by a flag if *enableFIPS { cryptoutil.FIPSEnabled = true } // ... } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a brittle FIPS detection mechanism (/proc/sys/crypto/fips_enabled) and proposes more robust, portable alternatives like build tags or configuration flags, which is crucial for a security-sensitive feature.	Medium
Possible issue	Fix incomplete validation of CAs Fix a bug in ValidateTrustedCA where it prematurely returns success. Modify the loop to break after processing a ConfigMap entry, ensuring all entries are validated before returning. internal/utils/crypto/fips.go [197-240] func ValidateTrustedCA(ctx context.Context, c client.Client, namespace string, ca *v1alpha1.LocalObjectReference) error { if !FIPSEnabled || ca == nil { return nil } cm := &corev1.ConfigMap{} if err := c.Get(ctx, types.NamespacedName{Namespace: namespace, Name: ca.Name}, cm); err != nil { return fmt.Errorf("could not load trusted CA %q: %w", ca.Name, err) } for k, v := range cm.Data { rest := []byte(v) for { block, remaining := pem.Decode(rest) if block == nil { - if len(rest) == 0 { - return nil + if len(bytes.TrimSpace(rest)) == 0 { + break // Finished processing this entry } return fmt.Errorf("%w: trusted CA entry %q contains invalid PEM", ErrInvalidPEM, k) } if block.Type != "CERTIFICATE" { return fmt.Errorf("%w: trusted CA entry %q has unexpected PEM block %q", ErrInvalidPEM, k, block.Type) } cert, err := x509.ParseCertificate(block.Bytes) if err != nil { return fmt.Errorf("%w: trusted CA entry %q failed to parse certificate: %v", ErrInvalidPEM, k, err) } if err := validateSignatureAlgorithm(cert.SignatureAlgorithm); err != nil { return fmt.Errorf("trusted CA entry %q certificate signature: %w", k, err) } if err := validatePublicKeyType(cert.PublicKey); err != nil { return fmt.Errorf("trusted CA entry %q certificate public key: %w", k, err) } rest = remaining } } return nil } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a logic bug in ValidateTrustedCA where the function would exit prematurely after validating only the first entry in a ConfigMap, leading to incomplete validation. The proposed fix is correct and ensures all entries are processed.	Medium
	Return status update with requeue In handleKeys.Handle, capture the result of g.StatusUpdate, set its Requeue flag to true, and return it. This ensures the status update is persisted before requeuing. internal/controller/ctlog/actions/handle_keys.go [82-90] meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ Type: ConfigCondition, Status: metav1.ConditionFalse, Reason: SignerKeyReason, Message: fmt.Sprintf("Waiting for Ctlog signer key: %v", err), ObservedGeneration: instance.Generation, }) -g.StatusUpdate(ctx, instance) -return g.Requeue() +res := g.StatusUpdate(ctx, instance) +res.Requeue = true +return res Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the result of g.StatusUpdate is ignored, which could lead to status changes not being persisted. Combining the status update and requeue into a single returned result is the correct pattern for this controller logic.	Medium
	Persist status then requeue In serverConfig.Handle, capture the result of i.StatusUpdate, set its Requeue flag to true, and return it. This ensures the status update is persisted before requeuing. internal/controller/ctlog/actions/server_config.go [99-107] meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ Type: ConfigCondition, Status: metav1.ConditionFalse, Reason: state.Failure.String(), Message: fmt.Sprintf("Invalid server config: %v", err), ObservedGeneration: instance.Generation, }) -i.StatusUpdate(ctx, instance) -return i.Requeue() +res := i.StatusUpdate(ctx, instance) +res.Requeue = true +return res Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the result of i.StatusUpdate is ignored, which could lead to status changes not being persisted. Combining the status update and requeue into a single returned result is the correct pattern for this controller logic.	Medium
Security	Handle trailing data in certificate validation Improve ValidateCertificatePEM to reject inputs with trailing non-PEM data. Change the loop to return an error if unexpected data is found after a valid certificate block. internal/utils/crypto/fips.go [123-139] func ValidateCertificatePEM(pemBytes []byte) error { if !FIPSEnabled { return nil } if len(bytes.TrimSpace(pemBytes)) == 0 { return ErrInvalidPEM } for { block, rest := pem.Decode(pemBytes) if block == nil { if len(bytes.TrimSpace(pemBytes)) == 0 { return nil } - return ErrInvalidPEM + return fmt.Errorf("%w: trailing data after PEM block", ErrInvalidPEM) } ... Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that ValidateCertificatePEM might not validate the entire input if it contains trailing data after a valid PEM block. The proposed fix improves robustness by ensuring all data is processed, which is a good security practice.	Medium
General	Enforce FIPS cipher suites In main, enhance FIPS TLS options by explicitly setting CipherSuites to a list of FIPS-approved algorithms and setting PreferServerCipherSuites to true. cmd/main.go [144-146] fipsTLSOpts := func(c *tls.Config) { c.MinVersion = tls.VersionTLS12 + c.CipherSuites = []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + } + c.PreferServerCipherSuites = true } Suggestion importance[1-10]: 7 __ Why: The suggestion improves FIPS compliance by not only setting the minimum TLS version but also explicitly defining a list of FIPS-approved cipher suites. This provides a stronger guarantee of compliance.	Medium
	Use stronger encryption in test helpers In GenerateECCertificatePEM, replace the weak x509.PEMCipher3DES encryption with the FIPS-compliant x509.PEMCipherAES256 for better security practices in test helpers. internal/utils/crypto/test/helpers.go [84-89] func GenerateECCertificatePEM(passwordProtected bool, CertPassword string, curve elliptic.Curve) ([]byte, []byte, []byte, error) { ... var block *pem.Block if passwordProtected { - block, err = x509.EncryptPEMBlock(rand.Reader, "EC PRIVATE KEY", privateKeyBytes, []byte(CertPassword), x509.PEMCipher3DES) //nolint:staticcheck + block, err = x509.EncryptPEMBlock(rand.Reader, "EC PRIVATE KEY", privateKeyBytes, []byte(CertPassword), x509.PEMCipherAES256) //nolint:staticcheck if err != nil { return nil, nil, nil, err } } else { ... Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies the use of a weak and deprecated cipher (3DES) in test code and recommends a stronger, modern alternative (AES-256). While this is in test code, aligning with security best practices is a valuable improvement.	Low
	Check PEM block type first In ValidatePublicKeyPEM, add a check to ensure the PEM block type is either "PUBLIC KEY" or "RSA PUBLIC KEY" before attempting to parse it. internal/utils/crypto/fips.go [107-120] block, _ := pem.Decode(pemBytes) if block == nil { return ErrInvalidPEM +} +switch block.Type { +case "PUBLIC KEY", "RSA PUBLIC KEY": +default: + return fmt.Errorf("%w: unexpected PEM block %q", ErrInvalidPEM, block.Type) } der := block.Bytes if key, err := x509.ParsePKIXPublicKey(der); err == nil { return validatePublicKeyType(key) } if key, err := x509.ParsePKCS1PublicKey(der); err == nil { return validatePublicKeyType(key) } return ErrUnsupportedKey Suggestion importance[1-10]: 5 __ Why: The suggestion improves the robustness of ValidatePublicKeyPEM by adding a check for the PEM block type. This prevents the function from attempting to parse data that is not a public key, making the validation stricter.	Low

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Limited audit context: The new requeue paths log a generic "waiting" message without a stable correlation identifier (e.g., user/CR identity or secret refs) that would be required to reliably reconstruct key-validation events in an audit trail. Referred Code keys, err := g.setupKeys(instance.Namespace, newKeyStatus) if err != nil { g.Logger.Info("waiting for Ctlog signer key", "error", err.Error()) meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ Type: ConfigCondition, Status: metav1.ConditionFalse, Reason: SignerKeyReason, Message: "Waiting for Ctlog signer key", ObservedGeneration: instance.Generation, }) g.StatusUpdate(ctx, instance) return g.Requeue() } if _, err = g.generateAndUploadSecret(ctx, instance, newKeyStatus, keys); err != nil { g.Logger.Info("waiting for Ctlog signer key", "error", err.Error()) meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ Type: ConfigCondition, Status: metav1.ConditionFalse, Reason: SignerKeyReason, Message: "Waiting for Ctlog signer key", ObservedGeneration: instance.Generation, ... (clipped 4 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Error text logging: New logs include raw err.Error() which may include internal secret/key identifiers and should be reviewed to ensure no sensitive material can ever appear in error strings produced by FIPS validation. Referred Code if cryptoutil.FIPSEnabled { if err := validateServerConfigSecret(secret); err != nil { i.Logger.Error(err, "invalid server config secret", "secret", instance.Spec.ServerConfigRef.Name) meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ Type: ConfigCondition, Status: metav1.ConditionFalse, Reason: state.Failure.String(), Message: "Invalid server config", ObservedGeneration: instance.Generation, }) i.StatusUpdate(ctx, instance) return i.Requeue() } } Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Make FIPS mode an explicit configuration The current automatic FIPS mode detection based on the node's filesystem is unreliable in a Kubernetes cluster. This should be replaced with an explicit configuration, such as a command-line flag, to ensure consistent behavior regardless of where the operator pod is scheduled. Examples: internal/utils/crypto/fips.go [52-65] func init() { FIPSEnabled = IsFIPS() } func IsFIPS() bool { const fipsPath = "/proc/sys/crypto/fips_enabled" data, err := os.ReadFile(fipsPath) if err != nil { return false ... (clipped 4 lines) Solution Walkthrough: Before: // internal/utils/crypto/fips.go var FIPSEnabled bool func init() { FIPSEnabled = IsFIPS() } func IsFIPS() bool { // Reads /proc/sys/crypto/fips_enabled from the node's filesystem data, err := os.ReadFile("/proc/sys/crypto/fips_enabled") if err != nil { return false } return strings.TrimSpace(string(data)) == "1" } After: // cmd/main.go var ( fipsMode bool ) func init() { flag.BoolVar(&fipsMode, "fips", false, "Enable FIPS mode for cryptographic validations.") } func main() { flag.Parse() cryptoutil.FIPSEnabled = fipsMode // ... } // internal/utils/crypto/fips.go // The automatic detection via init() and IsFIPS() is removed. var FIPSEnabled bool // Set externally from main package Suggestion importance[1-10]: 9 __ Why: This suggestion addresses a critical design flaw where automatic FIPS detection based on node state can cause unpredictable operator behavior, and proposing an explicit configuration is a significant architectural improvement for reliability.	High
General	return status update with requeue Refactor the error handling to use the Result from g.StatusUpdate and set Requeue on it, ensuring the status update is not lost. internal/controller/ctlog/actions/handle_keys.go [81-90] g.Logger.Info("waiting for Ctlog signer key", "error", err.Error()) meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ Type: ConfigCondition, Status: metav1.ConditionFalse, Reason: SignerKeyReason, Message: "Waiting for Ctlog signer key", ObservedGeneration: instance.Generation, }) -g.StatusUpdate(ctx, instance) -return g.Requeue() +res := g.StatusUpdate(ctx, instance) +res.Requeue = true +return res Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that g.StatusUpdate returns a Result object that should be used, and chaining Requeue on it is a better pattern than returning a new Result object, thus improving code correctness and consistency.	Medium
Possible issue	Improve PEM parsing to handle non-certificate blocks Modify ValidateCertificatePEM to skip non-certificate PEM blocks instead of erroring, making the parsing more robust. internal/utils/crypto/fips.go [123-160] func ValidateCertificatePEM(pemBytes []byte) error { if !FIPSEnabled { return nil } if len(bytes.TrimSpace(pemBytes)) == 0 { return ErrInvalidPEM } - for { + for len(pemBytes) > 0 { block, rest := pem.Decode(pemBytes) if block == nil { - if len(bytes.TrimSpace(pemBytes)) == 0 { + // No more PEM blocks, but there might be trailing non-PEM data. + // If the remaining data is just whitespace, we're done. + if len(bytes.TrimSpace(rest)) == 0 { return nil } + // Otherwise, the input is malformed. return ErrInvalidPEM } if block.Type != "CERTIFICATE" { - return fmt.Errorf("%w: unexpected PEM block %q", ErrInvalidPEM, block.Type) + // Skip non-certificate blocks, like comments or other keys. + pemBytes = rest + continue } cert, err := x509.ParseCertificate(block.Bytes) if err != nil { return fmt.Errorf("%w: failed to parse certificate: %v", ErrInvalidPEM, err) } if err := validateSignatureAlgorithm(cert.SignatureAlgorithm); err != nil { return fmt.Errorf("certificate signature: %w", err) } if err := validatePublicKeyType(cert.PublicKey); err != nil { return fmt.Errorf("certificate public key: %w", err) } pemBytes = rest } + return nil } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the current PEM validation is too strict and will fail on valid files containing non-certificate blocks, improving the function's robustness.	Low
	Improve trusted CA validation for PEM files Modify ValidateTrustedCA to skip non-certificate PEM blocks instead of erroring, making the parsing more robust. internal/utils/crypto/fips.go [197-240] func ValidateTrustedCA(ctx context.Context, c client.Client, namespace string, ca *v1alpha1.LocalObjectReference) error { if !FIPSEnabled || ca == nil { return nil } cm := &corev1.ConfigMap{} if err := c.Get(ctx, types.NamespacedName{Namespace: namespace, Name: ca.Name}, cm); err != nil { return fmt.Errorf("could not load trusted CA %q: %w", ca.Name, err) } for k, v := range cm.Data { rest := []byte(v) - for { + for len(rest) > 0 { block, remaining := pem.Decode(rest) if block == nil { - if len(rest) == 0 { + if len(bytes.TrimSpace(remaining)) == 0 { break } return fmt.Errorf("%w: trusted CA entry %q contains invalid PEM", ErrInvalidPEM, k) } if block.Type != "CERTIFICATE" { - return fmt.Errorf("%w: trusted CA entry %q has unexpected PEM block %q", ErrInvalidPEM, k, block.Type) + rest = remaining + continue } cert, err := x509.ParseCertificate(block.Bytes) if err != nil { return fmt.Errorf("%w: trusted CA entry %q failed to parse certificate: %v", ErrInvalidPEM, k, err) } if err := validateSignatureAlgorithm(cert.SignatureAlgorithm); err != nil { return fmt.Errorf("trusted CA entry %q certificate signature: %w", k, err) } if err := validatePublicKeyType(cert.PublicKey); err != nil { return fmt.Errorf("trusted CA entry %q certificate public key: %w", k, err) } rest = remaining } } return nil } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that the trusted CA validation is too strict and will fail on valid PEM data containing non-certificate blocks, improving the function's robustness.	Low
More

[JasonPowr]

@osmman When you have a chance could I get eyes on this again :), thanks

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	TLS cipher restrictions Description: When cryptoutil.FIPSEnabled is true, the webhook server TLS hardening only enforces tls.VersionTLS12 but does not explicitly restrict cipher suites/curves, so depending on the Go TLS implementation/build (e.g., not using a FIPS-validated crypto backend) the server could still negotiate non-FIPS or otherwise weaker cipher suites in TLS 1.2. main.go [141-148] Referred Code tlsOpts := []func(*tls.Config){} if cryptoutil.FIPSEnabled { setupLog.Info("Operator is running in FIPS enabled environment", "FIPSEnabled", cryptoutil.FIPSEnabled) fipsTLSOpts := func(c *tls.Config) { c.MinVersion = tls.VersionTLS12 } tlsOpts = append(tlsOpts, fipsTLSOpts) }
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Encrypted TLS key: ValidateTLS validates tls.PrivateKeyRef by calling ValidatePrivateKeyPEM(key, nil), which will always fail for encrypted PEM keys (no password path), potentially breaking legitimate encrypted TLS key usage in FIPS mode. Referred Code if tls.PrivateKeyRef != nil { key, err := kubernetes.GetSecretData(client, namespace, tls.PrivateKeyRef) if err != nil { return fmt.Errorf("failed to get private key: %w", err) } if err := ValidatePrivateKeyPEM(key, nil); err != nil { return fmt.Errorf("private key validation failed: %w", err) } } Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Use modern crypto API for keys Replace deprecated and potentially insecure key decryption functions with the modern pkcs8.ParsePrivateKey API to improve FIPS compliance, while retaining a fallback for compatibility. internal/utils/crypto/fips.go [78-97] + var der []byte + var err error if x509.IsEncryptedPEMBlock(block) { //nolint:staticcheck if len(password) == 0 { return ErrNoPassword } - decrypted, err := x509.DecryptPEMBlock(block, password) //nolint:staticcheck + // Try to parse with modern PKCS#8 encryption + var pkey interface{} + pkey, err = pkcs8.ParsePrivateKey(block.Bytes, password) + if err == nil { + return validatePrivateKeyType(pkey) + } + + // Fallback to deprecated method for other PEM encryption types + der, err = x509.DecryptPEMBlock(block, password) //nolint:staticcheck if err != nil { return fmt.Errorf("%w: %v", ErrFailedToDecrypt, err) } - der = decrypted + } else { + der = block.Bytes } if key, err := x509.ParsePKCS8PrivateKey(der); err == nil { return validatePrivateKeyType(key) } if key, err := x509.ParsePKCS1PrivateKey(der); err == nil { return validatePrivateKeyType(key) } if key, err := x509.ParseECPrivateKey(der); err == nil { return validatePrivateKeyType(key) } Apply / Chat Suggestion importance[1-10]: 9 __ Why: This is a critical security improvement that replaces deprecated functions (x509.IsEncryptedPEMBlock, x509.DecryptPEMBlock) with a modern, FIPS-compliant alternative (pkcs8.ParsePrivateKey), directly addressing potential use of insecure encryption algorithms.	High
	Enforce FIPS cipher suites Enforce FIPS-approved cipher suites and curve preferences in the TLS options to prevent negotiation of non-FIPS algorithms. cmd/main.go [144-146] fipsTLSOpts := func(c *tls.Config) { c.MinVersion = tls.VersionTLS12 + c.CipherSuites = []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + } + c.CurvePreferences = []tls.CurveID{ + tls.CurveP256, + tls.CurveP384, + tls.CurveP521, + } } Apply / Chat Suggestion importance[1-10]: 9 __ Why: This is a crucial security enhancement that hardens the FIPS-enabled TLS configuration by explicitly restricting it to FIPS-approved cipher suites and curve preferences, preventing the negotiation of weaker or non-compliant algorithms.	High
General	Provide more detailed FIPS error messages Enhance the status message for failed root certificate checks to include the specific FIPS validation error, providing more detailed feedback for easier debugging. internal/controller/ctlog/actions/server_config.go [165-177] rootCerts, err := i.handleRootCertificates(instance) if err != nil { + errMsg := "Waiting for Fulcio root certificate" + if cryptoutil.FIPSEnabled { + errMsg = fmt.Sprintf("Invalid Fulcio root certificate: %v", err) + } i.Logger.Info("waiting for Fulcio root certificate", "error", err.Error()) meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ Type: ConfigCondition, Status: metav1.ConditionFalse, Reason: FulcioReason, - Message: "Waiting for Fulcio root certificate", + Message: errMsg, ObservedGeneration: instance.Generation, }) i.StatusUpdate(ctx, instance) return i.Requeue() } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion improves user experience by providing a more descriptive status message when FIPS validation for a root certificate fails. This makes diagnosing configuration issues easier by exposing the underlying error.	Medium
	Include secret name in error When a custom server config secret fails FIPS validation, include the secret's name in the error and status message for easier debugging. internal/controller/ctlog/actions/server_config.go [97-110] if cryptoutil.FIPSEnabled { if err := validateServerConfigSecret(secret); err != nil { - i.Logger.Error(err, "invalid server config secret", "secret", instance.Spec.ServerConfigRef.Name) + wrappedErr := fmt.Errorf("invalid server config secret %q: %w", secret.Name, err) + i.Logger.Error(wrappedErr, "invalid server config secret") meta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ Type: ConfigCondition, Status: metav1.ConditionFalse, Reason: state.Failure.String(), - Message: "Invalid server config", + Message: wrappedErr.Error(), ObservedGeneration: instance.Generation, }) i.StatusUpdate(ctx, instance) return i.Requeue() } } Apply / Chat Suggestion importance[1-10]: 7 __ Why: This suggestion improves observability by adding the secret name to the error message when a custom server config fails FIPS validation. This makes it easier for users to identify the problematic resource.	Medium
Possible issue	Improve handling of non-PEM ConfigMap data Modify ValidateTrustedCA to gracefully skip ConfigMap data entries that do not contain any PEM blocks, preventing incorrect "invalid PEM" errors. internal/utils/crypto/fips.go [197-240] func ValidateTrustedCA(ctx context.Context, c client.Client, namespace string, ca *v1alpha1.LocalObjectReference) error { if !FIPSEnabled || ca == nil { return nil } cm := &corev1.ConfigMap{} if err := c.Get(ctx, types.NamespacedName{Namespace: namespace, Name: ca.Name}, cm); err != nil { return fmt.Errorf("could not load trusted CA %q: %w", ca.Name, err) } for k, v := range cm.Data { rest := []byte(v) + var foundPEM bool for { block, remaining := pem.Decode(rest) if block == nil { - if len(rest) == 0 { + if !foundPEM && len(bytes.TrimSpace(rest)) > 0 { + // Ignore entries with no PEM blocks break } - return fmt.Errorf("%w: trusted CA entry %q contains invalid PEM", ErrInvalidPEM, k) + if len(bytes.TrimSpace(rest)) > 0 { + return fmt.Errorf("%w: trusted CA entry %q contains trailing data", ErrInvalidPEM, k) + } + break } -... + foundPEM = true + + if block.Type != "CERTIFICATE" { + return fmt.Errorf("%w: trusted CA entry %q has unexpected PEM block %q", ErrInvalidPEM, k, block.Type) + } + + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return fmt.Errorf("%w: trusted CA entry %q failed to parse certificate: %v", ErrInvalidPEM, k, err) + } + + if err := validateSignatureAlgorithm(cert.SignatureAlgorithm); err != nil { + return fmt.Errorf("trusted CA entry %q certificate signature: %w", k, err) + } + if err := validatePublicKeyType(cert.PublicKey); err != nil { + return fmt.Errorf("trusted CA entry %q certificate public key: %w", k, err) + } + rest = remaining } } return nil } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential issue where ValidateTrustedCA would incorrectly fail on a ConfigMap containing non-certificate data. The proposed change makes the function more robust by gracefully skipping such entries.	Low
More