From 0a6dc7193492819535a87410300c91985055ad51 Mon Sep 17 00:00:00 2001 From: Hongchol Sinn Date: Tue, 16 Apr 2024 17:34:21 -0700 Subject: [PATCH] fix: Orphaned encrypted_value when Credentials are deleted - Fixed https://github.com/cloudfoundry/credhub/issues/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] --- ...ultCertificatesHandlerIntegrationTest.java | 4 +- .../CertificateVersionDeleteTest.java | 16 ++++- .../cloudfoundry/credhub/entity/Credential.kt | 10 ++++ .../credhub/entity/CredentialVersionData.kt | 10 ++-- ...faultCredentialVersionDataServiceTest.java | 58 +++++++++++++++++++ 5 files changed, 90 insertions(+), 8 deletions(-) diff --git a/backends/credhub/src/test/java/org/cloudfoundry/credhub/handlers/DefaultCertificatesHandlerIntegrationTest.java b/backends/credhub/src/test/java/org/cloudfoundry/credhub/handlers/DefaultCertificatesHandlerIntegrationTest.java index b90eef543..ad16da806 100644 --- a/backends/credhub/src/test/java/org/cloudfoundry/credhub/handlers/DefaultCertificatesHandlerIntegrationTest.java +++ b/backends/credhub/src/test/java/org/cloudfoundry/credhub/handlers/DefaultCertificatesHandlerIntegrationTest.java @@ -11,6 +11,7 @@ import org.springframework.jdbc.support.MetaDataAccessException; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.junit4.SpringRunner; +import org.springframework.transaction.annotation.Transactional; import org.cloudfoundry.credhub.CredhubTestApp; import org.cloudfoundry.credhub.certificates.DefaultCertificatesHandler; @@ -28,6 +29,7 @@ @RunWith(SpringRunner.class) @ActiveProfiles(value = "unit-test", resolver = DatabaseProfileResolver.class) @SpringBootTest(classes = CredhubTestApp.class) +@Transactional public class DefaultCertificatesHandlerIntegrationTest { @Autowired private DefaultCertificatesHandler defaultCertificatesHandler; @@ -77,7 +79,7 @@ private void insertTestCredentialsIntoPostgres(int count) { jdbcTemplate.update( "INSERT INTO credential_version (type, uuid, version_created_at, credential_uuid) " + - "SELECT 'foo', uuid, 0, uuid from credential"); + "SELECT 'cert', uuid, 0, uuid from credential"); jdbcTemplate.update( "INSERT INTO certificate_credential (uuid, transitional) " + diff --git a/backends/credhub/src/test/java/org/cloudfoundry/credhub/integration/CertificateVersionDeleteTest.java b/backends/credhub/src/test/java/org/cloudfoundry/credhub/integration/CertificateVersionDeleteTest.java index e26fa3d39..11a95a17a 100644 --- a/backends/credhub/src/test/java/org/cloudfoundry/credhub/integration/CertificateVersionDeleteTest.java +++ b/backends/credhub/src/test/java/org/cloudfoundry/credhub/integration/CertificateVersionDeleteTest.java @@ -1,5 +1,7 @@ package org.cloudfoundry.credhub.integration; +import java.util.UUID; + import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.test.context.ActiveProfiles; @@ -13,6 +15,7 @@ import com.jayway.jsonpath.JsonPath; import org.cloudfoundry.credhub.CredhubTestApp; +import org.cloudfoundry.credhub.data.EncryptedValueDataService; import org.cloudfoundry.credhub.helpers.RequestHelper; import org.cloudfoundry.credhub.utils.BouncyCastleFipsConfigurer; import org.cloudfoundry.credhub.utils.DatabaseProfileResolver; @@ -51,6 +54,9 @@ public class CertificateVersionDeleteTest { @Autowired private WebApplicationContext webApplicationContext; + @Autowired + EncryptedValueDataService encryptedValueDataService; + private MockMvc mockMvc; @Rule @@ -71,6 +77,9 @@ public void beforeEach() throws Exception { @Test public void deleteCertificateVersion_whenThereAreOtherVersionsOfTheCertificate_deletesTheSpecifiedVersion() throws Exception { + UUID aUuid = UUID.randomUUID(); + var nEncryptedValuesPre = encryptedValueDataService.countAllByCanaryUuid(aUuid); + final String credentialName = "/test-certificate"; String response = generateCertificateCredential(mockMvc, credentialName, true, "test", null, ALL_PERMISSIONS_TOKEN); @@ -81,13 +90,14 @@ public void deleteCertificateVersion_whenThereAreOtherVersionsOfTheCertificate_d .read("$.certificates[0].id"); final String version = RequestHelper.regenerateCertificate(mockMvc, uuid, false, ALL_PERMISSIONS_TOKEN); + assertThat("One associated encrypted value exist for each certificate vesion", + encryptedValueDataService.countAllByCanaryUuid(aUuid), equalTo(nEncryptedValuesPre + 2)); + final String versionUuid = JsonPath.parse(version).read("$.id"); final String versionValue = JsonPath.parse(version).read("$.value.certificate"); - final MockHttpServletRequestBuilder request = delete("/api/v1/certificates/" + uuid + "/versions/" + versionUuid) .header("Authorization", "Bearer " + ALL_PERMISSIONS_TOKEN) .accept(APPLICATION_JSON); - response = mockMvc.perform(request) .andExpect(status().isOk()) .andReturn().getResponse().getContentAsString(); @@ -103,6 +113,8 @@ public void deleteCertificateVersion_whenThereAreOtherVersionsOfTheCertificate_d final JSONArray jsonArray = new JSONArray(response); assertThat(jsonArray.length(), equalTo(1)); assertThat(JsonPath.parse(jsonArray.get(0).toString()).read("$.value.certificate"), equalTo(nonDeletedVersion)); + assertThat("Associated encrypted value is deleted when the certificate version is deleted", + encryptedValueDataService.countAllByCanaryUuid(aUuid), equalTo(nEncryptedValuesPre + 1)); } @Test diff --git a/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/entity/Credential.kt b/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/entity/Credential.kt index e5841e3af..c0d8c1627 100644 --- a/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/entity/Credential.kt +++ b/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/entity/Credential.kt @@ -5,10 +5,13 @@ import org.cloudfoundry.credhub.audit.AuditableCredential import org.cloudfoundry.credhub.constants.UuidConstants.Companion.UUID_BYTES import org.hibernate.annotations.GenericGenerator import java.util.UUID +import javax.persistence.CascadeType import javax.persistence.Column import javax.persistence.Entity +import javax.persistence.FetchType import javax.persistence.GeneratedValue import javax.persistence.Id +import javax.persistence.OneToMany import javax.persistence.Table @Entity @@ -21,6 +24,13 @@ class Credential : AuditableCredential { @GenericGenerator(name = "uuid2", strategy = "uuid2") override var uuid: UUID? = null + @OneToMany( + cascade = [CascadeType.REMOVE], + mappedBy = "credential", fetch = FetchType.LAZY + ) + var credentialVersions: MutableList> = + mutableListOf() + @Column(nullable = false) override var name: String? = null set(name) { diff --git a/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/entity/CredentialVersionData.kt b/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/entity/CredentialVersionData.kt index 2f6f2fe71..bd67024ed 100644 --- a/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/entity/CredentialVersionData.kt +++ b/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/entity/CredentialVersionData.kt @@ -43,21 +43,21 @@ abstract class CredentialVersionData>(credential: C @Column(length = UuidConstants.UUID_BYTES, columnDefinition = "VARBINARY") @GeneratedValue(generator = "uuid2") @GenericGenerator(name = "uuid2", strategy = "uuid2") - var uuid: UUID? = null + open var uuid: UUID? = null @OneToOne(cascade = [CascadeType.ALL]) @NotFound(action = NotFoundAction.IGNORE) @JoinColumn(name = "encrypted_value_uuid") - var encryptedCredentialValue: EncryptedValue? = null + open var encryptedCredentialValue: EncryptedValue? = null @Convert(converter = InstantMillisecondsConverter::class) @Column(nullable = false, columnDefinition = "BIGINT NOT NULL") @CreatedDate - lateinit var versionCreatedAt: Instant + open lateinit var versionCreatedAt: Instant @ManyToOne @JoinColumn(name = "credential_uuid", nullable = false) - var credential: Credential? = credential + open var credential: Credential? = credential // this is mapped with updatable and insertable false since it's managed by the DiscriminatorColumn annotation // surfacing property here lets us use it in JPA queries @@ -66,7 +66,7 @@ abstract class CredentialVersionData>(credential: C @Convert(converter = JsonNodeConverter::class) @Column(name = "metadata") - var metadata: JsonNode? = null + open var metadata: JsonNode? = null val nonce: ByteArray? get() = if (encryptedCredentialValue != null) this.encryptedCredentialValue!!.nonce else null diff --git a/components/credentials/src/test/java/org/cloudfoundry/credhub/services/DefaultCredentialVersionDataServiceTest.java b/components/credentials/src/test/java/org/cloudfoundry/credhub/services/DefaultCredentialVersionDataServiceTest.java index 07ab8b466..b08888ac9 100644 --- a/components/credentials/src/test/java/org/cloudfoundry/credhub/services/DefaultCredentialVersionDataServiceTest.java +++ b/components/credentials/src/test/java/org/cloudfoundry/credhub/services/DefaultCredentialVersionDataServiceTest.java @@ -7,10 +7,12 @@ import java.util.function.Consumer; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.mock.mockito.MockBean; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.junit4.SpringRunner; +import org.springframework.transaction.annotation.Propagation; import org.springframework.transaction.annotation.Transactional; import com.fasterxml.jackson.databind.JsonNode; @@ -31,11 +33,13 @@ import org.cloudfoundry.credhub.entity.CredentialVersionData; import org.cloudfoundry.credhub.entity.PasswordCredentialVersionData; import org.cloudfoundry.credhub.entity.SshCredentialVersionData; +import org.cloudfoundry.credhub.entity.UserCredentialVersionData; import org.cloudfoundry.credhub.entity.ValueCredentialVersionData; import org.cloudfoundry.credhub.exceptions.MaximumSizeException; import org.cloudfoundry.credhub.exceptions.ParameterizedValidationException; import org.cloudfoundry.credhub.repositories.CredentialRepository; import org.cloudfoundry.credhub.repositories.CredentialVersionRepository; +import org.cloudfoundry.credhub.repositories.EncryptedValueRepository; import org.cloudfoundry.credhub.util.CurrentTimeProvider; import org.cloudfoundry.credhub.utils.DatabaseProfileResolver; import org.cloudfoundry.credhub.utils.DatabaseUtilities; @@ -60,6 +64,7 @@ import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.not; import static org.hamcrest.core.IsCollectionContaining.hasItem; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; @@ -70,12 +75,18 @@ @Transactional public class DefaultCredentialVersionDataServiceTest { + @Value("${spring.profiles.active}") + private String activeSpringProfile; + @Autowired private CredentialVersionRepository credentialVersionRepository; @Autowired private CredentialRepository credentialRepository; + @Autowired + private EncryptedValueRepository encryptedValueRepository; + @Autowired private EncryptionKeyCanaryDataService encryptionKeyCanaryDataService; @@ -271,7 +282,9 @@ public void delete_onAnExistingCredential_returnsTrue() { } @Test + @Transactional(propagation = Propagation.NEVER) public void delete_onACredentialName_deletesAllCredentialsWithTheName() { + long nEncryptedValuesPre = encryptedValueRepository.count(); final Credential credential = credentialDataService .save(new Credential("/my-credential")); @@ -301,10 +314,14 @@ public void delete_onACredentialName_deletesAllCredentialsWithTheName() { assertThat(subject.findAllByName("/my-credential"), hasSize(0)); assertNull(credentialDataService.find("/my-credential")); + assertEquals("Associated encryptedValues are deleted when password credential is deleted", + nEncryptedValuesPre, encryptedValueRepository.count()); } @Test + @Transactional(propagation = Propagation.NEVER) public void delete_givenACredentialNameCasedDifferentlyFromTheActual_shouldBeCaseInsensitive() { + long nEncryptedValuesPre = encryptedValueRepository.count(); final Credential credentialName = credentialDataService .save(new Credential("/my-credential")); @@ -334,6 +351,47 @@ public void delete_givenACredentialNameCasedDifferentlyFromTheActual_shouldBeCas subject.delete("/MY-CREDENTIAL"); assertThat(subject.findAllByName("/my-credential"), empty()); + assertEquals("Associated encryptedValues are deleted when password credential is deleted", + nEncryptedValuesPre, encryptedValueRepository.count()); + } + + @Test + @Transactional(propagation = Propagation.NEVER) + public void delete_UserTypeCredential() { + long nEncryptedValuesPre = encryptedValueRepository.count(); + final Credential credentialName = credentialDataService.save( + new Credential("/my-credential")); + + final EncryptedValue encryptedValueA = new EncryptedValue(); + encryptedValueA.setEncryptionKeyUuid(activeCanaryUuid); + encryptedValueA.setEncryptedValue("credential-password".getBytes(UTF_8)); + encryptedValueA.setNonce(new byte[]{}); + + final UserCredentialVersionData credentialDataA = + new UserCredentialVersionData("test-user"); + credentialDataA.setCredential(credentialName); + credentialDataA.setEncryptedValueData(encryptedValueA); + credentialDataA.setSalt("salt"); + subject.save(credentialDataA); + + final EncryptedValue encryptedValueB = new EncryptedValue(); + encryptedValueB.setEncryptionKeyUuid(activeCanaryUuid); + encryptedValueB.setEncryptedValue("another password".getBytes(UTF_8)); + encryptedValueB.setNonce(new byte[]{}); + + final UserCredentialVersionData credentialDataB = new UserCredentialVersionData( + "/my-credential"); + credentialDataB.setCredential(credentialName); + credentialDataB.setEncryptedValueData(encryptedValueB); + credentialDataB.setSalt("salt"); + subject.save(credentialDataB); + + assertThat(subject.findAllByName("/my-credential"), hasSize(2)); + + subject.delete("/my-credential"); + assertThat(subject.findAllByName("/my-credential"), empty()); + assertEquals("Associated encryptedValues are deleted when user credential is deleted", + nEncryptedValuesPre, encryptedValueRepository.count()); } @Test