CredHub: Default key_usage for (CA) certificate generation

[jbuecher]

Problem Statement:
Currently we are facing the problem, that all the certificates for our BOSH deployments missing the important certificate V3 attribute key_usage. Even if a CA certificate is generated, CredHub doesn't set the attribute by default. This behaviour differs from the config-server Bootstrap-BOSH uses internally.

Proposed Solution:
In order to harmonise the behaviour of the internal config-server of bootstrap-bosh / bosh cli with CredHub itself, I'd like to propose the following solution based on:

• https://github.com/cloudfoundry/config-server/blob/master/types/certificate_generator.go#L99-L100
• https://github.com/cloudfoundry/config-server/blob/master/types/certificate_generator.go#L125

Using this approach a normal certificate would get the default key_usage: [ digital_signature, key_encipherment] (if nothing specific is provided) and a CA certificate would get the default key_usage: [ key_cert_sign, crl_sign] (if nothing specific is provided).

In case you have further questions please reach out to me. Looking forward hearing from you.

[h0nIg]

CC @beyhan @strehle

[strehle]

ok from SAP side, someone from broadcom should have a review now

[Copilot]

Pull Request Overview

This PR implements default key_usage values for certificate generation to harmonize CredHub's behavior with the config-server used by Bootstrap-BOSH. When no explicit key_usage is provided, CA certificates now default to [key_cert_sign, crl_sign] and regular certificates default to [digital_signature, key_encipherment].

Key Changes:

• Modified CertificateGenerationParameters.buildKeyUsage() to set default key usage based on certificate type (CA vs non-CA)
• Updated test assertions to expect default key usage values instead of null
• Added RemoteCredentialsHandler.getKeyUsageFromRequest() to parse key usage from remote backend object format

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
SignedCertificateGeneratorTest.java	Updated test expectations to verify default key usage is set for CA certificates when no explicit key_usage is provided
CertificateGenerationParameters.kt	Implemented default key usage logic and changed validKeyLengths from Arrays.asList to Kotlin's native listOf
RemoteCredentialsHandler.kt	Added support for parsing key usage from object format in remote backend responses

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jbuecher]

Hi @strehle, I've adjusted the provided code using the hints from copilot. The new commit includes:

• better error handling for reading the JSON text nodes
• providing a separate array for storing the key usages instead of altering the input

Could you take another look regarding the approval process.

[beyhan]

@h0nIg, @peterhaochen47 Is there any chance we could get another review for this? It looks like some people are waiting for this change.

[h0nIg]

@dlinsley

[peterhaochen47]

Using this approach a normal certificate would get the default key_usage: [ digital_signature, key_encipherment] (if nothing specific is provided) and a CA certificate would get the default key_usage: [ key_cert_sign, crl_sign] (if nothing specific is provided).

I think this is likely a breaking change. As the key usage extension is a critical field, so validators from all parties are required to enforce it. So the credhub users who previously did not specify this param when generating a cert will result in the absence of this field in the cert, which means there's no restrictions on its usage. Now after this change, there's restriction, which could break things (CredHub is used by many other systems, outside of BOSH).

We likely have to increment major version number or add a server-level config to feature-flag this / make this opt-in (the latter is preferred).

However, I wonder if this "harmonizing" could be done on the BOSH layer. Could BOSH specify/default the key_usage param when sending generation request to CredHub. (I could ask the BOSH team to chime in).

[h0nIg]

Using this approach a normal certificate would get the default key_usage: [ digital_signature, key_encipherment] (if nothing specific is provided) and a CA certificate would get the default key_usage: [ key_cert_sign, crl_sign] (if nothing specific is provided).

I think this is likely a breaking change. As the key usage extension is a critical field, so validators from all parties are required to enforce it. So the credhub users who previously did not specify this param when generating a cert will result in the absence of this field in the cert, which means there's no restrictions on its usage. Now after this change, there's restriction, which could break things (CredHub is used by many other systems, outside of BOSH).

We likely have to increment major version number or add a server-level config to feature-flag this / make this opt-in (the latter is preferred).

python 3.13 enforces the presence of these fields for a CA, therefore not a breaking change as such and rather an alignment

However, I wonder if this "harmonizing" could be done on the BOSH layer. Could BOSH specify/default the key_usage param when sending generation request to CredHub. (I could ask the BOSH team to chime in).

Why do you want to harmonize on bosh layer, every team specifying "ca=true" as of today, need to adjust their certificate definition?

