Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @FR4NK-W and @matzf)

router/dataplane.go, line 1265 at r1 (raw file):

	if p.path.IsXover() {
		if r, err := p.doXover(); err != nil {

Should the validateHopExpiry and verifyCurrentMAC checks done after a Xover info and HF update also be moved here with the other validations?

Code quote:

p.doXover()

Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @FR4NK-W)

router/dataplane.go, line 1265 at r1 (raw file):

Good point, thanks! Done.

Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker and @matzf)

router/dataplane.go line 110 at r4 (raw file):

var (
	alreadySet                    = serrors.New("already set")
	invalidSrcIA                  = serrors.New("invalid source IA")

Suggestion:

ISD-AS

router/dataplane.go line 111 at r4 (raw file):

	alreadySet                    = serrors.New("already set")
	invalidSrcIA                  = serrors.New("invalid source IA")
	invalidDstIA                  = serrors.New("invalid destination IA")

Suggestion:

ISD-AS

router/dataplane.go line 915 at r4 (raw file):

		pktIngressID := p.ingressInterface()
		expectedSrc, ok := p.d.internalNextHops[pktIngressID]
		if !ok || !expectedSrc.IP.Equal(p.srcAddr.IP) || expectedSrc.Port != p.srcAddr.Port {

I think you should only check the IP address, but not the port here.

Checking the port prohibits source port randomization that could be used for load balancing (e.g., in LAG or ECMP)

Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

router/dataplane.go line 110 at r4 (raw file):

var (
	alreadySet                    = serrors.New("already set")
	invalidSrcIA                  = serrors.New("invalid source IA")

Done.

router/dataplane.go line 111 at r4 (raw file):

	alreadySet                    = serrors.New("already set")
	invalidSrcIA                  = serrors.New("invalid source IA")
	invalidDstIA                  = serrors.New("invalid destination IA")

Done.

router/dataplane.go line 915 at r4 (raw file):

Done, ok for me.

Just for the record, the behavior is different for the intra-AS links, where this router implementation uses a dialed UDP connection and consequently (implicitly) checks the source port as well.

Reviewable status: 0 of 10 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

a discussion (no related file):
Ping @oncilla, is this ok for you now?

Reviewed 2 of 10 files at r1, 1 of 1 files at r6, all commit messages.
Reviewable status: 3 of 10 files reviewed, 6 unresolved discussions (waiting on @matzf and @oncilla)

pkg/slayers/path/scion/base.go line 99 at r6 (raw file):

func (s *Base) infIndexForHF(hf uint8) uint8 {
	if s.NumINF >= 2 {

I think we could just make a switch out of this:

switch {
case s.NumINF >= 1 && hf < s.PathMeta.SegLen[0]:
     return 0
case s.NumINF >= 2 && hf < s.PathMeta.SegLen[0]+s.PathMeta.SegLen[1]:
     return 1
default:
    return uint8(s.NumINF - 1)
}

router/dataplane.go line 912 at r6 (raw file):

// SrcIA checks by disguising packets as transit traffic.
func (p *scionPacketProcessor) validateTransitUnderlaySrc() (processResult, error) {
	if p.ingressID == 0 && !p.path.IsFirstHop() {

you could use an early return here, for less indentation in the remaining code.

if p.path.IsFirstHop() || p.ingressID != 0 {
    return processResult{},nil
}

Reviewable status: 1 of 11 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

pkg/slayers/path/scion/base.go line 99 at r6 (raw file):

Good idea, thanks!
I opted to completely omit the NumINF checks and rely entirely on SegLen. This changes the behavior for out of range hf values (returns 2 instead of NumINF-1), so that the range check needs to be done at the caller (only for IsXover, other callers already check sufficiently). IMO this makes quite a bit easier to understand, also for IsXover.

router/dataplane.go line 912 at r6 (raw file):

Good idea, thanks. Done.

Reviewed 2 of 2 files at r7, 1 of 1 files at r9.
Reviewable status: 3 of 11 files reviewed, 5 unresolved discussions (waiting on @matzf and @oncilla)

pkg/slayers/path/scion/base.go line 99 at r6 (raw file):

couldn't you run into panics if the segments is invalid? i.e. accessing the SegLen out of bounds?

Reviewable status: 3 of 11 files reviewed, 5 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

pkg/slayers/path/scion/base.go line 99 at r6 (raw file):

No, I think not; SegLen is a fixed size array [3]uint8.

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: 4 of 11 files reviewed, 4 unresolved discussions (waiting on @oncilla)

Reviewed 6 of 10 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, 1 of 2 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FR4NK-W and @lukedirtwalker)

Reviewed 5 of 10 files at r1, 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matzf)