colibri border router

[mawyss]

Implementation of the colibri library, the colibri packet format, and the border router forwarding logic.

Overview:

• go/lib/colibri/: contains methods for the creation and verification of the colibri packet timestamp and validation fields.
• go/lib/slayers/path/colibri/: implements the colibri path type layer. There is a full representation of a colibri path (colibri.go) and a minimal representation (colibri_minimal.go), which is optimized for the border router.
• go/pkg/router/colibri_processing.go: the actual router forwarding logic.

---

This change is 

[juagargi]

Very nice !
Thank you @mawyss for this contribution. I left only some comments about small issues I found.

Reviewed 21 of 21 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @mawyss)

---

go/lib/colibri/colibri.go, line 17 at r1 (raw file):

// Package colibri contains methods for the creation and verification of the colibri packet
// timestamp and validation fields.
package colibri

Since the file and package contain only functions related to COLIBRI MAC, what about changing the name of the file and package?

---

go/lib/colibri/colibri_test.go, line 242 at r1 (raw file):

func randUint64() uint64 {
	return uint64(rand.Uint32())<<32 + uint64(rand.Uint32())

We should try not to use random input in our unit tests. There are several reasons for this, among others:

• Difficult to estimate coverage.
• Difficult (sometimes) to reproduce.
• Sometimes frees us from looking for edge cases, under the false pretense they will be eventually caught.
• Fuzzing does exactly this ("random" parameters), but not in the unit tests.

---

go/lib/slayers/path/colibri/colibri.go, line 24 at r1 (raw file):

)

type ColibriPath struct {

Where is the ColibriPath used at?
Why do we need a different type than ColibriPathMinimal ?

---

go/lib/slayers/path/colibri/colibri_minimal.go, line 40 at r1 (raw file):

}

