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

control/BUILD.bazel

@@ -29,9 +29,11 @@ go_library(
         "//pkg/log:go_default_library",
         "//pkg/metrics:go_default_library",
         "//pkg/metrics/v2:go_default_library",
+        "//pkg/private/common:go_default_library",
         "//pkg/private/ctrl/path_mgmt:go_default_library",
         "//pkg/private/prom:go_default_library",
         "//pkg/private/serrors:go_default_library",
+        "//pkg/private/util:go_default_library",
         "//pkg/proto/hidden_segment:go_default_library",
         "//pkg/scrypto:go_default_library",
         "//pkg/scrypto/cppki:go_default_library",

control/revhandler.go

@@ -16,9 +16,12 @@ package control
 
 import (
 	"context"
+	"time"
 
+	"github.com/scionproto/scion/pkg/addr"
+	"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/private/util"
 	"github.com/scionproto/scion/private/revcache"
 )
 
@@ -28,13 +31,13 @@ type RevocationHandler struct {
 	RevCache revcache.RevCache
 }
 
-func (h RevocationHandler) Revoke(ctx context.Context, revInfo *path_mgmt.RevInfo) error {
-	if _, err := h.RevCache.Insert(ctx, revInfo); err != nil {
-		return serrors.WrapStr("inserting revocation", err,
-			"isd_as", revInfo.IA(),
-			"interface_id", revInfo.IfID,
-			"expiration", revInfo.Expiration(),
-		)
+func (h RevocationHandler) Revoke(ia addr.IA, ingress, egress uint64) error {
+	revInfo := &path_mgmt.RevInfo{
+		IfID:         common.IFIDType(egress),
+		RawIsdas:     ia,
+		RawTimestamp: util.TimeToSecs(time.Now()),
+		RawTTL:       10,
 	}
+	_, _ = h.RevCache.Insert(context.TODO(), revInfo)
 	return nil
 }

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/daemon/BUILD.bazel

@@ -15,9 +15,9 @@ go_library(
         "//pkg/daemon/internal/metrics:go_default_library",
         "//pkg/drkey:go_default_library",
         "//pkg/grpc:go_default_library",
+        "//pkg/log:go_default_library",
         "//pkg/metrics:go_default_library",
         "//pkg/private/common:go_default_library",
-        "//pkg/private/ctrl/path_mgmt:go_default_library",
         "//pkg/private/prom:go_default_library",
         "//pkg/private/serrors:go_default_library",
         "//pkg/proto/daemon:go_default_library",

pkg/daemon/apitypes.go

@@ -18,9 +18,10 @@ import (
 	"context"
 	"math/rand"
 	"net"
+	"time"
 
 	"github.com/scionproto/scion/pkg/addr"
-	"github.com/scionproto/scion/pkg/private/ctrl/path_mgmt"
+	"github.com/scionproto/scion/pkg/log"
 	"github.com/scionproto/scion/pkg/private/serrors"
 	"github.com/scionproto/scion/pkg/snet"
 	"github.com/scionproto/scion/private/topology"
@@ -55,8 +56,20 @@ type RevHandler struct {
 	Connector Connector
 }
 
-func (h RevHandler) Revoke(ctx context.Context, revInfo *path_mgmt.RevInfo) error {
-	return h.Connector.RevNotification(ctx, revInfo)
+func (h RevHandler) Revoke(ia addr.IA, ingress, egress uint64) error {
+	go func() {
+		defer log.HandlePanic()
+		ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+		defer cancel()
+		// Note that this ignores the ingress interface.
+		// internal/external connectivity down messages are thus treated the same.
+		// Need to extend the beacon protocol to distinguish this.
+		err := h.Connector.RevNotification(ctx, ia, egress)
+		if err != nil {
+			log.Debug("Error asynchronously notifying daemon of revocation", "err", err)
+		}
+	}()
+	return nil
 }
 
 // TopoQuerier can be used to get topology information from the SCION Daemon.

pkg/daemon/daemon.go

@@ -24,7 +24,6 @@ import (
 	"github.com/scionproto/scion/pkg/daemon/internal/metrics"
 	"github.com/scionproto/scion/pkg/drkey"
 	libmetrics "github.com/scionproto/scion/pkg/metrics"
-	"github.com/scionproto/scion/pkg/private/ctrl/path_mgmt"
 	"github.com/scionproto/scion/pkg/private/serrors"
 	"github.com/scionproto/scion/pkg/snet"
 )
@@ -81,7 +80,7 @@ type Connector interface {
 	// of URIs of the service in the local AS.
 	SVCInfo(ctx context.Context, svcTypes []addr.SVC) (map[addr.SVC][]string, error)
 	// RevNotification sends a RevocationInfo message to the daemon.
-	RevNotification(ctx context.Context, revInfo *path_mgmt.RevInfo) error
+	RevNotification(ctx context.Context, ia addr.IA, ifID uint64) error
 	// DRKeyGetASHostKey requests a AS-Host Key from the daemon.
 	DRKeyGetASHostKey(ctx context.Context, meta drkey.ASHostMeta) (drkey.ASHostKey, error)
 	// DRKeyGetHostASKey requests a Host-AS Key from the daemon.

pkg/daemon/grpc.go

@@ -28,7 +28,6 @@ import (
 	"github.com/scionproto/scion/pkg/drkey"
 	libgrpc "github.com/scionproto/scion/pkg/grpc"
 	"github.com/scionproto/scion/pkg/private/common"
-	"github.com/scionproto/scion/pkg/private/ctrl/path_mgmt"
 	"github.com/scionproto/scion/pkg/private/serrors"
 	sdpb "github.com/scionproto/scion/pkg/proto/daemon"
 	dkpb "github.com/scionproto/scion/pkg/proto/drkey"
@@ -165,11 +164,11 @@ func (c grpcConn) SVCInfo(
 	return result, nil
 }
 
-func (c grpcConn) RevNotification(ctx context.Context, revInfo *path_mgmt.RevInfo) error {
+func (c grpcConn) RevNotification(ctx context.Context, ia addr.IA, ifID uint64) error {
 	client := sdpb.NewDaemonServiceClient(c.conn)
 	_, err := client.NotifyInterfaceDown(ctx, &sdpb.NotifyInterfaceDownRequest{
-		Id:    uint64(revInfo.IfID),
-		IsdAs: uint64(revInfo.RawIsdas),
+		Id:    ifID,
+		IsdAs: uint64(ia),
 	})
 	c.metrics.incIfDown(err)
 	return err

pkg/daemon/mock_daemon/BUILD.bazel

@@ -18,7 +18,6 @@ go_library(
         "//pkg/addr:go_default_library",
         "//pkg/daemon:go_default_library",
         "//pkg/drkey:go_default_library",
-        "//pkg/private/ctrl/path_mgmt:go_default_library",
         "//pkg/snet:go_default_library",
         "@com_github_golang_mock//gomock:go_default_library",
     ],

pkg/snet/BUILD.bazel

@@ -24,9 +24,7 @@ go_library(
         "//pkg/log:go_default_library",
         "//pkg/metrics/v2:go_default_library",
         "//pkg/private/common:go_default_library",
-        "//pkg/private/ctrl/path_mgmt:go_default_library",
         "//pkg/private/serrors:go_default_library",
-        "//pkg/private/util:go_default_library",
         "//pkg/slayers:go_default_library",
         "//pkg/slayers/path:go_default_library",
         "//pkg/slayers/path/empty:go_default_library",