Use correct Key Manager value #1331
Conversation
sgayangi
requested review from
CrowleyRajapakse,
ashera96 and
npamudika
as code owners
WalkthroughAdds robust certificate handling to signature validation (base64 decoding, PEM/X.509 parsing and validation, error-aware marshal flow) and reduces logging verbosity by changing several Info logs to Debug. Changes
Sequence Diagram(s)sequenceDiagram
participant Sync as Synchronizer / Caller
participant K8s as k8s_client.marshalSignatureValidation
participant CR as TokenIssuer CR (create/update)
Sync->>K8s: request marshalSignatureValidation(config)
alt CertificateType == JWKS
K8s-->>Sync: SignatureValidation{JWKS} (nil error)
else CertificateType != JWKS
K8s->>K8s: try base64.Decode(value)
alt decode success
K8s->>K8s: pem.Decode -> x509.ParseCertificate
alt valid PEM/X.509
K8s-->>Sync: SignatureValidation{CertificateInline} (nil error)
else invalid PEM/X.509
K8s-->>Sync: error (invalid certificate)
end
else decode fail
K8s->>K8s: attempt pem.Decode on original value -> x509.ParseCertificate
alt valid PEM/X.509
K8s-->>Sync: SignatureValidation{CertificateInline} (nil error)
else invalid PEM/X.509
K8s-->>Sync: error (invalid certificate)
end
end
end
alt no error
Sync->>CR: assign SignatureValidation to tokenIssuer.Spec and create/update CR
CR-->>Sync: success / log
else error
Sync-->>CR: abort create/update, log error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apim-apk-agent/internal/k8sClient/k8s_client.go(2 hunks)apim-apk-agent/internal/messaging/km_listener.go(2 hunks)apim-apk-agent/internal/synchronizer/keymanagers_fetcher.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apim-apk-agent/internal/synchronizer/keymanagers_fetcher.go (1)
apim-apk-agent/internal/loggers/logger.go (1)
LoggerSynchronizer(51-51)
apim-apk-agent/internal/messaging/km_listener.go (2)
apim-apk-agent/internal/loggers/logger.go (1)
LoggerMessaging(50-50)apim-apk-agent/internal/eventhub/marshaller.go (1)
MarshalKeyManager(271-281)
apim-apk-agent/internal/k8sClient/k8s_client.go (1)
apim-apk-agent/internal/loggers/logger.go (1)
LoggerK8sClient(48-48)
🔇 Additional comments (4)
apim-apk-agent/internal/synchronizer/keymanagers_fetcher.go (1)
131-131: LGTM: Appropriate log level adjustment.Reducing verbosity by moving detailed key manager data to debug level is appropriate for production environments.
apim-apk-agent/internal/messaging/km_listener.go (1)
83-83: LGTM: Consistent log level adjustments.All three logging statements appropriately moved to debug level, reducing operational log verbosity while preserving diagnostic information.
Also applies to: 95-95, 97-97
apim-apk-agent/internal/k8sClient/k8s_client.go (2)
24-24: LGTM: Import addition.The
encoding/base64import is correctly added to support certificate value decoding.
799-802: LGTM: Debug logging additions.The debug logs provide useful diagnostic information for signature validation configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apim-apk-agent/internal/k8sClient/k8s_client.go (1)
808-838: Certificate validation implementation is robust and addresses past review concerns.The validation chain properly ensures certificate integrity:
Base64 detection (lines 817-822): Attempts decoding and uses the result if successful. While this could theoretically produce false positives, the subsequent validations prevent invalid certificates from being accepted.
PEM validation (lines 824-828): Ensures the certificate (decoded or original) is in valid PEM format. This catches any incorrectly decoded values.
X.509 validation (lines 830-833): Parses and validates the certificate structure, providing the final safety check.
The error propagation ensures invalid certificates are rejected at TokenIssuer creation/update time.
Optional optimization: Consider checking for PEM format markers (
-----BEGIN CERTIFICATE-----) before attempting base64 decode to avoid unnecessary processing. However, this is not critical since PEM format contains characters (dashes, newlines) that should cause base64 decode to fail naturally.Example optimization:
certValue := keyManagerConfig.CertificateValue // Check if the certificate value is base64 encoded and decode it -if decodedCert, err := base64.StdEncoding.DecodeString(certValue); err == nil { +if !strings.Contains(certValue, "-----BEGIN") { + if decodedCert, err := base64.StdEncoding.DecodeString(certValue); err == nil { // Successfully decoded, use the decoded value decodedCertStr := string(decodedCert) loggers.LoggerK8sClient.Debugf("Certificate value was base64 encoded, using decoded value") certValue = decodedCertStr + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apim-apk-agent/internal/k8sClient/k8s_client.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apim-apk-agent/internal/k8sClient/k8s_client.go (3)
apim-apk-agent/pkg/eventhub/types/types.go (1)
KeyManagerConfig(261-283)apim-apk-agent/internal/constants/constants.go (3)
GatewayGroup(23-23)GatewayKind(24-24)GatewayName(22-22)apim-apk-agent/internal/loggers/logger.go (1)
LoggerK8sClient(48-48)
🔇 Additional comments (4)
apim-apk-agent/internal/k8sClient/k8s_client.go (4)
24-27: LGTM! Appropriate imports for certificate validation.The added standard library imports (crypto/x509, encoding/base64, encoding/pem) are necessary and appropriate for implementing robust certificate validation.
645-650: LGTM! Proper error handling for signature validation.The error-aware flow correctly handles validation failures by logging and returning the error, preventing invalid TokenIssuer CRs from being created with malformed certificates.
786-791: LGTM! Consistent error handling in update path.The update flow mirrors the create path with proper error handling, ensuring consistency across TokenIssuer lifecycle operations.
811-834: LGTM! Appropriate log level adjustments.The log level changes correctly reduce verbosity for successful operations (Debug level) while preserving error visibility (Error level for validation failures at lines 826 and 831). This aligns with the PR objective of reducing logging verbosity.
2 participants
Purpose
$subject
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.