feat: present client certs for outbound SIP TLS mTLS (#530) - #776
feat: present client certs for outbound SIP TLS mTLS (#530)#776lixuanqun wants to merge 1 commit into
Conversation
Outbound SIP/TLS dials share the UserAgent tls.Config with the inbound listener, but Go's default client-certificate selection can return no cert when the peer's AcceptableCAs don't match our leaf. Set an explicit GetClientCertificate hook so configured identities are always presented when the trunk requires mTLS. Also allow optional tls.client_certs for a separate outbound identity, and document Redis TLS + SIP TLS/mTLS config keys in the README (redis.tls was already supported via livekit/protocol; see livekit#470). Fixes livekit#530 Fixes livekit#470 Co-authored-by: li xuanqun <793005378@qq.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| return func(*tls.CertificateRequestInfo) (*tls.Certificate, error) { | ||
| if len(certs) == 0 { | ||
| return nil, nil | ||
| } | ||
| return &certs[0], nil | ||
| } |
There was a problem hiding this comment.
🟡 Outbound TLS connection can crash when no client identity is configured
An empty result is handed back to Go's TLS client (return nil, nil at pkg/sip/tls.go:49) when no client identity is configured, so a call placed over an encrypted trunk that asks for a client certificate can crash the service instead of connecting without one.
Impact: If this path is ever reached, the process panics during the handshake instead of gracefully continuing without a client certificate.
crypto/tls requires GetClientCertificate to return a non-nil Certificate
Go's documentation for tls.Config.GetClientCertificate states the hook must return a non-nil *Certificate; to send no certificate it must return an empty &tls.Certificate{}. Internally the client does certMsg.certificate = *cert (TLS 1.3) / certMsg.certificates = chainToSend.Certificate (TLS 1.2), which nil-dereferences on a nil return. The default implementation returns new(Certificate) for exactly this reason.
Today pkg/sip/service.go:244-254 rejects a TLS config with zero certs, so clientCerts is always non-empty in production, making the branch effectively unreachable — but the helper is exported to the package and pkg/sip/tls_test.go:13-16 asserts the nil behaviour, locking in the contract violation for any future caller.
| return func(*tls.CertificateRequestInfo) (*tls.Certificate, error) { | |
| if len(certs) == 0 { | |
| return nil, nil | |
| } | |
| return &certs[0], nil | |
| } | |
| return func(*tls.CertificateRequestInfo) (*tls.Certificate, error) { | |
| if len(certs) == 0 { | |
| // crypto/tls requires a non-nil Certificate; an empty one means "send no cert". | |
| return &tls.Certificate{}, nil | |
| } | |
| return &certs[0], nil | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #776 +/- ##
==========================================
+ Coverage 65.25% 65.95% +0.69%
==========================================
Files 51 41 -10
Lines 6588 7962 +1374
==========================================
+ Hits 4299 5251 +952
- Misses 1915 2226 +311
- Partials 374 485 +111 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Outbound SIP/TLS dials share the UserAgent tls.Config with the inbound listener, but Go's default client-certificate selection can return no cert when the peer's AcceptableCAs don't match our leaf. Set an explicit GetClientCertificate hook so configured identities are always presented when the trunk requires mTLS.
Also allow optional tls.client_certs for a separate outbound identity, and document Redis TLS + SIP TLS/mTLS config keys in the README (redis.tls was already supported via livekit/protocol; see #470).
Fixes #530
Fixes #470