cppki: limit segment expiration by certificate lifetime

[uniquefine]

Update the beaconing.DefaultExtender to not create Segments with expiration date that is later than the current AS certificate expiration.

Instead the extender uses min(segmentExpiration,certificateExpiration) when creating an AS Hop.

Contributes to #4286

---

This change is 

[matzf]

Reviewed 1 of 13 files at r1.
Reviewable status: 1 of 13 files reviewed, 4 unresolved discussions (waiting on @oncilla and @uniquefine)

---

control/tasks.go line 59 at r1 (raw file):

	SegmentRegister       beaconing.RPC
	BeaconStore           Store
	Signer                seg.Signer

Remove this Signer? Looks like it's no longer used (Signer in Originator and Propagator is also unused).

---

control/beaconing/extender.go line 37 at r1 (raw file):

type SignerGen interface {
	// Generate generates a signer and returns its expiration time.
	Generate(ctx context.Context) (seg.Signer, time.Time, error)

Wouldn't it make sense to use private/trust.Signer in this file, instead of the seg.Signer interface? Then we can access the signer.Expiration directly and the existing control/trust.SignerGen interface suffices.

---

control/trust/signer.go line 38 at r1 (raw file):

// Sign signs the message with the latest available Signer.
func (s RenewingSigner) Sign(

This function no longer seems to be used. While the SignCMS one is still in use, I'm not sure this RenewingSigner abstraction is still worth keeping. I'd propose to remove this and pass the SignerGen around directly -- this would make the very few call sites for SignCMS just a few lines longer, and reduce the "magic".
We can also keep it for now and do look at this as a follow up.

---

pkg/slayers/path/hopfield.go line 152 at r1 (raw file):

// can be used in a HopField. The returned value is guaranteed to be >= 1.
// For durations that are out of range, an error is returned.
func ExpTimeFromSeconds(seconds float64) (uint8, error) {

The commit "avoid using duration.Seconds()" changes this function from taking a Duration parameter to a float64. In my opinion,Duration should be preferred.
If you want to avoid the duration.Seconds() call (which has just been moved out of this function now), keep the calculations in nanoseconds (i.e. multiply MaxTTL by time.Second, or even better, change MaxTTL to a duration constant).

The documentation is wrong: for values expTimeUnit to 2*expTimeUnit it will (correctly) return 0.
The "fits into" phrasing is not very obvious, perhaps explicitly say "less than or equal". Maybe even write it out in (pseudo-)code:

//   d <= ExpTimeToDuration(ExpTimeFromDuration(d))

[uniquefine]

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

---

control/tasks.go line 59 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Remove this Signer? Looks like it's no longer used (Signer in Originator and Propagator is also unused).

Good point, removed.

---

control/beaconing/extender.go line 37 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Wouldn't it make sense to use private/trust.Signer in this file, instead of the seg.Signer interface? Then we can access the signer.Expiration directly and the existing control/trust.SignerGen interface suffices.

True, I changed it to use trust.Signer directly. I didn't realize that we are using trust.Signer in all tests already.

---

control/trust/signer.go line 38 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

This function no longer seems to be used. While the SignCMS one is still in use, I'm not sure this RenewingSigner abstraction is still worth keeping. I'd propose to remove this and pass the SignerGen around directly -- this would make the very few call sites for SignCMS just a few lines longer, and reduce the "magic".
We can also keep it for now and do look at this as a follow up.

I agree that this doesn't add much. But as you mentioned, it's not really related to this PR. I'll do it in a followup PR instead.

---

pkg/slayers/path/hopfield.go line 152 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

The commit "avoid using duration.Seconds()" changes this function from taking a Duration parameter to a float64. In my opinion,Duration should be preferred.
If you want to avoid the duration.Seconds() call (which has just been moved out of this function now), keep the calculations in nanoseconds (i.e. multiply MaxTTL by time.Second, or even better, change MaxTTL to a duration constant).

The documentation is wrong: for values expTimeUnit to 2*expTimeUnit it will (correctly) return 0.
The "fits into" phrasing is not very obvious, perhaps explicitly say "less than or equal". Maybe even write it out in (pseudo-)code:

//   d <= ExpTimeToDuration(ExpTimeFromDuration(d))

I totally agree with what you said about the doc comment.

I now changed the MaxTTL to duration type. This makes some calculations more precise (hence the updated tests).
My issue with the duration is that the gobra check in CI fails. What can/should I do to resolve the failing check?

[uniquefine]

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

---

pkg/slayers/path/hopfield.go line 152 at r1 (raw file):

Previously, uniquefine (Sebastian Leisinger) wrote…

I totally agree with what you said about the doc comment.

I now changed the MaxTTL to duration type. This makes some calculations more precise (hence the updated tests).
My issue with the duration is that the gobra check in CI fails. What can/should I do to resolve the failing check?

Oh NVM, now it passed the check.

[matzf]

The PR description seems a bit inaccurate; this is changing the signer side only now, so the title should be something like "cppki: limit segment expiration by certificate lifetime"

This contributes to the linked issue, but the issue is not completed until the relying party is also updated to require this, in a second step.

Reviewed 2 of 13 files at r1, 7 of 23 files at r2, all commit messages.
Reviewable status: 9 of 25 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker, @oncilla, and @uniquefine)

---

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

	}
	hopF, epicMac := s.createHopF(ingress, egress, expTime, ts, beta)
	// make sure

// make sure what? :)

---

control/trust/signer.go line 61 at r2 (raw file):

func (s RenewingSigner) Generate(ctx context.Context) (trust.Signer, error) {
	return s.SignerGen.Generate(ctx)
}

This method seems redundant. Just use the SignerGen directly (pass SignerGen: signer.SignerGen instead of SignerGen: signer in control/cmd/control/main.go).

---

pkg/slayers/path/hopfield.go line 34 at r2 (raw file):

// MaxTTL is the maximum age of a HopField in seconds.
const MaxTTL = 24 * 60 * 60 * time.Second // One day in seconds

Nit: comments are no longer accurate.
Can use time.Hour to make the definition obvious enough that a comment is not required

Suggestion:

// MaxTTL is the maximum age of a HopField.
const MaxTTL = 24 * time.Hour

---

pkg/slayers/path/hopfield.go line 36 at r2 (raw file):

const MaxTTL = 24 * 60 * 60 * time.Second // One day in seconds

const expTimeUnit = MaxTTL / 256 // ~5m38s

Nit: // 5m38.5s

[uniquefine]

Reviewable status: 6 of 25 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker, @matzf, and @oncilla)

