snet: move SCMPHandler to cooked UDP Conn, don't propagate SCMP errors by default

gateway/gateway.go

@@ -406,26 +406,20 @@ func (g *Gateway) Run(ctx context.Context) error {
 		return serrors.WrapStr("unable to generate TLS config", err)
 	}
 
-	// scionNetworkNoSCMP is the network for the QUIC server connection. Because SCMP errors
-	// will cause the server's accepts to fail, we ignore SCMP.
-	scionNetworkNoSCMP := &snet.SCIONNetwork{
+	scionNetwork := &snet.SCIONNetwork{
 		Topology: g.Daemon,
-		// Discard all SCMP propagation, to avoid accept/read errors on the
-		// QUIC server/client.
-		SCMPHandler: snet.SCMPPropagationStopper{
-			Handler: snet.DefaultSCMPHandler{
-				RevocationHandler: revocationHandler,
-				SCMPErrors:        g.Metrics.SCMPErrors,
-			},
-			Log: log.FromCtx(ctx).Debug,
+		SCMPHandler: snet.DefaultSCMPHandler{
+			RevocationHandler: revocationHandler,
+			SCMPErrors:        g.Metrics.SCMPErrors,
+			Log:               log.FromCtx(ctx).Debug,
 		},
 		PacketConnMetrics: g.Metrics.SCIONPacketConnMetrics,
 		Metrics:           g.Metrics.SCIONNetworkMetrics,
 	}
 
 	// Initialize the UDP/SCION QUIC conn for outgoing Gateway Discovery RPCs and outgoing Prefix
 	// Fetching. Open up a random high port for this.
-	clientConn, err := scionNetworkNoSCMP.Listen(
+	clientConn, err := scionNetwork.Listen(
 		context.TODO(),
 		"udp",
 		&net.UDPAddr{IP: g.ControlClientIP},
@@ -469,17 +463,6 @@ func (g *Gateway) Run(ctx context.Context) error {
 		}
 	}
 
-	// scionNetwork is the network for all SCION connections, with the exception of the QUIC server
-	// and client connection.
-	scionNetwork := &snet.SCIONNetwork{
-		Topology: g.Daemon,
-		SCMPHandler: snet.DefaultSCMPHandler{
-			RevocationHandler: revocationHandler,
-			SCMPErrors:        g.Metrics.SCMPErrors,
-		},
-		PacketConnMetrics: g.Metrics.SCIONPacketConnMetrics,
-		Metrics:           g.Metrics.SCIONNetworkMetrics,
-	}
 	remoteMonitor := &control.RemoteMonitor{
 		IAs:                   remoteIAsChannel,
 		RemotesMonitored:      rmMetric,
@@ -518,7 +501,7 @@ func (g *Gateway) Run(ctx context.Context) error {
 	}()
 	logger.Debug("Remote monitor started.")
 
-	serverConn, err := scionNetworkNoSCMP.Listen(
+	serverConn, err := scionNetwork.Listen(
 		context.TODO(),
 		"udp",
 		g.ControlServerAddr,

gateway/pathhealth/pathwatcher.go

@@ -83,13 +83,6 @@ func (f *DefaultPathWatcherFactory) New(
 		return create(remote)
 	}
 	conn, err := (&snet.SCIONNetwork{
-		SCMPHandler: scmpHandler{
-			wrappedHandler: snet.DefaultSCMPHandler{
-				RevocationHandler: f.RevocationHandler,
-				SCMPErrors:        f.SCMPErrors,
-			},
-			pkts: pktChan,
-		},
 		PacketConnMetrics: f.SCIONPacketConnMetrics,
 		Topology:          f.Topology,
 	}).OpenRaw(ctx, &net.UDPAddr{IP: f.LocalIP.AsSlice()})
@@ -105,6 +98,10 @@ func (f *DefaultPathWatcherFactory) New(
 			IA:   f.LocalIA,
 			Host: addr.HostIP(f.LocalIP),
 		},
+		scmpHandler: snet.DefaultSCMPHandler{
+			RevocationHandler: f.RevocationHandler,
+			SCMPErrors:        f.SCMPErrors,
+		},
 		pktChan:          pktChan,
 		probesSent:       createCounter(f.ProbesSent, remote),
 		probesReceived:   createCounter(f.ProbesReceived, remote),
@@ -122,14 +119,16 @@ type pathWatcher struct {
 	// conn is the packet conn used to send probes on. The pathwatcher takes
 	// ownership and will close it on termination.
 	conn snet.PacketConn
+	// scmpHandler handles non-traceroute SCMP packets received on conn
+	scmpHandler snet.SCMPHandler
 	// id is used as SCMP traceroute ID. Since each pathwatcher should have it's
 	// own high port this value can be random.
 	id uint16
 	// localAddr is the local address used in the probe packet.
 	localAddr snet.SCIONAddress
 	// pktChan is the channel which provides the incoming packets on the
 	// connection.
-	pktChan <-chan traceroutePkt
+	pktChan chan traceroutePkt
 
 	probesSent       metrics.Counter
 	probesReceived   metrics.Counter
@@ -239,13 +238,20 @@ func (w *pathWatcher) drainConn(ctx context.Context) {
 			return
 		}
 		if err != nil {
-			if _, ok := err.(*snet.OpError); ok {
-				// ignore SCMP errors they are already dealt with in the SCMP
-				// handler.
-				continue
-			}
 			logger.Info("Unexpected error when reading probe reply", "err", err)
 		}
+		switch pld := pkt.Payload.(type) {
+		case snet.SCMPTracerouteReply:
+			w.pktChan <- traceroutePkt{
+				Remote:     pkt.Source.IA,
+				Identifier: pld.Identifier,
+				Sequence:   pld.Sequence,
+			}
+		case snet.SCMPPayload:
+			if w.scmpHandler != nil {
+				w.scmpHandler.Handle(&pkt)
+			}
+		}
 	}
 }
 

gateway/pathhealth/scmp.go

@@ -16,33 +16,10 @@ package pathhealth
 
 import (
 	"github.com/scionproto/scion/pkg/addr"
-	"github.com/scionproto/scion/pkg/private/serrors"
-	"github.com/scionproto/scion/pkg/snet"
 )
 
 type traceroutePkt struct {
 	Remote     addr.IA
 	Identifier uint16
 	Sequence   uint16
 }
-
-type scmpHandler struct {
-	wrappedHandler snet.SCMPHandler
-	pkts           chan<- traceroutePkt
-}
-
-func (h scmpHandler) Handle(pkt *snet.Packet) error {
-	if pkt.Payload == nil {
-		return serrors.New("no payload found")
-	}
-	tr, ok := pkt.Payload.(snet.SCMPTracerouteReply)
-	if !ok {
-		return h.wrappedHandler.Handle(pkt)
-	}
-	h.pkts <- traceroutePkt{
-		Remote:     pkt.Source.IA,
-		Identifier: tr.Identifier,
-		Sequence:   tr.Sequence,
-	}
-	return nil
-}

pkg/snet/conn.go

@@ -18,29 +18,16 @@ package snet
 import (
 	"context"
 	"net"
+	"syscall"
 	"time"
 
 	"github.com/scionproto/scion/pkg/private/common"
-	"github.com/scionproto/scion/pkg/private/ctrl/path_mgmt"
 	"github.com/scionproto/scion/pkg/private/serrors"
-	"github.com/scionproto/scion/pkg/slayers"
 )
 
-type OpError struct {
-	typeCode slayers.SCMPTypeCode
-	revInfo  *path_mgmt.RevInfo
-}
-
-func (e *OpError) RevInfo() *path_mgmt.RevInfo {
-	return e.revInfo
-}
-
-func (e *OpError) Error() string {
-	return e.typeCode.String()
-}
-
 var _ net.Conn = (*Conn)(nil)
 var _ net.PacketConn = (*Conn)(nil)
+var _ syscall.Conn = (*Conn)(nil)
 
 type Conn struct {
 	conn PacketConn
@@ -96,6 +83,7 @@ func NewCookedConn(
 			conn:        pconn,
 			buffer:      make([]byte, common.SupportedMTU),
 			replyPather: o.replyPather,
+			scmpHandler: o.scmpHandler,
 			local:       local,
 		},
 	}, nil
@@ -109,6 +97,18 @@ func (c *Conn) RemoteAddr() net.Addr {
 	return c.remote
 }
 
+func (c *Conn) SyscallConn() (syscall.RawConn, error) {
+	return c.conn.SyscallConn()
+}
+
+func (c *Conn) SetReadBuffer(n int) error {
+	return c.conn.SetReadBuffer(n)
+}
+
+func (c *Conn) SetWriteBuffer(n int) error {
+	return c.conn.SetWriteBuffer(n)
+}
+
 func (c *Conn) SetDeadline(t time.Time) error {
 	if err := c.scionConnReader.SetReadDeadline(t); err != nil {
 		return err
@@ -124,37 +124,46 @@ func (c *Conn) Close() error {
 }
 
 // ConnOption is a functional option type for configuring a Conn.
-type ConnOption func(o *options)
+type ConnOption func(o *connOptions)
 
 // WithReplyPather sets the reply pather for the connection.
 // The reply pather is responsible for determining the path to send replies to.
-// If the provided replyPather is not nil, it will be set as the reply pather for the connection.
+// If this option is not provided, DefaultReplyPather is used.
 func WithReplyPather(replyPather ReplyPather) ConnOption {
-	return func(o *options) {
-		if replyPather != nil {
-			o.replyPather = replyPather
-		}
+	return func(o *connOptions) {
+		o.replyPather = replyPather
+	}
+}
+
+// WithSCMPHandler sets the SCMP handler for the connection.
+// The SCMP handler is a callback to react to SCMP messages, specifically to error messages.
+func WithSCMPHandler(scmpHandler SCMPHandler) ConnOption {
+	return func(o *connOptions) {
+		o.scmpHandler = scmpHandler
 	}
 }
 
 // WithRemote sets the remote address for the connection.
+// This only applies to NewCookedConn, but not Dial/Listen.
 func WithRemote(addr *UDPAddr) ConnOption {
-	return func(o *options) {
+	return func(o *connOptions) {
 		o.remote = addr
 	}
 }
 
-type options struct {
+type connOptions struct {
 	replyPather ReplyPather
+	scmpHandler SCMPHandler
 	remote      *UDPAddr
 }
 
-func apply(opts []ConnOption) options {
-	o := options{
-		replyPather: DefaultReplyPather{},
-	}
+func apply(opts []ConnOption) connOptions {
+	o := connOptions{}
 	for _, option := range opts {
 		option(&o)
 	}
+	if o.replyPather == nil {
+		o.replyPather = DefaultReplyPather{}
+	}
 	return o
 }

pkg/snet/interface.go

@@ -22,6 +22,6 @@ import (
 
 type Network interface {
 	OpenRaw(ctx context.Context, addr *net.UDPAddr) (PacketConn, error)
-	Listen(ctx context.Context, network string, listen *net.UDPAddr) (*Conn, error)
-	Dial(ctx context.Context, network string, listen *net.UDPAddr, remote *UDPAddr) (*Conn, error)
+	Listen(ctx context.Context, network string, listen *net.UDPAddr, options ...ConnOption) (*Conn, error)
+	Dial(ctx context.Context, network string, listen *net.UDPAddr, remote *UDPAddr, options ...ConnOption) (*Conn, error)
 }