Implement Certificate Revocation List (1) #1383
Conversation
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 37 files out of 119 files are above the max files limit of 75. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughThe changes in this pull request include significant updates across various files related to the certificate authority service. Key modifications involve enhancements to Nginx configuration for improved routing and SSL handling, updates to protocol documentation, and the introduction of new fields in data structures for better tracking of certificate statuses. Additionally, several new methods and validation checks have been implemented, particularly for handling revocation lists and signing records. The overall structure and functionality of the service have been refined to support these enhancements effectively. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Danielius1922
force-pushed
the
adam/feature/crl-1
branch
2 times, most recently
from
d3acc56
to
36e6847
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 41
🧹 Outside diff range and nitpick comments (52)
certificate-authority/service/http/config.go (1)
16-16: Consider adding documentation for the CRLEnabled field.While the field name is self-explanatory, adding a comment would help future maintainers understand its purpose, especially since it's an internal configuration that affects runtime behavior.
+ // CRLEnabled controls whether Certificate Revocation List (CRL) functionality is enabled. + // This field is set programmatically and excluded from serialization. CRLEnabled bool `yaml:"-" json:"-"`certificate-authority/service/grpc/deleteSigningRecords.go (1)
14-27: Enhance security measures for certificate revocation.Certificate revocation is a security-critical operation. Consider implementing these security enhancements:
- Add revocation reason to the request parameters
- Implement detailed audit logging
- Add specific access control checks for revocation operations
Example enhancements:
type DeleteSigningRecordsRequest struct { + reason RevocationReason + comment string // Optional explanation // ... existing fields } func (s *CertificateAuthorityServer) DeleteSigningRecords(ctx context.Context, req *pb.DeleteSigningRecordsRequest) (*pb.DeletedSigningRecords, error) { owner, err := ownerToUUID(ctx, s.ownerClaim) if err != nil { return nil, s.logger.LogAndReturnError(errDeleteSigningRecords(err)) } + // Log revocation attempt + s.logger.Info("Certificate revocation requested", + "owner", owner, + "reason", req.reason, + "comment", req.comment) count, err := s.store.RevokeSigningRecords(ctx, owner, req) if err != nil { return nil, s.logger.LogAndReturnError(errDeleteSigningRecords(err)) } + // Log successful revocation + s.logger.Info("Certificates revoked successfully", + "owner", owner, + "count", count) return &pb.DeletedSigningRecords{ Count: count, }, nil }certificate-authority/service/http/signIdentityCertificate_test.go (1)
36-36: Consider adding test cases for certificate revocation scenarios.While the URI constant is correctly used, since this PR implements Certificate Revocation List functionality, consider adding test cases that verify:
- Signing requests for previously revoked certificates
- Signing requests with revocation status checks
Would you like me to help draft additional test cases for these scenarios?
certificate-authority/service/http/requestHandler.go (1)
22-24: Consider adding field documentation.While the field types are appropriate, adding documentation comments for the new fields would improve code maintainability:
type RequestHandler struct { config *Config mux *runtime.ServeMux + // cas handles certificate authority operations via gRPC cas *grpcService.CertificateAuthorityServer + // store provides persistence for certificate-related data store store.Store }certificate-authority/test/revocationList.go (2)
1-16: Add package documentation.Consider adding a package comment that describes the purpose and usage of these test utilities. This will help other developers understand how to use these helpers effectively in their tests.
+// Package test provides utilities for testing Certificate Revocation List (CRL) functionality. +// It includes helpers for generating test data, managing certificate serials, and validating +// revocation list operations. package test
23-42: Add documentation and input validation.The helper functions would benefit from proper documentation and input validation to improve usability and robustness.
Consider these improvements:
+// GetIssuerID generates a formatted issuer ID using a zero-padded 12-digit suffix. +// The format follows: "49000000-0000-0000-0000-XXXXXXXXXXXX" func GetIssuerID(i int) string { + if i < 0 { + i = 0 + } return fmt.Sprintf("49000000-0000-0000-0000-%012d", i) } +// GetCertificateSerial generates or retrieves a unique certificate serial number +// for the given index. The serial numbers are cached to ensure consistency. func GetCertificateSerial(i int) string { // ... existing code ... } +// GetCertificate creates a test RevocationListCertificate with the specified +// certificate index, revocation time, and expiration time. func GetCertificate(c int, rev, exp time.Time) *store.RevocationListCertificate { + if rev.After(exp) { + rev = exp + } return &store.RevocationListCertificate{ Serial: GetCertificateSerial(c), ValidUntil: pkgTime.UnixNano(exp), Revocation: pkgTime.UnixNano(rev), } }certificate-authority/pb/service.proto (2)
41-44: LGTM! Consider documenting the dual endpoints.The additional binding provides a more service-oriented path while maintaining backward compatibility.
Consider adding a comment explaining the purpose of dual endpoints and any future deprecation plans for the legacy endpoint.
Line range hint
1-91: Document CRL implementation details.As this is part 1 of the Certificate Revocation List implementation, consider:
- Adding OpenAPI documentation for CRL-specific endpoints
- Documenting the CRL check process in the service documentation
- Specifying the CRL distribution point format and update frequency
- Describing how to handle certificate validation against the CRL
Consider implementing:
- CRL caching mechanism
- Periodic CRL updates
- Metrics for revocation operations
- Integration with OCSP if needed
device-provisioning-service/Makefile (1)
57-59: Consider documenting the proto output path changes.Since this changes where the generated files are placed, it would be helpful to update the project documentation to reflect this change. This ensures other developers are aware of the new file locations.
Would you like me to help draft documentation updates for the README or contributing guidelines?
Also applies to: 60-63
certificate-authority/service/grpc/deleteSigningRecords_test.go (3)
25-28: Consider defining ownerClaim as a package constantThe "sub" string appears to be a standard claim identifier. Consider defining it as a package-level constant for better maintainability and reusability.
+const defaultOwnerClaim = "sub" func TestCertificateAuthorityServerDeleteSigningRecords(t *testing.T) { owner := events.OwnerToUUID("owner") - const ownerClaim = "sub" + ownerClaim := defaultOwnerClaim
53-62: Enhance error assertion in the missing token testWhile testing for missing token is crucial, consider making the error assertion more specific to verify the exact error type or message.
{ name: "missing token with ownerClaim in ctx", args: args{ req: &pb.DeleteSigningRecordsRequest{ IdFilter: []string{r.GetId()}, }, ctx: context.Background(), }, wantErr: true, + wantErrMsg: "missing token in context", },
100-100: Consider more robust HTTP host URL constructionThe URL construction with string concatenation could be improved using url.Parse or similar utilities to ensure proper URL formatting and escaping.
-"https://"+config.CERTIFICATE_AUTHORITY_HTTP_HOST +url.Parse(fmt.Sprintf("https://%s", config.CERTIFICATE_AUTHORITY_HTTP_HOST))certificate-authority/service/grpc/server_test.go (1)
86-86: LGTM! Consider adding CRL-specific test cases.The addition of the HTTP host parameter is correct and aligns with the implementation of Certificate Revocation List functionality. However, since this is part of CRL implementation, consider adding test cases that verify:
- The CRL endpoint is properly configured with the provided HTTP host
- The server correctly serves the CRL at the expected URL
- The CRL is accessible and properly formatted as per RFC 5280
Would you like me to help generate additional test cases for the CRL functionality?
certificate-authority/service/cleanDatabase_test.go (1)
Line range hint
81-93: Consider enhancing test assertions for new fields.While the test verifies basic record cleanup, consider adding explicit assertions to verify the Serial and IssuerId fields in the retrieved records before cleanup.
require.NoError(t, errR) got = append(got, r) } require.Len(t, got, 1) + require.Equal(t, big.NewInt(42).String(), got[0].GetCredential().GetSerial()) + require.Equal(t, "42424242-4242-4242-4242-424242424242", got[0].GetCredential().GetIssuerId()) time.Sleep(4 * time.Second)certificate-authority/service/grpc/getSigningRecords_test.go (1)
Line range hint
52-89: Consider adding more test cases for CRL scenarios.The current test structure is good, but consider adding test cases for:
- Revoked certificates
- Expired certificates
- Multiple serial numbers
- Invalid issuer IDs
Would you like me to help generate additional test cases for these scenarios?
certificate-authority/service/grpc/server.go (1)
31-39: Consider validating the CRL server address format.For enhanced security and reliability, consider adding validation for the CRL server address format before using it. This could prevent runtime issues with malformed addresses.
Suggested validations:
- Check for empty string
- Validate URL/address format
- Consider adding TLS verification settings if HTTPS is used
Also applies to: 105-105
certificate-authority/service/http/signCertificate_test.go (1)
Line range hint
84-104: Consider enhancing error handling test coverage.The
httpDoSignfunction handles errors well, but consider adding test cases for:
- Network failures
- Invalid token scenarios
- Malformed response handling
- Timeout scenarios
This would make the test suite more robust for the Certificate Revocation List implementation.
Example test enhancement:
func TestCertificateAuthorityServerSignCSRErrors(t *testing.T) { tests := []struct { name string setup func(*http.Request) wantErr error }{ { name: "network timeout", setup: func(r *http.Request) { // Setup timeout scenario }, wantErr: context.DeadlineExceeded, }, // Add more error scenarios } // Test implementation }certificate-authority/store/revocationList_test.go (2)
12-54: Consider adding more edge cases to strengthen test coverage.While the current test cases cover basic validation scenarios, consider adding these additional cases to improve robustness:
- Invalid serial number format (e.g., special characters)
- Future revocation time
- Negative revocation time
Example addition:
func TestRevocationListCertificateValidate(t *testing.T) { tests := []struct { name string input store.RevocationListCertificate wantErr bool }{ // ... existing tests ... + { + name: "Future revocation time", + input: store.RevocationListCertificate{ + Serial: "12345", + Revocation: time.Now().Add(24 * time.Hour).UnixNano(), + }, + wantErr: true, + }, + { + name: "Invalid serial format", + input: store.RevocationListCertificate{ + Serial: "12345#$%", + Revocation: time.Now().UnixNano(), + }, + wantErr: true, + }, }
44-53: Consider verifying specific error messages.The test loops currently only check for error presence/absence. Consider enhancing them to verify the specific error messages returned by the validation functions. This would ensure that appropriate error messages are provided to users and help with debugging.
Example enhancement:
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { err := tt.input.Validate() if tt.wantErr { - require.Error(t, err) + require.ErrorContains(t, err, tt.expectedErrMsg) return } require.NoError(t, err) }) }Also applies to: 135-144
certificate-authority/service/config.go (2)
69-71: Consider reordering fields for better readability.Consider ordering the fields more logically, with the primary address field first:
type HTTPConfig struct { - ExternalAddress string `yaml:"externalAddress" json:"externalAddress"` Addr string `yaml:"address" json:"address"` + ExternalAddress string `yaml:"externalAddress" json:"externalAddress"` Server httpServer.Config `yaml:",inline" json:",inline"` }
69-69: Document ExternalAddress usage for CRL.Since this is part of the CRL implementation, please add a comment explaining:
- The purpose of ExternalAddress in the context of CRL
- The expected format (e.g., https://example.com)
- How it will be used in CRL distribution points
type HTTPConfig struct { + // ExternalAddress is the publicly accessible URL of the service. + // It must be an HTTPS URL (e.g., https://ca.example.com) and will be used + // as the base URL for CRL distribution points. ExternalAddress string `yaml:"externalAddress" json:"externalAddress"`certificate-authority/service/grpc/config_test.go (2)
12-50: Consider adding more test cases for comprehensive coverage.While the current test cases cover basic scenarios, consider adding:
- Test case for exactly 1-minute expiration (edge case)
- Test case for negative ExpiresIn value
- Test case for maximum allowed expiration time
- Test case for empty ExternalAddress when enabled
This will ensure more robust validation of the CRL configuration.
Would you like me to provide the additional test cases?
52-82: Consider using a more realistic invalid CAPool test case.The current test uses an integer (42) for invalid CAPool type. Consider using a more realistic invalid value like a single string instead of a slice, which better represents potential real-world misconfiguration.
- CAPool: 42, + CAPool: "ca1.pem", // single string instead of []stringcertificate-authority/test/service.go (1)
25-43: Consider adding test cases for CRL edge cases.Since this is a test configuration file, consider adding test cases for:
- CRL expiration and renewal
- Database failover scenarios
- Invalid CRL configurations
- Different time zones handling for expiration
This will ensure robust testing of the CRL implementation.
Would you like me to help create additional test cases for these scenarios?
device-provisioning-service/pb/service.proto (1)
Line range hint
75-144: Consider documenting the API changes in OpenAPI specification.The HTTP binding changes look good, but since these endpoints are part of the Certificate Revocation List implementation:
- Consider updating the OpenAPI/Swagger documentation to reflect these changes
- Ensure that the error responses are properly documented for certificate-related failures
- Consider adding examples in the OpenAPI specification for the request/response bodies
This will help maintain API consistency and improve developer experience.
certificate-authority/pb/signingRecords_test.go (2)
11-95: Add test cases for temporal validation and enhance test data.The test structure is well-organized with good coverage of basic validation cases. Consider these improvements:
- Add test case for expired certificate (ValidUntilDate < Date)
- Add test case for future Date
- Consider using constants for test timestamps and certificate data
Example additional test case:
{ name: "Expired certificate", input: &pb.CredentialStatus{ Date: 1669462400000000000, // Later date ValidUntilDate: 1659462400000000000, // Earlier date CertificatePem: "valid-cert", Serial: "1234567890", IssuerId: uuid.New().String(), }, wantErr: true, },
97-189: Enhance SigningRecord validation test coverage.The test structure is well-organized with good basic validation coverage. Consider these improvements:
- Add test case for nil Credential field
- Add test cases for string field length boundaries
- Add test case for special characters in CommonName and Owner fields
Example additional test cases:
{ name: "Nil credential", input: &pb.SigningRecord{ Id: uuid.New().String(), Owner: "owner", CommonName: "common_name", DeviceId: uuid.New().String(), Credential: nil, }, wantErr: true, }, { name: "Very long common name", input: &pb.SigningRecord{ Id: uuid.New().String(), Owner: "owner", CommonName: strings.Repeat("a", 256), // Assuming there's a reasonable max length DeviceId: uuid.New().String(), Credential: validCredential, }, wantErr: true, },certificate-authority/service/config_test.go (1)
158-187: Consider adding more test cases for comprehensive validation.Given this is part of a certificate authority service, consider adding these test cases:
- Empty strings for both fields
- Malformed URLs (e.g., "http://[::1]:invalid")
- Both fields invalid simultaneously
- URLs with invalid schemes
Example addition:
{ name: "invalid address", args: args{ cfg: func() service.HTTPConfig { cfg := test.MakeHTTPConfig() cfg.Addr = "invalid" return cfg }(), }, wantErr: true, }, + { + name: "both fields invalid", + args: args{ + cfg: func() service.HTTPConfig { + cfg := test.MakeHTTPConfig() + cfg.Addr = "invalid" + cfg.ExternalAddress = "invalid" + return cfg + }(), + }, + wantErr: true, + }, + { + name: "empty fields", + args: args{ + cfg: func() service.HTTPConfig { + cfg := test.MakeHTTPConfig() + cfg.Addr = "" + cfg.ExternalAddress = "" + return cfg + }(), + }, + wantErr: true, + },certificate-authority/service/service.go (1)
Line range hint
103-123: Add integration tests for CRL functionality.The CRL implementation involves multiple components working together. Consider:
- Adding integration tests that verify the complete CRL workflow
- Testing different configuration scenarios (enabled/disabled)
- Security testing for the CRL distribution endpoint
- Performance testing for CRL generation and distribution
Consider implementing:
- Caching for CRL to improve performance
- Monitoring metrics for CRL operations
- Health checks for the CRL endpoint
Would you like me to help create integration tests or monitoring metrics for the CRL functionality?
certificate-authority/service/http/revocationList_test.go (5)
34-34: Simplify time conversion logic.The double Unix timestamp conversion
pkgTime.Unix(pkgTime.Unix(0, cert.Revocation).Unix(), 0)seems unnecessarily complex and could be simplified.Consider this simpler alternative:
-RevocationTime: pkgTime.Unix(pkgTime.Unix(0, cert.Revocation).Unix(), 0).UTC(), +RevocationTime: time.Unix(0, cert.Revocation).UTC(),
56-59: Fix loop variable scope and magic number.Two issues to address:
- The loop variable
iin range loops should be properly scoped to prevent potential issues in concurrent operations- The magic number
10should be defined as a constant with a meaningful nameConsider these improvements:
+const defaultTestCertificateCount = 10 -for i := range 10 { +for i := range defaultTestCertificateCount { + i := i // proper scope for loop variable rlc := test.GetCertificate(i, now, now.Add(time.Hour)) rl1.Certificates = append(rl1.Certificates, rlc) }Also applies to: 68-70
102-107: Enhance test coverage and validation.Consider adding these improvements to the test cases:
- Add a test case for expired CRL scenario
- Validate specific error types/messages
- Verify HTTP response status codes
Example additions:
type args struct { issuer string } tests := []struct { name string args args verifyCRL func(crl *x509.RevocationList) wantErr bool + wantStatus int + wantErrMsg string } // ... existing test cases ... +{ + name: "expired", + args: args{ + issuer: test.GetIssuerID(2), + }, + wantStatus: http.StatusGone, + wantErr: true, + wantErrMsg: "revocation list has expired", +},Also applies to: 147-147
152-157: Verify HTTP response status code.The test should validate the HTTP response status code before attempting to parse the response body.
Add status code verification:
httpResp := httpgwTest.HTTPDo(t, request) +require.Equal(t, tt.wantStatus, httpResp.StatusCode) respBody, err := io.ReadAll(httpResp.Body)
102-147: Add security-focused test cases.Consider adding test cases for security-critical scenarios:
- Malformed issuer IDs
- Very large certificate lists
- Invalid signature verification
- Tampered revocation times
These additional test cases would help ensure the robustness of the CRL implementation against potential security issues.
certificate-authority/service/grpc/signCertificate_test.go (1)
153-197: Add documentation for security implications.While the test case correctly validates that a device cannot obtain a new certificate with a different private key, it would be helpful to add comments explaining:
- The security implications of this test
- Why allowing different keys for the same device ID would be a security risk
Additionally, consider using a more specific error check on line 197:
- require.Error(t, err) + require.ErrorContains(t, err, "expected error message about invalid key")certificate-authority/store/mongodb/revocationList_test.go (4)
241-241: Use count variable instead of magic number for WaitGroup.The WaitGroup is initialized with a hardcoded value of 10, which should match the
countvariable for consistency and maintainability.- wg.Add(10) + wg.Add(count)
249-249: Define retry count as a constant.The retry count of 100 should be defined as a constant at package level for better maintainability and clarity.
+const maxRetries = 100 - for range 100 { + for range maxRetries {
278-278: Consider inline test data setup for better readability.The test relies on external helper
test.AddRevocationListToStorewhich makes it harder to understand the test data without context. Consider setting up test data inline or documenting the expected structure returned by the helper function.
1-341: Consider adding benchmark tests.Given the concurrent nature of the operations and the importance of performance in certificate revocation list handling, consider adding benchmark tests to measure and track the performance of these operations, especially for parallel updates.
Would you like me to help create benchmark tests for these operations?
certificate-authority/store/mongodb/signingRecords_test.go (1)
44-45: Enhance test coverage for new credential fields.The new
SerialandIssuerIdfields are using hardcoded values across all test cases. Consider:
- Testing different serial number formats and lengths
- Testing invalid UUID formats for
IssuerId- Adding edge cases (empty strings, very large numbers)
Example improvement for better test coverage:
-Serial: big.NewInt(42).String(), -IssuerId: "42424242-4242-4242-4242-424242424242", +Serial: fmt.Sprintf("%x", big.NewInt(test.GetTestCaseIdx()).Exp(big.NewInt(2), big.NewInt(64), nil)), +IssuerId: uuid.New().String(),Also applies to: 64-65, 83-84, 217-218, 283-284, 326-327, 341-342, 356-357, 371-372
certificate-authority/store/cqldb/signingRecords_test.go (1)
35-36: LGTM: Consistent implementation of CRL-related fieldsThe new
SerialandIssuerIdfields are consistently implemented across all test cases with appropriate test data.Consider adding these test cases to improve coverage:
- Validation for invalid serial numbers
- Explicit assertions to verify the new fields in test results
Example test case for invalid serial:
{ name: "invalid serial number", args: args{ sub: &store.SigningRecord{ Id: id1, Owner: "owner", CommonName: "commonName", Credential: &pb.CredentialStatus{ Serial: "invalid", // Invalid serial number IssuerId: "42424242-4242-4242-4242-424242424242", }, }, }, wantErr: true, },Also applies to: 55-56, 110-111, 130-131, 150-151, 169-170, 294-295, 310-311, 326-327, 363-364, 408-409, 423-424, 438-439, 574-575
certificate-authority/pb/doc.html (2)
349-349: Fix typo in DeleteSigningRecords descriptionThere's a typo in "certficate" that should be "certificate".
- <td><p>Revoke signed certficate or delete expired signed certificate records.</p></td> + <td><p>Revoke signed certificate or delete expired signed certificate records.</p></td>Also applies to: 356-356
551-551: Enhance DeletedSigningRecords descriptionThe description "Revoke or delete certificates" is concise but could be more informative about the difference between revocation and deletion.
Consider expanding the description to clarify:
- <p>Revoke or delete certificates</p> + <p>Response for certificate revocation or deletion operations. Revocation marks certificates as invalid while deletion removes expired certificate records.</p>certificate-authority/store/mongodb/store.go (1)
20-20: Ensureloggeris utilized within theStoremethodsThe
loggerfield has been added to theStorestruct but is not currently used within the provided methods. To make effective use of the logging capabilities, consider integratingloggerintoStoremethods where appropriate to log important events and errors.Also applies to: 91-92
certificate-authority/store/store.go (2)
47-47: Maintain Consistent Method Comment StyleFor consistency, consider rephrasing the comment for
SupportsRevocationListto start with a verb in the third-person singular form, matching the style of other method comments.Apply this diff to adjust the comment:
-// Check if the implementation supports the RevocationList feature +// SupportsRevocationList checks if the implementation supports the RevocationList feature.
51-51: Enhance Clarity inUpdateRevocationListMethod CommentThe current comment can be made clearer for better understanding. Rewriting it will help convey its functionality more effectively.
Apply this diff to improve the comment:
-// UpdateRevocationList updates revocation list number and validity and adds certificates to revocation list. Returns the updated revocation list. +// UpdateRevocationList updates the revocation list number and validity, adds certificates to the revocation list, and returns the updated revocation list.certificate-authority/service/http/revocationList.go (1)
67-69: Validate issuer ID and handle parsing errors appropriatelyThe parsing of
issuerIDusinguuid.Parseis correct. Returning the error directly is acceptable, but consider wrapping the error with additional context for better error tracing.Apply this diff to provide more context in the error:
if _, err := uuid.Parse(issuerID); err != nil { - return err + return fmt.Errorf("invalid issuer ID '%s': %w", issuerID, err) }certificate-authority/store/revocationList.go (1)
62-65: Address the TODO regarding expiration check inIsExpiredmethodThere's a TODO comment indicating the need to consider a delta when checking expiration. This could be important to handle network delays or clock skew between systems.
Would you like assistance in implementing an expiration check that accounts for a tolerance delta? For example:
const expirationDelta = 5 * time.Minute func (rl *RevocationList) IsExpired() bool { return rl.ValidUntil <= time.Now().Add(expirationDelta).UnixNano() }pkg/security/certificateSigner/certificateSigner.go (1)
Line range hint
66-82: Consider providing more descriptive error messagesIn the
parseCertificateRequestfunction, the error message"pem not found"might be unclear. Consider changing it to"failed to decode CSR PEM block"to provide more context and aid in debugging.certificate-authority/store/mongodb/signingRecords.go (1)
181-186: Remove unnecessary error check formongo.ErrNilDocumentThe
Findmethod does not returnmongo.ErrNilDocumentwhen no documents are found; instead, it returns a cursor with no results. The check formongo.ErrNilDocumentafterFindis unnecessary and can be removed to simplify the code.Apply this diff to remove the unnecessary error check:
if err != nil { - if errors.Is(err, mongo.ErrNilDocument) { - return nil - } return err }certificate-authority/service/grpc/signCertificate.go (1)
162-164: Improve the clarity of the error message when credential isnil.The error message "cannot create signing record" may not clearly indicate that
credentialisnil. Providing more specific information can aid in debugging.Update the error message for clarity:
- return nil, logger.LogAndReturnError(status.Errorf(codes.InvalidArgument, fmtError, errors.New("cannot create signing record"))) + return nil, logger.LogAndReturnError(status.Errorf(codes.Internal, fmtError, errors.New("credential is nil; failed to create signing record")))certificate-authority/store/mongodb/revocationList.go (1)
225-226: Ensure time synchronization and correct expiration checksThe condition
!rl.IsExpired()depends on the system's current time. To prevent issues caused by time discrepancies between systems, ensure that the time used for expiration checks is consistent and consider time skew.Recommendation:
- Use a time source that is consistent across your systems.
- If possible, include leeway in expiration checks to account for minimal time differences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (10)
certificate-authority/config.yamlis excluded by!**/*.yamlcertificate-authority/pb/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocertificate-authority/pb/service.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gocertificate-authority/pb/service.swagger.jsonis excluded by!**/*.jsoncertificate-authority/pb/service_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocertificate-authority/pb/signingRecords.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocharts/plgd-hub/templates/certificate-authority/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/values.yamlis excluded by!**/*.yamldevice-provisioning-service/pb/service.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.godevice-provisioning-service/pb/service.swagger.jsonis excluded by!**/*.json
📒 Files selected for processing (55)
- bundle/nginx/nginx.conf.template (1 hunks)
- certificate-authority/pb/README.md (3 hunks)
- certificate-authority/pb/doc.html (7 hunks)
- certificate-authority/pb/service.proto (2 hunks)
- certificate-authority/pb/signingRecords.go (3 hunks)
- certificate-authority/pb/signingRecords.proto (2 hunks)
- certificate-authority/pb/signingRecords_test.go (1 hunks)
- certificate-authority/service/cleanDatabase_test.go (5 hunks)
- certificate-authority/service/config.go (3 hunks)
- certificate-authority/service/config_test.go (1 hunks)
- certificate-authority/service/grpc/config.go (2 hunks)
- certificate-authority/service/grpc/config_test.go (1 hunks)
- certificate-authority/service/grpc/deleteSigningRecords.go (1 hunks)
- certificate-authority/service/grpc/deleteSigningRecords_test.go (4 hunks)
- certificate-authority/service/grpc/getSigningRecords.go (1 hunks)
- certificate-authority/service/grpc/getSigningRecords_test.go (5 hunks)
- certificate-authority/service/grpc/server.go (2 hunks)
- certificate-authority/service/grpc/server_test.go (1 hunks)
- certificate-authority/service/grpc/signCertificate.go (3 hunks)
- certificate-authority/service/grpc/signCertificate_test.go (4 hunks)
- certificate-authority/service/grpc/signIdentityCertificate_test.go (3 hunks)
- certificate-authority/service/grpc/signer.go (6 hunks)
- certificate-authority/service/grpc/signer_internal_test.go (1 hunks)
- certificate-authority/service/http/config.go (1 hunks)
- certificate-authority/service/http/requestHandler.go (1 hunks)
- certificate-authority/service/http/revocationList.go (1 hunks)
- certificate-authority/service/http/revocationList_test.go (1 hunks)
- certificate-authority/service/http/service.go (2 hunks)
- certificate-authority/service/http/signCertificate_test.go (6 hunks)
- certificate-authority/service/http/signIdentityCertificate_test.go (3 hunks)
- certificate-authority/service/service.go (2 hunks)
- certificate-authority/service/uri/uri.go (1 hunks)
- certificate-authority/store/cqldb/revocationList.go (1 hunks)
- certificate-authority/store/cqldb/signingRecords.go (1 hunks)
- certificate-authority/store/cqldb/signingRecords_test.go (18 hunks)
- certificate-authority/store/mongodb/bulkWriter.go (0 hunks)
- certificate-authority/store/mongodb/config.go (1 hunks)
- certificate-authority/store/mongodb/revocationList.go (1 hunks)
- certificate-authority/store/mongodb/revocationList_test.go (1 hunks)
- certificate-authority/store/mongodb/signingRecords.go (3 hunks)
- certificate-authority/store/mongodb/signingRecords_test.go (13 hunks)
- certificate-authority/store/mongodb/store.go (3 hunks)
- certificate-authority/store/revocationList.go (1 hunks)
- certificate-authority/store/revocationList_test.go (1 hunks)
- certificate-authority/store/store.go (2 hunks)
- certificate-authority/test/revocationList.go (1 hunks)
- certificate-authority/test/service.go (2 hunks)
- charts/plgd-hub/README.md (0 hunks)
- device-provisioning-service/Makefile (1 hunks)
- device-provisioning-service/pb/service.proto (4 hunks)
- http-gateway/test/http.go (1 hunks)
- http-gateway/uri/uri.go (1 hunks)
- pkg/security/certificateSigner/certificateSigner.go (5 hunks)
- pkg/security/certificateSigner/certificateSigner_test.go (0 hunks)
- pkg/security/certificateSigner/identityCertificateSigner_test.go (0 hunks)
💤 Files with no reviewable changes (4)
- certificate-authority/store/mongodb/bulkWriter.go
- charts/plgd-hub/README.md
- pkg/security/certificateSigner/certificateSigner_test.go
- pkg/security/certificateSigner/identityCertificateSigner_test.go
✅ Files skipped from review due to trivial changes (2)
- certificate-authority/service/uri/uri.go
- certificate-authority/store/cqldb/revocationList.go
🧰 Additional context used
🪛 GitHub Check: CodeQL
certificate-authority/store/mongodb/revocationList.go
[failure] 75-75: Database query built from user-controlled sources
This query depends on a user-provided value.
[failure] 169-169: Database query built from user-controlled sources
This query depends on a user-provided value.
[failure] 209-209: Database query built from user-controlled sources
This query depends on a user-provided value.
🔇 Additional comments (110)
certificate-authority/store/mongodb/config.go (2)
11-13: LGTM!The simplified
Validatemethod correctly delegates to the underlying Mongo configuration validation.
8-8: Verify the removal of bulk write configuration.The removal of
BulkWriteconfiguration field is a breaking change. Please ensure that bulk operations are not required for certificate operations, especially for the new Certificate Revocation List implementation.certificate-authority/service/http/config.go (1)
15-15: LGTM! Good formatting.The empty line improves readability by separating the configuration groups.
certificate-authority/service/grpc/getSigningRecords.go (1)
14-18: Clean and efficient implementation of the signing records callback!The refactored implementation is well-structured with several positive aspects:
- Simplified logic using a callback approach instead of iterator
- Clear error propagation
- Proper context handling
- Maintains security through owner validation
certificate-authority/service/grpc/deleteSigningRecords.go (1)
20-22: Method behavior changed from Delete to Revoke without API version change.The method still uses
DeleteSigningRecordsname but now callsRevokeSigningRecordsinternally. This change in behavior without a corresponding API version change could break existing clients who expect deletion rather than revocation.Let's check if this change is properly documented in the proto files:
Consider one of these approaches:
- Create a new
RevokeSigningRecordsmethod and deprecate this one- Version the API (e.g., v2) to signal breaking changes
- Document the behavior change clearly in API documentation if revocation was the intended behavior all along
certificate-authority/service/http/signIdentityCertificate_test.go (2)
12-12: LGTM! Good practice using centralized URI constants.The change to import URI constants from a dedicated package improves maintainability by centralizing URI definitions.
25-25: LGTM! Verify consistent URI usage across the codebase.The URI constant is correctly used in the test case.
✅ Verification successful
Let me check the content of the
.pb.gw.gofile to understand if it's an auto-generated file and verify the usage context.
URI constant usage is correctly implemented
The hardcoded URI path
/certificate-authority/api/v1/sign/identity-csronly appears in the auto-generated gRPC gateway code (service.pb.gw.go), which is expected. All other occurrences in the codebase properly use thecertAuthURI.SignIdentityCertificateconstant. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of SignIdentityCertificate URI constant # across the codebase to ensure all hardcoded instances are replaced. echo "Checking for any remaining hardcoded sign identity certificate URIs..." rg -l "/certificate-authority/api/v1/sign/identity-csr" --type go echo "Checking current usage of the URI constant..." rg "certAuthURI.SignIdentityCertificate" --type goLength of output: 712
Script:
#!/bin/bash # Check if the file is auto-generated head -n 5 certificate-authority/pb/service.pb.gw.go # Check the specific usage context rg -A 5 -B 5 "/certificate-authority/api/v1/sign/identity-csr" certificate-authority/pb/service.pb.gw.goLength of output: 1657
certificate-authority/service/http/requestHandler.go (2)
6-6: LGTM: Import statements are appropriate.The new imports are necessary for HTTP handling, route management, and data persistence.
Also applies to: 13-14
41-41: LGTM: Proper gRPC service registration.The gRPC service registration is correctly implemented with appropriate error handling.
certificate-authority/pb/signingRecords.go (2)
6-6: LGTM!The addition of
math/bigis appropriate for handling certificate serial numbers.
67-68: Verify the impact of validation changes.The refactoring to use credential.Validate() improves code organization. However, let's verify that all existing code paths are prepared for the potentially new validation errors.
✅ Verification successful
Let me analyze the validation changes and their impact based on the search results.
Validation changes are properly handled
The codebase shows consistent error handling patterns around Validate() calls:
- The SigningRecord.Validate() method is called with proper nil checks in multiple places:
- certificate-authority/store/mongodb/signingRecords.go
- certificate-authority/store/cqldb/signingRecords.go
- All callers properly handle validation errors through:
- Returning errors wrapped with additional context
- Proper error propagation up the call chain
- Consistent error handling patterns across the codebase
- The credential.Validate() method is part of a well-established validation pattern where:
- Parent objects validate their child objects
- Nil checks are performed before validation
- Validation errors are properly propagated
The refactoring to use credential.Validate() maintains consistency with the existing validation patterns in the codebase and all error cases are properly handled by the callers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all places where SigningRecord.Validate() is called to ensure proper error handling echo "Searching for direct Validate() calls:" rg -A 3 "Validate\(\)" --type go echo "Searching for error handling patterns around Validate():" ast-grep --pattern 'if err := $_.Validate(); err != nil { $$$ }'Length of output: 168007
certificate-authority/service/http/service.go (5)
5-14: LGTM: Import changes are well-organized and necessary.The new imports support the CRL functionality and follow Go's import organization conventions.
37-46: LGTM: Service configuration is well-structured.The configuration properly integrates CRL support while maintaining existing functionality. The use of package-specific constants enhances maintainability.
52-52: LGTM: RequestHandler properly initialized with store.The store parameter is correctly passed to the RequestHandler, maintaining proper error handling.
28-34: LGTM: Secure implementation of CRL endpoint whitelist.The implementation correctly:
- Uses QuoteMeta for safe regex pattern
- Restricts to GET method only
- Guards the feature with CRLEnabled flag
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information to verify the concern. The results show all usages of
SigningRevocationListin the codebase:Verified: CRL endpoint is securely implemented with proper access controls
The implementation is secure because:
- The CRL endpoint is only registered when
CRLEnabledis true in both the whitelist and route handler- The endpoint is consistently restricted to GET method only
- The endpoint path is only used for CRL distribution in the certificate signer configuration
- No other potentially unsafe access patterns were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify no other endpoints expose CRL data rg -g '*.go' "SigningRevocationList" -C 5Length of output: 4907
27-27: Verify all callers are updated with the new store parameter.The function signature change is breaking and requires updates to all callers.
certificate-authority/service/grpc/config.go (3)
16-22: Technical debt alert: Review CRL implementation for cqldbThe comment indicates temporary test-related implementation for cqldb. This should be properly tracked and addressed.
Would you like me to create a GitHub issue to track the implementation of CRL support in cqldb?
Consider exposing the Enabled flag in configuration
The
Enabledfield is currently hidden from YAML/JSON serialization. This makes it impossible to enable/disable CRL through configuration files, which might limit operational flexibility.
40-40: LGTM!The CRL field is properly integrated into the SignerConfig struct with appropriate serialization tags.
58-65: LGTM!The validation changes are well-implemented:
- Error messages are more specific and consistent
- CRL validation is properly integrated
- Error wrapping follows Go best practices
certificate-authority/pb/signingRecords.proto (1)
23-26: Consider making new fields optional for backward compatibility.The new fields enhance certificate tracking capabilities and are well-documented. However, since these fields are added to an existing message type, consider the following:
- Verify that these fields are properly utilized in the CRL implementation
- Consider marking these fields as optional (though all proto3 fields are technically optional) to explicitly indicate that they might be absent in existing records
✅ Verification successful
Based on the extensive search results, I can now provide a definitive response about the new fields in the protobuf file:
The new fields are properly integrated into the CRL implementation
The codebase shows comprehensive integration of these fields:
- The
serialandissuer_idfields are actively used in the CRL (Certificate Revocation List) implementation:
serialis used to identify certificates in revocation entries (RevocationListCertificate.Serial)issuer_idis used to track and manage CRLs per issuer (RevocationList.Id)- The fields are properly handled in MongoDB storage implementation with corresponding BSON tags
- The CRL functionality is fully implemented with proper validation, HTTP endpoints, and tests
- Making the fields optional is not necessary as they are already effectively optional in proto3 (zero values are used when not set)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new fields in CRL implementation # Test 1: Check if serial field is used in CRL-related code echo "Checking usage of serial field in CRL implementation..." rg -l "serial.*revocation" --type go # Test 2: Check if issuer_id is used for certificate chain validation echo "Checking usage of issuer_id in certificate validation..." rg -l "issuer_id.*validate" --type goLength of output: 322
Script:
#!/bin/bash # Let's try a broader search to understand the usage of these fields # Check for any usage of 'serial' field in go files echo "Checking usage of serial field..." rg -A 3 -B 3 "\.serial" --type go # Check for any usage of 'issuer_id' or 'issuerId' in go files echo "Checking usage of issuer_id field..." rg -A 3 -B 3 "(issuer_id|issuerId)" --type go # Check for any CRL related implementations echo "Checking CRL related implementations..." rg -A 3 -B 3 "CRL|RevocationList" --type go # Check the proto file implementation in more detail echo "Checking proto file implementation..." cat certificate-authority/pb/signingRecords.protoLength of output: 94808
certificate-authority/pb/service.proto (2)
57-60: LGTM! Consistent with SignIdentityCertificate implementation.The changes maintain a consistent API design pattern across the service.
80-86:⚠️ Potential issueConsider separating revocation and cleanup responsibilities.
Issues found:
- Typo in comment: "certficate" should be "certificate"
- The method serves dual purposes (revocation and cleanup), which might be better separated for clarity and maintainability.
Consider splitting into two distinct endpoints:
RevokeCertificatefor certificate revocationDeleteExpiredRecordsfor cleanup operationsThis separation would:
- Make the API more intuitive
- Allow different authorization levels
- Enable separate rate limiting if needed
- Improve monitoring and logging clarity
device-provisioning-service/Makefile (1)
57-59: LGTM! The output path changes align with modern Go practices.The change from
$(GOPATH)/srcto$(REPOSITORY_DIRECTORY)for both OpenAPI and gRPC gateway outputs is a good improvement. This makes the build more predictable and module-aware.Let's verify the proto generation paths are consistently updated:
Also applies to: 60-63
certificate-authority/service/grpc/deleteSigningRecords_test.go (4)
5-5: LGTM: Import additions are appropriateThe new imports support certificate serial number handling and token context management.
Also applies to: 17-17
69-69: LGTM: Explicit context handling improves test clarityThe explicit context passing through test args is a good practice that makes the test cases more maintainable and flexible.
Also applies to: 78-78
109-113: LGTM: Clear error handling in test executionThe explicit error handling with early return improves test clarity and maintainability.
39-40: LGTM: Certificate fields align with X.509 standardsThe addition of Serial and IssuerId fields properly supports certificate tracking and revocation. Consider adding validation tests for these new fields.
certificate-authority/service/cleanDatabase_test.go (4)
8-8: LGTM: Import changes align with new requirements.The addition of "math/big" supports the new Serial field, and the import alias change maintains consistency.
Also applies to: 21-21
79-79: LGTM: Context creation update is consistent.The change to use pkgGrpc maintains the same functionality while aligning with the updated import convention.
70-70: Verify HTTPS configuration in test environment.While enforcing HTTPS is correct for security, please ensure that the test environment properly handles HTTPS connections.
✅ Verification successful
Based on the search results, I can see that the test environment properly handles HTTPS/TLS connections. Specifically:
- The test configuration in
certificate-authority/test/service.goshows proper TLS setup:cfg.APIs.GRPC.TLS.ClientCertificateRequired = false TLS: config.MakeTLSClientConfig()
- The test helper functions in
test/config/config.goprovide both client and server TLS configurations:func MakeTLSClientConfig() client.Config func MakeTLSServerConfig() server.Config
- The HTTPS prefix addition is consistent with the test environment setup where TLS/HTTPS is properly configured and handled.
HTTPS configuration is properly set up in test environment
The change to enforce HTTPS is safe as the test infrastructure already supports and handles HTTPS connections appropriately through the existing TLS configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify HTTPS configuration in test environment # Check for TLS/HTTPS configuration in test setup echo "Checking test TLS configuration..." rg -A 5 "TLS|Certificate" "certificate-authority/test" # Check for any test skip conditions related to HTTPS echo "Checking for HTTPS-related test conditions..." rg "Skip.*[\"']https|TLS" --type goLength of output: 63355
52-53: Verify consistent field usage across the codebase.The new Serial and IssuerId fields are well-structured for certificate tracking and revocation. The Serial field appropriately uses big.Int for certificate serial numbers, and IssuerId follows UUID format.
✅ Verification successful
Let me gather more specific information about the Serial and IssuerId fields in the codebase.
Let me check one more aspect to verify the consistency of these fields across the codebase.
Based on the gathered information, I can now generate the final response:
The Serial and IssuerId fields are consistently implemented across the codebase
The fields are properly defined and used consistently:
- Serial field is correctly stored as a string in both proto definitions and database models
- IssuerId is consistently used with UUID format and properly documented as derived from the issuer's public certificate
- Both fields are properly integrated into the certificate signing and storage workflow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent implementation of Serial and IssuerId fields # across the codebase # Check for consistent field definitions in proto files and structs echo "Checking field definitions..." rg -A 2 "Serial.*string|IssuerId.*string" --type go --type proto # Check for proper UUID format usage for IssuerId echo "Checking IssuerId UUID format usage..." rg -A 2 '[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}' --type goLength of output: 28122
Script:
#!/bin/bash # Search for Serial and IssuerId field definitions in proto files and Go structs echo "Checking field definitions in proto and Go files..." rg -A 2 'Serial.*string|IssuerId.*string|Serial.*big\.Int' --type-add 'proto:*.proto' --type go --type proto # Search for usage patterns of these fields echo "Checking usage patterns..." ast-grep --pattern 'type $_ struct { $$$ Serial $_ $$$ }' echo "Checking IssuerId usage..." ast-grep --pattern 'type $_ struct { $$$ IssuerId $_ $$$ }'Length of output: 2992
Script:
#!/bin/bash # Check for proto definitions and field usage in signing records echo "Checking proto definitions..." rg -p "message (SigningRecord|CredentialStatus)" --type proto -A 10 # Check for any other implementations or usages echo "Checking implementations..." rg -p "func.*Sign.*Certificate" --type go -A 5Length of output: 21241
certificate-authority/service/grpc/getSigningRecords_test.go (4)
7-7: LGTM! Import changes are appropriate.The addition of
math/bigis suitable for handling certificate serial numbers, and the package alias update maintains consistency.Also applies to: 20-20
43-44: Consider adding Serial number validation in tests.The addition of
SerialandIssuerIdfields is appropriate for CRL implementation. However, consider adding test cases that verify:
- Serial number parsing back to
big.Int- Different serial number sizes
- Invalid serial number formats
Let's check if there are any validation tests for these fields:
101-101: LGTM! Package refactoring maintains token handling.The context creation update correctly maintains the token handling functionality while using the updated package.
Let's verify the token handling implementation:
✅ Verification successful
Token handling implementation verified and consistent
The change from
kitNetGrpctopkgGrpcis correct and maintains the same token handling functionality. TheCtxWithTokenfunction inpkg/net/grpc/auth.goproperly sets the authorization token in the gRPC metadata context, which is the expected behavior for gRPC authentication.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check token context handling in the new package ast-grep --pattern 'func CtxWithToken($ctx, $token) $ret { $$$ }'Length of output: 69
Script:
#!/bin/bash # Let's try to find the CtxWithToken function using ripgrep for better coverage rg -A 5 "func CtxWithToken" # Also search for the import statement to verify the package rg "pkgGrpc" certificate-authority/service/grpc/getSigningRecords_test.goLength of output: 957
92-92: Verify HTTPS URL construction safety.The server initialization now uses a constructed HTTPS URL. Please ensure:
- The URL is properly sanitized
- The host value is validated
- There's proper error handling for malformed URLs
Let's check URL handling across the codebase:
✅ Verification successful
Let's analyze how URLs are constructed and validated in the codebase. The previous search might have been too specific, so I've broadened it to catch more URL-related patterns.
Based on the search results, I can now generate a final response:
URL construction is properly handled and validated.
The URL construction pattern
"https://" + config.CERTIFICATE_AUTHORITY_HTTP_HOSTis consistently used across the codebase and the HTTPConfig struct includes proper validation:
- The
ExternalAddressis validated usingurl.ParseRequestURI()incertificate-authority/service/config.go- The host configuration is used consistently across test and production code
- The URL construction is part of a validated configuration structure rather than ad-hoc string concatenation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for URL validation and sanitization rg "https://" --type go -B 2 -A 2Length of output: 69
Script:
#!/bin/bash # Let's try a more comprehensive search for URL handling patterns rg "config\.CERTIFICATE_AUTHORITY_HTTP_HOST" --type go -B 2 -A 2 # Also search for URL validation/sanitization patterns rg "url\.(Parse|URL|QueryEscape)" --type go -B 2 -A 2 # Look for any HTTP/HTTPS related configuration ast-grep --pattern 'config.CERTIFICATE_AUTHORITY_HTTP_HOST' # Search for any URL construction patterns ast-grep --pattern '$var := "https://" + $host'Length of output: 14219
certificate-authority/service/grpc/signIdentityCertificate_test.go (3)
15-15: LGTM: Package rename fromkitNetGrpctopkgGrpcThe package rename maintains the same functionality while using a more standardized naming convention.
Line range hint
1-106: Verify test coverage for certificate revocationGiven that this PR implements Certificate Revocation List functionality, consider adding test cases to verify:
- Signing certificates for already revoked identities
- Handling of revocation status during signing
Let's check if these scenarios are covered elsewhere:
#!/bin/bash # Description: Look for revocation-related test cases # Test: Search for test files containing revocation tests rg -t go "Test.*Revocation" --glob "*_test.go"
52-52: LGTM: Updated context creation with token using renamed packageThe context creation with token has been correctly updated to use the renamed package
pkgGrpc. The functionality remains unchanged.Let's verify that all occurrences of the old package have been updated:
Also applies to: 80-80
certificate-authority/service/grpc/server.go (3)
26-26: LGTM! Field addition follows Go conventions.The new
crlServerAddressfield is appropriately typed and named for storing the CRL server address.
31-39: Verify the impact of constructor signature change.The constructor has been updated correctly to include the CRL server address. However, this is a breaking change that requires verification of all existing callers.
✅ Verification successful
Constructor signature change has been properly propagated
All existing callers of
NewCertificateAuthorityServerhave been updated to include the newcrlServerAddressparameter:
- Main service initialization in
certificate-authority/service/service.gousesconfig.APIs.HTTP.ExternalAddress- Test files consistently use
"https://"+config.CERTIFICATE_AUTHORITY_HTTP_HOST🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all existing calls to NewCertificateAuthorityServer to ensure they've been updated rg "NewCertificateAuthorityServer\(" --type go -B 2 -A 2Length of output: 94
Script:
#!/bin/bash # Search for NewCertificateAuthorityServer calls with case-insensitive and word boundary matches rg -w -i "NewCertificateAuthorityServer" --type go # Also check for test files that might use this constructor rg -w -i "NewCertificateAuthorityServer" --type go --glob "*_test.go" # Look for files that might contain the constructor implementation fd -e go | xargs grep -l "CertificateAuthorityServer"Length of output: 3592
105-105: Verify the NewSigner implementation handles the CRL address correctly.The
crlServerAddressis now passed to the signer, but we should verify its implementation in the signer code.✅ Verification successful
CRL server address is properly handled in the signer implementation
The verification shows that:
- The
crlServerAddressparameter is correctly passed throughNewSignertonewSigner- The address is properly stored in the
Signerstruct'scrl.serverAddressfield- It's only set when CRL is enabled (
if signerConfig.CRL.Enabled)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the NewSigner implementation to ensure proper handling of crlServerAddress ast-grep --pattern 'func NewSigner($_, $_, $crlServerAddress string, $_) ($_, $_) { $$$ }'Length of output: 94
Script:
#!/bin/bash # Let's try to find the NewSigner function implementation with a broader pattern ast-grep --pattern 'func NewSigner($$$) { $$$ }' # Also search for the file content directly rg "func NewSigner" -A 10Length of output: 902
Script:
#!/bin/bash # Let's check how crlServerAddress is used within the signer implementation rg "crlServerAddress" certificate-authority/service/grpc/signer.go -B 2 -A 10 # Also check the Signer struct definition to see how the address is stored ast-grep --pattern 'type Signer struct { $$$ }'Length of output: 2858
certificate-authority/service/http/signCertificate_test.go (4)
16-16: LGTM! Good refactoring of imports.The addition of centralized URI management through
certAuthURIand token handling throughpkgGrpcimproves code organization and maintainability.Also applies to: 18-18
70-70: LGTM! Consistent token management.Good update to use the standardized
pkgGrpcpackage for token context management.
116-116: LGTM! Consistent use of centralized URIs.Good update to use the centralized URI definitions from
certAuthURIpackage, which improves maintainability and consistency.Also applies to: 128-128
55-61: 🛠️ Refactor suggestionConsider expanding test coverage for revocation scenarios.
While testing multiple certificate requests with the same CSR is good, consider adding test cases that explicitly verify:
- Certificate revocation status tracking
- Handling of signing requests for previously revoked certificates
- Edge cases around certificate lifecycle management
Example test case structure:
{ name: "previously revoked certificate", args: args{ req: &pb.SignCertificateRequest{ CertificateSigningRequest: csr, // Add relevant fields for revocation testing }, }, wantErr: true, }certificate-authority/store/revocationList_test.go (1)
1-10: LGTM! Well-structured test package setup.The black-box testing approach using
store_testpackage is appropriate for testing the public API. Import declarations are clean and follow Go conventions.certificate-authority/service/config.go (2)
6-6: LGTM: Required import for URL validation.The addition of the
net/urlpackage is necessary for validating the newExternalAddressfield.
44-44: Verify all NewSigner usages are updated.The signature change of
NewSignerto includeExternalAddressis a breaking change. Let's verify all callers are updated.✅ Verification successful
Let me verify the NewSigner function definition to ensure we have a complete picture.
All NewSigner usages are correctly updated
The verification shows that all calls to NewSigner are consistent with its signature that includes the crlServerAddress parameter:
- In
certificate-authority/service/config.go: Usesc.APIs.HTTP.ExternalAddress- In
certificate-authority/service/grpc/server.go: Usess.crlServerAddress- In
certificate-authority/service/grpc/signer_internal_test.go: Uses empty string""for testing🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all NewSigner calls to ensure they include the new parameter ast-grep --pattern 'NewSigner($$$)'Length of output: 308
Script:
#!/bin/bash # Search for NewSigner function definition ast-grep --pattern 'func NewSigner($$$)'Length of output: 3651
certificate-authority/service/grpc/signer_internal_test.go (1)
Line range hint
119-126: Verify error handling for CRL server connectivity.The test doesn't verify how
NewSignerhandles CRL server connectivity issues. This is important for the Certificate Revocation List implementation.Let's check if there's error handling for CRL server issues in the implementation:
certificate-authority/service/grpc/config_test.go (2)
1-10: LGTM! Clean package structure and imports.The package name and imports follow Go conventions and contain only necessary dependencies.
53-56: Verify CRL configuration aligns with RFC 5280 requirements.The test uses a 1-hour expiration for CRL. Please verify this aligns with your CA's CRL update frequency requirements. According to RFC 5280, the CRL validity period should be aligned with:
- The CA's CRL issuance frequency
- The risk of key compromise
- Revocation checking requirements of relying parties
✅ Verification successful
Based on the search results, I can now provide a final response since I have sufficient information about the CRL implementation:
CRL configuration duration of 1 hour is appropriate and aligns with implementation
The test configuration using a 1-hour expiration for CRL is valid because:
- The codebase enforces a minimum CRL validity period of 1 minute through validation in
service/grpc/config.go- The production code in
certificate-authority/test/service.goalso uses the same 1-hour duration as the default configuration- The implementation supports dynamic CRL updates through
UpdateRevocationListQuerywithUpdateIfExpiredflag in the store🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CRL-related constants or documentation that might define these requirements rg -g '!*.test.go' -g '!*_test.go' "CRL" -A 5 -B 5Length of output: 20511
certificate-authority/test/service.go (4)
10-10: LGTM: Import added for CRL configuration.The grpc import is correctly added to support the new CRL configuration functionality.
53-53: LGTM: Clean integration of new configuration.The new HTTP and CRL configurations are properly integrated into the main config setup, maintaining the existing validation flow.
Also applies to: 59-59
33-43: Consider making CRL configuration more flexible.A few concerns about the current implementation:
- The expiration time is hard-coded to 1 hour. Consider making this configurable through environment variables or configuration files for different environments.
- CRL is only enabled for MongoDB. The reason for this limitation isn't clear and might need documentation.
25-31: Verify the security of HTTP/HTTPS configuration.The external address uses HTTPS while the internal address appears to use HTTP. Please verify if this is intentional and secure for your deployment environment.
device-provisioning-service/pb/service.proto (4)
75-75: Verify request validation for enrollment group creation.While using
body: "*"is common for POST endpoints, ensure that proper validation is implemented in the service layer, especially for certificate-related operations. This is crucial for maintaining security in the provisioning process.Also applies to: 78-78
89-89: LGTM! Well-structured update endpoint.The explicit
body: "enrollment_group"specification follows best practices by clearly defining the expected request body structure.Also applies to: 92-92
127-127: Ensure consistent validation across creation endpoints.Similar to the enrollment group creation, verify that proper validation is implemented for hub creation requests, maintaining consistency in security measures across the service.
Also applies to: 130-130
141-141: LGTM! Consistent with update endpoint patterns.The
body: "hub"specification maintains consistency with the enrollment group update pattern and follows RESTful best practices.Also applies to: 144-144
certificate-authority/pb/signingRecords_test.go (2)
1-9: LGTM! Package and imports are well-structured.The package name and imports follow Go best practices for external test packages.
1-189: Enhance security validation test coverage.Given that this code handles certificate management, consider adding security-focused test cases:
- Test invalid certificate PEM formats
- Test certificate PEM with potential injection patterns
- Test malformed serial numbers that might bypass validation
Let's verify the validation implementation:
✅ Verification successful
Let me search for the validation implementation to better understand how certificates are validated:
Let me try a different search pattern to find the validation implementation:
Let me search for the actual certificate validation implementation:
Based on the code review and verification results, I can now provide a final response:
Test coverage for certificate validation is adequate
The current test coverage appropriately validates certificates through multiple layers:
- The test cases verify empty/invalid certificate PEM content
- The serial number validation ensures proper decimal format using
big.Int.SetString- The implementation uses proper X509 parsing via security package's
ParseX509FromPEM- The test cases cover all validation scenarios including missing fields and invalid formats
The original review comment's concerns are already addressed by the existing implementation and test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for certificate validation logic ast-grep --pattern 'func (c *CredentialStatus) Validate() error { $$$ }' # Look for any existing security-related tests rg -i 'security|injection|malicious|invalid.*certificate' --type goLength of output: 29555
Script:
#!/bin/bash # Search for CredentialStatus validation implementation ast-grep --pattern 'func (c *CredentialStatus) Validate() error { $$$ }' # Search for certificate validation in the codebase rg -A 10 'func.*Validate.*Certificate' --type goLength of output: 127
Script:
#!/bin/bash # Search for the validation implementation in the proto/pb files rg -A 15 'func \(.*\) Validate\(\)' --type go certificate-authority/pb/ # Look for certificate validation patterns rg -A 10 'CertificatePem|Serial.*validate' --type go certificate-authority/Length of output: 39119
Script:
#!/bin/bash # Search for certificate validation in x509 package rg -A 10 'func.*Validate' --type go pkg/security/x509/ # Search for PEM validation rg -A 10 'ParseX509FromPEM|LoadX509' --type goLength of output: 6391
certificate-authority/service/config_test.go (2)
150-199: LGTM! Well-structured test implementation.The test follows good practices with table-driven tests and is consistent with the existing test patterns in the file.
162-163: Verify the test helper function implementation.Let's verify that
test.MakeHTTPConfig()sets secure defaults for the HTTP configuration.✅ Verification successful
Let's verify the security settings by checking the implementation of
config.MakeHttpServerConfig()which is used by all the test helper functions.
Test helper function
MakeHTTPConfig()provides reasonable defaults for testingThe implementation shows that:
- The HTTP server configuration sets appropriate timeouts:
- Read timeout: 8 seconds
- Read header timeout: 4 seconds
- Write timeout: 16 seconds
- Idle timeout: 30 seconds
- The certificate authority implementation properly sets HTTPS external address
- The device provisioning service includes authorization configuration
- All implementations use the common
MakeHttpServerConfig()with consistent timeout settingsThese settings are reasonable for testing purposes and don't expose any security concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of test.MakeHTTPConfig() # to ensure it sets secure defaults for HTTP configuration # Look for the helper function implementation ast-grep --pattern 'func MakeHTTPConfig() $_ { $$$ }' # Also check for any constants or variables used in the implementation rg -A 5 'MakeHTTPConfig'Length of output: 6433
Script:
#!/bin/bash # Check the implementation of MakeHttpServerConfig ast-grep --pattern 'func MakeHttpServerConfig() $_ { $$$ }' # Also look for any TLS or security related configurations rg -A 5 'MakeHttpServerConfig'Length of output: 5288
certificate-authority/service/service.go (2)
103-103: Verify external address configuration for CRL distribution point.The external address will be used as the CRL distribution point in certificates. Ensure that:
- The address is properly validated in the config
- It's accessible from external networks
- It matches the CRL endpoint path
✅ Verification successful
External address validation is properly implemented for CRL distribution point
The external address configuration is properly validated:
- The
HTTPConfig.Validate()function validates the external address usingurl.ParseRequestURI- The address is correctly passed to
NewCertificateAuthorityServerascrlServerAddress- Test cases cover validation of invalid addresses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the external address validation in config # Test: Check if the external address is validated in config rg -A 5 "ExternalAddress.*Validate" certificate-authority/service/config/ # Test: Check if the address is used in CRL-related code rg -A 5 "CRLDistributionPoints|crlDistributionPoints" certificate-authority/Length of output: 162
Script:
#!/bin/bash # Let's try different approaches to verify the external address usage and validation # Check config validation patterns rg -A 10 "type HTTP struct" certificate-authority/service/config/ rg "func.*Validate" certificate-authority/service/config/ # Check how external address is used in the codebase rg -A 5 "ExternalAddress" certificate-authority/service/ # Look for CRL distribution point implementation ast-grep --pattern 'func NewCertificateAuthorityServer($$$) ($_, error) { $$$ }' # Check for URL/address validation patterns rg -A 5 "(url|addr|address).*(Valid|Parse)" certificate-authority/service/Length of output: 7982
122-123: Verify CRL implementation details.The CRL feature can be toggled via configuration. Please ensure:
- Proper error handling when CRL is enabled but the database doesn't support required operations
- Clear documentation of CRL-related configuration options
- Appropriate logging when CRL operations are performed
✅ Verification successful
Based on the search results, I can now provide a comprehensive verification of the CRL implementation. Here's the final response:
CRL implementation has proper error handling and configuration validation
The CRL feature is well-implemented with:
- Proper configuration validation in
CRLConfig.Validate()ensuringExpiresInis > 1 minute- Error handling in MongoDB storage for CRL number sequence
- Conditional CRL distribution points in certificate signing
- Proper HTTP endpoint protection with JWT validation
- Database-specific CRL support (currently MongoDB only)
Implementation details to note:
- CRL is properly toggled via
config.Signer.CRL.Enabled- CRL validity period is configurable via
config.Signer.CRL.ExpiresIn- CRL distribution points are automatically configured when enabled
- HTTP endpoints for CRL are conditionally registered
- MongoDB implementation handles CRL number sequence starting from 1
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify CRL implementation details # Test: Check error handling in CRL-related code ast-grep --pattern 'if $err != nil { $$$ return $_, fmt.Errorf("CRL: %w", $err) }' # Test: Check for CRL-related logging rg "log\.(Debug|Info|Error).*CRL" # Test: Check for CRL documentation fd -e md -e txt . | xargs rg -l "CRL"Length of output: 162
Script:
#!/bin/bash # Let's try a different approach to verify CRL implementation # Check for CRL-related error handling with broader pattern ast-grep --pattern 'if err := $$.CRL$$; err != nil { $$$ }' # Look for CRL-related function implementations ast-grep --pattern 'func $_($$$) { $$$ CRL $$$ }' # Search for CRL-related code with context rg -A 3 -B 3 "CRL" --type go # Check configuration structure for CRL rg "type.*Config.*struct" -A 15 --type go | rg -B 15 -A 15 "CRL" # Look for CRL in comments and documentation rg "(?m)^//.*CRL" --type goLength of output: 30273
http-gateway/test/http.go (2)
196-199: LGTM! Implementation follows established patterns.The
AddIssuerIDmethod is well-implemented and consistent with other builder methods in the file, following the same patterns for parameter handling and method chaining.
196-199: Add test coverage for the new method.Since this is a test utility that will be used for testing CRL endpoints, it's important to ensure the method works as expected. Consider adding test cases that verify:
- The issuer ID is correctly stored in the URI parameters
- The method properly chains with other builder methods
- The built URL contains the issuer ID parameter in the expected format
Let's check if there are any existing tests for this file:
certificate-authority/service/grpc/signCertificate_test.go (2)
Line range hint
13-24: LGTM: Import changes look good.The switch from
kitNetGrpctopkgGrpcappears to be part of a consistent refactoring effort across the codebase.
94-106: Well-structured refactoring of CSR creation helpers.Good separation of concerns by splitting
createCSRinto two functions. This enables better testing of scenarios involving different keys for the same device ID.http-gateway/uri/uri.go (1)
27-27: Verify the usage pattern of IssuerIDKey in the codebase.Since this is part of the Certificate Revocation List implementation, let's verify how this constant is being used across the codebase to ensure consistent parameter handling.
✅ Verification successful
Based on the results, I can now generate the final response:
The usage of
IssuerIDKeyis consistent across the codebaseThe constant is properly defined and used in both the http-gateway and certificate-authority services:
- Used as a URL parameter key in
certificate-authority/service/uri/uri.gofor signing revocation list endpoint- Referenced in HTTP parameter handling in
certificate-authority/service/http/revocationList.go- Used in test utilities in
http-gateway/test/http.go- Consistently spelled as "issuerId" in all occurrences including protobuf definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the usage pattern of IssuerIDKey in the codebase # Test 1: Find direct references to the constant echo "=== Direct references to IssuerIDKey ===" rg --type go "IssuerIDKey" # Test 2: Find URL patterns using the parameter name echo -e "\n=== URL patterns with issuerId parameter ===" rg --type go "issuerId" # Test 3: Look for related handler functions echo -e "\n=== Related handler functions ===" ast-grep --pattern 'func $$$($_, $_, $_) { $$$ issuerId := $$$ $$$ }'Length of output: 1258
certificate-authority/pb/README.md (2)
123-128: LGTM! Well-documented certificate tracking fields.The new fields
serialandissuer_idare properly documented with clear descriptions and BSON tags. These fields are essential for certificate tracking and revocation list implementation.
154-154: LGTM! Clear description for DeletedSigningRecords.The description "Revoke or delete certificates" is concise and accurately describes the purpose of this message type.
certificate-authority/store/mongodb/revocationList_test.go (1)
237-269: LGTM! Well-implemented helper function.The helper function correctly handles concurrent operations with proper synchronization using WaitGroup and atomic operations. The retry logic appropriately handles race conditions that may occur during parallel execution.
bundle/nginx/nginx.conf.template (1)
Line range hint
123-136: LGTM! The certificate authority endpoint configuration looks secure.The updated location block correctly:
- Consolidates certificate-related endpoints under a single configuration
- Maintains proper SSL certificate validation
- Sets appropriate security headers
- Follows the established pattern for secure service proxying
Let's verify the certificate authority service integration:
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient information to verify the certificate authority service integration:
Certificate authority endpoint configuration is correctly integrated
The location block changes are properly integrated with the certificate authority service as evidenced by:
- The paths match the gRPC gateway bindings defined in
certificate-authority/pb/service.proto- SSL certificate paths are consistently used across the nginx configuration
- The configuration follows the same secure pattern used for other internal services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the certificate authority service configuration and related proto definitions # Test 1: Check if the certificate authority proto defines the HTTP bindings for these paths rg -A 5 "(/certificate-authority|/api/v1/sign)" # Test 2: Ensure SSL certificate paths exist in the configuration rg "/data/certs/(internal/endpoint|root_ca)"Length of output: 81186
certificate-authority/store/cqldb/signingRecords.go (2)
342-367: LGTM! Well-structured implementation with proper error handling.The changes to
LoadSigningRecordsdemonstrate good practices:
- Efficient iterator pattern for processing large datasets
- Proper error handling and resource cleanup
- Prevention of duplicate records
368-374:⚠️ Potential issueSecurity Concern: Implement proper certificate revocation handling.
The current implementation deletes records instead of maintaining a proper Certificate Revocation List (CRL). This approach has several security implications:
- Loss of historical data about revoked certificates
- No audit trail of revocations
- Potential security risks if relying parties have cached the certificates
A proper CRL implementation should:
- Mark certificates as revoked while retaining the records
- Include revocation reason and timestamp
- Support CRL distribution points
- Maintain an audit trail
Let's verify if there are any existing revoked certificates that need migration:
Would you like me to:
- Design a proper CRL implementation schema?
- Create a migration plan for existing certificates?
- Open a GitHub issue to track this security enhancement?
certificate-authority/store/mongodb/signingRecords_test.go (3)
101-103: LGTM! Good improvement in error handling.The early return after error check follows the fail-fast principle and prevents unnecessary code execution.
Line range hint
477-485: LGTM! Test execution flow is well-structured.The test maintains proper error handling, sorting, and comparison of results.
302-304: Verify interface compatibility for handler changes.The replacement of
Handlewithprocessmight break interface compatibility. Additionally,processis a less descriptive name compared toHandle.Let's check for interface implementations and usages:
certificate-authority/store/cqldb/signingRecords_test.go (3)
5-5: LGTM: Appropriate import for certificate serial numbersThe addition of "math/big" is necessary for handling large integer serial numbers in X.509 certificates.
386-388: LGTM: Improved function namingThe rename from
Handletoprocessbetter describes the function's purpose of processing signing records.
369-379:⚠️ Potential issueReview test logic for expired records
The test expects records to be empty after calling
DeleteNonDeviceExpiredRecords, but this function returnsErrNotSupported. This suggests a mismatch between the test's expectations and the actual implementation.Let's verify if this is intentional:
Consider one of these approaches:
- If the feature is not supported, update the test to expect records to remain
- If the feature should be supported, implement it and update the error handling
✅ Verification successful
Test logic for expired records is correct
The test logic is correct because:
- The CqlDB implementation explicitly returns
ErrNotSupportedsince expired records are automatically deleted by Cassandra itself (as documented in the interface comment).- The test verifies this behavior by checking that
DeleteNonDeviceExpiredRecordsreturnsErrNotSupported.- The empty records check (
require.Empty(t, h1.lcs)) is also correct as it verifies that Cassandra's TTL mechanism worked after waiting for 3 seconds.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if DeleteNonDeviceExpiredRecords is implemented elsewhere rg -A 5 "DeleteNonDeviceExpiredRecords" # Check for any related TODO or FIXME comments rg -i "todo.*delete.*expired|fixme.*delete.*expired"Length of output: 3779
certificate-authority/pb/doc.html (2)
494-511: Review the new CredentialStatus fieldsThe addition of
serialandissuer_idfields enhances certificate tracking capabilities:
serial: Tracks the serial number of issued certificatesissuer_id: Provides a unique identifier for the certificate issuerThese fields are essential for implementing Certificate Revocation List (CRL) functionality as they help uniquely identify certificates and their issuers.
Line range hint
386-443: Verify the impact of new API endpointsThe addition of new API endpoints with the
/certificate-authorityprefix while maintaining the old ones suggests a transition period. This could lead to confusion about which endpoint to use.Let's check if there are any references to these endpoints in other files:
Consider:
- Documenting which endpoint is preferred
- Adding deprecation notices for the old endpoints if they are being phased out
- Including version migration guidelines in the API documentation
certificate-authority/store/store.go (1)
41-41: Verify Compatibility ofLoadSigningRecordsSignature ChangeThe
LoadSigningRecordsmethod now uses theProcess[SigningRecord]generic function type. Ensure that all implementations of theStoreinterface have been updated to reflect this change to prevent any type compatibility issues.You can run the following script to find all implementations and verify their signatures:
✅ Verification successful
This will help us find all implementations of the LoadSigningRecords method and verify their signatures.
LoadSigningRecords Signature Change is Properly Implemented
The verification shows that both implementations of the
Storeinterface (MongoDB and CQLDB) have been correctly updated to use theProcess[SigningRecord]generic function type in theirLoadSigningRecordsmethod signatures:
certificate-authority/store/mongodb/signingRecords.go:func (s *Store) LoadSigningRecords(ctx context.Context, owner string, query *store.SigningRecordsQuery, p store.Process[store.SigningRecord]) errorcertificate-authority/store/cqldb/signingRecords.go:func (s *Store) LoadSigningRecords(ctx context.Context, owner string, query *store.SigningRecordsQuery, p store.Process[store.SigningRecord]) error🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all implementations of LoadSigningRecords in the codebase. # Expectation: All implementations should use the updated signature with Process[SigningRecord]. rg -A 3 'func.*LoadSigningRecords\(.*Process\[SigningRecord\]'Length of output: 64
Script:
#!/bin/bash # Let's first find all files that might contain LoadSigningRecords implementations rg -l "LoadSigningRecords" # Then let's look at the actual implementations with more context rg -B 2 -A 5 "func.*LoadSigningRecords"Length of output: 3412
certificate-authority/service/http/revocationList.go (4)
27-48: FunctioncreateCRLis correctly implementedThe
createCRLfunction correctly constructs a CRL using the provided revocation list, issuer certificate, and private key. The error handling and logic are appropriate.
80-82: Ensure proper handling of write errors to the responseThe error returned by
w.Write(crl)is currently being returned, which is good. Ensure that any non-nil errors are properly handled by the caller to prevent unnoticed failures.
85-89: Error handling inrevocationListmethod is appropriateThe
revocationListmethod correctly callswriteRevocationListand handles any errors by returning an appropriate HTTP response.
71-71:⚠️ Potential issueCorrect the typo in method name
GetCRLConfiguationThere is a typo in the method name
GetCRLConfiguation. It should beGetCRLConfiguration.Run the following script to verify the correct method name:
pkg/security/certificateSigner/certificateSigner.go (3)
20-20: LGTMThe
CRLDistributionPointsfield has been correctly added to theSignerConfigstruct to support certificate revocation list distribution points.
38-42: LGTMThe
WithCRLDistributionPointsoption function is properly implemented to set the CRL distribution points in the signer configuration.
Line range hint
84-128: LGTMThe
Signmethod has been appropriately updated to utilize the newparseCertificateRequestfunction and to includeCRLDistributionPointsin the certificate template.certificate-authority/service/grpc/signCertificate.go (2)
166-169: Verify the appropriateness of gRPC error codes used.Throughout the
SignCertificatemethod, errors are returned withcodes.InvalidArgument. Some of these errors might be better represented with different gRPC error codes, such ascodes.Internalfor server errors.Review the error codes to ensure they accurately represent the nature of the errors according to gRPC best practices.
Line range hint
29-58: Ensure all callers oftoSigningRecordare updated with the new parameter.The function
toSigningRecordnow includes the additional parameterissuerID. Make sure that all calls to this function throughout the codebase are updated to pass theissuerIDto prevent any argument mismatch errors.Run the following script to verify all usages:
✅ Verification successful
All callers of
toSigningRecordare properly updated with the new parameterBased on the search results, there is only one caller of
toSigningRecordincertificate-authority/service/grpc/signer.go, and it correctly passes all three required parameters:owner,s.issuerID, andtemplate. This matches the function signature defined insignCertificate.go. No further updates are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check all calls to `toSigningRecord` to ensure they include the new `issuerID` parameter. # Test: Search for calls to `toSigningRecord` and display the number of arguments passed. rg 'toSigningRecord\(' -A 0 -B 0Length of output: 288
certificate-authority/service/grpc/signer.go (12)
10-10: Importinggithub.com/google/uuidfor UUID generation.The addition of the
uuidpackage is appropriate for generating issuer IDs based on the public key.
13-13: Importinguripackage for CRL distribution points.The inclusion of
certificate-authority/service/uriis appropriate for constructing CRL distribution point URIs required for CRL management.
23-23: AddingissuerIDfield to theSignerstruct.The
issuerIDfield is added to uniquely identify the issuer based on the public key. This is appropriate for managing CRLs and associating certificates with their issuers.
26-29: Adding CRL configuration toSignerstruct.The addition of the
crlstruct withinSignerto store CRL server address and validity duration enables CRL support, which is essential for certificate revocation management.
139-141: AddingGetCertificatemethod toSigner.The addition of the
GetCertificatemethod allows external components to access the signer's certificate, which can be useful for validation or logging purposes.
147-149: AddingGetCRLConfigurationmethod toSigner.This method provides access to the CRL server address and validity duration, which is useful for CRL management and distribution point configuration.
151-153: AddingIsCRLEnabledmethod toSigner.This method allows checking if CRL is enabled, enhancing code clarity and facilitating conditional logic based on CRL support.
155-161: AddingnewCertificateSignermethod for code reuse.The
newCertificateSignermethod simplifies the creation of certificate signers based on theidentitySignerflag, improving code maintainability and reducing duplication.
162-186: Refactoring signing logic intosignmethod.The new
signmethod consolidates the signing logic for both standard and identity certificates, improving code reuse and maintainability. The inclusion of CRL distribution points when CRL is enabled is appropriate.
188-190: UpdatingSignmethod to use newsignmethod.The
Signmethod now utilizes the consolidatedsignfunction, which streamlines the signing process and reduces code duplication.
192-193: UpdatingSignIdentityCSRmethod to use newsignmethod.The
SignIdentityCSRmethod now callssignwithisIdentityCertificateset totrue, appropriately reusing the signing logic.
48-54: 🛠️ Refactor suggestionConsider security implications of using SHA1 in UUID generation.
The
getIssuerIDfunction usesuuid.NewSHA1to generate an issuer ID from the public key:return uuid.NewSHA1(uuid.NameSpaceX500, publicKeyRaw).String(), nilSince SHA1 is considered cryptographically weak, even though it's used here for generating a UUID, consider if this could introduce any security concerns or potential for collision.
Consider using a more secure hash function if the
uuidpackage supports it, such as SHA256, to reduce the risk of collisions.certificate-authority/store/mongodb/revocationList.go (4)
75-75: Duplicate CommentThis issue has been previously addressed in the past review comments.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 75-75: Database query built from user-controlled sources
This query depends on a user-provided value.
169-169: Duplicate CommentThis issue has been previously addressed in the past review comments.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 169-169: Database query built from user-controlled sources
This query depends on a user-provided value.
209-209: Duplicate CommentThis issue has been previously addressed in the past review comments.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 209-209: Database query built from user-controlled sources
This query depends on a user-provided value.
178-217: Verify the correctness of theincludeExpiredfilter logicWhen
includeExpiredisfalse, the filter modifies the query to return only non-expired certificates. Ensure that this logic aligns with the intended functionality and that it correctly filters out expired certificates.Verification Script:
✅ Verification successful
Based on the examination of the code and tests, I can now provide the final response:
The
includeExpiredfilter logic in GetRevocationList is correctly implementedThe implementation correctly filters expired certificates when
includeExpiredis false by:
- Using a MongoDB
$elemMatchto match documents containing at least one non-expired certificate- Using
$filterin the projection to return only non-expired certificates in the results- Comparing certificate validity (
ValidUntil) against the current time (now)The test cases verify this behavior:
- "all from issuer0" test confirms retrieval of all certificates when
includeExpired=true- "no valid from issuer0" test verifies that an error is returned when no valid certificates exist and
includeExpired=false- "non-expired from issuer4" test validates that only non-expired certificates are returned when
includeExpired=false🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the GetRevocationList function correctly filters expired certificates when includeExpired is false. # Test: Fetch a revocation list with includeExpired=false and ensure no expired certificates are returned. # Expected Result: All certificates in the revocation list have ValidUntil >= current time. # Note: Replace <issuerID> with a valid issuer ID for testing. issuerID="<issuerID>" currentTime=$(date +%s%N) # MongoDB query simulation (pseudocode) mongo --eval " db.revocationList.find({ _id: '$issuerID', certificates: { \$elemMatch: { ValidUntil: { \$gte: $currentTime } } } }, { certificates: { \$filter: { input: '\$certificates', as: 'cert', cond: { \$gte: ['\$\$cert.ValidUntil', $currentTime] } } } }) "Length of output: 542
Script:
#!/bin/bash # Let's analyze the implementation by looking at tests and related code # Find test files related to revocation list fd "revocation.*test.*" # Search for test cases specifically testing the includeExpired functionality rg -l "includeExpired.*GetRevocationList|GetRevocationList.*includeExpired" # Look for any other usages of GetRevocationList to understand the expected behavior ast-grep --pattern 'GetRevocationList($$$)' # Search for any comments or documentation about the expired certificates filtering rg -i "expired.*(certificate|revocation)"Length of output: 70327
Script:
#!/bin/bash # Let's examine the test files to understand the expected behavior of includeExpired parameter # Look at the revocation list test files rg -A 10 "GetRevocationList.*includeExpired" certificate-authority/store/mongodb/revocationList_test.go rg -A 10 "GetRevocationList.*includeExpired" certificate-authority/store/revocationList_test.go # Check if there are any other test cases related to expired certificates in the revocation list rg -A 10 "expired.*certificate.*revocation" certificate-authority/store/mongodb/revocationList_test.goLength of output: 579
Script:
#!/bin/bash # Let's look at the test cases and implementation more comprehensively # Find the test cases for GetRevocationList rg -B 10 "GetRevocationList.*includeExpired" certificate-authority/store/mongodb/revocationList_test.go # Look at the test helper function rg -A 10 "CheckRevocationList" certificate-authority/store/revocationList_test.go # Check the store interface definition rg -A 5 "type.*Store.*interface" certificate-authority/store/store.goLength of output: 1059
Script:
#!/bin/bash # Let's examine the test file content to understand the test cases # Get the full content of the test file cat certificate-authority/store/mongodb/revocationList_test.go # Look for any test data setup related to expired certificates rg -B 5 "ValidUntil.*now" certificate-authority/store/mongodb/revocationList_test.go rg -B 5 "ValidUntil.*time" certificate-authority/store/mongodb/revocationList_test.goLength of output: 9802
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 209-209: Database query built from user-controlled sources
This query depends on a user-provided value.
Danielius1922
force-pushed
the
adam/feature/crl-1
branch
from
36e6847
to
0b23411
Compare
There was a problem hiding this comment.
Actionable comments posted: 32
🧹 Outside diff range and nitpick comments (26)
pkg/security/x509/crl.go (1)
8-17: Enhance error handling and messagesWhile the implementation is functional, consider these improvements:
- Wrap the original error for better debugging
- Provide more specific error messages about URL requirements
Consider this enhancement:
func ValidateCRLDistributionPointAddress(s string) error { u, err := url.ParseRequestURI(s) if err != nil { - return fmt.Errorf("invalid address(%s)", s) + return fmt.Errorf("invalid CRL distribution point address %q: %w", s, err) } if !u.IsAbs() { - return fmt.Errorf("invalid relative URL address(%s)", s) + return fmt.Errorf("CRL distribution point address %q must be absolute URL", s) } return nil }certificate-authority/service/http/requestHandler.go (1)
57-59: Consider improving the error message specificity.The error message could be more specific about what failed during handler registration.
- return nil, fmt.Errorf("failed to register certificate-authority handler: %w", err) + return nil, fmt.Errorf("failed to register certificate-authority gRPC gateway handler: %w", err)certificate-authority/store/mongodb/store.go (1)
37-51: LGTM: Well-designed iterator implementationThe generic MongoIterator implementation provides a clean abstraction over mongo.Cursor with proper error handling and type safety.
Consider adding a doc comment to describe the purpose of MongoIterator:
// MongoIterator provides a generic type-safe iterator over MongoDB cursor resultscertificate-authority/store/store.go (1)
53-54: Consider separating Get and Issue operationsThe method
GetLatestIssuedOrIssueRevocationListcombines two distinct operations. This could make error handling and testing more complex. Consider splitting into two methods:
GetLatestValidRevocationListIssueNewRevocationListThis separation would:
- Make the interface more predictable
- Simplify error handling
- Make testing easier
- Allow for more specific error cases
certificate-authority/pb/service.proto (1)
80-86: Consider separating revocation from cleanup operations.While the documentation improvement clarifies the dual purpose, consider:
- Splitting this into two separate endpoints:
/certificate-authority/api/v1/signing/records/revokefor certificate revocation/certificate-authority/api/v1/signing/records/cleanupfor expired record deletionThis separation would:
- Make the API more intuitive
- Allow different authorization policies for each operation
- Enable better monitoring and auditing of revocation events
certificate-authority/service/grpc/deleteSigningRecords_test.go (2)
25-28: Consider using test helpers for JWT token creation.While the current implementation works, consider extracting the token creation logic into a test helper function since it might be reused across other test files dealing with authentication.
+func createTestToken(t *testing.T, owner string) string { + return config.CreateJwtToken(t, jwt.MapClaims{ + ownerClaim: owner, + }) +}
Line range hint
1-119: Consider adding CRL-specific test cases.While the basic CRUD operations are well tested, consider adding specific test cases for CRL-related scenarios:
- Deleting records for revoked certificates
- Handling multiple records with the same issuer
- Validation of serial number formats
Would you like me to help draft these additional test cases?
certificate-authority/service/cleanDatabase_test.go (1)
Line range hint
93-94: Consider making the test more reliable.The test uses real time delays (
time.Sleep(4 * time.Second)), which could lead to flaky tests in CI environments. Consider using a mock clock or time provider to make the test more deterministic.Example approach using a mock clock:
type mockClock struct { currentTime time.Time } func (m *mockClock) Now() time.Time { return m.currentTime } func (m *mockClock) Advance(d time.Duration) { m.currentTime = m.currentTime.Add(d) }certificate-authority/service/grpc/getSigningRecords_test.go (1)
92-92: Consider using url.Parse for robust URL construction.While the URL construction works, it would be more robust to use
url.Parseorurl.URLto handle URL construction properly.-"https://"+config.CERTIFICATE_AUTHORITY_HTTP_HOST +(&url.URL{ + Scheme: "https", + Host: config.CERTIFICATE_AUTHORITY_HTTP_HOST, +}).String()certificate-authority/service/http/signCertificate_test.go (1)
Line range hint
1-130: Consider adding comprehensive CRL-specific test casesSince this is part of a Certificate Revocation List implementation, consider adding test cases that specifically verify:
- Certificates are always issued with unique serial numbers
- Revoked certificate serials are tracked
- New certificates cannot reuse serials from revoked certificates
This will ensure the CRL implementation maintains proper certificate uniqueness and tracking.
certificate-authority/service/config.go (2)
44-47: Improve error message specificity.The current error message wraps the entire signer config, making it unclear which parameter caused the error. Consider making it more specific to help with troubleshooting.
- return fmt.Errorf("signer('%v') - %w", c.Signer, err) + return fmt.Errorf("failed to create signer: %w", err)
69-71: Add field documentation.Consider adding documentation for the ExternalAddress field to explain its purpose and format requirements.
type HTTPConfig struct { + // ExternalAddress is the publicly accessible URL for the CRL distribution point + // It must be a valid HTTPS URL that clients can use to fetch certificate revocation lists ExternalAddress string `yaml:"externalAddress" json:"externalAddress"` Addr string `yaml:"address" json:"address"` Server httpServer.Config `yaml:",inline" json:",inline"` }certificate-authority/test/service.go (2)
25-31: Consider adding host configuration validationWhile the function correctly sets up the HTTP configuration, it might be beneficial to add validation for the
CERTIFICATE_AUTHORITY_HTTP_HOSTconstant to ensure it's properly formatted and available.func MakeHTTPConfig() service.HTTPConfig { + if config.CERTIFICATE_AUTHORITY_HTTP_HOST == "" { + panic("CERTIFICATE_AUTHORITY_HTTP_HOST is not set") + } return service.HTTPConfig{ ExternalAddress: "https://" + config.CERTIFICATE_AUTHORITY_HTTP_HOST, Addr: config.CERTIFICATE_AUTHORITY_HTTP_HOST, Server: config.MakeHttpServerConfig(), } }
33-43: Enhance CRL configuration documentation and flexibilityA few suggestions for improvement:
- The MongoDB-only limitation for CRL should be documented with a comment explaining why.
- Consider making the expiration time configurable through environment variables or configuration.
+// MakeCRLConfig creates a CRL configuration. Currently, CRL is only supported +// for MongoDB as it requires specific storage capabilities for managing +// revocation lists efficiently. func MakeCRLConfig() grpc.CRLConfig { if config.ACTIVE_DATABASE() == database.MongoDB { return grpc.CRLConfig{ Enabled: true, - ExpiresIn: time.Hour, + ExpiresIn: config.GetEnvDuration("CRL_EXPIRES_IN", time.Hour), } } return grpc.CRLConfig{ Enabled: false, } }certificate-authority/service/grpc/signer_internal_test.go (1)
117-146: Enhance CRL test coverage with additional scenarios.Good progress on CRL test cases! Consider adding these scenarios to strengthen the test coverage:
- Empty CRL server address with CRL-enabled configuration
- Edge cases:
- CRL server with trailing slash
- CRL server with query parameters
- Zero/negative ExpiresIn duration
Example test case:
{ name: "crl enabled but empty server", args: args{ signerConfig: SignerConfig{ CertFile: urischeme.URIScheme(fullChainCrt), KeyFile: urischeme.URIScheme(intermediate2Key), CRL: CRLConfig{ Enabled: true, ExpiresIn: time.Hour, }, }, crlServer: "", }, wantErr: true, },certificate-authority/service/service.go (1)
123-123: Review direct database access in HTTP handlers.The change to pass
dbStoragedirectly to HTTP service instead of accessing through the CA server suggests a potential architectural concern. Consider:
- Whether this bypasses important business logic in the CA server
- If proper encapsulation is maintained
- Whether all database operations are properly audited and logged
This architectural change might impact:
- Security (direct DB access vs. controlled access through CA server)
- Maintainability (scattered business logic)
- Auditability (tracking certificate operations)
Consider keeping the database access encapsulated within the CA server and exposing only necessary operations through well-defined interfaces.
certificate-authority/store/mongodb/signingRecords.go (1)
121-121: Add documentation for the RevokeSigningRecords methodConsider adding documentation to explain the method's purpose, parameters, and behavior, especially regarding the handling of expired certificates.
+// RevokeSigningRecords revokes signing records for the given owner and query. +// It handles expired certificates by deleting them without adding them to the revocation list. +// For valid certificates, it adds them to their respective issuer's revocation list before deletion. func (s *Store) RevokeSigningRecords(ctx context.Context, ownerID string, query *store.RevokeSigningRecordsQuery) (int64, error) {certificate-authority/service/http/revocationList_test.go (3)
25-44: LGTM! Consider adding boundary value tests.The validation logic is thorough and handles all necessary CRL fields correctly. However, consider adding test cases for edge cases such as empty certificate lists and maximum-sized serial numbers.
49-49: Consider using a fixed timestamp for tests.Using
time.Now()in tests can lead to flaky behavior. Consider using a fixed timestamp:-now := time.Now() +now := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)
163-168: Add HTTP status code assertions.The test should verify the expected HTTP status codes for different scenarios:
httpResp := httpgwTest.HTTPDo(t, request) +require.Equal(t, http.StatusOK, httpResp.StatusCode) respBody, err := io.ReadAll(httpResp.Body)certificate-authority/service/grpc/signCertificate.go (1)
63-94: Consider enhancing error handling for edge cases.While the function handles the basic cases well, consider these improvements:
- Return a specific error when no record is found (currently returns nil, nil)
- Add logging for better debugging of lookup paths
Consider this enhancement:
func (s *CertificateAuthorityServer) getSigningRecord(ctx context.Context, signingRecord *pb.SigningRecord) (*pb.SigningRecord, error) { // ... existing code ... if err != nil { return nil, err } + if originalSr == nil { + return nil, fmt.Errorf("no signing record found for %s", signingRecord.GetId()) + } return originalSr, nil }certificate-authority/store/mongodb/revocationList_test.go (1)
190-235: Consider improving parallel test reliability and readability.The parallel test implementation has a few areas for improvement:
- The magic number 100 in the retry loop should be a named constant
- The test might be flaky due to timing dependencies
Consider these improvements:
+const ( + maxRetries = 100 + concurrentWorkers = 10 +) -for range 100 { +for range maxRetries {Also, consider adding jitter to the retry mechanism to reduce contention:
time.Sleep(time.Duration(rand.Int63n(100)) * time.Millisecond)certificate-authority/store/cqldb/signingRecords.go (1)
342-367: LGTM with a minor suggestion about error variable naming.The implementation correctly handles the migration to the new Process type and maintains proper error handling. However, consider renaming
errClosetocloseErrto align with Go's common error variable naming conventions.-errClose := i.close() +closeErr := i.close() if err == nil { - return errClose + return closeErr } return errcertificate-authority/store/cqldb/signingRecords_test.go (3)
Line range hint
41-90: Add validation test cases for Serial and IssuerIdThe test suite should include cases that verify the validation of the new fields:
- Invalid serial number formats
- Invalid issuer ID formats (non-UUID)
- Empty/nil values
Consider adding these test cases:
{ name: "invalid serial format", args: args{ sub: &store.SigningRecord{ Id: id1, Owner: "owner", CommonName: "commonName", Credential: &pb.CredentialStatus{ Serial: "invalid", IssuerId: "42424242-4242-4242-4242-424242424242", }, }, }, wantErr: true, }, { name: "invalid issuer ID format", args: args{ sub: &store.SigningRecord{ Id: id1, Owner: "owner", CommonName: "commonName", Credential: &pb.CredentialStatus{ Serial: big.NewInt(42).String(), IssuerId: "invalid", }, }, }, wantErr: true, },
Line range hint
557-600: Add sequential benchmark for baseline comparisonThe current benchmark uses concurrent updates which might mask potential performance issues with the new fields. Consider adding a sequential benchmark for baseline comparison.
Add this benchmark function:
func BenchmarkSigningRecordsSequential(b *testing.B) { // Same setup as BenchmarkSigningRecords for i := uint32(0); i < uint32(b.N); i++ { b.StopTimer() err := s.ClearTable(ctx) require.NoError(b, err) b.StartTimer() // Sequential updates for _, l := range data { err := s.UpdateSigningRecord(ctx, l) assert.NoError(b, err) } } }
Line range hint
1-600: Consider additional security measures for CRL implementationWhile the basic structure for CRL is in place, consider these security aspects:
- Ensure serial numbers are cryptographically secure when generated in production
- Consider adding tests for serial number uniqueness
- Add validation for minimum serial number length
Would you like assistance in implementing these security measures?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (10)
certificate-authority/config.yamlis excluded by!**/*.yamlcertificate-authority/pb/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocertificate-authority/pb/service.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gocertificate-authority/pb/service.swagger.jsonis excluded by!**/*.jsoncertificate-authority/pb/service_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocertificate-authority/pb/signingRecords.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocharts/plgd-hub/templates/certificate-authority/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/values.yamlis excluded by!**/*.yamldevice-provisioning-service/pb/service.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.godevice-provisioning-service/pb/service.swagger.jsonis excluded by!**/*.json
📒 Files selected for processing (56)
- bundle/nginx/nginx.conf.template (1 hunks)
- certificate-authority/pb/README.md (3 hunks)
- certificate-authority/pb/doc.html (7 hunks)
- certificate-authority/pb/service.proto (2 hunks)
- certificate-authority/pb/signingRecords.go (3 hunks)
- certificate-authority/pb/signingRecords.proto (2 hunks)
- certificate-authority/pb/signingRecords_test.go (1 hunks)
- certificate-authority/service/cleanDatabase_test.go (5 hunks)
- certificate-authority/service/config.go (3 hunks)
- certificate-authority/service/config_test.go (1 hunks)
- certificate-authority/service/grpc/config.go (2 hunks)
- certificate-authority/service/grpc/config_test.go (1 hunks)
- certificate-authority/service/grpc/deleteSigningRecords.go (1 hunks)
- certificate-authority/service/grpc/deleteSigningRecords_test.go (4 hunks)
- certificate-authority/service/grpc/getSigningRecords.go (1 hunks)
- certificate-authority/service/grpc/getSigningRecords_test.go (5 hunks)
- certificate-authority/service/grpc/server.go (2 hunks)
- certificate-authority/service/grpc/server_test.go (1 hunks)
- certificate-authority/service/grpc/signCertificate.go (3 hunks)
- certificate-authority/service/grpc/signCertificate_test.go (4 hunks)
- certificate-authority/service/grpc/signIdentityCertificate_test.go (3 hunks)
- certificate-authority/service/grpc/signer.go (6 hunks)
- certificate-authority/service/grpc/signer_internal_test.go (3 hunks)
- certificate-authority/service/http/config.go (1 hunks)
- certificate-authority/service/http/requestHandler.go (1 hunks)
- certificate-authority/service/http/revocationList.go (1 hunks)
- certificate-authority/service/http/revocationList_test.go (1 hunks)
- certificate-authority/service/http/service.go (2 hunks)
- certificate-authority/service/http/signCertificate_test.go (6 hunks)
- certificate-authority/service/http/signIdentityCertificate_test.go (3 hunks)
- certificate-authority/service/service.go (2 hunks)
- certificate-authority/service/uri/uri.go (1 hunks)
- certificate-authority/store/cqldb/revocationList.go (1 hunks)
- certificate-authority/store/cqldb/signingRecords.go (1 hunks)
- certificate-authority/store/cqldb/signingRecords_test.go (18 hunks)
- certificate-authority/store/mongodb/bulkWriter.go (0 hunks)
- certificate-authority/store/mongodb/config.go (1 hunks)
- certificate-authority/store/mongodb/revocationList.go (1 hunks)
- certificate-authority/store/mongodb/revocationList_test.go (1 hunks)
- certificate-authority/store/mongodb/signingRecords.go (3 hunks)
- certificate-authority/store/mongodb/signingRecords_test.go (13 hunks)
- certificate-authority/store/mongodb/store.go (3 hunks)
- certificate-authority/store/revocationList.go (1 hunks)
- certificate-authority/store/revocationList_test.go (1 hunks)
- certificate-authority/store/store.go (2 hunks)
- certificate-authority/test/revocationList.go (1 hunks)
- certificate-authority/test/service.go (2 hunks)
- charts/plgd-hub/README.md (0 hunks)
- device-provisioning-service/Makefile (1 hunks)
- device-provisioning-service/pb/service.proto (4 hunks)
- http-gateway/test/http.go (1 hunks)
- http-gateway/uri/uri.go (1 hunks)
- pkg/security/certificateSigner/certificateSigner.go (5 hunks)
- pkg/security/certificateSigner/certificateSigner_test.go (0 hunks)
- pkg/security/certificateSigner/identityCertificateSigner_test.go (0 hunks)
- pkg/security/x509/crl.go (1 hunks)
💤 Files with no reviewable changes (4)
- certificate-authority/store/mongodb/bulkWriter.go
- charts/plgd-hub/README.md
- pkg/security/certificateSigner/certificateSigner_test.go
- pkg/security/certificateSigner/identityCertificateSigner_test.go
🚧 Files skipped from review as they are similar to previous changes (24)
- bundle/nginx/nginx.conf.template
- certificate-authority/pb/README.md
- certificate-authority/pb/signingRecords.proto
- certificate-authority/pb/signingRecords_test.go
- certificate-authority/service/config_test.go
- certificate-authority/service/grpc/config_test.go
- certificate-authority/service/grpc/deleteSigningRecords.go
- certificate-authority/service/grpc/getSigningRecords.go
- certificate-authority/service/grpc/server.go
- certificate-authority/service/grpc/server_test.go
- certificate-authority/service/grpc/signCertificate_test.go
- certificate-authority/service/grpc/signIdentityCertificate_test.go
- certificate-authority/service/http/config.go
- certificate-authority/service/http/service.go
- certificate-authority/service/http/signIdentityCertificate_test.go
- certificate-authority/service/uri/uri.go
- certificate-authority/store/cqldb/revocationList.go
- certificate-authority/store/mongodb/config.go
- certificate-authority/store/revocationList_test.go
- certificate-authority/test/revocationList.go
- device-provisioning-service/Makefile
- device-provisioning-service/pb/service.proto
- http-gateway/test/http.go
- http-gateway/uri/uri.go
🧰 Additional context used
📓 Learnings (2)
certificate-authority/store/mongodb/revocationList.go (1)
Learnt from: Danielius1922 PR: plgd-dev/hub#1383 File: certificate-authority/store/mongodb/signingRecords.go:139-141 Timestamp: 2024-10-28T17:11:50.454Z Learning: In `certificate-authority/store/mongodb/signingRecords.go`, within the `RevokeSigningRecords` method, expired signing records are deleted but not added to the revocation list.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-10-28T17:11:50.454Z Learning: In `certificate-authority/store/mongodb/signingRecords.go`, within the `RevokeSigningRecords` method, expired signing records are deleted but not added to the revocation list.
🪛 GitHub Check: CodeQL
certificate-authority/store/mongodb/revocationList.go
[failure] 217-217: Database query built from user-controlled sources
This query depends on a user-provided value.
🔇 Additional comments (75)
pkg/security/x509/crl.go (2)
1-6: LGTM: Clean package structure with appropriate importsThe package name and imports are well-organized and follow Go conventions.
8-17: Verify consistent URL validation across the codebaseLet's ensure this validation is consistently applied wherever CRL distribution points are handled.
✅ Verification successful
Let me gather more specific information about the CRL-related code to make a better assessment.
Let me search for any other URL validation functions and CRL-related code to make a thorough assessment.
Based on the search results, I can now provide a final assessment. The code shows that the
ValidateCRLDistributionPointAddressfunction is consistently used throughout the codebase:
- It's used in the certificate authority's HTTP configuration validation (
certificate-authority/service/config.go)- It's used in the certificate signer's CRL setup (
certificate-authority/service/grpc/signer.go)- The CRL distribution points are properly propagated through the certificate signer configuration (
pkg/security/certificateSigner/certificateSigner.go)The validation is appropriately applied at all points where CRL distribution point URLs are handled, ensuring consistent URL validation across the codebase.
URL validation is consistently applied for CRL distribution points
The validation function is used at all required points in the codebase where CRL distribution point URLs are configured or validated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other CRL distribution point URL handling in the codebase # that might need to use this validation function # Look for CRL-related URL handling rg -l "(?i)(crl|distribution.*point|revocation.*list).*\b(url|address|endpoint)\b" # Look for direct URL parsing that might need this validation ast-grep --pattern 'url.Parse$_($$$)'Length of output: 224
Script:
#!/bin/bash # Check the actual content of files that mention CRL/distribution points rg -A 5 -B 5 "(?i)(crl|distribution.*point|revocation.*list).*\b(url|address|endpoint)\b" # Look specifically for imports or usage of the x509 package where this validation function resides rg -l "\".*security/x509\"" # Check for any other URL validation functions that might be doing similar work ast-grep --pattern 'func $_($_ string) error { $$$ url.Parse$_($$$) $$$ }'Length of output: 60814
Script:
#!/bin/bash # Look for URL validation functions and CRL-related code rg -A 5 -B 5 "func.*ValidateURL|func.*ValidateAddress|func.*ValidateCRL" # Check for CRL distribution point URL usage rg -A 5 -B 5 "CRLDistributionPoint|DistributionPoint.*URL" # Look for URL validation in tests rg -A 5 "url.Parse.*test"Length of output: 6555
certificate-authority/service/http/requestHandler.go (4)
5-5: LGTM: Import statements are well-organized.The new imports are properly organized and necessary for the added functionality.
Also applies to: 7-7, 14-15
19-25: LGTM: Type definition follows Go best practices.The struct has been properly made package-private and the new fields are well-organized.
30-41: LGTM: Parameter validation is thorough.The implementation properly validates all required dependencies, preventing potential nil pointer panics.
49-50: Address pending CRL-related concerns.While the CRL route registration is implemented correctly, there are two important items that need to be addressed:
- Cache Control Headers: The CRL endpoint should include appropriate cache control headers to optimize performance.
- Documentation: The CRL feature flag needs proper documentation and configuration guidance.
These issues were previously identified in the past reviews and still need to be addressed. Would you like assistance with implementing either of these improvements?
certificate-authority/pb/signingRecords.go (2)
6-6: LGTM: Appropriate import for certificate serial number handling.The addition of
math/bigis correct for handling certificate serial numbers which can be arbitrarily large integers.
67-69: LGTM: Clean delegation of credential validation.The changes correctly delegate credential validation to the new
CredentialStatus.Validatemethod, reducing code duplication.Let's verify that all credential validations are properly handled:
✅ Verification successful
Based on the verification results, I can now provide a final response:
Credential validation delegation is properly implemented and tested
The verification confirms:
- The
CredentialStatus.Validate()method has proper test coverage incertificate-authority/pb/signingRecords_test.go- The delegation pattern is correctly implemented with proper nil checks
- No other credential validation implementations that could conflict were found
- The validation logic is comprehensive, checking all required fields (date, validUntilDate, certificatePem, serial, issuerId)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any missed credential validations in tests or related files # Test 1: Look for test coverage of credential validation rg -l "TestCredentialStatus.*Validate|TestSigningRecord.*Validate" --type go # Test 2: Check for any direct field access that might bypass validation ast-grep --pattern 'credential.$$$' --type go # Test 3: Look for other credential validation implementations that might conflict rg "func.*Validate.*Credential" --type goLength of output: 398
Script:
#!/bin/bash # Let's improve our verification with correct syntax and broader checks # Check for test coverage rg "func Test.*Credential.*|func Test.*SigningRecord.*" --type go -A 5 # Check credential validation patterns correctly ast-grep --pattern 'if credential := $_.GetCredential(); credential != nil { $$$ }' # Look for credential-related validation methods rg "func.*Validate" --type go -A 2 # Check direct credential field access rg "credential\." --type go -A 2Length of output: 87638
certificate-authority/service/grpc/config.go (2)
40-40: LGTM!The CRL configuration is properly integrated into the SignerConfig structure with appropriate serialization tags.
16-22: Enhance security constraints for CRL configurationThe current CRL configuration lacks several important security constraints:
- No maximum validity period for CRLs is enforced
- No validation exists to ensure CRL validity period doesn't exceed the CA certificate's validity
- Consider adding a minimum update frequency field to ensure timely revocation updates
Let's check if these constraints are handled elsewhere:
Consider adding:
- MaxValidityPeriod field with a reasonable default (e.g., 7 days)
- MinUpdateFrequency field to enforce regular updates
- Validation to ensure CRL validity doesn't exceed the CA cert's validity
certificate-authority/store/mongodb/store.go (4)
7-7: LGTM: Store struct refactoringThe removal of bulkWriter in favor of logger and the addition of multierror handling aligns well with the architectural changes described in the PR objectives.
Also applies to: 20-20
53-75: Previous review comment is still applicableThe suggestion to continue processing after encountering an error remains valid for this code segment.
100-103: LGTM: Improved error handling in clearDatabasesThe use of multierror for aggregating collection drop errors provides better error reporting.
82-92: LGTM: Improved store initializationThe use of NewStoreWithCollections provides better initialization structure. However, let's verify the index configuration.
✅ Verification successful
The search results show consistent usage of MongoDB indexes across different services. For the certificate-authority service, the index configuration appears correct and follows the established patterns:
The
signingRecordsColhas two indexes:
commonNameKeyQueryIndexfor efficient queries on common name + ownerdeviceIDKeyQueryIndexfor efficient queries on device ID + ownerThe
revocationListColhas no indexes which is acceptable if it doesn't require specific query patterns.The implementation aligns with other services in the codebase that use similar compound indexes for owner-based queries. For example:
- identity-store uses owner+deviceID indexes
- m2m-oauth-server uses ID+owner indexes
- device-provisioning-service uses similar owner-based compound indexes
Index configuration is properly implemented
The index setup follows the established patterns across the codebase and provides efficient querying capabilities for the certificate authority's common use cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify index configuration consistency across the codebase # Look for other collections that might need similar indexes # Search for collection index definitions rg -g '*.go' "mongo\.IndexModel" -A 5Length of output: 15127
certificate-authority/store/store.go (2)
13-17: LGTM: Well-defined error variablesThe new error variables follow Go conventions and provide clear, specific error cases needed for CRL operations.
56-57: Verify transaction safety for revocation operationThe operation involves two steps: removing records and adding them to the revocation list. This should be atomic to prevent data inconsistency.
certificate-authority/pb/service.proto (3)
57-60: LGTM! The additional binding maintains API consistency.The new endpoint follows the same pattern as SignIdentityCertificate, maintaining a consistent API structure.
67-73: Enhance documentation for streaming behavior.While the endpoint addition is good, the documentation "Get signed certificate records" could be more descriptive about:
- The streaming nature of the response
- Any pagination or batch size limitations
- Expected response format
Additionally, note that the previous review comments about streaming implementation (context cancellation, backpressure management) should still be addressed in the handler implementation.
41-44: LGTM! Verify security controls for the new endpoint.The additional binding follows consistent API design patterns. Ensure that the new endpoint
/certificate-authority/api/v1/sign/identity-csris properly secured with the same authentication and authorization controls as the existing endpoint.✅ Verification successful
Based on the evidence gathered, I can now provide a final response regarding the security of the new endpoint:
The new endpoint is properly secured with authentication controls
The code analysis reveals that:
- The service uses a common authentication middleware for all endpoints (
CreateAuthMiddleware)- JWT token validation is implemented through the
validatorpackage- Both HTTP and gRPC services are configured with proper authorization:
- HTTP service is initialized with auth middleware and token validation
- gRPC service uses auth interceptors via
server.NewAuth(validator)- The new endpoint inherits these security controls as part of the service's global configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if authentication middleware is consistently applied rg -A 5 "certificate-authority/api/v1/sign/identity-csr" bundle/nginx/nginx.conf.templateLength of output: 89
Script:
#!/bin/bash # Let's try a broader search to understand the security setup # First, check for any auth middleware or security configurations rg -i "auth.*middleware|security.*config" -A 5 # Also search for the base path to see routing setup rg "/certificate-authority/api" -A 5 # Check for any gRPC interceptors or middleware in Go files ast-grep --pattern 'func $_(ctx context.Context, req *$_) (*$_, error) { $$$ }'Length of output: 124199
Script:
#!/bin/bash # Let's check for authentication middleware configuration in the certificate authority service rg -A 10 "CreateAuthMiddleware|authMiddleware" certificate-authority/ # Also check for any security interceptors in the service implementation ast-grep --pattern 'func New$_($_) (*$_, error) { $$$ }' # And look for any security configurations in the service setup rg -A 5 "security.*config|auth.*config" certificate-authority/Length of output: 1985
Script:
#!/bin/bash # Let's check the HTTP server configuration and middleware setup rg -A 10 "func New.*Server|func main" certificate-authority/service/ # Check for any gRPC interceptors that might handle auth rg -A 10 "UnaryServerInterceptor|StreamServerInterceptor" certificate-authority/ # Look for JWT or auth token validation code rg -A 5 "jwt|token|auth.*valid" certificate-authority/Length of output: 18942
certificate-authority/store/revocationList.go (1)
1-10: LGTM: Package structure and imports are appropriate.The package name and imports align well with the CRL storage functionality.
certificate-authority/service/grpc/deleteSigningRecords_test.go (5)
5-5: LGTM: Required imports for enhanced functionality.The addition of
math/bigis appropriate for handling certificate serial numbers, andpkgGrpcis needed for token context management.Also applies to: 17-17
53-62: LGTM: Good security test coverage.The new test case for missing token is a valuable addition to security testing, ensuring proper authentication checks.
109-113: LGTM: Improved error handling in tests.The error handling has been enhanced with proper checks and early returns, making the test results more reliable.
100-100: Verify HTTPS configuration for production readiness.The server initialization now includes the HTTPS host, which is good. However, we should ensure this aligns with the CRL distribution point requirements.
#!/bin/bash # Check if the HTTPS configuration is consistently used for CRL endpoints echo "Checking HTTPS configuration in CRL related code..." rg "crl.*endpoint|distribution.*point" echo "Checking certificate authority HTTP host usage..." rg "CERTIFICATE_AUTHORITY_HTTP_HOST"
39-40: Verify serial number and issuer ID format consistency.The test uses appropriate fields for certificate tracking, but we should verify that these formats align with the actual CRL implementation requirements.
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the serial number and issuer ID format consistency:
Serial number and issuer ID formats are consistent with implementation requirements
The verification shows:
- The serial number format using
big.NewInt(42).String()in the test aligns with theRevocationListCertificatestruct which stores serials as strings- The issuer ID format using UUID pattern "42424242-4242-4242-4242-424242424242" is consistent across all test files and is validated in the codebase (pb/signingRecords.go validates issuer IDs using uuid.Parse)
- This format is used consistently across multiple test files in the certificate-authority module, including store implementations for both MongoDB and CQLDB
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent serial number and issuer ID formats across the codebase echo "Checking serial number format consistency..." rg -A 2 "Serial.*big\.NewInt|Serial.*String\(\)" echo "Checking issuer ID format consistency..." rg -A 2 "IssuerId.*[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}"Length of output: 2216
Script:
#!/bin/bash # Let's try a more focused search for serial numbers and issuer IDs # Check serial number formats in test files echo "Checking serial number formats in tests..." rg -l "Serial.*" --type go --type test # Check actual serial number implementations echo -e "\nChecking serial number implementations..." rg -A 3 "type.*Record.*struct" --type go # Check issuer ID patterns echo -e "\nChecking issuer ID patterns..." rg -A 2 "IssuerId.*" --type go # Look for any CRL or certificate record related structs echo -e "\nChecking certificate record structures..." ast-grep --pattern 'type $_ struct { $$$ Serial $$$ $$$ }'Length of output: 16807
certificate-authority/service/cleanDatabase_test.go (3)
8-8: LGTM: Import changes align with new requirements.The addition of
math/bigsupports the new Serial field, and the package alias change maintains consistency across the codebase.Also applies to: 21-21
79-79: LGTM: Package alias update.The context creation logic remains unchanged, only the package reference has been updated.
#!/bin/bash # Description: Verify consistent usage of pkgGrpc across the codebase # Expect: No instances of kitNetGrpc remain rg 'kitNetGrpc' --type go
52-53: Verify UUID format for IssuerId field.The new fields enhance certificate tracking capabilities. However, let's verify the UUID format used in other test cases.
✅ Verification successful
UUID format is consistent across test files
The IssuerId field consistently uses the UUID format "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" across all test files in the certificate-authority module. The test value "42424242-4242-4242-4242-424242424242" follows this standard format.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check UUID format consistency across test files # Expected format: "42424242-4242-4242-4242-424242424242" rg -A 1 'IssuerId.*:.*"[0-9a-f-]{36}"' --type goLength of output: 46650
Script:
#!/bin/bash # Search for UUID format patterns in test files rg -A 1 'IssuerId.*:.*"[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}"' --type goLength of output: 4908
certificate-authority/service/grpc/getSigningRecords_test.go (3)
7-7: LGTM: Required import for certificate serial number handling.The addition of "math/big" import is appropriate for handling certificate serial numbers as large integers.
101-101: LGTM: Consistent with package refactoring.The context creation change aligns with the package alias update.
20-20: Verify consistent usage of the new package alias.The package alias has been changed from
kitNetGrpctopkgGrpc. Let's verify this change is consistent across the codebase.#!/bin/bash # Description: Check for any remaining references to the old package alias # and verify consistent usage of the new one echo "Checking for old alias usage..." rg "kitNetGrpc" -l echo "Verifying new alias usage pattern..." rg "pkgGrpc.*CtxWithToken" -lpkg/security/certificateSigner/certificateSigner.go (3)
Line range hint
66-82: LGTM! Well-structured CSR parsing functionThe function properly handles:
- PEM decoding
- CSR parsing
- Signature validation
- Error cases
18-21: Consider validating CRL distribution points URLsWhile the addition of
CRLDistributionPointsis correct, consider adding validation to ensure the URLs are properly formatted and accessible. This could prevent issues with invalid CRL endpoints being configured.Let's check if there's URL validation elsewhere in the codebase:
Consider implementing validation either:
- At configuration time using a validator interface
- In the WithCRLDistributionPoints option function
Line range hint
84-128: Consider CRL update frequency in certificate validity periodThe certificate validity period should be aligned with CRL update frequency to ensure timely revocation capability. Long-lived certificates with infrequently updated CRLs could pose security risks.
Let's check for CRL update configuration:
Consider:
- Documenting the relationship between certificate validity and CRL update frequency
- Adding validation to ensure the certificate validity period doesn't exceed maximum recommended duration based on CRL update frequency
- Implementing CRL caching and refresh mechanisms if not already present
certificate-authority/service/http/signCertificate_test.go (4)
16-16: LGTM: Import changes improve maintainabilityThe centralization of URI definitions through
certAuthURIand the update topkgGrpcalign with best practices for configuration management.Also applies to: 18-18
86-86: LGTM: Token extraction update is consistentThe update to use
pkgGrpc.TokenFromOutgoingMDmaintains consistent token handling across the codebase.
116-116: LGTM: URI usage is consistentThe update to use
certAuthURI.SignCertificatemaintains consistent URI management. However, let's verify all certificate-related endpoints are using the centralized URIs.#!/bin/bash # Search for any hardcoded certificate-related URIs that should use certAuthURI rg -g '!*.md' -g '!*.txt' "/v[0-9]+/api/certificate-authority"Also applies to: 128-128
70-70: Verify token validation in the service implementationThe token handling update looks good. However, since this is part of a Certificate Revocation List implementation, we should verify that the service properly validates tokens for certificate operations.
✅ Verification successful
Based on the gathered information, I can now generate the final response. The code shows that authentication and token validation are properly implemented in the certificate authority service:
Token validation is properly implemented in the service
The service has comprehensive token validation through:
- HTTP service using
validator.Configfor authorization configuration- gRPC service using auth interceptors for token validation
- Token validation before certificate operations through
ownerToUUIDfunction that extracts and validates owner from tokenThe token handling update in the test is consistent with the service's security implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for token validation in the service implementation rg -A 5 "func.*Sign.*Certificate" | rg -B 5 "ValidateToken|Authentication"Length of output: 64
Script:
#!/bin/bash # Let's search for the service implementation of SignCertificate ast-grep --pattern 'func ($service $_) SignCertificate($ctx context.Context, $req $_) ($resp $_)'Length of output: 99
Script:
#!/bin/bash # Let's also check for any authentication middleware or interceptors rg -g '!*.md' "func.*(Authentication|Auth).*Interceptor" -A 5Length of output: 6899
Script:
#!/bin/bash # Search for token validation in the context of certificate operations rg -g '!*.md' "ValidateToken|Authentication" certificate-authority/Length of output: 69
Script:
#!/bin/bash # Let's check how the service is initialized and configured with auth rg -g '!*.md' "NewService|MakeService" certificate-authority/ -A 10Length of output: 69
Script:
#!/bin/bash # Let's also check the main function to see how auth is set up rg -g '!*.md' "func main" certificate-authority/ -A 20Length of output: 1084
Script:
#!/bin/bash # Check for gRPC server setup in certificate authority rg -g '!*.md' "grpc.NewServer|Listen" certificate-authority/ -B 10 -A 10Length of output: 74
Script:
#!/bin/bash # Let's check the run function in the certificate authority service rg -g '!*.md' "func run" certificate-authority/ -B 5 -A 20Length of output: 1764
Script:
#!/bin/bash # Let's also check the service configuration structure rg -g '!*.md' "type Config struct" certificate-authority/ -A 15Length of output: 3358
Script:
#!/bin/bash # Check for any auth-related imports in the service files rg -g '!*.md' '^import \($' -A 15 certificate-authority/service/Length of output: 35368
certificate-authority/service/config.go (1)
16-16: LGTM!The import is correctly aliased and properly used for CRL distribution point validation.
certificate-authority/test/service.go (3)
10-10: LGTM: Import added for grpc packageThe import is necessary for accessing the CRLConfig type used in the new CRL implementation.
53-59: LGTM: Good modularization of configurationThe changes improve the code organization by:
- Extracting HTTP configuration into a separate function
- Adding CRL configuration in a logical location
- Maintaining the validation step for the complete configuration
33-43: Verify CRL implementation completenessSince this is part of the CRL implementation, let's verify the related components:
✅ Verification successful
CRL implementation appears complete and properly integrated
The verification shows a complete CRL implementation with all necessary components in place:
- CRL configuration is properly validated with appropriate constraints (minimum 1-minute expiry)
- HTTP handler is registered when CRL is enabled (
/revocation-listendpoint)- MongoDB implementation includes proper CRL storage and handling
- CRL distribution points are correctly configured in certificates when enabled
- Comprehensive test coverage exists for both config validation and functionality
The conditional CRL configuration based on MongoDB in the test file is consistent with the overall implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify CRL implementation completeness # Test 1: Check if CRL handler is properly registered in HTTP server echo "Checking CRL handler registration..." rg -p "CRLEnabled.*?{.*?}" -A 10 # Test 2: Verify MongoDB implementation for CRL echo "Checking MongoDB CRL implementation..." ast-grep --pattern 'func ($HANDLER *$TYPE) HandleCRL($CTX context.Context, $REQ *$REQ_TYPE)' # Test 3: Look for CRL-related tests echo "Checking CRL test coverage..." fd -e go -x rg -l "TestCRL"Length of output: 563
Script:
#!/bin/bash # Let's try different search patterns to verify CRL implementation # Check for CRL-related code in MongoDB implementation echo "Checking MongoDB CRL implementation..." rg -p "CRL" -t go ./certificate-authority -A 5 # Check for HTTP/gRPC handler registration echo "Checking handler registration..." rg -p "func.*CRL" -t go ./certificate-authority # Check the test file found earlier echo "Checking CRL test content..." cat ./certificate-authority/service/grpc/config_test.go # Look for CRL configuration usage echo "Checking CRL config usage..." rg "CRLConfig" -t go ./certificate-authorityLength of output: 15353
certificate-authority/service/grpc/signer_internal_test.go (1)
7-7: LGTM: Import and struct field additions are appropriate.The addition of the time import and crlServer field properly supports the new CRL functionality.
Also applies to: 52-52
certificate-authority/service/service.go (2)
122-123: Verify CRL feature flag implementation.The addition of
CRLEnabledconfiguration is good for feature control, but please ensure:
- Default configuration values are documented
- Error handling exists for scenarios where CRL is enabled but required configuration is missing
- Migration guide exists for enabling CRL in production
Let's verify the CRL configuration handling:
#!/bin/bash # Check for CRL configuration documentation rg -g 'README*' -g '*.md' 'CRL.*configuration|Certificate Revocation List' # Verify error handling when CRL is enabled ast-grep --pattern 'if $_.CRLEnabled { $$$ }'
103-103: Verify the external address configuration for CRL distribution.The addition of
ExternalAddressparameter is likely for CRL distribution points in certificates. Ensure that:
- The external address is properly configured in production environments
- The address is accessible from certificate consumers
Let's verify the external address usage:
✅ Verification successful
External address configuration for CRL distribution is properly integrated
The verification shows that:
- The external address is validated through
ValidateCRLDistributionPointAddressto ensure it's an absolute URL- It's used as
crlServerAddressin the CA server and signer for CRL distribution points- The configuration is properly tested and validated in both test and production code
- The CRL functionality is properly guarded with configuration options (
CRLConfig.Enabled)The changes are well-integrated and maintain proper validation of the external address configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CRL distribution point URL construction ast-grep --pattern 'config.APIs.HTTP.ExternalAddress' # Verify if the external address is used in CRL-related code rg -A 5 'ExternalAddress.*CRL'Length of output: 126
Script:
#!/bin/bash # Let's search for the usage of ExternalAddress in the codebase rg -A 5 "ExternalAddress" # Search for CRL-related code patterns ast-grep --pattern 'type CertificateAuthorityServer struct { $$$ }' # Search for NewCertificateAuthorityServer implementation ast-grep --pattern 'func NewCertificateAuthorityServer($$$) ($_, $_) { $$$ }'Length of output: 12304
Script:
#!/bin/bash # Let's verify the CRL-related functionality and external address usage rg -A 5 "crlServerAddress" # Search for CRL distribution point validation ast-grep --pattern 'func ValidateCRLDistributionPointAddress($_) $_' # Search for CRL configuration validation rg -A 5 "CRLConfig"Length of output: 12478
certificate-authority/store/mongodb/signingRecords.go (4)
Line range hint
22-27: LGTM! Good simplification of error handling.The simplified error handling maintains the same functionality while making the code more concise.
178-188: LGTM! Good use of generics and error handling.The updated method signature using
store.Process[store.SigningRecord]and the proper handling ofmongo.ErrNilDocumentdemonstrate good practices.
29-39:⚠️ Potential issueSecurity Concern: Include 'owner' in the update filter
The security concern from the previous review is still valid. The current implementation allows updating records based solely on
_id, which could permit unauthorized updates of records owned by other users.Apply this diff to include the owner in the filter:
- filter := bson.M{"_id": signingRecord.GetId()} + filter := bson.M{ + "_id": signingRecord.GetId(), + store.OwnerKey: signingRecord.Owner, + }
156-166: 🛠️ Refactor suggestionImprove error handling for revocation list updates
The current implementation stops at the first error when updating revocation lists. Consider aggregating errors to ensure all valid records are processed.
Apply this diff to aggregate errors:
+ var resultErr error for issuerID, record := range irs { query := store.UpdateRevocationListQuery{ IssuerID: issuerID, RevokedCertificates: record.certificates, } _, err := s.UpdateRevocationList(ctx, &query) if err != nil { - return 0, err + resultErr = multierror.Append(resultErr, err) } } + if resultErr != nil { + return 0, resultErr + }certificate-authority/service/grpc/signCertificate.go (4)
Line range hint
29-60: LGTM: Well-structured implementation of issuerID handling.The changes to include issuerID in the signing record are properly implemented, with good practices for UUID generation and time handling.
136-149: LGTM: Clean implementation of signing record updates.The function properly orchestrates the signing record update process, with good error handling and conditional revocation list updates.
166-170: LGTM: Good defensive programming with credential validation.The added nil check for the credential and improved error handling enhance the robustness of the certificate signing process.
96-133: Consider potential race condition in revocation list creation.When creating a new revocation list, there's a potential race condition between checking for existence and creation. While the current error handling catches duplicate IDs, consider using a more atomic operation if your store supports it.
Let's verify the revocation list handling in the codebase:
Consider implementing an atomic "upsert" operation in the store interface if supported by your database:
// Example interface addition type Store interface { UpsertRevocationList(ctx context.Context, list *store.RevocationList) error }certificate-authority/service/grpc/signer.go (7)
27-30: LGTM: Well-structured CRL configuration.The new
crlstruct field is well-organized, containing both the server address and validity duration for the CRL configuration.
49-55: LGTM: Robust issuer ID generation.The function generates a deterministic UUID based on the certificate's public key, ensuring consistent issuer IDs across restarts.
77-83: LGTM: Proper CRL configuration validation.Good practice to validate the CRL server address before enabling CRL functionality.
108-108: LGTM: Clean integration of CRL support.The updates maintain the existing certificate validation logic while cleanly integrating CRL support for both root and intermediate CA scenarios.
Also applies to: 130-130
162-167: LGTM: Clean separation of signer types.Good practice to separate identity certificate signer from standard certificate signer while reusing common configuration logic.
195-200: LGTM: Well-designed public interface.The public signing methods provide a clean interface while properly handling both standard and identity certificate signing scenarios.
146-147: Verify certificate chain access requirements.The
GetCertificatemethod only returns the leaf certificate. Verify if access to the full certificate chain is needed by any consumers.✅ Verification successful
Let me gather more context about the certificate chain usage by searching for related code patterns.
Let me search for the actual usage of the certificate chain to understand if the full chain is needed.
Based on the analysis of the code, I can now provide the final response:
GetCertificate implementation is correct as is.
The current implementation returning only the leaf certificate (
s.certificate[0]) is appropriate because:
- The only consumer (
createCRL) only needs the leaf certificate for CRL generation- The full chain verification is handled during signer initialization, not by consumers
- The certificate chain is maintained internally by the signer for its own verification purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of GetCertificate to verify if full chain access is needed rg -A 3 "GetCertificate\(\)"Length of output: 1506
Script:
#!/bin/bash # Search for certificate chain related patterns rg -A 3 "certificate\[|certChain|CertChain|chain" # Also check the struct definition and initialization ast-grep --pattern 'type Signer struct { $$$ certificate $$$ $$$ }'Length of output: 33691
Script:
#!/bin/bash # Search for certificate chain usage in the CA service rg -A 5 "s\.certificate\[|GetCertificate\(\)" certificate-authority/ # Search for certificate chain related functions ast-grep --pattern 'func $name($_) $ret { $$$ certificate[$_] $$$ }'Length of output: 1783
certificate-authority/store/mongodb/revocationList_test.go (2)
1-17: LGTM! Package declaration and imports are well-organized.The test package is correctly suffixed with _test and includes all necessary imports for testing and MongoDB operations.
1-347: Verify comprehensive security testing coverage.The test suite covers critical security scenarios for certificate revocation, but consider adding explicit test cases for:
- Malformed certificate data
- Edge cases in certificate validity periods
- Revocation list size limits
Let's verify the test coverage:
✅ Verification successful
Security test coverage is adequate
The existing test coverage is comprehensive and includes:
- Input validation for revocation list updates
- Duplicate certificate serial number handling
- UUID validation for issuer IDs
- Certificate signing request validation
- Proper error handling for invalid inputs
- Parallel execution safety checks
- Expiration time validation
The original review comment's suggestions for additional test cases are already covered in the implementation and tests:
- Malformed certificate data - covered by validation in
UpdateRevocationList- Edge cases in validity periods - covered in
TestUpdateRevocationList- Revocation list size handling - covered in parallel execution tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for additional security-related test cases in the codebase rg -l "TestRevocation.*Security|TestInvalid.*Certificate" --type goLength of output: 69
Script:
#!/bin/bash # Let's check for all test files related to certificates and revocation rg -l "Test.*Certificate|Test.*Revocation" --type go # Also check the actual implementation files for validation logic ast-grep --pattern 'func $_(ctx context.Context, $cert $_) error { $$$ }'Length of output: 2208
Script:
#!/bin/bash # Let's examine the test coverage in certificate-authority related files rg "func Test" -A 5 certificate-authority/service/http/revocationList_test.go rg "func Test" -A 5 certificate-authority/service/http/signCertificate_test.go # Check validation logic in the implementation rg "func.*UpdateRevocationList|func.*GetRevocationList" -A 5 certificate-authority/store/mongodb/revocationList.goLength of output: 2039
certificate-authority/store/mongodb/signingRecords_test.go (3)
101-101: LGTM! Good improvement to error handling.The early return after error check follows the fail-fast principle and improves code clarity.
44-45: Consider diversifying test data for new fields.The new
SerialandIssuerIdfields use the same values across all test cases. To improve test coverage, consider:
- Testing different serial number formats and lengths
- Testing invalid UUID formats for
IssuerId- Adding specific test cases for these new fields
Let's verify if there are any validation requirements for these fields:
Also applies to: 64-65, 83-84, 217-218, 283-284, 326-327, 341-342, 356-357, 371-372
302-304: Verify impact of handler interface change.The replacement of
Handle(ctx context.Context, iter store.SigningRecordIter)withprocess(sr *store.SigningRecord):
- Removes context propagation which might be needed for cancellation
- Changes the method name to be less descriptive
- Represents a breaking change that might affect other code
Let's check for other implementations of this interface:
certificate-authority/store/cqldb/signingRecords_test.go (3)
5-5: LGTM: Required import for certificate serial numbersThe addition of
math/bigis appropriate for handling certificate serial numbers which require arbitrary-precision arithmetic.
369-369: LGTM: Proper usage of LoadSigningRecordsThe test correctly uses nil query to load all records and properly implements the handler pattern.
Also applies to: 377-377
386-388: Verify all callers of the renamed handler functionThe handler function has been renamed from
Handletoprocess. While the new name is more descriptive, we should verify that all callers have been updated.certificate-authority/pb/doc.html (3)
349-349: LGTM: Method descriptions are clear and consistent.The updated descriptions for
GetSigningRecordsandDeleteSigningRecordsmethods accurately reflect their functionality. The changes align with the PR objective of implementing Certificate Revocation List.Also applies to: 356-356
494-511: LGTM: Well-documented certificate tracking enhancements.The addition of
serialandissuer_idfields toCredentialStatusimproves certificate tracking capabilities:
serialfield enables proper certificate revocation trackingissuer_idsupports multi-CA scenarios with its UUID-based identification- Both fields are well-documented with clear bson tags
551-551: LGTM: Description accurately reflects enhanced functionality.The updated description for
DeletedSigningRecordsclearly indicates its dual purpose of revoking or deleting certificates, which aligns with the Certificate Revocation List implementation objective.certificate-authority/store/mongodb/revocationList.go (4)
16-16: Previous feedback still applies here.
71-71: Previous feedback still applies here.
134-134: Previous feedback still applies here.
169-169: Previous feedback still applies here.
Danielius1922
force-pushed
the
adam/feature/crl-1
branch
from
0b23411
to
fb8c9e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (16)
certificate-authority/service/http/service.go (1)
27-27: Add documentation for the store parameter.Consider adding a doc comment explaining the role of the
store.Storeparameter and its relationship to CRL functionality.+// New creates a new HTTP service for the certificate authority. +// The store parameter is required for managing certificate revocation lists when CRLEnabled is true. func New(serviceName string, config Config, s store.Store, ca *grpcService.CertificateAuthorityServer, validator *validator.Validator, fileWatcher *fsnotify.Watcher, logger log.Logger, tracerProvider trace.TracerProvider) (*Service, error) {certificate-authority/service/grpc/deleteSigningRecords_test.go (1)
100-100: Consider making the URL scheme configurable.While using
https://is secure, consider making the URL scheme configurable for development/testing scenarios.-ca, err := grpc.NewCertificateAuthorityServer(ownerClaim, config.HubID(), "https://"+config.CERTIFICATE_AUTHORITY_HTTP_HOST, test.MakeConfig(t).Signer, store, fileWatcher, logger) +ca, err := grpc.NewCertificateAuthorityServer(ownerClaim, config.HubID(), config.CERTIFICATE_AUTHORITY_URL_SCHEME+"://"+config.CERTIFICATE_AUTHORITY_HTTP_HOST, test.MakeConfig(t).Signer, store, fileWatcher, logger)certificate-authority/service/grpc/getSigningRecords_test.go (1)
43-44: Consider adding test cases for edge cases.While the test data is correctly structured, consider adding test cases for:
- Invalid serial number formats
- Invalid UUID formats for IssuerId
- Empty/null values for both fields
This would help ensure robust validation in the implementation.
pkg/security/certificateSigner/certificateSigner.go (2)
19-22: LGTM! Consider documenting the field.The new
CRLDistributionPointsfield is appropriately added to theSignerConfigstruct. Consider adding a documentation comment to describe the expected format of the URLs and any validation requirements.type SignerConfig struct { ValidNotBefore time.Time ValidNotAfter time.Time + // CRLDistributionPoints contains a list of URLs where the Certificate Revocation List can be obtained. + // Each URL should be an HTTP/HTTPS endpoint serving the CRL in DER format. CRLDistributionPoints []string OverrideCertTemplate func(template *x509.Certificate) error }
Line range hint
67-83: Enhance error messages for better debugging.While the error handling is correct, consider making the error messages more specific to help with troubleshooting.
func parseCertificateRequest(csr []byte) (*x509.CertificateRequest, error) { csrBlock, _ := pem.Decode(csr) if csrBlock == nil { - return nil, errors.New("pem not found") + return nil, errors.New("failed to decode PEM block from CSR") }certificate-authority/service/service.go (1)
122-123: LGTM! Good architectural decision.The changes demonstrate good separation of concerns:
- CRL functionality is properly feature-flagged
- Direct database access for the HTTP service improves modularity by removing unnecessary coupling through the CA service
This design will make it easier to maintain and extend CRL functionality independently.
Consider documenting the rationale for this architectural decision in the service's design documentation to help future maintainers understand the separation of concerns between the HTTP service and CA service regarding CRL management.
certificate-authority/service/grpc/signer_internal_test.go (1)
68-69: Document the purpose of the validated field.The validated field has been added to multiple test cases, but its purpose and significance aren't documented. Consider adding a comment explaining when and why this field should be set to true.
Also applies to: 79-80, 90-90, 102-102, 114-114
certificate-authority/service/config_test.go (1)
150-150: Add documentation about security implications.Consider adding a comment block explaining the security implications of CRL distribution point validation, particularly why certain patterns are rejected.
Add this documentation:
+// TestHTTPConfigValidate verifies the validation logic for HTTP configuration, +// particularly focusing on CRL distribution point security: +// - Rejects relative paths to prevent path traversal +// - Ensures absolute URLs to prevent DNS rebinding +// - Validates HTTPS usage for secure CRL distribution func TestHTTPConfigValidate(t *testing.T) {certificate-authority/service/grpc/signer.go (1)
189-191: Consider providing more context in error messages.When signing fails, consider wrapping the error with additional context about the signing operation (identity vs. standard) to aid in debugging.
if err != nil { - return nil, nil, err + return nil, nil, fmt.Errorf("failed to sign %s certificate: %w", + map[bool]string{true: "identity", false: "standard"}[isIdentityCertificate], + err) }certificate-authority/service/grpc/signCertificate_test.go (2)
94-106: LGTM! Well-structured helper functions with good separation of concerns.The refactoring improves test flexibility by separating key generation from CSR creation. Consider adding a small enhancement for test reliability.
Consider adding key size validation in
createCSRWithKey:func createCSRWithKey(t *testing.T, commonName string, priv *ecdsa.PrivateKey) []byte { + require.Equal(t, priv.Curve, elliptic.P256(), "unexpected curve type") var cfg generateCertificate.Configuration cfg.Subject.CommonName = commonName csr, err := generateCertificate.GenerateCSR(cfg, priv) require.NoError(t, err) return csr }
153-197: LGTM! Critical security test case for device identity management.The test effectively verifies that:
- The same device ID + same key combination is allowed (legitimate reuse)
- The same device ID + different key combination is rejected (prevents spoofing)
Consider adding a doc comment to explain the security implications:
+// TestCertificateAuthorityServerSignCSRWithSameDevice verifies the security constraint +// that prevents device ID spoofing by ensuring that: +// 1. A device can reuse its existing key (legitimate case) +// 2. A device cannot register with a different key (potential spoofing attempt) func TestCertificateAuthorityServerSignCSRWithSameDevice(t *testing.T) {certificate-authority/store/mongodb/revocationList.go (1)
193-197: Consider adding indexes for performance optimizationThe query filters on
validUntilfield within the certificates array. Consider adding an index to optimize these queries, especially for large revocation lists.Add an index on the
certificates.validUntilfield:// Index definition to be added to your MongoDB initialization { "certificates.validUntil": 1 }certificate-authority/store/cqldb/signingRecords.go (1)
342-367: LGTM with a minor suggestion about error variable naming.The implementation is solid with proper error handling, resource cleanup, and deduplication. However, consider renaming
errClosetocloseErrto better align with Go naming conventions for error variables.-errClose := i.close() +closeErr := i.close() if err == nil { - return errClose + return closeErr }certificate-authority/store/mongodb/signingRecords_test.go (2)
44-45: Consider diversifying test data for new credential fields.The new
SerialandIssuerIdfields are consistently populated with the same hardcoded values across all test cases. To ensure robust testing, consider:
- Using different serial numbers to test various number ranges
- Testing different UUID formats for
IssuerId- Adding test cases for empty/invalid values
Example improvement:
func generateTestCredential(serial int64, issuerID string) *pb.CredentialStatus { return &pb.CredentialStatus{ CertificatePem: "certificate", Date: constDate().UnixNano(), ValidUntilDate: constDate().UnixNano(), Serial: big.NewInt(serial).String(), IssuerId: issuerID, } }Also applies to: 64-65, 83-84, 217-218, 283-284, 326-327, 341-342, 356-357, 371-372
302-304: Consider adding validation in the process method.While the simplified handler implementation is clean, consider adding validation to ensure robust error handling:
func (h *testSigningRecordHandler) process(sr *store.SigningRecord) (err error) { + if sr == nil { + return fmt.Errorf("signing record cannot be nil") + } h.lcs = append(h.lcs, sr) return nil }certificate-authority/service/grpc/config.go (1)
Line range hint
43-71: Potential data race due to unprotected 'validated' fieldThe
validatedboolean field inSignerConfigis not protected against concurrent access. IfValidate()is called concurrently by multiple goroutines, this could lead to a data race condition.Consider using
sync.Onceto ensure thread-safe validation. Here's an example of how you could modify the code:+import "sync" type SignerConfig struct { CAPool interface{} `yaml:"caPool" json:"caPool" description:"file path to the root certificates in PEM format"` KeyFile urischeme.URIScheme `yaml:"keyFile" json:"keyFile" description:"file name of CA private key in PEM format"` CertFile urischeme.URIScheme `yaml:"certFile" json:"certFile" description:"file name of CA certificate in PEM format"` ValidFrom string `yaml:"validFrom" json:"validFrom" description:"format https://github.com/karrick/tparse"` ExpiresIn time.Duration `yaml:"expiresIn" json:"expiresIn"` CRL CRLConfig `yaml:"crl" json:"crl"` - validated bool `yaml:"-" json:"-"` + validateOnce sync.Once `yaml:"-" json:"-"` + validateErr error `yaml:"-" json:"-"` } func (c *SignerConfig) Validate() error { - if c.validated { - return nil - } + c.validateOnce.Do(func() { caPoolArray, ok := strings.ToStringArray(c.CAPool) if !ok { - return fmt.Errorf("caPool('%v')", c.CAPool) + c.validateErr = fmt.Errorf("caPool('%v')", c.CAPool) + return } c.caPoolArray = urischeme.ToURISchemeArray(caPoolArray) if c.CertFile == "" { - return fmt.Errorf("certFile('%v')", c.CertFile) + c.validateErr = fmt.Errorf("certFile('%v')", c.CertFile) + return } if c.KeyFile == "" { - return fmt.Errorf("keyFile('%v')", c.KeyFile) + c.validateErr = fmt.Errorf("keyFile('%v')", c.KeyFile) + return } if c.ExpiresIn <= 0 { - return fmt.Errorf("expiresIn('%v')", c.ExpiresIn) + c.validateErr = fmt.Errorf("expiresIn('%v')", c.ExpiresIn) + return } _, err := tparse.ParseNow(time.RFC3339, c.ValidFrom) if err != nil { - return fmt.Errorf("validFrom('%v').%w", c.ValidFrom, err) + c.validateErr = fmt.Errorf("validFrom('%v').%w", c.ValidFrom, err) + return } if err := c.CRL.Validate(); err != nil { - return fmt.Errorf("crl.%w", err) + c.validateErr = fmt.Errorf("crl.%w", err) + return } + }) - c.validated = true - return nil + return c.validateErr }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (10)
certificate-authority/config.yamlis excluded by!**/*.yamlcertificate-authority/pb/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocertificate-authority/pb/service.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gocertificate-authority/pb/service.swagger.jsonis excluded by!**/*.jsoncertificate-authority/pb/service_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocertificate-authority/pb/signingRecords.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocharts/plgd-hub/templates/certificate-authority/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/values.yamlis excluded by!**/*.yamldevice-provisioning-service/pb/service.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.godevice-provisioning-service/pb/service.swagger.jsonis excluded by!**/*.json
📒 Files selected for processing (56)
- bundle/nginx/nginx.conf.template (1 hunks)
- certificate-authority/pb/README.md (3 hunks)
- certificate-authority/pb/doc.html (7 hunks)
- certificate-authority/pb/service.proto (2 hunks)
- certificate-authority/pb/signingRecords.go (3 hunks)
- certificate-authority/pb/signingRecords.proto (2 hunks)
- certificate-authority/pb/signingRecords_test.go (1 hunks)
- certificate-authority/service/cleanDatabase_test.go (5 hunks)
- certificate-authority/service/config.go (3 hunks)
- certificate-authority/service/config_test.go (1 hunks)
- certificate-authority/service/grpc/config.go (2 hunks)
- certificate-authority/service/grpc/config_test.go (1 hunks)
- certificate-authority/service/grpc/deleteSigningRecords.go (1 hunks)
- certificate-authority/service/grpc/deleteSigningRecords_test.go (4 hunks)
- certificate-authority/service/grpc/getSigningRecords.go (1 hunks)
- certificate-authority/service/grpc/getSigningRecords_test.go (5 hunks)
- certificate-authority/service/grpc/server.go (2 hunks)
- certificate-authority/service/grpc/server_test.go (1 hunks)
- certificate-authority/service/grpc/signCertificate.go (3 hunks)
- certificate-authority/service/grpc/signCertificate_test.go (4 hunks)
- certificate-authority/service/grpc/signIdentityCertificate_test.go (3 hunks)
- certificate-authority/service/grpc/signer.go (6 hunks)
- certificate-authority/service/grpc/signer_internal_test.go (7 hunks)
- certificate-authority/service/http/config.go (1 hunks)
- certificate-authority/service/http/requestHandler.go (1 hunks)
- certificate-authority/service/http/revocationList.go (1 hunks)
- certificate-authority/service/http/revocationList_test.go (1 hunks)
- certificate-authority/service/http/service.go (2 hunks)
- certificate-authority/service/http/signCertificate_test.go (6 hunks)
- certificate-authority/service/http/signIdentityCertificate_test.go (3 hunks)
- certificate-authority/service/service.go (2 hunks)
- certificate-authority/service/uri/uri.go (1 hunks)
- certificate-authority/store/cqldb/revocationList.go (1 hunks)
- certificate-authority/store/cqldb/signingRecords.go (1 hunks)
- certificate-authority/store/cqldb/signingRecords_test.go (18 hunks)
- certificate-authority/store/mongodb/bulkWriter.go (0 hunks)
- certificate-authority/store/mongodb/config.go (1 hunks)
- certificate-authority/store/mongodb/revocationList.go (1 hunks)
- certificate-authority/store/mongodb/revocationList_test.go (1 hunks)
- certificate-authority/store/mongodb/signingRecords.go (3 hunks)
- certificate-authority/store/mongodb/signingRecords_test.go (13 hunks)
- certificate-authority/store/mongodb/store.go (3 hunks)
- certificate-authority/store/revocationList.go (1 hunks)
- certificate-authority/store/revocationList_test.go (1 hunks)
- certificate-authority/store/store.go (2 hunks)
- certificate-authority/test/revocationList.go (1 hunks)
- certificate-authority/test/service.go (2 hunks)
- charts/plgd-hub/README.md (0 hunks)
- device-provisioning-service/Makefile (1 hunks)
- device-provisioning-service/pb/service.proto (4 hunks)
- http-gateway/test/http.go (1 hunks)
- http-gateway/uri/uri.go (1 hunks)
- pkg/security/certificateSigner/certificateSigner.go (5 hunks)
- pkg/security/certificateSigner/certificateSigner_test.go (0 hunks)
- pkg/security/certificateSigner/identityCertificateSigner_test.go (0 hunks)
- pkg/security/x509/crl.go (1 hunks)
💤 Files with no reviewable changes (4)
- certificate-authority/store/mongodb/bulkWriter.go
- charts/plgd-hub/README.md
- pkg/security/certificateSigner/certificateSigner_test.go
- pkg/security/certificateSigner/identityCertificateSigner_test.go
🚧 Files skipped from review as they are similar to previous changes (33)
- bundle/nginx/nginx.conf.template
- certificate-authority/pb/README.md
- certificate-authority/pb/doc.html
- certificate-authority/pb/service.proto
- certificate-authority/pb/signingRecords.go
- certificate-authority/pb/signingRecords.proto
- certificate-authority/pb/signingRecords_test.go
- certificate-authority/service/cleanDatabase_test.go
- certificate-authority/service/grpc/config_test.go
- certificate-authority/service/grpc/deleteSigningRecords.go
- certificate-authority/service/grpc/getSigningRecords.go
- certificate-authority/service/grpc/server.go
- certificate-authority/service/grpc/server_test.go
- certificate-authority/service/grpc/signIdentityCertificate_test.go
- certificate-authority/service/http/config.go
- certificate-authority/service/http/requestHandler.go
- certificate-authority/service/http/revocationList_test.go
- certificate-authority/service/http/signCertificate_test.go
- certificate-authority/service/http/signIdentityCertificate_test.go
- certificate-authority/service/uri/uri.go
- certificate-authority/store/cqldb/revocationList.go
- certificate-authority/store/mongodb/config.go
- certificate-authority/store/mongodb/revocationList_test.go
- certificate-authority/store/revocationList.go
- certificate-authority/store/revocationList_test.go
- certificate-authority/store/store.go
- certificate-authority/test/revocationList.go
- certificate-authority/test/service.go
- device-provisioning-service/Makefile
- device-provisioning-service/pb/service.proto
- http-gateway/test/http.go
- http-gateway/uri/uri.go
- pkg/security/x509/crl.go
🧰 Additional context used
📓 Learnings (2)
certificate-authority/store/mongodb/revocationList.go (1)
Learnt from: Danielius1922 PR: plgd-dev/hub#1383 File: certificate-authority/store/mongodb/signingRecords.go:139-141 Timestamp: 2024-10-28T17:11:50.454Z Learning: In `certificate-authority/store/mongodb/signingRecords.go`, within the `RevokeSigningRecords` method, expired signing records are deleted but not added to the revocation list.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-10-28T17:11:50.454Z Learning: In `certificate-authority/store/mongodb/signingRecords.go`, within the `RevokeSigningRecords` method, expired signing records are deleted but not added to the revocation list.
🪛 GitHub Check: CodeQL
certificate-authority/store/mongodb/revocationList.go
[failure] 217-217: Database query built from user-controlled sources
This query depends on a user-provided value.
🔇 Additional comments (60)
certificate-authority/service/http/service.go (4)
5-14: LGTM: Import changes are well-organized and necessary.The new imports support the CRL functionality and follow Go's import organization best practices.
23-23: LGTM: Appropriate encapsulation of requestHandler.The change from exported to unexported type improves encapsulation by making the request handler an internal implementation detail.
Line range hint
52-56: LGTM: Proper error handling in requestHandler initialization.The code properly handles initialization errors by closing the service before returning the error, preventing resource leaks.
27-34: Verify the security implications of the CRL endpoint pattern.While the use of
regexp.QuoteMetais good for security, we should verify that the patternSigningRevocationListBase + '/.*'doesn't allow access to unintended paths.✅ Verification successful
Let me analyze the results and generate one more verification script to ensure we have all the necessary information.
The additional check will help us understand the API path structure and verify if there are any other handlers that might affect the security of the CRL endpoint pattern.
CRL endpoint pattern is secure and well-structured
The pattern is secure because:
- The base path
/api/v1/signing/crlis properly structured and namespacedregexp.QuoteMetaescapes all regex metacharacters in the base path- The
\/.*suffix pattern is consistent with other authenticated endpoints in the codebase- The CRL endpoint is only accessible via GET method
- Similar pattern usage across the codebase shows this is a standard approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the SigningRevocationListBase constant and its usage rg -A 2 "SigningRevocationListBase.*=.*" # Check for any other similar endpoint patterns rg -A 2 "regexp\.QuoteMeta.*\+ `/.*`"Length of output: 1494
Script:
#!/bin/bash # Check the API constant value and structure rg "API.*=.*" certificate-authority/service/uri/uri.go # Check if there are any other routes or handlers using SigningRevocationListBase ast-grep --pattern 'func $FUNC($$$) { $$$ SigningRevocationListBase $$$ }'Length of output: 171
certificate-authority/store/mongodb/store.go (4)
37-51: Well-designed iterator implementation!The generic
MongoIterator[T]is a clean abstraction over mongo.Cursor that:
- Provides type safety through generics
- Follows the iterator pattern
- Handles errors appropriately
53-75: Previous review comment about error handling is still applicable.The current implementation stops processing on the first error, which limits the function's utility in handling partial failures.
99-104: Clean implementation of database cleanup!The error handling using multierror is appropriate, and the implementation properly cleans up both collections.
19-21: Verify the performance impact of removing bulk operations.The replacement of
bulkWriterwithloggersuggests a shift from batch operations to individual operations. While this simplifies the code, it might impact performance for high-throughput scenarios.certificate-authority/service/grpc/deleteSigningRecords_test.go (6)
5-5: LGTM: Import additions are appropriate.The new imports support the serial number handling (
math/big) and token context management (pkgGrpc).Also applies to: 17-17
25-28: LGTM: Well-structured test setup.Good practice to isolate token creation and context setup. This allows for better testing of authentication scenarios.
53-62: LGTM: Good security validation test case.The new test case properly validates the authentication requirement by ensuring operations fail when the context lacks a valid token.
69-69: LGTM: Improved test case clarity with explicit contexts.The addition of explicit context parameters makes the authentication requirements clearer in each test case.
Also applies to: 78-78
109-113: LGTM: Clean error handling pattern.The error handling follows good practices with clear separation of error and success cases.
39-40: LGTM: Essential fields added for certificate tracking.The addition of
SerialandIssuerIdfields enhances the credential tracking capabilities needed for the Certificate Revocation List implementation.Let's verify the consistency of these new fields across the codebase:
✅ Verification successful
LGTM: Fields are consistently implemented across the codebase
The verification confirms that
SerialandIssuerIdfields are properly implemented in the Certificate Authority's data structures:
- Found in
certificate-authority/pb/signingRecords.pb.goas part of theCredentialStatusstruct with correct types and BSON tags- Used consistently in test files across the certificate authority service
- Properly integrated with the revocation list functionality in
certificate-authority/store/revocationList.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the Serial and IssuerId fields are consistently implemented # Look for the credential structure definition and usage ast-grep --pattern 'type Credential struct { $$$ Serial $$$ IssuerId $$$ $$$ }'Length of output: 89
Script:
#!/bin/bash # Let's try a broader search for credential structures and their fields rg -i "credential.*struct" -A 10 # Also search for specific field usage patterns rg "Serial.*string" -A 2 rg "IssuerId.*string" -A 2 # Search for test files that might use these fields fd ".*test.go$" --exec rg -l "Serial|IssuerId"Length of output: 17158
certificate-authority/service/grpc/getSigningRecords_test.go (3)
7-7: LGTM: Import changes are appropriate.The addition of
math/bigis necessary for handling certificate serial numbers, and the package alias update maintains consistency with the new package structure.Also applies to: 20-20
101-101: LGTM: Package update is consistent.The context creation update correctly uses the new package alias while maintaining the same functionality.
92-92: Verify HTTP host configuration consistency.The server initialization now includes the HTTP host parameter. Let's ensure this configuration is consistent across the codebase.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the HTTP host configuration usage across the codebase.
HTTP host configuration is consistently used across the codebase
The verification shows that
CERTIFICATE_AUTHORITY_HTTP_HOSTis consistently used:
- All server initializations properly prefix the host with "https://"
- The configuration is defined in
test/config/config.goas "localhost:20012"- The host is consistently used in both HTTP and gRPC test setups
- External address configuration in service setup also follows the same pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent HTTP host configuration across tests and implementation echo "Checking HTTP host configuration in test files..." rg "CERTIFICATE_AUTHORITY_HTTP_HOST" -A 2 echo "Checking NewCertificateAuthorityServer usage..." ast-grep --pattern 'NewCertificateAuthorityServer($$$, "https://" + $_, $$$)'Length of output: 3650
pkg/security/certificateSigner/certificateSigner.go (4)
39-43: LGTM! Secure implementation with defensive copying.The function correctly implements defensive copying using
slices.Clone, preventing external modifications from affecting internal state.
85-92: LGTM! Good validation and error handling.The changes properly validate CA certificate presence and handle CSR parsing errors.
Line range hint
129-134: LGTM! Secure certificate creation and chain building.The implementation correctly uses a secure random number generator and properly builds the certificate chain.
110-122:⚠️ Potential issueAdd security constraints to the certificate template.
While the CRL distribution points are correctly added, the certificate template should include key usage constraints as suggested in the previous review. This is important for security best practices.
Additionally, consider adding these security-related fields:
template := x509.Certificate{ // ... existing fields ... + BasicConstraints: x509.BasicConstraints{ + IsCA: false, + MaxPathLen: -1, + }, CRLDistributionPoints: s.cfg.CRLDistributionPoints, }certificate-authority/service/config.go (3)
16-16: LGTM!The import is properly aliased and necessary for the CRL validation functionality.
69-71: LGTM!The HTTPConfig struct changes are well-organized:
- ExternalAddress field is properly tagged
- Field ordering follows Go conventions
75-78: Verify CRL distribution point address validation implementation.The validation is delegated to ValidateCRLDistributionPointAddress, but we should verify if it implements all necessary security checks as suggested in previous reviews.
#!/bin/bash # Description: Verify the implementation of ValidateCRLDistributionPointAddress # Test: Search for the validation function implementation ast-grep --pattern 'func ValidateCRLDistributionPointAddress(address string) error { $$$ }' # Test: Check for specific validation checks rg -A 10 'func ValidateCRLDistributionPointAddress' # Test: Look for test cases to understand validation coverage rg -A 10 'TestValidateCRLDistributionPointAddress'certificate-authority/service/service.go (1)
103-103: Verify external address validation.The external address is now passed to the gRPC service. Ensure that this address is properly validated in the configuration loading to prevent invalid URLs in CRL distribution points.
✅ Verification successful
External address validation is properly implemented
The external address is validated through
ValidateCRLDistributionPointAddresswhich ensures:
- The address is a valid URL (via
url.ParseRequestURI)- The URL is absolute (via
IsAbs()check)- The validation is enforced in both config loading and gRPC service initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for validation of ExternalAddress in config files rg -A 5 "ExternalAddress.*validate|validate.*ExternalAddress"Length of output: 63
Script:
#!/bin/bash # Let's search for the configuration struct and validation logic rg -A 10 "type.*Config.*struct" certificate-authority/ # Also search for URL validation patterns rg -A 5 "url\.|ValidateURL|ParseURL|IsValidURL" certificate-authority/Length of output: 8375
Script:
#!/bin/bash # Let's check the validation function for CRL distribution point address rg -A 5 "ValidateCRLDistributionPointAddress" # Also check the implementation in the x509 package fd -e go . | xargs rg -l "func.*ValidateCRLDistributionPointAddress"Length of output: 1475
Script:
#!/bin/bash # Let's check the complete validation in the x509 package rg -A 10 "func ValidateCRLDistributionPointAddress" pkg/security/x509/crl.goLength of output: 330
certificate-authority/service/grpc/signer_internal_test.go (4)
7-7: LGTM: Time import is required for CRL configuration.The addition of the time import is appropriate for handling the new CRL expiration configurations in the test cases.
52-52: LGTM: CRL server parameter addition.The crlServer field is a good addition to the args struct, enabling proper testing of CRL server configurations.
122-155: LGTM: Comprehensive CRL server test cases added.The new test cases effectively cover:
- Valid CRL server configuration with proper URL
- Invalid CRL server with malformed URL
- CRL-specific fields like Enabled and ExpiresIn
This addresses the previous review comment requesting CRL server validation tests.
160-160: 🛠️ Refactor suggestionConsider parameterizing ownerClaim and hubID.
As suggested in the previous review, consider making ownerClaim and hubID configurable through test parameters for better test flexibility.
Apply this change:
-got, err := NewSigner("ownerClaim", "hubID", tt.args.crlServer, tt.args.signerConfig) +got, err := NewSigner(tt.args.ownerClaim, tt.args.hubID, tt.args.crlServer, tt.args.signerConfig)And update the args struct:
type args struct { + ownerClaim string + hubID string signerConfig SignerConfig crlServer string }certificate-authority/service/config_test.go (1)
162-163: Verify test helper implementation.The test relies on
test.MakeHTTPConfig()for the valid case. Let's verify its implementation to ensure it creates a secure configuration.✅ Verification successful
Let me gather more information about the HTTP server configuration since the initial results show that
MakeHTTPConfigusesconfig.MakeHttpServerConfig().
Let me try searching with a different path pattern since the previous search didn't find the config package.
From the search results, I found the implementation of
MakeHTTPConfig()incertificate-authority/test/service.go. Let me verify its specific implementation.
Let me check the implementation of
MakeHttpServerConfig()since it's used in theMakeHTTPConfig()implementation.
Test helper implementation verified and secure.
The
test.MakeHTTPConfig()creates a secure configuration with appropriate timeouts:
- Uses
MakeHttpServerConfig()which sets reasonable timeout values:
- ReadTimeout: 8 seconds
- ReadHeaderTimeout: 4 seconds
- WriteTimeout: 16 seconds
- IdleTimeout: 30 seconds
- Sets proper external HTTPS address
- Uses server configuration from the standard HTTP server package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of test.MakeHTTPConfig() # to ensure it creates a secure configuration with proper CRL settings # Look for the test helper implementation rg -A 10 "func MakeHTTPConfig" "certificate-authority/test" # Check for any CRL-related constants or configurations rg "CRL" "certificate-authority/test"Length of output: 1158
Script:
#!/bin/bash # Check the implementation of MakeHttpServerConfig and related security settings rg -A 10 "func MakeHttpServerConfig" "certificate-authority/config" # Look for any TLS or security-related configurations rg "TLS|ReadTimeout|WriteTimeout|IdleTimeout" "certificate-authority/config"Length of output: 282
Script:
#!/bin/bash # Find config-related files first fd config # Then check the implementation of HTTP server config and security settings rg -A 10 "func MakeHttpServerConfig" # Look for any TLS or security-related configurations rg "TLS|ReadTimeout|WriteTimeout|IdleTimeout"Length of output: 83371
Script:
#!/bin/bash # Get the full implementation of MakeHTTPConfig and related configurations ast-grep --pattern 'func MakeHTTPConfig() service.HTTPConfig { $$$ }' # Also check for any HTTP server config defaults rg -A 5 "type HTTPConfig struct" "certificate-authority/service"Length of output: 2460
Script:
#!/bin/bash # Look for MakeHttpServerConfig implementation in test/config directory rg -A 10 "func MakeHttpServerConfig" "test/config" # Also check the httpServer.Config structure rg -A 5 "type Config struct" "pkg/net/http/server"Length of output: 1094
certificate-authority/store/mongodb/signingRecords.go (5)
9-14: LGTM: Import additions and error handling simplificationThe new imports support the added functionality, and the simplified error handling in
CreateSigningRecordis appropriate since MongoDB's error messages are sufficiently descriptive.Also applies to: 26-26
29-39: Security: Include owner in update filterThe previous security concern remains valid. The update filter should include the owner field to prevent unauthorized updates of records owned by other users.
121-151: LGTM: Well-structured revocation logicThe implementation correctly:
- Separates the handling of expired and valid records
- Uses efficient data structures for tracking records by issuer
- Properly collects necessary certificate information for revocation
156-165: Consider aggregating errors during revocation list updatesThe current implementation stops on the first error, potentially leaving some revocation lists unprocessed. Consider aggregating errors to ensure all lists are processed.
178-188: LGTM: Improved type safety and error handlingThe changes improve the function by:
- Using a type-safe Process interface
- Adding proper handling for nil documents
- Maintaining clean error propagation
certificate-authority/service/grpc/signCertificate.go (3)
Line range hint
29-60: LGTM: Clean implementation of issuerID addition.The function correctly integrates the new issuerID parameter and follows good practices for UUID generation and timestamp handling.
136-149: LGTM: Clean implementation with proper error handling.The function effectively coordinates the signing record update process with revocation list management.
166-169: LGTM: Improved robustness with credential validation.The added null check for credential prevents potential issues and provides clear error messages.
certificate-authority/service/grpc/signer.go (6)
27-30: LGTM: Well-structured CRL configuration.The CRL configuration is properly encapsulated in a nested struct with clear fields for server address and validity duration.
49-55: LGTM: Robust issuer ID generation.The implementation correctly generates a deterministic UUID based on the public key, with proper error handling and appropriate namespace selection.
108-108: LGTM: Clean refactoring of signer initialization.The changes properly integrate the new CRL configuration while maintaining existing certificate chain validation logic.
Also applies to: 130-130
146-147: LGTM: Well-designed getter methods.The getters for certificate and CRL configuration provide clean access to necessary information while maintaining encapsulation.
Also applies to: 154-160
162-167: LGTM: Clean factory method implementation.The function provides a clean interface for creating appropriate certificate signers while promoting code reuse.
195-200: LGTM: Clean public API for signing operations.The methods provide a clear interface for different types of signing operations while sharing common implementation details.
certificate-authority/service/grpc/signCertificate_test.go (2)
13-19: LGTM! Import changes maintain package consistency.The update from
kitNetGrpctopkgGrpcaligns with the codebase's package standardization.
73-73: LGTM! Context setup properly handles authorization.The update to use
pkgGrpc.CtxWithTokenmaintains consistent authorization handling across the codebase.Also applies to: 138-138
certificate-authority/store/mongodb/revocationList.go (3)
25-27: LGTM! Well-structured error handlingThe error handling is comprehensive with proper wrapping of MongoDB-specific errors and clear error messages.
Also applies to: 112-114
128-143: Consider handling expired signing recordsBased on the learnings from
signingRecords.go, expired signing records are deleted but not added to the revocation list. Consider whether these expired records should be included in the revocation list for audit purposes.Let's verify the current handling of expired records:
#!/bin/bash # Search for expired record handling in the codebase rg -A 10 'expired.*signing.*record'
71-71: Ensure proper validation of map keys before using in MongoDB queryWhile the code validates individual certificates, it's important to ensure that the serial numbers used as map keys are properly sanitized before being used in MongoDB queries to prevent potential injection attacks.
Let's verify the format of serial numbers in the codebase:
certificate-authority/store/cqldb/signingRecords.go (1)
368-374:⚠️ Potential issueCritical Security Issue: Certificate deletion instead of revocation.
I strongly agree with the previous review comment about implementing proper certificate revocation. Deleting certificates instead of marking them as revoked is a significant security risk and compliance issue:
- Violates RFC 5280 requirements for certificate revocation
- Prevents proper certificate status checking
- Eliminates audit trail for revoked certificates
- Could allow reuse of revoked certificate serial numbers
This should be prioritized as a critical security enhancement. The previous review provided a detailed implementation guide that aligns with industry standards.
Let's verify if any other components rely on certificate status checks:
#!/bin/bash # Search for certificate status validation patterns rg -g '!{*.sum,*.mod}' -i '(check|validate|verify).*certificate.*(status|revocation|revoked)'certificate-authority/store/mongodb/signingRecords_test.go (2)
101-103: LGTM! Improved error handling.The simplified error handling with early return improves code clarity and follows good practices.
Line range hint
477-485: LGTM! Comprehensive test coverage.The test implementation properly validates the LoadSigningRecords functionality across various scenarios with appropriate error handling and assertions.
certificate-authority/store/cqldb/signingRecords_test.go (4)
386-388: LGTM: Handler function rename improves clarityThe rename from
Handletoprocessbetter describes the function's purpose while maintaining the same implementation.
Line range hint
408-439: LGTM: Comprehensive test coverage for credential fieldsThe test cases thoroughly validate the new Serial and IssuerId fields across different scenarios, including:
- Different Serial values (42, 43, 44)
- Consistent IssuerId validation
- Various filtering conditions
35-36: 🛠️ Refactor suggestionImprove test data organization for new credential fields
The Serial and IssuerId fields are added across all test cases, but the implementation could be improved:
- The Serial values (42, 43, 44) follow a pattern but are scattered across the test file
- The IssuerId is duplicated across all test cases
Previous review comment about creating test helpers for Serial and IssuerId generation is still valid. Additionally, consider:
const ( testIssuerID = "42424242-4242-4242-4242-424242424242" ) func generateTestSerial(idx int) string { return big.NewInt(int64(42 + idx)).String() }Also applies to: 55-56, 110-111, 130-131, 150-151, 169-170, 294-295, 310-311, 326-327, 363-364, 408-409, 423-424, 438-439, 574-575
542-542:⚠️ Potential issueFix hardcoded owner in LoadSigningRecords test
The hardcoded "owner" value prevents proper testing of owner-based filtering.
Apply the fix as suggested in the previous review:
-err := s.LoadSigningRecords(ctx, "owner", tt.args.query, h.process) +err := s.LoadSigningRecords(ctx, tt.args.owner, tt.args.query, h.process)certificate-authority/service/grpc/config.go (1)
Line range hint
47-71:certificate-authority/service/http/revocationList.go (3)
23-25: 🛠️ Refactor suggestionConsider using more specific error codes for better error handling
Currently, the
errCannotGetRevocationListfunction maps all errors tocodes.Internal. It's advisable to return more specific gRPC error codes based on the underlying error to provide clearer feedback to clients. For example:
store.ErrNotFound→codes.NotFound- Validation errors (e.g., invalid issuer ID) →
codes.InvalidArgument- Parsing errors →
codes.InvalidArgument- Unexpected internal errors →
codes.Internal
66-87: 🛠️ Refactor suggestionEnhance HTTP response with security headers and caching directives
To improve security and performance, consider adding the following HTTP headers:
Security Headers:
X-Content-Type-Options: nosniffX-Frame-Options: DENYCaching Headers:
Cache-Control: public, max-age=3600ETagheader based on the CRL content for cache validationImplementing these headers ensures better protection against certain types of attacks and optimizes client-side caching of the CRL.
52-64: 🛠️ Refactor suggestionImplement exponential backoff and context checks in retry mechanism
The
tryGetRevocationListmethod currently retries immediately without delay and does not check for context cancellation. Improving the retry logic with exponential backoff and context awareness enhances reliability and responsiveness.
Danielius1922
force-pushed
the
adam/feature/crl-1
branch
from
fb8c9e6
to
e573b2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (10)
certificate-authority/service/http/service.go (1)
29-34: Consider CRL configuration flexibility.The current implementation enables/disables CRL functionality through a boolean flag. Consider if more granular configuration might be needed in the future, such as:
- CRL update intervals
- Different CRL endpoints for different certificate types
- CRL caching settings
It might be worth planning for these potential requirements now to avoid future breaking changes.
certificate-authority/store/mongodb/store.go (1)
53-75: Add documentation for processCursor function.Please add documentation explaining:
- The purpose of the function
- The generic type parameter T
- The Process callback parameter
- The return values (count and error)
Apply this diff:
+// processCursor iterates over a MongoDB cursor and processes each document using the provided callback. +// It returns the count of successfully processed documents and any errors encountered during iteration. +// Type parameter T represents the document type to be processed. func processCursor[T any](ctx context.Context, cr *mongo.Cursor, p store.Process[T]) (int, error) {certificate-authority/service/grpc/deleteSigningRecords_test.go (1)
25-28: Consider using test helpers for JWT token creationWhile the token creation is functional, consider extracting this common test setup into a dedicated test helper function, as it's likely to be reused across other test files.
+func createTestToken(t *testing.T, owner string) string { + return config.CreateJwtToken(t, jwt.MapClaims{ + ownerClaim: owner, + }) +}pkg/security/certificateSigner/certificateSigner.go (1)
Line range hint
67-83: Consider enhancing error messages for better debugging.While the error handling is comprehensive, consider making the error messages more specific to help with troubleshooting.
- return nil, errors.New("pem not found") + return nil, errors.New("failed to decode PEM block from CSR")certificate-authority/test/service.go (2)
33-43: Consider making CRL configuration more flexible.A few concerns about the current implementation:
- The CRL expiration time is hard-coded to 1 hour
- CRL is only enabled for MongoDB without clear justification
Consider:
- Making the expiration time configurable through environment variables or configuration files
- Document why CRL is only supported with MongoDB
- Add validation for the expiration time to ensure it's reasonable (e.g., not too short or too long)
Example configuration structure:
type CRLConfig struct { Enabled bool ExpiresIn time.Duration `env:"CRL_EXPIRES_IN" default:"1h" validate:"required,min=15m,max=24h"` }
25-43: Add tests for new configuration functions.The new
MakeHTTPConfigandMakeCRLConfigfunctions should have corresponding test cases to verify:
- Correct configuration generation
- Proper handling of different database types for CRL
- Validation of configuration values
Would you like me to help generate test cases for these new functions?
certificate-authority/service/grpc/signer_internal_test.go (1)
117-150: LGTM: Good test coverage for CRL server functionality.The test cases effectively cover both success and failure scenarios for CRL server configuration. Consider adding a few more edge cases:
- Empty CRL server URL with CRL enabled
- Valid URL but with CRL disabled
- HTTPS URL with invalid certificate
certificate-authority/service/service.go (1)
Line range hint
103-123: Consider implementing periodic CRL generation.While the code sets up the infrastructure for CRL support, consider adding a periodic task (similar to the existing cleanup job) that:
- Generates and updates the CRL periodically
- Handles CRL caching to prevent database load
- Monitors CRL size and implements pagination if needed
You could utilize the existing cron scheduler infrastructure:
// Example structure for periodic CRL generation _, err = s.NewJob( gocron.CronJob(config.Signer.CRL.UpdateInterval, true), gocron.NewTask(func() { if err := generateAndStoreCRL(ctx, dbStorage); err != nil { logger.Error("Failed to generate CRL", err) } }), )certificate-authority/store/mongodb/signingRecords_test.go (1)
302-304: Consider improving the handler implementation.The current implementation has a few potential improvements:
- The method name 'process' could be more descriptive, e.g., 'appendSigningRecord'
- The error return is unused as the method always returns nil
Consider this refactor:
-func (h *testSigningRecordHandler) process(sr *store.SigningRecord) (err error) { +func (h *testSigningRecordHandler) appendSigningRecord(sr *store.SigningRecord) { h.lcs = append(h.lcs, sr) - return nil }certificate-authority/store/mongodb/revocationList.go (1)
191-214: Consider adding an index onCertificates.ValidUntilfor performance optimizationTo enhance query performance when filtering non-expired certificates, consider adding an index on the
Certificates.ValidUntilfield, especially as the number of certificates grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (10)
certificate-authority/config.yamlis excluded by!**/*.yamlcertificate-authority/pb/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocertificate-authority/pb/service.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gocertificate-authority/pb/service.swagger.jsonis excluded by!**/*.jsoncertificate-authority/pb/service_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocertificate-authority/pb/signingRecords.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocharts/plgd-hub/templates/certificate-authority/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/values.yamlis excluded by!**/*.yamldevice-provisioning-service/pb/service.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.godevice-provisioning-service/pb/service.swagger.jsonis excluded by!**/*.json
📒 Files selected for processing (56)
- bundle/nginx/nginx.conf.template (1 hunks)
- certificate-authority/pb/README.md (3 hunks)
- certificate-authority/pb/doc.html (7 hunks)
- certificate-authority/pb/service.proto (2 hunks)
- certificate-authority/pb/signingRecords.go (3 hunks)
- certificate-authority/pb/signingRecords.proto (2 hunks)
- certificate-authority/pb/signingRecords_test.go (1 hunks)
- certificate-authority/service/cleanDatabase_test.go (5 hunks)
- certificate-authority/service/config.go (3 hunks)
- certificate-authority/service/config_test.go (1 hunks)
- certificate-authority/service/grpc/config.go (2 hunks)
- certificate-authority/service/grpc/config_test.go (1 hunks)
- certificate-authority/service/grpc/deleteSigningRecords.go (1 hunks)
- certificate-authority/service/grpc/deleteSigningRecords_test.go (4 hunks)
- certificate-authority/service/grpc/getSigningRecords.go (1 hunks)
- certificate-authority/service/grpc/getSigningRecords_test.go (5 hunks)
- certificate-authority/service/grpc/server.go (2 hunks)
- certificate-authority/service/grpc/server_test.go (1 hunks)
- certificate-authority/service/grpc/signCertificate.go (3 hunks)
- certificate-authority/service/grpc/signCertificate_test.go (4 hunks)
- certificate-authority/service/grpc/signIdentityCertificate_test.go (3 hunks)
- certificate-authority/service/grpc/signer.go (6 hunks)
- certificate-authority/service/grpc/signer_internal_test.go (3 hunks)
- certificate-authority/service/http/config.go (1 hunks)
- certificate-authority/service/http/requestHandler.go (1 hunks)
- certificate-authority/service/http/revocationList.go (1 hunks)
- certificate-authority/service/http/revocationList_test.go (1 hunks)
- certificate-authority/service/http/service.go (2 hunks)
- certificate-authority/service/http/signCertificate_test.go (6 hunks)
- certificate-authority/service/http/signIdentityCertificate_test.go (3 hunks)
- certificate-authority/service/service.go (2 hunks)
- certificate-authority/service/uri/uri.go (1 hunks)
- certificate-authority/store/cqldb/revocationList.go (1 hunks)
- certificate-authority/store/cqldb/signingRecords.go (1 hunks)
- certificate-authority/store/cqldb/signingRecords_test.go (18 hunks)
- certificate-authority/store/mongodb/bulkWriter.go (0 hunks)
- certificate-authority/store/mongodb/config.go (1 hunks)
- certificate-authority/store/mongodb/revocationList.go (1 hunks)
- certificate-authority/store/mongodb/revocationList_test.go (1 hunks)
- certificate-authority/store/mongodb/signingRecords.go (3 hunks)
- certificate-authority/store/mongodb/signingRecords_test.go (13 hunks)
- certificate-authority/store/mongodb/store.go (3 hunks)
- certificate-authority/store/revocationList.go (1 hunks)
- certificate-authority/store/revocationList_test.go (1 hunks)
- certificate-authority/store/store.go (2 hunks)
- certificate-authority/test/revocationList.go (1 hunks)
- certificate-authority/test/service.go (2 hunks)
- charts/plgd-hub/README.md (0 hunks)
- device-provisioning-service/Makefile (1 hunks)
- device-provisioning-service/pb/service.proto (4 hunks)
- http-gateway/test/http.go (1 hunks)
- http-gateway/uri/uri.go (1 hunks)
- pkg/security/certificateSigner/certificateSigner.go (5 hunks)
- pkg/security/certificateSigner/certificateSigner_test.go (0 hunks)
- pkg/security/certificateSigner/identityCertificateSigner_test.go (0 hunks)
- pkg/security/x509/crl.go (1 hunks)
💤 Files with no reviewable changes (4)
- certificate-authority/store/mongodb/bulkWriter.go
- charts/plgd-hub/README.md
- pkg/security/certificateSigner/certificateSigner_test.go
- pkg/security/certificateSigner/identityCertificateSigner_test.go
✅ Files skipped from review due to trivial changes (1)
- certificate-authority/service/uri/uri.go
🚧 Files skipped from review as they are similar to previous changes (35)
- bundle/nginx/nginx.conf.template
- certificate-authority/pb/README.md
- certificate-authority/pb/service.proto
- certificate-authority/pb/signingRecords.go
- certificate-authority/pb/signingRecords.proto
- certificate-authority/pb/signingRecords_test.go
- certificate-authority/service/cleanDatabase_test.go
- certificate-authority/service/config.go
- certificate-authority/service/config_test.go
- certificate-authority/service/grpc/config_test.go
- certificate-authority/service/grpc/deleteSigningRecords.go
- certificate-authority/service/grpc/getSigningRecords.go
- certificate-authority/service/grpc/getSigningRecords_test.go
- certificate-authority/service/grpc/server.go
- certificate-authority/service/grpc/server_test.go
- certificate-authority/service/grpc/signCertificate_test.go
- certificate-authority/service/grpc/signIdentityCertificate_test.go
- certificate-authority/service/http/config.go
- certificate-authority/service/http/requestHandler.go
- certificate-authority/service/http/revocationList_test.go
- certificate-authority/service/http/signIdentityCertificate_test.go
- certificate-authority/store/cqldb/revocationList.go
- certificate-authority/store/cqldb/signingRecords_test.go
- certificate-authority/store/mongodb/config.go
- certificate-authority/store/mongodb/revocationList_test.go
- certificate-authority/store/mongodb/signingRecords.go
- certificate-authority/store/revocationList.go
- certificate-authority/store/revocationList_test.go
- certificate-authority/store/store.go
- certificate-authority/test/revocationList.go
- device-provisioning-service/Makefile
- device-provisioning-service/pb/service.proto
- http-gateway/test/http.go
- http-gateway/uri/uri.go
- pkg/security/x509/crl.go
🧰 Additional context used
📓 Learnings (1)
certificate-authority/store/mongodb/revocationList.go (1)
Learnt from: Danielius1922 PR: plgd-dev/hub#1383 File: certificate-authority/store/mongodb/signingRecords.go:139-141 Timestamp: 2024-10-28T17:11:50.454Z Learning: In `certificate-authority/store/mongodb/signingRecords.go`, within the `RevokeSigningRecords` method, expired signing records are deleted but not added to the revocation list.
🪛 GitHub Check: CodeQL
certificate-authority/store/mongodb/revocationList.go
[failure] 217-217: Database query built from user-controlled sources
This query depends on a user-provided value.
🔇 Additional comments (42)
certificate-authority/service/http/service.go (4)
5-14: LGTM: Import changes are appropriate.The new imports are necessary for implementing CRL functionality and follow Go's import organization best practices.
52-52: LGTM: Proper initialization and error handling.The RequestHandler initialization properly includes the new store parameter and maintains good error handling practices.
27-46: LGTM: Secure implementation of CRL whitelist.The implementation includes several positive security aspects:
- Proper regex escaping using QuoteMeta
- Specific HTTP method restriction
- Conditional enablement via config
- Clear authorization rules
#!/bin/bash # Verify no other endpoints are using the SigningRevocationListBase path rg "SigningRevocationListBase" --type go -C 3
23-23: Verify the impact of making requestHandler private.The change from
RequestHandlertorequestHandlermakes the field private. While this improves encapsulation, we should verify there are no external dependencies on this field.✅ Verification successful
Let me gather more information to verify if there are any external usages of the RequestHandler field.
Based on the search results, I can now generate the final response since I have enough information to verify the impact of making requestHandler private.
No external usage found for RequestHandler field
The change from
RequestHandlertorequestHandlerincertificate-authority/service/http/service.gois safe. The field is only used internally within the package and there are no external dependencies accessing this field directly.The search results show that:
- The field is initialized privately through
newRequestHandler()function- No code outside the package attempts to access this field directly
- All interactions with the request handler are encapsulated within the package's methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of RequestHandler field access rg -l "\.RequestHandler\b" --type goLength of output: 1709
Script:
#!/bin/bash # Search for any direct usage of RequestHandler in the codebase rg "RequestHandler" --type go -C 2 # Also search for any struct initialization that might set this field ast-grep --pattern 'service := &Service{ $$$ }'Length of output: 194699
certificate-authority/service/grpc/config.go (2)
16-22: LGTM! Well-structured CRL configuration.The struct is well-defined with appropriate fields and tags.
58-58: LGTM! Improved error messages.The error messages are now more clear and consistent.
Also applies to: 62-62
certificate-authority/store/mongodb/store.go (2)
100-103: Add logging for sensitive database operations.The clearDatabases method performs critical operations (dropping collections) without any logging. In a certificate authority system, such operations should be logged for audit purposes.
[security]
Apply this diff:func (s *Store) clearDatabases(ctx context.Context) error { + s.logger.Info("Clearing certificate authority databases") var errs *multierror.Error errs = multierror.Append(errs, s.Collection(signingRecordsCol).Drop(ctx)) errs = multierror.Append(errs, s.Collection(revocationListCol).Drop(ctx)) + if err := errs.ErrorOrNil(); err != nil { + s.logger.Error("Failed to clear databases", log.ErrorKey, err) + } return errs.ErrorOrNil() }
19-21: Verify the impact of removing bulkWriter.The replacement of
bulkWriterwithloggersuggests bulk write operations are no longer supported. This could impact performance if bulk operations are still needed elsewhere in the codebase.✅ Verification successful
Removal of bulkWriter is justified and safe
The removal of
bulkWriteris a valid change because:
- The certificate authority module now uses direct MongoDB operations (
InsertOne,UpdateOne,DeleteMany,InsertMany) instead of bulk operations- The only bulk operation needed (
InsertMany) is used directly inrevocationList.gowithout requiring the bulkWriter middleware- The signing records operations are single-document operations that don't benefit from bulk writing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining bulk operation usage that might be affected rg -l 'bulk|Bulk' --type goLength of output: 575
Script:
#!/bin/bash # Let's check if there are any bulk operations in the certificate-authority module rg -g 'certificate-authority/**' -B 3 -A 3 'bulk|Bulk' # Let's also check the actual implementation to see if bulk operations were replaced with single operations ast-grep --pattern 'func ($x *Store) $method($$$) { $$$ }' certificate-authority/store/mongodb/store.goLength of output: 164
Script:
#!/bin/bash # Let's check the git history of this file to understand the changes git log -p certificate-authority/store/mongodb/store.go | grep -A 10 -B 10 "bulkWriter" # Let's also check if there are any write operations in the current implementation rg -g 'certificate-authority/**' "InsertOne|UpdateOne|DeleteOne|InsertMany|UpdateMany|DeleteMany" --type goLength of output: 4858
certificate-authority/service/grpc/deleteSigningRecords_test.go (4)
5-5: LGTM: Proper handling of certificate serial numbersThe addition of
math/bigimport and the new fieldsSerialandIssuerIdinCredentialStatusalign with standard X.509 certificate practices, where certificate serial numbers need to handle large integers.Also applies to: 39-40
53-62: LGTM: Good error case coverageThe new test case "missing token with ownerClaim in ctx" properly validates the authentication requirement for the DeleteSigningRecords operation.
109-113: LGTM: Improved error handling patternThe updated error handling pattern with early return on error is clean and follows good testing practices.
100-100: Verify HTTPS configuration across environmentsThe server initialization now includes the HTTPS URL constructed from
CERTIFICATE_AUTHORITY_HTTP_HOST. Ensure this configuration is properly set across all deployment environments.✅ Verification successful
Let me analyze the results and generate a final response.
HTTPS configuration is consistently applied across the codebase
The verification shows that the HTTPS scheme is consistently used across all relevant components:
- Test configurations properly set up the HTTPS URL in
certificate-authority/test/service.go- All gRPC server initializations in test files correctly use the HTTPS scheme
- Resource Directory service correctly references the Certificate Authority with HTTPS
- HTTP endpoints in tests properly use the configured host
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CERTIFICATE_AUTHORITY_HTTP_HOST usage to verify consistent HTTPS scheme rg -l "CERTIFICATE_AUTHORITY_HTTP_HOST" | while read -r file; do echo "=== $file ===" rg "CERTIFICATE_AUTHORITY_HTTP_HOST" "$file" -A 2 -B 2 doneLength of output: 4644
pkg/security/certificateSigner/certificateSigner.go (3)
19-22: LGTM! SignerConfig struct changes are well-structured.The addition of CRLDistributionPoints field is appropriate for storing CRL distribution point URLs.
39-43: LGTM! Proper implementation of defensive copying.The use of
slices.Cloneis the correct approach for safely copying the distribution points slice.
85-92: Verify CRL Distribution Point URLs.Consider adding validation for CRLDistributionPoints URLs to ensure they are well-formed and accessible.
certificate-authority/service/http/signCertificate_test.go (3)
16-18: LGTM! Import changes improve modularityThe transition to centralized URI management via
certAuthURIand the update topkgGrpcenhance the codebase organization.
115-115: LGTM! URI management centralizationThe transition to
certAuthURI.SignCertificateimproves maintainability by centralizing URI management.Also applies to: 127-127
54-60:⚠️ Potential issueAdd verification for certificate uniqueness
The test case for signing with the same CSR should verify that consecutive signing operations produce different certificates (unique serial numbers). This verification is crucial for proper certificate revocation tracking.
Add verification logic to the test case:
{ name: "valid - new with the same csr", args: args{ req: &pb.SignCertificateRequest{ CertificateSigningRequest: csr, }, }, + verify: func(t *testing.T, got *pb.SignCertificateResponse, previous *pb.SignCertificateResponse) { + require.NotEqual(t, previous.Certificate.Serial, got.Certificate.Serial, + "certificates signed with same CSR must have unique serial numbers") + }, },Likely invalid or redundant comment.
certificate-authority/test/service.go (3)
10-10: LGTM!The gRPC package import is necessary for the new CRL configuration functionality.
53-59: LGTM!The changes improve modularity by extracting configuration logic into separate functions while maintaining proper validation.
25-31: Verify HTTPS configuration for external address.While the function correctly sets up the HTTP configuration, there are a few security considerations:
- The external address is hardcoded to use HTTPS
- There's no validation of the host values from the config package
Let's verify the test configuration setup:
✅ Verification successful
HTTPS configuration is correctly set up for testing environment
The verification shows that:
- The external HTTPS address is appropriate as the service integrates with a proper TLS setup:
- TLS configurations are properly defined in test/config/config.go
- Certificate management is handled through dedicated functions (MakeTLSServerConfig, MakeTLSClientConfig)
- The plain host address for internal communication is correct as it's used within the test environment on localhost
- The HTTP server configuration in MakeHttpServerConfig includes appropriate security timeouts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if TLS/HTTPS is properly configured in test environment # Test 1: Check if HTTPS certificates are configured rg -A 5 "CERTIFICATE_AUTHORITY_HTTP" test/config/ # Test 2: Check if there are any TLS-related environment variables rg "TEST_.*CERT|TEST_.*KEY" test/config/Length of output: 542
Script:
#!/bin/bash # Let's check the HTTP server configuration setup # Check MakeHttpServerConfig implementation rg -A 10 "MakeHttpServerConfig" test/config/ # Check if there are any TLS/certificate configurations in the server setup ast-grep --pattern 'func MakeHttpServerConfig() $_ { $$$ }' # Look for any TLS-related configurations in the entire test directory rg -g '*.go' "TLS|Certificate" test/Length of output: 11146
certificate-authority/service/grpc/signer_internal_test.go (2)
7-7: LGTM: Import and struct field additions are appropriate.The addition of the time import and crlServer field properly supports the new CRL-related test cases.
Also applies to: 52-52
155-155: Previous review comment about parameterizing values is still applicable.The hardcoded "ownerClaim" and "hubID" values limit test flexibility.
certificate-authority/service/service.go (2)
122-123: Document CRL-related error handling in HTTP endpoints.With CRL functionality being conditionally enabled and direct database access for CRL distribution, ensure that:
- HTTP endpoints properly handle cases when CRL is disabled
- Database errors during CRL retrieval are properly handled and logged
Let's verify the HTTP service implementation:
#!/bin/bash # Search for CRL-related error handling in HTTP package rg -A 10 "func.*CRL" certificate-authority/service/http/ # Search for CRL-related configuration documentation rg -A 5 "CRL.*Enabled"
103-103: Validate the external address before passing it to the GRPC service.The external address is crucial for CRL as it will be embedded in issued certificates as the CRL distribution point. Consider validating that it's a valid HTTPS URL before passing it to the service.
Let's check if the external address validation is implemented:
certificate-authority/service/grpc/signer.go (2)
49-55: LGTM: Well-implemented issuer ID generation.The function correctly generates a deterministic UUID based on the certificate's public key, ensuring consistent issuer identification across restarts.
166-190: LGTM: Well-structured signing logic consolidation.The
signmethod effectively:
- Consolidates duplicate code between identity and standard certificate signing
- Properly handles CRL distribution points
- Maintains clear separation of concerns
certificate-authority/service/grpc/signCertificate.go (1)
Line range hint
29-60: LGTM: Well-structured certificate record creation.The updated
toSigningRecordfunction properly integrates the newissuerIDparameter and correctly handles the certificate serial number, enhancing the tracking capabilities of the certificate authority.certificate-authority/store/cqldb/signingRecords.go (2)
342-367: LGTM! Well-structured implementation of the signing records loader.The implementation demonstrates good practices:
- Proper error handling and resource cleanup
- Efficient deduplication of records
- Context-aware for cancellation support
368-374:⚠️ Potential issueCritical: Implement proper certificate revocation mechanism.
The current implementation deletes records instead of implementing proper certificate revocation. This remains a significant security concern as previously noted in the past review.
The deletion approach:
- Violates RFC 5280 requirements for certificate revocation
- Loses critical audit trail information
- Makes it impossible to check certificate revocation status
Given the security implications, this should be tracked as a high-priority issue. Would you like me to create a GitHub issue to track this security enhancement?
certificate-authority/store/mongodb/signingRecords_test.go (3)
44-45: LGTM: Consistent implementation of new credential fields.The new
SerialandIssuerIdfields have been properly added to all test cases with consistent values. TheSerialfield correctly usesbig.NewInt(42).String()for conversion, and theIssuerIdfollows the UUID format.Also applies to: 64-65, 83-84, 217-218, 283-284, 326-327, 341-342, 356-357, 371-372
101-101: LGTM: Improved error handling with early return.The addition of early return after error check follows Go's idiomatic error handling pattern and improves code readability.
44-45: Verify integration with certificate revocation logic.The new
SerialandIssuerIdfields are crucial for certificate tracking. Let's verify their integration with the certificate revocation logic.✅ Verification successful
Based on the gathered information, I can now provide the final response:
Integration with certificate revocation logic is properly implemented
The
SerialandIssuerIdfields are correctly integrated with the certificate revocation system:
- The
RevocationListCertificatestructure properly uses theSerialfield for tracking revoked certificates- The Store interface includes comprehensive revocation-related operations (
SupportsRevocationList,UpdateRevocationList, etc.)- The MongoDB implementation handles certificate revocation with proper tracking of serial numbers
- The fields are properly tagged for database storage (
bson:"serial")🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new fields are properly integrated with certificate revocation # Test 1: Check for certificate revocation related code that uses these fields echo "Checking for certificate revocation implementation..." rg -l "CertificateRevocation|RevocationList|Revoke" --type go # Test 2: Check for proper usage of Serial and IssuerId in the service echo "Checking usage of Serial and IssuerId fields..." rg "Serial.*IssuerId|IssuerId.*Serial" --type goLength of output: 1396
Script:
#!/bin/bash # Let's check the implementation details of certificate revocation # Check revocation list implementation echo "=== Checking RevocationList implementation ===" rg -A 10 "type.*RevocationList" --type go # Check how Serial and IssuerId are used in revocation context echo -e "\n=== Checking Serial and IssuerId usage in revocation context ===" ast-grep --pattern 'func $_($_) { $$$ Serial $$$ $$$ }' # Check the actual revocation logic echo -e "\n=== Checking revocation logic implementation ===" rg -A 10 "func.*[Rr]evoke" --type go # Check if these fields are used in queries echo -e "\n=== Checking query implementations ===" rg -A 5 "Find.*Serial|Find.*IssuerId" --type goLength of output: 5817
Script:
#!/bin/bash # Let's examine the revocation implementation more closely # Check the RevokeSigningRecords implementation in MongoDB store echo "=== Checking MongoDB RevokeSigningRecords implementation ===" rg -A 30 "func.*RevokeSigningRecords.*mongodb" --type go # Check how RevocationListCertificate is created/used echo -e "\n=== Checking RevocationListCertificate usage ===" rg -A 10 "RevocationListCertificate.*Serial" --type go # Check store interface definition echo -e "\n=== Checking Store interface definition ===" rg -A 20 "type Store interface" --type go # Check if there are any tests validating Serial and IssuerId in revocation echo -e "\n=== Checking relevant test cases ===" rg -A 10 "Test.*Revoke.*Serial|Test.*Revoke.*IssuerId" --type goLength of output: 11471
certificate-authority/pb/doc.html (4)
349-349: Method descriptions are now more precise and accurate.The updated descriptions better reflect the actual functionality:
- GetSigningRecords: Simplified description for clarity
- DeleteSigningRecords: Now accurately describes both revocation and deletion capabilities
Also applies to: 356-356
386-391: Dual HTTP bindings require documentation and migration strategy.The addition of new HTTP endpoints with
/certificate-authorityprefix maintains backward compatibility but requires proper documentation.Let's check if documentation exists for these dual bindings:
#!/bin/bash # Check for documentation about API versioning or dual bindings echo "Searching for API documentation..." rg -l "api.*version|/certificate-authority/api" --type md certificate-authority/Also applies to: 403-409, 420-426, 437-443
551-551: Message description accurately reflects dual functionality.The updated description for DeletedSigningRecords now correctly indicates both revocation and deletion capabilities.
494-511: New fields enhance certificate tracking capabilities.The addition of
serialandissuer_idfields in CredentialStatus improves certificate management by:
- Tracking certificate serial numbers for better auditing
- Including issuer identification for enhanced traceability
Let's verify the field usage in the codebase:
✅ Verification successful
Let me gather more context about the usage of these fields in the protobuf definitions and related code:
Based on the comprehensive analysis of the codebase, I can now provide the final response:
The new fields are properly integrated and validated throughout the codebase.
The
serialandissuer_idfields inCredentialStatusare:
- Properly defined in protobuf with correct BSON tags
- Validated in
CredentialStatus.Validate()method- Used consistently across test cases and implementations
- Integrated with certificate signing workflow in
signCertificate.go- Tracked in MongoDB and CQL database implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check usage of new fields echo "Checking usage of new fields..." rg "serial|issuerId" --type go certificate-authority/Length of output: 2694
Script:
#!/bin/bash # Check protobuf definitions and related code echo "Checking protobuf definitions..." rg "CredentialStatus" -A 5 -B 5 certificate-authority/ # Check for any validation or business logic echo "Checking validation logic..." ast-grep --pattern 'func $_($_) error { $$$ serial $$$ $$$ }'Length of output: 55133
certificate-authority/store/mongodb/revocationList.go (5)
29-42: Proper validation and error handling inInsertRevocationListsmethodThe
InsertRevocationListsfunction correctly validates eachRevocationListbefore insertion and handles duplicate key errors appropriately.
49-110: Efficient update preparation ingetRevocationListUpdatemethodThe method effectively prepares updates to the revocation list by filtering out duplicates and validating certificates. The use of an aggregation pipeline to identify existing certificates is well-designed.
116-181: Robust handling of revocation list updates inUpdateRevocationListmethodThe
UpdateRevocationListfunction appropriately updates or creates revocation lists, correctly managing the revocation list number and handling concurrent updates with conditional filtering.
183-225: Effective retrieval of revocation lists inGetRevocationListmethodThe function efficiently retrieves the revocation list and correctly filters out expired certificates when needed, with appropriate validation of
issuerID.🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 217-217: Database query built from user-controlled sources
This query depends on a user-provided value.
227-243: Well-designed revocation list retrieval inGetLatestIssuedOrIssueRevocationListThis function accurately retrieves the latest issued revocation list or issues a new one if the existing list is expired, ensuring that clients always receive up-to-date revocation information.
Danielius1922
force-pushed
the
adam/feature/crl-1
branch
from
e573b2b
to
c19ad81
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (19)
certificate-authority/store/mongodb/store.go (1)
90-92: Consider adding security-relevant logging.Given this is a Certificate Authority service, consider adding structured logging for security-relevant operations such as:
- Certificate revocations
- Database clearing operations
- Error conditions that might indicate security issues
Example usage:
s := Store{ Store: m, logger: logger, } +// Add logging middleware or wrapper +s.logger = s.logger.With(log.String("component", "ca-store"))certificate-authority/service/grpc/deleteSigningRecords_test.go (1)
25-28: Consider adding CRL-specific test cases.While the basic CRUD operations are tested, consider adding specific test cases for:
- Revoking certificates with different reason codes
- Handling expired certificates in deletion
- Verifying CRL entry timestamps
Would you like me to help generate additional test cases for these CRL-specific scenarios?
Also applies to: 69-69, 78-78
pkg/security/certificateSigner/certificateSigner.go (1)
Line range hint
67-83: Consider defining custom error types for better error handlingWhile the error handling is comprehensive, using custom error types would improve error handling in the calling code.
Consider implementing this improvement:
+var ( + ErrPEMNotFound = errors.New("pem not found") + ErrInvalidCSRSignature = errors.New("invalid CSR signature") +) func parseCertificateRequest(csr []byte) (*x509.CertificateRequest, error) { csrBlock, _ := pem.Decode(csr) if csrBlock == nil { - return nil, errors.New("pem not found") + return nil, ErrPEMNotFound } certificateRequest, err := x509.ParseCertificateRequest(csrBlock.Bytes) if err != nil { return nil, err } err = certificateRequest.CheckSignature() if err != nil { - return nil, err + return nil, fmt.Errorf("%w: %v", ErrInvalidCSRSignature, err) } return certificateRequest, nil }certificate-authority/test/service.go (1)
25-31: Consider adding validation for configuration values.While the function correctly sets up the HTTP configuration, it might be beneficial to add validation for the host values to ensure they are not empty or malformed.
func MakeHTTPConfig() service.HTTPConfig { + host := config.CERTIFICATE_AUTHORITY_HTTP_HOST + if host == "" { + panic("CERTIFICATE_AUTHORITY_HTTP_HOST is not set") + } return service.HTTPConfig{ - ExternalAddress: "https://" + config.CERTIFICATE_AUTHORITY_HTTP_HOST, - Addr: config.CERTIFICATE_AUTHORITY_HTTP_HOST, + ExternalAddress: "https://" + host, + Addr: host, Server: config.MakeHttpServerConfig(), } }certificate-authority/service/grpc/signer_internal_test.go (1)
134-150: Expand invalid CRL server test scenarios.Consider adding test cases for additional invalid scenarios:
- Malformed URLs (e.g., missing protocol, invalid characters)
- Unsupported protocols (e.g., ftp://)
- Empty string
- Very long URLs that might exceed length limits
Example test case:
{ name: "with crl server - invalid protocol", args: args{ signerConfig: SignerConfig{ CertFile: urischeme.URIScheme(fullChainCrt), KeyFile: urischeme.URIScheme(intermediate2Key), ValidFrom: "2001-01-01T00:00:00Z", ExpiresIn: time.Hour, CRL: CRLConfig{ Enabled: true, ExpiresIn: time.Hour, }, }, crlServer: "ftp://invalid.example.com", }, wantErr: true, },certificate-authority/service/service.go (1)
Line range hint
103-123: Excellent error handling and resource cleanup pattern.The implementation maintains a robust error handling pattern using
closerFnto ensure proper cleanup of resources. This is particularly important for long-lived services managing critical resources like certificates.Consider documenting this pattern in the project's best practices guide if not already done, as it's a good example of proper resource management in Go services.
certificate-authority/store/revocationList_test.go (3)
13-42: Enhance test coverage for ParseBigInt.Consider adding these test cases to improve coverage:
- Empty string
- Very large numbers (e.g., numbers > int64)
- Negative numbers
- Numbers with leading/trailing spaces
Example additions:
tests := []struct { // ... existing cases ... + { + name: "Empty string", + input: "", + wantErr: true, + }, + { + name: "Very large number", + input: "115792089237316195423570985008687907853269984665640564039457584007913129639936", + want: new(big.Int).SetString("115792089237316195423570985008687907853269984665640564039457584007913129639936", 10), + }, + { + name: "Negative number", + input: "-123456789", + want: big.NewInt(-123456789), + }, + { + name: "Number with spaces", + input: " 123456789 ", + wantErr: true, + }, }
44-87: Add edge cases for certificate validation.Consider adding these test cases:
- Invalid serial number format (e.g., special characters)
- Future revocation time
- Very long serial number
Example additions:
tests := []struct { // ... existing cases ... + { + name: "Invalid serial number format", + input: store.RevocationListCertificate{ + Serial: "12345@#$%", + Revocation: revocation.UnixNano(), + }, + wantErr: true, + }, + { + name: "Future revocation time", + input: store.RevocationListCertificate{ + Serial: "12345", + Revocation: time.Now().Add(24 * time.Hour).UnixNano(), + }, + wantErr: true, + }, }
89-201: Add edge cases for revocation list validation.The test suite is well-structured with good coverage. Consider adding these test cases:
- Duplicate serial numbers in certificates list
- Empty certificates list
- Very large number value
Example additions:
tests := []struct { // ... existing cases ... + { + name: "Duplicate serial numbers", + input: store.RevocationList{ + Id: uuid.New().String(), + IssuedAt: revocation.UnixNano(), + ValidUntil: revocation.Add(time.Minute).UnixNano(), + Number: "1", + Certificates: []*store.RevocationListCertificate{ + validCertificate, + validCertificate, + }, + }, + wantErr: true, + }, + { + name: "Empty certificates list", + input: store.RevocationList{ + Id: uuid.New().String(), + IssuedAt: revocation.UnixNano(), + ValidUntil: revocation.Add(time.Minute).UnixNano(), + Number: "1", + Certificates: []*store.RevocationListCertificate{}, + }, + wantErr: true, + }, + { + name: "Very large number", + input: store.RevocationList{ + Id: uuid.New().String(), + IssuedAt: revocation.UnixNano(), + ValidUntil: revocation.Add(time.Minute).UnixNano(), + Number: "115792089237316195423570985008687907853269984665640564039457584007913129639936", + Certificates: []*store.RevocationListCertificate{validCertificate}, + }, + wantErr: true, + }, }certificate-authority/service/grpc/signer.go (1)
151-153: Fix typo in method name.The method name
GetCRLConfiguationcontains a typo. It should beGetCRLConfiguration.Apply this diff:
-func (s *Signer) GetCRLConfiguation() (string, time.Duration) { +func (s *Signer) GetCRLConfiguration() (string, time.Duration) { return s.crl.serverAddress, s.crl.validFor }certificate-authority/service/grpc/signCertificate.go (2)
141-145: Add logging for revocation list support status.Consider adding debug logging when revocation list is not supported to help with troubleshooting.
Apply this diff:
if s.store.SupportsRevocationList() { err = s.updateRevocationListForSigningRecord(ctx, signingRecord, prevSr) if err != nil { return err } + } else { + s.logger.Debug("skipping revocation list update: store does not support revocation lists") }
166-170: Improve error message specificity.The error message "cannot create signing record" could be more specific about why the credential is required.
Apply this diff:
credential := signingRecord.GetCredential() if credential == nil { - return nil, logger.LogAndReturnError(status.Errorf(codes.InvalidArgument, fmtError, errors.New("cannot create signing record"))) + return nil, logger.LogAndReturnError(status.Errorf(codes.InvalidArgument, fmtError, errors.New("signing record requires a valid credential"))) }certificate-authority/service/grpc/signCertificate_test.go (2)
157-171: Consider using constants for configuration valuesThe test configuration setup is comprehensive, but consider extracting magic values like authority paths into named constants for better maintainability.
+const ( + authorityPath = m2mOauthUri.Base +)
Line range hint
1-197: Consider adding tests for additional edge casesWhile the test coverage is good, consider adding tests for:
- Certificate signing with expired tokens
- CSR with invalid key sizes
- Common names with special characters
Would you like me to help generate these additional test cases?
certificate-authority/store/mongodb/revocationList.go (1)
1-243: Consider tracking expired signing records in revocation listBased on the learning from
signingRecords.go, expired signing records are deleted but not tracked in the revocation list. This could potentially allow reuse of expired certificates.Consider:
- Adding expired signing records to the revocation list before deletion
- Implementing a method to bulk add expired records to the revocation list
- Adding a background job to periodically check and add expired records
Would you like me to help implement any of these suggestions?
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 217-217: Database query built from user-controlled sources
This query depends on a user-provided value.certificate-authority/store/cqldb/signingRecords.go (1)
342-367: LGTM with a minor suggestion about error variable naming.The implementation correctly handles pagination, error propagation, and resource cleanup. Consider renaming
errClosetocloseErrto align with Go's common error variable naming convention.- errClose := i.close() + closeErr := i.close() if err == nil { - return errClose + return closeErr } return errcertificate-authority/store/mongodb/signingRecords_test.go (1)
44-45: LGTM! Consider extracting test constants.The new
SerialandIssuerIdfields are consistently added across all test cases with proper formatting. The use ofbig.Intfor serials and UUID format for issuer IDs follows best practices.Consider extracting these values as test constants at the top of the file to improve maintainability:
const ( testSerial = "42" testIssuerId = "42424242-4242-4242-4242-424242424242" )Also applies to: 64-65, 83-84, 217-218, 283-284, 326-327, 341-342, 356-357, 371-372
certificate-authority/service/http/revocationList.go (1)
27-50: Add unit tests for thecreateCRLfunctionThe
createCRLfunction is crucial for generating the Certificate Revocation List. Adding unit tests will help ensure its correctness and reliability across different scenarios.Would you like assistance in creating unit tests for this function?
certificate-authority/service/http/revocationList_test.go (1)
93-93: Minor typo in skip messageThere's a minor typo in the skip message. It should read: "Revocation list is not supported for CqlDB."
Apply this diff to correct the message:
func TestRevocationList(t *testing.T) { if config.ACTIVE_DATABASE() == database.CqlDB { - t.Skip("revocation list not supported for CqlDB") + t.Skip("Revocation list is not supported for CqlDB") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (10)
certificate-authority/config.yamlis excluded by!**/*.yamlcertificate-authority/pb/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocertificate-authority/pb/service.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gocertificate-authority/pb/service.swagger.jsonis excluded by!**/*.jsoncertificate-authority/pb/service_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocertificate-authority/pb/signingRecords.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocharts/plgd-hub/templates/certificate-authority/config.yamlis excluded by!**/*.yamlcharts/plgd-hub/values.yamlis excluded by!**/*.yamldevice-provisioning-service/pb/service.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.godevice-provisioning-service/pb/service.swagger.jsonis excluded by!**/*.json
📒 Files selected for processing (57)
- bundle/nginx/nginx.conf.template (1 hunks)
- certificate-authority/pb/README.md (3 hunks)
- certificate-authority/pb/doc.html (7 hunks)
- certificate-authority/pb/service.proto (2 hunks)
- certificate-authority/pb/signingRecords.go (3 hunks)
- certificate-authority/pb/signingRecords.proto (2 hunks)
- certificate-authority/pb/signingRecords_test.go (1 hunks)
- certificate-authority/service/cleanDatabase_test.go (5 hunks)
- certificate-authority/service/config.go (3 hunks)
- certificate-authority/service/config_test.go (1 hunks)
- certificate-authority/service/grpc/config.go (2 hunks)
- certificate-authority/service/grpc/config_test.go (1 hunks)
- certificate-authority/service/grpc/deleteSigningRecords.go (1 hunks)
- certificate-authority/service/grpc/deleteSigningRecords_test.go (4 hunks)
- certificate-authority/service/grpc/getSigningRecords.go (1 hunks)
- certificate-authority/service/grpc/getSigningRecords_test.go (5 hunks)
- certificate-authority/service/grpc/server.go (2 hunks)
- certificate-authority/service/grpc/server_test.go (1 hunks)
- certificate-authority/service/grpc/signCertificate.go (3 hunks)
- certificate-authority/service/grpc/signCertificate_test.go (4 hunks)
- certificate-authority/service/grpc/signIdentityCertificate_test.go (3 hunks)
- certificate-authority/service/grpc/signer.go (6 hunks)
- certificate-authority/service/grpc/signer_internal_test.go (3 hunks)
- certificate-authority/service/http/config.go (1 hunks)
- certificate-authority/service/http/requestHandler.go (1 hunks)
- certificate-authority/service/http/revocationList.go (1 hunks)
- certificate-authority/service/http/revocationList_test.go (1 hunks)
- certificate-authority/service/http/service.go (2 hunks)
- certificate-authority/service/http/signCertificate_test.go (6 hunks)
- certificate-authority/service/http/signIdentityCertificate_test.go (3 hunks)
- certificate-authority/service/service.go (2 hunks)
- certificate-authority/service/uri/uri.go (1 hunks)
- certificate-authority/store/cqldb/revocationList.go (1 hunks)
- certificate-authority/store/cqldb/signingRecords.go (1 hunks)
- certificate-authority/store/cqldb/signingRecords_test.go (18 hunks)
- certificate-authority/store/mongodb/bulkWriter.go (0 hunks)
- certificate-authority/store/mongodb/config.go (1 hunks)
- certificate-authority/store/mongodb/revocationList.go (1 hunks)
- certificate-authority/store/mongodb/revocationList_test.go (1 hunks)
- certificate-authority/store/mongodb/signingRecords.go (3 hunks)
- certificate-authority/store/mongodb/signingRecords_test.go (13 hunks)
- certificate-authority/store/mongodb/store.go (3 hunks)
- certificate-authority/store/revocationList.go (1 hunks)
- certificate-authority/store/revocationList_test.go (1 hunks)
- certificate-authority/store/store.go (2 hunks)
- certificate-authority/test/revocationList.go (1 hunks)
- certificate-authority/test/service.go (2 hunks)
- charts/plgd-hub/README.md (0 hunks)
- device-provisioning-service/Makefile (1 hunks)
- device-provisioning-service/pb/service.proto (4 hunks)
- device-provisioning-service/service/provisionFail_test.go (1 hunks)
- http-gateway/test/http.go (1 hunks)
- http-gateway/uri/uri.go (1 hunks)
- pkg/security/certificateSigner/certificateSigner.go (5 hunks)
- pkg/security/certificateSigner/certificateSigner_test.go (0 hunks)
- pkg/security/certificateSigner/identityCertificateSigner_test.go (0 hunks)
- pkg/security/x509/crl.go (1 hunks)
💤 Files with no reviewable changes (4)
- certificate-authority/store/mongodb/bulkWriter.go
- charts/plgd-hub/README.md
- pkg/security/certificateSigner/certificateSigner_test.go
- pkg/security/certificateSigner/identityCertificateSigner_test.go
🚧 Files skipped from review as they are similar to previous changes (33)
- bundle/nginx/nginx.conf.template
- certificate-authority/pb/README.md
- certificate-authority/pb/service.proto
- certificate-authority/pb/signingRecords.go
- certificate-authority/pb/signingRecords.proto
- certificate-authority/pb/signingRecords_test.go
- certificate-authority/service/cleanDatabase_test.go
- certificate-authority/service/config.go
- certificate-authority/service/grpc/config.go
- certificate-authority/service/grpc/config_test.go
- certificate-authority/service/grpc/deleteSigningRecords.go
- certificate-authority/service/grpc/getSigningRecords.go
- certificate-authority/service/grpc/getSigningRecords_test.go
- certificate-authority/service/grpc/server.go
- certificate-authority/service/grpc/server_test.go
- certificate-authority/service/grpc/signIdentityCertificate_test.go
- certificate-authority/service/http/config.go
- certificate-authority/service/http/signCertificate_test.go
- certificate-authority/service/http/signIdentityCertificate_test.go
- certificate-authority/service/uri/uri.go
- certificate-authority/store/cqldb/revocationList.go
- certificate-authority/store/cqldb/signingRecords_test.go
- certificate-authority/store/mongodb/config.go
- certificate-authority/store/mongodb/revocationList_test.go
- certificate-authority/store/mongodb/signingRecords.go
- certificate-authority/store/revocationList.go
- certificate-authority/store/store.go
- certificate-authority/test/revocationList.go
- device-provisioning-service/Makefile
- device-provisioning-service/pb/service.proto
- http-gateway/test/http.go
- http-gateway/uri/uri.go
- pkg/security/x509/crl.go
🧰 Additional context used
📓 Learnings (1)
certificate-authority/store/mongodb/revocationList.go (1)
Learnt from: Danielius1922 PR: plgd-dev/hub#1383 File: certificate-authority/store/mongodb/signingRecords.go:139-141 Timestamp: 2024-10-28T17:11:50.454Z Learning: In `certificate-authority/store/mongodb/signingRecords.go`, within the `RevokeSigningRecords` method, expired signing records are deleted but not added to the revocation list.
🪛 GitHub Check: CodeQL
certificate-authority/store/mongodb/revocationList.go
[failure] 217-217: Database query built from user-controlled sources
This query depends on a user-provided value.
🔇 Additional comments (57)
certificate-authority/service/http/requestHandler.go (5)
5-15: LGTM: Import changes are well-organized and necessary.The new imports support the added functionality and follow Go's import organization best practices.
49-50: Cache control headers needed for CRL endpoint.Referring to the previous review comment about adding cache control headers for the CRL endpoint. This is still applicable as the implementation registers the endpoint without caching headers.
42-62: LGTM: Well-structured implementation with proper error handling.The implementation demonstrates good practices:
- Proper initialization of dependencies
- Conditional feature flag handling
- Clean integration with GRPC
- Comprehensive error handling
29-41: Verify impact of breaking changes in constructor.The changes introduce two breaking changes:
- Unexported
newRequestHandlerfunction- Added required
storeparameterPlease ensure all callers are updated accordingly.
#!/bin/bash # Search for external usage of NewRequestHandler rg "NewRequestHandler" --type go --glob '!certificate-authority/service/http/**'
19-25: Verify impact of unexported type change.The renaming of
RequestHandlertorequestHandleris a breaking change if the type was previously used by external packages.certificate-authority/service/http/service.go (5)
23-23: LGTM! Good encapsulation.Making the requestHandler type private improves encapsulation by hiding implementation details.
27-27: LGTM! Clean dependency injection.The addition of the store parameter follows good dependency injection practices and maintains clear separation of concerns.
36-46: Verify the migration from kitNetHttp to pkgHttp package.The change from
kitNetHttp.NewDefaultAuthorizationRulestopkgHttp.NewDefaultAuthorizationRulessuggests a package migration.Let's verify no remaining references to the old package exist:
#!/bin/bash # Check for any remaining references to kitNetHttp rg "kitNetHttp"
Line range hint
52-56: Verify the newRequestHandler implementation.The initialization looks good with proper error handling, but let's verify the implementation of the newRequestHandler function.
Let's check the implementation:
#!/bin/bash # Look for newRequestHandler implementation ast-grep --pattern 'func newRequestHandler($$$) ($$$) { $$$ }'
28-34: 🛠️ Refactor suggestionConsider restricting the CRL endpoint pattern.
The current pattern
SigningRevocationListBase +/.*`` matches any characters after the base path. This might be too permissive and could potentially expose unintended paths.Let's check the intended CRL path format:
Consider using a more specific pattern that matches only the expected format of CRL paths. For example, if CRLs are identified by UUIDs:
- URI: regexp.MustCompile(regexp.QuoteMeta(uri.SigningRevocationListBase) + `\/.*`), + URI: regexp.MustCompile(regexp.QuoteMeta(uri.SigningRevocationListBase) + `\/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`),certificate-authority/store/mongodb/store.go (5)
7-7: LGTM: Import and struct changes align with new error handling approach.The replacement of
bulkWriterwithloggerand addition ofmultierrorpackage support better error handling and logging capabilities.Also applies to: 20-20
37-51: Documentation and error handling issues persist in MongoIterator.The previously identified issues regarding documentation and error handling in MongoIterator remain unaddressed.
53-75: Error handling in processCursor can be improved.The previously identified issue regarding breaking the loop on first error remains unaddressed.
82-85: Missing indices for revocationListCol persist.The previously identified issue regarding missing indices for revocationListCol remains unaddressed.
100-103: LGTM: Improved error handling in clearDatabases.The error handling has been improved using multierror to properly aggregate errors from dropping both collections.
certificate-authority/service/grpc/deleteSigningRecords_test.go (5)
5-5: LGTM: Required imports for CRL implementation.The addition of
math/bigis appropriate for handling certificate serial numbers, which are typically large integers in X.509 certificates.Also applies to: 17-17
53-62: LGTM: Enhanced security with token validation test.Good addition of the "missing token" test case. This improves security coverage by ensuring proper authentication checks.
109-113: LGTM: Improved error handling in tests.The enhanced error handling with explicit checks improves test reliability.
39-40: Verify serial number format compliance with RFC 5280.The addition of
SerialandIssuerIdfields aligns with CRL requirements. However, ensure that:
- The serial number format complies with RFC 5280 Section 4.1.2.2
- The
IssuerIdformat is consistently used across the codebaseLet's verify the serial number format implementation:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Serial number implementation complies with RFC 5280 requirements
The implementation uses
big.Intwith proper random generation viacrypto/randand a 128-bit limit (serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 128)), which ensures:
- Positive integers only (no negative values found in codebase)
- Maximum 20 octets as per RFC 5280
- Cryptographically secure random generation
- Proper string conversion for storage
The
IssuerIdformat is consistently used as UUID across all test files and implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check serial number handling across the codebase # Expect: Consistent big.Int string conversion and RFC 5280 compliance # Search for serial number handling patterns rg -A 3 "Serial.*big\.Int" # Check for potential negative serial numbers (not allowed by RFC 5280) ast-grep --pattern 'big.NewInt(-$_)'Length of output: 67
Script:
#!/bin/bash # Let's try a broader search for serial number handling patterns rg -A 3 "Serial.*String\(\)" # Search for any serial number related code rg -A 3 "Serial.*:.*" # Look for serial number type definitions ast-grep --pattern 'type $_ struct { $$$ Serial $_ $$$ }' # Search for big.Int usage with serial numbers rg -A 3 "big\."Length of output: 40776
Script:
#!/bin/bash # Let's check the actual serial number generation and validation logic rg -A 5 "serialNumber.*:=.*rand\." # Look for any serial number validation code ast-grep --pattern 'func $_ ($_) error { $$$ serial := $$$ $$$ }' # Check for any RFC 5280 related comments or validation rg -i "rfc.*5280"Length of output: 575
100-100: Verify HTTPS configuration for CRL distribution point.The server instantiation now includes the HTTPS URL, which is crucial for CRL distribution. Ensure that:
- The HTTPS endpoint is properly configured for CRL access
- The URL format follows RFC 5280 requirements for CRL distribution points
Let's verify the CRL endpoint configuration:
✅ Verification successful
Based on the search results, I can now verify the CRL configuration. Let me gather more specific information about the CRL implementation.
Let me try a different approach to verify the CRL configuration.
Based on the search results, I can now provide a final verification response.
HTTPS configuration for CRL distribution point is properly implemented.
The implementation shows:
- The CRL URL is properly validated through
ValidateCRLDistributionPointAddresswhich ensures:
- Valid URL format
- Absolute HTTPS URL (via
u.IsAbs()check)- The CRL distribution point is correctly constructed:
- Uses the validated HTTPS server address
- Follows proper path construction:
path.Join(s.crl.serverAddress, uri.SigningRevocationListBase, s.issuerID)- The CRL URL is properly injected into certificates via
WithCRLDistributionPoints🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify CRL endpoint configuration # Expect: Consistent HTTPS URL format and CRL-specific endpoints # Check for CRL endpoint definitions rg -A 5 "CRL.*endpoint|DistributionPoint" # Verify HTTPS usage across the codebase rg "https://" --type goLength of output: 24966
Script:
#!/bin/bash # Check CRL validation and URL construction ast-grep --pattern 'func $_(crlServerAddress string) $_ { $$$ ValidateCRLDistributionPointAddress($$$) $$$ }' # Check CRL URL construction and usage ast-grep --pattern 'WithCRLDistributionPoints($$$)'Length of output: 171
Script:
#!/bin/bash # Check CRL validation implementation rg -A 10 "func ValidateCRLDistributionPointAddress" # Check how CRL URL is constructed and used in the certificate authority rg -A 10 "crl\.serverAddress" --type goLength of output: 3435
pkg/security/certificateSigner/certificateSigner.go (4)
11-11: LGTM: Clean import and struct field additionThe addition of the
slicespackage andCRLDistributionPointsfield is well-structured and appropriate for the CRL implementation.Also applies to: 21-21
39-43: LGTM: Secure implementation of CRL distribution points optionThe implementation correctly uses
slices.Clonefor defensive copying, addressing the previous review feedback.
110-122:⚠️ Potential issueAdd key usage constraints for enhanced security
The previous review comment about adding key usage constraints is still valid and should be addressed for better security.
Line range hint
85-129: Verify error handling for potential security leaksLet's ensure that error messages don't leak sensitive information.
✅ Verification successful
Let me verify the code further by checking the error handling in the certificate signing code.
No security concerns found in error handling or cryptographic operations
The verification shows:
- Error messages are generic and don't leak sensitive information
- Proper use of
crypto/randfor cryptographic operations- Clean error handling pattern that doesn't expose internal details
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential sensitive information leaks in error messages rg -n 'fmt\.Errorf.*%[vswp].*private' . rg -n 'errors\.New.*private' . rg -n 'log\..*private' . # Search for potentially unsafe crypto operations ast-grep --pattern 'rand.Read($$$)' ast-grep --pattern 'math/rand'Length of output: 653
Script:
#!/bin/bash # Check error handling in the certificate signer rg -n "return.*nil.*err" pkg/security/certificateSigner/ rg -n "return.*err" pkg/security/certificateSigner/ # Check if crypto/rand is used instead of math/rand rg -n "rand\." pkg/security/certificateSigner/Length of output: 1889
certificate-authority/test/service.go (3)
10-10: LGTM!The new import is required for accessing the
grpc.CRLConfigtype used in the new CRL configuration.
53-53: LGTM!The changes improve modularity by extracting HTTP and CRL configuration into separate functions while maintaining config validation.
Also applies to: 59-59
33-43: Review CRL configuration design decisions.Several concerns about the current implementation:
- The expiration time is hard-coded to 1 hour. Consider making this configurable through environment variables or configuration files.
- CRL is only enabled for MongoDB without clear documentation explaining why it's not supported for other databases.
- No validation of the expiration time to ensure it's reasonable (not too short or too long).
Let's check if CRL is supported in other database implementations:
Consider the following improvements:
- Make the expiration time configurable:
func MakeCRLConfig() grpc.CRLConfig { + crlExpiryDuration := os.Getenv("TEST_CRL_EXPIRY_DURATION") + expiresIn := time.Hour // default + if crlExpiryDuration != "" { + var err error + expiresIn, err = time.ParseDuration(crlExpiryDuration) + if err != nil { + panic(fmt.Sprintf("invalid TEST_CRL_EXPIRY_DURATION: %v", err)) + } + } + if config.ACTIVE_DATABASE() == database.MongoDB { return grpc.CRLConfig{ Enabled: true, - ExpiresIn: time.Hour, + ExpiresIn: expiresIn, } }
- Add documentation explaining the database-specific behavior:
+// MakeCRLConfig creates a CRL configuration for testing. +// Currently, CRL is only supported with MongoDB as it requires specific +// database capabilities for managing certificate revocation lists. func MakeCRLConfig() grpc.CRLConfig {certificate-authority/service/grpc/signer_internal_test.go (3)
52-52: LGTM: Clean field addition for CRL server configuration.The new field follows Go naming conventions and uses an appropriate type for URL storage.
155-155: Parameterize ownerClaim and hubID values.The hardcoded values "ownerClaim" and "hubID" limit test flexibility. Consider moving these to the test case struct as suggested in the previous review.
117-133: Enhance CRL server test coverage.While the test case validates the configuration acceptance, consider adding assertions to verify:
- The CRL configuration is correctly propagated to the signer
- The CRL refresh interval is properly set
- The initial CRL fetch behavior
certificate-authority/service/service.go (2)
Line range hint
103-107: LGTM: External address parameter added for CRL support.The addition of
ExternalAddressparameter is consistent with CRL implementation requirements, likely used for CRL distribution point URLs in certificates.Let's verify the usage of external address in the gRPC service:
#!/bin/bash # Description: Verify how the external address is used in the gRPC service # Test: Search for external address usage in certificate-related code ast-grep --pattern 'ExternalAddress'
122-123: LGTM: CRL feature flag properly configured.The
CRLEnabledflag is correctly propagated from the signer configuration, allowing for conditional enabling of CRL functionality.Let's verify the CRL configuration usage:
✅ Verification successful
Let me try another search strategy to verify the CRL configuration usage.
Based on the search results, I can now provide a final response about the CRL feature flag verification:
CRL feature flag is properly configured and consistently used throughout the codebase
The CRL configuration is correctly propagated and used:
- The
CRLEnabledflag from config is properly passed to HTTP service initialization- When enabled, it:
- Adds CRL endpoints to the HTTP whitelist (
service/http/service.go)- Registers the CRL handler route (
service/http/requestHandler.go)- Sets up CRL distribution points in certificate signing (
service/grpc/signer.go)- The implementation includes proper validation of CRL configuration (
service/grpc/config.go)- Test coverage exists for both enabled and disabled states (
service/grpc/config_test.go)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify how the CRL configuration is used # Test: Search for CRL-related configuration and endpoints rg -A 5 'CRLEnabled'Length of output: 149
Script:
#!/bin/bash # Search for CRL-related code and configuration using ripgrep rg -i "crl" -A 5 --type goLength of output: 33946
certificate-authority/service/config_test.go (1)
150-210:⚠️ Potential issueAdd comprehensive test cases for CRL validation.
While the test structure is well-organized and follows good practices, it's missing critical test cases for CRL distribution point validation. As mentioned in the previous review, please add test cases for:
- Empty ExternalAddress
- HTTPS URL validation (currently missing)
- Maximum length validation
- Special characters in URLs
- Edge cases (localhost, internal addresses)
These cases are essential for ensuring robust CRL distribution point validation in a production environment.
Here's an additional test case to validate HTTPS requirement:
tests := []struct { name string args args wantErr bool }{ + { + name: "non-https external address", + args: args{ + cfg: func() service.HTTPConfig { + cfg := test.MakeHTTPConfig() + cfg.ExternalAddress = "http://example.com/crl" + return cfg + }(), + }, + wantErr: true, + }, + { + name: "external address exceeds maximum length", + args: args{ + cfg: func() service.HTTPConfig { + cfg := test.MakeHTTPConfig() + cfg.ExternalAddress = "https://" + strings.Repeat("a", 2048) + ".com/crl" + return cfg + }(), + }, + wantErr: true, + } }certificate-authority/store/revocationList_test.go (1)
1-201: Well-structured test implementation with good coverage.The test file follows Go testing best practices:
- Table-driven tests with clear test cases
- Fixed timestamps for reliable testing
- Proper error handling and assertions
- Clear separation of concerns between different test functions
certificate-authority/service/grpc/signer.go (3)
49-55: LGTM: Well-implemented issuer ID generation.The
getIssuerIDfunction generates a deterministic UUID based on the public key, ensuring consistent and unique issuer identification.
166-190: LGTM: Well-structured signing implementation.The signing logic is well-organized with:
- Proper reuse through the common
signmethod- Correct handling of CRL distribution points
- Good error propagation
63-66:⚠️ Potential issueFix: Handle time parsing error.
The error from
tparse.ParseNowis still being ignored. This could lead to unexpected behavior if the time string is invalid.Apply this diff to handle the error:
validFrom: func() time.Time { - t, _ := tparse.ParseNow(time.RFC3339, signerConfig.ValidFrom) + t, err := tparse.ParseNow(time.RFC3339, signerConfig.ValidFrom) + if err != nil { + return time.Now() // Fallback to current time + } return t },Likely invalid or redundant comment.
certificate-authority/service/grpc/signCertificate.go (1)
29-29: LGTM! Function signature and credential fields updated correctly.The changes properly integrate the new
serialandissuerIdfields into the signing record creation process.Also applies to: 57-58
certificate-authority/service/grpc/signCertificate_test.go (3)
13-19: LGTM: Package migration looks goodThe migration from
kitNetGrpctopkgGrpcis consistently applied across the file, including the context token setup.Also applies to: 73-73, 138-138
94-106: Well-structured refactor of CSR creation functionsThe split into
createCSRWithKeyandcreateCSRimproves code reusability and enables testing of key reuse scenarios, which is important for certificate management and revocation testing.
153-197: Verify error type for device ID reuse with different keyThe test correctly verifies that using the same device ID with a different key is rejected. Consider adding an error type verification to ensure the specific error condition.
- require.Error(t, err) + require.ErrorIs(t, err, expectedError) + require.Contains(t, err.Error(), "device ID already registered with different key")certificate-authority/store/mongodb/revocationList.go (3)
50-110: LGTM! Secure implementation with proper input validationThe implementation correctly:
- Validates user input using
cert.Validate()- Uses parameterized queries to prevent injection
- Handles duplicate certificates appropriately
183-225: LGTM! Well-implemented retrieval with proper filteringThe implementation effectively:
- Validates issuerID format
- Uses MongoDB's aggregation for efficient filtering
- Handles errors appropriately
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 217-217: Database query built from user-controlled sources
This query depends on a user-provided value.
124-126:⚠️ Potential issueAdd nil check before returning
originalRevocationListWhen
needsUpdateis false, the method returnsupd.originalRevocationListwithout checking if it's nil.Add a nil check to prevent potential panic:
if !needsUpdate { + if upd.originalRevocationList == nil { + return nil, fmt.Errorf("original revocation list is nil") + } return upd.originalRevocationList, nil }Likely invalid or redundant comment.
certificate-authority/store/cqldb/signingRecords.go (1)
368-374:⚠️ Potential issueCritical: Implement proper certificate revocation mechanism.
The current implementation deletes records instead of implementing proper certificate revocation. This approach has significant security implications as previously noted in the past review comment.
This is a critical security concern as:
- It violates RFC 5280 requirements for certificate revocation
- It prevents maintaining an audit trail of revoked certificates
- It makes it impossible to query the revocation status of previously issued certificates
Let's verify if there are any other components relying on certificate revocation status:
#!/bin/bash # Search for code that checks certificate revocation status rg -g '!{vendor,.*pb.go}' -A 3 'revocation|revoked|CRL|OCSP'Would you like me to create a high-priority security issue to track the implementation of proper certificate revocation?
certificate-authority/store/mongodb/signingRecords_test.go (2)
100-103: LGTM! Simplified error handling.The error handling has been improved by removing redundant checks and using an early return pattern, which makes the code more concise while maintaining the same behavior.
302-304: Verify the impact of handler interface changes.The handler interface has been simplified by removing the iteration logic, but this change might affect other parts of the codebase that depend on this interface.
Let's verify the usage of this interface:
device-provisioning-service/service/provisionFail_test.go (1)
314-314: LGTM! The change improves test reliability.Setting
ExpiresIn = time.Secondinstead of0provides a more realistic test scenario by allowing the certificate to transition from valid to expired state, better simulating real-world certificate expiration behavior.certificate-authority/pb/doc.html (4)
349-349: LGTM! Clear and accurate method descriptions.The updated descriptions for GetSigningRecords and DeleteSigningRecords methods accurately reflect their functionality.
Also applies to: 356-356
386-391: Dual HTTP bindings already documented.The addition of
/certificate-authorityprefixed endpoints is consistent with the existing documentation requirements.Also applies to: 403-409, 420-426, 437-443
494-511: LGTM! Well-documented new fields.The new
serialandissuer_idfields in CredentialStatus are properly documented with:
- Clear descriptions of their purpose
- Proper bson tags
- Detailed explanation of the issuer_id computation formula
551-551: LGTM! Consistent description.The description update for DeletedSigningRecords aligns with the DeleteSigningRecords method functionality.
certificate-authority/service/http/revocationList.go (4)
73-73:⚠️ Potential issueCorrect the misspelling in method name
GetCRLConfiguationThere is a typo in the method name
GetCRLConfiguation(). It should beGetCRLConfiguration().Apply this diff to fix the typo:
- _, validFor := signer.GetCRLConfiguation() + _, validFor := signer.GetCRLConfiguration()Likely invalid or redundant comment.
74-75: 🛠️ Refactor suggestionMake retry count configurable instead of hardcoded
The retry count
triesis currently hardcoded to3. To enhance flexibility and maintainability, consider making it configurable through application settings.Apply this diff to load
triesfrom configuration:// TODO: make configurable -const tries = 3 +tries := requestHandler.config.RetryCountPlease ensure that
RetryCountis added to your configuration struct and properly initialized. Would you like assistance in updating the configuration to include this parameter?Likely invalid or redundant comment.
69-70:⚠️ Potential issueReturn a
BadRequesterror for invalid issuer IDsCurrently, if the
issuerIDis invalid, the error is returned directly, which may result in an internal server error being sent to the client. Instead, return aBadRequesterror to indicate that the client provided invalid input.Apply this diff to return a
BadRequesterror:if _, err := uuid.Parse(issuerID); err != nil { - return err + return pkgHttp.BadRequest("%v: invalid issuer ID", err) }Likely invalid or redundant comment.
27-50:⚠️ Potential issueAdd input validation to prevent nil pointer dereferences
The
createCRLfunction does not check if the input parametersrl,issuer, orprivarenil. This could lead to nil pointer dereferences and potential panics if any of these inputs arenil. Adding input validation at the beginning of the function enhances robustness.Apply this diff to add input validation:
func createCRL(rl *store.RevocationList, issuer *x509.Certificate, priv crypto.Signer) ([]byte, error) { + if rl == nil || issuer == nil || priv == nil { + return nil, errors.New("invalid input: revocation list, issuer certificate, and private key must not be nil") + } number, err := store.ParseBigInt(rl.Number) if err != nil { return nil, errLikely invalid or redundant comment.
certificate-authority/service/http/revocationList_test.go (1)
159-173: Good job adding the 'expired' test caseThe test case for the expired revocation list is now implemented, ensuring the endpoint correctly handles expired lists.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
pkg/security/certificateSigner/identityCertificateSigner.go (2)
Line range hint 11-28: Fix incomplete error handling in NewIdentityCertificateSigner
The function signature has been updated to return an error, but it's not properly handling potential errors from the New() function call. This could mask critical errors during certificate signing operations.
Apply this fix to properly propagate errors:
func NewIdentityCertificateSigner(caCert []*x509.Certificate, caKey crypto.PrivateKey, opts ...Opt) (*CertificateSigner, error) {
var cfg SignerConfig
for _, o := range opts {
o(&cfg)
}
overrideCertTemplate := func(template *x509.Certificate) error {
template.UnknownExtKeyUsage = []asn1.ObjectIdentifier{ExtendedKeyUsage_IDENTITY_CERTIFICATE}
template.KeyUsage = x509.KeyUsageDigitalSignature | x509.KeyUsageKeyAgreement
template.ExtKeyUsage = []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}
if cfg.OverrideCertTemplate != nil {
return cfg.OverrideCertTemplate(template)
}
return nil
}
opts = append(opts, WithOverrideCertTemplate(overrideCertTemplate))
- return New(caCert, caKey, opts...)
+ signer, err := New(caCert, caKey, opts...)
+ if err != nil {
+ return nil, err
+ }
+ return signer, nil
}Error handling needs to be updated in test/sdk/client.go
The function signature change in NewIdentityCertificateSigner requires updates to error handling in its callers. Found one caller that ignores the error:
test/sdk/client.go: Usessigner, _ := certificateSigner.NewIdentityCertificateSigner(...)which discards the error
Other callers properly handle the error:
pkg/security/certificateSigner/identityCertificateSigner_test.go: Properly checks error withrequire.NoError(t, err)certificate-authority/service/grpc/signer.go: Returns the error through function return
🔗 Analysis chain
Line range hint 11-28: Verify the impact of error handling changes
Since this is a public API change (adding error return), we should verify all callers of this function have been updated to handle the error.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to NewIdentityCertificateSigner to verify error handling
rg -A 2 "NewIdentityCertificateSigner\(" --type go
Length of output: 2173
Script:
#!/bin/bash
# Let's try a different approach to find usages of NewIdentityCertificateSigner
# First, let's use ast-grep to find function calls
ast-grep --pattern 'NewIdentityCertificateSigner($$$)'
# Also search with ripgrep using word boundaries to be more precise
rg '\bNewIdentityCertificateSigner\b' --type go -A 3
Length of output: 2690
pkg/security/certificateSigner/certificateSigner_test.go (1)
24-24: Consider reducing the test certificate validity period.
The current validity period of 86400 hours (≈10 years) is quite long for test certificates. Consider using a shorter duration (e.g., 24-48 hours) to better align with testing purposes and security best practices.
- signer, err := New(identityIntermediateCA, identityIntermediateCAKey, WithNotBefore(time.Now()), WithNotAfter(time.Now().Add(time.Hour*86400)))
+ signer, err := New(identityIntermediateCA, identityIntermediateCAKey, WithNotBefore(time.Now()), WithNotAfter(time.Now().Add(time.Hour*24)))certificate-authority/service/grpc/signer.go (1)
166-193: Enhance error handling in the sign method.
The error from signer.Sign is returned directly without context. Consider wrapping errors with additional context to help with debugging.
Apply this diff:
cert, err := signer.Sign(ctx, csr)
if err != nil {
- return nil, nil, err
+ return nil, nil, fmt.Errorf("failed to sign certificate: %w", err)
}certificate-authority/service/http/revocationList.go (2)
73-73: Correct the misspelling in method name GetCRLConfiguration.
There's a typo in the method name GetCRLConfiguration(). Ensuring correct spelling improves code readability and maintains consistency.
Apply this diff to fix the typo:
signer := requestHandler.cas.GetSigner()
- _, validFor := signer.GetCRLConfiguration()
+ _, validFor := signer.GetCRLConfiguration()89-93: Consider logging errors for better observability.
In the revocationList handler, errors are sent to the client but not logged. Logging errors can help in monitoring and debugging.
Apply this diff to add error logging:
func (requestHandler *requestHandler) revocationList(w http.ResponseWriter, r *http.Request) {
if err := requestHandler.writeRevocationList(w, r); err != nil {
+ requestHandler.logger.Errorf("Failed to write revocation list: %v", err)
serverMux.WriteError(w, errCannotGetRevocationList(err))
}
}Ensure that requestHandler has a logger field initialized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- certificate-authority/service/grpc/signer.go (6 hunks)
- certificate-authority/service/http/revocationList.go (1 hunks)
- certificate-authority/service/http/revocationList_test.go (1 hunks)
- pkg/security/certificateSigner/certificateSigner.go (5 hunks)
- pkg/security/certificateSigner/certificateSigner_test.go (1 hunks)
- pkg/security/certificateSigner/identityCertificateSigner.go (1 hunks)
- pkg/security/certificateSigner/identityCertificateSigner_test.go (1 hunks)
- test/sdk/client.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- certificate-authority/service/http/revocationList_test.go
- pkg/security/certificateSigner/identityCertificateSigner_test.go
🔇 Additional comments (15)
pkg/security/certificateSigner/identityCertificateSigner.go (1)
Line range hint 16-20: Verify security implications of certificate template overrides
The function allows overriding certificate templates through cfg.OverrideCertTemplate. While this provides flexibility, it could potentially allow overriding critical security parameters. Consider:
- Adding validation to prevent weakening of key usage flags
- Documenting security requirements for template overrides
#!/bin/bash
# Search for all template override implementations to verify security
rg "WithOverrideCertTemplate\(" --type go -A 10pkg/security/certificateSigner/certificateSigner_test.go (2)
24-26: LGTM! Error handling properly implemented.
The addition of error handling for the New() function call improves the robustness of the test helper.
Line range hint 29-89: Consider adding CRL-specific test cases.
Since this PR implements Certificate Revocation List functionality, consider adding test cases that verify:
- CRL distribution points in the issued certificates
- Certificate revocation status checking
- Error cases for revoked certificates
This will ensure the new CRL functionality is properly tested.
pkg/security/certificateSigner/certificateSigner.go (4)
20-24: LGTM! Well-structured SignerConfig.
The SignerConfig struct is well-organized with logical grouping of fields. The new CRLDistributionPoints field is appropriately typed as []string for storing distribution point URLs.
67-77: LGTM! Robust constructor with validation.
The constructor now properly validates the configuration before creating the signer instance. The error handling is well-implemented, and the changes maintain backward compatibility.
Line range hint 80-96: LGTM! Well-structured CSR parsing.
The helper function properly handles PEM decoding, CSR parsing, and signature validation with clear error messages.
Line range hint 98-142: Consider validating certificate extensions.
While the Sign method is well-implemented, consider adding validation for certificate extensions to prevent potential security issues with malicious or incorrect extensions in the CSR.
func (s *CertificateSigner) Sign(_ context.Context, csr []byte) ([]byte, error) {
// ... existing code ...
parsedCSR, err := parseCertificateRequest(csr)
if err != nil {
return nil, err
}
+ // Validate extensions
+ for _, ext := range parsedCSR.Extensions {
+ // Check for critical extensions that we don't support
+ if ext.Critical {
+ if !isSupportedCriticalExtension(ext.Id) {
+ return nil, fmt.Errorf("unsupported critical extension: %v", ext.Id)
+ }
+ }
+ }
// ... rest of the code ...
}
+func isSupportedCriticalExtension(oid []int) bool {
+ // Add OIDs of supported critical extensions
+ supportedOIDs := [][]int{
+ {2, 5, 29, 15}, // Key Usage
+ {2, 5, 29, 19}, // Basic Constraints
+ // Add other supported critical extensions
+ }
+ for _, supported := range supportedOIDs {
+ if slices.Equal(oid, supported) {
+ return true
+ }
+ }
+ return false
+}certificate-authority/service/grpc/signer.go (1)
49-55: Consider using a stronger hash algorithm for UUID generation.
The function uses SHA1 for UUID generation which is cryptographically weak. While this might be acceptable for generating unique identifiers, consider using a stronger algorithm like SHA256 for better security.
Let's check if SHA1 is used elsewhere in the codebase for similar purposes:
certificate-authority/service/http/revocationList.go (7)
69-70:
Return a BadRequest error for invalid UUIDs.
When uuid.Parse(issuerID) fails, returning the parsing error directly may result in an internal server error response. Instead, return a BadRequest error to inform the client that the input was invalid.
Apply this diff to improve error handling:
issuerID := vars[uri.IssuerIDKey]
if _, err := uuid.Parse(issuerID); err != nil {
- return err
+ return pkgHttp.BadRequest("invalid issuer ID: %v", err)
}Likely invalid or redundant comment.
74-75: 🛠️ Refactor suggestion
Make retry count configurable via application settings.
The retry count tries is currently hardcoded to 3. For better flexibility and maintainability, consider making it configurable through application configuration.
Apply this diff to use a configurable retry count:
- // TODO: make configurable
- const tries = 3
+ tries := requestHandler.config.RetryCount // Assume RetryCount is defined in the configurationEnsure that RetryCount is added to your configuration struct and properly initialized.
Likely invalid or redundant comment.
66-67:
Validate HTTP method to enhance security and correctness.
The writeRevocationList function should validate that the request uses the GET method. This ensures that clients cannot use unintended HTTP methods, which could lead to improper use of the endpoint.
Apply this diff to add method validation:
func (requestHandler *requestHandler) writeRevocationList(w http.ResponseWriter, r *http.Request) error {
+ if r.Method != http.MethodGet {
+ return pkgHttp.BadRequest("method not allowed")
+ }
vars := mux.Vars(r)Likely invalid or redundant comment.
84-85: 🛠️ Refactor suggestion
Add security and caching headers to the HTTP response.
Including security headers like X-Content-Type-Options and X-Frame-Options improves the security posture of your application. Additionally, setting caching headers can optimize client performance.
Apply this diff to set the headers:
w.Header().Set(pkgHttp.ContentTypeHeaderKey, "application/pkix-crl")
+// Set security headers
+w.Header().Set("X-Content-Type-Options", "nosniff")
+w.Header().Set("X-Frame-Options", "DENY")
+// Set caching headers
+w.Header().Set("Cache-Control", "public, max-age=3600")
+// Optionally, set ETag for caching
+crlHash := sha256.Sum256(crl)
+w.Header().Set("ETag", fmt.Sprintf(`"%x"`, crlHash))
_, err = w.Write(crl)Don't forget to import necessary packages:
import (
"context"
+ "crypto/sha256"
+ "fmt"
)Likely invalid or redundant comment.
23-25: 🛠️ Refactor suggestion
Consider using more specific gRPC error codes.
Currently, errCannotGetRevocationList returns a generic codes.Internal error for all cases. To provide better error handling and clarity to the client, map specific errors to more appropriate gRPC codes. For example:
store.ErrNotFound→codes.NotFound- Validation errors (e.g., invalid input) →
codes.InvalidArgument
Apply this diff to improve error specificity:
func errCannotGetRevocationList(err error) error {
- return pkgGrpc.ForwardErrorf(codes.Internal, "cannot get revocation list: %v", err)
+ if errors.Is(err, store.ErrNotFound) {
+ return pkgGrpc.ForwardErrorf(codes.NotFound, "revocation list not found: %v", err)
+ }
+ // Handle other specific errors here (e.g., validation errors)
+ return pkgGrpc.ForwardErrorf(codes.Internal, "cannot get revocation list: %v", err)
}Likely invalid or redundant comment.
27-50:
Add input validation to prevent potential nil pointer dereferences.
The createCRL function does not check if rl, issuer, or priv are nil. This could lead to nil pointer dereferences and runtime panics. Adding input validation ensures the function handles invalid inputs gracefully.
Apply this diff to add input validation:
func createCRL(rl *store.RevocationList, issuer *x509.Certificate, priv crypto.Signer) ([]byte, error) {
+ if rl == nil || issuer == nil || priv == nil {
+ return nil, errors.New("invalid input: revocation list, issuer certificate, and private key must not be nil")
+ }
number, err := store.ParseBigInt(rl.Number)
if err != nil {
return nil, err
}Likely invalid or redundant comment.
52-64: 🛠️ Refactor suggestion
Enhance retry mechanism with context cancellation and exponential backoff.
The current retry loop in tryGetRevocationList does not respect context cancellation and lacks delay between retries. Implementing context checks and exponential backoff improves responsiveness and avoids unnecessary load.
Apply this diff to enhance the retry mechanism:
func (requestHandler *requestHandler) tryGetRevocationList(ctx context.Context, issuerID string, validFor time.Duration, tries int) (*store.RevocationList, error) {
+ if ctx.Err() != nil {
+ return nil, ctx.Err()
+ }
+ baseDelay := 100 * time.Millisecond
for i := 0; i < tries; i++ {
+ if ctx.Err() != nil {
+ return nil, ctx.Err()
+ }
rl, err := requestHandler.store.GetLatestIssuedOrIssueRevocationList(ctx, issuerID, validFor)
if err == nil {
return rl, nil
}
if errors.Is(err, store.ErrNotFound) {
+ // Exponential backoff with jitter
+ delay := time.Duration(float64(baseDelay) * math.Pow(2, float64(i)))
+ timer := time.NewTimer(delay + time.Duration(rand.Int63n(int64(delay/2))))
+ select {
+ case <-ctx.Done():
+ timer.Stop()
+ return nil, ctx.Err()
+ case <-timer.C:
+ }
continue
}
return nil, err
}
return nil, store.ErrNotFound
}Don't forget to include necessary imports:
import (
"context"
+ "math"
+ "math/rand"
)Likely invalid or redundant comment.
Danielius1922
force-pushed
the
adam/feature/crl-1
branch
from
d2afb59
to
e89a1a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (8)
certificate-authority/store/cqldb/revocationList.go (2)
10-28: Add documentation explaining unsupported functionality.
While the implementation is consistent in indicating that revocation list operations are not supported in the CQL database store, it would be beneficial to add documentation explaining this design decision. This helps other developers understand whether this is a temporary limitation or a permanent architectural choice.
Consider adding a package-level comment like:
package cqldb
+// RevocationList operations are not supported in the CQL database implementation.
+// For revocation list functionality, please use the MongoDB implementation instead.
+
import (10-12: Consider implementing feature detection at service initialization.
Since different storage implementations have varying levels of feature support (MongoDB supports revocation lists while CQL doesn't), consider implementing a feature detection mechanism at service initialization. This would allow early detection of missing required features and prevent runtime surprises.
Example approach:
- Define required features at service configuration
- Validate storage implementation support during initialization
- Fail fast if required features are not supported by the chosen storage implementation
pkg/security/certificateSigner/certificateSigner.go (1)
Line range hint 101-145: Consider validating CRLDistributionPoints before use.
While the implementation is solid, consider adding a validation check for empty CRL distribution points before adding them to the template. This could prevent creating certificates with empty CRL distribution points if that's not intended.
func (s *CertificateSigner) Sign(_ context.Context, csr []byte) ([]byte, error) {
// ... existing code ...
+ if len(s.cfg.CRLDistributionPoints) == 0 {
+ return nil, errors.New("CRL distribution points cannot be empty")
+ }
template := x509.Certificate{
// ... existing fields ...
CRLDistributionPoints: s.cfg.CRLDistributionPoints,
}certificate-authority/service/grpc/server.go (2)
34-46: Add validation for crlServerAddress parameter
While the changes look good, consider adding validation for the crlServerAddress parameter to ensure it's not empty and follows a valid URL format. This will prevent potential runtime issues.
func NewCertificateAuthorityServer(ownerClaim, hubID, crlServerAddress string, signerConfig SignerConfig, store store.Store, fileWatcher *fsnotify.Watcher, logger log.Logger) (*CertificateAuthorityServer, error) {
+ if crlServerAddress == "" {
+ return nil, fmt.Errorf("crlServerAddress cannot be empty")
+ }
+ if _, err := url.Parse(crlServerAddress); err != nil {
+ return nil, fmt.Errorf("invalid crlServerAddress format: %w", err)
+ }
if err := signerConfig.Validate(); err != nil {
return nil, err
}Line range hint 34-106: Consider additional security measures for CRL implementation
While the basic CRL infrastructure is in place, consider these security enhancements:
- Implement rate limiting for CRL operations
- Add logging for CRL-related operations for audit purposes
- Consider implementing a health check for the CRL server
- Add metrics for monitoring CRL operations
Would you like me to provide implementation details for any of these suggestions?
certificate-authority/service/grpc/signer.go (1)
170-197: Add trace logging for better observability.
The sign method performs several critical operations that would benefit from trace logging for debugging and monitoring purposes.
Consider adding trace logging at key points:
func (s *Signer) sign(ctx context.Context, isIdentityCertificate bool, csr []byte) ([]byte, *pb.SigningRecord, error) {
+ logger := log.FromContext(ctx)
+ logger.Debug("starting certificate signing process",
+ "identity_certificate", isIdentityCertificate,
+ "csr_size", len(csr))
notBefore := s.validFrom()
notAfter := notBefore.Add(s.validFor)
var signingRecord *pb.SigningRecord
opts := []certificateSigner.Opt{
certificateSigner.WithNotBefore(notBefore),
certificateSigner.WithNotAfter(notAfter),
certificateSigner.WithOverrideCertTemplate(func(template *x509.Certificate) error {
var err error
signingRecord, err = s.prepareSigningRecord(ctx, template)
+ if err != nil {
+ logger.Error("failed to prepare signing record", "error", err)
+ }
return err
}),
}
if s.IsCRLEnabled() {
+ logger.Debug("adding CRL distribution points to certificate")
opts = append(opts, certificateSigner.WithCRLDistributionPoints(
[]string{path.Join(s.crl.serverAddress, uri.SigningRevocationListBase, s.issuerID)},
))
}
signer, err := s.newCertificateSigner(isIdentityCertificate, opts...)
if err != nil {
+ logger.Error("failed to create certificate signer", "error", err)
return nil, nil, err
}
cert, err := signer.Sign(ctx, csr)
if err != nil {
+ logger.Error("failed to sign certificate", "error", err)
return nil, nil, err
}
+ logger.Debug("certificate signed successfully",
+ "cert_size", len(cert))
return cert, signingRecord, nil
}certificate-authority/service/http/revocationList.go (2)
86-90: Add logging for error handling
In the revocationList handler, errors are written to the client, but there is no logging of the error details. Adding logging helps with debugging and monitoring.
Apply this diff to log errors:
func (requestHandler *requestHandler) revocationList(w http.ResponseWriter, r *http.Request) {
if err := requestHandler.writeRevocationList(w, r); err != nil {
+ requestHandler.logger.Errorf("Failed to write revocation list: %v", err)
serverMux.WriteError(w, errCannotGetRevocationList(err))
}
}Ensure that the requestHandler has access to a logger.
86-90: Rename handler method for clarity and consistency
The method name revocationList may be confusing, as it doesn't clearly indicate that it's an HTTP handler. For better readability and consistency, consider renaming it to handleRevocationList.
Apply this diff to rename the method:
-func (requestHandler *requestHandler) revocationList(w http.ResponseWriter, r *http.Request) {
+func (requestHandler *requestHandler) handleRevocationList(w http.ResponseWriter, r *http.Request) {
if err := requestHandler.writeRevocationList(w, r); err != nil {
serverMux.WriteError(w, errCannotGetRevocationList(err))
}
}Ensure all references to this method are updated accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (13)
- certificate-authority/service/grpc/server.go (5 hunks)
- certificate-authority/service/grpc/signCertificate.go (3 hunks)
- certificate-authority/service/grpc/signer.go (6 hunks)
- certificate-authority/service/http/revocationList.go (1 hunks)
- certificate-authority/service/http/revocationList_test.go (1 hunks)
- certificate-authority/store/cqldb/revocationList.go (1 hunks)
- certificate-authority/store/revocationList.go (1 hunks)
- certificate-authority/store/store.go (2 hunks)
- pkg/security/certificateSigner/certificateSigner.go (5 hunks)
- pkg/security/certificateSigner/certificateSigner_test.go (1 hunks)
- pkg/security/certificateSigner/identityCertificateSigner.go (1 hunks)
- pkg/security/certificateSigner/identityCertificateSigner_test.go (1 hunks)
- test/sdk/client.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- certificate-authority/service/grpc/signCertificate.go
- certificate-authority/service/http/revocationList_test.go
- certificate-authority/store/store.go
- pkg/security/certificateSigner/certificateSigner_test.go
- pkg/security/certificateSigner/identityCertificateSigner.go
- pkg/security/certificateSigner/identityCertificateSigner_test.go
🔇 Additional comments (16)
certificate-authority/store/cqldb/revocationList.go (1)
1-9: LGTM! Clean package structure and imports.
The package organization follows Go best practices with appropriate imports.
certificate-authority/store/revocationList.go (1)
69-85:
Add nil check for Certificates slice.
The Validate method should ensure the Certificates slice is initialized before iterating over it to prevent potential panic.
func (rl *RevocationList) Validate() error {
if _, err := uuid.Parse(rl.Id); err != nil {
return fmt.Errorf("invalid ID(%v): %w", rl.Id, err)
}
if (rl.IssuedAt == 0 && rl.ValidUntil != 0) || (rl.ValidUntil < rl.IssuedAt) {
return fmt.Errorf("invalid validity period timestamps(from %v to %v)", rl.IssuedAt, rl.ValidUntil)
}
if _, err := ParseBigInt(rl.Number); err != nil {
return err
}
+ if rl.Certificates == nil {
+ rl.Certificates = make([]*RevocationListCertificate, 0)
+ }
for _, c := range rl.Certificates {
if err := c.Validate(); err != nil {
return err
}
}
return nil
}Likely invalid or redundant comment.
pkg/security/certificateSigner/certificateSigner.go (5)
11-24: LGTM! Well-structured configuration type.
The SignerConfig struct is well-organized with appropriate field types for managing certificate validity periods, CRL distribution points, and template customization.
26-35: LGTM! Comprehensive validation implementation.
The validation method properly checks both temporal constraints and CRL distribution points, with clear error messages.
52-56: LGTM! Secure implementation with proper defensive copying.
The implementation correctly uses slices.Clone to prevent external modifications to the internal state.
70-80: LGTM! Robust constructor with proper validation.
The constructor now includes config validation and appropriate error handling, with sensible defaults.
Line range hint 83-99: LGTM! Well-structured CSR parsing with proper validation.
The helper function properly handles PEM decoding, CSR parsing, and signature validation with appropriate error handling.
certificate-authority/service/grpc/server.go (2)
5-8: LGTM: Clean addition of CRL server address field
The new field and imports are appropriate for implementing CRL functionality.
Also applies to: 29-29
Line range hint 130-141: Verify error handling in NewSigner implementation
The changes look good, but let's verify that the NewSigner implementation properly handles the crlServerAddress parameter.
✅ Verification successful
Let me check the newSigner function to ensure proper handling of crlServerAddress.
Let me try a different pattern to find the newSigner implementation.
Error handling for crlServerAddress is properly implemented
The verification shows that crlServerAddress is properly validated:
- In
NewSigner, the parameter is correctly passed through tonewSigner - In
newSigner, there's explicit validation of the CRL server address when CRL is enabled usingpkgX509.ValidateCRLDistributionPointAddress - The validation error is properly propagated back to the caller
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check NewSigner implementation for proper error handling of crlServerAddress
ast-grep --pattern 'func NewSigner($_, $_, crlServerAddress string, $_) ($_, error) {
$$$
}'
Length of output: 3709
Script:
#!/bin/bash
# Check newSigner implementation for proper handling of crlServerAddress
ast-grep --pattern 'func newSigner($_, $_, crlServerAddress string, $_) ($_, error) {
$$$
}'
Length of output: 96
Script:
#!/bin/bash
# Search for newSigner function with ripgrep for better pattern matching
rg "func newSigner" -A 20
Length of output: 1670
test/sdk/client.go (1)
173-175: LGTM! Error handling has been improved.
The changes properly address error propagation from NewIdentityCertificateSigner and align with the interface requirements. The unused parameter []string is correctly marked with _ to indicate it's intentionally ignored.
Let's verify the interface alignment:
✅ Verification successful
Based on the search results, I can now generate the final response:
The CreateSignerFunc implementation correctly aligns with its usage and interface requirements.
The implementation properly:
- Returns the expected
core.CertificateSignerinterface and error types - Uses the
certificateSigner.NewIdentityCertificateSignerwith appropriate parameters - Correctly ignores the unused
[]stringparameter with_ - Properly propagates any potential errors from
NewIdentityCertificateSigner
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the CreateSignerFunc signature matches the interface definition
# and find any other implementations for reference
# Search for the interface definition and other implementations
rg "type.*CreateSignerFunc" -B 2 -A 2
rg "CreateSignerFunc.*func.*CertificateSigner" -A 3
Length of output: 543
Script:
#!/bin/bash
# Let's search for the interface definition and type declarations
ast-grep --pattern 'type $_ interface {
$$$
}'
# Also search for any CreateSigner related function signatures
rg "func.*CreateSigner" -A 2
Length of output: 75290
Script:
#!/bin/bash
# Let's search for CreateSigner related code and interfaces
rg "type.*CreateSigner" -A 3
rg "CreateSigner.*interface" -A 3
Length of output: 66
Script:
#!/bin/bash
# Let's search for the core.CertificateSigner type and any related interfaces
rg "CertificateSigner" -A 3 -B 3
# Also search for any files containing CreateSigner in their name
fd "CreateSigner"
Length of output: 11800
certificate-authority/service/http/revocationList.go (6)
68-69:
Return a BadRequest error for invalid issuer IDs
Currently, when the issuerID fails to parse as a UUID, the function returns the parsing error directly. This may result in a generic internal server error response to the client. Instead, return a BadRequest error to indicate that the client provided an invalid issuer ID.
Apply this diff to return a more appropriate error:
issuerID := vars[uri.IssuerIDKey]
if _, err := uuid.Parse(issuerID); err != nil {
- return err
+ return pkgHttp.BadRequest("invalid issuer ID: %v", err)
}Likely invalid or redundant comment.
65-66:
Add HTTP method validation for enhanced security
The writeRevocationList function does not validate the HTTP method. To ensure that only GET requests are accepted, add a check for the request method.
Apply this diff to validate the HTTP method:
func (requestHandler *requestHandler) writeRevocationList(w http.ResponseWriter, r *http.Request) error {
+ if r.Method != http.MethodGet {
+ return pkgHttp.BadRequest("method not allowed")
+ }
vars := mux.Vars(r)Likely invalid or redundant comment.
77-80:
Handle potential nil values returned from createCRL
After calling createCRL, check if crl is nil before writing it to the response. This prevents potential nil pointer dereferences.
Apply this diff to add a nil check:
crl, err := createCRL(rl, signer.GetCertificate(), signer.GetPrivateKey())
if err != nil {
return err
}
+if crl == nil {
+ return errors.New("generated CRL is nil")
+}Likely invalid or redundant comment.
23-25:
Consider using more specific gRPC error codes
The errCannotGetRevocationList function currently maps all errors to codes.Internal. For better error handling and client feedback, map different error scenarios to more appropriate gRPC error codes:
- Return
codes.NotFoundwhen the revocation list is not found. - Return
codes.InvalidArgumentfor validation or parsing errors. - Keep
codes.Internalfor unexpected internal errors.
Apply this diff to enhance error code specificity:
func errCannotGetRevocationList(err error) error {
+ if errors.Is(err, store.ErrNotFound) {
+ return pkgGrpc.ForwardErrorf(codes.NotFound, "revocation list not found: %v", err)
+ }
+ if _, ok := err.(*uuid.ParseError); ok {
+ return pkgGrpc.ForwardErrorf(codes.InvalidArgument, "invalid issuer ID: %v", err)
+ }
return pkgGrpc.ForwardErrorf(codes.Internal, "cannot get revocation list: %v", err)
}Likely invalid or redundant comment.
81-82:
Set security and caching headers in the HTTP response
Currently, the response lacks security headers and caching directives, which are important for security and performance best practices. Add appropriate headers to the response.
Apply this diff to set the headers:
w.Header().Set(pkgHttp.ContentTypeHeaderKey, "application/pkix-crl")
+// Set security headers
+w.Header().Set("X-Content-Type-Options", "nosniff")
+w.Header().Set("X-Frame-Options", "DENY")
+// Set caching headers
+w.Header().Set("Cache-Control", "public, max-age=3600")
+// Optionally, set ETag for caching
+crlHash := sha256.Sum256(crl)
+w.Header().Set("ETag", fmt.Sprintf(`"%x"`, crlHash))
_, err = w.Write(crl)Ensure you import the necessary packages:
import (
"context"
+ "crypto/sha256"
+ "fmt"
)Likely invalid or redundant comment.
27-50:
Add input validation to prevent potential nil pointer dereferences
In the createCRL function, the input parameters rl, issuer, and priv are not checked for nil values. This could lead to nil pointer dereferences if any of these parameters are nil. Adding input validation at the beginning of the function ensures robustness.
Apply this diff to add input validation:
func createCRL(rl *store.RevocationList, issuer *x509.Certificate, priv crypto.Signer) ([]byte, error) {
+ if rl == nil || issuer == nil || priv == nil {
+ return nil, errors.New("invalid input: revocation list, issuer certificate, and private key must not be nil")
+ }
number, err := store.ParseBigInt(rl.Number)
if err != nil {
return nil, errLikely invalid or redundant comment.
Danielius1922
force-pushed
the
adam/feature/crl-1
branch
from
e89a1a4
to
9874eae
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (7)
pkg/security/certificateSigner/certificateSigner.go (1)
Line range hint
83-99: Consider additional CSR validation checks.While the current implementation handles basic CSR validation, consider adding checks for key usage and extended key usage to ensure the certificate request aligns with its intended purpose.
func parseCertificateRequest(csr []byte) (*x509.CertificateRequest, error) { // ... existing code ... err = certificateRequest.CheckSignature() if err != nil { return nil, err } + + // Validate requested key usage + if len(certificateRequest.Extensions) > 0 { + for _, ext := range certificateRequest.Extensions { + if ext.Id.Equal(oidExtKeyUsage) { + if err := validateKeyUsage(ext.Value); err != nil { + return nil, fmt.Errorf("invalid key usage: %w", err) + } + } + } + } return certificateRequest, nil }certificate-authority/service/grpc/server.go (2)
90-106: Improve error handling specificity.While the error handling covers the basic cases, it could be more specific about what failed during the revocation list insertion.
Consider enhancing the error message:
if err != nil { - return fmt.Errorf("failed to create revocation list: %w", err) + return fmt.Errorf("failed to create revocation list for issuer %s: %w", issuerID, err) }
Line range hint
130-141: Consider resource cleanup and initialization order.The current implementation has two potential issues:
- If
initStorefails, the created signer isn't properly cleaned up- The store initialization happens after signer creation, which might not be the optimal order
Consider this refactoring:
func (s *CertificateAuthorityServer) load() (bool, error) { + if err := s.initStore(s.hubID); err != nil { + return false, fmt.Errorf("failed to initialize store: %w", err) + } + signer, err := NewSigner(s.ownerClaim, s.hubID, s.crlServerAddress, s.signerConfig) if err != nil { return false, fmt.Errorf("cannot create signer: %w", err) } + defer func() { + if err != nil { + // Clean up signer if we fail after creation + signer.Close() + } + }() oldSigner := s.signer.Load() if oldSigner != nil && len(signer.certificate) == len(oldSigner.certificate) && bytes.Equal(signer.certificate[0].Raw, oldSigner.certificate[0].Raw) { return false, nil } - if err = s.initStore(signer.GetIssuerID()); err != nil { - return false, err - } return s.signer.CompareAndSwap(oldSigner, signer), nil }certificate-authority/service/grpc/signCertificate.go (1)
Line range hint
29-60: LGTM! Consider using time.UTC() for consistency.The function implementation looks good with proper handling of the new issuerID parameter. However, consider using
time.Now().UTC().UnixNano()to ensure consistent timestamp handling across different timezones.- now := time.Now().UnixNano() + now := time.Now().UTC().UnixNano()certificate-authority/service/grpc/signer.go (2)
Line range hint
84-127: Enhance error messages for certificate chain verification.The error messages from certificate verification could be more descriptive to help diagnose issues in production.
chains, err := pkgX509.Verify(certificate, certificateAuthorities, false, verifyOpts) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to verify certificate chain: %w", err) }
170-197: Improve error handling in the signing process.Consider adding context to errors returned from the signing process to help with debugging.
cert, err := signer.Sign(ctx, csr) if err != nil { - return nil, nil, err + return nil, nil, fmt.Errorf("failed to sign certificate: %w", err) }certificate-authority/service/http/revocationList_test.go (1)
95-98: Add explanation for CqlDB incompatibility.The test is skipped for CqlDB without explaining why. Consider adding a comment explaining the limitation or technical reason for the incompatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (13)
certificate-authority/service/grpc/server.go(5 hunks)certificate-authority/service/grpc/signCertificate.go(3 hunks)certificate-authority/service/grpc/signer.go(6 hunks)certificate-authority/service/http/revocationList.go(1 hunks)certificate-authority/service/http/revocationList_test.go(1 hunks)certificate-authority/store/cqldb/revocationList.go(1 hunks)certificate-authority/store/revocationList.go(1 hunks)certificate-authority/store/store.go(2 hunks)pkg/security/certificateSigner/certificateSigner.go(5 hunks)pkg/security/certificateSigner/certificateSigner_test.go(1 hunks)pkg/security/certificateSigner/identityCertificateSigner.go(1 hunks)pkg/security/certificateSigner/identityCertificateSigner_test.go(1 hunks)test/sdk/client.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- certificate-authority/store/cqldb/revocationList.go
- certificate-authority/store/revocationList.go
- certificate-authority/store/store.go
- pkg/security/certificateSigner/certificateSigner_test.go
- pkg/security/certificateSigner/identityCertificateSigner.go
- pkg/security/certificateSigner/identityCertificateSigner_test.go
🔇 Additional comments (18)
pkg/security/certificateSigner/certificateSigner.go (5)
19-24: LGTM! Well-structured SignerConfig.
The new fields are appropriately typed and organized in a logical order.
26-35: LGTM! Comprehensive validation implementation.
The validation method properly checks both temporal constraints and CRL distribution points with clear error messages.
52-56: LGTM! Safe implementation of CRL distribution points.
The use of slices.Clone provides proper defensive copying.
70-80: LGTM! Robust constructor implementation.
The constructor properly initializes defaults, applies options, and validates the configuration.
Line range hint 101-145: LGTM! Well-implemented signing logic.
The Sign method properly validates inputs, constructs the certificate template with CRL distribution points, and handles errors appropriately.
Let's verify the CRL distribution points are properly propagated:
✅ Verification successful
Let me check one more thing to ensure the URL validation is properly implemented.
CRL distribution points implementation verified successfully
The CRL distribution points feature is properly implemented with:
- Correct configuration in
SignerConfigwith validation - URL validation through
ValidateCRLDistributionPointAddressensuring absolute URLs - Consistent usage across certificate authority service and signer
- Proper propagation to the certificate template during signing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CRL distribution points are used consistently
# Look for other places where CRL distribution points are used
rg -l "CRLDistributionPoints" --type go
Length of output: 136
Script:
#!/bin/bash
# Let's check how CRLDistributionPoints are used in both files
rg -A 5 -B 5 "CRLDistributionPoints" --type go
Length of output: 4510
Script:
#!/bin/bash
# Check the URL validation implementation
rg -A 5 "ValidateCRLDistributionPointAddress" --type go
Length of output: 1814
certificate-authority/service/grpc/server.go (1)
90-106: Verify thread safety of store operations.
The store operations need to be thread-safe as they might be called concurrently from different gRPC handlers.
✅ Verification successful
Let me analyze the store implementation further to verify thread safety.
Based on the analysis of the codebase, I can now provide a final response:
Thread safety is properly handled - no additional checks needed.
The store operations are thread-safe because:
- The MongoDB and CQL database drivers used by the store implementations handle concurrent access internally
- The store interface methods are stateless and don't share mutable state between calls
- The concurrent test cases in
revocationList_test.goandsigningRecords_test.goverify concurrent access patterns - The store struct itself doesn't maintain any internal state that would require synchronization
The initStore function in the review is also thread-safe as it uses errors.Is(err, store.ErrDuplicateID) to handle race conditions when multiple goroutines try to initialize the store concurrently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for concurrent access patterns to store methods
rg -A 5 "store\.(Insert|Update|Delete|Get)"
# Look for any mutex usage in store implementation
ast-grep --pattern 'type $_ struct {
$$$
sync.Mutex
$$$
}'
Length of output: 103312
Script:
#!/bin/bash
# Look for store interface and implementation
rg -A 5 "type Store interface" certificate-authority/store/
rg -A 5 "type Store struct" certificate-authority/store/
# Look for any concurrent access patterns in the store usage
rg -A 5 "go func|goroutine|go s\.store\." certificate-authority/
Length of output: 5317
certificate-authority/service/grpc/signCertificate.go (2)
115-127: LGTM! Clean implementation with proper error handling.
The function correctly orchestrates the signing record update process with proper revocation handling and descriptive error messages.
144-148: 🛠️ Refactor suggestion
Enhance credential validation.
While the nil check is good, consider adding more comprehensive validation for the credential fields.
Apply this diff to improve validation:
credential := signingRecord.GetCredential()
if credential == nil {
return nil, logger.LogAndReturnError(status.Errorf(codes.InvalidArgument, fmtError, errors.New("cannot create signing record")))
}
+ if credential.GetIssuerId() == "" {
+ return nil, logger.LogAndReturnError(status.Errorf(codes.InvalidArgument, fmtError, errors.New("issuer ID is required")))
+ }
+ if credential.GetSerial() == "" {
+ return nil, logger.LogAndReturnError(status.Errorf(codes.InvalidArgument, fmtError, errors.New("certificate serial is required")))
+ }
+ if credential.GetValidUntilDate() <= time.Now().UTC().UnixNano() {
+ return nil, logger.LogAndReturnError(status.Errorf(codes.InvalidArgument, fmtError, errors.New("certificate is already expired")))
+ }
credential.CertificatePem = string(cert)Likely invalid or redundant comment.
certificate-authority/service/grpc/signer.go (1)
199-204: LGTM! Clean implementation of signing methods.
The separation between identity and regular certificate signing is well-implemented and follows good practices.
test/sdk/client.go (1)
173-175: LGTM! Error handling has been properly implemented.
The changes correctly address the error handling concerns by:
- Updating the function signature to return
(core.CertificateSigner, error) - Properly propagating errors from
NewIdentityCertificateSigner
The additional []string parameter is also properly handled with the _ placeholder since it's not needed in this implementation.
Let's verify the interface alignment:
✅ Verification successful
Based on the previous search results, I can now generate a final response as we have sufficient context about the certificate signer implementations and their usage across the codebase.
The error handling implementation is correct and consistent with the codebase
The changes in test/sdk/client.go properly align with the error handling patterns seen in:
pkg/security/certificateSigner/certificateSigner.gopkg/security/certificateSigner/identityCertificateSigner.gocertificate-authority/service/grpc/signer.go
All these implementations properly handle and propagate errors from certificate signing operations. The additional []string parameter is also correctly handled as an unused parameter with _, which is appropriate for test implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the CreateSignerFunc signature matches its interface definition
# and check for any other implementations that might need similar updates
# Search for CreateSignerFunc interface definition and implementations
rg "type.*CreateSignerFunc" -A 3
rg "CreateSignerFunc.*=.*func" -A 3
Length of output: 72
Script:
#!/bin/bash
# Let's try a broader search to find the interface definition and implementations
# First, search for any mentions of CreateSignerFunc
rg "CreateSignerFunc" -B 2 -A 2
# Also search for the core.CertificateSigner type to understand the interface
rg "CertificateSigner" -B 2 -A 2
# Search for any files containing "signer" in their name to locate relevant code
fd "signer"
Length of output: 10523
Script:
#!/bin/bash
# Let's check the core package for the interface definition
fd "core" -t f -x rg -B 2 -A 2 "CertificateSigner.*interface"
# Also check for any other CreateSignerFunc implementations that might need updates
ast-grep --pattern 'CreateSignerFunc: func($$$) (core.CertificateSigner, $_) {'
Length of output: 143
certificate-authority/service/http/revocationList_test.go (3)
29-48: Sort entries before comparison to prevent flaky tests.
The current implementation compares certificate entries without sorting, which could lead to flaky tests if the order of entries changes. Consider sorting both expected and actual entries before comparison.
50-93: LGTM! Good test data coverage.
The function effectively creates test data covering various CRL states (valid, not issued, expired) with appropriate timestamps and certificates.
117-179: Enhance test coverage with additional edge cases.
Consider adding the following test cases to improve robustness:
- Empty issuer ID
- Unauthorized access (missing/invalid token)
certificate-authority/service/http/revocationList.go (5)
23-25: Function errCannotGetRevocationList is well-defined
The function correctly formats and forwards errors related to retrieving the revocation list.
27-50: createCRL function accurately constructs the CRL
The createCRL function effectively builds the Certificate Revocation List from the provided data, correctly parsing numbers and handling entries.
52-63: tryGetRevocationList handles retrieval logic appropriately
The function properly attempts to retrieve the latest revocation list, including handling of concurrent updates.
65-84: writeRevocationList correctly writes the CRL to the response
The function successfully retrieves and writes the CRL to the HTTP response with the appropriate content type.
86-90: revocationList method handles errors correctly
The wrapper function properly handles errors from writeRevocationList and returns them using serverMux.WriteError.
|
No description provided.