Add "repeatable" field to AgentAttributes

[dfeldman]

This field will allow us to clear out old agent entries in order to resolve #1836 in SPIRE (additional changes in SPIRE itself required), and also tackle #2203.

Signed-off-by: Daniel Feldman dfeldman.mn@gmail.com

[rturner3]

Hmm, I wonder if we can't be a little more precise in the naming of this field. It's not too clear to me what it means to be for an Agent to be "repeatable".

What about can_auto_reattest or reattestable? To me, it makes sense to make the name of this field resemble the purpose it will be used for in SPIRE Server, which is primarily to make decisions based on whether or not an agent can present the same attestation material multiple times for node attestation.

[evan2645]

I agree that repeatable isn't very self documenting (but the code comment is good). In finding a name for this field, it may help to consider that it is part of an AgentAttributes message.

Here are a few other things rolling around in my mental word cloud

tofu_attestation
unsafe_attestation
attest_once

I think it is also useful to get the polarity correct. For example, is the normal condition TOFU or not TOFU ... or even the desired condition. TOFU attestation is much less secure than methods that can be safely re-attested, so IMO the polarity should be oriented in that way (i.e. TOFU is the exceptional case).

[dfeldman]

I am good with any of these names.
I feel the polarity should be that if the flag is true, it should NOT be TOFU. That way if any plugins forget to fill in this field or are using old versions of this proto, then it will revert to the existing, safe, TOFU behavior. That is why I picked the name "repeatable" :)

[rturner3]

I agree with @dfeldman 's reasoning about the default stance being TOFU. We have to consider non-builtin NodeAttestor plugins that will not be setting this field yet. It seems preferable to me to consider them TOFU until explicitly indicated otherwise by the developer of the plugin.

Also since this field will be used to determine whether a given NodeAttestor plugin's expired data can be safely purged from the database, it is probably better to assume that a plugin is not eligible for this behavior until explicitly opted in.

@evan2645 any preference on the name? I am leaning towards can_auto_reattest, since it matches the desired polarity.

[dfeldman]

Can we go ahead with can_auto_reattest?

[evan2645]

I feel the polarity should be that if the flag is true, it should NOT be TOFU. That way if any plugins forget to fill in this field or are using old versions of this proto, then it will revert to the existing, safe, TOFU behavior.
...

It seems preferable to me to consider them TOFU until explicitly indicated otherwise by the developer of the plugin.
...
Also since this field will be used to determine whether a given NodeAttestor plugin's expired data can be safely purged from the database, it is probably better to assume that a plugin is not eligible for this behavior until explicitly opted in.

All of that makes good sense!

Can we go ahead with can_auto_reattest?

Sure. Another option could be simply can_reattest since evicting and agent is considered more like a fresh attestation than a re-attestation, but either is OK with me

Thank you @dfeldman!

[rturner3]

Suggested change

	// Optional. If repeatable is true, then this attestation may happen more
	// than one time for a given agent. This allows the server to clear out
	// old entries automatically (since they can be easily recreated).
	// Optional. If can_auto_reattest is true, then this attestation method
	// allows an agent to attest multiple times with the same
	// attestation payload without operator intervention.
	// This also allows the server to clear out old entries automatically
	// since they can be easily recreated.

[rturner3]

Thanks, @dfeldman !