Validate TLS config: return error if cert or key is missing #13134
Conversation
|
Could you add a test? |
|
Added test |
Co-authored-by: Damien Mathieu <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13134 +/- ##
==========================================
+ Coverage 91.28% 91.30% +0.01%
==========================================
Files 508 509 +1
Lines 28661 28747 +86
==========================================
+ Hits 26164 26248 +84
- Misses 1980 1986 +6
+ Partials 517 513 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This will need a changelog entry. And we should have another PR in contrib fixing the failing tests. |
|
Thks!! |
|
Hi! I've opened the contrib PR to fix the tests: |
|
open-telemetry/opentelemetry-collector-contrib/pull/40452 will fix the contrib tests. I am working on it |
gupta-nu
force-pushed
the
fix-13130-tls-validate
branch
from
b42cd7f to
4df2df8
Compare
|
Please add changelog entry |
| // If cert or key is provided, require both to be present | ||
| if certProvided != keyProvided { | ||
| return errors.New("TLS configuration must include certificate and key (CertFile/CertPem and KeyFile/KeyPem)") | ||
| } |
There was a problem hiding this comment.
I don't think this check fixes issue #13130. The issue was originally filed because we noticed the case where the TLS config is empty led to errors, but these errors only surface at connection time. (Admittedly, the issue description may not have been clear about this). But this check doesn't fail if all fields are empty.
Moreover, I believe this is identical to the first check in Config.loadCertificate (c.hasCert() != c.hasKey()), which is run at startup time (slightly later than config validation, but still good enough I would say).
If we want to move as many checks as possible to config validation time, looking at the code for Config.loadCertificate, I think the appropriate check would be c.hasCertFile() != c.hasCertPem() && c.hasKeyFile() != c.hasKeyPem() (ie. we have exactly one field setting a certificate, and exactly one field setting a key).
If we just want to fix the immediate issue, I think checking c.hasCert() || c.hasKey() would be enough.
And if we don't mind an error on startup rather than an error at config validation time, considering the Config.loadCertificate function already HAS a check for this, I think the fix with the smallest diff would be to simply add a proper error message for that case, and remove the if c.hasCert() || c.hasKey() test in Config.loadTLSConfig() which prevents the check from running.
Description
If TLS is configured without a certificate or key, it currently fails at runtime with a TLS handshake error. current change fixes #13130 by adding a validation check in configtls.Config.Validate() to ensure that TLS configurations include both a certificate and a key.
missing certs caused confusing runtime handshake errors. With this change, users get clear validation error earlier.
Fixes #13130