Default KeyUsage for CA certificates regeneration

[jbuecher]

Hello, as discussed in the previous PR regarding this topic, the setting of the default KeyUsage extension does not completely fulfil our use case. Therefore, I've provided an additional change to set the KeyUsage during the regeneration process as well.

[jbuecher]

@strehle @beyhan could you take a look at these changes

[jbuecher]

@hsinn0 @peterhaochen47 could you have a look at this as well?

[peterhaochen47]

Your original PR contains an integration test CertificateGenerateWitDefaultKeyUsagesTest, do you wanna update/add to that test for the changes in this PR?

[peterhaochen47]

The following recommendation is AI-generated:

---

Issue 1: Inconsistent Parameter Handling

The Problem

Generation path (commit 9b47c4cb):

// In DefaultCredentialsHandler.generateCredential()
val noKeyUsages = req.generationRequestParameters?.keyUsage?.isEmpty() ?: true
if (isCa && noKeyUsages && defaultCAKeyUsages) {
    req.generationRequestParameters?.keyUsage = arrayOf(KEY_CERT_SIGN, CRL_SIGN)  // String[]
}
// Later...
val certificateGenerationParameters = CertificateGenerationParameters(generateRequest.generationRequestParameters!!)

The key usages are set as strings on CertificateGenerationRequestParameters before CertificateGenerationParameters is constructed.

