Auth: Prune pending TLS identities #14261

markylaing · 2024-10-11T10:55:04Z

As part of the TLS fine-grained authorization specification, pending TLS identities must be pruned when their associated token expires. This pull request adds logic to the autoRemoveExpiredTokens task such that the leader will retrieve all the pending TLS identities and delete those that have expired. Non-leaders will continue to cancel "certificate add" and "cluster join" operations in-memory in each member.

There was a lot of duplication of logic to identify the database leader. This PR adds a method to Daemon and State to determine if the member is a leader and refactors existing code to use that method. If the Daemon is standalone it is considered to be the leader. It is the responsibility of the caller to check (*State).ServerClustered before calling (*State).LeaderInfo when necessary.

Marked as draft until #14207 is merged.

lxd/identities.go

lxd/daemon.go

lxd/instance_post.go

lxd/tokens.go

test/suites/auth.sh

lxd/daemon.go

lxd/patches.go

lxd/tokens.go

lxd/daemon.go

markylaing · 2024-10-23T14:49:01Z

@tomponline @hamistao third time lucky, it has passed! Unfortunately this is probably bad news as it means we have a racy test that fails intermittently in CI. I'll keep looking but I can't see how my PR is affecting this.

markylaing · 2024-10-24T06:31:04Z

@tomponline @hamistao third time lucky, it has passed! Unfortunately this is probably bad news as it means we have a racy test that fails intermittently in CI. I'll keep looking but I can't see how my PR is affecting this.

I just realised that I posted this comment on the wrong PR. It's meant for #14324

lxd/api_cluster.go

test/suites/auth.sh

markylaing · 2024-10-24T09:16:46Z

Please can you review the use of s.ServerClustered and in functions where there is also a call to s.LeaderInfo check leaderInfo.Clustered instead, that way we are keeping checks for cluster state to the leaderInfo struct. Perhaps ultimately we may unexport s.Clustered from State....

I have kept these checks because checking s.Clustered is quick, whereas s.LeaderInfo may call d.gateway.LeaderAddress which may be slow. I think it's fine to keep s.Clustered because sometimes we just need to know if we're clustered, and not necessary get the leader information.

tomponline · 2024-10-24T09:18:36Z

Please can you review the use of s.ServerClustered and in functions where there is also a call to s.LeaderInfo check leaderInfo.Clustered instead, that way we are keeping checks for cluster state to the leaderInfo struct. Perhaps ultimately we may unexport s.Clustered from State....

I have kept these checks because checking s.Clustered is quick, whereas s.LeaderInfo may call d.gateway.LeaderAddress which may be slow. I think it's fine to keep s.Clustered because sometimes we just need to know if we're clustered, and not necessary get the leader information.

Yeah but wont we do an s.LeaderInfo call if we are clustered in those situations, and s.LeaderInfo is cheap because it tests s.ServerClustered internally first?

If we werent also calling s.LeaderInfo in the same function i would agree testing s.ServerClustered directly is fine.

Signed-off-by: Mark Laing <[email protected]>

markylaing · 2024-10-24T15:53:18Z

I think I've addressed everything. Let me know if you have any more thoughts on #14261 (comment) but otherwise ready for another round.

Signed-off-by: Mark Laing <[email protected]>

Also change "id" to "operation" in the log context. Signed-off-by: Mark Laing <[email protected]>

On `POST /1.0/certificates` with a trust_token, the CertificateAddToken operation associated with the token is cancelled regardless of where it is invalid/expired. This commit performs the analogous task for pending TLS identities and removes them if invalid when the token is received. Signed-off-by: Mark Laing <[email protected]>

lxd/tokens.go

tomponline

LGTM, just that Debug log line query?

hamistao · 2024-10-31T14:20:34Z

lxd/api_cluster.go

+	}
+
+	if !leaderInfo.Clustered {
+		return response.InternalError(cluster.ErrNodeIsNotClustered)


Would this be better classified as a BadRequest?
This does impact the metrics so just making sure we are getting this right.

I thought that but I didn't want to change the existing HTTP code as that's technically an API change

Signed-off-by: Mark Laing <[email protected]>

…ken is used. Signed-off-by: Mark Laing <[email protected]>

…identities. Signed-off-by: Mark Laing <[email protected]>

Signed-off-by: Mark Laing <[email protected]>

hamistao

LGTM!

github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Oct 11, 2024

markylaing marked this pull request as draft October 11, 2024 10:55

markylaing self-assigned this Oct 11, 2024

markylaing force-pushed the prune-pending-tls-identities branch 2 times, most recently from 6e803ae to c99214a Compare October 15, 2024 16:04

github-actions bot removed Documentation Documentation needs updating API Changes to the REST API labels Oct 15, 2024

markylaing marked this pull request as ready for review October 15, 2024 16:04

markylaing requested review from tomponline and hamistao October 15, 2024 16:04

markylaing commented Oct 15, 2024

View reviewed changes

lxd/identities.go Outdated Show resolved Hide resolved

markylaing force-pushed the prune-pending-tls-identities branch from c99214a to 70db42b Compare October 15, 2024 16:36