Overall i would partially agree on the breaking change aspect for regular certificates, but not for CA's. @peterhaochen47 would you agree to merge this with the CA-based defaults only, leaving out the regular certificate defaults (remove or make it configurable)?

[peterhaochen47]

Why do you want to harmonize on bosh layer, every team specifying "ca=true" as of today, need to adjust their certificate definition?

What I was thinking is BOSH director itself (not the BOSH manifest authors, who won't have to adjust anything) performing the harmonizing between credhub vs. config-server. BOSH director could default this key_usage cert generation param when calling credhub API, when it sees the "ca=true" in the corresponding BOSH variable. This makes more sense than doing this in the credhub layer, since this "harmonizing between credhub vs. config-server within BOSH" is not a product goal/concern of CredHub (which serves many other purposes outside of BOSH); but it possibly is BOSH's.

Overall i would partially agree on the breaking change aspect for regular certificates, but not for CA's. @peterhaochen47 would you agree to merge this with the CA-based defaults only, leaving out the regular certificate defaults (remove or make it configurable)?

The regular certs defaulting is a breaking change for sure. The CAs defaulting is less concrete of a breaking change, but the concern remains. The spec does not forbid a cert to have a key_usage of key_cert_sign, crl_sign plus some other usages. A CA cert could currently be used for other purposes alongside key_cert_sign and crl_sign (and due to the absence of key-usage faces no restrictions); but after this change, the restrictions will apply & break the system (for the non-key_cert_sign/crl_sign usages).

So my preference is doing this in the BOSH layer (a more appropriate place to perform the harmonizing). Or, though less preferred, feature-flag the CA defaulting in credhub (and then consider releasing the breaking change at next credhub major version bump).

[jbuecher]

As proposed by @peterhaochen47:

Or, though less preferred, feature-flag the CA defaulting in credhub (and then consider releasing the breaking change at next credhub major version bump).

I've refactored the code to reflect the requested changes. Now, the deployments adjusts based on the values in application.yml, which could be adjusted by an environment variable during deployment.

[jbuecher]

Hi @peterhaochen47 @h0nIg, sorry to bother you during the busiest time of the year. But could you spare some time to give the new proposal another look. It seems the colleagues are eagerly waiting for this change. By having this setup, this default key usage setting could be enabled or disabled via configuration. I think this would not result in a breaking change ^^

[peterhaochen47]

Hi @jbuecher, I started a review a few days ago. See code comments above ^.

[jbuecher]

Hi @peterhaochen47, I assume you mean the comments here in thread. Beside the code comments from Copilot I haven't found someone else's. Okay, this means that you'll consider this feature (also having this new config flag in place) in the next major version?

certificates:
  concatenate_cas: true
  use_default_key_usages: true # (default: false)

[peterhaochen47]

Hi @peterhaochen47, I assume you mean the comments here in thread. Beside the code comments from Copilot I haven't found someone else's. Okay, this means that you'll consider this feature (also having this new config flag in place) in the next major version?

certificates:
  concatenate_cas: true
  use_default_key_usages: true # (default: false)

I have tagged you in the relevant threads above.
Yes, we will consider what's proposed: opt-in key_usages defaulting for CA (no major version bump, since it's opt in)

[jbuecher]

Hi @peterhaochen47 , okay. These are good news for us. Then I'll assume from implementation side is everything done. If you require further (code) changes before getting this PR merged, please leave a review comment inside the respective files (clearer for me to distinguish between all the comments in the thread).

[peterhaochen47]

Please see individual threads

[peterhaochen47]

DefaultCredentialsHandlerTest could have more test coverage, e.g.:

DefaultCredentialsHandlerTest.patch -> apply this patch with git

[jbuecher]

Hi @peterhaochen47, thanks a lot for your support. Patch is successfully applied to the DefaultCredentialsHandlerTest

[peterhaochen47]

LGTM.

I can merge soon if you don't have anything else you wanna change.

Also, it'll be great if you could squash all those commits and leave a final, combined commit message about all the commits (as there are a lot of changes in directions/intentions over the course of these commits, where the earlier messages are often not true anymore) so that the final change set and their associated commit message is cleaner and easier to read. This is something I can also do.

[jbuecher]

Hi @peterhaochen47, thanks a lot for your support. Have squashed all the commits to provide a cleaner history. Feel free to merge the PR.