[TT-1570/TT-12587]mTLS: allow validating client certificates against root certificate authority

jeffy-mathew commented 1 month ago

User description

Description

mTLS: allow validating client certificates against root certificate authority

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] 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

Added comprehensive tests for validating client certificates against a root certificate authority in mutual TLS scenarios.
Implemented helper functions to generate root, server, and client certificates and keys for testing purposes.
Enhanced the ValidateRequestCerts function to handle multiple peer certificates and improved error handling.

Changes walkthrough 📝

Relevant files

Tests

cert_test.go

Add mTLS validation tests and certificate generation helpers

gateway/cert_test.go

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

Implemented helper functions to generate root, server, and client
certificates and keys.

Enhanced mutual TLS tests to include valid and invalid client
scenarios.

+328/-0

Enhancement

helpers.go

Enhance certificate validation logic in mTLS

internal/crypto/helpers.go

Modified ValidateRequestCerts to handle multiple peer certificates.

Improved error handling for certificate validation.

+22/-17

💡 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

no api changes detected

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

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

🧪 PR contains tests

🔒 Security concerns

- Sensitive Information Exposure:
The PR deals with handling TLS certificates and private keys, which are sensitive. It is crucial to ensure that these items are handled securely, not exposed in logs, and are properly secured in memory and during transmission. - Certificate Validation: The PR adds functionality for validating client certificates against a root certificate authority. It is essential to ensure that this validation is robust and does not allow invalid or revoked certificates to be accepted.

⚡ Key issues to review

**Possible Bug:** The PR introduces a significant amount of new code related to certificate handling and mTLS, which could potentially introduce bugs or security issues if not implemented correctly. Thorough review and testing are required to ensure that the implementation correctly handles certificate validation, error handling, and edge cases. **Performance Concern:** The PR includes operations such as generating RSA keys and certificates, which are computationally expensive. It's important to ensure that these operations do not negatively impact the performance of the system, especially under load.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a check for the presence of `Extensions` to prevent index out of range errors ___ Consider checking for the presence of `Extensions` before accessing `Extensions[0]` to avoid potential index out of range errors. This is important for robust error handling and to prevent runtime panics. [gateway/cert_test.go [116]](https://github.com/TykTechnologies/tyk/pull/6405/files#diff-481696cb18de8c3880e0ca21318fe00fd4eb89bc48994e43b7c34729ef4a7ee2R116-R116) ```diff -if string(cert.Leaf.Extensions[0].Value) == certID { +if len(cert.Leaf.Extensions) > 0 && string(cert.Leaf.Extensions[0].Value) == certID { ``` Suggestion importance[1-10]: 10 Why: This suggestion addresses a potential runtime panic by adding a necessary check for the presence of `Extensions`, which is crucial for robust error handling.	10
Best practice	Use `fmt.Errorf` for dynamic error messages instead of `errors.New` with string concatenation ___ Instead of concatenating strings to form the error message, use `fmt.Errorf` for better error handling and formatting. This approach allows for easier inclusion of variable content within the error message and is more consistent with Go best practices. [gateway/cert_test.go [126]](https://github.com/TykTechnologies/tyk/pull/6405/files#diff-481696cb18de8c3880e0ca21318fe00fd4eb89bc48994e43b7c34729ef4a7ee2R126-R126) ```diff -return errors.New("Certificate with SHA256 " + HexSHA256(r.TLS.PeerCertificates[0].Raw) + " not allowed") +return fmt.Errorf("Certificate with SHA256 %s not allowed", HexSHA256(r.TLS.PeerCertificates[0].Raw)) ``` Suggestion importance[1-10]: 8 Why: Using `fmt.Errorf` is a best practice in Go for dynamic error messages, improving readability and maintainability. This suggestion correctly identifies and addresses this improvement.	8
Maintainability	Refactor repeated certificate generation logic into a setup function for better test manageability ___ To improve the test's readability and maintainability, consider moving the repeated certificate generation logic (`generateRootCertAndKey`, `generateServerCertAndKeyChain`, `generateClientCertAndKeyChain`) into a setup function or using table-driven tests. This will reduce duplication and make the tests easier to manage. [gateway/cert_test.go [295-333]](https://github.com/TykTechnologies/tyk/pull/6405/files#diff-481696cb18de8c3880e0ca21318fe00fd4eb89bc48994e43b7c34729ef4a7ee2R295-R333) ```diff -rootCertPEM, rootKeyPEM, err := generateRootCertAndKey(t) -assert.NoError(t, err) -serverCertPEM, serverKeyPEM, err := generateServerCertAndKeyChain(t, rootCertPEM, rootKeyPEM) -assert.NoError(t, err) -clientCertPEM, clientKeyPEM, err := generateClientCertAndKeyChain(t, rootCertPEM, rootKeyPEM) -assert.NoError(t, err) +setupCertificates := func(t *testing.T) (rootCertPEM, rootKeyPEM, serverCertPEM, serverKeyPEM, clientCertPEM, clientKeyPEM []byte) { + rootCertPEM, rootKeyPEM, err := generateRootCertAndKey(t) + assert.NoError(t, err) + serverCertPEM, serverKeyPEM, err := generateServerCertAndKeyChain(t, rootCertPEM, rootKeyPEM) + assert.NoError(t, err) + clientCertPEM, clientKeyPEM, err := generateClientCertAndKeyChain(t, rootCertPEM, rootKeyPEM) + assert.NoError(t, err) + return rootCertPEM, rootKeyPEM, serverCertPEM, serverKeyPEM, clientCertPEM, clientKeyPEM +} ``` Suggestion importance[1-10]: 7 Why: Refactoring repeated logic into a setup function improves maintainability and readability, although it is not critical. This suggestion correctly identifies and addresses the improvement.	7
Maintainability	Isolate certificate matching logic into a separate function for clarity and maintainability ___ Refactor the loop that checks for a matching certificate to a separate function. This will improve code readability and maintainability by isolating specific logic into a dedicated function, making the main validation function cleaner and easier to understand. [internal/crypto/helpers.go [105-124]](https://github.com/TykTechnologies/tyk/pull/6405/files#diff-3d1fc755c46eaa99e9f1edf358b9e00842342ae6333902959d7a68b46d156829R105-R124) ```diff -for _, peerCertificate := range r.TLS.PeerCertificates { - certID := HexSHA256(peerCertificate.Raw) - for _, cert := range certs { - if cert == nil { - continue - } - if string(cert.Leaf.Extensions[0].Value) == certID { - if time.Now().After(cert.Leaf.NotAfter) { - return ErrCertExpired +func matchCertificate(peerCertificates []x509.Certificate, certs []tls.Certificate) error { + for _, peerCertificate := range peerCertificates { + certID := HexSHA256(peerCertificate.Raw) + for _, cert := range certs { + if cert == nil { + continue } - return nil + if string(cert.Leaf.Extensions[0].Value) == certID { + if time.Now().After(cert.Leaf.NotAfter) { + return ErrCertExpired + } + return nil + } } } + return errors.New("no matching certificate found") } ``` Suggestion importance[1-10]: 7 Why: Isolating the certificate matching logic into a separate function improves readability and maintainability, making the main function cleaner. This suggestion correctly identifies and addresses the improvement.	7