Skip to content

fix: validation for certificates#7734

Merged
arkodg merged 11 commits intoenvoyproxy:mainfrom
kkk777-7:fix-cert-validate
Jan 25, 2026
Merged

fix: validation for certificates#7734
arkodg merged 11 commits intoenvoyproxy:mainfrom
kkk777-7:fix-cert-validate

Conversation

@kkk777-7
Copy link
Copy Markdown
Member

@kkk777-7 kkk777-7 commented Dec 14, 2025

What this PR does / why we need it:

Fix CA certificate validation.

Currently, CA certificate validation only check first block.
So, if user stores the CA bundle in Secret and first block is invalid (e.g. expired certificate), CA certificate validation fails.
This behavior causes #7608.

Fixed:
Support validating all block. and invalid block is exclude.
If no valid certificates exist, return ListenerError with InvalidCertificateRef condition.
If valid certificates exist and invalid certificates exist, return ListenerError with PartiallyInvalidCertificateRef condition.

Related PR

#7909

Tests

Which issue(s) this PR fixes:

Fixes #7608

Release Notes: Yes

Related and Reference Issue

kubernetes/ingress-nginx#4234 (comment)

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 14, 2025

Codecov Report

❌ Patch coverage is 82.08955% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.87%. Comparing base (b8d986c) to head (621b57d).

Files with missing lines Patch % Lines
internal/gatewayapi/tls.go 78.57% 10 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7734      +/-   ##
==========================================
+ Coverage   72.81%   72.87%   +0.06%     
==========================================
  Files         237      237              
  Lines       35623    35669      +46     
==========================================
+ Hits        25938    25993      +55     
+ Misses       7834     7830       -4     
+ Partials     1851     1846       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kkk777-7 kkk777-7 force-pushed the fix-cert-validate branch 2 times, most recently from 16c0013 to e589c4a Compare December 14, 2025 13:03
return nil, fmt.Errorf("%s/%s must contain valid %s and %s, unable to validate certificate in %s: %w", secret.Namespace, secret.Name, corev1.TLSCertKey, corev1.TLSPrivateKeyKey, corev1.TLSCertKey, err)
}

certBlock, _ := pem.Decode(certData)
certBlock, _ := pem.Decode(validData)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we handle multiple pems for certificate bundles?

@kkk777-7 kkk777-7 marked this pull request as ready for review December 14, 2025 15:28
@kkk777-7 kkk777-7 requested a review from a team as a code owner December 14, 2025 15:28
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
@arkodg arkodg added this to the v1.7.0-rc.1 Release milestone Dec 19, 2025
@arkodg arkodg mentioned this pull request Dec 21, 2025
// No errors but no valid PEM blocks found - PEM decoding failed
return nil, fmt.Errorf("unable to decode pem data for certificate")
}
return validData, nil
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not returning the expired cert errors here if there is a valid cert. IMO we should return the error and log it as warning if validData is non empty.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @rudrakhp @arkodg , I've updated the logic.

if no valid certs exist, returns custom error with InvalidCertificateRef Reason,
and if valid and invalid certs exist, returns custom error with PartiallyInvalidCertificateRef Reason.

@kkk777-7
Copy link
Copy Markdown
Member Author

@rudrakhp
thanks for your comments! and sorry for delay response.
currently, I'm handling another issue which is related to this. #7909

I’d appreciate it if I could address 7909 first, then I’ll handle this.

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
@netlify
Copy link
Copy Markdown

netlify bot commented Jan 17, 2026

Deploy Preview for cerulean-figolla-1f9435 ready!

Name Link
🔨 Latest commit 621b57d
🔍 Latest deploy log https://app.netlify.com/projects/cerulean-figolla-1f9435/deploys/697089b6eb65710008d2ded1
😎 Deploy Preview https://deploy-preview-7734--cerulean-figolla-1f9435.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
@kkk777-7
Copy link
Copy Markdown
Member Author

hi @rudrakhp , @arkodg , @zhaohuabing , please review it when you have a moment.

arkodg
arkodg previously approved these changes Jan 20, 2026
@arkodg arkodg requested review from a team and rudrakhp January 20, 2026 00:39
zirain
zirain previously approved these changes Jan 21, 2026
@zirain
Copy link
Copy Markdown
Member

zirain commented Jan 21, 2026

@kkk777-7 can you fix the conflict?

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
@kkk777-7
Copy link
Copy Markdown
Member Author

/retest

zhaohuabing
zhaohuabing previously approved these changes Jan 22, 2026
Signed-off-by: Arko Dasgupta <arkodg@users.noreply.github.com>
@arkodg arkodg merged commit c52327a into envoyproxy:main Jan 25, 2026
SadmiB pushed a commit to SadmiB/gateway that referenced this pull request Jan 30, 2026
* fix: validation for certificates

Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com>
Signed-off-by: Sadmi Bouhafs <sadmibouhafs@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ClientTrafficPolicy: Single expired client certificate CA in caCertificateRefs breaks gateway

5 participants