spao: Update timestamp / Sequence Number field

[JordiSubira]

This PR introduces modification on the SPAO header. More concretely, the changes concerning the Timestamp / Sequence Number field specified in #4300.

---

This change is 

[matzf]

Reviewed 18 of 18 files at r1, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @JordiSubira)

---

control/config/drkey.go line 41 at r1 (raw file):

	// Picking the value equal or shorter than half of the drkey Grace Period ensures
	// that we accept packets for active keys only.
	DefaultAcceptanceWindowOffset = 2*time.Second + 500*time.Millisecond

Naming inconsistency; this definition has Offset, but other definitions with the exact same interpretation are just "acceptance window". This naming inconsistency could be confusing, as it's not super obvious everywhere whether this is the half-window or the full window duration.

---

control/config/drkey.go line 42 at r1 (raw file):

	// that we accept packets for active keys only.
	DefaultAcceptanceWindowOffset = 2*time.Second + 500*time.Millisecond
	EnvVarAccpetanceWindow        = "SCION_TESTING_ACCEPTANCE_WINDOW"

Suggestion:

	EnvVarAcceptanceWindow

---

control/config/drkey.go line 42 at r1 (raw file):

	// that we accept packets for active keys only.
	DefaultAcceptanceWindowOffset = 2*time.Second + 500*time.Millisecond
	EnvVarAccpetanceWindow        = "SCION_TESTING_ACCEPTANCE_WINDOW"

As these are now read by the router, having these definitions in control/config does not seem appropriate.
Perhaps the new private/drkey package that you've added would be a good place for these things? Then you could keep all constants local and expose only the function that returns the effective value (and by that avoid the duplication of the logic for parsing the environment variable, that we now have).

---

