-
Notifications
You must be signed in to change notification settings - Fork 145
feat(tls): allow configuring TLS versions and cipher suites #575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Expose configuration options for TLS MinVersion, MaxVersion, and CipherSuites backed by crypto/tls. This allows LiveKit to interoperate with providers that require non-default or legacy TLS configurations.
dennwc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few minor adjustments.
pkg/config/config.go
Outdated
| Certs []TLSCert `yaml:"certs"` | ||
| KeyLog string `yaml:"key_log"` | ||
|
|
||
| MinVersion string `yaml:"min_version"` // min TLS version, accepts: "1.0", "1.1", "1.2", "1.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Values like 1.0 are dangerous in YAML, since they will be interpreted as a number by default. The user must then know to set it to "1.0" (a string).
So I'd propose we settle on accepting just tls1.0 and/or TLS1.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks. I’ve updated the code to accept only TLS1.0 and tls1.0 to avoid YAML numeric parsing issues.
pkg/sip/tls.go
Outdated
|
|
||
| // ParseCipherSuites parses cipher suite names to uint16 IDs. | ||
| // Logs a warning for each insecure cipher suite configured. | ||
| func ParseCipherSuites(suites []string, log logger.Logger) ([]uint16, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func ParseCipherSuites(suites []string, log logger.Logger) ([]uint16, error) { | |
| func ParseCipherSuites(log logger.Logger, suites []string) ([]uint16, error) { |
pkg/sip/tls.go
Outdated
|
|
||
| // ParseCipherSuites parses cipher suite names to uint16 IDs. | ||
| // Logs a warning for each insecure cipher suite configured. | ||
| func ParseCipherSuites(suites []string, log logger.Logger) ([]uint16, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func ParseCipherSuites(suites []string, log logger.Logger) ([]uint16, error) { | |
| func parseCipherSuites(suites []string, log logger.Logger) ([]uint16, error) { |
Can be unexported, since these are only used it this package.
pkg/sip/tls.go
Outdated
|
|
||
| // ParseTLSVersion parses a TLS version string to its uint16 constant. | ||
| // Accepts formats: "1.0", "1.1", "1.2", "1.3" or "TLS 1.0", "TLS 1.1", "TLS 1.2", "TLS 1.3". | ||
| func ParseTLSVersion(version string) (uint16, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func ParseTLSVersion(version string) (uint16, error) { | |
| func parseTLSVersion(version string) (uint16, error) { |
Same here - can be unexported.
731d552 to
3654345
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #575 +/- ##
==========================================
- Coverage 65.25% 64.61% -0.64%
==========================================
Files 51 34 -17
Lines 6588 6478 -110
==========================================
- Hits 4299 4186 -113
+ Misses 1915 1875 -40
- Partials 374 417 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Expose configuration options for TLS MinVersion, MaxVersion, and CipherSuites backed by crypto/tls. This allows LiveKit to interoperate with providers that require non-default or legacy TLS configurations.