Skip to content

Commit

Permalink
all: remember the cert props
Browse files Browse the repository at this point in the history
  • Loading branch information
EugeneOne1 committed Nov 1, 2022
1 parent c5565a9 commit 9793820
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 61 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ and this project adheres to

### Changed

- DNS-over-TLS resolvers aren't returned anymore when the configured TLS
certificate contains no IP addresses ([#4927]).
- Responses with `SERVFAIL` code are now cached for at least 30 seconds.

### Deprecated
Expand All @@ -56,6 +58,7 @@ and this project adheres to
[#4898]: https://github.com/AdguardTeam/AdGuardHome/issues/4898
[#4916]: https://github.com/AdguardTeam/AdGuardHome/issues/4916
[#4925]: https://github.com/AdguardTeam/AdGuardHome/issues/4925
[#4927]: https://github.com/AdguardTeam/AdGuardHome/issues/4927
[#4942]: https://github.com/AdguardTeam/AdGuardHome/issues/4942
[#4986]: https://github.com/AdguardTeam/AdGuardHome/issues/4986
[#4990]: https://github.com/AdguardTeam/AdGuardHome/issues/4990
Expand Down
18 changes: 18 additions & 0 deletions internal/aghtls/aghtls.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ package aghtls

import (
"crypto/tls"
"crypto/x509"
"fmt"
"net/netip"

"github.com/AdguardTeam/golibs/log"
)
Expand Down Expand Up @@ -69,3 +71,19 @@ func SaferCipherSuites() (safe []uint16) {

return safe
}

// IsIPCertificate returns true if cert has at least a single IP address among
// its subjectAltNames.
func IsIPCertificate(cert *x509.Certificate) (ok bool) {
if len(cert.IPAddresses) > 0 {
return true
}

for _, name := range cert.DNSNames {
if _, err := netip.ParseAddr(name); err == nil {
return true
}
}

return false
}
28 changes: 19 additions & 9 deletions internal/dnsforward/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/AdguardTeam/AdGuardHome/internal/aghalg"
"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
"github.com/AdguardTeam/AdGuardHome/internal/aghtls"
"github.com/AdguardTeam/AdGuardHome/internal/filtering"
"github.com/AdguardTeam/dnsproxy/proxy"
"github.com/AdguardTeam/dnsproxy/upstream"
Expand Down Expand Up @@ -146,13 +147,12 @@ type FilteringConfig struct {

// TLSConfig is the TLS configuration for HTTPS, DNS-over-HTTPS, and DNS-over-TLS
type TLSConfig struct {
cert tls.Certificate

TLSListenAddrs []*net.TCPAddr `yaml:"-" json:"-"`
QUICListenAddrs []*net.UDPAddr `yaml:"-" json:"-"`
HTTPSListenAddrs []*net.TCPAddr `yaml:"-" json:"-"`

// Reject connection if the client uses server name (in SNI) that doesn't match the certificate
StrictSNICheck bool `yaml:"strict_sni_check" json:"-"`

// PEM-encoded certificates chain
CertificateChain string `yaml:"certificate_chain" json:"certificate_chain"`
// PEM-encoded private key
Expand All @@ -168,13 +168,20 @@ type TLSConfig struct {
// used for ClientID checking and Discovery of Designated Resolvers (DDR).
ServerName string `yaml:"-" json:"-"`

cert tls.Certificate
// DNS names from certificate (SAN) or CN value from Subject
dnsNames []string

// OverrideTLSCiphers, when set, contains the names of the cipher suites to
// use. If the slice is empty, the default safe suites are used.
OverrideTLSCiphers []string `yaml:"override_tls_ciphers,omitempty" json:"-"`

// StrictSNICheck controls if the connections with SNI mismatching the
// certificate's ones should be rejected.
StrictSNICheck bool `yaml:"strict_sni_check" json:"-"`

// hasIPAddrs is set during the certificate parsing and is true if the
// configured certificate contains at least a single IP address.
hasIPAddrs bool
}

// DNSCryptConfig is the DNSCrypt server configuration struct.
Expand Down Expand Up @@ -484,12 +491,15 @@ func (s *Server) prepareTLS(proxyConfig *proxy.Config) error {
return fmt.Errorf("failed to parse TLS keypair: %w", err)
}

var x *x509.Certificate
x, err = x509.ParseCertificate(s.conf.cert.Certificate[0])
if err != nil {
return fmt.Errorf("x509.ParseCertificate(): %w", err)
}

s.conf.hasIPAddrs = aghtls.IsIPCertificate(x)

if s.conf.StrictSNICheck {
var x *x509.Certificate
x, err = x509.ParseCertificate(s.conf.cert.Certificate[0])
if err != nil {
return fmt.Errorf("x509.ParseCertificate(): %w", err)
}
if len(x.DNSNames) != 0 {
s.conf.dnsNames = x.DNSNames
log.Debug("dns: using DNS names from certificate's SAN: %v", x.DNSNames)
Expand Down
44 changes: 22 additions & 22 deletions internal/dnsforward/dns.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,21 +259,13 @@ func (s *Server) onDHCPLeaseChanged(flags int) {
//
// See https://www.ietf.org/archive/id/draft-ietf-add-ddr-10.html.
func (s *Server) processDDRQuery(dctx *dnsContext) (rc resultCode) {
pctx := dctx.proxyCtx
q := pctx.Req.Question[0]

if !s.conf.HandleDDR {
return resultCodeSuccess
}

pctx := dctx.proxyCtx
q := pctx.Req.Question[0]
if q.Name == ddrHostFQDN {
if s.dnsProxy.TLSListenAddr == nil && s.conf.HTTPSListenAddrs == nil &&
s.dnsProxy.QUICListenAddr == nil || q.Qtype != dns.TypeSVCB {
pctx.Res = s.makeResponse(pctx.Req)

return resultCodeFinish
}

pctx.Res = s.makeDDRResponse(pctx.Req)

return resultCodeFinish
Expand All @@ -291,6 +283,10 @@ func (s *Server) processDDRQuery(dctx *dnsContext) (rc resultCode) {
// [draft standard]: https://www.ietf.org/archive/id/draft-ietf-add-ddr-10.html.
func (s *Server) makeDDRResponse(req *dns.Msg) (resp *dns.Msg) {
resp = s.makeResponse(req)
if req.Question[0].Qtype != dns.TypeSVCB {
return resp
}

// TODO(e.burkov): Think about storing the FQDN version of the server's
// name somewhere.
domainName := dns.Fqdn(s.conf.ServerName)
Expand All @@ -312,20 +308,24 @@ func (s *Server) makeDDRResponse(req *dns.Msg) (resp *dns.Msg) {
resp.Answer = append(resp.Answer, ans)
}

for _, addr := range s.dnsProxy.TLSListenAddr {
values := []dns.SVCBKeyValue{
&dns.SVCBAlpn{Alpn: []string{"dot"}},
&dns.SVCBPort{Port: uint16(addr.Port)},
}
if s.conf.hasIPAddrs {
// Only add DNS-over-TLS resolvers in case the certificate contains IP
// addresses.
for _, addr := range s.dnsProxy.TLSListenAddr {
values := []dns.SVCBKeyValue{
&dns.SVCBAlpn{Alpn: []string{"dot"}},
&dns.SVCBPort{Port: uint16(addr.Port)},
}

ans := &dns.SVCB{
Hdr: s.hdr(req, dns.TypeSVCB),
Priority: 1,
Target: domainName,
Value: values,
}
ans := &dns.SVCB{
Hdr: s.hdr(req, dns.TypeSVCB),
Priority: 1,
Target: domainName,
Value: values,
}

resp.Answer = append(resp.Answer, ans)
resp.Answer = append(resp.Answer, ans)
}
}

for _, addr := range s.dnsProxy.QUICListenAddr {
Expand Down
23 changes: 11 additions & 12 deletions internal/dnsforward/dns_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,19 +157,9 @@ func TestServer_ProcessDDRQuery(t *testing.T) {
func prepareTestServer(t *testing.T, portDoH, portDoT, portDoQ int, ddrEnabled bool) (s *Server) {
t.Helper()

proxyConf := proxy.Config{}

if portDoT > 0 {
proxyConf.TLSListenAddr = []*net.TCPAddr{{Port: portDoT}}
}

if portDoQ > 0 {
proxyConf.QUICListenAddr = []*net.UDPAddr{{Port: portDoQ}}
}

s = &Server{
dnsProxy: &proxy.Proxy{
Config: proxyConf,
Config: proxy.Config{},
},
conf: ServerConfig{
FilteringConfig: FilteringConfig{
Expand All @@ -181,8 +171,17 @@ func prepareTestServer(t *testing.T, portDoH, portDoT, portDoQ int, ddrEnabled b
},
}

if portDoT > 0 {
s.dnsProxy.TLSListenAddr = []*net.TCPAddr{{Port: portDoT}}
s.conf.hasIPAddrs = true
}

if portDoQ > 0 {
s.dnsProxy.QUICListenAddr = []*net.UDPAddr{{Port: portDoQ}}
}

if portDoH > 0 {
s.conf.TLSConfig.HTTPSListenAddrs = []*net.TCPAddr{{Port: portDoH}}
s.conf.HTTPSListenAddrs = []*net.TCPAddr{{Port: portDoH}}
}

return s
Expand Down
20 changes: 2 additions & 18 deletions internal/home/tls.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ import (
"encoding/pem"
"fmt"
"net/http"
"net/netip"
"os"
"strings"
"sync"
"time"

"github.com/AdguardTeam/AdGuardHome/internal/aghalg"
"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
"github.com/AdguardTeam/AdGuardHome/internal/aghtls"
"github.com/AdguardTeam/AdGuardHome/internal/dnsforward"
"github.com/AdguardTeam/golibs/errors"
"github.com/AdguardTeam/golibs/log"
Expand Down Expand Up @@ -513,22 +513,6 @@ func validateCertChain(certs []*x509.Certificate, srvName string) (err error) {
return nil
}

// certHasIP returns true if cert has at least a single IP address either in its
// DNS names or in the IP addresses section.
func certHasIP(cert *x509.Certificate) (ok bool) {
if len(cert.IPAddresses) > 0 {
return true
}

for _, name := range cert.DNSNames {
if _, err := netip.ParseAddr(name); err == nil {
return true
}
}

return false
}

// parseCertChain parses the certificate chain from raw data, and returns it.
// If ok is true, the returned error, if any, is not critical.
func parseCertChain(chain []byte) (parsedCerts []*x509.Certificate, ok bool, err error) {
Expand All @@ -550,7 +534,7 @@ func parseCertChain(chain []byte) (parsedCerts []*x509.Certificate, ok bool, err

log.Info("tls: number of certs: %d", len(parsedCerts))

if !certHasIP(parsedCerts[0]) {
if !aghtls.IsIPCertificate(parsedCerts[0]) {
err = errors.Error(`certificate has no IP addresses` +
`, this may cause issues with DNS-over-TLS clients`)
}
Expand Down

0 comments on commit 9793820

Please sign in to comment.