From 15b19ff7268da689933dd369862930afc98fcc8e Mon Sep 17 00:00:00 2001 From: Rahul Somasundaram Date: Wed, 5 Oct 2022 00:12:53 +0530 Subject: [PATCH] changes done as per review comments --- CHANGELOG.md | 13 +++++++++++-- internal/aghtls/aghtls.go | 5 ++++- internal/dnsforward/config.go | 6 ++++-- internal/home/home.go | 15 ++++++++++++++- internal/home/web.go | 24 ++++++++---------------- 5 files changed, 41 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 71b93339..35b3a259 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,9 +22,18 @@ and this project adheres to See also the [v0.107.16 GitHub milestone][ms-v0.107.15]. -[ms-v0.107.16]: https://github.com/AdguardTeam/AdGuardHome/milestone/52?closed=1 ---> +[ms-v0.107.16]: https://github.com/AdguardTeam/AdGuardHome/milestone/52?closed= +### Added + +- The new optional `tls.override_tls_ciphers` property list, which can be set in + the configuration file. It allows overriding TLS Ciphers that are used for + https listeners ([#4925]) + +[#4925]: https://github.com/AdguardTeam/AdGuardHome/issues/4925 + + +--> ## [v0.107.15] - 2022-10-03 diff --git a/internal/aghtls/aghtls.go b/internal/aghtls/aghtls.go index 209bef1d..4bd20a01 100644 --- a/internal/aghtls/aghtls.go +++ b/internal/aghtls/aghtls.go @@ -34,11 +34,14 @@ func SaferCipherSuites() (safe []uint16) { return safe } -func UserPreferredCipherSuites(ciphers []string) (userCiphers []uint16) { +// ParseCipherIDs returns a set of cipher suites with the cipher names provided +func ParseCipherIDs(ciphers []string) (userCiphers []uint16) { for _, s := range tls.CipherSuites() { if slices.Contains(ciphers, s.Name) { userCiphers = append(userCiphers, s.ID) log.Debug("user specified cipher : %s, ID : %d", s.Name, s.ID) + } else { + log.Error("unknown cipher : %s ", s) } } diff --git a/internal/dnsforward/config.go b/internal/dnsforward/config.go index 3a70bbeb..9994f1cb 100644 --- a/internal/dnsforward/config.go +++ b/internal/dnsforward/config.go @@ -166,8 +166,10 @@ type TLSConfig struct { // DNS names from certificate (SAN) or CN value from Subject dnsNames []string - // ciphers specified by user - TLSCiphers []string `yaml:"tls_ciphers" json:"-"` + // OverrideTLSCiphers holds the cipher names. If the slice is empty + // default set of ciphers are used for https listener, else this is + // considered. + OverrideTLSCiphers []string `yaml:"override_tls_ciphers" json:"-"` } // DNSCryptConfig is the DNSCrypt server configuration struct. diff --git a/internal/home/home.go b/internal/home/home.go index 0b325298..fad2c695 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -383,7 +383,7 @@ func initWeb(args options, clientBuildFS fs.FS) (web *Web, err error) { clientBetaFS: clientBetaFS, serveHTTP3: config.DNS.ServeHTTP3, - tlsCiphers: config.TLS.TLSCiphers, + tlsCiphers: getTLSCiphers(), } web = newWeb(&webConf) @@ -888,3 +888,16 @@ type jsonError struct { // Message is the error message, an opaque string. Message string `json:"message"` } + +// getTLSCiphers check for overriden tls ciphers, if the slice is +// empty, then default safe ciphers are used +func getTLSCiphers() []uint16 { + var cipher []uint16 + + if len(config.TLS.OverrideTLSCiphers) == 0 { + cipher = aghtls.SaferCipherSuites() + } else { + cipher = aghtls.ParseCipherIDs(config.TLS.OverrideTLSCiphers) + } + return cipher +} diff --git a/internal/home/web.go b/internal/home/web.go index d10e8dd5..5ece2819 100644 --- a/internal/home/web.go +++ b/internal/home/web.go @@ -11,7 +11,6 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/aghhttp" "github.com/AdguardTeam/AdGuardHome/internal/aghnet" - "github.com/AdguardTeam/AdGuardHome/internal/aghtls" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" "github.com/AdguardTeam/golibs/netutil" @@ -34,6 +33,10 @@ const ( ) type webConfig struct { + + // Ciphers that are used for https listener + tlsCiphers []uint16 + clientFS fs.FS clientBetaFS fs.FS @@ -57,9 +60,6 @@ type webConfig struct { firstRun bool serveHTTP3 bool - - // ciphers specified by user - tlsCiphers []string } // httpsServer contains the data for the HTTPS server. @@ -291,14 +291,6 @@ func (web *Web) tlsServerLoop() { web.httpsServer.cond.L.Unlock() - var cipher []uint16 - - if len(web.conf.tlsCiphers) == 0 { - cipher = aghtls.SaferCipherSuites() - } else { - cipher = aghtls.UserPreferredCipherSuites(web.conf.tlsCiphers) - } - addr := netutil.JoinHostPort(web.conf.BindHost.String(), web.conf.PortHTTPS) web.httpsServer.server = &http.Server{ ErrorLog: log.StdLog("web: https", log.DEBUG), @@ -306,7 +298,7 @@ func (web *Web) tlsServerLoop() { TLSConfig: &tls.Config{ Certificates: []tls.Certificate{web.httpsServer.cert}, RootCAs: Context.tlsRoots, - CipherSuites: cipher, + CipherSuites: web.conf.tlsCiphers, MinVersion: tls.VersionTLS12, }, Handler: withMiddlewares(Context.mux, limitRequestBody), @@ -318,7 +310,7 @@ func (web *Web) tlsServerLoop() { printHTTPAddresses(aghhttp.SchemeHTTPS) if web.conf.serveHTTP3 { - go web.mustStartHTTP3(addr, cipher) + go web.mustStartHTTP3(addr) } log.Debug("web: starting https server") @@ -330,7 +322,7 @@ func (web *Web) tlsServerLoop() { } } -func (web *Web) mustStartHTTP3(address string, ciphers []uint16) { +func (web *Web) mustStartHTTP3(address string) { defer log.OnPanic("web: http3") web.httpsServer.server3 = &http3.Server{ @@ -340,7 +332,7 @@ func (web *Web) mustStartHTTP3(address string, ciphers []uint16) { TLSConfig: &tls.Config{ Certificates: []tls.Certificate{web.httpsServer.cert}, RootCAs: Context.tlsRoots, - CipherSuites: ciphers, + CipherSuites: web.conf.tlsCiphers, MinVersion: tls.VersionTLS12, }, Handler: withMiddlewares(Context.mux, limitRequestBody),