Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughSeveral services related to certificate authority, HTTP routing, and TLS configuration were updated. The authorization rules are now instantiated with multiple API paths, incorporating a deprecated constant. CRL enablement is centralized, and signing record revocation skips invalid entries. Helm charts gained new helper functions to configure CRL and certificate settings. Routing endpoints were modified to reflect updated paths. Additionally, certificate validation in both client and server code now accepts a client flag, with new logic ensuring consistent checks for certificate file pairs, and the authorization rule creation now supports a variadic API paths argument. Changes
Sequence Diagram(s)sequenceDiagram
participant Svc as HTTP Service New
participant Auth as NewDefaultAuthorizationRules
Svc->>Auth: Call with (uri.API, uri.DeprecatedAPI)
loop For each API path
Auth->>Auth: Compile AuthArgs with regex
end
Auth-->>Svc: Return authorization rules map
sequenceDiagram
participant CM as CertManager
participant V as Config.Validate
participant TLS as GetClientTLSConfig
CM->>V: Validate(client flag)
alt client flag is true
Note over CM: Skip CertFile/KeyFile errors
else client flag is false
Note over CM: Require both CertFile and KeyFile present
end
CM->>CM: isSetClientCertificate check
CM->>TLS: Assemble TLS config with client certificate if set
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jkralik
force-pushed
the
jkralik/feature/crl-helm
branch
from
9861b3a to
b6c8994
Compare
jkralik
force-pushed
the
jkralik/feature/crl-helm
branch
from
fbef721 to
9b023f1
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
pkg/security/certManager/server/certManager.go (2)
32-55:⚠️ Potential issueMethod signature in server/certManager.go is out of sync with general package
The
Validatemethod in this file doesn't have theoptionalparameter, but it's being called with a boolean parameter. This indicates that the method signature here hasn't been updated to match the one in the general package.The method in this file should be updated to match the signature in the general package:
-func (c *Config) Validate() error { +func (c *Config) Validate(optional bool) error { if c.validated { return nil } caPoolArray, ok := strings.ToStringArray(c.CAPool) if !ok { return fmt.Errorf("caPool('%v') - unsupported", c.CAPool) } c.caPoolArray = urischeme.ToURISchemeArray(caPoolArray) - if !c.CAPoolIsOptional && len(caPoolArray) == 0 { + if !optional && !c.CAPoolIsOptional && len(caPoolArray) == 0 { return fmt.Errorf("caPool('%v') - is empty", c.CAPool) } if c.CertFile == "" { return fmt.Errorf("certFile('%v')", c.CertFile) } if c.KeyFile == "" { return fmt.Errorf("keyFile('%v')", c.KeyFile) } if err := c.CRL.Validate(); err != nil { return fmt.Errorf("CRL configuration is invalid: %w", err) } c.validated = true return nil }
85-89:⚠️ Potential issueInvalid call to Validate method without parameter
The code is calling the local
config.Validate()method without a parameter, but later callscfg.Validate(false)with a parameter. This inconsistency could lead to compatibility issues.Update the call to include the appropriate parameter:
if !config.validated { - if err := config.Validate(); err != nil { + if err := config.Validate(false); err != nil { return nil, err } }
🧹 Nitpick comments (8)
certificate-authority/service/service.go (1)
161-163: Centralized CRL enablement logic improves consistencyThe change centralizes the CRL enablement determination by setting it once based on clear conditions and immediately updating the relevant configuration property. This is better than having this logic scattered across the code.
Consider adding a comment explaining the conditions required for CRL to be enabled, as this is a critical security feature:
+ // CRL is enabled only when externalAddress is set and the database storage supports revocation list crlEnabled := externalAddress != "" && dbStorage.SupportsRevocationList() config.Signer.CRL.Enabled = crlEnabledpkg/security/tls/client.go (1)
36-37: Improved validation logic for certificate and key filesThe validation now correctly ensures that both key and certificate files are either both present or both absent, replacing the previous separate checks. This enforces proper certificate-key pair configuration.
However, note that the AI summary mentioned a new parameter
client boolbeing added to theValidatemethod signature, but it's not visible in this code segment. Verify if this parameter should be implemented as part of this change.pkg/security/certManager/general/certManager.go (1)
240-244: Thread-safe check for client certificates
Locking withinisSetClientCertificateensures concurrency safety. Consider renaming the method (e.g.,isClientCertConfiguredLocked) to reflect the locking explicitly.charts/plgd-hub/templates/_helpers.tpl (5)
123-129: Maintain consistency for internal CRL config
internalCRLConfigparallels the external certificate config. Double-check that both internal and external configurations share consistent naming and structure for clarity.
220-235: CRL authorization config extends logic
crlAuthorizationConfigmerges local CRL definitions with global ones. Consider documenting how local vs. global CRL settings merge to avoid confusion among chart users.
274-289: Unified approach for internal CRL
crlInternalConfigmerges both local and global internal CRL definitions. Align naming (e.g.,internalCRLConfigvs.crlInternalConfig) for consistency across all CRL definitions.
681-688: Dynamic CA pool key resolution
crlHttpTlsCaPoolKeyreturns "ca.crt" if a custom CA pool key is provided. This is simple and direct; consider verifying or logging the resolved filenames to assist debugging.
697-704: tls.key resolution
crlHttpTlsKeyKeyanalogously returns "tls.key". If there's potential for multiple keys, consider a naming pattern or documentation for large-scale usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (45)
.github/workflows/golangci-lint.ymlis excluded by!**/*.ymlcertificate-authority/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/certificate-authority/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/certificate-authority/deployment.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/certs/authorization/ca-pool.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/certs/authorization/crl.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/certs/coap/crl.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/certs/internal/crl.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/coap-gateway/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/coap-gateway/deployment.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/device-provisioning-service/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/device-provisioning-service/deployment.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/grpc-gateway/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/grpc-gateway/deployment.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/grpc-reflection/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/grpc-reflection/deployment.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/http-gateway/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/http-gateway/deployment.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/identity-store/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/identity-store/deployment.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/m2m-oauth-server/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/m2m-oauth-server/deployment.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/mock-oauth-server/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/mock-oauth-server/deployment.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/mongodb-standby-tool/job.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/resource-aggregate/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/resource-aggregate/deployment.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/resource-directory/deployment.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/snippet-service/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/templates/snippet-service/deployment.yamlis excluded by!**/*.yamlcharts/plgd-hub/values.yamlis excluded by!**/*.yamlcloud2cloud-connector/config.yamlis excluded by!**/*.yamlcloud2cloud-gateway/config.yamlis excluded by!**/*.yamlcoap-gateway/config.yamlis excluded by!**/*.yamldevice-provisioning-service/config.yamlis excluded by!**/*.yamlgrpc-gateway/config.yamlis excluded by!**/*.yamlhttp-gateway/config.yamlis excluded by!**/*.yamlhttp-gateway/web/package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonidentity-store/config.yamlis excluded by!**/*.yamlm2m-oauth-server/config.yamlis excluded by!**/*.yamlresource-aggregate/config.yamlis excluded by!**/*.yamlresource-directory/config.yamlis excluded by!**/*.yamlsnippet-service/config.yamlis excluded by!**/*.yamltest/helm/mock.plgd.cloud.yamlis excluded by!**/*.yamltools/grpc-reflection/config.yamlis excluded by!**/*.yaml
📒 Files selected for processing (14)
certificate-authority/service/http/service.go(1 hunks)certificate-authority/service/service.go(1 hunks)certificate-authority/service/uri/uri.go(1 hunks)certificate-authority/store/mongodb/signingRecords.go(1 hunks)charts/plgd-hub/templates/_helpers.tpl(4 hunks)http-gateway/web/packages/mock-server/src/routes/devices.js(1 hunks)http-gateway/web/src/containers/Certificates/constants.ts(1 hunks)http-gateway/web/src/containers/Devices/hooks.ts(1 hunks)pkg/net/http/auth.go(1 hunks)pkg/security/certManager/general/certManager.go(4 hunks)pkg/security/certManager/general/clientCertManager.go(1 hunks)pkg/security/certManager/server/certManager.go(1 hunks)pkg/security/tls/client.go(1 hunks)pkg/security/tls/crl.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
certificate-authority/store/mongodb/signingRecords.go (1)
Learnt from: Danielius1922
PR: plgd-dev/hub#1383
File: certificate-authority/store/mongodb/signingRecords.go:139-141
Timestamp: 2024-11-12T08:47:34.608Z
Learning: In `certificate-authority/store/mongodb/signingRecords.go`, within the `RevokeSigningRecords` method, expired signing records are deleted but not added to the revocation list.
🔇 Additional comments (24)
http-gateway/web/src/containers/Certificates/constants.ts (1)
2-2:Details
✅ Verification successful
API path updated to include service prefix - ensure consistent usage across codebase
The certificate endpoint URL has been updated to include the certificate-authority prefix. This change aligns with a broader restructuring of API paths in the application.
🏁 Script executed:
#!/bin/bash # Verify consistent usage of the new certificate authority endpoint across the codebase # Check for references to the old endpoint that might need updating rg -A 2 -B 2 "/api/v1/signing/records" --type ts --type jsLength of output: 1481
API Path Consistency Verified
- The certificate endpoint now correctly uses the
/certificate-authority/api/v1/signing/recordspath.- Verification across
constants.ts,devices.js, andhooks.tsconfirms consistent usage of the new endpoint.- No additional modifications are required.
certificate-authority/store/mongodb/signingRecords.go (1)
158-161: Handle legacy records with empty issuerIDs correctlyThis is a good defensive change to skip processing records with empty issuerIDs during revocation. This ensures that only valid records are included in revocation lists, preventing potential errors when adding to the revocation list.
The comment adequately explains the purpose of this check, making the code more maintainable.
certificate-authority/service/uri/uri.go (1)
8-8:Details
✅ Verification successful
Clear labeling of deprecated API path - ensure consistent transition strategy
Explicitly marking the old API path as deprecated is a good practice for managing API transitions. This constant will help maintain backward compatibility while the system transitions to the new path structure.
🏁 Script executed:
#!/bin/bash # Check how the DeprecatedAPI constant is used across the codebase # to ensure consistent handling of the API deprecation rg -A 2 -B 2 "DeprecatedAPI" --type goLength of output: 877
Deprecated API transition strategy confirmed
The constant
DeprecatedAPIis clearly defined incertificate-authority/service/uri/uri.goas"/api/v1"and its usage—particularly in the authorization rules setup incertificate-authority/service/http/service.go—demonstrates a consistent approach to API deprecation. No further changes are needed.http-gateway/web/src/containers/Devices/hooks.ts (1)
131-131:Details
❓ Verification inconclusive
Updated certificate endpoint to use new path structure
This change correctly implements the new API path structure for certificate-related operations, consistent with the changes in other parts of the application.
However, note that the provisioning record endpoint on line 144 still uses the old path structure (
${url}/api/v1/provisioning-records). Consider whether this endpoint should also be updated for consistency.
🏁 Script executed:
#!/bin/bash # Check if other provisioning-related endpoints still use the old path pattern # or if they should also be updated rg -A 2 -B 2 "/api/v1/provisioning-records" --type ts --type jsLength of output: 1416
Action: Confirm Endpoint Consistency
The certificate endpoint update to
/certificate-authority/api/v1/signing/recordshas been verified and is correct. However, our search confirms that the provisioning records endpoint still uses the legacy/api/v1/provisioning-recordsin multiple parts of the codebase (e.g., inhttp-gateway/web/src/containers/DeviceProvisioning/constants.ts, the hooks file, and in the mock server routes). Please verify whether the provisioning records endpoint should remain unchanged or if it also needs to be updated to a new path structure for consistency.pkg/security/tls/crl.go (1)
113-113:Details
❓ Verification inconclusive
Added
omitemptytag for more efficient serializationAdding the
omitemptytag to the HTTP field ensures that nil values aren't serialized to JSON/YAML, resulting in cleaner output and reduced payload size when CRL's HTTP configuration isn't needed.Let's check if there are other fields in this package that might benefit from similar
omitemptytag additions:
🏁 Script executed:
#!/bin/bash # Find other struct fields in this package that could benefit from omitempty tags rg -A 1 -B 1 "type \w+Config struct" pkg/security/tls/ rg "json:\"[^,]+\"" pkg/security/tls/ | grep -v "omitempty"Length of output: 2396
Serializer Tags Consistency: Review Additional Candidates for
omitemptyThe recent change to add the
omitemptytag for theHTTPfield inCRLConfigimproves serialization when that configuration isn’t supplied. A search of the package revealed several other fields that currently lack theomitemptytag and might similarly benefit from it:
In
pkg/security/tls/client.gowithinClientConfig:
CAPool(json:"caPool") – Consider addingomitemptysince this value may be nil.KeyFile(json:"keyFile") – Evaluate if omitting empty strings is appropriate.CertFile(json:"certFile") – Ditto as above.UseSystemCAPool(json:"useSystemCaPool") – Assess if a false default should be omitted.CRL(json:"crl") – Determine if its absence should result in omission.In
pkg/security/tls/crl.go:
- Several fields (e.g.,
MaxIdleConns,MaxConnsPerHost,MaxIdleConnsPerHost,IdleConnTimeout,Timeout, and possibly others within related config structs) are defined withoutomitempty. Please review whether omitting these fields when they are set to zero is desirable for your serialization use cases.Please review and confirm that adding
omitemptyto these fields won’t inadvertently affect consumers relying on the current behavior.http-gateway/web/packages/mock-server/src/routes/devices.js (1)
166-166:Details
✅ Verification successful
API path updated for better service organization
The endpoint has been updated to follow a more structured approach by prefixing the path with the service name (
certificate-authority). This change aligns with API best practices for better discoverability and organization.Let's check if this endpoint change is consistent with other client code that might need to be updated:
🏁 Script executed:
#!/bin/bash # Search for references to the old API path that might need updating rg "/api/v1/signing/records" --type js --type ts # Compare with new path to ensure this change is consistent rg "/certificate-authority/api/v1/signing/records" --type js --type tsLength of output: 955
Verification Complete: Endpoint References Consistent
The updated endpoint
/certificate-authority/api/v1/signing/recordsis used consistently across the codebase:
- Defined in
http-gateway/web/packages/mock-server/src/routes/devices.js- Referenced in both
http-gateway/web/src/containers/Certificates/constants.tsandhttp-gateway/web/src/containers/Devices/hooks.tsNo further changes are required.
pkg/security/certManager/server/certManager.go (1)
99-99:Details
✅ Verification successful
Validate method signature updated to support conditional validation
The update to pass
falseto theValidatemethod indicates that the validation logic has been enhanced to support conditional validation. This allows for more flexibility in certificate validation across different contexts.Let's examine the changes to the
Validatemethod signature and implementation to understand the meaning of this new parameter:
🏁 Script executed:
#!/bin/bash # Search for the updated Validate method in the general package rg -A 20 "func \(c \*Config\) Validate\(optional bool\)" pkg/security/certManager/general/ # Look for all calls to this method to ensure consistency rg "Validate\((true|false)\)" pkg/security/certManager/Length of output: 340
Action Required: Confirming Conditional Validation Usage
The updated signature of the
Validatemethod now accepts a boolean parameter to control conditional validation. Our investigation confirms that:
- In
pkg/security/certManager/general/clientCertManager.go, the method is correctly called ascfg.Validate(true).- In
pkg/security/certManager/server/certManager.go, the method is correctly called ascfg.Validate(false).This consistent usage across both client and server contexts verifies that the update supports flexible certificate validation as intended.
certificate-authority/service/http/service.go (1)
40-40:Details
✅ Verification successful
Added support for deprecated API paths in authorization rules
The change correctly adds
uri.DeprecatedAPIto the authorization rules, ensuring backward compatibility while moving to new API paths.
🏁 Script executed:
#!/bin/bash # Verify that DeprecatedAPI constant exists and is used properly echo "Checking for DeprecatedAPI constant definition..." fd --type f "\.go$" | xargs grep -l "DeprecatedAPI.*=" echo "Checking for usages of DeprecatedAPI:" rg "DeprecatedAPI" -A 2 -B 2Length of output: 1159
Verified: Deprecated API constant usage is correct
The changes add backward compatibility as intended. The constant
DeprecatedAPIis properly defined incertificate-authority/service/uri/uri.goand is correctly used incertificate-authority/service/http/service.gowith the call topkgHttp.NewDefaultAuthorizationRules(uri.API, uri.DeprecatedAPI). No further modifications are needed.pkg/net/http/auth.go (2)
21-30: Improved authorization rules to support multiple API pathsThe function has been enhanced to accept multiple API paths via a variadic parameter, making it more flexible. The implementation correctly handles each API path and adds proper error handling for empty API paths.
32-36: Refactored return statement to reduce code redundancyThe return statement has been simplified to reuse the same authorization arguments slice for all HTTP methods, which follows the DRY principle and makes the code more maintainable.
pkg/security/certManager/general/clientCertManager.go (1)
53-53:Details
✅ Verification successful
Updated Validate method call with client flag parameter
The code now passes
trueto theValidatemethod, indicating this is a client certificate configuration. This aligns with changes in the certificate validation system.
🏁 Script executed:
#!/bin/bash # Verify that the Validate method in Config accepts a bool parameter echo "Checking for Validate method implementation with bool parameter:" rg "func \(\w+\s+\*?Config\) Validate\(.*bool" -A 5Length of output: 729
Client Certificate Validation Confirmed
The verification confirms that the
Validatemethod inpkg/security/certManager/general/certManager.goaccepts a boolean parameter. The update to passtrueinpkg/security/certManager/general/clientCertManager.gocorrectly signals a client certificate configuration. No further modifications are necessary.pkg/security/certManager/general/certManager.go (3)
41-55: Revisit the optional certificate logic inValidate(client bool)
By makingCertFileandKeyFileoptional only whenclient == true, the current approach implies that client certificates are always optional but server certificates are strictly required. Verify that this behavior aligns with actual requirements for mutual TLS, as client certificates might also need enforcement in some contexts.
191-195: Conditional client certificate assignment looks correct
Proactively assigningGetClientCertificatebased on whether the client certificate is set is a good approach. This avoids unnecessary loading of certificates when they are not present.
272-272: Double-check the AND condition when loading certs
Line 272 checksif a.config.KeyFile == "" && a.config.CertFile == "". This omits certificate loading if both are empty, meaning one non-empty file triggers loading. Confirm that partial certificate data is intentional and does not break mutual TLS.charts/plgd-hub/templates/_helpers.tpl (10)
116-121: Ensure alignment of external certificate logic
The newexternalCertificateConfigreferences.Values.extraCAPool.external. Confirm that external CA pools or file paths exist where required.
185-188: Conditional CRL inclusion
The CRL block is only included if$httpTls.crlis defined. Ensure you have test scenarios where CRL is absent to confirm the fallback logic works properly.
190-219: Centralized CRL configuration
Definingplgd-hub.CRLConfigto handle CRL parameters in one place is neat. Verify that all CRL values (e.g., timeouts, concurrency) are tested with realistic Helm values to catch misconfigurations early.
237-252: Fallback to certificate authority config
CRLConfigFromCertificateAuthorityprovides a fallback if CRL is not explicitly enabled. Ensure that using default coap-based timeouts for CRL is acceptable or documented.
254-272: Conditional CoAP CRL configuration
crlCoapConfigfirst checks if CRL is enabled, then falls back to eitherCRLConfigor certificate authority. Confirm there's no unintended double fallback or conflicting CRL.
672-679: Guard check for CRL authorization
crlAuthorizationEnabledchecks for a valid CA pool. Confirm that a missing CA pool gracefully disables CRL, and that the user is aware CRL won't be enforced.
689-696: tls.crt resolution
Similar approach forcrlHttpTlsCrtKeyreturns "tls.crt". Confirm no overlap with possibly existing user-defined certificate naming conventions in multi-tenant deployments.
705-712: Check for internal CRL pool
crlInternalEnabledis consistent with the rest of the CRL detection. Verify that enabling internal CRL doesn't conflict with other certificate managers or CRL settings.
714-721: CoAP-level CRL enablement
crlCoapEnabledfunnels the logic for CoAP CRL. Ensure that downstream references to this toggle handle partial or incomplete CRL data.
774-781: CRL volume and mount definitions
crlVolumeandcrlMountre-use the existing extra CA pool logic. This approach is DRY and coherent. Good job.
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
jkralik
force-pushed
the
jkralik/feature/crl-helm
branch
from
16bcdef to
d7e7895
Compare
|
1 participant
No description provided.