type ColibriPathMinimal struct {

It would be nice to have some doc string here.

---

go/lib/slayers/path/colibri/colibri_minimal.go, line 120 at r1 (raw file):

// serialized. Then the Raw buffer is copied to b.
func (c *ColibriPathMinimal) SerializeTo(b []byte) error {
	if len(b) < c.Len() {

I realize that the signature of the function comes from an interface, and we don't want to change it. Since it doesn't return the length of the bytes serialized, can we check that the buffer is exactly Len() bytes long?

Same would apply for SerializeToInternal

---

go/lib/slayers/path/colibri/colibri_minimal.go, line 176 at r1 (raw file):

}

// UpdateCurrHF increases/decreases the CurrHF index depending on the R flag.

We should update the comment to indicate it doesn't depend on R anymore.

---

go/lib/slayers/path/colibri/hopfield_test.go, line 66 at r1 (raw file):

func randBytes(l uint16) []byte {
	r := make([]byte, l)
	rand.Read(r)

As mentioned in the colibri_test, let's avoid random inputs.

---

go/pkg/router/colibri_processing.go, line 146 at r1 (raw file):

	if c.ingressID == 0 {
		// Received packet from within AS

According to your comment, we could use this fact to check that if C=1 the sender's address is that of a COLIBRI service.
(we will assume that the AS checks no spoofing for the service address happens).
This can be done in a later iteration, it's not necessary to implement it in this PR 😃

---

go/pkg/router/colibri_processing.go, line 163 at r1 (raw file):

				return r, err
			}
			return c.forwardToRemoteEgress(egressId)

We are ignoring errors from forwardToLocalEgress. Why not make the decision here of BR transit or AS transit?

---

go/pkg/router/control/conf.go, line 119 at r1 (raw file):

// DeriveColibriKey derives the private Colibri key from the given key.
func DeriveColibriKey(k []byte) []byte {

Why not use the scrypto.DeriveColibriKey function?
I also wonder why the regular SCION DeriveHFMacKey is not using scrypto as well.

[mawyss]

Reviewable status: 17 of 23 files reviewed, 9 unresolved discussions (waiting on @juagargi)

---

go/lib/colibri/colibri.go, line 17 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Since the file and package contain only functions related to COLIBRI MAC, what about changing the name of the file and package?

The package also contains methods for the creation/verification of timestamps (expiration tick and high precision packet timestamp). It is possible that the colibri package will need to be extended later to the requirements of the end hosts, so I did not go for a more specific name. For the epic PR I received feedback to name the package simply "epic" (the package contains also methods regarding timestamps and MACs), so having "colibri" here would be consistent. Or do you anticipate having a colibri library package for the colibri service, so that it conflicts with this one?

---

go/lib/colibri/colibri_test.go, line 242 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

We should try not to use random input in our unit tests. There are several reasons for this, among others:

• Difficult to estimate coverage.
• Difficult (sometimes) to reproduce.
• Sometimes frees us from looking for edge cases, under the false pretense they will be eventually caught.
• Fuzzing does exactly this ("random" parameters), but not in the unit tests.

Done. Replaced random input with static values.

---

go/lib/slayers/path/colibri/colibri.go, line 24 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Where is the ColibriPath used at?
Why do we need a different type than ColibriPathMinimal ?

The ColibriPath is currently used in the router unit tests (dataplane_test.go). It will be needed to create colibri packets at the hosts later. Note that also the SCION path type has different representations (decoded vs. raw).

---

go/lib/slayers/path/colibri/colibri_minimal.go, line 40 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

It would be nice to have some doc string here.

Done.

---

go/lib/slayers/path/colibri/colibri_minimal.go, line 120 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

I realize that the signature of the function comes from an interface, and we don't want to change it. Since it doesn't return the length of the bytes serialized, can we check that the buffer is exactly Len() bytes long?

Same would apply for SerializeToInternal

I am not sure whether this would work: usually there is a buffer (e.g. 1500 bytes), and each layer gets serialized one after the other, so the buffer can actually be larger than c.Len(). Also the SCION path type implementation checks for "<" and not for "=".

---

go/lib/slayers/path/colibri/colibri_minimal.go, line 176 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

We should update the comment to indicate it doesn't depend on R anymore.

Done.

---

go/lib/slayers/path/colibri/hopfield_test.go, line 66 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

As mentioned in the colibri_test, let's avoid random inputs.

Done.

---

go/pkg/router/colibri_processing.go, line 146 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

According to your comment, we could use this fact to check that if C=1 the sender's address is that of a COLIBRI service.
(we will assume that the AS checks no spoofing for the service address happens).
This can be done in a later iteration, it's not necessary to implement it in this PR 😃

Ok, let's do that later. We need to be a bit careful at on-path ASes, because the source host in the address header of the COLIBRI packet denotes a service within the first AS (if it is an IP address).
Could it make sense to only use service identifiers for C=1 instead of actual IPs?

---

go/pkg/router/colibri_processing.go, line 163 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

We are ignoring errors from forwardToLocalEgress. Why not make the decision here of BR transit or AS transit?

I am not sure what you mean: the decision of BR transit or AS transit is actually made at this point. The router tries to forward the packet itself to the given egress interface (BR transit), and if that fails (because the router is not responsible for that interface) it forwards the packet to the appropriate border router (AS transit).

---

go/pkg/router/control/conf.go, line 119 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Why not use the scrypto.DeriveColibriKey function?
I also wonder why the regular SCION DeriveHFMacKey is not using scrypto as well.

Good question. I was asking this myself too, and decided to implement it in the same way as with standard SCION to omit any undesirable side effects.
My guess would be that the router should be light-weight, and not need to import also asymmetric crypto functionality as would be the case in the scrypto package.

[juagargi]

Reviewed 1 of 11 files at r2.
Reviewable status: 13 of 23 files reviewed, 2 unresolved discussions (waiting on @juagargi and @mawyss)

---

go/lib/colibri/colibri.go, line 17 at r1 (raw file):

Previously, mawyss wrote…

The package also contains methods for the creation/verification of timestamps (expiration tick and high precision packet timestamp). It is possible that the colibri package will need to be extended later to the requirements of the end hosts, so I did not go for a more specific name. For the epic PR I received feedback to name the package simply "epic" (the package contains also methods regarding timestamps and MACs), so having "colibri" here would be consistent. Or do you anticipate having a colibri library package for the colibri service, so that it conflicts with this one?

Alright, let's keep it like that at least for now (or forever).

---

go/lib/slayers/path/colibri/colibri.go, line 24 at r1 (raw file):

Previously, mawyss wrote…

The ColibriPath is currently used in the router unit tests (dataplane_test.go). It will be needed to create colibri packets at the hosts later. Note that also the SCION path type has different representations (decoded vs. raw).

👍

---

go/lib/slayers/path/colibri/colibri_minimal.go, line 120 at r1 (raw file):

Previously, mawyss wrote…

I am not sure whether this would work: usually there is a buffer (e.g. 1500 bytes), and each layer gets serialized one after the other, so the buffer can actually be larger than c.Len(). Also the SCION path type implementation checks for "<" and not for "=".

I marked this comment as resolved, as the discussion is non blocking for the PR, and only informative.

Structs like InfoField are serialized only from this code. In this code we call their SerializeTo with a slice with the exact size. They could check for the exact size.

What I meant is that these methods SerializeTo(_ []byte) come from the Path interface. It is called upon a Path object in SCION.SerializeTo
https://github.com/mawyss/scion/blob/colibri_dataplane/go/lib/slayers/scion.go#L181
where the buf is allocated with the size computed from calling Len (so the exact size).

I would (but not now) change the interface of SerializeTo, so that it would return the size of the serialized version, so that it can be called upon a big enough buffer (with more bytes than just Len) that does not need to be reallocated every time. But we are not going to change the interface now: that is what I meant with "don't want to change it").
After checking the rest of the implementers of the Path.SerializeTo prototype, I see that all check for the length to be sufficient --and not exact--, so I actually prefer to leave it as you coded it.

---

go/pkg/router/colibri_processing.go, line 163 at r1 (raw file):

Previously, mawyss wrote…

I am not sure what you mean: the decision of BR transit or AS transit is actually made at this point. The router tries to forward the packet itself to the given egress interface (BR transit), and if that fails (because the router is not responsible for that interface) it forwards the packet to the appropriate border router (AS transit).

Let's trace a call to this function foward(): let's say the packet has C=0, so we arrive at:

	} else {
		// Data plane forwarding
		if c.destinedToLocalHost(egressId) {
			return c.forwardToLocalHost()
		} else {
			if r, err := c.forwardToLocalEgress(egressId); err == nil {
				return r, err
			}
			return c.forwardToRemoteEgress(egressId)
		}
	}

In there, let's say that destinedToLocalHost returns false. We end up calling forwardToLocalEgress. Let's say that forwardToLocalEgress returns an error: looking at that function, a problem updating the current hop field or serializing would do. Then, since we have:

			if r, err := c.forwardToLocalEgress(egressId); err == nil {
				return r, err
			}
			return c.forwardToRemoteEgress(egressId)

we will directly call forwardToRemoteEgress. But the packet was meant to be forwarded to local egress, and to report an error. Not to be sent to remote egress.

---

go/pkg/router/control/conf.go, line 119 at r1 (raw file):

Previously, mawyss wrote…

Good question. I was asking this myself too, and decided to implement it in the same way as with standard SCION to omit any undesirable side effects.
My guess would be that the router should be light-weight, and not need to import also asymmetric crypto functionality as would be the case in the scrypto package.

Idk. To be honest, I would change this and also the regular SCION to use the respective functions from scrypto.
Please leave a TODO(mawyss) so we (or someone else) will check at a later point (or remove the TODO).

[mawyss]

Reviewable status: 13 of 23 files reviewed, 2 unresolved discussions (waiting on @juagargi)

---

go/lib/colibri/colibri.go, line 17 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Alright, let's keep it like that at least for now (or forever).

Ok.

---

go/lib/slayers/path/colibri/colibri_minimal.go, line 120 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

I marked this comment as resolved, as the discussion is non blocking for the PR, and only informative.

Structs like InfoField are serialized only from this code. In this code we call their SerializeTo with a slice with the exact size. They could check for the exact size.

What I meant is that these methods SerializeTo(_ []byte) come from the Path interface. It is called upon a Path object in SCION.SerializeTo
https://github.com/mawyss/scion/blob/colibri_dataplane/go/lib/slayers/scion.go#L181
where the buf is allocated with the size computed from calling Len (so the exact size).

I would (but not now) change the interface of SerializeTo, so that it would return the size of the serialized version, so that it can be called upon a big enough buffer (with more bytes than just Len) that does not need to be reallocated every time. But we are not going to change the interface now: that is what I meant with "don't want to change it").
After checking the rest of the implementers of the Path.SerializeTo prototype, I see that all check for the length to be sufficient --and not exact--, so I actually prefer to leave it as you coded it.

Ok, then let's keep this then.

---

go/pkg/router/colibri_processing.go, line 163 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Let's trace a call to this function foward(): let's say the packet has C=0, so we arrive at:

	} else {
		// Data plane forwarding
		if c.destinedToLocalHost(egressId) {
			return c.forwardToLocalHost()
		} else {
			if r, err := c.forwardToLocalEgress(egressId); err == nil {
				return r, err
			}
			return c.forwardToRemoteEgress(egressId)
		}
	}

In there, let's say that destinedToLocalHost returns false. We end up calling forwardToLocalEgress. Let's say that forwardToLocalEgress returns an error: looking at that function, a problem updating the current hop field or serializing would do. Then, since we have:

			if r, err := c.forwardToLocalEgress(egressId); err == nil {
				return r, err
			}
			return c.forwardToRemoteEgress(egressId)

we will directly call forwardToRemoteEgress. But the packet was meant to be forwarded to local egress, and to report an error. Not to be sent to remote egress.

Done. The errors should now be properly returned.

---

go/pkg/router/control/conf.go, line 119 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Idk. To be honest, I would change this and also the regular SCION to use the respective functions from scrypto.
Please leave a TODO(mawyss) so we (or someone else) will check at a later point (or remove the TODO).

Done. I added a comment regarding this. In the scionproto slack I asked about the reason for this, so we will hopefully know soon.

[juagargi]

Reviewed 9 of 11 files at r2, 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @mawyss)