From f3c21fbffbe5f7d7b5e96686666e253de9437e33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorben=20Kr=C3=BCger?= Date: Wed, 13 Jul 2022 15:45:57 +0200 Subject: [PATCH 01/25] initial test --- control/beaconing/extender.go | 3 +++ control/cmd/control/main.go | 1 + private/app/path/pathprobe/paths.go | 5 ++++- private/topology/json/json.go | 11 ++++++----- private/topology/topology.go | 5 +++++ router/dataplane.go | 27 ++++++++++++++++++++++----- 6 files changed, 41 insertions(+), 11 deletions(-) diff --git a/control/beaconing/extender.go b/control/beaconing/extender.go index 2c02c5e35f..f7e5cf3930 100644 --- a/control/beaconing/extender.go +++ b/control/beaconing/extender.go @@ -89,6 +89,8 @@ func (s *DefaultExtender) Extend( if err != nil { return serrors.WrapStr("creating hop entry", err) } + // TODO: peer entries are missing here! + peerBeta := extractBeta(pseg) ^ binary.BigEndian.Uint16(hopEntry.HopField.MAC[:2]) peerEntries, epicPeerMacs, err := s.createPeerEntries(egress, peers, ts, peerBeta) if err != nil { @@ -184,6 +186,7 @@ func (s *DefaultExtender) createPeerEntry(ingress, egress uint16, ts time.Time, return seg.PeerEntry{}, nil, serrors.WrapStr("checking remote ingress interface", err, "ingress_interface", ingress) } + hopF, epicMac := s.createHopF(ingress, egress, ts, beta) return seg.PeerEntry{ PeerMTU: int(remoteInMTU), diff --git a/control/cmd/control/main.go b/control/cmd/control/main.go index d481cd607c..54fcced714 100644 --- a/control/cmd/control/main.go +++ b/control/cmd/control/main.go @@ -129,6 +129,7 @@ func realMain(ctx context.Context) error { return topo.Run(errCtx) }) intfs := ifstate.NewInterfaces(adaptInterfaceMap(topo.InterfaceInfoMap()), ifstate.Config{}) + log.Info("XXX INTERFACES", "intfs", intfs) g.Go(func() error { defer log.HandlePanic() sub := topo.Subscribe() diff --git a/private/app/path/pathprobe/paths.go b/private/app/path/pathprobe/paths.go index 5a64f601c8..1b183617c4 100644 --- a/private/app/path/pathprobe/paths.go +++ b/private/app/path/pathprobe/paths.go @@ -186,7 +186,9 @@ func (p Prober) GetStatuses(ctx context.Context, paths []snet.Path, continue } pathsPerIP[localIP.String()] = append(pathsPerIP[localIP.String()], path) - addStatus(PathKey(path), Status{Status: StatusTimeout, LocalIP: localIP}) + //FIXME + //addStatus(PathKey(path), Status{Status: StatusTimeout, LocalIP: localIP}) + addStatus(PathKey(path), Status{Status: StatusAlive, LocalIP: localIP}) } // Sequence number for the sent traceroute packets. @@ -209,6 +211,7 @@ func (p Prober) GetStatuses(ctx context.Context, paths []snet.Path, // Send probe for each path. for _, path := range paths { + log.Info("YYY", "underlay", path.UnderlayNextHop) originalPath, ok := path.Dataplane().(snetpath.SCION) if !ok { return serrors.New("not a scion path") diff --git a/private/topology/json/json.go b/private/topology/json/json.go index 08e7cf333d..6914acf6ee 100644 --- a/private/topology/json/json.go +++ b/private/topology/json/json.go @@ -107,11 +107,12 @@ type GatewayInfo struct { // BRInterface contains the information for an data-plane BR socket that is external (i.e., facing // the neighboring AS). type BRInterface struct { - Underlay Underlay `json:"underlay,omitempty"` - IA string `json:"isd_as"` - LinkTo string `json:"link_to"` - MTU int `json:"mtu"` - BFD *BFD `json:"bfd,omitempty"` + Underlay Underlay `json:"underlay,omitempty"` + IA string `json:"isd_as"` + LinkTo string `json:"link_to"` + MTU int `json:"mtu"` + BFD *BFD `json:"bfd,omitempty"` + Remote common.IFIDType `json:"remote,omitempty"` } // Underlay is the underlay information for a BR interface. diff --git a/private/topology/topology.go b/private/topology/topology.go index 56bc023210..5419e30709 100644 --- a/private/topology/topology.go +++ b/private/topology/topology.go @@ -243,6 +243,11 @@ func (t *RWTopology) populateBR(raw *jsontopo.Topology) error { return err } ifinfo.LinkType = LinkTypeFromString(rawIntf.LinkTo) + //FIXME + if ifinfo.LinkType == Peer { + ifinfo.RemoteIFID = 42 + } + if err = ifinfo.CheckLinks(t.IsCore, name); err != nil { return err } diff --git a/router/dataplane.go b/router/dataplane.go index 3410979683..6ef8186893 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -997,6 +997,8 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { return processResult{}, nil case ingress == topology.Child && egress == topology.Child: return processResult{}, nil + case egress == topology.Peer: + return processResult{}, nil default: return p.packSCMP( slayers.SCMPTypeParameterProblem, @@ -1010,9 +1012,8 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { func (p *scionPacketProcessor) updateNonConsDirIngressSegID() error { // against construction dir the ingress router updates the SegID, ifID == 0 // means this comes from this AS itself, so nothing has to be done. - // TODO(lukedirtwalker): For packets destined to peer links this shouldn't - // be updated. - if !p.infoField.ConsDir && p.ingressID != 0 { + // For packets destined to peer links this shouldn't be updated. + if !p.infoField.Peer && !p.infoField.ConsDir && p.ingressID != 0 { p.infoField.UpdateSegID(p.hopField.Mac) if err := p.path.SetInfoField(p.infoField, int(p.path.PathMeta.CurrINF)); err != nil { return serrors.WrapStr("update info field", err) @@ -1032,7 +1033,9 @@ func (p *scionPacketProcessor) currentHopPointer() uint16 { } func (p *scionPacketProcessor) verifyCurrentMAC() (processResult, error) { - fullMac := path.FullMAC(p.mac, p.infoField, p.hopField, p.macBuffers.scionInput) + //FIXME + //fullMac := path.FullMAC(p.mac, p.infoField, p.hopField, p.macBuffers.scionInput) + fullMac := p.hopField.Mac if subtle.ConstantTimeCompare(p.hopField.Mac[:path.MacLen], fullMac[:path.MacLen]) == 0 { return p.packSCMP( slayers.SCMPTypeParameterProblem, @@ -1048,7 +1051,7 @@ func (p *scionPacketProcessor) verifyCurrentMAC() (processResult, error) { } // Add the full MAC to the SCION packet processor, // such that EPIC does not need to recalculate it. - p.cachedMac = fullMac + p.cachedMac = fullMac[:] return processResult{}, nil } @@ -1077,6 +1080,12 @@ func (p *scionPacketProcessor) processEgress() error { return serrors.WrapStr("update info field", err) } } + + //FIXME + if p.infoField.Peer { + return nil + } + if err := p.path.IncPath(); err != nil { // TODO parameter problem invalid path return serrors.WrapStr("incrementing path", err) @@ -1123,6 +1132,9 @@ func (p *scionPacketProcessor) ingressInterface() uint16 { } func (p *scionPacketProcessor) egressInterface() uint16 { + if p.infoField.Peer { + return 42 + } if p.infoField.ConsDir { return p.hopField.ConsEgress } @@ -1310,6 +1322,9 @@ func (p *scionPacketProcessor) process() (processResult, error) { } egressID := p.egressInterface() + if egressID == 0 { + egressID = 42 + } if c, ok := p.d.external[egressID]; ok { if err := p.processEgress(); err != nil { return processResult{}, err @@ -1325,6 +1340,8 @@ func (p *scionPacketProcessor) process() (processResult, error) { if !p.infoField.ConsDir { errCode = slayers.SCMPCodeUnknownHopFieldIngress } + + log.Debug("DATAPLANE", "p", p.d.external, "egressID", egressID) return p.packSCMP( slayers.SCMPTypeParameterProblem, errCode, From 84207433f91a7261cf32d829c83da015da9a0eba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorben=20Kr=C3=BCger?= Date: Wed, 13 Jul 2022 17:37:36 +0200 Subject: [PATCH 02/25] configtest --- control/cmd/control/main.go | 1 - pkg/slayers/path/BUILD.bazel | 1 + pkg/slayers/path/peerfield.go | 75 +++++++++++++++++++++++++++++ pkg/slayers/path/scion/base.go | 2 +- pkg/slayers/path/scion/raw.go | 29 +++++++++++ private/app/path/pathprobe/paths.go | 1 - private/topology/json/json.go | 12 ++--- private/topology/topology.go | 3 +- router/connector.go | 6 +++ router/control/conf.go | 5 +- router/dataplane.go | 67 ++++++++++++++++++++++---- tools/topology/topo.py | 9 +++- topology/peering-test.topo | 40 +++++++++++++++ 13 files changed, 227 insertions(+), 24 deletions(-) create mode 100644 pkg/slayers/path/peerfield.go create mode 100644 topology/peering-test.topo diff --git a/control/cmd/control/main.go b/control/cmd/control/main.go index 54fcced714..d481cd607c 100644 --- a/control/cmd/control/main.go +++ b/control/cmd/control/main.go @@ -129,7 +129,6 @@ func realMain(ctx context.Context) error { return topo.Run(errCtx) }) intfs := ifstate.NewInterfaces(adaptInterfaceMap(topo.InterfaceInfoMap()), ifstate.Config{}) - log.Info("XXX INTERFACES", "intfs", intfs) g.Go(func() error { defer log.HandlePanic() sub := topo.Subscribe() diff --git a/pkg/slayers/path/BUILD.bazel b/pkg/slayers/path/BUILD.bazel index d580caa581..f6583b9cc2 100644 --- a/pkg/slayers/path/BUILD.bazel +++ b/pkg/slayers/path/BUILD.bazel @@ -7,6 +7,7 @@ go_library( "infofield.go", "mac.go", "path.go", + "peerfield.go", ], importpath = "github.com/scionproto/scion/pkg/slayers/path", visibility = ["//visibility:public"], diff --git a/pkg/slayers/path/peerfield.go b/pkg/slayers/path/peerfield.go new file mode 100644 index 0000000000..5bc9ffdb78 --- /dev/null +++ b/pkg/slayers/path/peerfield.go @@ -0,0 +1,75 @@ +// Copyright 2022 Thorben Krüger +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package path + +import ( + "encoding/binary" + + "github.com/scionproto/scion/pkg/private/serrors" +) + +// PeerField is the HopField used in the SCION peer path type. +// +// The Peer Field has the following format: +// 0 1 2 3 +// 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +// |r r r r r r I E| ExpTime | PeerIF | +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +// | EgressIF | | +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + +// | MAC | +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +// +type PeerField struct { + Flags uint8 + // Timestamp + (1 + ExpTime) * (24*60*60)/256 + ExpTime uint8 + // PeerIF is the interface ID of the remote peer + PeerIF uint16 + // EgressIF is local egress interface ID. + EgressIF uint16 + // Mac is the 6-byte Message Authentication Code to authenticate the PeerField. + Mac [MacLen]byte +} + +// DecodeFromBytes populates the fields from a raw buffer. The buffer must be of length >= +// path.HopLen. +func (p *PeerField) DecodeFromBytes(raw []byte) error { + if len(raw) < HopLen { + return serrors.New("PeerField raw too short", "expected", HopLen, "actual", len(raw)) + } + p.Flags = raw[0] + p.ExpTime = raw[1] + p.PeerIF = binary.BigEndian.Uint16(raw[2:4]) + p.EgressIF = binary.BigEndian.Uint16(raw[4:6]) + copy(p.Mac[:], raw[6:6+MacLen]) + return nil +} + +// SerializeTo writes the fields into the provided buffer. The buffer must be of length >= +// path.HopLen. +func (p *PeerField) SerializeTo(b []byte) error { + if len(b) < HopLen { + return serrors.New("buffer for PeerField too short", "expected", MacLen, "actual", len(b)) + } + b[0] = p.Flags + b[1] = p.ExpTime + binary.BigEndian.PutUint16(b[2:4], p.PeerIF) + binary.BigEndian.PutUint16(b[4:6], p.EgressIF) + copy(b[6:6+MacLen], p.Mac[:]) + + return nil +} diff --git a/pkg/slayers/path/scion/base.go b/pkg/slayers/path/scion/base.go index dc6a9fc66e..9089fdbc01 100644 --- a/pkg/slayers/path/scion/base.go +++ b/pkg/slayers/path/scion/base.go @@ -76,7 +76,7 @@ func (s *Base) IncPath() error { } if int(s.PathMeta.CurrHF) >= s.NumHops-1 { s.PathMeta.CurrHF = uint8(s.NumHops - 1) - return serrors.New("path already at end") + return serrors.New(fmt.Sprintf("path already at end, currhf %d, numhops %d", s.PathMeta.CurrHF, s.NumHops)) } s.PathMeta.CurrHF++ // Update CurrINF diff --git a/pkg/slayers/path/scion/raw.go b/pkg/slayers/path/scion/raw.go index 8591093774..860019d001 100644 --- a/pkg/slayers/path/scion/raw.go +++ b/pkg/slayers/path/scion/raw.go @@ -163,6 +163,35 @@ func (s *Raw) IsFirstHop() bool { return s.PathMeta.CurrHF == 0 } +// GetPeerField returns the PeerField at a given index. +func (s *Raw) GetPeerField(idx int) (path.PeerField, error) { + if idx >= s.NumHops { + return path.PeerField{}, + serrors.New("PeerField index out of bounds", "max", s.NumHops-1, "actual", idx) + } + hopOffset := MetaLen + s.NumINF*path.InfoLen + idx*path.HopLen + hop := path.PeerField{} + if err := hop.DecodeFromBytes(s.Raw[hopOffset : hopOffset+path.HopLen]); err != nil { + return path.PeerField{}, err + } + return hop, nil +} + +// GetCurrentPeerField is a convenience method that returns the current hop field pointed to by the +// CurrHF index in the path meta header. +func (s *Raw) GetCurrentPeerField() (path.PeerField, error) { + return s.GetPeerField(int(s.PathMeta.CurrHF)) +} + +// SetPeerField updates the PeerField at a given index. +func (s *Raw) SetPeerField(hop path.PeerField, idx int) error { + if idx >= s.NumHops { + return serrors.New("PeerField index out of bounds", "max", s.NumHops-1, "actual", idx) + } + hopOffset := MetaLen + s.NumINF*path.InfoLen + idx*path.HopLen + return hop.SerializeTo(s.Raw[hopOffset : hopOffset+path.HopLen]) +} + // IsPenultimateHop returns whether the current hop is the penultimate hop on the path. func (s *Raw) IsPenultimateHop() bool { return int(s.PathMeta.CurrHF) == (s.NumHops - 2) diff --git a/private/app/path/pathprobe/paths.go b/private/app/path/pathprobe/paths.go index 1b183617c4..0f54230c5a 100644 --- a/private/app/path/pathprobe/paths.go +++ b/private/app/path/pathprobe/paths.go @@ -211,7 +211,6 @@ func (p Prober) GetStatuses(ctx context.Context, paths []snet.Path, // Send probe for each path. for _, path := range paths { - log.Info("YYY", "underlay", path.UnderlayNextHop) originalPath, ok := path.Dataplane().(snetpath.SCION) if !ok { return serrors.New("not a scion path") diff --git a/private/topology/json/json.go b/private/topology/json/json.go index 6914acf6ee..7e55348f37 100644 --- a/private/topology/json/json.go +++ b/private/topology/json/json.go @@ -107,12 +107,12 @@ type GatewayInfo struct { // BRInterface contains the information for an data-plane BR socket that is external (i.e., facing // the neighboring AS). type BRInterface struct { - Underlay Underlay `json:"underlay,omitempty"` - IA string `json:"isd_as"` - LinkTo string `json:"link_to"` - MTU int `json:"mtu"` - BFD *BFD `json:"bfd,omitempty"` - Remote common.IFIDType `json:"remote,omitempty"` + Underlay Underlay `json:"underlay,omitempty"` + IA string `json:"isd_as"` + LinkTo string `json:"link_to"` + MTU int `json:"mtu"` + BFD *BFD `json:"bfd,omitempty"` + RemoteIFID common.IFIDType `json:"remote_if_id,omitempty"` } // Underlay is the underlay information for a BR interface. diff --git a/private/topology/topology.go b/private/topology/topology.go index 5419e30709..d2d3f5856e 100644 --- a/private/topology/topology.go +++ b/private/topology/topology.go @@ -243,9 +243,8 @@ func (t *RWTopology) populateBR(raw *jsontopo.Topology) error { return err } ifinfo.LinkType = LinkTypeFromString(rawIntf.LinkTo) - //FIXME if ifinfo.LinkType == Peer { - ifinfo.RemoteIFID = 42 + ifinfo.RemoteIFID = rawIntf.RemoteIFID } if err = ifinfo.CheckLinks(t.IsCore, name); err != nil { diff --git a/router/connector.go b/router/connector.go index f2bd767213..7f7c35cf1f 100644 --- a/router/connector.go +++ b/router/connector.go @@ -91,9 +91,15 @@ func (c *Connector) AddExternalInterface(localIfID common.IFIDType, link control if !c.ia.Equal(link.Local.IA) { return serrors.WithCtx(errMultiIA, "current", c.ia, "new", link.Local.IA) } + // FIXME, maybe change signature of AddLinkType to include remote ID if err := c.DataPlane.AddLinkType(intf, link.LinkTo); err != nil { return serrors.WrapStr("adding link type", err, "if_id", localIfID) } + if link.LinkTo == topology.Peer { + if err := c.DataPlane.AddRemotePeer(intf, uint16(link.Remote.IFID)); err != nil { + return serrors.WrapStr("adding peering link", err, "local", localIfID, "remote", link.Remote.IFID) + } + } if err := c.DataPlane.AddNeighborIA(intf, link.Remote.IA); err != nil { return serrors.WrapStr("adding neighboring IA", err, "if_id", localIfID) } diff --git a/router/control/conf.go b/router/control/conf.go index b11e4c27d4..1eed46cf81 100644 --- a/router/control/conf.go +++ b/router/control/conf.go @@ -50,10 +50,11 @@ type LinkInfo struct { MTU int } -// LinkEnd represents on end of a link. +// LinkEnd represents one end of a link. type LinkEnd struct { IA addr.IA Addr *net.UDPAddr + IFID common.IFIDType } type ObservableDataplane interface { @@ -174,10 +175,12 @@ func confExternalInterfaces(dp Dataplane, cfg *Config) error { Local: LinkEnd{ IA: cfg.IA, Addr: snet.CopyUDPAddr(iface.Local), + IFID: iface.ID, }, Remote: LinkEnd{ IA: iface.IA, Addr: snet.CopyUDPAddr(iface.Remote), + IFID: iface.RemoteIFID, }, Instance: iface.BRName, BFD: WithDefaults(BFD(iface.BFD)), diff --git a/router/dataplane.go b/router/dataplane.go index 6ef8186893..b47920d65c 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -98,6 +98,7 @@ type DataPlane struct { external map[uint16]BatchConn linkTypes map[uint16]topology.LinkType neighborIAs map[uint16]addr.IA + remotePeerIF map[uint16]uint16 internal BatchConn internalIP *net.IPAddr internalNextHops map[uint16]*net.UDPAddr @@ -265,6 +266,24 @@ func (d *DataPlane) AddLinkType(ifID uint16, linkTo topology.LinkType) error { return nil } +// AddRemotePeer adds the remote peering interface ID for local +// interface ID. If the link type for the given ID is already set to +// a different type, this method will return an error. This can only +// be called on a not yet running dataplane. +func (d *DataPlane) AddRemotePeer(localifID, remoteifID uint16) error { + if t, ok := d.linkTypes[localifID]; ok && t != topology.Peer { + return serrors.WithCtx(unsupportedPathType, "type", t) + } + if _, exists := d.remotePeerIF[localifID]; exists { + return serrors.WithCtx(alreadySet, "localifID", localifID) + } + if d.remotePeerIF == nil { + d.remotePeerIF = make(map[uint16]uint16) + } + d.remotePeerIF[localifID] = remoteifID + return nil +} + // AddExternalInterfaceBFD adds the inter AS connection BFD session. func (d *DataPlane) AddExternalInterfaceBFD(ifID uint16, conn BatchConn, src, dst control.LinkEnd, cfg control.BFD) error { @@ -781,6 +800,11 @@ type scionPacketProcessor struct { path *scion.Raw // hopField is the current hopField field, is updated during processing. hopField path.HopField + // localPeer is the local peerField field, is updated during processing. + localPeer path.PeerField + // remotePeer is the remote peerField field, is updated during processing. + remotePeer path.PeerField + // infoField is the current infoField field, is updated during processing. infoField path.InfoField // segmentChange indicates if the path segment was changed during processing. @@ -846,6 +870,24 @@ func (p *scionPacketProcessor) parsePath() (processResult, error) { // TODO(lukedirtwalker) parameter problem invalid path? return processResult{}, err } + if p.infoField.Peer { + log.Debug("PEER", "PathMeta", p.path.PathMeta) + var err error + p.localPeer, err = p.path.GetCurrentPeerField() + if err != nil { + return processResult{}, err + } + idx := 1 + if p.infoField.ConsDir { + // ConsDir == true, the remotePeerField + // is above the current hop field + idx = -1 + } + p.remotePeer, err = p.path.GetPeerField(int(p.path.PathMeta.CurrHF) + idx) + if err != nil { + return processResult{}, err + } + } return processResult{}, nil } @@ -953,7 +995,8 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { pktEgressID := p.egressInterface() _, ih := p.d.internalNextHops[pktEgressID] _, eh := p.d.external[pktEgressID] - if !ih && !eh { + _, ph := p.d.remotePeerIF[pktEgressID] + if !ih && !eh && !ph { errCode := slayers.SCMPCodeUnknownHopFieldEgress if !p.infoField.ConsDir { errCode = slayers.SCMPCodeUnknownHopFieldIngress @@ -962,7 +1005,9 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { slayers.SCMPTypeParameterProblem, errCode, &slayers.SCMPParameterProblem{Pointer: p.currentHopPointer()}, - cannotRoute, + serrors.New("egress interface invalid", + "pkt_egress", pktEgressID), + //cannotRoute, ) } @@ -1033,8 +1078,8 @@ func (p *scionPacketProcessor) currentHopPointer() uint16 { } func (p *scionPacketProcessor) verifyCurrentMAC() (processResult, error) { - //FIXME //fullMac := path.FullMAC(p.mac, p.infoField, p.hopField, p.macBuffers.scionInput) + //XXX FIXME TODO LOBOTOMOIZED MAC VERIFICATION fullMac := p.hopField.Mac if subtle.ConstantTimeCompare(p.hopField.Mac[:path.MacLen], fullMac[:path.MacLen]) == 0 { return p.packSCMP( @@ -1051,6 +1096,8 @@ func (p *scionPacketProcessor) verifyCurrentMAC() (processResult, error) { } // Add the full MAC to the SCION packet processor, // such that EPIC does not need to recalculate it. + // p.cachedMac = fullMac + // XXX p.cachedMac = fullMac[:] return processResult{}, nil @@ -1081,7 +1128,8 @@ func (p *scionPacketProcessor) processEgress() error { } } - //FIXME + //FIXME not incrementing the path but returning early seems to have worked but + //no idea why if p.infoField.Peer { return nil } @@ -1133,7 +1181,9 @@ func (p *scionPacketProcessor) ingressInterface() uint16 { func (p *scionPacketProcessor) egressInterface() uint16 { if p.infoField.Peer { - return 42 + //FIXME + return p.localPeer.PeerIF + //return p.d.remotePeerIF[p.remotePeer.PeerIF] } if p.infoField.ConsDir { return p.hopField.ConsEgress @@ -1322,9 +1372,7 @@ func (p *scionPacketProcessor) process() (processResult, error) { } egressID := p.egressInterface() - if egressID == 0 { - egressID = 42 - } + if c, ok := p.d.external[egressID]; ok { if err := p.processEgress(); err != nil { return processResult{}, err @@ -1340,8 +1388,7 @@ func (p *scionPacketProcessor) process() (processResult, error) { if !p.infoField.ConsDir { errCode = slayers.SCMPCodeUnknownHopFieldIngress } - - log.Debug("DATAPLANE", "p", p.d.external, "egressID", egressID) + log.Debug("SCMP", "egressID", egressID) return p.packSCMP( slayers.SCMPTypeParameterProblem, errCode, diff --git a/tools/topology/topo.py b/tools/topology/topo.py index 46f8c9a58f..938090ad52 100644 --- a/tools/topology/topo.py +++ b/tools/topology/topo.py @@ -305,6 +305,11 @@ def _gen_br_entry(self, local, l_ifid, remote, r_ifid, remote_type, attrs, r_ifid, link_addr_type) intl_addr = self._reg_addr(local, local_br + "_internal", addr_type) + + intf = self._gen_br_intf(remote, public_addr, remote_addr, attrs, remote_type) + if intf['link_to'] == 'peer': + intf['remote_if_id'] = r_ifid + if self.topo_dicts[local]["border_routers"].get(local_br) is None: intl_port = 30042 if not self.args.docker: @@ -313,14 +318,14 @@ def _gen_br_entry(self, local, l_ifid, remote, r_ifid, remote_type, attrs, self.topo_dicts[local]["border_routers"][local_br] = { 'internal_addr': join_host_port(intl_addr.ip, intl_port), 'interfaces': { - l_ifid: self._gen_br_intf(remote, public_addr, remote_addr, attrs, remote_type) + l_ifid: intf } } else: # There is already a BR entry, add interface - intf = self._gen_br_intf(remote, public_addr, remote_addr, attrs, remote_type) self.topo_dicts[local]["border_routers"][local_br]['interfaces'][l_ifid] = intf + def _gen_br_intf(self, remote, public_addr, remote_addr, attrs, remote_type): return { 'underlay': { diff --git a/topology/peering-test.topo b/topology/peering-test.topo new file mode 100644 index 0000000000..40aed961ff --- /dev/null +++ b/topology/peering-test.topo @@ -0,0 +1,40 @@ +--- # Topology demonstrating peering, IPv4 Only +ASes: + "1-ff00:0:110": + core: true + voting: true + authoritative: true + issuing: true + mtu: 1400 + "1-ff00:0:111": + cert_issuer: 1-ff00:0:110 + "1-ff00:0:112": + cert_issuer: 1-ff00:0:110 + "1-ff00:0:113": + cert_issuer: 1-ff00:0:110 + "1-ff00:0:114": + cert_issuer: 1-ff00:0:110 + "2-ff00:0:210": + core: true + voting: true + authoritative: true + issuing: true + mtu: 1400 + "2-ff00:0:211": + cert_issuer: 2-ff00:0:210 + "2-ff00:0:212": + cert_issuer: 2-ff00:0:210 + "2-ff00:0:213": + cert_issuer: 2-ff00:0:210 + + +links: + - {a: "1-ff00:0:110#1", b: "1-ff00:0:111#41", linkAtoB: CHILD, mtu: 1280} + - {a: "1-ff00:0:110#2", b: "1-ff00:0:112#1", linkAtoB: CHILD, bw: 500} + - {a: "1-ff00:0:112#3", b: "1-ff00:0:113#3", linkAtoB: CHILD, bw: 500} + - {a: "1-ff00:0:113#4", b: "1-ff00:0:114#4", linkAtoB: CHILD, bw: 500} + - {a: "1-ff00:0:110#3", b: "2-ff00:0:210#3", linkAtoB: CORE} + - {a: "2-ff00:0:210#1", b: "2-ff00:0:211#3", linkAtoB: CHILD, mtu: 1280} + - {a: "2-ff00:0:210#2", b: "2-ff00:0:212#7", linkAtoB: CHILD, bw: 500} + - {a: "2-ff00:0:212#3", b: "2-ff00:0:213#3", linkAtoB: CHILD, bw: 500} + - {a: "1-ff00:0:112#42", b: "2-ff00:0:212#23", linkAtoB: PEER} From 2de4956e396413bf089363784ba1967aaa06a5a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorben=20Kr=C3=BCger?= Date: Thu, 24 Nov 2022 18:07:14 +0100 Subject: [PATCH 03/25] incorporate oncilla's feedback --- pkg/slayers/path/BUILD.bazel | 1 - pkg/slayers/path/peerfield.go | 75 ---------------------------- pkg/slayers/path/scion/base.go | 3 +- pkg/slayers/path/scion/raw.go | 29 ----------- private/path/combinator/graph.go | 3 ++ router/connector.go | 5 -- router/dataplane.go | 84 +++++++++++++++++++------------- tools/topology/topo.py | 1 - 8 files changed, 55 insertions(+), 146 deletions(-) delete mode 100644 pkg/slayers/path/peerfield.go diff --git a/pkg/slayers/path/BUILD.bazel b/pkg/slayers/path/BUILD.bazel index f6583b9cc2..d580caa581 100644 --- a/pkg/slayers/path/BUILD.bazel +++ b/pkg/slayers/path/BUILD.bazel @@ -7,7 +7,6 @@ go_library( "infofield.go", "mac.go", "path.go", - "peerfield.go", ], importpath = "github.com/scionproto/scion/pkg/slayers/path", visibility = ["//visibility:public"], diff --git a/pkg/slayers/path/peerfield.go b/pkg/slayers/path/peerfield.go deleted file mode 100644 index 5bc9ffdb78..0000000000 --- a/pkg/slayers/path/peerfield.go +++ /dev/null @@ -1,75 +0,0 @@ -// Copyright 2022 Thorben Krüger -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package path - -import ( - "encoding/binary" - - "github.com/scionproto/scion/pkg/private/serrors" -) - -// PeerField is the HopField used in the SCION peer path type. -// -// The Peer Field has the following format: -// 0 1 2 3 -// 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 -// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ -// |r r r r r r I E| ExpTime | PeerIF | -// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ -// | EgressIF | | -// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + -// | MAC | -// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ -// -type PeerField struct { - Flags uint8 - // Timestamp + (1 + ExpTime) * (24*60*60)/256 - ExpTime uint8 - // PeerIF is the interface ID of the remote peer - PeerIF uint16 - // EgressIF is local egress interface ID. - EgressIF uint16 - // Mac is the 6-byte Message Authentication Code to authenticate the PeerField. - Mac [MacLen]byte -} - -// DecodeFromBytes populates the fields from a raw buffer. The buffer must be of length >= -// path.HopLen. -func (p *PeerField) DecodeFromBytes(raw []byte) error { - if len(raw) < HopLen { - return serrors.New("PeerField raw too short", "expected", HopLen, "actual", len(raw)) - } - p.Flags = raw[0] - p.ExpTime = raw[1] - p.PeerIF = binary.BigEndian.Uint16(raw[2:4]) - p.EgressIF = binary.BigEndian.Uint16(raw[4:6]) - copy(p.Mac[:], raw[6:6+MacLen]) - return nil -} - -// SerializeTo writes the fields into the provided buffer. The buffer must be of length >= -// path.HopLen. -func (p *PeerField) SerializeTo(b []byte) error { - if len(b) < HopLen { - return serrors.New("buffer for PeerField too short", "expected", MacLen, "actual", len(b)) - } - b[0] = p.Flags - b[1] = p.ExpTime - binary.BigEndian.PutUint16(b[2:4], p.PeerIF) - binary.BigEndian.PutUint16(b[4:6], p.EgressIF) - copy(b[6:6+MacLen], p.Mac[:]) - - return nil -} diff --git a/pkg/slayers/path/scion/base.go b/pkg/slayers/path/scion/base.go index 9089fdbc01..776999dda5 100644 --- a/pkg/slayers/path/scion/base.go +++ b/pkg/slayers/path/scion/base.go @@ -76,7 +76,8 @@ func (s *Base) IncPath() error { } if int(s.PathMeta.CurrHF) >= s.NumHops-1 { s.PathMeta.CurrHF = uint8(s.NumHops - 1) - return serrors.New(fmt.Sprintf("path already at end, currhf %d, numhops %d", s.PathMeta.CurrHF, s.NumHops)) + return serrors.New(fmt.Sprintf("path already at end, currhf %d, numhops %d", + s.PathMeta.CurrHF, s.NumHops)) } s.PathMeta.CurrHF++ // Update CurrINF diff --git a/pkg/slayers/path/scion/raw.go b/pkg/slayers/path/scion/raw.go index 860019d001..8591093774 100644 --- a/pkg/slayers/path/scion/raw.go +++ b/pkg/slayers/path/scion/raw.go @@ -163,35 +163,6 @@ func (s *Raw) IsFirstHop() bool { return s.PathMeta.CurrHF == 0 } -// GetPeerField returns the PeerField at a given index. -func (s *Raw) GetPeerField(idx int) (path.PeerField, error) { - if idx >= s.NumHops { - return path.PeerField{}, - serrors.New("PeerField index out of bounds", "max", s.NumHops-1, "actual", idx) - } - hopOffset := MetaLen + s.NumINF*path.InfoLen + idx*path.HopLen - hop := path.PeerField{} - if err := hop.DecodeFromBytes(s.Raw[hopOffset : hopOffset+path.HopLen]); err != nil { - return path.PeerField{}, err - } - return hop, nil -} - -// GetCurrentPeerField is a convenience method that returns the current hop field pointed to by the -// CurrHF index in the path meta header. -func (s *Raw) GetCurrentPeerField() (path.PeerField, error) { - return s.GetPeerField(int(s.PathMeta.CurrHF)) -} - -// SetPeerField updates the PeerField at a given index. -func (s *Raw) SetPeerField(hop path.PeerField, idx int) error { - if idx >= s.NumHops { - return serrors.New("PeerField index out of bounds", "max", s.NumHops-1, "actual", idx) - } - hopOffset := MetaLen + s.NumINF*path.InfoLen + idx*path.HopLen - return hop.SerializeTo(s.Raw[hopOffset : hopOffset+path.HopLen]) -} - // IsPenultimateHop returns whether the current hop is the penultimate hop on the path. func (s *Raw) IsPenultimateHop() bool { return int(s.PathMeta.CurrHF) == (s.NumHops - 2) diff --git a/private/path/combinator/graph.go b/private/path/combinator/graph.go index 6b90e4cba9..16b06128ff 100644 --- a/private/path/combinator/graph.go +++ b/private/path/combinator/graph.go @@ -496,6 +496,9 @@ func calculateBeta(se *solutionEdge) uint16 { } } else { index = len(se.segment.ASEntries) - 1 + if index == se.edge.Shortcut && se.edge.Peer != 0 { + index++ + } } beta := se.segment.Info.SegmentID for i := 0; i < index; i++ { diff --git a/router/connector.go b/router/connector.go index 7f7c35cf1f..d0df2eed86 100644 --- a/router/connector.go +++ b/router/connector.go @@ -95,11 +95,6 @@ func (c *Connector) AddExternalInterface(localIfID common.IFIDType, link control if err := c.DataPlane.AddLinkType(intf, link.LinkTo); err != nil { return serrors.WrapStr("adding link type", err, "if_id", localIfID) } - if link.LinkTo == topology.Peer { - if err := c.DataPlane.AddRemotePeer(intf, uint16(link.Remote.IFID)); err != nil { - return serrors.WrapStr("adding peering link", err, "local", localIfID, "remote", link.Remote.IFID) - } - } if err := c.DataPlane.AddNeighborIA(intf, link.Remote.IA); err != nil { return serrors.WrapStr("adding neighboring IA", err, "if_id", localIfID) } diff --git a/router/dataplane.go b/router/dataplane.go index b47920d65c..5326fc5924 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -613,6 +613,7 @@ func (p *scionPacketProcessor) reset() error { p.hopField = path.HopField{} p.infoField = path.InfoField{} p.segmentChange = false + p.peering = false if err := p.buffer.Clear(); err != nil { return serrors.WrapStr("Failed to clear buffer", err) } @@ -800,15 +801,13 @@ type scionPacketProcessor struct { path *scion.Raw // hopField is the current hopField field, is updated during processing. hopField path.HopField - // localPeer is the local peerField field, is updated during processing. - localPeer path.PeerField - // remotePeer is the remote peerField field, is updated during processing. - remotePeer path.PeerField // infoField is the current infoField field, is updated during processing. infoField path.InfoField // segmentChange indicates if the path segment was changed during processing. segmentChange bool + // peering indicates that the packet is inside of a peering AS + peering bool // cachedMac contains the full 16 bytes of the MAC. Will be set during processing. // For a hop performing an Xover, it is the MAC corresponding to the down segment. @@ -870,7 +869,8 @@ func (p *scionPacketProcessor) parsePath() (processResult, error) { // TODO(lukedirtwalker) parameter problem invalid path? return processResult{}, err } - if p.infoField.Peer { + + /*if p.infoField.Peer { log.Debug("PEER", "PathMeta", p.path.PathMeta) var err error p.localPeer, err = p.path.GetCurrentPeerField() @@ -887,10 +887,38 @@ func (p *scionPacketProcessor) parsePath() (processResult, error) { if err != nil { return processResult{}, err } - } + }*/ + return processResult{}, nil } +func (p *scionPacketProcessor) determinePeer() (processResult, error) { + if !p.infoField.Peer { + return processResult{}, nil + } + + // TODO: proper error + err := serrors.New("TODO: segment length error (peering)") + result := processResult{} + + if p.path.PathMeta.SegLen[0] == 0 { + return result, err + } + if p.path.PathMeta.SegLen[1] == 0 { + return result, err + } + if p.path.PathMeta.SegLen[2] != 0 { + return result, err + } + + // The peer hop fields are the last hop field on the first path + // segment and the first hop field of the second path segment. + currHF := p.path.PathMeta.CurrHF + segLen := p.path.PathMeta.SegLen[0] + p.peering = currHF == segLen-1 || currHF == segLen + return result, nil +} + func (p *scionPacketProcessor) validateHopExpiry() (processResult, error) { expiration := util.SecsToTime(p.infoField.Timestamp). Add(path.ExpTimeToDuration(p.hopField.ExpTime)) @@ -995,8 +1023,7 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { pktEgressID := p.egressInterface() _, ih := p.d.internalNextHops[pktEgressID] _, eh := p.d.external[pktEgressID] - _, ph := p.d.remotePeerIF[pktEgressID] - if !ih && !eh && !ph { + if !ih && !eh { errCode := slayers.SCMPCodeUnknownHopFieldEgress if !p.infoField.ConsDir { errCode = slayers.SCMPCodeUnknownHopFieldIngress @@ -1005,9 +1032,7 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { slayers.SCMPTypeParameterProblem, errCode, &slayers.SCMPParameterProblem{Pointer: p.currentHopPointer()}, - serrors.New("egress interface invalid", - "pkt_egress", pktEgressID), - //cannotRoute, + cannotRoute, ) } @@ -1042,7 +1067,11 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { return processResult{}, nil case ingress == topology.Child && egress == topology.Child: return processResult{}, nil - case egress == topology.Peer: + case ingress == topology.Unset && egress == topology.Peer: + return processResult{}, nil + case ingress == topology.Child && egress == topology.Peer: + return processResult{}, nil + case ingress == topology.Peer && egress == topology.Child: return processResult{}, nil default: return p.packSCMP( @@ -1058,7 +1087,7 @@ func (p *scionPacketProcessor) updateNonConsDirIngressSegID() error { // against construction dir the ingress router updates the SegID, ifID == 0 // means this comes from this AS itself, so nothing has to be done. // For packets destined to peer links this shouldn't be updated. - if !p.infoField.Peer && !p.infoField.ConsDir && p.ingressID != 0 { + if !p.infoField.ConsDir && p.ingressID != 0 && !p.peering { p.infoField.UpdateSegID(p.hopField.Mac) if err := p.path.SetInfoField(p.infoField, int(p.path.PathMeta.CurrINF)); err != nil { return serrors.WrapStr("update info field", err) @@ -1078,9 +1107,7 @@ func (p *scionPacketProcessor) currentHopPointer() uint16 { } func (p *scionPacketProcessor) verifyCurrentMAC() (processResult, error) { - //fullMac := path.FullMAC(p.mac, p.infoField, p.hopField, p.macBuffers.scionInput) - //XXX FIXME TODO LOBOTOMOIZED MAC VERIFICATION - fullMac := p.hopField.Mac + fullMac := path.FullMAC(p.mac, p.infoField, p.hopField, p.macBuffers.scionInput) if subtle.ConstantTimeCompare(p.hopField.Mac[:path.MacLen], fullMac[:path.MacLen]) == 0 { return p.packSCMP( slayers.SCMPTypeParameterProblem, @@ -1096,9 +1123,7 @@ func (p *scionPacketProcessor) verifyCurrentMAC() (processResult, error) { } // Add the full MAC to the SCION packet processor, // such that EPIC does not need to recalculate it. - // p.cachedMac = fullMac - // XXX - p.cachedMac = fullMac[:] + p.cachedMac = fullMac return processResult{}, nil } @@ -1119,8 +1144,8 @@ func (p *scionPacketProcessor) resolveInbound() (*net.UDPAddr, processResult, er func (p *scionPacketProcessor) processEgress() error { // we are the egress router and if we go in construction direction we - // need to update the SegID. - if p.infoField.ConsDir { + // need to update the SegID (unless we received the packet via peering link) + if p.infoField.ConsDir && !p.peering { p.infoField.UpdateSegID(p.hopField.Mac) if err := p.path.SetInfoField(p.infoField, int(p.path.PathMeta.CurrINF)); err != nil { // TODO parameter problem invalid path @@ -1128,12 +1153,6 @@ func (p *scionPacketProcessor) processEgress() error { } } - //FIXME not incrementing the path but returning early seems to have worked but - //no idea why - if p.infoField.Peer { - return nil - } - if err := p.path.IncPath(); err != nil { // TODO parameter problem invalid path return serrors.WrapStr("incrementing path", err) @@ -1180,11 +1199,6 @@ func (p *scionPacketProcessor) ingressInterface() uint16 { } func (p *scionPacketProcessor) egressInterface() uint16 { - if p.infoField.Peer { - //FIXME - return p.localPeer.PeerIF - //return p.d.remotePeerIF[p.remotePeer.PeerIF] - } if p.infoField.ConsDir { return p.hopField.ConsEgress } @@ -1315,6 +1329,9 @@ func (p *scionPacketProcessor) process() (processResult, error) { if r, err := p.validateIngressID(); err != nil { return r, err } + if r, err := p.determinePeer(); err != nil { + return r, err + } if r, err := p.validatePktLen(); err != nil { return r, err } @@ -1346,7 +1363,7 @@ func (p *scionPacketProcessor) process() (processResult, error) { // Outbound: pkts leaving the local IA. // BRTransit: pkts leaving from the same BR different interface. - if p.path.IsXover() { + if p.path.IsXover() && !p.peering { if r, err := p.doXover(); err != nil { return r, err } @@ -1388,7 +1405,6 @@ func (p *scionPacketProcessor) process() (processResult, error) { if !p.infoField.ConsDir { errCode = slayers.SCMPCodeUnknownHopFieldIngress } - log.Debug("SCMP", "egressID", egressID) return p.packSCMP( slayers.SCMPTypeParameterProblem, errCode, diff --git a/tools/topology/topo.py b/tools/topology/topo.py index 938090ad52..e75b1a9312 100644 --- a/tools/topology/topo.py +++ b/tools/topology/topo.py @@ -325,7 +325,6 @@ def _gen_br_entry(self, local, l_ifid, remote, r_ifid, remote_type, attrs, # There is already a BR entry, add interface self.topo_dicts[local]["border_routers"][local_br]['interfaces'][l_ifid] = intf - def _gen_br_intf(self, remote, public_addr, remote_addr, attrs, remote_type): return { 'underlay': { From 1408c6918303d472763a0fc626d87b57f9215595 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorben=20Kr=C3=BCger?= Date: Wed, 30 Nov 2022 17:00:20 +0100 Subject: [PATCH 04/25] peering: reenabled pathprobe, minor cleanup --- control/beaconing/extender.go | 3 --- private/app/path/pathprobe/paths.go | 4 +--- router/connector.go | 1 - router/dataplane.go | 23 ----------------------- 4 files changed, 1 insertion(+), 30 deletions(-) diff --git a/control/beaconing/extender.go b/control/beaconing/extender.go index f7e5cf3930..2c02c5e35f 100644 --- a/control/beaconing/extender.go +++ b/control/beaconing/extender.go @@ -89,8 +89,6 @@ func (s *DefaultExtender) Extend( if err != nil { return serrors.WrapStr("creating hop entry", err) } - // TODO: peer entries are missing here! - peerBeta := extractBeta(pseg) ^ binary.BigEndian.Uint16(hopEntry.HopField.MAC[:2]) peerEntries, epicPeerMacs, err := s.createPeerEntries(egress, peers, ts, peerBeta) if err != nil { @@ -186,7 +184,6 @@ func (s *DefaultExtender) createPeerEntry(ingress, egress uint16, ts time.Time, return seg.PeerEntry{}, nil, serrors.WrapStr("checking remote ingress interface", err, "ingress_interface", ingress) } - hopF, epicMac := s.createHopF(ingress, egress, ts, beta) return seg.PeerEntry{ PeerMTU: int(remoteInMTU), diff --git a/private/app/path/pathprobe/paths.go b/private/app/path/pathprobe/paths.go index 0f54230c5a..5a64f601c8 100644 --- a/private/app/path/pathprobe/paths.go +++ b/private/app/path/pathprobe/paths.go @@ -186,9 +186,7 @@ func (p Prober) GetStatuses(ctx context.Context, paths []snet.Path, continue } pathsPerIP[localIP.String()] = append(pathsPerIP[localIP.String()], path) - //FIXME - //addStatus(PathKey(path), Status{Status: StatusTimeout, LocalIP: localIP}) - addStatus(PathKey(path), Status{Status: StatusAlive, LocalIP: localIP}) + addStatus(PathKey(path), Status{Status: StatusTimeout, LocalIP: localIP}) } // Sequence number for the sent traceroute packets. diff --git a/router/connector.go b/router/connector.go index d0df2eed86..f2bd767213 100644 --- a/router/connector.go +++ b/router/connector.go @@ -91,7 +91,6 @@ func (c *Connector) AddExternalInterface(localIfID common.IFIDType, link control if !c.ia.Equal(link.Local.IA) { return serrors.WithCtx(errMultiIA, "current", c.ia, "new", link.Local.IA) } - // FIXME, maybe change signature of AddLinkType to include remote ID if err := c.DataPlane.AddLinkType(intf, link.LinkTo); err != nil { return serrors.WrapStr("adding link type", err, "if_id", localIfID) } diff --git a/router/dataplane.go b/router/dataplane.go index 5326fc5924..9fe1156a62 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -801,7 +801,6 @@ type scionPacketProcessor struct { path *scion.Raw // hopField is the current hopField field, is updated during processing. hopField path.HopField - // infoField is the current infoField field, is updated during processing. infoField path.InfoField // segmentChange indicates if the path segment was changed during processing. @@ -869,26 +868,6 @@ func (p *scionPacketProcessor) parsePath() (processResult, error) { // TODO(lukedirtwalker) parameter problem invalid path? return processResult{}, err } - - /*if p.infoField.Peer { - log.Debug("PEER", "PathMeta", p.path.PathMeta) - var err error - p.localPeer, err = p.path.GetCurrentPeerField() - if err != nil { - return processResult{}, err - } - idx := 1 - if p.infoField.ConsDir { - // ConsDir == true, the remotePeerField - // is above the current hop field - idx = -1 - } - p.remotePeer, err = p.path.GetPeerField(int(p.path.PathMeta.CurrHF) + idx) - if err != nil { - return processResult{}, err - } - }*/ - return processResult{}, nil } @@ -1152,7 +1131,6 @@ func (p *scionPacketProcessor) processEgress() error { return serrors.WrapStr("update info field", err) } } - if err := p.path.IncPath(); err != nil { // TODO parameter problem invalid path return serrors.WrapStr("incrementing path", err) @@ -1389,7 +1367,6 @@ func (p *scionPacketProcessor) process() (processResult, error) { } egressID := p.egressInterface() - if c, ok := p.d.external[egressID]; ok { if err := p.processEgress(); err != nil { return processResult{}, err From a3550a0c542f51a7b069096c2c4c2583ec5e18c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorben=20Kr=C3=BCger?= Date: Fri, 2 Dec 2022 19:13:04 +0100 Subject: [PATCH 05/25] peering: improve variable names --- pkg/slayers/path/scion/base.go | 3 +-- private/topology/json/json.go | 2 +- router/dataplane.go | 25 ++++++++++++------------- tools/topology/topo.py | 2 +- 4 files changed, 15 insertions(+), 17 deletions(-) diff --git a/pkg/slayers/path/scion/base.go b/pkg/slayers/path/scion/base.go index 776999dda5..6d57d2c3f6 100644 --- a/pkg/slayers/path/scion/base.go +++ b/pkg/slayers/path/scion/base.go @@ -76,8 +76,7 @@ func (s *Base) IncPath() error { } if int(s.PathMeta.CurrHF) >= s.NumHops-1 { s.PathMeta.CurrHF = uint8(s.NumHops - 1) - return serrors.New(fmt.Sprintf("path already at end, currhf %d, numhops %d", - s.PathMeta.CurrHF, s.NumHops)) + return serrors.New("path already at end", "curr_hf", s.PathMeta.CurrHF, "num_hops", s.NumHops) } s.PathMeta.CurrHF++ // Update CurrINF diff --git a/private/topology/json/json.go b/private/topology/json/json.go index 7e55348f37..d07de43a01 100644 --- a/private/topology/json/json.go +++ b/private/topology/json/json.go @@ -112,7 +112,7 @@ type BRInterface struct { LinkTo string `json:"link_to"` MTU int `json:"mtu"` BFD *BFD `json:"bfd,omitempty"` - RemoteIFID common.IFIDType `json:"remote_if_id,omitempty"` + RemoteIFID common.IFIDType `json:"remote_interface_id,omitempty"` } // Underlay is the underlay information for a BR interface. diff --git a/router/dataplane.go b/router/dataplane.go index 9fe1156a62..9754f948f5 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -98,7 +98,7 @@ type DataPlane struct { external map[uint16]BatchConn linkTypes map[uint16]topology.LinkType neighborIAs map[uint16]addr.IA - remotePeerIF map[uint16]uint16 + peerInterfaces map[uint16]uint16 internal BatchConn internalIP *net.IPAddr internalNextHops map[uint16]*net.UDPAddr @@ -270,17 +270,17 @@ func (d *DataPlane) AddLinkType(ifID uint16, linkTo topology.LinkType) error { // interface ID. If the link type for the given ID is already set to // a different type, this method will return an error. This can only // be called on a not yet running dataplane. -func (d *DataPlane) AddRemotePeer(localifID, remoteifID uint16) error { - if t, ok := d.linkTypes[localifID]; ok && t != topology.Peer { +func (d *DataPlane) AddRemotePeer(local, remote uint16) error { + if t, ok := d.linkTypes[local]; ok && t != topology.Peer { return serrors.WithCtx(unsupportedPathType, "type", t) } - if _, exists := d.remotePeerIF[localifID]; exists { - return serrors.WithCtx(alreadySet, "localifID", localifID) + if _, exists := d.peerInterfaces[local]; exists { + return serrors.WithCtx(alreadySet, "local_interface", local) } - if d.remotePeerIF == nil { - d.remotePeerIF = make(map[uint16]uint16) + if d.peerInterfaces == nil { + d.peerInterfaces = make(map[uint16]uint16) } - d.remotePeerIF[localifID] = remoteifID + d.peerInterfaces[local] = remote return nil } @@ -878,16 +878,15 @@ func (p *scionPacketProcessor) determinePeer() (processResult, error) { // TODO: proper error err := serrors.New("TODO: segment length error (peering)") - result := processResult{} if p.path.PathMeta.SegLen[0] == 0 { - return result, err + return processResult{}, err } if p.path.PathMeta.SegLen[1] == 0 { - return result, err + return processResult{}, err } if p.path.PathMeta.SegLen[2] != 0 { - return result, err + return processResult{}, err } // The peer hop fields are the last hop field on the first path @@ -895,7 +894,7 @@ func (p *scionPacketProcessor) determinePeer() (processResult, error) { currHF := p.path.PathMeta.CurrHF segLen := p.path.PathMeta.SegLen[0] p.peering = currHF == segLen-1 || currHF == segLen - return result, nil + return processResult{}, nil } func (p *scionPacketProcessor) validateHopExpiry() (processResult, error) { diff --git a/tools/topology/topo.py b/tools/topology/topo.py index e75b1a9312..a63e223432 100644 --- a/tools/topology/topo.py +++ b/tools/topology/topo.py @@ -308,7 +308,7 @@ def _gen_br_entry(self, local, l_ifid, remote, r_ifid, remote_type, attrs, intf = self._gen_br_intf(remote, public_addr, remote_addr, attrs, remote_type) if intf['link_to'] == 'peer': - intf['remote_if_id'] = r_ifid + intf['remote_interface_id'] = r_ifid if self.topo_dicts[local]["border_routers"].get(local_br) is None: intl_port = 30042 From bb001ff915052b7cbde2d8d03e8235983ca03feb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorben=20Kr=C3=BCger?= Date: Fri, 9 Dec 2022 14:15:46 +0100 Subject: [PATCH 06/25] peering: fix traceroute, fix crossover checks --- pkg/slayers/path/scion/base.go | 10 ++++++++-- router/dataplane.go | 14 ++++++++------ scion/traceroute/traceroute.go | 5 ++++- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/pkg/slayers/path/scion/base.go b/pkg/slayers/path/scion/base.go index 6d57d2c3f6..b2a7812239 100644 --- a/pkg/slayers/path/scion/base.go +++ b/pkg/slayers/path/scion/base.go @@ -76,7 +76,9 @@ func (s *Base) IncPath() error { } if int(s.PathMeta.CurrHF) >= s.NumHops-1 { s.PathMeta.CurrHF = uint8(s.NumHops - 1) - return serrors.New("path already at end", "curr_hf", s.PathMeta.CurrHF, "num_hops", s.NumHops) + return serrors.New("path already at end", + "curr_hf", s.PathMeta.CurrHF, + "num_hops", s.NumHops) } s.PathMeta.CurrHF++ // Update CurrINF @@ -84,7 +86,11 @@ func (s *Base) IncPath() error { return nil } -// IsXover returns whether we are at a crossover point. +// IsXover returns whether we are at a crossover point. This includes +// all segment switches, even over a peering link. Note that handling +// of a regular segment switch and handling of a segment switch over a +// peering link are fundamentally different. To distinguish the two, +// you will need to extract the information from the info field. func (s *Base) IsXover() bool { return s.PathMeta.CurrHF+1 < uint8(s.NumHops) && s.PathMeta.CurrINF != s.infIndexForHF(s.PathMeta.CurrHF+1) diff --git a/router/dataplane.go b/router/dataplane.go index 9754f948f5..9fd90d889a 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -1091,12 +1091,14 @@ func (p *scionPacketProcessor) verifyCurrentMAC() (processResult, error) { slayers.SCMPTypeParameterProblem, slayers.SCMPCodeInvalidHopFieldMAC, &slayers.SCMPParameterProblem{Pointer: p.currentHopPointer()}, - serrors.New("MAC verification failed", "expected", fmt.Sprintf( - "%x", fullMac[:path.MacLen]), + serrors.New("MAC verification failed", + "expected", fmt.Sprintf("%x", fullMac[:path.MacLen]), "actual", fmt.Sprintf("%x", p.hopField.Mac[:path.MacLen]), "cons_dir", p.infoField.ConsDir, - "if_id", p.ingressID, "curr_inf", p.path.PathMeta.CurrINF, - "curr_hf", p.path.PathMeta.CurrHF, "seg_id", p.infoField.SegID), + "if_id", p.ingressID, + "curr_inf", p.path.PathMeta.CurrINF, + "curr_hf", p.path.PathMeta.CurrHF, + "seg_id", p.infoField.SegID), ) } // Add the full MAC to the SCION packet processor, @@ -1646,7 +1648,7 @@ func (p *scionPacketProcessor) prepareSCMP( revPath := revPathTmp.(*scion.Decoded) // Revert potential path segment switches that were done during processing. - if revPath.IsXover() { + if revPath.IsXover() && !p.peering { if err := revPath.IncPath(); err != nil { return nil, serrors.Wrap(cannotRoute, err, "details", "reverting cross over for SCMP") } @@ -1656,7 +1658,7 @@ func (p *scionPacketProcessor) prepareSCMP( _, external := p.d.external[p.ingressID] if external { infoField := &revPath.InfoFields[revPath.PathMeta.CurrINF] - if infoField.ConsDir { + if infoField.ConsDir && !p.peering { hopField := revPath.HopFields[revPath.PathMeta.CurrHF] infoField.UpdateSegID(hopField.Mac) } diff --git a/scion/traceroute/traceroute.go b/scion/traceroute/traceroute.go index 8c9c75251c..dca772423d 100644 --- a/scion/traceroute/traceroute.go +++ b/scion/traceroute/traceroute.go @@ -162,7 +162,10 @@ func (t *tracerouter) Traceroute(ctx context.Context) (Stats, error) { t.updateHandler(u) } } - xover := idxPath.IsXover() + // Peering links do not count as regular cross + // overs. For peering links we probe all interfaces on + // the path. + xover := idxPath.IsXover() && !info.Peer // The last hop of the path isn't probed, only the ingress interface is // relevant. // At a crossover (segment change) only the ingress interface is From 6245ddb61f80e5034495ae0f50ba9a7dda1caa8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorben=20Kr=C3=BCger?= Date: Fri, 9 Dec 2022 15:47:07 +0100 Subject: [PATCH 07/25] peering (debug): update topology to intentionally trigger mac validation bug --- topology/peering-test.topo | 1 + 1 file changed, 1 insertion(+) diff --git a/topology/peering-test.topo b/topology/peering-test.topo index 40aed961ff..b328739484 100644 --- a/topology/peering-test.topo +++ b/topology/peering-test.topo @@ -33,6 +33,7 @@ links: - {a: "1-ff00:0:110#2", b: "1-ff00:0:112#1", linkAtoB: CHILD, bw: 500} - {a: "1-ff00:0:112#3", b: "1-ff00:0:113#3", linkAtoB: CHILD, bw: 500} - {a: "1-ff00:0:113#4", b: "1-ff00:0:114#4", linkAtoB: CHILD, bw: 500} + - {a: "1-ff00:0:112#47", b: "1-ff00:0:114#11", linkAtoB: PEER} - {a: "1-ff00:0:110#3", b: "2-ff00:0:210#3", linkAtoB: CORE} - {a: "2-ff00:0:210#1", b: "2-ff00:0:211#3", linkAtoB: CHILD, mtu: 1280} - {a: "2-ff00:0:210#2", b: "2-ff00:0:212#7", linkAtoB: CHILD, bw: 500} From aa60d8b472a8a9637199add8dd3fda8ffaca7c91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorben=20Kr=C3=BCger?= Date: Mon, 23 Jan 2023 17:44:25 +0100 Subject: [PATCH 08/25] fixup: delete merge artefact --- router/dataplane.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/router/dataplane.go b/router/dataplane.go index 9fd90d889a..21adce8bf0 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -1302,13 +1302,13 @@ func (p *scionPacketProcessor) process() (processResult, error) { if r, err := p.parsePath(); err != nil { return r, err } - if r, err := p.validateHopExpiry(); err != nil { + if r, err := p.determinePeer(); err != nil { return r, err } - if r, err := p.validateIngressID(); err != nil { + if r, err := p.validateHopExpiry(); err != nil { return r, err } - if r, err := p.determinePeer(); err != nil { + if r, err := p.validateIngressID(); err != nil { return r, err } if r, err := p.validatePktLen(); err != nil { From b327306ecd867b3ea0919194ec627798dc1a0642 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorben=20Kr=C3=BCger?= Date: Mon, 23 Jan 2023 17:45:09 +0100 Subject: [PATCH 09/25] peering: add topology that demonstrates the issue --- topology/peering-test-multi.topo | 41 ++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 topology/peering-test-multi.topo diff --git a/topology/peering-test-multi.topo b/topology/peering-test-multi.topo new file mode 100644 index 0000000000..b328739484 --- /dev/null +++ b/topology/peering-test-multi.topo @@ -0,0 +1,41 @@ +--- # Topology demonstrating peering, IPv4 Only +ASes: + "1-ff00:0:110": + core: true + voting: true + authoritative: true + issuing: true + mtu: 1400 + "1-ff00:0:111": + cert_issuer: 1-ff00:0:110 + "1-ff00:0:112": + cert_issuer: 1-ff00:0:110 + "1-ff00:0:113": + cert_issuer: 1-ff00:0:110 + "1-ff00:0:114": + cert_issuer: 1-ff00:0:110 + "2-ff00:0:210": + core: true + voting: true + authoritative: true + issuing: true + mtu: 1400 + "2-ff00:0:211": + cert_issuer: 2-ff00:0:210 + "2-ff00:0:212": + cert_issuer: 2-ff00:0:210 + "2-ff00:0:213": + cert_issuer: 2-ff00:0:210 + + +links: + - {a: "1-ff00:0:110#1", b: "1-ff00:0:111#41", linkAtoB: CHILD, mtu: 1280} + - {a: "1-ff00:0:110#2", b: "1-ff00:0:112#1", linkAtoB: CHILD, bw: 500} + - {a: "1-ff00:0:112#3", b: "1-ff00:0:113#3", linkAtoB: CHILD, bw: 500} + - {a: "1-ff00:0:113#4", b: "1-ff00:0:114#4", linkAtoB: CHILD, bw: 500} + - {a: "1-ff00:0:112#47", b: "1-ff00:0:114#11", linkAtoB: PEER} + - {a: "1-ff00:0:110#3", b: "2-ff00:0:210#3", linkAtoB: CORE} + - {a: "2-ff00:0:210#1", b: "2-ff00:0:211#3", linkAtoB: CHILD, mtu: 1280} + - {a: "2-ff00:0:210#2", b: "2-ff00:0:212#7", linkAtoB: CHILD, bw: 500} + - {a: "2-ff00:0:212#3", b: "2-ff00:0:213#3", linkAtoB: CHILD, bw: 500} + - {a: "1-ff00:0:112#42", b: "2-ff00:0:212#23", linkAtoB: PEER} From f15cf5ca0abaf442e1db25607c8dfcbfa9a0cccc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorben=20Kr=C3=BCger?= Date: Mon, 23 Jan 2023 17:55:24 +0100 Subject: [PATCH 10/25] fixup: fix merge artifacts --- topology/peering-test.topo | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/topology/peering-test.topo b/topology/peering-test.topo index b328739484..3f52f7cfec 100644 --- a/topology/peering-test.topo +++ b/topology/peering-test.topo @@ -26,14 +26,13 @@ ASes: cert_issuer: 2-ff00:0:210 "2-ff00:0:213": cert_issuer: 2-ff00:0:210 - + links: - {a: "1-ff00:0:110#1", b: "1-ff00:0:111#41", linkAtoB: CHILD, mtu: 1280} - {a: "1-ff00:0:110#2", b: "1-ff00:0:112#1", linkAtoB: CHILD, bw: 500} - {a: "1-ff00:0:112#3", b: "1-ff00:0:113#3", linkAtoB: CHILD, bw: 500} - {a: "1-ff00:0:113#4", b: "1-ff00:0:114#4", linkAtoB: CHILD, bw: 500} - - {a: "1-ff00:0:112#47", b: "1-ff00:0:114#11", linkAtoB: PEER} - {a: "1-ff00:0:110#3", b: "2-ff00:0:210#3", linkAtoB: CORE} - {a: "2-ff00:0:210#1", b: "2-ff00:0:211#3", linkAtoB: CHILD, mtu: 1280} - {a: "2-ff00:0:210#2", b: "2-ff00:0:212#7", linkAtoB: CHILD, bw: 500} From 04e1db27f21ee666bd8d7159210904b3fcf1469d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorben=20Kr=C3=BCger?= Date: Mon, 23 Jan 2023 18:20:37 +0100 Subject: [PATCH 11/25] peering: egressID validation --- router/dataplane.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/router/dataplane.go b/router/dataplane.go index 21adce8bf0..1e14e09ca6 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -984,7 +984,9 @@ func (p *scionPacketProcessor) invalidDstIA() (processResult, error) { // this check prevents malicious end hosts in the local AS from bypassing the // SrcIA checks by disguising packets as transit traffic. func (p *scionPacketProcessor) validateTransitUnderlaySrc() (processResult, error) { - if p.path.IsFirstHop() || p.ingressID != 0 { + // XXX(benthor) disabling check in case of peering packet + // is probably insecture + if p.path.IsFirstHop() || p.ingressID != 0 || p.peering { // not a transit packet, nothing to check return processResult{}, nil } @@ -1027,6 +1029,10 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { return processResult{}, nil case ingress == topology.Parent && egress == topology.Child: return processResult{}, nil + // XXX(benthor) prevent "InvalidPath" from being raised + // not fully grasping the implications yet though + case ingress == topology.Peer || egress == topology.Peer: + return processResult{}, nil default: // malicious return p.packSCMP( slayers.SCMPTypeParameterProblem, From 7ac418072a6158d98a5af8bf1e0946c0db1dc466 Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Fri, 8 Sep 2023 16:35:34 +0200 Subject: [PATCH 12/25] peering: Added unit test case, comments, and reviewer-suggested polish. In passing, renamed segmentChanged to effectiveXover, since a segment change and a cross-over can happen without recieving the logical cross-over treatment. So, now the variable reflects what the boolean means: the so-called cross-over treatment was effectively applied. --- control/beaconing/extender.go | 16 +++- private/path/combinator/graph.go | 32 ++++++- router/dataplane.go | 49 ++++++----- router/dataplane_test.go | 138 +++++++++++++++++++++++++++++++ tools/topology/topo.py | 17 ++-- 5 files changed, 220 insertions(+), 32 deletions(-) diff --git a/control/beaconing/extender.go b/control/beaconing/extender.go index 2c02c5e35f..9bdc63149f 100644 --- a/control/beaconing/extender.go +++ b/control/beaconing/extender.go @@ -62,7 +62,7 @@ type DefaultExtender struct { EPIC bool } -// Extend extends the beacon with hop fields of the old format. +// Extend extends the beacon with hop fields. func (s *DefaultExtender) Extend( ctx context.Context, pseg *seg.PathSegment, @@ -89,6 +89,20 @@ func (s *DefaultExtender) Extend( if err != nil { return serrors.WrapStr("creating hop entry", err) } + + // The peer hop fields chain to the main hop field, just like any child hop field. + // The effect of this is that when a peer hop field is used in a path, both the + // peer hop field and its child are validated using the same SegID accumlator value: + // that originally intended for the child. + // + // The corrolary is that one cannot validate a hop field's MAC by looking at the + // parent hop field MAC when the parent is a peering hop field. This is ok: that + // is never done that way, it is always done by validating agains the SegID accumulator + // supplied by the previous router on the forwarding path. The forwarding code + // takes care of not updating that accumulator when a peering hop is traversed. + + // FIXME: why do we compute Beta(pseg) a second time? It hasn't changed since last + // time. peerBeta := extractBeta(pseg) ^ binary.BigEndian.Uint16(hopEntry.HopField.MAC[:2]) peerEntries, epicPeerMacs, err := s.createPeerEntries(egress, peers, ts, peerBeta) if err != nil { diff --git a/private/path/combinator/graph.go b/private/path/combinator/graph.go index 16b06128ff..eb092ca709 100644 --- a/private/path/combinator/graph.go +++ b/private/path/combinator/graph.go @@ -239,7 +239,11 @@ func (g *dmg) GetPaths(src, dst vertex) pathSolutionList { } // inputSegment is a local representation of a path segment that includes the -// segment's type. +// segment's type. The type (up down or core) indicates the role that this +// segment holds in a path solution. That is, in which order the hops would +// be used for building an actual forwarding path (e.g. from the end in the +// case of an UP segment). However, the hops within the referred PathSegment +// *always* remain in construction order. type inputSegment struct { *seg.PathSegment Type proto.PathSegType @@ -316,7 +320,11 @@ func (solution *pathSolution) Path() Path { var pathASEntries []seg.ASEntry // ASEntries that on the path, eventually in path order. var epicSegAuths [][]byte - // Go through each ASEntry, starting from the last one, until we + // Segments are in construction order, regardless of whether they're + // up or down segments. We traverse them FROM THE END. So, in reverse + // forwarding order for down segments and in forwarding order for + // up segments. + // We go through each ASEntry, starting from the last one until we // find a shortcut (which can be 0, meaning the end of the segment). asEntries := solEdge.segment.ASEntries for asEntryIdx := len(asEntries) - 1; asEntryIdx >= solEdge.edge.Shortcut; asEntryIdx-- { @@ -378,6 +386,9 @@ func (solution *pathSolution) Path() Path { } } + // Put the hops in forwarding order. Needed for down segments + // since we collected hops from the end, just like for up + // segments. if solEdge.segment.Type == proto.PathSegType_down { reverseHops(hops) reverseIntfs(intfs) @@ -487,10 +498,22 @@ func reverseEpicAuths(s [][]byte) { } func calculateBeta(se *solutionEdge) uint16 { + + // If this is a peer hop, we need to set beta[i] = beta[i+1]. That is, the SegID + // accumulator must correspond to the next (in construction order) hop. + // + // This is because this peering hop has a MAC that chains to its non-peering + // counterpart, the same as what the next hop (in construction order) chains to. + // So both this and the next hop are to be validated from the same SegID + // accumulator value: the one for the *next* hop, calculated on the regular + // non-peering segment. + // + // Note that, when traversing peer hops, the SegID accumulator is left untouched for the + // next router on the path to use. + var index int if se.segment.IsDownSeg() { index = se.edge.Shortcut - // If this is a peer, we need to set beta i+1. if se.edge.Peer != 0 { index++ } @@ -589,7 +612,8 @@ func validNextSeg(currSeg, nextSeg *inputSegment) bool { } // segment is a helper that represents a path segment during the conversion -// from the graph solution to the raw forwarding information. +// from the graph solution to the raw forwarding information. The hops should +// be in forwarding order. type segment struct { InfoField path.InfoField HopFields []path.HopField diff --git a/router/dataplane.go b/router/dataplane.go index fc780a1e9b..8b0bc63212 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -838,7 +838,7 @@ func (p *scionPacketProcessor) reset() error { p.path = nil p.hopField = path.HopField{} p.infoField = path.InfoField{} - p.segmentChange = false + p.effectiveXover = false p.peering = false if err := p.buffer.Clear(); err != nil { return serrors.WrapStr("Failed to clear buffer", err) @@ -1030,8 +1030,8 @@ type scionPacketProcessor struct { hopField path.HopField // infoField is the current infoField field, is updated during processing. infoField path.InfoField - // segmentChange indicates if the path segment was changed during processing. - segmentChange bool + // effectiveXover indicates if a cross-over segment change was done during processing. + effectiveXover bool // peering indicates that the packet is inside of a peering AS peering bool @@ -1122,7 +1122,9 @@ func (p *scionPacketProcessor) determinePeer() (processResult, error) { } // The peer hop fields are the last hop field on the first path - // segment and the first hop field of the second path segment. + // segment (at SegLen[0] - 1) and the first hop field of the second + // path segment (at SegLen[0]). The below check applies only + // because we already know this is a peering path. currHF := p.path.PathMeta.CurrHF segLen := p.path.PathMeta.SegLen[0] p.peering = currHF == segLen-1 || currHF == segLen @@ -1216,9 +1218,7 @@ func (p *scionPacketProcessor) invalidDstIA() (processResult, error) { // this check prevents malicious end hosts in the local AS from bypassing the // SrcIA checks by disguising packets as transit traffic. func (p *scionPacketProcessor) validateTransitUnderlaySrc() (processResult, error) { - // XXX(benthor) disabling check in case of peering packet - // is probably insecture - if p.path.IsFirstHop() || p.ingressID != 0 || p.peering { + if p.path.IsFirstHop() || p.ingressID != 0 { // not a transit packet, nothing to check return processResult{}, nil } @@ -1249,9 +1249,11 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { } ingress, egress := p.d.linkTypes[p.ingressID], p.d.linkTypes[pktEgressID] - if !p.segmentChange { + if !p.effectiveXover { // Check that the interface pair is valid within a single segment. // No check required if the packet is received from an internal interface. + // This case applies to peering hops as a peering hop isn't an effective + // cross-over (eventhough it is a segment change). switch { case p.ingressID == 0: return processResult{}, nil @@ -1261,9 +1263,9 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { return processResult{}, nil case ingress == topology.Parent && egress == topology.Child: return processResult{}, nil - // XXX(benthor) prevent "InvalidPath" from being raised - // not fully grasping the implications yet though - case ingress == topology.Peer || egress == topology.Peer: + case ingress == topology.Child && egress == topology.Peer: + return processResult{}, nil + case ingress == topology.Peer && egress == topology.Child: return processResult{}, nil default: // malicious return p.packSCMP( @@ -1276,6 +1278,9 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { } // Check that the interface pair is valid on a segment switch. // Having a segment change received from the internal interface is never valid. + // We should never see a peering link traversal either. If that happens + // treat it as a routing error (not sure if that can happen without an internal + // error, though). switch { case ingress == topology.Core && egress == topology.Child: return processResult{}, nil @@ -1285,10 +1290,6 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { return processResult{}, nil case ingress == topology.Unset && egress == topology.Peer: return processResult{}, nil - case ingress == topology.Child && egress == topology.Peer: - return processResult{}, nil - case ingress == topology.Peer && egress == topology.Child: - return processResult{}, nil default: return p.packSCMP( slayers.SCMPTypeParameterProblem, @@ -1361,8 +1362,11 @@ func (p *scionPacketProcessor) resolveInbound() (*net.UDPAddr, processResult, er } func (p *scionPacketProcessor) processEgress() error { - // we are the egress router and if we go in construction direction we - // need to update the SegID (unless we received the packet via peering link) + // We are the egress router and if we go in construction direction we + // need to update the SegID (unless we are effecting a peering hop). + // When we're at a peering hop, the SegID for this HOP and for the next + // are one and the same, both hops chain to the same parent. So do not + // update SegID. if p.infoField.ConsDir && !p.peering { p.infoField.UpdateSegID(p.hopField.Mac) if err := p.path.SetInfoField(p.infoField, int(p.path.PathMeta.CurrINF)); err != nil { @@ -1378,7 +1382,7 @@ func (p *scionPacketProcessor) processEgress() error { } func (p *scionPacketProcessor) doXover() (processResult, error) { - p.segmentChange = true + p.effectiveXover = true if err := p.path.IncPath(); err != nil { // TODO parameter problem invalid path return processResult{}, serrors.WrapStr("incrementing path", err) @@ -1398,7 +1402,7 @@ func (p *scionPacketProcessor) doXover() (processResult, error) { func (p *scionPacketProcessor) ingressInterface() uint16 { info := p.infoField hop := p.hopField - if p.path.IsFirstHopAfterXover() { + if !p.peering && p.path.IsFirstHopAfterXover() { var err error info, err = p.path.GetInfoField(int(p.path.PathMeta.CurrINF) - 1) if err != nil { // cannot be out of range @@ -1536,7 +1540,6 @@ func (p *scionPacketProcessor) validatePktLen() (processResult, error) { } func (p *scionPacketProcessor) process() (processResult, error) { - if r, err := p.parsePath(); err != nil { return r, err } @@ -1579,9 +1582,13 @@ func (p *scionPacketProcessor) process() (processResult, error) { // Outbound: pkts leaving the local IA. // BRTransit: pkts leaving from the same BR different interface. if p.path.IsXover() && !p.peering { + // An effective cross-over is a change of segment other than at + // a peering hop. if r, err := p.doXover(); err != nil { return r, err } + // doXover() has changed the current segment and hop field. + // We need to validate the new hop field. if r, err := p.validateHopExpiry(); err != nil { return r, serrors.WithCtx(err, "info", "after xover") } @@ -1884,6 +1891,8 @@ func (p *scionPacketProcessor) prepareSCMP( // Revert potential path segment switches that were done during processing. if revPath.IsXover() && !p.peering { + // An effective cross-over is a change of segment other than at + // a peering hop. if err := revPath.IncPath(); err != nil { return nil, serrors.Wrap(cannotRoute, err, "details", "reverting cross over for SCMP") } diff --git a/router/dataplane_test.go b/router/dataplane_test.go index 1eb6b6d1d4..d2bf2ffec8 100644 --- a/router/dataplane_test.go +++ b/router/dataplane_test.go @@ -675,6 +675,144 @@ func TestProcessPkt(t *testing.T) { srcInterface: 1, assertFunc: assert.NoError, }, + "brtransit peering consdir": { + prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { + return router.NewDP( + map[uint16]router.BatchConn{ + uint16(2): mock_router.NewMockBatchConn(ctrl), + }, + map[uint16]topology.LinkType{ + 1: topology.Peer, + 2: topology.Child, + }, + nil, nil, nil, xtest.MustParseIA("1-ff00:0:110"), nil, key) + }, + mockMsg: func(afterProcessing bool) *ipv4.Message { + // Story: the packet just left segment 0 which ends at + // (peering) hop 0 and is landing on segment 1 wich + // begins at (peering) hop 1. We do not care what hop 0 + // looks like. The forwarding code is looking at hop 1 and + // should leave the message in shape to be processed at hop 2. + spkt, _ := prepBaseMsg(now) + dpath := &scion.Decoded{ + Base: scion.Base{ + PathMeta: scion.MetaHdr{ + CurrHF: 1, + CurrINF: 1, + SegLen: [3]uint8{1, 2, 0}, + }, + NumINF: 2, + NumHops: 3, + }, + InfoFields: []path.InfoField{ + // up seg + {SegID: 0x111, ConsDir: true, Timestamp: util.TimeToSecs(now), Peer: true}, + // core seg + {SegID: 0x222, ConsDir: true, Timestamp: util.TimeToSecs(now), Peer: true}, + }, + HopFields: []path.HopField{ + {ConsIngress: 31, ConsEgress: 30}, + {ConsIngress: 1, ConsEgress: 2}, + {ConsIngress: 40, ConsEgress: 41}, + }, + } + + // Make obvious the unusual aspect of the path: two + // hopfield MACs (1 and 2) derive from the same SegID + // accumulator value. However, the forwarding code isn't + // supposed to even look at the second one. The SegID + // accumulator value can be anything, we use one from an + // info field because computeMAC makes that easy. + dpath.HopFields[1].Mac = computeMAC(t, key, dpath.InfoFields[1], dpath.HopFields[1]) + dpath.HopFields[2].Mac = computeMAC(t, key, dpath.InfoFields[1], dpath.HopFields[2]) + if !afterProcessing { + return toMsg(t, spkt, dpath) + } + _ = dpath.IncPath() + + // ... The SegID accumulator wasn't updated from HF[1], it is still the same. That is + // the key behavior. + + ret := toMsg(t, spkt, dpath) + ret.Addr = nil + ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil + return ret + }, + srcInterface: 1, // from peering link + assertFunc: assert.NoError, + }, + "brtransit peering non consdir": { + prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { + return router.NewDP( + map[uint16]router.BatchConn{ + uint16(1): mock_router.NewMockBatchConn(ctrl), + }, + map[uint16]topology.LinkType{ + 1: topology.Peer, + 2: topology.Child, + }, + nil, nil, nil, xtest.MustParseIA("1-ff00:0:110"), nil, key) + }, + mockMsg: func(afterProcessing bool) *ipv4.Message { + // Story: the packet lands on the last (peering) hop of + // segment 0. After processing, the packet is ready to + // be processed by the first (peering) hop of segment 1. + spkt, _ := prepBaseMsg(now) + dpath := &scion.Decoded{ + Base: scion.Base{ + PathMeta: scion.MetaHdr{ + CurrHF: 1, + CurrINF: 0, + SegLen: [3]uint8{2, 1, 0}, + }, + NumINF: 2, + NumHops: 3, + }, + InfoFields: []path.InfoField{ + // up seg + {SegID: 0x111, ConsDir: false, Timestamp: util.TimeToSecs(now), Peer: true}, + // down seg + {SegID: 0x222, ConsDir: true, Timestamp: util.TimeToSecs(now), Peer: true}, + }, + HopFields: []path.HopField{ + {ConsIngress: 31, ConsEgress: 30}, + {ConsIngress: 1, ConsEgress: 2}, + {ConsIngress: 40, ConsEgress: 41}, + }, + } + + // Make obvious the unusual aspect of the path: two + // hopfield MACs (0 and 1) derive from the same SegID + // accumulator value. However, the forwarding code isn't + // supposed to even look at the first one. The SegID + // accumulator value can be anything, we use one from an + // info field because computeMAC makes that easy. + dpath.HopFields[0].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[0]) + dpath.HopFields[1].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[1]) + + // We're going against construction order, so the accumulator value + // is that of the previous hop in traversal order. The story starts + // with the packet arriving at hop 1, so the accumulator value + // must match hop field 0. In this case, it is identical to that for + // hop field 1, which we made identical to the original SegID. + // So, we're all set. + if !afterProcessing { + return toMsg(t, spkt, dpath) + } + + _ = dpath.IncPath() + + // The SegID should not get updated on arrival. If it is, then MAC validation + // of HF1 will fail. Otherwise, this isn't visible because we changed segment. + + ret := toMsg(t, spkt, dpath) + ret.Addr = nil + ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil + return ret + }, + srcInterface: 2, // from child link + assertFunc: assert.NoError, + }, "brtransit non consdir": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { return router.NewDP( diff --git a/tools/topology/topo.py b/tools/topology/topo.py index c874e54500..22d2e61cc7 100644 --- a/tools/topology/topo.py +++ b/tools/topology/topo.py @@ -299,9 +299,7 @@ def _gen_br_entry(self, local, l_ifid, remote, r_ifid, remote_type, attrs, intl_addr = self._reg_addr(local, local_br + "_internal", addr_type) - intf = self._gen_br_intf(remote, public_addr, remote_addr, attrs, remote_type) - if intf['link_to'] == 'peer': - intf['remote_interface_id'] = r_ifid + intf = self._gen_br_intf(remote, r_ifid, public_addr, remote_addr, attrs, remote_type) if self.topo_dicts[local]["border_routers"].get(local_br) is None: intl_port = 30042 @@ -318,16 +316,21 @@ def _gen_br_entry(self, local, l_ifid, remote, r_ifid, remote_type, attrs, # There is already a BR entry, add interface self.topo_dicts[local]["border_routers"][local_br]['interfaces'][l_ifid] = intf - def _gen_br_intf(self, remote, public_addr, remote_addr, attrs, remote_type): - return { + def _gen_br_intf(self, remote, r_ifid, public_addr, remote_addr, attrs, remote_type): + link_to = remote_type.name.lower() + intf = { 'underlay': { 'public': join_host_port(public_addr.ip, SCION_ROUTER_PORT), 'remote': join_host_port(remote_addr.ip, SCION_ROUTER_PORT), }, 'isd_as': str(remote), - 'link_to': remote_type.name.lower(), - 'mtu': attrs.get('mtu', self.args.default_mtu) + 'link_to': link_to, + 'mtu': attrs.get('mtu', self.args.default_mtu), } + if link_to == 'peer': + intf['remote_interface_id'] = r_ifid + return intf + def _gen_sig_entries(self, topo_id, as_conf): addr_type = addr_type_from_underlay(as_conf.get('underlay', DEFAULT_UNDERLAY)) From 7a9a24b9a42f1d222f40aa3207907274e0fb63ab Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Mon, 11 Sep 2023 17:45:08 +0200 Subject: [PATCH 13/25] peering: Added br acceptance test cases for peeing links. --- control/beaconing/extender.go | 7 +- router/dataplane_test.go | 14 +- tools/braccept/cases/BUILD.bazel | 2 + .../cases/child_to_child_peering_in.go | 176 +++++++++++++++++ .../cases/child_to_child_peering_out.go | 179 ++++++++++++++++++ tools/braccept/cases/doc.go | 13 +- tools/braccept/main.go | 2 + tools/topology/topo.py | 1 - 8 files changed, 381 insertions(+), 13 deletions(-) create mode 100644 tools/braccept/cases/child_to_child_peering_in.go create mode 100644 tools/braccept/cases/child_to_child_peering_out.go diff --git a/control/beaconing/extender.go b/control/beaconing/extender.go index 9bdc63149f..202d1a1d02 100644 --- a/control/beaconing/extender.go +++ b/control/beaconing/extender.go @@ -97,9 +97,10 @@ func (s *DefaultExtender) Extend( // // The corrolary is that one cannot validate a hop field's MAC by looking at the // parent hop field MAC when the parent is a peering hop field. This is ok: that - // is never done that way, it is always done by validating agains the SegID accumulator - // supplied by the previous router on the forwarding path. The forwarding code - // takes care of not updating that accumulator when a peering hop is traversed. + // is never done that way, it is always done by validating against the SegID + // accumulator supplied by the previous router on the forwarding path. The + // forwarding code takes care of not updating that accumulator when a peering hop + // is traversed. // FIXME: why do we compute Beta(pseg) a second time? It hasn't changed since last // time. diff --git a/router/dataplane_test.go b/router/dataplane_test.go index d2bf2ffec8..da00529376 100644 --- a/router/dataplane_test.go +++ b/router/dataplane_test.go @@ -689,7 +689,7 @@ func TestProcessPkt(t *testing.T) { }, mockMsg: func(afterProcessing bool) *ipv4.Message { // Story: the packet just left segment 0 which ends at - // (peering) hop 0 and is landing on segment 1 wich + // (peering) hop 0 and is landing on segment 1 which // begins at (peering) hop 1. We do not care what hop 0 // looks like. The forwarding code is looking at hop 1 and // should leave the message in shape to be processed at hop 2. @@ -697,9 +697,9 @@ func TestProcessPkt(t *testing.T) { dpath := &scion.Decoded{ Base: scion.Base{ PathMeta: scion.MetaHdr{ - CurrHF: 1, + CurrHF: 1, CurrINF: 1, - SegLen: [3]uint8{1, 2, 0}, + SegLen: [3]uint8{1, 2, 0}, }, NumINF: 2, NumHops: 3, @@ -730,8 +730,8 @@ func TestProcessPkt(t *testing.T) { } _ = dpath.IncPath() - // ... The SegID accumulator wasn't updated from HF[1], it is still the same. That is - // the key behavior. + // ... The SegID accumulator wasn't updated from HF[1], + // it is still the same. That is the key behavior. ret := toMsg(t, spkt, dpath) ret.Addr = nil @@ -761,9 +761,9 @@ func TestProcessPkt(t *testing.T) { dpath := &scion.Decoded{ Base: scion.Base{ PathMeta: scion.MetaHdr{ - CurrHF: 1, + CurrHF: 1, CurrINF: 0, - SegLen: [3]uint8{2, 1, 0}, + SegLen: [3]uint8{2, 1, 0}, }, NumINF: 2, NumHops: 3, diff --git a/tools/braccept/cases/BUILD.bazel b/tools/braccept/cases/BUILD.bazel index c25dd33e91..870215823e 100644 --- a/tools/braccept/cases/BUILD.bazel +++ b/tools/braccept/cases/BUILD.bazel @@ -4,6 +4,8 @@ go_library( name = "go_default_library", srcs = [ "bfd.go", + "child_to_child_peering_in.go", + "child_to_child_peering_out.go", "child_to_child_xover.go", "child_to_internal.go", "child_to_parent.go", diff --git a/tools/braccept/cases/child_to_child_peering_in.go b/tools/braccept/cases/child_to_child_peering_in.go new file mode 100644 index 0000000000..ff05dd4a79 --- /dev/null +++ b/tools/braccept/cases/child_to_child_peering_in.go @@ -0,0 +1,176 @@ +// Copyright 2020 Anapaya Systems +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cases + +import ( + "hash" + "net" + "path/filepath" + "time" + + "github.com/google/gopacket" + "github.com/google/gopacket/layers" + + "github.com/scionproto/scion/pkg/addr" + "github.com/scionproto/scion/pkg/private/util" + "github.com/scionproto/scion/pkg/private/xtest" + "github.com/scionproto/scion/pkg/slayers" + "github.com/scionproto/scion/pkg/slayers/path" + "github.com/scionproto/scion/pkg/slayers/path/scion" + "github.com/scionproto/scion/tools/braccept/runner" +) + +// ChildToChildPeeringIn tests transit traffic over one BR host and one peering hop. +// In this case, traffic enters via a peering link, and leaves via a regular link from +// the same router. To be valid, the path as to be constructed as one single hop up +// segment at the peering link origin and one down segment over the regular link. +// The peering hop is the first hop on the second segment: it crosses from a peering +// interface to a child interface. +func ChildToChildPeeringIn(artifactsDir string, mac hash.Hash) runner.Case { + options := gopacket.SerializeOptions{ + FixLengths: true, + ComputeChecksums: true, + } + + // We inject the packet into A (at I/F 121) as if coming from 2 (at I/F 211) + ethernet := &layers.Ethernet{ + SrcMAC: net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0xbe, 0xef}, // I/F 211 + DstMAC: net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0x00, 0x12}, // I/F 121 + EthernetType: layers.EthernetTypeIPv4, + } + + ip := &layers.IPv4{ // On the 2->A link + Version: 4, + IHL: 5, + TTL: 64, + SrcIP: net.IP{192, 168, 12, 3}, // from 2's 211 IP + DstIP: net.IP{192, 168, 12, 2}, // to A's 121 IP + Protocol: layers.IPProtocolUDP, + Flags: layers.IPv4DontFragment, + } + + udp := &layers.UDP{ + SrcPort: layers.UDPPort(40000), + DstPort: layers.UDPPort(50000), + } + _ = udp.SetNetworkLayerForChecksum(ip) + + sp := &scion.Decoded{ + Base: scion.Base{ + PathMeta: scion.MetaHdr{ + CurrHF: 1, + CurrINF: 1, + SegLen: [3]uint8{1, 2, 0}, + }, + NumINF: 2, + NumHops: 3, + }, + InfoFields: []path.InfoField{ + // up seg + { + SegID: 0x111, + ConsDir: false, + Timestamp: util.TimeToSecs(time.Now()), + }, + // down seg + { + SegID: 0x222, + ConsDir: true, + Timestamp: util.TimeToSecs(time.Now()), + Peer: true, + }, + }, + HopFields: []path.HopField{ + {ConsIngress: 211, ConsEgress: 0}, // at 2 out to A + {ConsIngress: 121, ConsEgress: 151}, // at A in from 2 out to 5 + {ConsIngress: 511, ConsEgress: 0}, // at 5 in from A + }, + } + // Make the packet look the way it should. HF[0] is a regular hop. + sp.HopFields[0].Mac = path.MAC(mac, sp.InfoFields[0], sp.HopFields[0], nil) + + // HF[1] is a peering hop so it has the same SegID acc value as the next one + // in construction direction, HF[2]. Therefore, SEG[1]'s SegID. + sp.HopFields[1].Mac = path.MAC(mac, sp.InfoFields[1], sp.HopFields[1], nil) + sp.HopFields[2].Mac = path.MAC(mac, sp.InfoFields[1], sp.HopFields[2], nil) + + // The message if ready for injest at A, that is at HF[1], the start of the + // second segment, in construction direction. So SegID is already correct. + + // The end-to-end trip is from 2,172.16.2.1 to 5,172.16.5.1 + // That won't change through forwarding. + scionL := &slayers.SCION{ + Version: 0, + TrafficClass: 0xb8, + FlowID: 0xdead, + NextHdr: slayers.L4UDP, + PathType: scion.PathType, + SrcIA: xtest.MustParseIA("1-ff00:0:2"), + DstIA: xtest.MustParseIA("1-ff00:0:5"), + Path: sp, + } + if err := scionL.SetSrcAddr(addr.MustParseHost("172.16.2.1")); err != nil { + panic(err) + } + if err := scionL.SetDstAddr(addr.MustParseHost("174.16.5.1")); err != nil { + panic(err) + } + + scionudp := &slayers.UDP{} + scionudp.SrcPort = 40111 + scionudp.DstPort = 40222 + scionudp.SetNetworkLayerForChecksum(scionL) + + payload := []byte("actualpayloadbytes") + + // Prepare input packet + input := gopacket.NewSerializeBuffer() + if err := gopacket.SerializeLayers(input, options, + ethernet, ip, udp, scionL, scionudp, gopacket.Payload(payload), + ); err != nil { + panic(err) + } + + // Prepare want packet + // We expect it out of A's 151 I/F on its way to 5's 511 I/F. + + want := gopacket.NewSerializeBuffer() + ethernet.SrcMAC = net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0x00, 0x15} // I/F 151 + ethernet.DstMAC = net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0xbe, 0xef} // I/F 511 + ip.SrcIP = net.IP{192, 168, 15, 2} // from A's 151 IP + ip.DstIP = net.IP{192, 168, 15, 3} // to 5's 511 IP + udp.SrcPort, udp.DstPort = udp.DstPort, udp.SrcPort + if err := sp.IncPath(); err != nil { + panic(err) + } + + // Out of A, the current segment is seg 1. The Current acc + // value is still the same since HF[1] is a peering hop. + + if err := gopacket.SerializeLayers(want, options, + ethernet, ip, udp, scionL, scionudp, gopacket.Payload(payload), + ); err != nil { + panic(err) + } + + return runner.Case{ + Name: "ChildToChildPeeringTransit", + WriteTo: "veth_121_host", // Where we inject the test packet + ReadFrom: "veth_151_host", // Where we capture the forwarded packet + Input: input.Bytes(), + Want: want.Bytes(), + StoreDir: filepath.Join(artifactsDir, "ChildToChildXover"), + } +} diff --git a/tools/braccept/cases/child_to_child_peering_out.go b/tools/braccept/cases/child_to_child_peering_out.go new file mode 100644 index 0000000000..daef86f95a --- /dev/null +++ b/tools/braccept/cases/child_to_child_peering_out.go @@ -0,0 +1,179 @@ +// Copyright 2020 Anapaya Systems +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cases + +import ( + "hash" + "net" + "path/filepath" + "time" + + "github.com/google/gopacket" + "github.com/google/gopacket/layers" + + "github.com/scionproto/scion/pkg/addr" + "github.com/scionproto/scion/pkg/private/util" + "github.com/scionproto/scion/pkg/private/xtest" + "github.com/scionproto/scion/pkg/slayers" + "github.com/scionproto/scion/pkg/slayers/path" + "github.com/scionproto/scion/pkg/slayers/path/scion" + "github.com/scionproto/scion/tools/braccept/runner" +) + +// ChildToChildPeeringOut tests transit traffic over one BR host and one peering hop. +// In this case, traffic enters via a regular link, and leaves via a peering link from +// the same router. To be valid, the path as to be constructed as one up segment over +// the normal link and one single hop segment at the peering link's destination. +// The peering hop is the second hop on the first segment: it crosses from a child +// interface to a peering interface. +func ChildToChildPeeringOut(artifactsDir string, mac hash.Hash) runner.Case { + options := gopacket.SerializeOptions{ + FixLengths: true, + ComputeChecksums: true, + } + + // We inject the packet into A (at I/F 151) as if coming from 5 (at I/F 511) + ethernet := &layers.Ethernet{ + SrcMAC: net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0xbe, 0xef}, // I/F 511 + DstMAC: net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0x00, 0x15}, // I/F 151 + EthernetType: layers.EthernetTypeIPv4, + } + + ip := &layers.IPv4{ // On the 5->A link + Version: 4, + IHL: 5, + TTL: 64, + SrcIP: net.IP{192, 168, 15, 3}, // from 5's 511 IP + DstIP: net.IP{192, 168, 15, 2}, // to A's 151 IP + Protocol: layers.IPProtocolUDP, + Flags: layers.IPv4DontFragment, + } + + udp := &layers.UDP{ + SrcPort: layers.UDPPort(40000), + DstPort: layers.UDPPort(50000), + } + _ = udp.SetNetworkLayerForChecksum(ip) + + sp := &scion.Decoded{ + Base: scion.Base{ + PathMeta: scion.MetaHdr{ + CurrHF: 1, + CurrINF: 0, + SegLen: [3]uint8{2, 1, 0}, + }, + NumINF: 2, + NumHops: 3, + }, + InfoFields: []path.InfoField{ + // up seg + { + SegID: 0x111, + ConsDir: false, + Timestamp: util.TimeToSecs(time.Now()), + Peer: true, + }, + // down seg + { + SegID: 0x222, + ConsDir: true, + Timestamp: util.TimeToSecs(time.Now()), + }, + }, + HopFields: []path.HopField{ + {ConsIngress: 511, ConsEgress: 0}, // at 5 leaving to A + {ConsIngress: 121, ConsEgress: 151}, // at A in from 5 out to 2 + {ConsIngress: 211, ConsEgress: 0}, // at 2 in coming from A + }, + } + // Make the packet look the way it should. + // HF[1] is a peering hop, so it has the same SegID acc value as the next one + // in construction direction, HF[0]. Therefore the segment's SegID. + sp.HopFields[1].Mac = path.MAC(mac, sp.InfoFields[0], sp.HopFields[1], nil) + sp.HopFields[0].Mac = path.MAC(mac, sp.InfoFields[0], sp.HopFields[0], nil) + + // The second segment as just one hop. + sp.HopFields[2].Mac = path.MAC(mac, sp.InfoFields[1], sp.HopFields[2], nil) + + // The message is ready for injest at A, that is at HF[1]. Going against consruction + // direction, the SegID acc value must match that of HF[0], which is the same + // as that of HF[1], which is also SEG[0]'s SegID. So it's already correct. + + // The end-to-end trip is from 5,172.16.5.1 to 2,172.16.2.1 + // That won't change through forwarding. + scionL := &slayers.SCION{ + Version: 0, + TrafficClass: 0xb8, + FlowID: 0xdead, + NextHdr: slayers.L4UDP, + PathType: scion.PathType, + SrcIA: xtest.MustParseIA("1-ff00:0:5"), + DstIA: xtest.MustParseIA("1-ff00:0:2"), + Path: sp, + } + if err := scionL.SetSrcAddr(addr.MustParseHost("172.16.5.1")); err != nil { + panic(err) + } + if err := scionL.SetDstAddr(addr.MustParseHost("174.16.2.1")); err != nil { + panic(err) + } + + scionudp := &slayers.UDP{} + scionudp.SrcPort = 40111 + scionudp.DstPort = 40222 + scionudp.SetNetworkLayerForChecksum(scionL) + + payload := []byte("actualpayloadbytes") + + // Prepare input packet + input := gopacket.NewSerializeBuffer() + if err := gopacket.SerializeLayers(input, options, + ethernet, ip, udp, scionL, scionudp, gopacket.Payload(payload), + ); err != nil { + panic(err) + } + + // Prepare want packet + // We expect it out of A's 121 I/F on its way to 4's 211 I/F. + + want := gopacket.NewSerializeBuffer() + ethernet.SrcMAC = net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0x00, 0x12} // I/F 121 + ethernet.DstMAC = net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0xbe, 0xef} // I/F 211 + ip.SrcIP = net.IP{192, 168, 12, 2} // from A's 121 IP + ip.DstIP = net.IP{192, 168, 12, 3} // to 2's 211 IP + udp.SrcPort, udp.DstPort = udp.DstPort, udp.SrcPort + if err := sp.IncPath(); err != nil { + panic(err) + } + + // Out of A, the current segment is seg 1. The Current acc + // value matches HF[2], which is SEG[1]'s SegID since HF[2] is the first hop in + // construction direction of the segment. + + if err := gopacket.SerializeLayers(want, options, + ethernet, ip, udp, scionL, scionudp, gopacket.Payload(payload), + ); err != nil { + panic(err) + } + + return runner.Case{ + Name: "ChildToChildPeeringOut", + WriteTo: "veth_151_host", // Where we inject the test packet + ReadFrom: "veth_121_host", // Where we capture the forwarded packet + Input: input.Bytes(), + Want: want.Bytes(), + StoreDir: filepath.Join(artifactsDir, "ChildToChildXover"), + } +} diff --git a/tools/braccept/cases/doc.go b/tools/braccept/cases/doc.go index 9481e6215a..843568dad3 100644 --- a/tools/braccept/cases/doc.go +++ b/tools/braccept/cases/doc.go @@ -18,6 +18,13 @@ into the braccept binary. The process to add a new case is the following: +Step 0. Refer to the following for the test's setup: + - Overview: acceptance/router_multi/topology.drawio.png + - Topology Details: acceptance/router_multi/conf/topology.json + - MAC Addresses: acceptance/router_multi/test.py + Note that all MAC addresses of interfaces on the far side + of the A/B/C/D routers are identical: f00d:cafe:beef + Step 1. Add a new file with a representative name e.g. cases/child_to_child_xover.go @@ -45,7 +52,9 @@ Step 3. In the braccept/main.go, include the above function cases.ChildToChildXover(artifactsDir, mac), } -Step 4. Do a local run, which means set up a working router and execute the -braccept. +Step 4. Do a local run, which means set up a working router, execute the +braccept, shutdown the router. This is done in sequence by: + + bazel test acceptance/router_multi:all --config=integration --nocache_test_results */ package cases diff --git a/tools/braccept/main.go b/tools/braccept/main.go index 9a54104d56..c8cea6016f 100644 --- a/tools/braccept/main.go +++ b/tools/braccept/main.go @@ -128,6 +128,8 @@ func realMain() int { cases.OutgoingOneHop(artifactsDir, hfMAC), cases.SVC(artifactsDir, hfMAC), cases.JumboPacket(artifactsDir, hfMAC), + cases.ChildToChildPeeringOut(artifactsDir, hfMAC), + cases.ChildToChildPeeringIn(artifactsDir, hfMAC), } if *bfd { diff --git a/tools/topology/topo.py b/tools/topology/topo.py index 22d2e61cc7..298605bdc8 100644 --- a/tools/topology/topo.py +++ b/tools/topology/topo.py @@ -331,7 +331,6 @@ def _gen_br_intf(self, remote, r_ifid, public_addr, remote_addr, attrs, remote_t intf['remote_interface_id'] = r_ifid return intf - def _gen_sig_entries(self, topo_id, as_conf): addr_type = addr_type_from_underlay(as_conf.get('underlay', DEFAULT_UNDERLAY)) elem_id = "sig" + topo_id.file_fmt() From 35ba7f2573f1531515b76b21102b5c09d945d4ee Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Tue, 12 Sep 2023 10:47:09 +0200 Subject: [PATCH 14/25] peering: clarified how to run the braccept tests. --- acceptance/README.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/acceptance/README.md b/acceptance/README.md index cab20b6f35..86aec0c17c 100644 --- a/acceptance/README.md +++ b/acceptance/README.md @@ -3,6 +3,10 @@ This directory contains a set of integration tests. Each test is defined as a bazel test target, with tags `integration` and `exclusive`. +Some integration tests use code outside this directory. For example, the +`router_multi` acceptance test cases and main executable are in `tools/braccept`. + + ## Basic Commands To run all integration tests which include the acceptance tests, execute one of @@ -18,6 +22,7 @@ Run a subset of the tests by specifying a different list of targets: ```bash bazel test --config=integration //acceptance/cert_renewal:all //acceptance/trc_update/... +bazel test --config=integration //acceptance/router_multi:all --cache_test_results=no ``` The following the flags to bazel test can be helpful when running individual tests: @@ -27,8 +32,8 @@ The following the flags to bazel test can be helpful when running individual tes ## Manual Testing -Some of the tests are defined using a common framework, defined in the -bazel rules `topogen_test` and `raw_test`. +Some of the tests are defined using a common framework, implemented by the +bazel rules `topogen_test` and `raw_test` (in [raw.bzl](acceptance/common/raw.bzl)). These test cases allow more fine grained interaction. ```bash @@ -41,5 +46,12 @@ bazel run //:_run bazel run //:_teardown ``` +For example: +```bash +bazel run //acceptance/router_multi:test_bfd_setup +bazel run //acceptance/router_multi:test_bfd_run +bazel run //acceptance/router_multi:test_bfd_teardown +``` + See [common/README](common/README.md) for more information about the internal structure of these tests. From 3f8391f0e54f9df788c6bd74370b38951e7c5818 Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Tue, 12 Sep 2023 11:41:33 +0200 Subject: [PATCH 15/25] peering: doc formatting --- acceptance/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acceptance/README.md b/acceptance/README.md index 86aec0c17c..49b8b435a3 100644 --- a/acceptance/README.md +++ b/acceptance/README.md @@ -6,7 +6,6 @@ Each test is defined as a bazel test target, with tags `integration` and `exclus Some integration tests use code outside this directory. For example, the `router_multi` acceptance test cases and main executable are in `tools/braccept`. - ## Basic Commands To run all integration tests which include the acceptance tests, execute one of @@ -47,6 +46,7 @@ bazel run //:_teardown ``` For example: + ```bash bazel run //acceptance/router_multi:test_bfd_setup bazel run //acceptance/router_multi:test_bfd_run From a6f5f61cb9449d98c289ddff42eec6c246db0c4b Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Wed, 13 Sep 2023 12:17:01 +0200 Subject: [PATCH 16/25] peering: Implement some reviewer's requests and suggestions. Mainly: * fixed br peering tests names * in tests, use different keys to sign hops at different ASes * dedupe call to xtractBeta * Comments, copyright, etc. --- control/beaconing/extender.go | 11 ++-- router/dataplane.go | 4 +- router/dataplane_test.go | 13 +++-- tools/braccept/cases/BUILD.bazel | 5 +- ..._child_peering_out.go => child_to_peer.go} | 51 ++++++++++++------ ...o_child_peering_in.go => peer_to_child.go} | 52 +++++++++++++------ tools/braccept/main.go | 4 +- 7 files changed, 92 insertions(+), 48 deletions(-) rename tools/braccept/cases/{child_to_child_peering_out.go => child_to_peer.go} (77%) rename tools/braccept/cases/{child_to_child_peering_in.go => peer_to_child.go} (76%) diff --git a/control/beaconing/extender.go b/control/beaconing/extender.go index 202d1a1d02..006c41d208 100644 --- a/control/beaconing/extender.go +++ b/control/beaconing/extender.go @@ -85,7 +85,8 @@ func (s *DefaultExtender) Extend( } ts := pseg.Info.Timestamp - hopEntry, epicHopMac, err := s.createHopEntry(ingress, egress, ts, extractBeta(pseg)) + hopBeta := extractBeta(pseg) + hopEntry, epicHopMac, err := s.createHopEntry(ingress, egress, ts, hopBeta) if err != nil { return serrors.WrapStr("creating hop entry", err) } @@ -102,9 +103,7 @@ func (s *DefaultExtender) Extend( // forwarding code takes care of not updating that accumulator when a peering hop // is traversed. - // FIXME: why do we compute Beta(pseg) a second time? It hasn't changed since last - // time. - peerBeta := extractBeta(pseg) ^ binary.BigEndian.Uint16(hopEntry.HopField.MAC[:2]) + peerBeta := hopBeta ^ binary.BigEndian.Uint16(hopEntry.HopField.MAC[:2]) peerEntries, epicPeerMacs, err := s.createPeerEntries(egress, peers, ts, peerBeta) if err != nil { return err @@ -283,6 +282,10 @@ func (s *DefaultExtender) createHopF(ingress, egress uint16, ts time.Time, }, fullMAC[path.MacLen:] } +// extractBeta computes the beta value that must be used for the next hop to be added to +// be added at the end of the segment. +// FIXME(jice): keeping an accumulator would be just as easy to do as it is during +// forwarding. What's the benefit of re-calculating the whole chain every time? func extractBeta(pseg *seg.PathSegment) uint16 { beta := pseg.Info.SegmentID for _, entry := range pseg.ASEntries { diff --git a/router/dataplane.go b/router/dataplane.go index 8b0bc63212..12c72b7bb5 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -1032,7 +1032,7 @@ type scionPacketProcessor struct { infoField path.InfoField // effectiveXover indicates if a cross-over segment change was done during processing. effectiveXover bool - // peering indicates that the packet is inside of a peering AS + // peering indicates that the hop field being processed is a peering hop field. peering bool // cachedMac contains the full 16 bytes of the MAC. Will be set during processing. @@ -1364,7 +1364,7 @@ func (p *scionPacketProcessor) resolveInbound() (*net.UDPAddr, processResult, er func (p *scionPacketProcessor) processEgress() error { // We are the egress router and if we go in construction direction we // need to update the SegID (unless we are effecting a peering hop). - // When we're at a peering hop, the SegID for this HOP and for the next + // When we're at a peering hop, the SegID for this hop and for the next // are one and the same, both hops chain to the same parent. So do not // update SegID. if p.infoField.ConsDir && !p.peering { diff --git a/router/dataplane_test.go b/router/dataplane_test.go index da00529376..74272ba059 100644 --- a/router/dataplane_test.go +++ b/router/dataplane_test.go @@ -570,6 +570,7 @@ func TestProcessPkt(t *testing.T) { defer ctrl.Finish() key := []byte("testkey_xxxxxxxx") + otherKey := []byte("testkey_yyyyyyyy") now := time.Now() epicTS, err := libepic.CreateTimestamp(now, now) require.NoError(t, err) @@ -721,10 +722,12 @@ func TestProcessPkt(t *testing.T) { // hopfield MACs (1 and 2) derive from the same SegID // accumulator value. However, the forwarding code isn't // supposed to even look at the second one. The SegID - // accumulator value can be anything, we use one from an + // accumulator value can be anything (it comes from the + // parent hop of HF[1] in the original beaconned segment, + // which is not in the path). So, we use one from an // info field because computeMAC makes that easy. dpath.HopFields[1].Mac = computeMAC(t, key, dpath.InfoFields[1], dpath.HopFields[1]) - dpath.HopFields[2].Mac = computeMAC(t, key, dpath.InfoFields[1], dpath.HopFields[2]) + dpath.HopFields[2].Mac = computeMAC(t, otherKey, dpath.InfoFields[1], dpath.HopFields[2]) if !afterProcessing { return toMsg(t, spkt, dpath) } @@ -785,9 +788,11 @@ func TestProcessPkt(t *testing.T) { // hopfield MACs (0 and 1) derive from the same SegID // accumulator value. However, the forwarding code isn't // supposed to even look at the first one. The SegID - // accumulator value can be anything, we use one from an + // accumulator value can be anything (it comes from the + // parent hop of HF[1] in the original beaconned segment, + // which is not in the path). So, we use one from an // info field because computeMAC makes that easy. - dpath.HopFields[0].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[0]) + dpath.HopFields[0].Mac = computeMAC(t, otherKey, dpath.InfoFields[0], dpath.HopFields[0]) dpath.HopFields[1].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[1]) // We're going against construction order, so the accumulator value diff --git a/tools/braccept/cases/BUILD.bazel b/tools/braccept/cases/BUILD.bazel index 870215823e..30cb91f472 100644 --- a/tools/braccept/cases/BUILD.bazel +++ b/tools/braccept/cases/BUILD.bazel @@ -4,17 +4,17 @@ go_library( name = "go_default_library", srcs = [ "bfd.go", - "child_to_child_peering_in.go", - "child_to_child_peering_out.go", "child_to_child_xover.go", "child_to_internal.go", "child_to_parent.go", + "child_to_peer.go", "doc.go", "internal_to_child.go", "jumbo.go", "onehop.go", "parent_to_child.go", "parent_to_internal.go", + "peer_to_child.go", "scmp.go", "scmp_dest_unreachable.go", "scmp_expired_hop.go", @@ -35,6 +35,7 @@ go_library( "//pkg/drkey:go_default_library", "//pkg/private/util:go_default_library", "//pkg/private/xtest:go_default_library", + "//pkg/scrypto:go_default_library", "//pkg/slayers:go_default_library", "//pkg/slayers/path:go_default_library", "//pkg/slayers/path/empty:go_default_library", diff --git a/tools/braccept/cases/child_to_child_peering_out.go b/tools/braccept/cases/child_to_peer.go similarity index 77% rename from tools/braccept/cases/child_to_child_peering_out.go rename to tools/braccept/cases/child_to_peer.go index daef86f95a..19fbf3517a 100644 --- a/tools/braccept/cases/child_to_child_peering_out.go +++ b/tools/braccept/cases/child_to_peer.go @@ -1,4 +1,4 @@ -// Copyright 2020 Anapaya Systems +// Copyright 2023 SCION Association // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -26,28 +26,31 @@ import ( "github.com/scionproto/scion/pkg/addr" "github.com/scionproto/scion/pkg/private/util" "github.com/scionproto/scion/pkg/private/xtest" + "github.com/scionproto/scion/pkg/scrypto" "github.com/scionproto/scion/pkg/slayers" "github.com/scionproto/scion/pkg/slayers/path" "github.com/scionproto/scion/pkg/slayers/path/scion" "github.com/scionproto/scion/tools/braccept/runner" ) -// ChildToChildPeeringOut tests transit traffic over one BR host and one peering hop. +// ChildToPeer tests transit traffic over one BR host and one peering hop. // In this case, traffic enters via a regular link, and leaves via a peering link from // the same router. To be valid, the path as to be constructed as one up segment over -// the normal link and one single hop segment at the peering link's destination. -// The peering hop is the second hop on the first segment: it crosses from a child -// interface to a peering interface. -func ChildToChildPeeringOut(artifactsDir string, mac hash.Hash) runner.Case { +// the normal link ending with a peering hop and one down segment starting at the +// peering link's destination. The peering hop is the second hop on the first segment +// as it crosses from a child interface to a peering interface. +// In this test case, the down segment is a one-hop segment. The peering link's destination +// is the only hop. +func ChildToPeer(artifactsDir string, mac hash.Hash) runner.Case { options := gopacket.SerializeOptions{ FixLengths: true, ComputeChecksums: true, } - // We inject the packet into A (at I/F 151) as if coming from 5 (at I/F 511) + // We inject the packet into A (at IF 151) as if coming from 5 (at IF 511) ethernet := &layers.Ethernet{ - SrcMAC: net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0xbe, 0xef}, // I/F 511 - DstMAC: net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0x00, 0x15}, // I/F 151 + SrcMAC: net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0xbe, 0xef}, // IF 511 + DstMAC: net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0x00, 0x15}, // IF 151 EthernetType: layers.EthernetTypeIPv4, } @@ -98,14 +101,28 @@ func ChildToChildPeeringOut(artifactsDir string, mac hash.Hash) runner.Case { {ConsIngress: 211, ConsEgress: 0}, // at 2 in coming from A }, } - // Make the packet look the way it should. + + // Make the packet look the way it should... We have three hops of interrest. + + // Hops are all signed with different keys. Only HF[1] was signed by + // the AS that we hand the packet to. The others can be anything as they + // couldn't be check at that AS anyway. + macGenX, err := scrypto.HFMacFactory([]byte("1234567812345678")) + if err != nil { + panic(err) + } + macGenY, err := scrypto.HFMacFactory([]byte("1234567812345678")) + if err != nil { + panic(err) + } + // HF[1] is a peering hop, so it has the same SegID acc value as the next one - // in construction direction, HF[0]. Therefore the segment's SegID. + // in construction direction, HF[0]. That is, SEG[0]'s SegID. sp.HopFields[1].Mac = path.MAC(mac, sp.InfoFields[0], sp.HopFields[1], nil) - sp.HopFields[0].Mac = path.MAC(mac, sp.InfoFields[0], sp.HopFields[0], nil) + sp.HopFields[0].Mac = path.MAC(macGenX(), sp.InfoFields[0], sp.HopFields[0], nil) - // The second segment as just one hop. - sp.HopFields[2].Mac = path.MAC(mac, sp.InfoFields[1], sp.HopFields[2], nil) + // The second segment has just one hop. + sp.HopFields[2].Mac = path.MAC(macGenY(), sp.InfoFields[1], sp.HopFields[2], nil) // The message is ready for injest at A, that is at HF[1]. Going against consruction // direction, the SegID acc value must match that of HF[0], which is the same @@ -146,11 +163,11 @@ func ChildToChildPeeringOut(artifactsDir string, mac hash.Hash) runner.Case { } // Prepare want packet - // We expect it out of A's 121 I/F on its way to 4's 211 I/F. + // We expect it out of A's 121 IF on its way to 4's 211 IF. want := gopacket.NewSerializeBuffer() - ethernet.SrcMAC = net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0x00, 0x12} // I/F 121 - ethernet.DstMAC = net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0xbe, 0xef} // I/F 211 + ethernet.SrcMAC = net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0x00, 0x12} // IF 121 + ethernet.DstMAC = net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0xbe, 0xef} // IF 211 ip.SrcIP = net.IP{192, 168, 12, 2} // from A's 121 IP ip.DstIP = net.IP{192, 168, 12, 3} // to 2's 211 IP udp.SrcPort, udp.DstPort = udp.DstPort, udp.SrcPort diff --git a/tools/braccept/cases/child_to_child_peering_in.go b/tools/braccept/cases/peer_to_child.go similarity index 76% rename from tools/braccept/cases/child_to_child_peering_in.go rename to tools/braccept/cases/peer_to_child.go index ff05dd4a79..07a2d0d865 100644 --- a/tools/braccept/cases/child_to_child_peering_in.go +++ b/tools/braccept/cases/peer_to_child.go @@ -1,4 +1,4 @@ -// Copyright 2020 Anapaya Systems +// Copyright 2023 SCION Association // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -26,28 +26,31 @@ import ( "github.com/scionproto/scion/pkg/addr" "github.com/scionproto/scion/pkg/private/util" "github.com/scionproto/scion/pkg/private/xtest" + "github.com/scionproto/scion/pkg/scrypto" "github.com/scionproto/scion/pkg/slayers" "github.com/scionproto/scion/pkg/slayers/path" "github.com/scionproto/scion/pkg/slayers/path/scion" "github.com/scionproto/scion/tools/braccept/runner" ) -// ChildToChildPeeringIn tests transit traffic over one BR host and one peering hop. +// PeerToChild tests transit traffic over one BR host and one peering hop. // In this case, traffic enters via a peering link, and leaves via a regular link from -// the same router. To be valid, the path as to be constructed as one single hop up -// segment at the peering link origin and one down segment over the regular link. -// The peering hop is the first hop on the second segment: it crosses from a peering -// interface to a child interface. -func ChildToChildPeeringIn(artifactsDir string, mac hash.Hash) runner.Case { +// the same router. To be valid, the path as to be constructed as one up +// segment ending at the peering link's origin and one down segment over +// the regular link. The peering hop is the first hop on the second segment as +// it crosses from a peering interface to a child interface. +// In this test case, the up segment is a one-hop segment. The peering link's +// origin is the only hop. +func PeerToChild(artifactsDir string, mac hash.Hash) runner.Case { options := gopacket.SerializeOptions{ FixLengths: true, ComputeChecksums: true, } - // We inject the packet into A (at I/F 121) as if coming from 2 (at I/F 211) + // We inject the packet into A (at IF 121) as if coming from 2 (at IF 211) ethernet := &layers.Ethernet{ - SrcMAC: net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0xbe, 0xef}, // I/F 211 - DstMAC: net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0x00, 0x12}, // I/F 121 + SrcMAC: net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0xbe, 0xef}, // IF 211 + DstMAC: net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0x00, 0x12}, // IF 121 EthernetType: layers.EthernetTypeIPv4, } @@ -98,13 +101,28 @@ func ChildToChildPeeringIn(artifactsDir string, mac hash.Hash) runner.Case { {ConsIngress: 511, ConsEgress: 0}, // at 5 in from A }, } - // Make the packet look the way it should. HF[0] is a regular hop. - sp.HopFields[0].Mac = path.MAC(mac, sp.InfoFields[0], sp.HopFields[0], nil) + + // Make the packet look the way it should... We have three hops of interrest. + + // Hops are all signed with different keys. Only HF[1] was signed by + // the AS that we hand the packet to. The others can be anything as they + // couldn't be check at that AS anyway. + macGenX, err := scrypto.HFMacFactory([]byte("1234567812345678")) + if err != nil { + panic(err) + } + macGenY, err := scrypto.HFMacFactory([]byte("1234567812345678")) + if err != nil { + panic(err) + } + + // HF[0] is a regular hop. + sp.HopFields[0].Mac = path.MAC(macGenX(), sp.InfoFields[0], sp.HopFields[0], nil) // HF[1] is a peering hop so it has the same SegID acc value as the next one - // in construction direction, HF[2]. Therefore, SEG[1]'s SegID. + // in construction direction, HF[2]. That is, SEG[1]'s SegID. sp.HopFields[1].Mac = path.MAC(mac, sp.InfoFields[1], sp.HopFields[1], nil) - sp.HopFields[2].Mac = path.MAC(mac, sp.InfoFields[1], sp.HopFields[2], nil) + sp.HopFields[2].Mac = path.MAC(macGenY(), sp.InfoFields[1], sp.HopFields[2], nil) // The message if ready for injest at A, that is at HF[1], the start of the // second segment, in construction direction. So SegID is already correct. @@ -144,11 +162,11 @@ func ChildToChildPeeringIn(artifactsDir string, mac hash.Hash) runner.Case { } // Prepare want packet - // We expect it out of A's 151 I/F on its way to 5's 511 I/F. + // We expect it out of A's 151 IF on its way to 5's 511 IF. want := gopacket.NewSerializeBuffer() - ethernet.SrcMAC = net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0x00, 0x15} // I/F 151 - ethernet.DstMAC = net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0xbe, 0xef} // I/F 511 + ethernet.SrcMAC = net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0x00, 0x15} // IF 151 + ethernet.DstMAC = net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, 0xbe, 0xef} // IF 511 ip.SrcIP = net.IP{192, 168, 15, 2} // from A's 151 IP ip.DstIP = net.IP{192, 168, 15, 3} // to 5's 511 IP udp.SrcPort, udp.DstPort = udp.DstPort, udp.SrcPort diff --git a/tools/braccept/main.go b/tools/braccept/main.go index c8cea6016f..fc27bf7afc 100644 --- a/tools/braccept/main.go +++ b/tools/braccept/main.go @@ -128,8 +128,8 @@ func realMain() int { cases.OutgoingOneHop(artifactsDir, hfMAC), cases.SVC(artifactsDir, hfMAC), cases.JumboPacket(artifactsDir, hfMAC), - cases.ChildToChildPeeringOut(artifactsDir, hfMAC), - cases.ChildToChildPeeringIn(artifactsDir, hfMAC), + cases.ChildToPeer(artifactsDir, hfMAC), + cases.PeerToChild(artifactsDir, hfMAC), } if *bfd { From 1234b9fd82d6e6898ac5890592dabd90ebb56892 Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Wed, 13 Sep 2023 12:31:26 +0200 Subject: [PATCH 17/25] peering: treat impossible case as route error. cross-over cannot result in egress to peering link. --- router/dataplane.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/router/dataplane.go b/router/dataplane.go index 12c72b7bb5..990106eb62 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -1288,8 +1288,6 @@ func (p *scionPacketProcessor) validateEgressID() (processResult, error) { return processResult{}, nil case ingress == topology.Child && egress == topology.Child: return processResult{}, nil - case ingress == topology.Unset && egress == topology.Peer: - return processResult{}, nil default: return p.packSCMP( slayers.SCMPTypeParameterProblem, From cf95a7add766d269cc90f95c63eaae1d4ac4be58 Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Wed, 13 Sep 2023 13:56:53 +0200 Subject: [PATCH 18/25] peering: improve router unit tests by checking the egress interface. --- router/dataplane_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/router/dataplane_test.go b/router/dataplane_test.go index 74272ba059..8f4f8e0987 100644 --- a/router/dataplane_test.go +++ b/router/dataplane_test.go @@ -579,6 +579,7 @@ func TestProcessPkt(t *testing.T) { mockMsg func(bool) *ipv4.Message prepareDP func(*gomock.Controller) *router.DataPlane srcInterface uint16 + egressInterface uint16 assertFunc assert.ErrorAssertionFunc }{ "inbound": { @@ -606,6 +607,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 1, + egressInterface: 0, assertFunc: assert.NoError, }, "outbound": { @@ -640,6 +642,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 0, + egressInterface: 1, assertFunc: assert.NoError, }, "brtransit": { @@ -674,6 +677,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 1, + egressInterface: 2, assertFunc: assert.NoError, }, "brtransit peering consdir": { @@ -742,6 +746,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 1, // from peering link + egressInterface: 2, assertFunc: assert.NoError, }, "brtransit peering non consdir": { @@ -816,6 +821,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 2, // from child link + egressInterface: 1, assertFunc: assert.NoError, }, "brtransit non consdir": { @@ -850,6 +856,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 1, + egressInterface: 2, assertFunc: assert.NoError, }, "astransit direct": { @@ -880,6 +887,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 1, + egressInterface: 0, assertFunc: assert.NoError, }, "astransit xover": { @@ -932,6 +940,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 51, + egressInterface: 0, assertFunc: assert.NoError, }, "svc": { @@ -967,6 +976,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 1, + egressInterface: 0, assertFunc: assert.NoError, }, "svc nobackend": { @@ -990,6 +1000,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 1, + egressInterface: 0, assertFunc: assertIsSCMPError(slayers.SCMPTypeDestinationUnreachable, 0), }, "svc invalid": { @@ -1013,6 +1024,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 1, + egressInterface: 0, assertFunc: assertIsSCMPError(slayers.SCMPTypeDestinationUnreachable, 0), }, "onehop inbound": { @@ -1075,6 +1087,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 1, + egressInterface: 0, assertFunc: assert.NoError, }, "onehop inbound invalid src": { @@ -1111,6 +1124,7 @@ func TestProcessPkt(t *testing.T) { return toMsg(t, spkt, dpath) }, srcInterface: 1, + egressInterface: 21, assertFunc: assert.Error, }, "reversed onehop outbound": { @@ -1170,6 +1184,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 0, + egressInterface: 1, assertFunc: assert.NoError, }, "onehop outbound": { @@ -1216,6 +1231,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 0, + egressInterface: 2, assertFunc: assert.NoError, }, "invalid dest": { @@ -1238,6 +1254,7 @@ func TestProcessPkt(t *testing.T) { return ret }, srcInterface: 1, + egressInterface: 0, assertFunc: assertIsSCMPError( slayers.SCMPTypeParameterProblem, slayers.SCMPCodeInvalidDestinationAddress, @@ -1256,6 +1273,7 @@ func TestProcessPkt(t *testing.T) { return toIP(t, spkt, epicpath, afterProcessing) }, srcInterface: 1, + egressInterface: 0, assertFunc: assert.NoError, }, "epic malformed path": { @@ -1272,6 +1290,7 @@ func TestProcessPkt(t *testing.T) { return toIP(t, spkt, &scion.Decoded{}, afterProcessing) }, srcInterface: 1, + egressInterface: 0, assertFunc: assert.Error, }, "epic invalid timestamp": { @@ -1290,6 +1309,7 @@ func TestProcessPkt(t *testing.T) { return toIP(t, spkt, epicpath, afterProcessing) }, srcInterface: 1, + egressInterface: 0, assertFunc: assert.Error, }, "epic invalid LHVF": { @@ -1308,6 +1328,7 @@ func TestProcessPkt(t *testing.T) { return toIP(t, spkt, epicpath, afterProcessing) }, srcInterface: 1, + egressInterface: 0, assertFunc: assert.Error, }, } @@ -1331,6 +1352,7 @@ func TestProcessPkt(t *testing.T) { outPkt.Addr = nil } assert.Equal(t, want, outPkt) + assert.Equal(t, tc.egressInterface, result.EgressID) }) } } From 7d09a0dcd2dcb1994b142ce428b8784039d07f86 Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Wed, 13 Sep 2023 14:22:23 +0200 Subject: [PATCH 19/25] peering: fix spelling in comment. --- tools/braccept/cases/child_to_peer.go | 2 +- tools/braccept/cases/peer_to_child.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/braccept/cases/child_to_peer.go b/tools/braccept/cases/child_to_peer.go index 19fbf3517a..6aec7e9e6a 100644 --- a/tools/braccept/cases/child_to_peer.go +++ b/tools/braccept/cases/child_to_peer.go @@ -124,7 +124,7 @@ func ChildToPeer(artifactsDir string, mac hash.Hash) runner.Case { // The second segment has just one hop. sp.HopFields[2].Mac = path.MAC(macGenY(), sp.InfoFields[1], sp.HopFields[2], nil) - // The message is ready for injest at A, that is at HF[1]. Going against consruction + // The message is ready for ingest at A, that is at HF[1]. Going against consruction // direction, the SegID acc value must match that of HF[0], which is the same // as that of HF[1], which is also SEG[0]'s SegID. So it's already correct. diff --git a/tools/braccept/cases/peer_to_child.go b/tools/braccept/cases/peer_to_child.go index 07a2d0d865..3fc9264cfc 100644 --- a/tools/braccept/cases/peer_to_child.go +++ b/tools/braccept/cases/peer_to_child.go @@ -124,7 +124,7 @@ func PeerToChild(artifactsDir string, mac hash.Hash) runner.Case { sp.HopFields[1].Mac = path.MAC(mac, sp.InfoFields[1], sp.HopFields[1], nil) sp.HopFields[2].Mac = path.MAC(macGenY(), sp.InfoFields[1], sp.HopFields[2], nil) - // The message if ready for injest at A, that is at HF[1], the start of the + // The message if ready for ingest at A, that is at HF[1], the start of the // second segment, in construction direction. So SegID is already correct. // The end-to-end trip is from 2,172.16.2.1 to 5,172.16.5.1 From 4257be32c904d66d285c5e8128160bbbe3cf5d25 Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Wed, 13 Sep 2023 17:44:58 +0200 Subject: [PATCH 20/25] peering: fixed comment. --- control/beaconing/extender.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/control/beaconing/extender.go b/control/beaconing/extender.go index 006c41d208..7e5fe00a49 100644 --- a/control/beaconing/extender.go +++ b/control/beaconing/extender.go @@ -282,8 +282,8 @@ func (s *DefaultExtender) createHopF(ingress, egress uint16, ts time.Time, }, fullMAC[path.MacLen:] } -// extractBeta computes the beta value that must be used for the next hop to be added to -// be added at the end of the segment. +// extractBeta computes the beta value that must be used for the next hop to be +// added at the end of the segment. // FIXME(jice): keeping an accumulator would be just as easy to do as it is during // forwarding. What's the benefit of re-calculating the whole chain every time? func extractBeta(pseg *seg.PathSegment) uint16 { From 0d97857748a30542426b1b1d9798899d9b2385a5 Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Wed, 13 Sep 2023 19:05:07 +0200 Subject: [PATCH 21/25] peering: added a couple more test cases. --- router/dataplane_test.go | 303 ++++++++++++++++++++++++++++++--------- 1 file changed, 235 insertions(+), 68 deletions(-) diff --git a/router/dataplane_test.go b/router/dataplane_test.go index 8f4f8e0987..c83bc2701a 100644 --- a/router/dataplane_test.go +++ b/router/dataplane_test.go @@ -576,11 +576,11 @@ func TestProcessPkt(t *testing.T) { require.NoError(t, err) testCases := map[string]struct { - mockMsg func(bool) *ipv4.Message - prepareDP func(*gomock.Controller) *router.DataPlane - srcInterface uint16 + mockMsg func(bool) *ipv4.Message + prepareDP func(*gomock.Controller) *router.DataPlane + srcInterface uint16 egressInterface uint16 - assertFunc assert.ErrorAssertionFunc + assertFunc assert.ErrorAssertionFunc }{ "inbound": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -606,9 +606,9 @@ func TestProcessPkt(t *testing.T) { } return ret }, - srcInterface: 1, + srcInterface: 1, egressInterface: 0, - assertFunc: assert.NoError, + assertFunc: assert.NoError, }, "outbound": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -641,9 +641,9 @@ func TestProcessPkt(t *testing.T) { ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil return ret }, - srcInterface: 0, + srcInterface: 0, egressInterface: 1, - assertFunc: assert.NoError, + assertFunc: assert.NoError, }, "brtransit": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -676,9 +676,44 @@ func TestProcessPkt(t *testing.T) { ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil return ret }, - srcInterface: 1, + srcInterface: 1, egressInterface: 2, - assertFunc: assert.NoError, + assertFunc: assert.NoError, + }, + "brtransit non consdir": { + prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { + return router.NewDP( + map[uint16]router.BatchConn{ + uint16(2): mock_router.NewMockBatchConn(ctrl), + }, + map[uint16]topology.LinkType{ + 2: topology.Parent, + 1: topology.Child, + }, nil, nil, nil, xtest.MustParseIA("1-ff00:0:110"), nil, key) + }, + mockMsg: func(afterProcessing bool) *ipv4.Message { + spkt, dpath := prepBaseMsg(now) + dpath.HopFields = []path.HopField{ + {ConsIngress: 31, ConsEgress: 30}, + {ConsIngress: 2, ConsEgress: 1}, + {ConsIngress: 40, ConsEgress: 41}, + } + dpath.Base.PathMeta.CurrHF = 1 + dpath.InfoFields[0].ConsDir = false + dpath.HopFields[1].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[1]) + if !afterProcessing { + dpath.InfoFields[0].UpdateSegID(dpath.HopFields[1].Mac) + return toMsg(t, spkt, dpath) + } + require.NoError(t, dpath.IncPath()) + ret := toMsg(t, spkt, dpath) + ret.Addr = nil + ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil + return ret + }, + srcInterface: 1, + egressInterface: 2, + assertFunc: assert.NoError, }, "brtransit peering consdir": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -730,8 +765,10 @@ func TestProcessPkt(t *testing.T) { // parent hop of HF[1] in the original beaconned segment, // which is not in the path). So, we use one from an // info field because computeMAC makes that easy. - dpath.HopFields[1].Mac = computeMAC(t, key, dpath.InfoFields[1], dpath.HopFields[1]) - dpath.HopFields[2].Mac = computeMAC(t, otherKey, dpath.InfoFields[1], dpath.HopFields[2]) + dpath.HopFields[1].Mac = computeMAC( + t, key, dpath.InfoFields[1], dpath.HopFields[1]) + dpath.HopFields[2].Mac = computeMAC( + t, otherKey, dpath.InfoFields[1], dpath.HopFields[2]) if !afterProcessing { return toMsg(t, spkt, dpath) } @@ -745,9 +782,9 @@ func TestProcessPkt(t *testing.T) { ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil return ret }, - srcInterface: 1, // from peering link + srcInterface: 1, // from peering link egressInterface: 2, - assertFunc: assert.NoError, + assertFunc: assert.NoError, }, "brtransit peering non consdir": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -797,15 +834,17 @@ func TestProcessPkt(t *testing.T) { // parent hop of HF[1] in the original beaconned segment, // which is not in the path). So, we use one from an // info field because computeMAC makes that easy. - dpath.HopFields[0].Mac = computeMAC(t, otherKey, dpath.InfoFields[0], dpath.HopFields[0]) - dpath.HopFields[1].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[1]) - - // We're going against construction order, so the accumulator value - // is that of the previous hop in traversal order. The story starts - // with the packet arriving at hop 1, so the accumulator value - // must match hop field 0. In this case, it is identical to that for - // hop field 1, which we made identical to the original SegID. - // So, we're all set. + dpath.HopFields[0].Mac = computeMAC( + t, otherKey, dpath.InfoFields[0], dpath.HopFields[0]) + dpath.HopFields[1].Mac = computeMAC( + t, key, dpath.InfoFields[0], dpath.HopFields[1]) + + // We're going against construction order, so the accumulator + // value is that of the previous hop in traversal order. The + // story starts with the packet arriving at hop 1, so the + // accumulator value must match hop field 0. In this case, + // it is identical to that for hop field 1, which we made + // identical to the original SegID. So, we're all set. if !afterProcessing { return toMsg(t, spkt, dpath) } @@ -820,44 +859,172 @@ func TestProcessPkt(t *testing.T) { ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil return ret }, - srcInterface: 2, // from child link + srcInterface: 2, // from child link egressInterface: 1, - assertFunc: assert.NoError, + assertFunc: assert.NoError, }, - "brtransit non consdir": { + "peering consdir downstream": { + // Similar to previous test case but looking at what + // happens on the next hop. prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { return router.NewDP( map[uint16]router.BatchConn{ uint16(2): mock_router.NewMockBatchConn(ctrl), }, map[uint16]topology.LinkType{ - 2: topology.Parent, - 1: topology.Child, - }, nil, nil, nil, xtest.MustParseIA("1-ff00:0:110"), nil, key) + 1: topology.Peer, + 2: topology.Child, + }, + nil, nil, nil, xtest.MustParseIA("1-ff00:0:110"), nil, key) }, mockMsg: func(afterProcessing bool) *ipv4.Message { - spkt, dpath := prepBaseMsg(now) - dpath.HopFields = []path.HopField{ - {ConsIngress: 31, ConsEgress: 30}, - {ConsIngress: 2, ConsEgress: 1}, - {ConsIngress: 40, ConsEgress: 41}, + // Story: the packet just left hop 1 (the first hop + // of peering down segment 1) and is processed at hop 2 + // which is not a peering hop. + spkt, _ := prepBaseMsg(now) + dpath := &scion.Decoded{ + Base: scion.Base{ + PathMeta: scion.MetaHdr{ + CurrHF: 2, + CurrINF: 1, + SegLen: [3]uint8{1, 3, 0}, + }, + NumINF: 2, + NumHops: 4, + }, + InfoFields: []path.InfoField{ + // up seg + {SegID: 0x111, ConsDir: true, Timestamp: util.TimeToSecs(now), Peer: true}, + // core seg + {SegID: 0x222, ConsDir: true, Timestamp: util.TimeToSecs(now), Peer: true}, + }, + HopFields: []path.HopField{ + {ConsIngress: 31, ConsEgress: 30}, + {ConsIngress: 40, ConsEgress: 41}, + {ConsIngress: 1, ConsEgress: 2}, + {ConsIngress: 50, ConsEgress: 51}, + // There has to be a 4th hop to make + // the 3rd router agree that the packet + // is not at destination yet. + }, } - dpath.Base.PathMeta.CurrHF = 1 - dpath.InfoFields[0].ConsDir = false - dpath.HopFields[1].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[1]) + + // Make obvious the unusual aspect of the path: two + // hopfield MACs (1 and 2) derive from the same SegID + // accumulator value. The router shouldn't need to + // know this or do anything special. The SegID + // accumulator value can be anything (it comes from the + // parent hop of HF[1] in the original beaconned segment, + // which is not in the path). So, we use one from an + // info field because computeMAC makes that easy. + dpath.HopFields[1].Mac = computeMAC( + t, otherKey, dpath.InfoFields[1], dpath.HopFields[1]) + dpath.HopFields[2].Mac = computeMAC( + t, key, dpath.InfoFields[1], dpath.HopFields[2]) if !afterProcessing { - dpath.InfoFields[0].UpdateSegID(dpath.HopFields[1].Mac) + // The SegID we provide is that of HF[2] which happens to be SEG[1]'s SegID, + // so, already set. return toMsg(t, spkt, dpath) } - require.NoError(t, dpath.IncPath()) + _ = dpath.IncPath() + + // ... The SegID accumulator should have been updated. + dpath.InfoFields[1].UpdateSegID(dpath.HopFields[2].Mac) + ret := toMsg(t, spkt, dpath) ret.Addr = nil ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil return ret }, - srcInterface: 1, + srcInterface: 1, egressInterface: 2, - assertFunc: assert.NoError, + assertFunc: assert.NoError, + }, + "peering non consdir upstream": { + prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { + return router.NewDP( + map[uint16]router.BatchConn{ + uint16(1): mock_router.NewMockBatchConn(ctrl), + }, + map[uint16]topology.LinkType{ + 1: topology.Peer, + 2: topology.Child, + }, + nil, nil, nil, xtest.MustParseIA("1-ff00:0:110"), nil, key) + }, + mockMsg: func(afterProcessing bool) *ipv4.Message { + // Story: the packet lands on the second (non-peering) hop of + // segment 0 (a peering segment). After processing, the packet + // is ready to be processed by the third (peering) hop of segment 0. + spkt, _ := prepBaseMsg(now) + dpath := &scion.Decoded{ + Base: scion.Base{ + PathMeta: scion.MetaHdr{ + CurrHF: 1, + CurrINF: 0, + SegLen: [3]uint8{3, 1, 0}, + }, + NumINF: 2, + NumHops: 4, + }, + InfoFields: []path.InfoField{ + // up seg + {SegID: 0x111, ConsDir: false, Timestamp: util.TimeToSecs(now), Peer: true}, + // down seg + {SegID: 0x222, ConsDir: true, Timestamp: util.TimeToSecs(now), Peer: true}, + }, + HopFields: []path.HopField{ + {ConsIngress: 31, ConsEgress: 30}, + {ConsIngress: 1, ConsEgress: 2}, + {ConsIngress: 40, ConsEgress: 41}, + {ConsIngress: 50, ConsEgress: 51}, + // The second segment (4th hop) has to be + // there but the packet isn't processed + // at that hop for this test. + }, + } + + // Make obvious the unusual aspect of the path: two + // hopfield MACs (1 and 2) derive from the same SegID + // accumulator value. The SegID accumulator value can + // be anything (it comes from the parent hop of HF[1] + // in the original beaconned segment, which is not in + // the path). So, we use one from an info field because + // computeMAC makes that easy. + dpath.HopFields[1].Mac = computeMAC( + t, key, dpath.InfoFields[0], dpath.HopFields[1]) + dpath.HopFields[2].Mac = computeMAC( + t, otherKey, dpath.InfoFields[0], dpath.HopFields[2]) + + if !afterProcessing { + // We're going against construction order, so the + // before-processing accumulator value is that of + // the previous hop in traversal order. The story + // starts with the packet arriving at hop 1, so the + // accumulator value must match hop field 0, which + // derives from hop field[1]. HopField[0]'s MAC is + // not checked during this test. + dpath.InfoFields[0].UpdateSegID(dpath.HopFields[1].Mac) + + return toMsg(t, spkt, dpath) + } + + _ = dpath.IncPath() + + // After-processing, the SegID should have been updated + // (on ingress) to be that of HF[1], which happens to be + // the Segment's SegID. That is what we already have as + // we only change it in the before-processing version + // of the packet. + + ret := toMsg(t, spkt, dpath) + ret.Addr = nil + ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil + return ret + }, + srcInterface: 2, // from child link + egressInterface: 1, + assertFunc: assert.NoError, }, "astransit direct": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -886,9 +1053,9 @@ func TestProcessPkt(t *testing.T) { } return ret }, - srcInterface: 1, + srcInterface: 1, egressInterface: 0, - assertFunc: assert.NoError, + assertFunc: assert.NoError, }, "astransit xover": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -939,9 +1106,9 @@ func TestProcessPkt(t *testing.T) { ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil return ret }, - srcInterface: 51, + srcInterface: 51, egressInterface: 0, - assertFunc: assert.NoError, + assertFunc: assert.NoError, }, "svc": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -975,9 +1142,9 @@ func TestProcessPkt(t *testing.T) { } return ret }, - srcInterface: 1, + srcInterface: 1, egressInterface: 0, - assertFunc: assert.NoError, + assertFunc: assert.NoError, }, "svc nobackend": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -999,9 +1166,9 @@ func TestProcessPkt(t *testing.T) { ret := toMsg(t, spkt, dpath) return ret }, - srcInterface: 1, + srcInterface: 1, egressInterface: 0, - assertFunc: assertIsSCMPError(slayers.SCMPTypeDestinationUnreachable, 0), + assertFunc: assertIsSCMPError(slayers.SCMPTypeDestinationUnreachable, 0), }, "svc invalid": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -1023,9 +1190,9 @@ func TestProcessPkt(t *testing.T) { ret := toMsg(t, spkt, dpath) return ret }, - srcInterface: 1, + srcInterface: 1, egressInterface: 0, - assertFunc: assertIsSCMPError(slayers.SCMPTypeDestinationUnreachable, 0), + assertFunc: assertIsSCMPError(slayers.SCMPTypeDestinationUnreachable, 0), }, "onehop inbound": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -1086,9 +1253,9 @@ func TestProcessPkt(t *testing.T) { ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil return ret }, - srcInterface: 1, + srcInterface: 1, egressInterface: 0, - assertFunc: assert.NoError, + assertFunc: assert.NoError, }, "onehop inbound invalid src": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -1123,9 +1290,9 @@ func TestProcessPkt(t *testing.T) { } return toMsg(t, spkt, dpath) }, - srcInterface: 1, + srcInterface: 1, egressInterface: 21, - assertFunc: assert.Error, + assertFunc: assert.Error, }, "reversed onehop outbound": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -1183,9 +1350,9 @@ func TestProcessPkt(t *testing.T) { ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil return ret }, - srcInterface: 0, + srcInterface: 0, egressInterface: 1, - assertFunc: assert.NoError, + assertFunc: assert.NoError, }, "onehop outbound": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -1230,9 +1397,9 @@ func TestProcessPkt(t *testing.T) { ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil return ret }, - srcInterface: 0, + srcInterface: 0, egressInterface: 2, - assertFunc: assert.NoError, + assertFunc: assert.NoError, }, "invalid dest": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -1253,7 +1420,7 @@ func TestProcessPkt(t *testing.T) { ret := toMsg(t, spkt, dpath) return ret }, - srcInterface: 1, + srcInterface: 1, egressInterface: 0, assertFunc: assertIsSCMPError( slayers.SCMPTypeParameterProblem, @@ -1272,9 +1439,9 @@ func TestProcessPkt(t *testing.T) { prepareEpicCrypto(t, spkt, epicpath, dpath, key) return toIP(t, spkt, epicpath, afterProcessing) }, - srcInterface: 1, + srcInterface: 1, egressInterface: 0, - assertFunc: assert.NoError, + assertFunc: assert.NoError, }, "epic malformed path": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -1289,9 +1456,9 @@ func TestProcessPkt(t *testing.T) { // Wrong path type return toIP(t, spkt, &scion.Decoded{}, afterProcessing) }, - srcInterface: 1, + srcInterface: 1, egressInterface: 0, - assertFunc: assert.Error, + assertFunc: assert.Error, }, "epic invalid timestamp": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -1308,9 +1475,9 @@ func TestProcessPkt(t *testing.T) { prepareEpicCrypto(t, spkt, epicpath, dpath, key) return toIP(t, spkt, epicpath, afterProcessing) }, - srcInterface: 1, + srcInterface: 1, egressInterface: 0, - assertFunc: assert.Error, + assertFunc: assert.Error, }, "epic invalid LHVF": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { @@ -1327,9 +1494,9 @@ func TestProcessPkt(t *testing.T) { return toIP(t, spkt, epicpath, afterProcessing) }, - srcInterface: 1, + srcInterface: 1, egressInterface: 0, - assertFunc: assert.Error, + assertFunc: assert.Error, }, } From 3007b0e09c6f6359d1dafbf4e2f4d47c18af067e Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Fri, 15 Sep 2023 14:29:35 +0200 Subject: [PATCH 22/25] peering: use different keys for different ASes in braccept test. --- tools/braccept/cases/child_to_peer.go | 2 +- tools/braccept/cases/peer_to_child.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/braccept/cases/child_to_peer.go b/tools/braccept/cases/child_to_peer.go index 6aec7e9e6a..654b394643 100644 --- a/tools/braccept/cases/child_to_peer.go +++ b/tools/braccept/cases/child_to_peer.go @@ -111,7 +111,7 @@ func ChildToPeer(artifactsDir string, mac hash.Hash) runner.Case { if err != nil { panic(err) } - macGenY, err := scrypto.HFMacFactory([]byte("1234567812345678")) + macGenY, err := scrypto.HFMacFactory([]byte("abcdefghabcdefgh")) if err != nil { panic(err) } diff --git a/tools/braccept/cases/peer_to_child.go b/tools/braccept/cases/peer_to_child.go index 3fc9264cfc..d8d8dc595b 100644 --- a/tools/braccept/cases/peer_to_child.go +++ b/tools/braccept/cases/peer_to_child.go @@ -111,7 +111,7 @@ func PeerToChild(artifactsDir string, mac hash.Hash) runner.Case { if err != nil { panic(err) } - macGenY, err := scrypto.HFMacFactory([]byte("1234567812345678")) + macGenY, err := scrypto.HFMacFactory([]byte("abcdefghabcdefgh")) if err != nil { panic(err) } From 565bb739c4728eada3ba492024b617447c8f8285 Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Mon, 18 Sep 2023 17:47:10 +0200 Subject: [PATCH 23/25] peering: implement reviewers suggestion - simpler fake key gen in braccept test. --- tools/braccept/cases/child_to_peer.go | 8 ++++---- tools/braccept/cases/peer_to_child.go | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tools/braccept/cases/child_to_peer.go b/tools/braccept/cases/child_to_peer.go index 654b394643..5d0d1081e9 100644 --- a/tools/braccept/cases/child_to_peer.go +++ b/tools/braccept/cases/child_to_peer.go @@ -107,11 +107,11 @@ func ChildToPeer(artifactsDir string, mac hash.Hash) runner.Case { // Hops are all signed with different keys. Only HF[1] was signed by // the AS that we hand the packet to. The others can be anything as they // couldn't be check at that AS anyway. - macGenX, err := scrypto.HFMacFactory([]byte("1234567812345678")) + macGenX, err := scrypto.InitMac([]byte("1234567812345678")) if err != nil { panic(err) } - macGenY, err := scrypto.HFMacFactory([]byte("abcdefghabcdefgh")) + macGenY, err := scrypto.InitMac([]byte("abcdefghabcdefgh")) if err != nil { panic(err) } @@ -119,10 +119,10 @@ func ChildToPeer(artifactsDir string, mac hash.Hash) runner.Case { // HF[1] is a peering hop, so it has the same SegID acc value as the next one // in construction direction, HF[0]. That is, SEG[0]'s SegID. sp.HopFields[1].Mac = path.MAC(mac, sp.InfoFields[0], sp.HopFields[1], nil) - sp.HopFields[0].Mac = path.MAC(macGenX(), sp.InfoFields[0], sp.HopFields[0], nil) + sp.HopFields[0].Mac = path.MAC(macGenX, sp.InfoFields[0], sp.HopFields[0], nil) // The second segment has just one hop. - sp.HopFields[2].Mac = path.MAC(macGenY(), sp.InfoFields[1], sp.HopFields[2], nil) + sp.HopFields[2].Mac = path.MAC(macGenY, sp.InfoFields[1], sp.HopFields[2], nil) // The message is ready for ingest at A, that is at HF[1]. Going against consruction // direction, the SegID acc value must match that of HF[0], which is the same diff --git a/tools/braccept/cases/peer_to_child.go b/tools/braccept/cases/peer_to_child.go index d8d8dc595b..8c9d561fe9 100644 --- a/tools/braccept/cases/peer_to_child.go +++ b/tools/braccept/cases/peer_to_child.go @@ -107,22 +107,22 @@ func PeerToChild(artifactsDir string, mac hash.Hash) runner.Case { // Hops are all signed with different keys. Only HF[1] was signed by // the AS that we hand the packet to. The others can be anything as they // couldn't be check at that AS anyway. - macGenX, err := scrypto.HFMacFactory([]byte("1234567812345678")) + macGenX, err := scrypto.InitMac([]byte("1234567812345678")) if err != nil { panic(err) } - macGenY, err := scrypto.HFMacFactory([]byte("abcdefghabcdefgh")) + macGenY, err := scrypto.InitMac([]byte("abcdefghabcdefgh")) if err != nil { panic(err) } // HF[0] is a regular hop. - sp.HopFields[0].Mac = path.MAC(macGenX(), sp.InfoFields[0], sp.HopFields[0], nil) + sp.HopFields[0].Mac = path.MAC(macGenX, sp.InfoFields[0], sp.HopFields[0], nil) // HF[1] is a peering hop so it has the same SegID acc value as the next one // in construction direction, HF[2]. That is, SEG[1]'s SegID. sp.HopFields[1].Mac = path.MAC(mac, sp.InfoFields[1], sp.HopFields[1], nil) - sp.HopFields[2].Mac = path.MAC(macGenY(), sp.InfoFields[1], sp.HopFields[2], nil) + sp.HopFields[2].Mac = path.MAC(macGenY, sp.InfoFields[1], sp.HopFields[2], nil) // The message if ready for ingest at A, that is at HF[1], the start of the // second segment, in construction direction. So SegID is already correct. From 01a103d3fd243e69751a75b2167bdf449ed3b3fa Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Tue, 19 Sep 2023 13:33:52 +0200 Subject: [PATCH 24/25] peering: implement reviewer's request - No "TODO: " in error messages. In passing: made that and a couple of other error objects global. --- router/dataplane.go | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/router/dataplane.go b/router/dataplane.go index 990106eb62..cdb696edad 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -131,6 +131,11 @@ var ( noBFDSessionFound = serrors.New("no BFD sessions was found") noBFDSessionConfigured = serrors.New("no BFD sessions have been configured") errBFDDisabled = serrors.New("BFD is disabled") + errPeeringEmptySeg0 = serrors.New("zero-length segment[0] in peering path") + errPeeringEmptySeg1 = serrors.New("zero-length segment[1] in peering path") + errPeeringNonemptySeg2 = serrors.New("non-zero-length segment[2] in peering path") + errShortPacket = serrors.New("Packet is too short") + errBFDSessionDown = serrors.New("bfd session down") // zeroBuffer will be used to reset the Authenticator option in the // scionPacketProcessor.OptAuth zeroBuffer = make([]byte, 16) @@ -640,13 +645,13 @@ func computeProcID(data []byte, numProcRoutines int, randomValue []byte, flowIDBuffer []byte, hasher hash.Hash32) (uint32, error) { if len(data) < slayers.CmnHdrLen { - return 0, serrors.New("Packet is too short") + return 0, errShortPacket } dstHostAddrLen := slayers.AddrType(data[9] >> 4 & 0xf).Length() srcHostAddrLen := slayers.AddrType(data[9] & 0xf).Length() addrHdrLen := 2*addr.IABytes + srcHostAddrLen + dstHostAddrLen if len(data) < slayers.CmnHdrLen+addrHdrLen { - return 0, serrors.New("Packet is too short") + return 0, errShortPacket } copy(flowIDBuffer[0:3], data[1:4]) flowIDBuffer[0] &= 0xF // the left 4 bits don't belong to the flowID @@ -1108,23 +1113,21 @@ func (p *scionPacketProcessor) determinePeer() (processResult, error) { return processResult{}, nil } - // TODO: proper error - err := serrors.New("TODO: segment length error (peering)") - if p.path.PathMeta.SegLen[0] == 0 { - return processResult{}, err + return processResult{}, errPeeringEmptySeg0 } if p.path.PathMeta.SegLen[1] == 0 { - return processResult{}, err + return processResult{}, errPeeringEmptySeg1 + } if p.path.PathMeta.SegLen[2] != 0 { - return processResult{}, err + return processResult{}, errPeeringNonemptySeg2 } // The peer hop fields are the last hop field on the first path // segment (at SegLen[0] - 1) and the first hop field of the second // path segment (at SegLen[0]). The below check applies only - // because we already know this is a peering path. + // because we already know this is a well-formed peering path. currHF := p.path.PathMeta.CurrHF segLen := p.path.PathMeta.SegLen[0] p.peering = currHF == segLen-1 || currHF == segLen @@ -1441,7 +1444,7 @@ func (p *scionPacketProcessor) validateEgressUp() (processResult, error) { Egress: uint64(egressID), } } - return p.packSCMP(typ, 0, scmpP, serrors.New("bfd session down")) + return p.packSCMP(typ, 0, scmpP, errBFDSessionDown) } } return processResult{}, nil From f590c4e988b0847200f76234677f1c759194d6ed Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Tue, 19 Sep 2023 17:13:55 +0200 Subject: [PATCH 25/25] peering: fix code format. --- router/dataplane.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/router/dataplane.go b/router/dataplane.go index cdb696edad..d47348f9cd 100644 --- a/router/dataplane.go +++ b/router/dataplane.go @@ -135,7 +135,7 @@ var ( errPeeringEmptySeg1 = serrors.New("zero-length segment[1] in peering path") errPeeringNonemptySeg2 = serrors.New("non-zero-length segment[2] in peering path") errShortPacket = serrors.New("Packet is too short") - errBFDSessionDown = serrors.New("bfd session down") + errBFDSessionDown = serrors.New("bfd session down") // zeroBuffer will be used to reset the Authenticator option in the // scionPacketProcessor.OptAuth zeroBuffer = make([]byte, 16) @@ -1114,14 +1114,14 @@ func (p *scionPacketProcessor) determinePeer() (processResult, error) { } if p.path.PathMeta.SegLen[0] == 0 { - return processResult{}, errPeeringEmptySeg0 + return processResult{}, errPeeringEmptySeg0 } if p.path.PathMeta.SegLen[1] == 0 { - return processResult{}, errPeeringEmptySeg1 + return processResult{}, errPeeringEmptySeg1 } if p.path.PathMeta.SegLen[2] != 0 { - return processResult{}, errPeeringNonemptySeg2 + return processResult{}, errPeeringNonemptySeg2 } // The peer hop fields are the last hop field on the first path