Add an audit event for creating provisioning tokens

[bl-nero]

This change creates a new event, keeping the old, trusted-cluster-specific one. We should remove the old one after notifying the users and some grace period (I'll file a separate issue for this.)

Related issue: #16359.

Not addressed, will address later: deleting provisioning tokens.

[zmb3]

Looks good, though

We should remove the old one after notifying the users and some grace period (I'll file a separate issue for this.)

We basically never want to remove audit events, because they can still exist in customers' audit logs, and we want to be able to interpret them even if we no longer emit them.

[bl-nero]

Note to self: looks like I forgot about the other place that emits audit events (lib/auth/auth.go, function GenerateToken). Any suggestion for when exactly this method is called? (It's currently deprecated.)

[zmb3]

Note to self: looks like I forgot about the other place that emits audit events (lib/auth/auth.go, function GenerateToken). Any suggestion for when exactly this method is called? (It's currently deprecated.)

Probably comes from the tctl tokens add and tctl nodes add commands.

[bl-nero]

We basically never want to remove audit events, because they can still exist in customers' audit logs, and we want to be able to interpret them even if we no longer emit them.

That's a valid point; how about filing an issue to no longer emit them, even if we leave the code that is interpreting these?

[bl-nero]

Note to self: looks like I forgot about the other place that emits audit events (lib/auth/auth.go, function GenerateToken). Any suggestion for when exactly this method is called? (It's currently deprecated.)

Probably comes from the tctl tokens add and tctl nodes add commands.

Nope, I confirmed: both of these go through the non-deprecated flow.

[zmb3]

We basically never want to remove audit events, because they can still exist in customers' audit logs, and we want to be able to interpret them even if we no longer emit them.

That's a valid point; how about filing an issue to no longer emit them, even if we leave the code that is interpreting these?

I would just stop emitting them now. If this work closes one old issue that we haven't found the time to get to but opens another, we're back to hoping that some day we will be able to get back to this.

In fact, the best approach is probably:

• merge this one as-is (emitting both the new and the legacy trusted cluster event)
• backport this to all supported branches
• merge a separate PR that eliminates the emitting of the legacy event (do not backport this, one, so the old event only stops being emitted in the newest major version)

[bl-nero]

@strideynet Ping, as you requested :)

[strideynet]

Looks like this is nearly there, just had a few thoughts come to mind.

[ibeckermayer]

In fact, the best approach is probably:

• merge this one as-is (emitting both the new and the legacy trusted cluster event)
• backport this to all supported branches
• merge a separate PR that eliminates the emitting of the legacy event (do not backport this, one, so the old event only stops being emitted in the newest major version)

I'm not sure about this, is it not unnecessarily complex to emit two audit events for the same action? What if we just remove the emission of the legacy event here, call it a breaking api change and ship it with the next major release, and not worry about any backports?

@zmb3

[zmb3]

call it a breaking api change and ship it with the next major release, and not worry about any backports?

That's a fine option too. The only thing I'd like to avoid is having a patch release that suddenly stops emitting an event that the previous version did emit.

[ibeckermayer]

Are there any docs that should be changed to reflect this new event?

[bl-nero]

Are there any docs that should be changed to reflect this new event?

Good call, there is a reference page; although it's explicitly said it's not exhaustive, I'll add the join_token.create event there.

[strideynet]

Just a few minor changes - this looks great now.

[bl-nero]

Now that's embarassing. Looks like my "non-brittle" strategy was actually hiding a bug in my code, the Jurassic Park way. After moving the event emitting code to auth_with_roles.go, I forgot to remove it from auth.go, which resulted in the legacy event being emitted twice.

[public-teleport-github-review-bot]

@bl-nero See the table below for backport results.

Branch	Result
branch/v11	Failed
branch/v12	Failed
branch/v13	Create PR

[bl-nero]

@zmb3 @strideynet Unless you protest, I will not to port the change further than v13, since this is when CreateTokenV2 was first introduced, and the solution for previous branches would differ too much. I don't think it's worth the effort.

[strideynet]

@zmb3 @strideynet Unless you protest, I will not to port the change further than v13, since this is when CreateTokenV2 was first introduced, and the solution for previous branches would differ too much. I don't think it's worth the effort.

This seems acceptable to me :)