[TT-1570/TT-12588] extend certs endpoint with is_ca

jeffy-mathew commented 1 month ago

User description

Description

extend certs endpoint with is_ca flag fixes flay test TestCertificateHandlerTLS https://tyktech.atlassian.net/browse/TT-5261

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

[ ] I ensured that the documentation is up to date
[ ] I explained why this PR updates go.mod in detail with reasoning why it's required
[ ] I would like a code coverage CI quality gate exception and have explained why

PR Type

Enhancement, Tests

Description

Extended certificate structures (CertificateBasics and CertificateMeta) to include the IsCA field.
Updated functions ExtractCertificateBasics and ExtractCertificateMeta to populate the IsCA field.
Enhanced ValidateRequestCerts function to handle multiple peer certificates and improved error handling.
Added comprehensive tests for validating client certificates against a root certificate authority and for the IsCA field.
Updated Swagger API documentation to include the is_ca field in relevant components.

Changes walkthrough 📝

Relevant files

Enhancement

manager.go Extend certificate structures to include `IsCA` field. certs/manager.go Added `IsCA` field to `CertificateBasics` and `CertificateMeta` structs. Updated `ExtractCertificateBasics` and `ExtractCertificateMeta` functions to populate `IsCA` field.	+4/-0
helpers.go Enhance certificate validation logic in `ValidateRequestCerts`. internal/crypto/helpers.go Modified `ValidateRequestCerts` function to handle multiple peer certificates. Improved error handling for certificate validation.	+22/-17

Tests

cert_test.go

Add tests for certificate authority validation and `IsCA` field.

gateway/cert_test.go

Added tests for validating client certificates against a root
certificate authority.

Included tests for adding and listing certificates with IsCA field.

Added helper functions for generating root, server, and client
certificates.

+367/-3

Documentation

swagger.yml

Update Swagger API documentation to include `is_ca` field.

swagger.yml

Added is_ca field to CertificateBasics and CertificateMeta components.

+6/-0

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions[bot] commented 1 month ago

API Changes

--- prev.txt    2024-07-11 14:30:57.348101223 +0000
+++ current.txt 2024-07-11 14:30:54.036106152 +0000
@@ -4816,6 +4816,7 @@
    HasPrivateKey bool      `json:"has_private"`
    NotBefore     time.Time `json:"not_before"`
    NotAfter      time.Time `json:"not_after"`
+   IsCA          bool      `json:"is_ca"`
 }

 func ExtractCertificateBasics(cert *tls.Certificate, certID string) *CertificateBasics
@@ -4841,6 +4842,7 @@
    NotBefore     time.Time `json:"not_before,omitempty"`
    NotAfter      time.Time `json:"not_after,omitempty"`
    DNSNames      []string  `json:"dns_names,omitempty"`
+   IsCA          bool      `json:"is_ca"`
 }

 func ExtractCertificateMeta(cert *tls.Certificate, certID string) *CertificateMeta

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

