fix: deduplicate CA certificates in ClientTrafficPolicy mTLS#8909
fix: deduplicate CA certificates in ClientTrafficPolicy mTLS#8909yuehaii wants to merge 10 commits into
Conversation
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8909 +/- ##
==========================================
+ Coverage 74.75% 74.76% +0.01%
==========================================
Files 252 252
Lines 40567 40590 +23
==========================================
+ Hits 30326 30349 +23
+ Misses 8169 8167 -2
- Partials 2072 2074 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
Signed-off-by: Hai <20416005+yuehaii@users.noreply.github.com>
|
For the CI checking of two conformance test and three e2e test failures. Those failures should have no relation with the fixing code. The fixing code just de-duplicate the cert.
The cause of those failure should be environmental related, not code caused issue
|
|
/retest |
@guydc, thanks for retesting. I checked the ci failure. There is only one TestE2E/RateLimitCIDRInvertMatchAlwaysEnforce/rate_limit_all_IPs_except_CIDR case failure this time.
Below are the test failure related code. The e2e server didn't response caused failure, and has no relation with current fix. Maybe we can bypass this CI failure.
|
| } | ||
| irCACert.Certificate = append(irCACert.Certificate, validCaCertBytes...) | ||
| } | ||
| irCACert.Certificate = deduplicatePEMCerts(irCACert.Certificate) |
There was a problem hiding this comment.
- can some info be added to the PR why we should dedup instead of reject
- also if we choose to dedup, can we avoid a proactive append instead ?
There was a problem hiding this comment.
openssl works well with same (CA) cert occurring more than once. I suppose envoy gw should also support such scenario for tolerance.
https://security.stackexchange.com/questions/62055/how-to-delete-duplicates-in-ca-bundle-certificate-file
Its a good suggestion. I will do de-dup during append process
There was a problem hiding this comment.
hi @arkodg , good day. may I know whether you have more questions?
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Signed-off-by: Hai <20416005+yuehaii@users.noreply.github.com>
| tls: | ||
| alpnProtocols: null | ||
| caCertificate: | ||
| certificate: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUR3VENDQXFtZ0F3SUJBZ0lVWTNmMFI2SExoVGNJU2FIaURhUkF0d1dHMDJrd0RRWUpLb1pJaHZjTkFRRUwKQlFBd2J6RUxNQWtHQTFVRUJoTUNWVk14Q3pBSkJnTlZCQWdNQWxaQk1SRXdEd1lEVlFRSERBaFRiMjFsUTJsMAplVEVUTUJFR0ExVUVDZ3dLUlc1MmIzbFFjbTk0ZVRFUU1BNEdBMVVFQ3d3SFIyRjBaWGRoZVRFWk1CY0dBMVVFCkF3d1FiWFJzY3k1bGVHRnRjR3hsTG1OdmJUQWdGdzB5TlRFeU1UTXhOREl3TXpGYUdBOHlNVEkxTVRFeE9URTAKTWpBek1Wb3diekVMTUFrR0ExVUVCaE1DVlZNeEN6QUpCZ05WQkFnTUFsWkJNUkV3RHdZRFZRUUhEQWhUYjIxbApRMmwwZVRFVE1CRUdBMVVFQ2d3S1JXNTJiM2xRY205NGVURVFNQTRHQTFVRUN3d0hSMkYwWlhkaGVURVpNQmNHCkExVUVBd3dRYlhSc2N5NWxlR0Z0Y0d4bExtTnZiVENDQVNJd0RRWUpLb1pJaHZjTkFRRUJCUUFEZ2dFUEFEQ0MKQVFvQ2dnRUJBT3Jxc21LS20xd1NZNlF2dm9vQkY1dGZVanByYWNwb3lERTVJVGVxMVcyeHVMRHBzWG9CTTZJVApzSG5LSWM3a3gzcEZmTk5YZG5TVE1PYU5CNEoycEIvUG5EdjhMK3FWN2dPYS91bGdjMnpJUVpCaVFtWEt0WTNFClNHL0ZQZmM1YmZ0bC9KcktHaExkaTdSNm40SkxIbGw0OVgyZ0xZc2hvM3ZNTXprQXJRUTVpaVVCRDlnSE1GVU4KV1loOTRONnQyQUxxY3A4cTZQZ2Y5R0Y5Z2Y2OGMzSWtnNlBQL3FTR2dQTTRMWHZQYlgyM0ZqUjBPdzcyTE8ybwphbDdTMVNwWFM5MGJyV2VXWThwMFBocVd1QnBSeWl4T1JFcWltY1BndXdGTisyYi9KZVJpeFlldTJjZisrYXU5ClhVWkFyYmc2UDJsNDNjSC9EZUNCbWdtTU9lVjExY0VDQXdFQUFhTlRNRkV3SFFZRFZSME9CQllFRksvTTVyK0gKMVNQdFhlOHBzd2srcmh3T1JFUW5NQjhHQTFVZEl3UVlNQmFBRksvTTVyK0gxU1B0WGU4cHN3aytyaHdPUkVRbgpNQThHQTFVZEV3RUIvd1FGTUFNQkFmOHdEUVlKS29aSWh2Y05BUUVMQlFBRGdnRUJBRnh5NmNpNVl3eVhndDcrCisxdjF0LzVsSS9vTytGNVZROWxCY29HK3UzRDduTGFtdkpEdFl0TkdxbFFZUVEwYXpXeDFqaEpjTzNQMnhGcUIKdTNJUU5GQ3VrdlZNVDFnd05UUDQwMFRQcmptOWFJblVYcE0wRUZtbXp0R0o4akJWdkVmc0kxK0ZUWnR4ckdzSQpBeGZpM1JzYjJISWZCVStzOEFpRTdOb2lVSFlJWi9pL21wNGswczlZVzNBQk9sT2duY0w1eEsyeWdhMGJGeW1HCjNmYVBrSEVURHVpaUVoMnR6cThuUThETWl1TVZwUEtzeEZJOWtVQ1FhcVZZZ2lKVjVrVjVZZGRyVHgyMC9wVWwKQ1RNVS9nd1pSUzVBMlJ1ZHQvWDZKMXNhRUFROXhHcTVNQ25wczFnazVXV09sRTJhZXZQYkRsTmV6RkVRcjJabAp4OXJKOTR3PQotLS0tLUVORCBDRVJUSUZJQ0FURS0tLS0tCi0tLS0tQkVHSU4gQ0VSVElGSUNBVEUtLS0tLQpNSUlEd1RDQ0FxbWdBd0lCQWdJVVkzZjBSNkhMaFRjSVNhSGlEYVJBdHdXRzAya3dEUVlKS29aSWh2Y05BUUVMCkJRQXdiekVMTUFrR0ExVUVCaE1DVlZNeEN6QUpCZ05WQkFnTUFsWkJNUkV3RHdZRFZRUUhEQWhUYjIxbFEybDAKZVRFVE1CRUdBMVVFQ2d3S1JXNTJiM2xRY205NGVURVFNQTRHQTFVRUN3d0hSMkYwWlhkaGVURVpNQmNHQTFVRQpBd3dRYlhSc2N5NWxlR0Z0Y0d4bExtTnZiVEFnRncweU5URXlNVE14TkRJd016RmFHQTh5TVRJMU1URXhPVEUwCk1qQXpNVm93YnpFTE1Ba0dBMVVFQmhNQ1ZWTXhDekFKQmdOVkJBZ01BbFpCTVJFd0R3WURWUVFIREFoVGIyMWwKUTJsMGVURVRNQkVHQTFVRUNnd0tSVzUyYjNsUWNtOTRlVEVRTUE0R0ExVUVDd3dIUjJGMFpYZGhlVEVaTUJjRwpBMVVFQXd3UWJYUnNjeTVsZUdGdGNHeGxMbU52YlRDQ0FTSXdEUVlKS29aSWh2Y05BUUVCQlFBRGdnRVBBRENDCkFRb0NnZ0VCQU9ycXNtS0ttMXdTWTZRdnZvb0JGNXRmVWpwcmFjcG95REU1SVRlcTFXMnh1TERwc1hvQk02SVQKc0huS0ljN2t4M3BGZk5OWGRuU1RNT2FOQjRKMnBCL1BuRHY4TCtxVjdnT2EvdWxnYzJ6SVFaQmlRbVhLdFkzRQpTRy9GUGZjNWJmdGwvSnJLR2hMZGk3UjZuNEpMSGxsNDlYMmdMWXNobzN2TU16a0FyUVE1aWlVQkQ5Z0hNRlVOCldZaDk0TjZ0MkFMcWNwOHE2UGdmOUdGOWdmNjhjM0lrZzZQUC9xU0dnUE00TFh2UGJYMjNGalIwT3c3MkxPMm8KYWw3UzFTcFhTOTBicldlV1k4cDBQaHFXdUJwUnlpeE9SRXFpbWNQZ3V3Rk4rMmIvSmVSaXhZZXUyY2YrK2F1OQpYVVpBcmJnNlAybDQzY0gvRGVDQm1nbU1PZVYxMWNFQ0F3RUFBYU5UTUZFd0hRWURWUjBPQkJZRUZLL001citICjFTUHRYZThwc3drK3Jod09SRVFuTUI4R0ExVWRJd1FZTUJhQUZLL001citIMVNQdFhlOHBzd2srcmh3T1JFUW4KTUE4R0ExVWRFd0VCL3dRRk1BTUJBZjh3RFFZSktvWklodmNOQVFFTEJRQURnZ0VCQUZ4eTZjaTVZd3lYZ3Q3KworMXYxdC81bEkvb08rRjVWUTlsQmNvRyt1M0Q3bkxhbXZKRHRZdE5HcWxRWVFRMGF6V3gxamhKY08zUDJ4RnFCCnUzSVFORkN1a3ZWTVQxZ3dOVFA0MDBUUHJqbTlhSW5VWHBNMEVGbW16dEdKOGpCVnZFZnNJMStGVFp0eHJHc0kKQXhmaTNSc2IySElmQlUrczhBaUU3Tm9pVUhZSVovaS9tcDRrMHM5WVczQUJPbE9nbmNMNXhLMnlnYTBiRnltRwozZmFQa0hFVER1aWlFaDJ0enE4blE4RE1pdU1WcFBLc3hGSTlrVUNRYXFWWWdpSlY1a1Y1WWRkclR4MjAvcFVsCkNUTVUvZ3daUlM1QTJSdWR0L1g2SjFzYUVBUTl4R3E1TUNucHMxZ2s1V1dPbEUyYWV2UGJEbE5lekZFUXIyWmwKeDlySjk0dz0KLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo= |
There was a problem hiding this comment.
I understand that this is a side-effect from your change where an existing test used the same cert twice. Can you fix the existing test input please to use different certs, so that the test continues to check the same thing essentially?
There was a problem hiding this comment.
thanks for the input, @guydc . I have updated the test with different certs.
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
Signed-off-by: Hai <20416005+yuehaii@users.noreply.github.com>
What type of PR is this?
fix: deduplicate CA certificates in ClientTrafficPolicy mTLS
What this PR does / why we need it:
when a single secret's ca.crt field contains a PEM bundle with duplicate certificate blocks, the translator concatenates all bytes without checking for duplicates.
the resulting xDS TLSCertificate resource carries a CA bundle with repeated CERTIFICATE PEM blocks. boring SSL's X509_STORE rejects duplicate X509_STORE_add calls for the same certificate and responds with a NACK. envoy discards the entire update and continues using the previous configuration.
Which issue(s) this PR fixes:
Fixes #8847
Test:
Release Notes: Yes