Skip to content
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.

Sign up for GitHub

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

Jump to bottom

KCM: Remove the oldest expired credential if no more space. #6917

Closed
wants to merge 2 commits into from
Closed

KCM: Remove the oldest expired credential if no more space. #6917

wants to merge 2 commits into from

Conversation

aplopez
Copy link
Contributor

@aplopez aplopez commented Sep 7, 2023

When adding a new credential to KCM and the user has already reached their limit, the oldest expired credential will be removed to free some space.
If no expired credential is found to be removed, the operation will fail as it happened in the previous versions.

Resolves: #6667

@aplopez aplopez marked this pull request as ready for review September 8, 2023 11:11
thalman
thalman reviewed Sep 20, 2023
View reviewed changes
src/responder/kcm/secrets/secrets.c Show resolved Hide resolved
src/responder/kcm/secrets/secrets.c Outdated Show resolved Hide resolved
src/responder/kcm/secrets/secrets.c Outdated Show resolved Hide resolved
@aplopez aplopez force-pushed the kcm-auto-delete branch 3 times, most recently from 2af0775 to f72af59 Compare October 3, 2023 15:01
@aplopez
Copy link
Contributor Author

aplopez commented Oct 3, 2023

Replaced a wrong return by a goto done.

thalman
thalman approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

sumit-bose
sumit-bose requested changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thanks for the patch, it is working well in my tests. Please see my in-line comments.

As a general comment about talloc_new(). In both cases you are only allocating a single memory area. Imo it would be sufficient to do this allocation directly on NULL and avoid the overhead of using talloc_new().

bye,
Sumit

src/responder/kcm/secrets/secrets.c Outdated Show resolved Hide resolved
src/responder/kcm/secrets/secrets.c Outdated Show resolved Hide resolved
@aplopez
KCM: Remove the oldest expired credential if no more space.
66aaa61 
:feature: When adding a new credential to KCM and the user has
          already reached their limit, the oldest expired credential
          will be removed to free some space.
          If no expired credential is found to be removed, the operation
          will fail as it happened in the previous versions.

Resolves: SSSD#6667
@aplopez
Copy link
Contributor Author

aplopez commented Oct 9, 2023

As a general comment about talloc_new(). In both cases you are only allocating a single memory area. Imo it would be sufficient to do this allocation directly on NULL and avoid the overhead of using talloc_new().

As discussed off-line, I did these changes for local_db_remove_oldest_expired_secret() but not for get_secret_expiration_time() which is allocating several structures.

@aplopez
KCM: Display in the log the limit as set by the user
d374a11 
max_uid_ccaches is unconditionally incremented by 2 in ccdb_secdb_init()
to create space for some internal entries. We cannot just show this
value as it is not what the user configured.
sumit-bose
sumit-bose approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thanks for the updates, ACK.

bye,
Sumit

@pbrezina pbrezina added the Ready to push Ready to push label Oct 11, 2023
@pbrezina
Copy link
Member

Pushed PR: #6917

  • master
    • 96d8b77 - KCM: Display in the log the limit as set by the user
    • 93ee015 - KCM: Remove the oldest expired credential if no more space.
  • sssd-2-9
    • 834b536 - KCM: Display in the log the limit as set by the user
    • 1fa7210 - KCM: Remove the oldest expired credential if no more space.

@pbrezina pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Oct 11, 2023
@pbrezina pbrezina closed this Oct 11, 2023
@pbrezina pbrezina mentioned this pull request Oct 11, 2023
Open
@aplopez aplopez deleted the kcm-auto-delete branch November 28, 2023 14:19
@alexey-tikhonov alexey-tikhonov mentioned this pull request Dec 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thalman thalman thalman approved these changes

@sumit-bose sumit-bose sumit-bose approved these changes

Assignees

@thalman thalman

@sumit-bose sumit-bose

Labels
Pushed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

KCM: provide mechanism to purge expired credentials
5 participants
@aplopez @pbrezina @thalman @sumit-bose @alexey-tikhonov