Skip to content

Commit 0c3cc90

Browse files
committed
Pull request 2750: AGDNS-3720-imp-cert-has-ip
Squashed commit of the following: commit fb4eef0 Merge: 4d3772f e0b474f Author: Maksim Kazantsev <m.kazantsev@adguard.com> Date: Thu Aug 13 17:50:58 2026 +0300 Merge branch 'master' into AGDNS-3720-imp-cert-has-ip commit 4d3772f Author: Maksim Kazantsev <m.kazantsev@adguard.com> Date: Thu Aug 13 17:28:29 2026 +0300 home: imp tests; commit 7f2d3fe Author: Maksim Kazantsev <m.kazantsev@adguard.com> Date: Thu Aug 13 11:55:48 2026 +0300 home: fix test; commit a1459d0 Author: Maksim Kazantsev <m.kazantsev@adguard.com> Date: Thu Aug 13 11:29:57 2026 +0300 home: imp test; commit c24fadd Author: Maksim Kazantsev <m.kazantsev@adguard.com> Date: Wed Aug 12 15:17:04 2026 +0300 dnsforward: imp code; commit d9ad570 Author: Maksim Kazantsev <m.kazantsev@adguard.com> Date: Wed Aug 12 15:14:23 2026 +0300 home: cover has ip addrs with tests; rm invalid todo;
1 parent e0b474f commit 0c3cc90

3 files changed

Lines changed: 132 additions & 21 deletions

File tree

