cloudfoundry · bruce-ricard · Sep 22, 2021 · Jul 16, 2021 · Jul 19, 2021 · Jul 19, 2021
diff --git a/applications/credhub-api/src/test/resources/application-minimum-duration.yml b/applications/credhub-api/src/test/resources/application-minimum-duration.yml
@@ -0,0 +1,4 @@
+certificates:
+  ca_minimum_duration_in_days: 1825
+  leaf_minimum_duration_in_days: 1460
+
diff --git a/backends/credhub/src/docs/asciidoc/snippets/certificates-v1.adoc b/backends/credhub/src/docs/asciidoc/snippets/certificates-v1.adoc
@@ -7,6 +7,17 @@ See https://github.com/pivotal-cf/credhub-release/blob/master/docs/ca-rotation.m
 
 ---
 
+=== Minimum Duration
+
+A minimum duration can be configured for leaf and CA certificates using the `certificates.leaf_minimum_duration_in_days` and `certificates.ca_minimum_duration_in_days` server-level configuration fields. When these fields are configured, if a request to generate or regenerate a certificate has a duration lower than the minimum, then the minimum duration is used instead.
+
+The API response will include two fields:
+
+* A `duration_overridden` field that is `true` when the minimum duration was used instead, or `false` if the requested duration was used.
+* A `duration_used` field that is the duration (in days) used when the certificate was generated.
+
+---
+
 === Get All Certificates
 operation::GET__certificates__returns_certificates[]
 
@@ -24,7 +35,10 @@ Note: The certificate versions will be sorted in descending order of their creat
 === Regenerate a Certificate
 operation::POST__certificates_uuid_regenerate__returns_certificate[]
 
-Note: If a certificate credential only has one version and it is marked as transitional the credential cannot be regenerated using this endpoint.
+Note:
+
+* If a certificate credential only has one version and it is marked as transitional the credential cannot be regenerated using this endpoint.
+* If the duration used to generate the currently active version of the certificate is lower than the minimum duration, the regenerated certificate will use the minimum duration instead and the response will contain the `duration_overridden` flag set to true. The duration value used to regenerate the certificate is included in the `duration_used` field of the response.
 
 ---
 

diff --git a/backends/credhub/src/docs/asciidoc/snippets/credentials-v1.adoc b/backends/credhub/src/docs/asciidoc/snippets/credentials-v1.adoc
@@ -78,6 +78,11 @@ operation::GET__get_by_name__returns_results[]
 === Generate a Certificate Credential
 operation::POST__generate_certificate_returns__certificate_credential[]
 
+Notes: 
+
+* If the duration is overridden by the minimum duration, the response will contain the `duration_overridden` flag set to true. It will also include the actual duration used to generate the certificate in the `duration_used` field.
+* When the mode is set to converge, certificates are no longer regenerated if the duration doesn't match the existing certificate's duration.
+
 ---
 
 === Generate a Password Credential

diff --git a/backends/credhub/src/docs/asciidoc/snippets/introduction.adoc b/backends/credhub/src/docs/asciidoc/snippets/introduction.adoc
@@ -36,3 +36,5 @@ Credential responses include a unique identifier in the key 'id'. This ID is a u
 As of 2.0.0, set requests always overwrite the credential that already exists.
 
 As of 2.0.1, generate requests can be set to overwrite, no-overwrite, or converge for the mode parameter. The default mode for generate is converge as of 2.0.0. Converge will only overwrite if the generate request parameters do not match the existing credential.
+
+As of 2.10.0, when the generate requests are set to converge for the mode parameter, converge will not overwrite certificates if duration is the only parameter that does not match the existing certificate credentials.
diff --git a/...edhub/src/main/kotlin/org/cloudfoundry/credhub/certificates/DefaultCertificatesHandler.kt b/...edhub/src/main/kotlin/org/cloudfoundry/credhub/certificates/DefaultCertificatesHandler.kt
@@ -25,6 +25,7 @@ import org.cloudfoundry.credhub.services.DefaultCertificateService
 import org.cloudfoundry.credhub.services.PermissionCheckingService
 import org.cloudfoundry.credhub.views.CertificateCredentialView
 import org.cloudfoundry.credhub.views.CertificateCredentialsView
+import org.cloudfoundry.credhub.views.CertificateGenerationView
 import org.cloudfoundry.credhub.views.CertificateVersionView
 import org.cloudfoundry.credhub.views.CertificateView
 import org.cloudfoundry.credhub.views.CredentialView