Regeneration path (PR #1049):

// In DefaultCertificatesHandler.handleRegenerate()
val generateRequest = generationRequestGenerator.createGenerateRequest(existingCredentialVersion)
// CertificateGenerationParameters is ALREADY created at this point

if (defaultCAKeyUsages && existingCredentialVersion.isCertificateAuthority) {
    (generateRequest as CertificateGenerateRequest).setKeyUsage(...)  // Modifies KeyUsage object
}

The CertificateGenerationParameters is already created by generationRequestGenerator.createGenerateRequest(), so the PR has to modify the KeyUsage object directly.

The Fix: Align Regeneration with Generation

The cleanest fix is to modify key usages before CertificateGenerationParameters is created in the regeneration path too. This requires changing where the logic lives.

Option A: Move the logic into GenerationRequestGenerator

This is the most architecturally clean solution. The GenerationRequestGenerator creates the request from an existing credential - it could also handle applying default key usages.

// In GenerationRequestGenerator (or its implementation)
class DefaultGenerationRequestGenerator(
    @Value("\${certificates.enable_default_ca_key_usages:false}") private val defaultCAKeyUsages: Boolean,
) : GenerationRequestGenerator {

    override fun createGenerateRequest(
        existingCredentialVersion: CertificateCredentialVersion
    ): BaseCredentialGenerateRequest {
        val certReader = existingCredentialVersion.parsedCertificate
        
        // Build request parameters from existing cert
        val requestParameters = CertificateGenerationRequestParameters()
        requestParameters.commonName = certReader.commonName
        // ... copy other fields ...
        
        // Apply default key usages BEFORE CertificateGenerationParameters is created
        if (defaultCAKeyUsages && existingCredentialVersion.isCertificateAuthority) {
            if (certReader.keyUsage == null) {
                requestParameters.keyUsage = arrayOf(KEY_CERT_SIGN, CRL_SIGN)
            }
        }
        
        // Now create CertificateGenerationParameters
        val generateRequest = CertificateGenerateRequest()
        generateRequest.setRequestGenerationParameters(requestParameters)
        generateRequest.setCertificateGenerationParameters(
            CertificateGenerationParameters(requestParameters)
        )
        return generateRequest
    }
}

Then DefaultCertificatesHandler.handleRegenerate() doesn't need to modify anything:

// In DefaultCertificatesHandler - NO key usage logic needed
override fun handleRegenerate(credentialUuid: String, request: CertificateRegenerateRequest): CredentialView {
    val existingCredentialVersion = certificateService.findByCredentialUuid(credentialUuid)
    val generateRequest = generationRequestGenerator.createGenerateRequest(existingCredentialVersion)
    // ... rest of the method unchanged ...
}

Option B: Delay CertificateGenerationParameters creation in regeneration

If you can't modify GenerationRequestGenerator, restructure the flow in the handler to set request parameters first, then create CertificateGenerationParameters:

// In DefaultCertificatesHandler.handleRegenerate()
val existingCredentialVersion = certificateService.findByCredentialUuid(credentialUuid)

// Get a "raw" request with only requestParameters populated
val generateRequest = generationRequestGenerator.createRawGenerateRequest(existingCredentialVersion)
val certRequest = generateRequest as CertificateGenerateRequest

// Apply defaults at the string level (consistent with generation)
if (defaultCAKeyUsages && existingCredentialVersion.isCertificateAuthority) {
    val existingKeyUsages = certRequest.generationRequestParameters?.keyUsage
    if (existingKeyUsages == null || existingKeyUsages.isEmpty()) {
        certRequest.generationRequestParameters?.keyUsage = arrayOf(KEY_CERT_SIGN, CRL_SIGN)
    }
}

// NOW create CertificateGenerationParameters from the modified request params
certRequest.setCertificateGenerationParameters(
    CertificateGenerationParameters(certRequest.generationRequestParameters!!)
)

This requires GenerationRequestGenerator to have a method that doesn't fully build CertificateGenerationParameters yet.

---

Issue 2: Mutability Concern

The Problem

PR #1049 changes CertificateGenerationParameters.keyUsage from val to var:

// Before
val keyUsage: KeyUsage?

// After (PR #1049)
var keyUsage: KeyUsage?

This makes the class more mutable, which can lead to:

1. Unexpected modifications in other parts of the code
2. Thread-safety issues if the object is shared
3. Harder-to-trace bugs if state changes after construction

The Fix Options

Option A: Avoid the mutation entirely (Preferred)

If you implement Option A from Issue 1 above (move logic to GenerationRequestGenerator), you don't need to change keyUsage to var at all. The key usages are set on CertificateGenerationRequestParameters (string array) before CertificateGenerationParameters is constructed, so keyUsage can remain val.

Option B: Create a new CertificateGenerationParameters instead of mutating

If you must work with an existing CertificateGenerationParameters, create a new one with the modified value instead of mutating:

// In CertificateGenerationParameters - add a copy method
fun withKeyUsage(newKeyUsage: KeyUsage): CertificateGenerationParameters {
    return CertificateGenerationParameters(
        keyLength = this.keyLength,
        duration = this.duration,
        isSelfSigned = this.isSelfSigned,
        caName = this.caName,
        isCa = this.isCa,
        x500Principal = this.x500Principal,
        alternativeNames = this.alternativeNames,
        extendedKeyUsage = this.extendedKeyUsage,
        keyUsage = newKeyUsage,  // The only change
        allowTransitionalParentToSign = this.allowTransitionalParentToSign
    )
}

Then in the handler:

if (defaultCAKeyUsages && existingCredentialVersion.isCertificateAuthority) {
    val currentParams = certRequest.generationParameters as CertificateGenerationParameters
    if (currentParams.keyUsage == null) {
        val newParams = currentParams.withKeyUsage(
            KeyUsage(KeyUsage.keyCertSign or KeyUsage.cRLSign)
        )
        certRequest.setCertificateGenerationParameters(newParams)
    }
}

This requires adding a secondary constructor or a copy/withKeyUsage method, but keeps keyUsage as val.

Option C: Use Kotlin data class copy (if refactored to data class)

If CertificateGenerationParameters were a data class, you'd get copy() for free:

data class CertificateGenerationParameters(
    val keyLength: Int,
    val duration: Int,
    val isSelfSigned: Boolean,
    val caName: String?,
    val isCa: Boolean,
    val x500Principal: X500Principal?,
    val alternativeNames: GeneralNames?,
    val extendedKeyUsage: ExtendedKeyUsage?,
    val keyUsage: KeyUsage?,
    var allowTransitionalParentToSign: Boolean = false
) : GenerationParameters() {
    // Move validation and building logic to companion or factory
}

// Usage:
val newParams = currentParams.copy(keyUsage = defaultCaKeyUsage())

However, this would be a larger refactor.

---

Recommended Solution

The cleanest fix that addresses both issues is Option A from Issue 1: Move the default key usage logic into GenerationRequestGenerator.

Why this is best:

Benefit	Explanation
Consistent parameter handling	Both generation and regeneration set key usages at the string level before CertificateGenerationParameters is built
No mutability change needed	keyUsage stays as val
Single responsibility	The generator is responsible for building the request correctly
Easier testing	You can unit test GenerationRequestGenerator in isolation
Cleaner handlers	Handlers don't need to know about key usage defaulting logic

Implementation Steps:

1. Add defaultCAKeyUsages flag to GenerationRequestGenerator (or its implementation)

2. Move the key usage defaulting logic into createGenerateRequest()

3. Remove setKeyUsage() method from CertificateGenerateRequest (not needed anymore)

4. Keep keyUsage as val in CertificateGenerationParameters

5. Remove the key usage logic from DefaultCertificatesHandler.handleRegenerate()

6. Update tests to verify GenerationRequestGenerator applies defaults correctly

---

Code Sketch

Let me check the current GenerationRequestGenerator to provide a more concrete implementation:

[2 tools called]

Now let me look at CertificateCredentialRegeneratable since that's where the certificate regeneration request is built:

[3 tools called]

Now I can see the full picture. Here's the concrete implementation for fixing both issues:

---

Concrete Fix Implementation

The Problem Location

The CertificateCredentialRegeneratable.createGenerateRequest() creates CertificateGenerationParameters directly from the CertificateReader (line 26-29). At this point, the key usage is extracted from the existing certificate and is immutable (val).

PR #1049 tries to modify it after construction by:

1. Changing keyUsage to var
2. Adding setKeyUsage() method

The Clean Fix

Modify CertificateCredentialRegeneratable to apply default key usages before constructing CertificateGenerationParameters, similar to how the generation path works.

---

Step 1: Inject the configuration into CertificateCredentialRegeneratable

The challenge is that CertificateCredentialRegeneratable is instantiated via Supplier<Regeneratable> in GenerationRequestGenerator, so it doesn't have access to Spring's @Value injection.

Solution: Change the architecture to use Spring injection

// CertificateCredentialRegeneratable.java - Updated
package org.cloudfoundry.credhub.service.regeneratables;

import org.cloudfoundry.credhub.ErrorMessages;
import org.cloudfoundry.credhub.domain.CertificateCredentialVersion;
import org.cloudfoundry.credhub.domain.CertificateGenerationParameters;
import org.cloudfoundry.credhub.domain.CredentialVersion;
import org.cloudfoundry.credhub.exceptions.ParameterizedValidationException;
import org.cloudfoundry.credhub.requests.BaseCredentialGenerateRequest;
import org.cloudfoundry.credhub.requests.CertificateGenerateRequest;
import org.cloudfoundry.credhub.requests.CertificateGenerationRequestParameters;
import org.cloudfoundry.credhub.utils.CertificateReader;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.stereotype.Component;

import static org.apache.commons.lang3.StringUtils.isEmpty;
import static org.cloudfoundry.credhub.requests.CertificateGenerationRequestParameters.CRL_SIGN;
import static org.cloudfoundry.credhub.requests.CertificateGenerationRequestParameters.KEY_CERT_SIGN;

@Component
public class CertificateCredentialRegeneratable implements Regeneratable {

  private final boolean defaultCAKeyUsages;

  public CertificateCredentialRegeneratable(
      @Value("${certificates.enable_default_ca_key_usages:false}") boolean defaultCAKeyUsages
  ) {
    this.defaultCAKeyUsages = defaultCAKeyUsages;
  }

  @Override
  public BaseCredentialGenerateRequest createGenerateRequest(final CredentialVersion credentialVersion) {
    final CertificateCredentialVersion certificateCredential = (CertificateCredentialVersion) credentialVersion;
    final CertificateReader reader = certificateCredential.getParsedCertificate();

    if (isEmpty(certificateCredential.getCaName()) && !reader.isSelfSigned()) {
      throw new ParameterizedValidationException(
        ErrorMessages.CANNOT_REGENERATE_NON_GENERATED_CERTIFICATE);
    }

    // Build request parameters from the existing certificate
    final CertificateGenerationRequestParameters requestParameters = 
        buildRequestParametersFromReader(reader, certificateCredential);

    // Apply default key usages BEFORE CertificateGenerationParameters is created
    // This is consistent with the generation path in DefaultCredentialsHandler
    if (defaultCAKeyUsages && certificateCredential.isCertificateAuthority()) {
      if (reader.getKeyUsage() == null) {
        requestParameters.setKeyUsage(new String[]{KEY_CERT_SIGN, CRL_SIGN});
      }
    }

    // Now create CertificateGenerationParameters from request parameters
    final CertificateGenerationParameters certificateGenerationParameters = 
        new CertificateGenerationParameters(requestParameters);

    final CertificateGenerateRequest generateRequest = new CertificateGenerateRequest();
    generateRequest.setName(certificateCredential.getName());
    generateRequest.setType(certificateCredential.getCredentialType());
    generateRequest.setCertificateGenerationParameters(certificateGenerationParameters);
    generateRequest.setOverwrite(true);
    return generateRequest;
  }

  private CertificateGenerationRequestParameters buildRequestParametersFromReader(
      CertificateReader reader, 
      CertificateCredentialVersion credentialVersion
  ) {
    CertificateGenerationRequestParameters params = new CertificateGenerationRequestParameters();
    params.setCommonName(reader.getCommonName());
    params.setOrganization(reader.getOrganization());
    params.setOrganizationUnit(reader.getOrganizationUnit());
    params.setLocality(reader.getLocality());
    params.setState(reader.getState());
    params.setCountry(reader.getCountry());
    params.setKeyLength(reader.getKeyLength());
    params.setDuration(reader.getDurationDays());
    params.setCa(reader.isCa());
    params.setSelfSigned(reader.isSelfSigned());
    params.setCaName(credentialVersion.getCaName());
    params.setAlternativeNames(reader.getAlternativeNames());
    params.setExtendedKeyUsage(reader.getExtendedKeyUsageStrings());
    params.setKeyUsage(reader.getKeyUsageStrings());  // May be null
    return params;
  }
}

Step 2: Update GenerationRequestGenerator to use Spring injection

// GenerationRequestGenerator.kt - Updated
package org.cloudfoundry.credhub.generate

import org.cloudfoundry.credhub.ErrorMessages
import org.cloudfoundry.credhub.domain.CredentialVersion
import org.cloudfoundry.credhub.exceptions.EntryNotFoundException
import org.cloudfoundry.credhub.requests.BaseCredentialGenerateRequest
import org.cloudfoundry.credhub.service.regeneratables.CertificateCredentialRegeneratable
import org.cloudfoundry.credhub.service.regeneratables.NotRegeneratable
import org.cloudfoundry.credhub.service.regeneratables.PasswordCredentialRegeneratable
import org.cloudfoundry.credhub.service.regeneratables.Regeneratable
import org.cloudfoundry.credhub.service.regeneratables.RsaCredentialRegeneratable
import org.cloudfoundry.credhub.service.regeneratables.SshCredentialRegeneratable
import org.cloudfoundry.credhub.service.regeneratables.UserCredentialRegeneratable
import org.springframework.stereotype.Component

@Component
class GenerationRequestGenerator(
    private val certificateCredentialRegeneratable: CertificateCredentialRegeneratable  // Injected
) {
    private val regeneratableTypeProducers: MutableMap<String, () -> Regeneratable>

    init {
        regeneratableTypeProducers = HashMap()
        regeneratableTypeProducers["password"] = { PasswordCredentialRegeneratable() }
        regeneratableTypeProducers["user"] = { UserCredentialRegeneratable() }
        regeneratableTypeProducers["ssh"] = { SshCredentialRegeneratable() }
        regeneratableTypeProducers["rsa"] = { RsaCredentialRegeneratable() }
        regeneratableTypeProducers["certificate"] = { certificateCredentialRegeneratable }  // Use injected instance
    }

    fun createGenerateRequest(credentialVersion: CredentialVersion?): BaseCredentialGenerateRequest {
        if (credentialVersion == null) {
            throw EntryNotFoundException(ErrorMessages.Credential.INVALID_ACCESS)
        }
        val regeneratable = regeneratableTypeProducers
            .getOrDefault(credentialVersion.getCredentialType()) { NotRegeneratable() }
            .invoke()
        return regeneratable.createGenerateRequest(credentialVersion)
    }
}

Step 3: Remove the key usage logic from DefaultCertificatesHandler

The handler no longer needs to modify key usages - it's handled in CertificateCredentialRegeneratable:

// In DefaultCertificatesHandler.kt - Remove the defaultCAKeyUsages field and logic
@Profile("!remote")
@Service
class DefaultCertificatesHandler(
    private val certificateService: DefaultCertificateService,
    private val credentialGenerator: UniversalCredentialGenerator,
    private val generationRequestGenerator: GenerationRequestGenerator,
    private val auditRecord: CEFAuditRecord,
    private val permissionCheckingService: PermissionCheckingService,
    private val userContextHolder: UserContextHolder,
    @Value("\${security.authorization.acls.enabled}") private val enforcePermissions: Boolean,
    @Value("\${certificates.concatenate_cas:false}") var concatenateCas: Boolean,
    // REMOVED: @Value("\${certificates.enable_default_ca_key_usages:false}") var defaultCAKeyUsages: Boolean,
) : CertificatesHandler {
    
    override fun handleRegenerate(
        credentialUuid: String,
        request: CertificateRegenerateRequest,
    ): CredentialView {
        checkPermissionsByCredentialUuid(credentialUuid, WRITE)

        val existingCredentialVersion = certificateService.findByCredentialUuid(credentialUuid)

        val generateRequest = generationRequestGenerator.createGenerateRequest(existingCredentialVersion)
        generateRequest.metadata = request.metadata

        if (request.allowTransitionalParentToSign) {
            (generateRequest as CertificateGenerateRequest).setAllowTransitionalParentToSign(true)
        }
        if (request.keyLength != null) {
            (generateRequest as CertificateGenerateRequest).setKeyLength(request.keyLength!!)
        }
        if (request.duration != null) {
            (generateRequest as CertificateGenerateRequest).setDuration(request.duration!!)
        }
        // REMOVED: No key usage logic here anymore

        // ... rest of the method unchanged
    }
}

Step 4: Keep keyUsage as val in CertificateGenerationParameters

No changes needed to CertificateGenerationParameters.kt - keyUsage remains immutable:

val keyUsage: KeyUsage?  // Keep as val, no change needed

Step 5: Remove setKeyUsage() from CertificateGenerateRequest

The method added by PR #1049 is no longer needed.

---

Summary of Changes

File	Change
CertificateCredentialRegeneratable.java	Add @Component, inject config, apply defaults before creating CertificateGenerationParameters
GenerationRequestGenerator.kt	Inject CertificateCredentialRegeneratable instead of using Supplier
DefaultCertificatesHandler.kt	Remove defaultCAKeyUsages field and key usage logic
CertificateGenerationParameters.kt	No change - keyUsage stays as val
CertificateGenerateRequest.kt	No change - don't add setKeyUsage() method

---

Benefits

Issue	Resolution
Inconsistent parameter handling	Both paths now set key usages at the CertificateGenerationRequestParameters (string array) level before CertificateGenerationParameters is constructed
Mutability concern	keyUsage remains val - no mutation after construction
Single responsibility	The Regeneratable knows how to build a proper request including defaults
Testability	CertificateCredentialRegeneratable can be unit tested with different config values
No code duplication	Uses the same CertificateGenerationParameters(requestParameters) constructor path as generation

[jbuecher]

thanks a lot for the review @peterhaochen47. I've provided two commits:

• 8ebe735
• 2aec936

The first one goes a little bit more in the direction of your feedback and extracts the key usage generation logic into a shared utility object.

The second one (which I would prefer) takes up the review of the AI and outsourced the logic from the handler into the CertificateCredentialRegeneratable.

[peterhaochen47]

@jbuecher I think the combined effects of all commits look good. Feel free to squash.