Skip to content

Commit

Permalink
fix: Orphaned encrypted_value when Credentials are deleted
Browse files Browse the repository at this point in the history
- 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]
  • Loading branch information
hsinn0 committed Apr 17, 2024
1 parent a26ebb2 commit 0a6dc71
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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) " +
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -51,6 +54,9 @@ public class CertificateVersionDeleteTest {
@Autowired
private WebApplicationContext webApplicationContext;

@Autowired
EncryptedValueDataService encryptedValueDataService;

private MockMvc mockMvc;

@Rule
Expand All @@ -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);
Expand All @@ -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();
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<CredentialVersionData<*>> =
mutableListOf()

@Column(nullable = false)
override var name: String? = null
set(name) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,21 +43,21 @@ abstract class CredentialVersionData<Z : CredentialVersionData<Z>>(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
Expand All @@ -66,7 +66,7 @@ abstract class CredentialVersionData<Z : CredentialVersionData<Z>>(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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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"));

Expand Down Expand Up @@ -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"));

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 0a6dc71

Please sign in to comment.