Skip to content

cluster: use hostname for TLS validation instead of resolved IP #5266

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.

Sign up for GitHub

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

Open
toller892 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

cluster: use hostname for TLS validation instead of resolved IP #5266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions cluster/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type Peer struct {

resolvedPeers []string
resolvePeersTimeout time.Duration
tlsTransport *TLSTransport

mtx sync.RWMutex
states map[string]State
Expand Down Expand Up @@ -173,7 +174,7 @@ func Create(

ctx, cancel := context.WithTimeout(context.Background(), resolveTimeout)
defer cancel()
resolvedPeers, err := resolvePeers(ctx, knownPeers, advertiseAddr, &net.Resolver{}, waitIfEmpty)
resolvedPeers, peerHostnames, err := resolvePeers(ctx, knownPeers, advertiseAddr, &net.Resolver{}, waitIfEmpty)
if err != nil {
return nil, fmt.Errorf("resolve peers: %w", err)
}
Expand Down Expand Up @@ -248,10 +249,13 @@ func Create(

if tlsTransportConfig != nil {
l.Info("using TLS for gossip")
cfg.Transport, err = NewTLSTransport(context.Background(), l, reg, cfg.BindAddr, cfg.BindPort, tlsTransportConfig)
if err != nil {
return nil, fmt.Errorf("tls transport: %w", err)
t, tErr := NewTLSTransport(context.Background(), l, reg, cfg.BindAddr, cfg.BindPort, tlsTransportConfig)
if tErr != nil {
return nil, fmt.Errorf("tls transport: %w", tErr)
}
t.SetPeerHostnames(peerHostnames)
cfg.Transport = t
p.tlsTransport = t
}

ml, err := memberlist.Create(cfg)
Expand Down Expand Up @@ -450,12 +454,16 @@ func (p *Peer) refresh() {

ctx, cancel := context.WithTimeout(context.Background(), p.resolvePeersTimeout)
defer cancel()
resolvedPeers, err := resolvePeers(ctx, p.knownPeers, p.advertiseAddr, &net.Resolver{}, false)
resolvedPeers, peerHostnames, err := resolvePeers(ctx, p.knownPeers, p.advertiseAddr, &net.Resolver{}, false)
if err != nil {
logger.Debug(fmt.Sprintf("%v", p.knownPeers), "err", err)
return
}

if p.tlsTransport != nil {
p.tlsTransport.SetPeerHostnames(peerHostnames)
}

members := p.mlist.Members()
for _, peer := range resolvedPeers {
var isPeerFound bool
Expand Down Expand Up @@ -729,13 +737,14 @@ func (b simpleBroadcast) Message() []byte { return []byte(
func (b simpleBroadcast) Invalidates(memberlist.Broadcast) bool { return false }
func (b simpleBroadcast) Finished() {}

func resolvePeers(ctx context.Context, peers []string, myAddress string, res *net.Resolver, waitIfEmpty bool) ([]string, error) {
func resolvePeers(ctx context.Context, peers []string, myAddress string, res *net.Resolver, waitIfEmpty bool) ([]string, map[string]string, error) {
var resolvedPeers []string
hostnames := make(map[string]string)

for _, peer := range peers {
host, port, err := net.SplitHostPort(peer)
if err != nil {
return nil, fmt.Errorf("split host/port for peer %s: %w", peer, err)
return nil, nil, fmt.Errorf("split host/port for peer %s: %w", peer, err)
}

retryCtx, cancel := context.WithCancel(ctx)
Expand Down Expand Up @@ -774,16 +783,18 @@ func resolvePeers(ctx context.Context, peers []string, myAddress string, res *ne
return nil
})
if err != nil {
return nil, err
return nil, nil, err
}
}

for _, ip := range ips {
resolvedPeers = append(resolvedPeers, net.JoinHostPort(ip.String(), port))
resolved := net.JoinHostPort(ip.String(), port)
resolvedPeers = append(resolvedPeers, resolved)
hostnames[resolved] = host
}
}

return resolvedPeers, nil
return resolvedPeers, hostnames, nil
}

func removeMyAddr(ips []net.IPAddr, targetPort, myAddr string) []net.IPAddr {
Expand Down
28 changes: 28 additions & 0 deletions cluster/cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ package cluster

import (
"context"
"net"
"testing"
"time"

Expand Down Expand Up @@ -438,3 +439,30 @@ func testPeerNames(t *testing.T, name1, name2 string) {
require.NotEqual(t, p1.Name(), p2.Name(), "peers should have different names")
}
}

func TestResolvePeersHostnameMapping(t *testing.T) {
ctx := context.Background()
res := &net.Resolver{}

t.Run("IPAddressPeers", func(t *testing.T) {
peers := []string{"192.168.1.1:9094", "10.0.0.2:9094"}
resolved, hostnames, err := resolvePeers(ctx, peers, "", res, false)
require.NoError(t, err)
require.Equal(t, peers, resolved)
// IP addresses get mapped back to themselves; this is harmless since
// TLS will still use IP SAN validation when ServerName is an IP.
require.Equal(t, "192.168.1.1", hostnames["192.168.1.1:9094"])
require.Equal(t, "10.0.0.2", hostnames["10.0.0.2:9094"])
})

t.Run("LocalhostPeer", func(t *testing.T) {
peers := []string{"localhost:9094"}
resolved, hostnames, err := resolvePeers(ctx, peers, "", res, false)
require.NoError(t, err)
require.NotEmpty(t, resolved)
// localhost resolves to an IP; the mapping should point back to "localhost".
for _, r := range resolved {
require.Equal(t, "localhost", hostnames[r])
}
})
}
12 changes: 10 additions & 2 deletions cluster/connection_pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ func newConnectionPool(tlsClientCfg *tls.Config) (*connectionPool, error) {

// borrowConnection returns a *tlsConn from the pool. The connection does not
// need to be returned to the pool because each connection has its own locking.
func (pool *connectionPool) borrowConnection(addr string, timeout time.Duration) (*tlsConn, error) {
// If hostname is non-empty, it is used as the TLS ServerName for certificate
// validation instead of the IP address from addr.
func (pool *connectionPool) borrowConnection(addr string, timeout time.Duration, hostname string) (*tlsConn, error) {
pool.mtx.Lock()
defer pool.mtx.Unlock()
if pool.cache == nil {
Expand All @@ -59,7 +61,13 @@ func (pool *connectionPool) borrowConnection(addr string, timeout time.Duration)
if exists && conn.alive() {
return conn, nil
}
conn, err := dialTLSConn(addr, timeout, pool.tlsConfig)
tlsCfg := pool.tlsConfig
if hostname != "" {
clone := tlsCfg.Clone()
clone.ServerName = hostname
tlsCfg = clone
}
conn, err := dialTLSConn(addr, timeout, tlsCfg)
if err != nil {
return nil, err
}
Expand Down
26 changes: 24 additions & 2 deletions cluster/tls_transport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ type TLSTransport struct {
tlsServerCfg *tls.Config
tlsClientCfg *tls.Config

// peerHostnames maps resolved IP:port addresses back to the original
// hostname provided by the user. This is used to set the TLS ServerName
// to the hostname instead of the IP, enabling proper certificate
// validation against DNS SANs.
peerHostnames map[string]string
Comment on lines +60 to +64

@coderabbitai coderabbitai Bot Jun 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Data race on peerHostnames map.

SetPeerHostnames writes the map from Peer.refresh() (runs every 15s), while WriteTo (Line 220) and DialTimeout (Lines 239-246) read from it concurrently via memberlist goroutines. There is no synchronization protecting this field.

🔒 Proposed fix using sync.RWMutex

Add a mutex to protect peerHostnames:

 type TLSTransport struct {
 	ctx          context.Context
 	cancel       context.CancelFunc
 	logger       *slog.Logger
 	bindAddr     string
 	bindPort     int
 	done         chan struct{}
 	listener     net.Listener
 	packetCh     chan *memberlist.Packet
 	streamCh     chan net.Conn
 	connPool     *connectionPool
 	tlsServerCfg *tls.Config
 	tlsClientCfg *tls.Config

+	peerHostnamesMtx sync.RWMutex
 	// peerHostnames maps resolved IP:port addresses back to the original
 	// hostname provided by the user. This is used to set the TLS ServerName
 	// to the hostname instead of the IP, enabling proper certificate
 	// validation against DNS SANs.
 	peerHostnames map[string]string

Update SetPeerHostnames:

 func (t *TLSTransport) SetPeerHostnames(hostnames map[string]string) {
+	t.peerHostnamesMtx.Lock()
+	defer t.peerHostnamesMtx.Unlock()
 	t.peerHostnames = hostnames
 }

Add a helper to safely read hostname:

func (t *TLSTransport) getPeerHostname(addr string) (string, bool) {
	t.peerHostnamesMtx.RLock()
	defer t.peerHostnamesMtx.RUnlock()
	host, ok := t.peerHostnames[addr]
	return host, ok
}

Then use t.getPeerHostname(addr) in WriteTo and DialTimeout.

Also applies to: 145-150, 220-220, 238-246

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cluster/tls_transport.go` around lines 60 - 64, peerHostnames on TLSTransport
is accessed concurrently (written in SetPeerHostnames and read in
WriteTo/DialTimeout) causing a data race; add a sync.RWMutex field (e.g.,
peerHostnamesMtx) to TLSTransport, lock it for writing inside SetPeerHostnames
when replacing/updating the map, and use RLock/RUnlock when reading in WriteTo
and DialTimeout by adding a helper method getPeerHostname(addr string) (returns
hostname, bool) that acquires the read lock and returns the map value; update
all reads to call getPeerHostname and ensure writes use the write lock to
eliminate the race.


packetsSent prometheus.Counter
packetsRcvd prometheus.Counter
streamsSent prometheus.Counter
Expand Down Expand Up @@ -136,6 +142,13 @@ func NewTLSTransport(
return t, nil
}

// SetPeerHostnames sets the mapping from resolved IP:port addresses to the
// original hostname provided by the user. This allows the TLS transport to
// use the hostname for certificate verification instead of the IP address.
func (t *TLSTransport) SetPeerHostnames(hostnames map[string]string) {
t.peerHostnames = hostnames
}

// FinalAdvertiseAddr is given the user's configured values (which
// might be empty) and returns the desired IP and port to advertise to
// the rest of the cluster.
Expand Down Expand Up @@ -204,7 +217,7 @@ func (t *TLSTransport) Shutdown() error {
// from the pool, and writes to it. It also returns a timestamp of when
// the packet was written.
func (t *TLSTransport) WriteTo(b []byte, addr string) (time.Time, error) {
conn, err := t.connPool.borrowConnection(addr, DefaultTCPTimeout)
conn, err := t.connPool.borrowConnection(addr, DefaultTCPTimeout, t.peerHostnames[addr])
if err != nil {
t.writeErrs.WithLabelValues("packet").Inc()
return time.Now(), fmt.Errorf("failed to dial: %w", err)
Expand All @@ -222,7 +235,16 @@ func (t *TLSTransport) WriteTo(b []byte, addr string) (time.Time, error) {
// DialTimeout is used to create a connection that allows memberlist
// to perform two-way communications with a peer.
func (t *TLSTransport) DialTimeout(addr string, timeout time.Duration) (net.Conn, error) {
conn, err := dialTLSConn(addr, timeout, t.tlsClientCfg)
tlsCfg := t.tlsClientCfg
if host, ok := t.peerHostnames[addr]; ok {
// Clone the TLS config and set ServerName to the original hostname
// so that certificate validation checks against DNS SANs instead of
// IP SANs. See https://github.com/prometheus/alertmanager/issues/5112.
clone := tlsCfg.Clone()
clone.ServerName = host
tlsCfg = clone
}
conn, err := dialTLSConn(addr, timeout, tlsCfg)
if err != nil {
t.writeErrs.WithLabelValues("stream").Inc()
return nil, fmt.Errorf("failed to dial: %w", err)
Expand Down