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.

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

CredHub accumulates orphaned data in the the encrypted_value table #231

Closed
3 tasks done
dueckminor opened this issue Jan 24, 2022 · 11 comments · Fixed by #728
Closed
3 tasks done

CredHub accumulates orphaned data in the the encrypted_value table #231

dueckminor opened this issue Jan 24, 2022 · 11 comments · Fixed by #728

Comments

@dueckminor
Copy link

dueckminor commented Jan 24, 2022

What version of the credhub server you are using?
2.11.1

What version of the credhub cli you are using?
2.9.0

If you were attempting to accomplish a task, what was it you were attempting to do?
We rotate credentials regularly, and have seen that the size of the CredHub DB is growing constantly. The DB size is growing even if we rotate a credential by deleting and setting it (this deletes old credential versions).

What did you expect to happen?
I would expect, that the credhub database doesn't grow so fast. We had databases using 15 GB storage. After I have manually deleted the orphaned entries the database size was only 40 MB.

What was the actual behaviour?
Values it the encrypted_value table are worthless if there are no entries in the credential_version, password_credential or user_credential table, because nobody can know to which credential the encrypted value belongs to.

If I generate a password credential like this:

credhub generate --name=/foo --type=password

I get two additional entries in the encrypted_value table. One for the value and one for the parameters. But If I delete the credential, no values will be deleted from the encrypted_value table.

To get rid of these entries, you may use the following SQL query (use at your own risk!):

DELETE FROM encrypted_value
WHERE
    NOT EXISTS (select 1 from credential_version where encrypted_value_uuid=encrypted_value.uuid)
AND
    NOT EXISTS (select 1 from password_credential where password_parameters_uuid=encrypted_value.uuid)
AND
    NOT EXISTS (select 1 from user_credential where password_parameters_uuid=encrypted_value.uuid)

But if the database has already a huge size, this will take some time (It took about a week to delete all orphaned entries)
By adding some indexes you could speed it up dramatically:

CREATE INDEX CONCURRENTLY IF NOT EXISTS credential_version_encrypted_value_uuid_idx ON credential_version USING btree (encrypted_value_uuid);
CREATE INDEX CONCURRENTLY IF NOT EXISTS password_credential_password_parameters_uuid_idx ON password_credential USING btree (password_parameters_uuid);
CREATE INDEX CONCURRENTLY IF NOT EXISTS user_credential_password_parameters_uuid_idx ON user_credential USING btree (password_parameters_uuid);

But that is just to get rid of existing entries. I would expect that the encrypted values get deleted together with the corresponding entries in the other tables.

Please confirm where necessary:

  • I have included a log output (N/A)
  • My log includes an error message (N/A)
  • I have included steps for reproduction