internal/dnsforward/process.go

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -251,28 +251,29 @@ func (s *Server) makeDDRResponse(req *dns.Msg) (resp *dns.Msg) {
251251
// server's TLS certificate contains IP addresses. req and resp must not be
252252
// nil.
253253
func (s *Server) appendDoTResolvers(req, resp *dns.Msg, domainName string) {
254-
hasIPAddrs := s.tlsConfigProvider.HasIPAddrs()
255-
256-
if hasIPAddrs {
257-
// Only add DNS-over-TLS resolvers in case the certificate contains IP
258-
// addresses.
259-
//
260-
// See https://github.com/AdguardTeam/AdGuardHome/issues/4927.
261-
for _, addr := range s.dnsProxy.TLSListenAddr {
262-
values := []dns.SVCBKeyValue{
263-
&dns.SVCBAlpn{Alpn: []string{"dot"}},
264-
&dns.SVCBPort{Port: uint16(addr.Port)},
265-
}
254+
if !s.tlsConfigProvider.HasIPAddrs() {
255+
return
256+
}
266257

267-
ans := &dns.SVCB{
268-
Hdr: s.hdr(req, dns.TypeSVCB),
269-
Priority: 1,
270-
Target: domainName,
271-
Value: values,
272-
}
258+
// Only add DNS-over-TLS resolvers in case the certificate contains IP
259+
// addresses.
260+
//
261+
// See https://github.com/AdguardTeam/AdGuardHome/issues/4927.
262+
for _, addr := range s.dnsProxy.TLSListenAddr {
263+
values := []dns.SVCBKeyValue{
264+
&dns.SVCBAlpn{Alpn: []string{"dot"}},
265+
&dns.SVCBPort{Port: uint16(addr.Port)},
266+
}
273267

274-
resp.Answer = append(resp.Answer, ans)
268+
ans := &dns.SVCB{
269+
Hdr: s.hdr(req, dns.TypeSVCB),
270+
Priority: 1,
271+
Target: domainName,
272+
Value: values,
275273
}
274+
275+
resp.Answer = append(resp.Answer, ans)
276+
276277
}
277278
}
278279

internal/home/tls.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -916,8 +916,6 @@ func (m *tlsManager) HasIPAddrs() (ok bool) {
916916
return false
917917
}
918918

919-
// TODO(m.kazantsev): Consider storing the value instead of parsing each
920-
// time.
921919
return aghtls.CertificateHasIP(m.tlsCert.Leaf)
922920
}
923921

internal/home/tls_internal_test.go

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"crypto/x509"
99
"encoding/pem"
1010
"math/big"
11+
"net"
1112
"os"
1213
"path/filepath"
1314
"testing"
@@ -200,6 +201,35 @@ func newCertWithoutIP(tb testing.TB) (
200201
return caCert, buf.Bytes(), leafKeyPEM
201202
}
202203

204+
// newCertWithIP generates a self-signed certificate with an IP address in its
205+
// SAN extension and returns the PEM-encoded certificate and private key.
206+
func newCertWithIP(tb testing.TB) (certPEM, keyPEM []byte) {
207+
tb.Helper()
208+
209+
key, err := rsa.GenerateKey(rand.Reader, 2048)
210+
require.NoError(tb, err)
211+
212+
now := time.Now()
213+
tmpl := &x509.Certificate{
214+
SerialNumber: big.NewInt(1),
215+
NotBefore: now.Add(-time.Hour),
216+
NotAfter: now.Add(time.Hour),
217+
KeyUsage: x509.KeyUsageDigitalSignature,
218+
IPAddresses: []net.IP{net.ParseIP("192.0.2.1")},
219+
}
220+
221+
certDER, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &key.PublicKey, key)
222+
require.NoError(tb, err)
223+
224+
certPEM = pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER})
225+
keyPEM = pem.EncodeToMemory(&pem.Block{
226+
Type: "RSA PRIVATE KEY",
227+
Bytes: x509.MarshalPKCS1PrivateKey(key),
228+
})
229+
230+
return certPEM, keyPEM
231+
}
232+
203233
// newCertAndKey is a helper function that generates certificate and key.
204234
func newCertAndKey(tb testing.TB, n int64) (certDER []byte, key *rsa.PrivateKey) {
205235
tb.Helper()
@@ -348,3 +378,85 @@ func TestTLSManager_Reload(t *testing.T) {
348378
extTLSConf = m.ExtendedTLSConfig()
349379
assertCertSerialNumber(t, extTLSConf, snAfter)
350380
}
381+
382+
func TestTLSManager_HasIPAddrs(t *testing.T) {
383+
t.Parallel()
384+
385+
_, noIPChainPEM, noIPKeyPEM := newCertWithoutIP(t)
386+
ipChainPEM, ipKeyPEM := newCertWithIP(t)
387+
388+
noIPSettings := &aghtls.ExtendedTLSConfig{
389+
Enabled: true,
390+
CertificateChain: string(noIPChainPEM),
391+
PrivateKey: string(noIPKeyPEM),
392+
}
393+
ipSettings := &aghtls.ExtendedTLSConfig{
394+
Enabled: true,
395+
CertificateChain: string(ipChainPEM),
396+
PrivateKey: string(ipKeyPEM),
397+
}
398+
399+
testCases := []struct {
400+
want assert.BoolAssertionFunc
401+
name string
402+
certificateChainData []byte
403+
privateKeyData []byte
404+
settings *aghtls.ExtendedTLSConfig
405+
}{{
406+
name: "no_ip_in_cert",
407+
settings: noIPSettings,
408+
want: assert.False,
409+
}, {
410+
name: "has_ip_in_cert",
411+
settings: ipSettings,
412+
want: assert.True,
413+
}, {
414+
name: "updated_to_ip",
415+
settings: noIPSettings,
416+
certificateChainData: ipChainPEM,
417+
privateKeyData: ipKeyPEM,
418+
want: assert.True,
419+
}, {
420+
name: "updated_to_no_ip",
421+
settings: ipSettings,
422+
certificateChainData: noIPChainPEM,
423+
privateKeyData: noIPKeyPEM,
424+
want: assert.False,
425+
}}
426+
427+
for _, tc := range testCases {
428+
t.Run(tc.name, func(t *testing.T) {
429+
// Do not run in parallel because the test modifies the TLS
430+
// manager's state.
431+
ctx := testutil.ContextWithTimeout(t, testTimeout)
432+
433+
m, err := newTLSManager(ctx, &tlsManagerConfig{
434+
logger: testLogger,
435+
confModifier: agh.EmptyConfigModifier{},
436+
manager: aghtls.EmptyManager{},
437+
extTLSConf: tc.settings,
438+
servePlainDNS: false,
439+
})
440+
require.NoError(t, err)
441+
442+
if tc.certificateChainData == nil && tc.privateKeyData == nil {
443+
tc.want(t, m.HasIPAddrs())
444+
445+
return
446+
}
447+
448+
func() {
449+
m.mu.Lock()
450+
defer m.mu.Unlock()
451+
452+
var cert tls.Certificate
453+
cert, err = tls.X509KeyPair(tc.certificateChainData, tc.privateKeyData)
454+
require.NoError(t, err)
455+
456+
m.tlsCert = &cert
457+
}()
458+
459+
tc.want(t, m.HasIPAddrs())
460+
})
461+
}
462+
}

0 commit comments

Comments
 (0)