-
Notifications
You must be signed in to change notification settings - Fork 932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Auth: Prune pending TLS identities #14261
base: main
Are you sure you want to change the base?
Auth: Prune pending TLS identities #14261
Conversation
6e803ae
to
c99214a
Compare
c99214a
to
70db42b
Compare
21cacd4
to
ece5b8d
Compare
ece5b8d
to
7bff5ff
Compare
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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 <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
7bff5ff
to
c790be2
Compare
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 <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Also change "id" to "operation" in the log context. Signed-off-by: Mark Laing <mark.laing@canonical.com>
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 <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
…ken is used. Signed-off-by: Mark Laing <mark.laing@canonical.com>
…identities. Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
c790be2
to
5e95847
Compare
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
andState
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.