If you are a PCF customer with an Operation Manager (PCF Ops Manager) please direct your questions to support (https://support.pivotal.io/)

@hsinn0
Copy link
Contributor

hsinn0 commented May 23, 2022

@swalchemist
Copy link
Contributor

@dueckminor Thanks, we have this on our roadmap.

@swalchemist
Copy link
Contributor

#190 seems similar.

@bruce-ricard
Copy link
Contributor

I would like to give @dueckminor the medal🥇of the best Github issue I have ever seen.

Very well documented, explained, with reproduction steps, and even a possible cleanup script.

Sorry it took us so long to look at it.

@hsinn0
Copy link
Contributor

hsinn0 commented Mar 20, 2024

PR #701 is obsolete. New fix is in PR #711.

@hsinn0
Copy link
Contributor

hsinn0 commented Mar 22, 2024

Fixed in CredHub 2.12.67. Thank you.

@hsinn0 hsinn0 reopened this Apr 10, 2024
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@hsinn0
Copy link
Contributor

hsinn0 commented Apr 10, 2024

Reopening this issue as we are undoing the fix that included database trigger creation in next release, because of the issue with Amazon RDS for MySQL where the trigger creation privilege is not part of default configuration and requires operator intervention and db restart. We will try to come up with resolution that does not involve database triggers.

hsinn0 added a commit that referenced this issue Apr 12, 2024
…y deletion

- Re-implemented the fix for #231 with JPA cascade-remove instead of database triggers.
- Modified the transaction propagation behavior of the unit test methods that verify `encrypted_value record` count, so the count can be obtained after the delete transactions is done.
- Fixed typos in test code.

[#187367323]
hsinn0 added a commit that referenced this issue Apr 15, 2024
…y deletion

- Re-implemented the fix for #231 with JPA cascade-remove instead of database triggers.
- Modified the transaction propagation behavior of the unit test methods that verify `encrypted_value record` count, so the count can be obtained after the delete transactions is done.
- Fixed typos in test code.

[#187367323]
hsinn0 added a commit that referenced this issue Apr 17, 2024
- Fixed #231 in JPA layer instead of using database triggers.
- Used `LAZY` fetch for the JPA one-to-many mapping from `credential` to `crdential_version` because `EAGER` causes memory and perfromance issue when many credentials are loaded, e.g. `DefaultCertificatesHandler.handleGetAllRequest()`. The issue with `EAGER` fetch was reproducible in `DefaultCertificatesHandlerIntegrationTest`.
- Made `DefaultCertificatesHandlerIntegrationTest` transactional so as not to commit test data. This not only makes sure no dirty test data is left but also makes the test runs a little faster.
- Changed the `credetial_version.type` of test data in `DefaultCertificatesHandlerIntegrationTest` to a valid value 'cert' as previous invalid type value caused data vdalidation failure by Hibernate.
- Changed properties of `CredentialVersionData` entity to be non-final, i.e. added Kotlin `open` keyword to them, so the lazy loading can work properly. This takes care of following exception for example:
```
WARN org.hibernate.tuple.entity.PojoEntityTuplizer - HHH000305: Could not create proxy factory for:org.cloudfoundry.credhub.entity.CredentialVersionData
  org.hibernate.HibernateException: Getter methods of lazy classes cannot be final: org.cloudfoundry.credhub.entity.CredentialVersionData#getUuid
```

[#187367323]
bruce-ricard pushed a commit that referenced this issue Apr 17, 2024
- Fixed #231 in JPA layer instead of using database triggers.
- Used `LAZY` fetch for the JPA one-to-many mapping from `credential` to `crdential_version` because `EAGER` causes memory and perfromance issue when many credentials are loaded, e.g. `DefaultCertificatesHandler.handleGetAllRequest()`. The issue with `EAGER` fetch was reproducible in `DefaultCertificatesHandlerIntegrationTest`.
- Made `DefaultCertificatesHandlerIntegrationTest` transactional so as not to commit test data. This not only makes sure no dirty test data is left but also makes the test runs a little faster.
- Changed the `credetial_version.type` of test data in `DefaultCertificatesHandlerIntegrationTest` to a valid value 'cert' as previous invalid type value caused data vdalidation failure by Hibernate.
- Changed properties of `CredentialVersionData` entity to be non-final, i.e. added Kotlin `open` keyword to them, so the lazy loading can work properly. This takes care of following exception for example:
```
WARN org.hibernate.tuple.entity.PojoEntityTuplizer - HHH000305: Could not create proxy factory for:org.cloudfoundry.credhub.entity.CredentialVersionData
  org.hibernate.HibernateException: Getter methods of lazy classes cannot be final: org.cloudfoundry.credhub.entity.CredentialVersionData#getUuid
```

[#187367323]
@hsinn0 hsinn0 reopened this Apr 17, 2024
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@hsinn0
Copy link
Contributor

hsinn0 commented Apr 17, 2024

Re-opening as we still have to release the fix.

@peterhaochen47
Copy link
Member

Closing as fix is released: https://github.com/pivotal/credhub-release/releases/tag/2.12.70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

6 participants