pkg/slayers/pkt_auth.go line 171 at r1 (raw file):

	if p.TimestampSN >= (1 << 48) {
		return serrors.New("Timestamp value should be smaller than 2^24")

Suggestion:

should be smaller than 2^48

---

pkg/slayers/pkt_auth.go line 187 at r1 (raw file):

	o.OptData[6] = byte(p.TimestampSN >> 40)
	o.OptData[7] = byte(p.TimestampSN >> 32)
	binary.BigEndian.PutUint32(o.OptData[8:12], uint32(p.TimestampSN))

Nit: separately reading/writing the first two bytes seems a bit awkward and looks like it could easily be forgotten. I'd either handle all six bytes explicitly here, or create simple helper functions to do this (something like bigEndianPutUint48 and bigEndianUint48).

---

pkg/slayers/pkt_auth_test.go line 65 at r1 (raw file):

			spiFunc: initSPI,
			algo:    algo,
			ts: binary.LittleEndian.Uint64(

Aside: the value is big-endian (right!?), so the LittleEndian here seems a bit odd. It's probably clearer to just specify a uint64 value anyway (like ts in the global var-block).
I remember that we had discussed and changed this in the original PR, put it seems we missed some places.

---

pkg/spao/mac.go line 133 at r1 (raw file):

	buf[6] = byte(opt.TimestampSN() >> 40)
	buf[7] = byte(opt.TimestampSN() >> 32)
	binary.BigEndian.PutUint32(buf[8:12], uint32(opt.TimestampSN()))

Same as above, create helper function for 48 bit write

---

pkg/spao/timestamp.go line 26 at r1 (raw file):

// RelativeTimestamp returns the relative timestamp (RelTime) as the time diference from
// time instant t to the beginning of the drkey epoch.
func RelativeTimestamp(key drkey.ASHostKey, t time.Time) (uint64, error) {

Shouldn't this also be usable for HostHostKey? If so, just pass the Epoch directly?

---

pkg/spao/timestamp.go line 35 at r1 (raw file):

// AbsoluteTimestamp returns the absolute timestamp (AbsTime) based on the
// relTime (Timestamp / Sequence Number field in SPAO hedaer) and the DRKey

Suggestion:

header

---

private/drkey/BUILD.bazel line 6 at r1 (raw file):

    name = "go_default_library",
    srcs = ["provider.go"],
    importpath = "github.com/scionproto/scion/private/drkey",

Seems to be imported as drkeytools everywhere, so perhaps create this as private/drkey/drkeytools directly?

Maybe drkeyutil would be slightly more conventional than ...tools (standard library has ioutil, httputil) , but ok for me either way.

---

private/drkey/provider.go line 27 at r1 (raw file):

type FakeProvider struct {
	KeyDuration      time.Duration

EpochDuration

---

private/drkey/provider.go line 58 at r1 (raw file):

	}

	awBegin := t.Add(-(p.AcceptanceWindow + time.Nanosecond))

Use ! Before instead of subtracting Nanosecond and After?

Btw, is it intentional that the awEnd is not included in the allowed range?

FWIW, you could use cppki.Validity to represent the acceptance window, then the expressions below can be simplified a bit by using .Contains.

---

private/drkey/provider.go line 78 at r1 (raw file):

}

func (p *FakeProvider) getASHostTreble(

Triple?

---

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

	)
	if err != nil {
		log.Error("Selecting key to authenticate the incoming packet", "err", err)

log.Debug

---

tools/braccept/main.go line 79 at r1 (raw file):

	// Set acceptance window to longer than default value to handle with test time fluctuation
	err = os.Setenv(config.EnvVarAccpetanceWindow, "1m")

The +/- 2.5 seconds are not enough for the tests to pass otherwise? Any idea where so much delay is added?

---

tools/braccept/cases/scmp_expired_hop.go line 197 at r1 (raw file):

// SCMPExpiredHopMessageBack tests a packet with an expired hop field. The relative timestamp
// can be encoded in the SPAO header and sent back to the src.
func SCMPExpiredHopMessageBack(artifactsDir string, mac hash.Hash) runner.Case {

Can this case now be removed?

[JordiSubira]

Reviewable status: 3 of 24 files reviewed, 14 unresolved discussions (waiting on @matzf)

---

control/config/drkey.go line 41 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Naming inconsistency; this definition has Offset, but other definitions with the exact same interpretation are just "acceptance window". This naming inconsistency could be confusing, as it's not super obvious everywhere whether this is the half-window or the full window duration.

Done.

---

control/config/drkey.go line 42 at r1 (raw file):

	// that we accept packets for active keys only.
	DefaultAcceptanceWindowOffset = 2*time.Second + 500*time.Millisecond
	EnvVarAccpetanceWindow        = "SCION_TESTING_ACCEPTANCE_WINDOW"

Done.

---

control/config/drkey.go line 42 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

As these are now read by the router, having these definitions in control/config does not seem appropriate.
Perhaps the new private/drkey package that you've added would be a good place for these things? Then you could keep all constants local and expose only the function that returns the effective value (and by that avoid the duplication of the logic for parsing the environment variable, that we now have).

Done.

---

pkg/slayers/pkt_auth.go line 171 at r1 (raw file):

	if p.TimestampSN >= (1 << 48) {
		return serrors.New("Timestamp value should be smaller than 2^24")

Done.

---

pkg/slayers/pkt_auth_test.go line 65 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Aside: the value is big-endian (right!?), so the LittleEndian here seems a bit odd. It's probably clearer to just specify a uint64 value anyway (like ts in the global var-block).
I remember that we had discussed and changed this in the original PR, put it seems we missed some places.

Done.

---

pkg/spao/mac.go line 133 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Same as above, create helper function for 48 bit write

Done.

---

pkg/spao/timestamp.go line 26 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Shouldn't this also be usable for HostHostKey? If so, just pass the Epoch directly?

Done.

---

pkg/spao/timestamp.go line 35 at r1 (raw file):

// AbsoluteTimestamp returns the absolute timestamp (AbsTime) based on the
// relTime (Timestamp / Sequence Number field in SPAO hedaer) and the DRKey

Done.

---

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

Previously, matzf (Matthias Frei) wrote…

log.Debug

Done.

---

tools/braccept/main.go line 79 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

The +/- 2.5 seconds are not enough for the tests to pass otherwise? Any idea where so much delay is added?

Done. At the time, some test were not passing for me, however, now they seem to consistently pass.

---

tools/braccept/cases/scmp_expired_hop.go line 197 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Can this case now be removed?

Done.

---

private/drkey/BUILD.bazel line 6 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Seems to be imported as drkeytools everywhere, so perhaps create this as private/drkey/drkeytools directly?

Maybe drkeyutil would be slightly more conventional than ...tools (standard library has ioutil, httputil) , but ok for me either way.

Done.

---

private/drkey/provider.go line 27 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

EpochDuration

Done.

---

private/drkey/provider.go line 58 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Use ! Before instead of subtracting Nanosecond and After?

Btw, is it intentional that the awEnd is not included in the allowed range?

FWIW, you could use cppki.Validity to represent the acceptance window, then the expressions below can be simplified a bit by using .Contains.

Yes, there's a reason. In the specification we say that a (the width of the acceptance window, can be smaller or equal to the minimum DRKey epoch length. If the case is that a it is equal and we take awEnd as included we can fall into the following situation:

• e.g. current epoch E_i, starts at t_i, next epoch E_i+1 starts at t_i+1.
• Relative timestamp is 0.
• We are exactly in E_i/2 (E_i being in the exact middle point of epoch i).
• Then instants t_i and t_i+1 would be within the acceptance window. This is not how is described in "Absolute time and DRKey selection".

In any case, I can change the sentence in the specification and use here the validity.Contains function. Instead of "The acceptance window is equal or smaller than the minimum DRKey epoch length" only "The acceptance window is smaller than the minimum DRKey epoch length".

---

private/drkey/provider.go line 78 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Triple?

Done. Although, I think treble is valid as well, meaning three times something...

[matzf]

Reviewed 21 of 21 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @JordiSubira)

---

pkg/slayers/pkt_auth.go line 217 at r2 (raw file):

}

func bigEndian(b []byte) uint64 {

Nit, bigEndianUint48 maybe?

---

pkg/spao/timestamp_test.go line 36 at r2 (raw file):

		"valid": {
			currentTime: time.Now().UTC(),
			epoch:       getEpoch(time.Now()),

In contrast to the other calls here, This time.Now() does not have the .UTC(). Is this still correct? Could we drop all of the others as well?

---

private/drkey/drkeyutil/drkey.go line 27 at r2 (raw file):

	// DefaultEpochDuration is the default duration for the drkey SecretValue and derived keys
	DefaultEpochDuration   = 24 * time.Hour
	DefaultPrefetchEntries = 10000

This one seems to be a bit out of place here, I'd leave this in control/config/drkey.go.

---

private/drkey/drkeyutil/drkey.go line 36 at r2 (raw file):

	// Picking the value equal or shorter than half of the drkey Grace Period ensures
	// that we accept packets for active keys only.
	DefaultAcceptanceWindowLength = 5 * time.Minute

The description is incorrect now, as the length defines the full acceptance window size.
I'd also rename the DefaultAcceptanceWindowLength to just DefaultAcceptanceWindow to avoid having two different names.

• widown -> window
• acceptanceWindowOffset -> acceptance window

---

private/drkey/drkeyutil/drkey.go line 37 at r2 (raw file):

	// that we accept packets for active keys only.
	DefaultAcceptanceWindowLength = 5 * time.Minute
	EnvVarAccpetanceWindow        = "SCION_TESTING_ACCEPTANCE_WINDOW"

Suggestion:

EnvVarAcceptanceWindow

---

private/drkey/drkeyutil/drkey.go line 37 at r2 (raw file):

	// that we accept packets for active keys only.
	DefaultAcceptanceWindowLength = 5 * time.Minute
	EnvVarAccpetanceWindow        = "SCION_TESTING_ACCEPTANCE_WINDOW"

Could you please add a short documentation for SCION_TESTING_ACCEPTANCE_WINDWO to the envvar section in the router, doc/manuals/router.rst.
The SCION_TESTING_EPOCH_DURATION is also missing in the router documentation -- please copy this over control.rst.

---

private/drkey/drkeyutil/provider.go line 70 at r2 (raw file):

	absTimeNext := spao.AbsoluteTimestamp(keys[2].Epoch, timestamp)
	switch {
	// case absTimeCurrent.After(awBegin) && absTimeCurrent.Before(awEnd):

Nit, remove commented line?

[JordiSubira]

Reviewable status: 17 of 25 files reviewed, 5 unresolved discussions (waiting on @matzf)

---

pkg/spao/timestamp_test.go line 36 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

In contrast to the other calls here, This time.Now() does not have the .UTC(). Is this still correct? Could we drop all of the others as well?

Done.

---

private/drkey/drkeyutil/drkey.go line 27 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

This one seems to be a bit out of place here, I'd leave this in control/config/drkey.go.

Done.

---

private/drkey/drkeyutil/drkey.go line 36 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

The description is incorrect now, as the length defines the full acceptance window size.
I'd also rename the DefaultAcceptanceWindowLength to just DefaultAcceptanceWindow to avoid having two different names.

• widown -> window
• acceptanceWindowOffset -> acceptance window

Done.

---

private/drkey/drkeyutil/drkey.go line 37 at r2 (raw file):

	// that we accept packets for active keys only.
	DefaultAcceptanceWindowLength = 5 * time.Minute
	EnvVarAccpetanceWindow        = "SCION_TESTING_ACCEPTANCE_WINDOW"

Done.

---

private/drkey/drkeyutil/drkey.go line 37 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Could you please add a short documentation for SCION_TESTING_ACCEPTANCE_WINDWO to the envvar section in the router, doc/manuals/router.rst.
The SCION_TESTING_EPOCH_DURATION is also missing in the router documentation -- please copy this over control.rst.

I've changed it in #4300

[matzf]

Reviewed 8 of 8 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @JordiSubira)

---

private/drkey/drkeyutil/drkey.go line 37 at r2 (raw file):

Previously, JordiSubira wrote…

I've changed it in #4300

Ok, sure that works. Generally, the idea would be to update the (usage-)documentation directly together with the implementation.

[matzf]

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

[matzf]

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