---

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

Previously, matzf (Matthias Frei) wrote…

// make sure what? :)

Done.

---

control/trust/signer.go line 61 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

This method seems redundant. Just use the SignerGen directly (pass SignerGen: signer.SignerGen instead of SignerGen: signer in control/cmd/control/main.go).

Done.

[matzf]

Reviewed 15 of 23 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker, @oncilla, and @uniquefine)

---

private/path/combinator/__debug_bin line 0 at r3 (raw file):
Accidentally committed?

[uniquefine]

Reviewable status: 24 of 25 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker, @matzf, and @oncilla)

---

private/path/combinator/__debug_bin line at r3 (raw file):

Previously, matzf (Matthias Frei) wrote…

Accidentally committed?

Yes, reverted

[matzf]

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

[matzf]

@lukedirtwalker, @oncilla, this PR is waiting for your review.

[oncilla]

Reviewed 2 of 13 files at r1, 18 of 23 files at r2, 4 of 4 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @uniquefine)

---

control/beaconing/extender.go line 101 at r4 (raw file):

	// Make sure the hop expiration time is not longer than the signer expiration time.
	expTime := s.MaxExpTime()
	if ts.Add(path.ExpTimeToDuration(expTime)).After(signer.Expiration) {

This currently silently reduces the expiration time until we go to zero.

Let's add a gauge that indicates that the configured maximum expiration time can no longer be supported.
Operators should be able to create an alert based on that guage.

---

pkg/slayers/path/hopfield.go line 160 at r4 (raw file):

		return 0, serrors.New("duration too large", "seconds", d)
	}
	return uint8(d*256/MaxTTL - 1), nil

add brackets to make it unambiguous even if the reader does not know the precedence order.

Code quote:

d*256/MaxTTL

[lukedirtwalker]

given both Matthias and Dominik already review(ed) it, I'll pass on doing a review.

[uniquefine]

Reviewable status: 19 of 26 files reviewed, 2 unresolved discussions (waiting on @matzf and @oncilla)

---

control/beaconing/extender.go line 101 at r4 (raw file):

Previously, oncilla (Dominik Roos) wrote…

This currently silently reduces the expiration time until we go to zero.

Let's add a gauge that indicates that the configured maximum expiration time can no longer be supported.
Operators should be able to create an alert based on that guage.

Done.

---

pkg/slayers/path/hopfield.go line 160 at r4 (raw file):

Previously, oncilla (Dominik Roos) wrote…

add brackets to make it unambiguous even if the reader does not know the precedence order.

Done.

[oncilla]

Reviewed 7 of 7 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @uniquefine)

---

control/observability.go line 249 at r5 (raw file):

			prometheus.GaugeOpts{
				Name: "control_segment_expiration_deficient",
				Help: "Indicates whether the expiration time of the segment is below the maximum." +

Suggestion:

the configured maximum." +

---

control/observability.go line 252 at r5 (raw file):

					" This happens when the signer expiration time is lower than the maximum segment expiration time.",
			},
			[]string{"src"},