**Possible Bug:** The logic in `ValidateRequestCerts` function in `internal/crypto/helpers.go` has been changed to loop through all peer certificates and match against a list of allowed certificates. This change might introduce issues if the peer presents multiple certificates and the first one is not the one expected to match. It would be safer to ensure that the correct certificate is being validated against the allowed list. **Data Consistency:** The addition of the `IsCA` field in various structures (`CertificateBasics`, `CertificateMeta`) needs to be thoroughly tested to ensure that it is correctly populated in all relevant API responses and internal logic where these structures are used.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a nil check for `cert.Leaf` to prevent potential runtime errors ___ To prevent potential nil pointer dereference, add a check to ensure `cert.Leaf` is not nil before accessing its properties in the `ExtractCertificateBasics` and `ExtractCertificateMeta` functions. [certs/manager.go [286]](https://github.com/TykTechnologies/tyk/pull/6406/files#diff-78e768b2719ac9f70038499f847de2843db20d8ca21a963ea63b82010d711039R286-R286) ```diff -IsCA: cert.Leaf.IsCA, +IsCA: cert.Leaf != nil && cert.Leaf.IsCA, ``` Suggestion importance[1-10]: 10 Why: Adding a nil check for `cert.Leaf` is crucial to prevent potential nil pointer dereference, which can cause runtime errors. This suggestion addresses a possible bug and improves the robustness of the code.	10
	Add nil checks for `r.TLS` and `r.TLS.PeerCertificates` to prevent runtime panics ___ It's recommended to check if `r.TLS` and `r.TLS.PeerCertificates` are not nil before accessing `r.TLS.PeerCertificates`. This prevents potential runtime panics due to nil pointer dereference. [internal/crypto/helpers.go [105]](https://github.com/TykTechnologies/tyk/pull/6406/files#diff-3d1fc755c46eaa99e9f1edf358b9e00842342ae6333902959d7a68b46d156829R105-R105) ```diff +if r.TLS == nil \|\| r.TLS.PeerCertificates == nil { + return errors.New("No TLS or PeerCertificates found") +} for _, peerCertificate := range r.TLS.PeerCertificates { certID := HexSHA256(peerCertificate.Raw) ... ``` Suggestion importance[1-10]: 10 Why: This suggestion addresses a potential runtime panic due to nil pointer dereference, which is a critical issue. Adding these nil checks improves the robustness of the code.	10
	Add nil checks for `cert.Leaf` and `cert.Leaf.Extensions` to prevent runtime panics ___ Consider handling the case where `cert.Leaf` or `cert.Leaf.Extensions` might be nil to avoid potential nil pointer dereference. [internal/crypto/helpers.go [116]](https://github.com/TykTechnologies/tyk/pull/6406/files#diff-3d1fc755c46eaa99e9f1edf358b9e00842342ae6333902959d7a68b46d156829R116-R116) ```diff -if string(cert.Leaf.Extensions[0].Value) == certID { +if cert.Leaf != nil && len(cert.Leaf.Extensions) > 0 && string(cert.Leaf.Extensions[0].Value) == certID { ... ``` Suggestion importance[1-10]: 10 Why: This suggestion prevents potential runtime panics by adding necessary nil checks, which is crucial for the stability and reliability of the code.	10
	Add error handling after URL parsing to ensure stability ___ Consider checking for errors after parsing the URL. This ensures that any issues with URL parsing are caught early, preventing potential runtime panics or misbehaviors in subsequent requests. [gateway/cert_test.go [1903-1904]](https://github.com/TykTechnologies/tyk/pull/6406/files#diff-481696cb18de8c3880e0ca21318fe00fd4eb89bc48994e43b7c34729ef4a7ee2R1903-R1904) ```diff u, err := url.Parse(ts.URL) +if err != nil { + t.Fatalf("Failed to parse URL: %v", err) +} ``` Suggestion importance[1-10]: 9 Why: Adding error handling after URL parsing is crucial for preventing potential runtime panics or misbehaviors, which enhances the robustness of the code.	9
Maintainability	Refactor repeated `IsCA` assignments into a common function to reduce duplication ___ Refactor the repeated code for setting `IsCA` in both `ExtractCertificateBasics` and `ExtractCertificateMeta` into a common function. This will improve maintainability and reduce code duplication. [certs/manager.go [286-312]](https://github.com/TykTechnologies/tyk/pull/6406/files#diff-78e768b2719ac9f70038499f847de2843db20d8ca21a963ea63b82010d711039R286-R312) ```diff -IsCA: cert.Leaf.IsCA, +IsCA: getIsCA(cert), ``` Suggestion importance[1-10]: 8 Why: Refactoring the repeated code for setting `IsCA` into a common function improves maintainability and reduces code duplication. This makes the codebase cleaner and easier to manage.	8
Enhancement	Use a unique serial number for certificates to enhance security ___ Instead of using a hardcoded value for the serial number, consider generating a random or unique serial number for each certificate to enhance security and comply with best practices. [gateway/cert_test.go [1924]](https://github.com/TykTechnologies/tyk/pull/6406/files#diff-481696cb18de8c3880e0ca21318fe00fd4eb89bc48994e43b7c34729ef4a7ee2R1924-R1924) ```diff -SerialNumber: big.NewInt(1), +SerialNumber: big.NewInt(rand.Int63()), // Ensure unique serial number ``` Suggestion importance[1-10]: 8 Why: Using a unique serial number for certificates is a best practice that enhances security and ensures compliance with standards.	8
	Add assertions for response content to verify API behavior ___ To improve the robustness of the test, consider adding assertions to check the contents of the response body in addition to the status code, ensuring that the API behaves as expected. [gateway/cert_test.go [1888-1893]](https://github.com/TykTechnologies/tyk/pull/6406/files#diff-481696cb18de8c3880e0ca21318fe00fd4eb89bc48994e43b7c34729ef4a7ee2R1888-R1893) ```diff -_, _ = ts.Run(t, test.TestCase{ +response, _ := ts.Run(t, test.TestCase{ Domain: "localhost", Client: validCertClient, Path: "/static-mtls", Code: http.StatusOK, }) +assert.Contains(t, response.Body, "expected content") ``` Suggestion importance[1-10]: 7 Why: Adding assertions for the response content improves the robustness of the test by ensuring that the API behaves as expected, not just returning the correct status code.	7
	Improve the error message clarity when no valid certificate is found ___ Refactor the error message to include the type of error for clarity when no valid certificate is found. [internal/crypto/helpers.go [126]](https://github.com/TykTechnologies/tyk/pull/6406/files#diff-3d1fc755c46eaa99e9f1edf358b9e00842342ae6333902959d7a68b46d156829R126-R126) ```diff -return errors.New("Certificate with SHA256 " + HexSHA256(r.TLS.PeerCertificates[0].Raw) + " not allowed") +return fmt.Errorf("Certificate validation failed: Certificate with SHA256 %s not allowed", HexSHA256(r.TLS.PeerCertificates[0].Raw)) ``` Suggestion importance[1-10]: 7 Why: The suggestion improves the clarity of the error message, making it more informative. While this is a good enhancement, it is not as critical as preventing runtime panics.	7
Documentation	Document the new `IsCA` field to clarify its purpose and usage ___ Ensure that the `IsCA` field is appropriately documented in the API or user documentation, explaining its role and impact on the certificate handling process, especially since it's a new addition to the data structure. [certs/manager.go [274]](https://github.com/TykTechnologies/tyk/pull/6406/files#diff-78e768b2719ac9f70038499f847de2843db20d8ca21a963ea63b82010d711039R274-R274) ```diff -IsCA bool `json:"is_ca"` +IsCA bool `json:"is_ca" // Indicates if the certificate is a Certificate Authority` ``` Suggestion importance[1-10]: 7 Why: Adding documentation for the new `IsCA` field is a good practice to clarify its purpose and usage. This improves code readability and helps other developers understand the role of the field.	7