Peering links continued

[jiceatscion]

Restarting where #4299 left off.

Added test cases.
Applied reviewer's suggestions.
Changed a field name that was miss-leading.
Added some comments.

---

This change is 

[matzf]

Reviewed 5 of 10 files at r1, 13 of 13 files at r2, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @jiceatscion)

---

control/beaconing/extender.go line 106 at r2 (raw file):

	// FIXME: why do we compute Beta(pseg) a second time? It hasn't changed since last
	// time.

+1, reusing the extractBeta(pseg) value would make this a lot clearer.

---

router/dataplane.go line 1035 at r2 (raw file):

	// 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

Nit: i find the term "peering AS" to be a bit inaccurate, I'd prefer "peering hop field". E.g. "peering indicates that the current hop is a peering hop field"

---

router/dataplane.go line 1291 at r2 (raw file):

	case ingress == topology.Child && egress == topology.Child:
		return processResult{}, nil
	case ingress == topology.Unset && egress == topology.Peer:

The comment above seems to say this that this case is an error, and think I agree.
I don't think a peering link could ever be a valid egress interface after a cross-over.

---

router/dataplane.go line 1367 at r2 (raw file):

	// 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

Nit. HOP -> hop (looks like an acronym otherwise)

---

router/dataplane_test.go line 727 at r2 (raw file):

				// 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])

Nit. Just to make sure there is no confusion here, the MAC for hop field 2 would in practice be computed with the secret key of a different AS. It might be good to illustrate this, by just using a different key.

---

router/dataplane_test.go line 741 at r2 (raw file):

				return ret
			},
			srcInterface: 1, // from peering link

Somewhat aside: it's a bit strange that this test harness does not assert that the egress interface (result.EgressID) is correct.

---

tools/braccept/main.go line 132 at r2 (raw file):

		cases.JumboPacket(artifactsDir, hfMAC),
		cases.ChildToChildPeeringOut(artifactsDir, hfMAC),
		cases.ChildToChildPeeringIn(artifactsDir, hfMAC),

Should we perhaps add (a) test(s) for processing the hop(s) before/after a peering hop?
The processing in these cases is identical to the various ChildToParent / ParentToChild test cases, the only difference is that the path segments have the peer flag set, which the router should effectively ignore.

Another case that might be interesting to cover would be PeerToInternalHost / InternalHostToPeer, i.e. a path ending/starting in a peering hop.

Note: all of these cases are also covered by the running the "end2end_integration" and scion_integration tests in the default topology, but having an explicit test could still be helpful.

---

tools/braccept/cases/child_to_child_peering_in.go line 1 at r2 (raw file):

// Copyright 2020 Anapaya Systems

// Copyright 2023 SCION Association

---

tools/braccept/cases/child_to_child_peering_in.go line 41 at r2 (raw file):

// 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 {

Following the naming scheme for the other braccept cases, this would be PeerToChild. The name does not describe the global path, but the hop that is processed at the router.

---

tools/braccept/cases/child_to_child_peering_in.go line 47 at r2 (raw file):

	}

	// We inject the packet into A (at I/F 121) as if coming from 2 (at I/F 211)

Nit. I believe we've usually abbreviated interface as "IF", not "I/F".

---

tools/braccept/cases/child_to_child_peering_in.go line 107 at r2 (raw file):

	// 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)

Same comment as in the unit test; computing the MAC with the same key (mac object contains key) may be misleading. If we want to set up the different MACs for illustration, we should use separate keys for the different ASes.

---

tools/braccept/cases/child_to_child_peering_out.go line 1 at r2 (raw file):

// Copyright 2020 Anapaya Systems

// Copyright 2023 SCION Association

---

tools/braccept/cases/child_to_child_peering_out.go line 41 at r2 (raw file):

// 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 {

ChildToPeer

[jiceatscion]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @matzf)

---

control/beaconing/extender.go line 106 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

+1, reusing the extractBeta(pseg) value would make this a lot clearer.

done

---

router/dataplane.go line 1035 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit: i find the term "peering AS" to be a bit inaccurate, I'd prefer "peering hop field". E.g. "peering indicates that the current hop is a peering hop field"

done

---

router/dataplane.go line 1367 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit. HOP -> hop (looks like an acronym otherwise)

done

---

router/dataplane_test.go line 727 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit. Just to make sure there is no confusion here, the MAC for hop field 2 would in practice be computed with the secret key of a different AS. It might be good to illustrate this, by just using a different key.

done

---

tools/braccept/cases/child_to_child_peering_in.go line 1 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

// Copyright 2023 SCION Association

done

---

tools/braccept/cases/child_to_child_peering_in.go line 41 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Following the naming scheme for the other braccept cases, this would be PeerToChild. The name does not describe the global path, but the hop that is processed at the router.

done

---

tools/braccept/cases/child_to_child_peering_in.go line 47 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit. I believe we've usually abbreviated interface as "IF", not "I/F".

done

---

tools/braccept/cases/child_to_child_peering_in.go line 107 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Same comment as in the unit test; computing the MAC with the same key (mac object contains key) may be misleading. If we want to set up the different MACs for illustration, we should use separate keys for the different ASes.

done

---

tools/braccept/cases/child_to_child_peering_out.go line 1 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

// Copyright 2023 SCION Association

done

---

tools/braccept/cases/child_to_child_peering_out.go line 41 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

ChildToPeer

done

[jiceatscion]

Reviewable status: 11 of 18 files reviewed, 8 unresolved discussions (waiting on @matzf)

---

router/dataplane.go line 1291 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

The comment above seems to say this that this case is an error, and think I agree.
I don't think a peering link could ever be a valid egress interface after a cross-over.

done

[jiceatscion]

Reviewable status: 11 of 18 files reviewed, 8 unresolved discussions (waiting on @matzf)

---

router/dataplane_test.go line 741 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Somewhat aside: it's a bit strange that this test harness does not assert that the egress interface (result.EgressID) is correct.

True, but if we change this we need to update every test case to provide the expected egress ID. Should we?

[jiceatscion]

Reviewable status: 11 of 18 files reviewed, 8 unresolved discussions (waiting on @matzf)

---

tools/braccept/main.go line 132 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Should we perhaps add (a) test(s) for processing the hop(s) before/after a peering hop?
The processing in these cases is identical to the various ChildToParent / ParentToChild test cases, the only difference is that the path segments have the peer flag set, which the router should effectively ignore.

Another case that might be interesting to cover would be PeerToInternalHost / InternalHostToPeer, i.e. a path ending/starting in a peering hop.

Note: all of these cases are also covered by the running the "end2end_integration" and scion_integration tests in the default topology, but having an explicit test could still be helpful.

Actually that's the case covered by the braccept test, while the unit test covers the two-routers with 4 hops case. But yeah, the later does not look at what happens downstream/upstream for the same packet. I guess I can add that, or did you mean something else?

Also, I'm not sure how the braccept and the unit tests really differ for the router. They seems to be testing the same thing, albeit with the accept test having to contend with a bunch of layers that are not the focus. So, not sure where adding a test case is more desirable.

[jiceatscion]

Reviewable status: 11 of 18 files reviewed, 8 unresolved discussions (waiting on @matzf)

---

router/dataplane_test.go line 741 at r2 (raw file):

Previously, jiceatscion wrote…

True, but if we change this we need to update every test case to provide the expected egress ID. Should we?

Ah, never mind I just did. And, yes we should. I think it found a couple of bugs...

[jiceatscion]

Reviewable status: 11 of 18 files reviewed, 7 unresolved discussions (waiting on @matzf)

---

router/dataplane_test.go line 741 at r2 (raw file):

Previously, jiceatscion wrote…

Ah, never mind I just did. And, yes we should. I think it found a couple of bugs...

Nah, my test was wrong. Fixed.
and so...
done

[matzf]

Reviewed 3 of 7 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jiceatscion)

---

control/beaconing/extender.go line 288 at r3 (raw file):

// 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?

"... to be added to be added ..."

Regarding the FIXME: first, I think there would be virtually no benefit in passing around an accumulator in the path segment. My gut feeling is that computing this value is just as cheap as having to include it in the protobuf message which would result in (tiny, but still) overheads in encoding, decoding, signature computations and storage.
Also, I think that trusting such a value would weaken the routing security properties in SCION, so we'd have to always check it by computing the XOR. Just now, I can't really put my finger on what the exact security issue would be though, to be honest.

---

tools/braccept/main.go line 132 at r2 (raw file):

But yeah, the later does not look at what happens downstream/upstream for the same packet. I guess I can add that, or did you mean something else?

Yes that is exactly what I meant.

The braccept vs unittests is an excellent question, and I don't really know the right answer. I can't see very strong reasons for either choice. It sounds like the unit test would be the more convenient option, so perhaps pick that one?

---

tools/braccept/cases/child_to_child_peering_in.go line 107 at r2 (raw file):

Previously, jiceatscion wrote…

done

X and Y are the same keys. Could we make these keys obviously different?
Btw, I think using scrypto.InitMac would be a tiny bit more obvious than the HFMacFactory thing.

[jiceatscion]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matzf)

---

tools/braccept/main.go line 132 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

But yeah, the later does not look at what happens downstream/upstream for the same packet. I guess I can add that, or did you mean something else?

Yes that is exactly what I meant.

The braccept vs unittests is an excellent question, and I don't really know the right answer. I can't see very strong reasons for either choice. It sounds like the unit test would be the more convenient option, so perhaps pick that one?

I agree the unit test is more convenient. You got it!

[jiceatscion]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matzf)

---

control/beaconing/extender.go line 288 at r3 (raw file):

Previously, matzf (Matthias Frei) wrote…

"... to be added to be added ..."

Regarding the FIXME: first, I think there would be virtually no benefit in passing around an accumulator in the path segment. My gut feeling is that computing this value is just as cheap as having to include it in the protobuf message which would result in (tiny, but still) overheads in encoding, decoding, signature computations and storage.
Also, I think that trusting such a value would weaken the routing security properties in SCION, so we'd have to always check it by computing the XOR. Just now, I can't really put my finger on what the exact security issue would be though, to be honest.

Ah, no. I didn't mean to pass it around during beaconing (although we do seem to find it economical during forwarding - but I also have unspecific security concerns anyway). I just meant to keep it as non-wire meta-data in paths structures so we don't compute it again and again while constructing a path object. Ok, maybe appending more than one hop at the same control service never happens. In that case, I'll remove my comment.

Fixed the echo.

[matzf]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion)

---

control/beaconing/extender.go line 288 at r3 (raw file):

Previously, jiceatscion wrote…

Ah, no. I didn't mean to pass it around during beaconing (although we do seem to find it economical during forwarding - but I also have unspecific security concerns anyway). I just meant to keep it as non-wire meta-data in paths structures so we don't compute it again and again while constructing a path object. Ok, maybe appending more than one hop at the same control service never happens. In that case, I'll remove my comment.

Fixed the echo.

Ah, I see, sure. Yes, we do extend the same path segments multiple times in the same control service.

[jiceatscion]

Reviewable status: 17 of 18 files reviewed, 1 unresolved discussion (waiting on @matzf)

---

tools/braccept/cases/child_to_child_peering_in.go line 107 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

X and Y are the same keys. Could we make these keys obviously different?
Btw, I think using scrypto.InitMac would be a tiny bit more obvious than the HFMacFactory thing.

Oops, I was meaning to change one the keys and forgot. Re. InitMac(). Can't. path.Mac() wants the generator, not the key. (WTF do we need a generator made by a factory? Hopefully we'll never feel the need for a FactoryGenerator)

Anyway.

done

[jiceatscion]

Reviewable status: 17 of 18 files reviewed, all discussions resolved (waiting on @matzf)

---

tools/braccept/main.go line 132 at r2 (raw file):

Previously, jiceatscion wrote…

I agree the unit test is more convenient. You got it!

done.

[matzf]

Reviewed 1 of 1 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)

---

tools/braccept/cases/child_to_child_peering_in.go line 107 at r2 (raw file):

Can't. path.Mac() wants the generator, not the key

No, this does not seem right. path.MAC wants a hash.Hash, the thing returned from scrypto.InitMac(). The HFMacFactory returns the generator, func() hash.Hash, which we now (need to) invoke in the call to path.MAC.
The generator thing is quite useless...

[jiceatscion]

Reviewable status: 16 of 18 files reviewed, all discussions resolved (waiting on @matzf)

---

tools/braccept/cases/child_to_child_peering_in.go line 107 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Can't. path.Mac() wants the generator, not the key

No, this does not seem right. path.MAC wants a hash.Hash, the thing returned from scrypto.InitMac(). The HFMacFactory returns the generator, func() hash.Hash, which we now (need to) invoke in the call to path.MAC.
The generator thing is quite useless...

Uh...that's right. I got lost through the redirections. Fixed.

[matzf]

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)

[matzf]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion)

---

router/dataplane.go line 1112 at r10 (raw file):

	// TODO: proper error
	err := serrors.New("TODO: segment length error (peering)")

Cross-checking with the review comments in #4299; this was commented on to be fixed.
The default approach in this package is to define the error in the corresponding global var block e.g. as

invalidPeerPath = serrors.New("invalid segment lengths for peering path")

[jiceatscion]

Reviewable status: 17 of 18 files reviewed, 1 unresolved discussion (waiting on @matzf)

---

router/dataplane.go line 1112 at r10 (raw file):

Previously, matzf (Matthias Frei) wrote…

Cross-checking with the review comments in #4299; this was commented on to be fixed.
The default approach in this package is to define the error in the corresponding global var block e.g. as

invalidPeerPath = serrors.New("invalid segment lengths for peering path")

Done.

[jiceatscion]

Reviewable status: 17 of 18 files reviewed, 1 unresolved discussion (waiting on @matzf)

---

router/dataplane.go line 1112 at r10 (raw file):

Previously, jiceatscion wrote…

Done.

Uh... at least I thought that's what I'd done. Does this still need more changes? I do not understand what you are referring to by "in the corresponding global var block". There's only one block.

[jiceatscion]

Reviewable status: 17 of 18 files reviewed, 1 unresolved discussion (waiting on @matzf)

---

router/dataplane.go line 1112 at r10 (raw file):

Previously, jiceatscion wrote…

Uh... at least I thought that's what I'd done. Does this still need more changes? I do not understand what you are referring to by "in the corresponding global var block". There's only one block.

Ah, got it. You were commenting on revision 10.
So, done.

[matzf]

Reviewed 1 of 1 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)