This type of label is usually named "isd_as". Let's use that for consistency.

---

control/tasks.go line 219 at r5 (raw file):

		EPIC:       t.EPIC,
	}
	if t.Metrics != nil {

Personally, I prefer the following:

...
SegementExpirationDeficient: func() metrics.Gauge {
    if t.Metrics == nil {
        return nil
    }
    return metrics.GaugeWith(
			metrics.NewPromGauge(t.Metrics.SegmentExpirationDeficient),
			"isd_as", ia.String(),
		)

}(),

This way, initialization is not split across multiple lines just because of a conditional.

(non-blocking) because it is a matter of taste.

---

control/beaconing/extender.go line 101 at r4 (raw file):

Previously, uniquefine (Sebastian Leisinger) wrote…

Done.

You also need a to unset it once it is not deficient anymore.

---

pkg/slayers/path/hopfield.go line 160 at r4 (raw file):

Previously, uniquefine (Sebastian Leisinger) wrote…

Done.

The crucial part is the * and /. Please add the brackets there.
I think go formating already does a good job at indicating precedence between */ and - by adding the whitespace

uint8((d*256)/MaxTTL - 1)

[uniquefine]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @oncilla)

---

control/observability.go line 249 at r5 (raw file):

			prometheus.GaugeOpts{
				Name: "control_segment_expiration_deficient",
				Help: "Indicates whether the expiration time of the segment is below the maximum." +

Done.

---

control/observability.go line 252 at r5 (raw file):

Previously, oncilla (Dominik Roos) wrote…

This type of label is usually named "isd_as". Let's use that for consistency.

Done, This service doesn't use "isd_as" as label on any of the other metrics though.

---

control/tasks.go line 219 at r5 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Personally, I prefer the following:

...
SegementExpirationDeficient: func() metrics.Gauge {
    if t.Metrics == nil {
        return nil
    }
    return metrics.GaugeWith(
			metrics.NewPromGauge(t.Metrics.SegmentExpirationDeficient),
			"isd_as", ia.String(),
		)

}(),

This way, initialization is not split across multiple lines just because of a conditional.

(non-blocking) because it is a matter of taste.

done, agreed

---

control/beaconing/extender.go line 101 at r4 (raw file):

Previously, oncilla (Dominik Roos) wrote…

You also need a to unset it once it is not deficient anymore.

Done. This should be granular enough. If the AS Certificate is expiring the metric will be 1 for all segments. When it's renewed the metric will immediately drop to 0.

---

pkg/slayers/path/hopfield.go line 160 at r4 (raw file):

Previously, oncilla (Dominik Roos) wrote…

The crucial part is the * and /. Please add the brackets there.
I think go formating already does a good job at indicating precedence between */ and - by adding the whitespace

uint8((d*256)/MaxTTL - 1)

💡 I didn't think that it makes a difference

[oncilla]

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

---

control/observability.go line 252 at r5 (raw file):

Previously, uniquefine (Sebastian Leisinger) wrote…

Done, This service doesn't use "isd_as" as label on any of the other metrics though.

It does not, but it does use src with very different semantics 😉
But come to think of it, it does not add this label in other places because it is single ISD-AS anyway. I think we should drop it for consistency.

---

pkg/slayers/path/hopfield.go line 160 at r4 (raw file):

Previously, uniquefine (Sebastian Leisinger) wrote…

💡 I didn't think that it makes a difference

For the record, the formula was correct in the beginning already

But time.Duration is an integer behind the scenes. You need to be careful when dealing with divisions and time.Duration.
That's why it is extra helpful to the reader if they do not need to worry about precedence rules.

[uniquefine]

Reviewable status: 22 of 27 files reviewed, 2 unresolved discussions (waiting on @matzf and @oncilla)

---

control/observability.go line 252 at r5 (raw file):

Previously, oncilla (Dominik Roos) wrote…

It does not, but it does use src with very different semantics 😉
But come to think of it, it does not add this label in other places because it is single ISD-AS anyway. I think we should drop it for consistency.

Removed, Done.

---

pkg/slayers/path/hopfield.go line 160 at r4 (raw file):

Previously, oncilla (Dominik Roos) wrote…

For the record, the formula was correct in the beginning already

But time.Duration is an integer behind the scenes. You need to be careful when dealing with divisions and time.Duration.
That's why it is extra helpful to the reader if they do not need to worry about precedence rules.

Done.

[matzf]

Reviewed 2 of 7 files at r5, 2 of 5 files at r6, 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)

[oncilla]

Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @uniquefine)