@@ -79,7 +80,7 @@ class DefaultCertificatesHandler(
 
         auditRecord.setVersion(credentialVersion)
 
-        return CertificateView(credentialVersion, concatenateCas)
+        return CertificateGenerationView(credentialVersion, concatenateCas)
     }
 
     override fun handleGetAllRequest(): CertificateCredentialsView {

diff --git a/...credhub/src/main/kotlin/org/cloudfoundry/credhub/credentials/DefaultCredentialsHandler.kt b/...credhub/src/main/kotlin/org/cloudfoundry/credhub/credentials/DefaultCredentialsHandler.kt
@@ -88,7 +88,7 @@ class DefaultCredentialsHandler(
         auditRecord.setVersion(credentialVersion)
         auditRecord.setResource(credentialVersion.credential)
 
-        return CredentialView.fromEntity(credentialVersion, concatenateCas)
+        return CredentialView.fromEntity(credentialVersion, concatenateCas, true)
     }
 
     override fun setCredential(setRequest: BaseCredentialSetRequest<*>): CredentialView {

diff --git a/...s/credhub/src/main/kotlin/org/cloudfoundry/credhub/regenerate/DefaultRegenerateHandler.kt b/...s/credhub/src/main/kotlin/org/cloudfoundry/credhub/regenerate/DefaultRegenerateHandler.kt
@@ -68,7 +68,7 @@ class DefaultRegenerateHandler(
         auditRecord.setVersion(credentialVersion)
         auditRecord.setResource(credentialVersion.credential)
 
-        return CredentialView.fromEntity(credentialVersion, concatenateCas)
+        return CredentialView.fromEntity(credentialVersion, concatenateCas, true)
     }
 
     override fun handleBulkRegenerate(signerName: String): BulkRegenerateResults {

diff --git a/...credhub/src/test/java/org/cloudfoundry/credhub/handlers/DefaultRegenerateHandlerTest.java b/...credhub/src/test/java/org/cloudfoundry/credhub/handlers/DefaultRegenerateHandlerTest.java
@@ -185,7 +185,7 @@ public void handleRegenerate_whenUserHasPermission_andAclsEnabled_regeneratesCre
       .thenReturn(savedCredentialVersion);
 
     CredentialView actualCredentialView = subjectWithAclsEnabled.handleRegenerate(CREDENTIAL_NAME, null);
-    CredentialView expectedCredentialView = CredentialView.fromEntity(savedCredentialVersion);
+    CredentialView expectedCredentialView = CredentialView.fromEntity(savedCredentialVersion, false, true);
 
     verify(permissionCheckingService, times(1)).hasPermission(USER, CREDENTIAL_NAME, PermissionOperation.WRITE);
     assertThat(actualCredentialView).isEqualTo(expectedCredentialView);
@@ -291,7 +291,7 @@ public void handleRegenerate_whenAclsAreDisabled_regeneratesCredential() {
       .thenReturn(savedCredentialVersion);
 
     CredentialView actualCredentialView = subjectWithAclsDisabled.handleRegenerate(CREDENTIAL_NAME, null);
-    CredentialView expectedCredentialView = CredentialView.fromEntity(savedCredentialVersion);
+    CredentialView expectedCredentialView = CredentialView.fromEntity(savedCredentialVersion, false, true);
 
     verify(permissionCheckingService, times(0)).hasPermission(USER, CREDENTIAL_NAME, PermissionOperation.WRITE);
     assertThat(actualCredentialView).isEqualTo(expectedCredentialView);

diff --git a/...s/credhub/src/test/java/org/cloudfoundry/credhub/integration/CertificateGenerateTest.java b/...s/credhub/src/test/java/org/cloudfoundry/credhub/integration/CertificateGenerateTest.java
@@ -128,6 +128,8 @@ public void certificateGeneration_shouldGenerateCorrectCertificate() throws Exce
         assertThat(result.getBoolean("certificate_authority"), equalTo(true));
         assertThat(result.getBoolean("self_signed"), equalTo(true));
         assertThat(result.getBoolean("generated"), equalTo(true));
+        assertThat(result.getBoolean("duration_overridden"), equalTo(false));
+        assertThat(result.getInt("duration_used"), equalTo(1));
 
         assertThat(picardCert, notNullValue());
Original file line number	Diff line number	Diff line change
Expand Up		@@ -36,3 +36,5 @@ Credential responses include a unique identifier in the key 'id'. This ID is a u
		As of 2.0.0, set requests always overwrite the credential that already exists.

		As of 2.0.1, generate requests can be set to overwrite, no-overwrite, or converge for the mode parameter. The default mode for generate is converge as of 2.0.0. Converge will only overwrite if the generate request parameters do not match the existing credential.

		As of 2.10.0, when the generate requests are set to converge for the mode parameter, converge will not overwrite certificates if duration is the only parameter that does not match the existing certificate credentials.
Copy link Contributor Author pvaramballypivot Jul 29, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. We took a guess on the version the new feature will be in. This might need to be updated. Copy link Contributor bruce-ricard Aug 16, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Two things: This looks like a breaking change to me. If we ship it as is, I would say we need to bump major. We are trying to stay on the "one version" idea, so that we don't have multiple versions to maintain and patch. I think that we would like this feature to be configurable through a feature flag so that we don't need to bump major and be forced to maintain multiple versions. Copy link Member peterhaochen47 Aug 16, 2021 • edited Loading Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Yeah this change is what I was thinking about as well. However, Jesse's initial proposal argues that accounting for duration in the `converge` mode (aka the existing behavior) is wrong to begin with. I personally don't have context on how consumers of credhub use the `converge` mode, but I believe `bosh` is the only credhub consumer that depends on this feature. So I'm kind of relying on the bosh team to judge whether the old behavior is a bug. Also I think bumping CredHub major to 3.0 is not the end of the world? We'll just have to bump CredHub in all the TAS versions from LTS 2.9 to LTS 3.0 (which shouldn't break TAS, bc TAS doesn't use CredHub's certificate feature anyway). Copy link Contributor ystros Aug 16, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. As a member of the BOSH team, I think I would consider the old behavior a bug. The behavior of `converge` is meant to cause a credential to regenerate when the properties declared for the variable have changed. For things like a change to common name or signing CA, it makes a lot of sense that converge mode would cause the certificate to be regenerated - the underlying identity of the certificate has changed. This does not seem the case with duration / expiration date. The cases this would be a breaking change would be as follows: The BOSH operator has a manifest that defines a certificate using `update_mode: converge`. They want to update only the duration field in order to either lengthen or shorten its expiration date. They expect that doing a BOSH deploy will automatically regenerate the certificate with `update_mode: converge` for them using the new duration. The changes would cause it to not regenerate in this case. I am not aware of any product or customer relying on this behavior. It seems very narrow to me, and can be solved by other workflows (such as making an explicit `credhub generate` CLI or equivalent API call). That said, if someone is relying on it, it would be a breaking change for them. We added an explicit feature flag on a temporary branch earlier that would default back to the earlier converge behavior unless turned on. It wasn't difficult, but we're holding off on merging it into this PR. Let us know which direction you would like to go in (release a new major, deem this as a bug fix and not worry about it, or add an explicit feature flag) and we'll proceed accordingly. Copy link Member peterhaochen47 Aug 17, 2021 • edited Loading Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Thanks, @bruce-ricard and I can discuss tomorrow and get back to you. Meanwhile @ystros, can I ask for your input? It seems like the tension is between: Considering duration is a bug, because when the `converge` was introduced, it was never meant for only extending cert duration, which is a narrow and invalid use case. Hence, the lack of a feature flag does not constitute a breaking change, but a bug fix. CredHub can include the current PR in Credhub 2.10. BOSH can include this in a patch as well. Using the `converge` mode to only extend cert duration is a valid use case that people rely on. So CredHub should 1) include a feature flag (with the default being the existing behavior) and include this in a CredHub 2.10 or 2) bump major to CredHub 3.0 and BOSH would need to do the same (right?). What I'm trying to get at, is BOSH team might have better insight on whether using the `converge` mode to only extend cert duration is a valid use case that people rely on (CredHub, AFAIK, does not have a standalone cert use case outside of BOSH). Or to phrase the question a little differently: When the current PR (aka no feature flag) is merged and released, would the resulting new BOSH version be considered having a breaking change & needs major version bump? What would the BOSH team's decision be in this scenario? Copy link Contributor bruce-ricard Aug 17, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. OK right, fair enough. I actually talked with Jesse about that and I now remember suggesting that the current behavior is a bug...