feat(starr): add mTLS client certificate support for all starr apps #556
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add TLSClientCert, TLSClientKey, and TLSCACert fields to StarrConfig - Implement certificate loading and validation in validateApp() - Support custom CA certificates for private certificate authorities - Maintain backward compatibility when mTLS fields are not configured - Add docker volume mount note for certificate directories - Update all example configurations with mTLS options
- Add static error ErrInvalidCA for wrapped error handling (err113) - Extract configureTLS helper function to reduce complexity (funlen) - Move embedded field to top of struct declaration - Align all struct tags in columns (tagalign) - Add nolint comment for unavoidable line length
davidnewhall
requested changes
Aug 30, 2025
| app, conf.URL, expandHomedir(conf.TLSCACert), err) | ||
| } | ||
|
|
||
| caCertPool := x509.NewCertPool() |
Collaborator
There was a problem hiding this comment.
Suggested change
| caCertPool := x509.NewCertPool() | |
| tlsConfig.RootCA = x509.NewCertPool() |
Can you just set this directly here? (I'm not sure if .RootCAs is an interface or not.)
| ValidSSL bool `json:"valid_ssl" toml:"valid_ssl" xml:"valid_ssl" yaml:"valid_ssl"` | ||
| Timeout cnfg.Duration `json:"timeout" toml:"timeout" xml:"timeout" yaml:"timeout"` | ||
| starr.Config | ||
|
|
Collaborator
There was a problem hiding this comment.
Extra whitespace you don't need here.
Comment on lines
+42
to
45
| TLSClientCert string `json:"tls_client_cert" toml:"tls_client_cert" xml:"tls_client_cert" yaml:"tls_client_cert"` //nolint:lll | ||
| TLSClientKey string `json:"tls_client_key" toml:"tls_client_key" xml:"tls_client_key" yaml:"tls_client_key"` | ||
| TLSCACert string `json:"tls_ca_cert" toml:"tls_ca_cert" xml:"tls_ca_cert" yaml:"tls_ca_cert"` | ||
| } |
Collaborator
There was a problem hiding this comment.
Can you consider this instead? You'll have to make a few other changes, but I think this will be a bit cleaner in the end.
Suggested change
| TLSClientCert string `json:"tls_client_cert" toml:"tls_client_cert" xml:"tls_client_cert" yaml:"tls_client_cert"` //nolint:lll | |
| TLSClientKey string `json:"tls_client_key" toml:"tls_client_key" xml:"tls_client_key" yaml:"tls_client_key"` | |
| TLSCACert string `json:"tls_ca_cert" toml:"tls_ca_cert" xml:"tls_ca_cert" yaml:"tls_ca_cert"` | |
| } | |
| TLS *ClientTLS `json:"tls" toml:"tls" xml:"tls" yaml:"tls"` | |
| } | |
| type ClientTLS struct { | |
| Cert string `json:"client_cert" toml:"client_cert" xml:"client_cert" yaml:"client_cert"` | |
| Key string `json:"client_key" toml:"client_key" xml:"client_key" yaml:"client_key"` | |
| CA string `json:"ca_cert" toml:"ca_cert" xml:"ca_cert" yaml:"ca_cert"` | |
| } |
| tlsConfig := &tls.Config{InsecureSkipVerify: !conf.ValidSSL} //nolint:gosec | ||
|
|
||
| // Add mTLS if certificates are configured | ||
| if conf.TLSClientCert != "" && conf.TLSClientKey != "" { |
Collaborator
There was a problem hiding this comment.
Suggested change
| if conf.TLSClientCert != "" && conf.TLSClientKey != "" { | |
| if conf.TLSClientCert != "" || conf.TLSClientKey != "" { |
I usually do an "or" on these two variables when dealing with SSL. In case a user sets one, they'll get an error. Instead of nothing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds mutual TLS (mTLS) client certificate authentication support to Unpackerr, enabling secure communication with Starr applications (Sonarr, Radarr, Lidarr, Readarr, Whisparr) that are protected behind reverse proxies requiring client certificates.
Testing
Configuration Example
TOML Configuration
Docker Compose
Implementation Details
The implementation modifies the HTTP transport configuration to include TLS client certificates when provided: