paths: Add check for hopfield count <= 64 when deserializing a scion path

[jiceatscion]

Added unit test for deserialization and unit test for router ingress.

Fixes #4482

[matzf]

This change is 

[matzf]

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

---

pkg/slayers/path/scion/decoded.go line 52 at r2 (raw file):

	// the length of a scion header. Yet a path of more than 64 hops cannot be followed to
	// the end because CurrHF is only 6 bits long.
	if s.NumHops > MaxHops {

This check now only applies to the Decoded variant of the SCION path representation, but we mainly use the Raw one e.g. in the router. The cleanest way to check in both cases, is to move this (back) to Base.

---

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

				dpath.Base.PathMeta.SegLen = [3]uint8{24, 24, 17}
				dpath.Base.NumINF = 3
				dpath.Base.NumHops = 65

As discussed, this seems to result in a broken path (possibly because only one info field is written).

[jiceatscion]

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

---

pkg/slayers/path/scion/decoded.go line 52 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

This check now only applies to the Decoded variant of the SCION path representation, but we mainly use the Raw one e.g. in the router. The cleanest way to check in both cases, is to move this (back) to Base.

Done.

---

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

Previously, matzf (Matthias Frei) wrote…

As discussed, this seems to result in a broken path (possibly because only one info field is written).

done

[matzf]

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jiceatscion)

---

pkg/slayers/path/scion/raw_test.go line 91 at r3 (raw file):

	assert.NoError(t, overlongPath.SerializeTo(b)) // permitted, if only to enable this test.
	s := &scion.Raw{}
	assert.Error(t, s.DecodeFromBytes(b)) // invalid raw packet.

Nit: Maybe also add the assert.ErrorIs check here to ensure we get the expected error and that the path is not broken in some other way.

---

router/dataplane_test.go line 670 at r3 (raw file):

				expected := serrors.New("NumHops too large",
					"NumHops", 65, "Maximum", scion.MaxHops).Error()
				return assert.Equal(t, expected, err.Error())

Nit: "serrors" can be compared with Is, and so assert.ErrorIs(t, serrors.New("...",...), err) can be used to shorten this.

Suggestion:

				expected := serrors.New("NumHops too large",
					"NumHops", 65, "Maximum", scion.MaxHops)
				return assert.ErrorIs(t, expected, err)

[jiceatscion]

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

---

pkg/slayers/path/scion/raw_test.go line 91 at r3 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit: Maybe also add the assert.ErrorIs check here to ensure we get the expected error and that the path is not broken in some other way.

done

---

router/dataplane_test.go line 670 at r3 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit: "serrors" can be compared with Is, and so assert.ErrorIs(t, serrors.New("...",...), err) can be used to shorten this.

It is indeed nicer. Unfortunately we just found that a bug in serror prevents doing this. Filed # #4486

[matzf]

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