Add logic to generate certificates using minimum durations #201

pvaramballypivot · 2021-07-23T19:07:01Z

Receive 2 new fields in the server configuration file: ca_minimum_duration and leaf_minimum_duration
Update cert generation/regeneration logic to use the values in these fields such that the cert gets the larger duration between the requested and the new fields
Update converge mode logic to no longer consider the duration field
Return a duration_overridden flag in the generate/regenerate API response when the minimum duration is used

For more information, refer this issue: pivotal/credhub-release#60

cc @pabloarodas @ystros

cf-gitbot · 2021-07-23T19:07:04Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/178992746

The labels on this github issue will be updated when the story is started.

linux-foundation-easycla · 2021-07-23T19:07:07Z

The committers are authorized under a signed CLA.

✅ Preethi Varambally (9fe6607, a196a39, fc8e5ea, df85eb6, 1197675)
✅ Pablo Rodas (2922f78, 7740945, 3a71bf0)
✅ Brian Upton (b38c668, 4b2b59b, e48eff4, 496e70c)

pvaramballypivot · 2021-07-29T22:15:57Z

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.


We took a guess on the version the new feature will be in. This might need to be updated.

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.

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).

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.

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?

OK right, fair enough. I actually talked with Jesse about that and I now remember suggesting that the current behavior is a bug...

peterhaochen47 · 2021-08-12T22:43:25Z

Finished a read through. The general approach & all the changes look good on a high level. Great job! Particularly, thank you for fixing these existing problems:

Fixing an existing mistake on the controller test + docs (e48eff4)
Updating media type (496e70c)

The change that most resembles a breaking change is the "loosen" effect of the converge mode. Citing the doc change:

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.

I would like to think about this a little longer, given that this change is not feature-flagged (unlike the 2 minimum duration configs, which are defaulted to empty). Would there be any potential unintended consequences?

Note to self: TODO - Think about whether this feature warrants an acceptance (aka end-to-end) test like this.

I will finalize the review, hopefully early next week. With @bruce-ricard giving a high-level second opinion. And then we can working on merging and releasing.

ystros · 2021-08-12T22:56:07Z

Updating media type (496e70c)

We can drop that commit if you think it's too unrelated (which is why we made it as a separate commit). We noticed when running the linter that it was complaining loudly about that due to Spring version, so fixed it as we were addressing other things. The linter warnings were present in the original code and was not caused by changes we made.

bruce-ricard

Added some inline comments and questions about the code.

Please let me know if something doesn't make sense.

Also, the first commit in the PR has its commit message starting with "WIP". Is that commit still a work in progress? If not could you amend it to get rid of the "WIP"?

bruce-ricard · 2021-08-16T13:55:11Z

applications/credhub-api/src/test/resources/application-minimum-duration.yml

@@ -0,0 +1,3 @@
+certificates:
+  ca_minimum_duration: 1825
+  leaf_minimum_duration: 1460


You should update your text editor's setting to add newlines at the end of files. Unix files should end in a newline.

applications/credhub-api/src/test/resources/application-minimum-duration.yml

bruce-ricard · 2021-08-16T14:00:29Z

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.


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.

bruce-ricard · 2021-08-16T14:05:28Z

...ents/credentials/src/main/java/org/cloudfoundry/credhub/generators/CertificateGenerator.java

+  private final Integer caMinimumDuration;
+  private final Integer leafMinimumDuration;


I would like to see these variables to be of type Duration.

In the configuration, we don't have an option and have to use Strings and Integer, but once the middleware is passed, I think that it's better to use types that represent better what the object is.
It also solves the problem of the variable name not expressing which unit the integer is in.

While I'm generally in favor of better types, we don't think it adds a great deal of value in this case. The two "duration" fields are only used to overwrite the pre-existing duration field on the CertificateGenerationParameters class. That field comes in via the API and is already an integer in units of days:
https://github.com/cloudfoundry-incubator/credhub/blob/3a71bf0a2f00e8d031eb48569ddd083077ccbaf5/components/credentials/src/main/kotlin/org/cloudfoundry/credhub/domain/CertificateGenerationParameters.kt#L41

If we were to convert it to a Duration, we would just be immediately converting it back to an integer when we do set it on the generation parameters. See:
https://github.com/cloudfoundry-incubator/credhub/blob/65c2c17153bee5bb8ccfae307c83734e3000f116/components/credentials/src/main/java/org/cloudfoundry/credhub/generators/CertificateGenerator.java#L133-L139
and
https://github.com/cloudfoundry-incubator/credhub/blob/65c2c17153bee5bb8ccfae307c83734e3000f116/components/credentials/src/main/java/org/cloudfoundry/credhub/generators/CertificateGenerator.java#L61

The variables are not used anywhere else that would benefit from the more rich datatype. We've renamed the variables to include InDays to make units more clear. Let us know if that isn't sufficient and we can update it to be an actual Duration object.

Hmm alright, indeed. I guess I was hoping that there was some intermediate layer in which we could have put the Duration. It looks kinda superfluous here. Although I think that I would have still done it myself, because I like high level types. Your call :).

...ents/credentials/src/main/java/org/cloudfoundry/credhub/generators/CertificateGenerator.java

bruce-ricard · 2021-08-16T14:09:25Z

...ents/credentials/src/main/java/org/cloudfoundry/credhub/generators/CertificateGenerator.java

+    Boolean durationOverridden = null;
+
+    params.setDuration(getCertificateDuration(params));
+
+    if (originalDuration != params.getDuration()) {
+      durationOverridden = true;
+    }
+
    if (params.isSelfSigned()) {
      try {
        final String cert = CertificateFormatter.pemOf(signedCertificateGenerator.getSelfSigned(keyPair, params));
-        return new CertificateCredentialValue(cert, cert, privatePem, null, params.isCa(), params.isSelfSigned(), true, false);
+        return new CertificateCredentialValue(cert, cert, privatePem, null, null, params.isCa(), params.isSelfSigned(), true, false, durationOverridden);


It appears that you create a variable of type Boolean, but you never use the value false.

If I read this correctly, you initialize it to null, optionally set it to true on line 64, but no code ever sets it to false.

Am I reading this correctly?
If yes, then maybe Boolean isn't the type you want to use.

We just pushed a change to the behavior here which I described in the other comment.

When the feature is not configured, the field is omitted (null)

When the feature is configured but the requested duration was used, the field is false.

When the feature is configured and the minimum duration was used instead, the field is true.

Let us know if this makes sense.

...edentials/src/main/kotlin/org/cloudfoundry/credhub/domain/CertificateGenerationParameters.kt

bruce-ricard · 2021-08-16T14:24:15Z

...redentials/src/main/kotlin/org/cloudfoundry/credhub/credential/CertificateCredentialValue.kt

@@ -55,6 +55,9 @@ class CertificateCredentialValue : CredentialValue {
    @JsonIgnore
    var versionCreatedAt: Instant? = null

+    @JsonInclude(Include.NON_NULL)
+    var durationOverridden: Boolean? = null


Are you making this nullable for backwards compatibility with older certificates that won't have this property?

The general intent behind it being nullable (here and in other spots) is so that when the feature is disabled, the API does not return the field at all. This allows the corresponding CLI update (see cloudfoundry/credhub-cli#110) to:

Not display duration_overridden in CLI output when the server-side feature is disabled

Not display duration_overridden in CLI output when talking to older versions of the CredHub server that doesn't have this feature (perhaps not a large problem)

Display duration_overridden: true in CLI output when the server-side feature is enabled and overridden.

It seemed heavy handed to always display duration_overridden in CLI output, so we arrived at making this nullable. However, your other feedback about this field never being set to false is a good catch. We're considering adding a 4th case:
4. Display duration_overridden: false in CLI output when the server-side feature is enabled but the duration is NOT overridden.

Thoughts?

OK thank you for the explanation, that makes a lot of sense.

There are 2 things I'd like to talk about: A general software point, and discussing these 4 options you mention.

I am a big believer in the Software billion dollar mistake. I believe that we should try to avoid using null as much as possible.
In this specific case, if you need a type containing 3 or 4 values, I would like to see some sort of enum created to represent all the possible options.

Regarding your specific question

Display duration_overridden: false in CLI output when the server-side feature is enabled but the duration is NOT overridden.

I think that it does make sense.

I am also trying to think about the possibility of making the UX better. Putting myself in the shoes of a user, I am wondering how I feel about seeing a response containing duration_overwritten: true. "Overwritten to what?" I can hear myself think. Is the duration displayed in the CLI's output? If it is, then why bother add the "duration_overwritten" flag? The user will notice that it's not the one he asked for.

Completely different topic: forgive my non-native English, but I after some googling it seems to me like the proper spelling of the word is "overwritten". Google isn't too happy when you use "d"s instead of "t"s.

We've updated the duration_overridden field to be non-nullable, so it will always appear in the API response regardless of whether the feature is enabled or not. That should make the API more consistent. We are doing separate work to update the logic of the CLI PR.

We have not renamed it - overridden is proper spelling, based off of the word override. Versus overwritten is based off of the word overwrote. Override seems more correct to us here, as the server is making a decision to reject the duration passed and use a different duration instead. But either word should work.

overridden is proper spelling, based off of the word override

Oh right, of course. I did some googling and I may have learned something about the different between "overwrite" and "override". Makes a lot of sense now, this appears to indeed be an override, and not an overwrite.

We've updated the duration_overridden field to be non-nullable, so it will always appear in the API response regardless of whether the feature is enabled or not. That should make the API more consistent.

Now we lose the possibility to disable the feature for old users who don't need it and don't want breaking changes. This is important to us, so that we can ship this in a patch release, and not have to maintain multiple versions (as mentioned in another conversation).

My previous comments were not talking at the feature level, only about the implementation detail of this field. I think that I was pretty happy with the feature.

Here is my position, let me know if you disagree:

I think that we need a type that allows to represent 3 values:

Duration overridden (and obviously "feature enabled") -- the previous true

Feature enabled but duration kept -- the previous false

Feature disabled -- the previous null

I really want to avoid null, so I'd rather not use a nullable boolean.

Hence, I think that we need an Enum type to represent all these values, and depending on the values, we can decide what we return back.

bruce-ricard · 2021-08-16T14:24:44Z

...redentials/src/main/kotlin/org/cloudfoundry/credhub/credential/CertificateCredentialValue.kt

@@ -55,6 +55,9 @@ class CertificateCredentialValue : CredentialValue {
    @JsonIgnore
    var versionCreatedAt: Instant? = null

+    @JsonInclude(Include.NON_NULL)


I'm a bit confused by the NON_NULL tag together with the nullable value. Could you elaborate please?

This is a jackson annotation that means: if this field has null value, do not include it when serializing this java object into json. Aka, ignore when null. Aka, only include when not null.

I don't understand what difference that makes then.

If you include null values, then the variable gets set to null.
If you don't, the variable stays set to null.

Doesn't it equal null regardless?

This is just about how the resulting serialized json looks like. For example, when you have the java object foo=abc, bar=null:

with the NON_NULL annotation, the resulting json looks like {foo:"abc"}

without the NON_NULL annotation, the resulting json looks like {foo:"abc", bar:"null"}

I see. So I guess my main point, following the discussion about this field being nullable, is that if this field can't be null, we don't need any shady annotation.

Since the field is no longer nullable, it doesn't use this annotation anymore.

However, since this class is used in contexts unrelated to the generate and regenerate endpoints, we have marked it instead with the JsonIgnore annotation. This directs the JSON renderer not to output the field at all.

This was necessary since this class is also used in GET endpoints like /api/v1/data?name=cert-name, we do not want the field to appear - the duration being overridden and the originally requested duration are not persisted data in the database per the requirements laid out in the original issue.

I am confused again. Why did the "nullable" change impact the fact that the field should be included or not in JSON, and hence the JsonIgnore annotation?

We only wanted to conditionally include the field in responses for the generate / regenerate API endpoints. However, this class is used also with the GET endpoints. When the field was nullable, it only ever get set to a non-null value when generating or regenerating a certificate. Due to the previous JsonInclude(Include.NON_NULL) annotation, this meant it was not rendered for the GET endpoints, since the value was null on those code paths.

However, since it is now not nullable, it will always have a value (false for duration_overridden, 0 for duration_used). This means that the GET endpoints would start rendering:

{ "name": "some-cert", ...more properties..., "duration_overridden": false, "duration_used": 0 }

which is not desirable - these fields only apply when generating/regenerating the certificate. So we added the JsonIgnore to ensure it is not output. This is now controlled by the new CertificateGenerationView class introduced in one of the latest commits. The existing trustedCa and versionCreatedAt fields on this CertificateCredentialValue class have similar JsonIgnore annotations, likely for the same reason.

...ents/credentials/src/main/java/org/cloudfoundry/credhub/generators/CertificateGenerator.java

linux-foundation-easycla · 2021-08-16T22:05:31Z

The committers are authorized under a signed CLA.

✅ Preethi Varambally (9fe6607, a196a39, fc8e5ea, df85eb6, 1197675, 65c2c17)
✅ Pablo Rodas (2922f78, 7740945, 3a71bf0)
✅ Brian Upton (b38c668, 4b2b59b, e48eff4, 496e70c)

bruce-ricard · 2021-08-18T08:50:22Z

You haven't replied or acted on the comment about your first commit starting with the letters "WIP". I am guessing that you missed it.

I still believe that it would be better to amend it. Let me know what you think.

bruce-ricard · 2021-08-18T11:18:07Z

...redentials/src/main/kotlin/org/cloudfoundry/credhub/credential/CertificateCredentialValue.kt

@@ -55,6 +55,9 @@ class CertificateCredentialValue : CredentialValue {
    @JsonIgnore
    var versionCreatedAt: Instant? = null

+    @JsonInclude(Include.NON_NULL)
+    var durationOverridden: Boolean? = null


OK thank you for the explanation, that makes a lot of sense.

There are 2 things I'd like to talk about: A general software point, and discussing these 4 options you mention.

I am a big believer in the Software billion dollar mistake. I believe that we should try to avoid using null as much as possible.
In this specific case, if you need a type containing 3 or 4 values, I would like to see some sort of enum created to represent all the possible options.

Regarding your specific question

Display duration_overridden: false in CLI output when the server-side feature is enabled but the duration is NOT overridden.

I think that it does make sense.

I am also trying to think about the possibility of making the UX better. Putting myself in the shoes of a user, I am wondering how I feel about seeing a response containing duration_overwritten: true. "Overwritten to what?" I can hear myself think. Is the duration displayed in the CLI's output? If it is, then why bother add the "duration_overwritten" flag? The user will notice that it's not the one he asked for.

Completely different topic: forgive my non-native English, but I after some googling it seems to me like the proper spelling of the word is "overwritten". Google isn't too happy when you use "d"s instead of "t"s.

...redentials/src/main/kotlin/org/cloudfoundry/credhub/credential/CertificateCredentialValue.kt

bruce-ricard · 2021-08-18T11:23:31Z

.../credentials/src/test/java/org/cloudfoundry/credhub/generators/CertificateGeneratorTest.java

@@ -317,6 +319,8 @@ public void whenSelfSignIsTrue_itGeneratesAValidSelfSignedCertificate() throws E
      equalTo(CertificateFormatter.pemOf(certificate)));
    assertThat(certificateCredential.getCa(), equalTo(CertificateFormatter.pemOf(certificate)));
    verify(signedCertificateGenerator, times(1)).getSelfSigned(rootCaKeyPair, inputParameters);
+    assertThat(certificateCredential.getDurationOverridden(),
+            equalTo(null));


When the null reaches the tests, we have gone too far.

bruce-ricard · 2021-08-18T11:26:31Z

...redentials/src/main/kotlin/org/cloudfoundry/credhub/credential/CertificateCredentialValue.kt

@@ -55,6 +55,9 @@ class CertificateCredentialValue : CredentialValue {
    @JsonIgnore
    var versionCreatedAt: Instant? = null

+    @JsonInclude(Include.NON_NULL)


I see. So I guess my main point, following the discussion about this field being nullable, is that if this field can't be null, we don't need any shady annotation.

bruce-ricard · 2021-08-18T11:52:20Z

...ents/credentials/src/main/java/org/cloudfoundry/credhub/generators/CertificateGenerator.java

+    return Math.max(params.getDuration(), leafMinimumDurationInDays);
+  }
+
+  private boolean isMinimumDurationEnabled() {


That actually appears to be logically incorrect, isn't it?

If the CA minimum duration is set to 3, and the leaf isn't set, and you are generating a leaf, this function will return true, which will result in the durationOverwritten field to be true or false, as if the feature was enabled for leaves. While it was only enabled for CAs.

So I didn't realize during my first review that it was important to know if the feature was enabled or not, for the purpose of showing the information in the response or not.

So it's annoying. It looks like you need to have 2 functions: isMinimumDurationEnabledForCAs and ...ForLeaves.

Unless you actually assume that if a user sets one of them, then the whole feature is enabled. I guess that that's what you had in mind.

Unless you actually assume that if a user sets one of them, then the whole feature is enabled. I guess that that's what you had in mind.

Yes. We considered enabling either a leaf minimum or a CA minimum to be enabling the feature as a whole. However, since we've removed the null response, this method has been removed entirely.

👌

We may want this function back if we want to be able to disable the feature, and mentioned in a previous conversation.

Signed-off-by: Pablo Rodas <prodas@vmware.com> Signed-off-by: Brian Upton <bupton@vmware.com>

Created equalsIgnoringDuration to CertificateGenerationRequestParameters class Co-authored-by: Preethi Varambally <pvarambally@vmware.com> Co-authored-by: Brian Upton <bupton@vmware.com> [#174516521] **[JIRA Issue]** Deprecated property for smoke test apps domain is required

* Slight refactor of CertificateGenerationParameters to have equals reuse the new equalsIgnoringDuration method * Fixes wrong expected duration in CertificateGenerator tests * Updates spec that previously assumed that certs would converge based on duration * Add integration tests around converge and minimum behaviors Co-authored-by: Preethi Varambally <pvarambally@vmware.com> Co-authored-by: Pablos Rodas <prodas@vmware.com> [#174516521] **[JIRA Issue]** Deprecated property for smoke test apps domain is required

Authored-by: Preethi Varambally <pvarambally@vmware.com> Authored-by: Brian Upton <bupton@vmware.com> Authored-by: Pablo Rodas <prodas@vmware.com> [#174516521] **[JIRA Issue]** Deprecated property for smoke test apps domain is required

Added validation to set the value to true only when the duration changed. Modified integration tests. Co-authored-by: Brian Upton <bupton@vmware.com> Co-authored-by: Preethi Varambally <pvarambally@vmware.com> [#174516521] **[JIRA Issue]** Deprecated property for smoke test apps domain is required

Co-authored-by: Pablo Rodas <prodas@vmware.com> Co-authored-by: Preethi Varambally <pvarambally@vmware.com> [#174516521] **[JIRA Issue]** Deprecated property for smoke test apps domain is required

There isn't similar logging within CredHub, and we want to have more discussion before introducing a new logging pattern. Signed-off-by: Preethi Varambally <pvarambally@vmware.com>

Co-authored-by: Brian Upton <bupton@vmware.com>

Authored-by: Preethi Varambally <pvarambally@pivotal.io> Authored-by: Brian Upton <bupton@vmware.com> Authored-by: Pablo Rodas <prodas@vmware.com>

The previous docs for /api/v1/certificates/:id/regenerate were using the wrong view and thus the docs did not reflect what the endpoint actually returned. Authored-by: Preethi Varambally <pvarambally@pivotal.io> Authored-by: Brian Upton <bupton@vmware.com> Authored-by: Pablo Rodas <prodas@vmware.com>

The APPLICATION_JSON_UTF8 media type was deprecated in Spring 5.2 in favor of just using APPLICATION_JSON and setting a character encoding when used as a contentType. The linting script complains about these usages. It is unclear what the intention behind using APPLICATION_JSON_UTF8 was, but testing character encoding feels out-of-scope for tests that are being used to generate documentation. Authored-by: Brian Upton <bupton@vmware.com> Authored-by: Pablo Rodas <prodas@vmware.com>

Authored-by: Brian Upton <bupton@vmware.com> Authored-by: Preethi Varambally <pvarambally@pivotal.io>

- The field will be set to null when the feature is turned off - The field will be set to false when the feature is turned on but the requested duration is used - The field will be set to true when the feature is turned on and the minimum duration is used Co-authored-by: Brian Upton <bupton@vmware.com> Co-authored-by: Pablo Rodas <prodas@vmware.com> Co-authored-by: Preethi Varambally <pvarambally@pivotal.io>

Also documents behavior of the duration_override API response field and improves formatting of other mentions of the field. Co-authored-by: Brian Upton <bupton@vmware.com> Co-authored-by: Pablo Rodas <prodas@vmware.com> Co-authored-by: Preethi Varambally <pvarambally@pivotal.io>

ystros · 2021-08-18T17:03:46Z

Amended the initial commit to remove the "WIP".

Removes the null case from all usages of the new durationOverridden field. Because the existing CertificateView is used for all certificate-related endpoints (including GET, DELETE, etc), a new view class had to be introduced to prevent the duration overridden field from showing up on inappropriate endpoints. The new CertificateGenerationView inherits from the original CertificateView and only adds fields that are specific to the generate and regenerate endpoints. Co-authored-by: Brian Upton <bupton@vmware.com> Co-authored-by: Pablo Rodas <prodas@vmware.com> Co-authored-by: Preethi Varambally <pvarambally@pivotal.io>

The new duration_used field contains the duration (either the requested or minimum) that was actually used to generate or regenerate the certificate. Co-authored-by: Brian Upton <bupton@vmware.com> Co-authored-by: Pablo Rodas <prodas@vmware.com> Co-authored-by: Preethi Varambally <pvarambally@pivotal.io>

peterhaochen47

LGTM

applications/credhub-api/src/test/resources/application-minimum-duration.yml

bruce-ricard · 2021-09-20T22:01:13Z

...ds/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);


very hard to understand what is happening in this function with the false, true parameters.

bruce-ricard · 2021-09-20T22:05:05Z

...redentials/src/main/kotlin/org/cloudfoundry/credhub/credential/CertificateCredentialValue.kt

+
+    @JsonIgnore
+    var durationUsed: Int = 0
+
    constructor() : super() {}

    constructor(


What a nightmare of a constructor this has become over time. I wanna try to refactor this.

cf-gitbot added the unscheduled label Jul 23, 2021

This was referenced Jul 23, 2021

Display duration_overridden flag in the generate/regenerate output when not null cloudfoundry/credhub-cli#110

Closed

Update spec and template to include minimum duration for certificates pivotal/credhub-release#61

Merged

pvaramballypivot commented Jul 29, 2021

View reviewed changes

bruce-ricard suggested changes Aug 16, 2021

View reviewed changes

pvaramballypivot force-pushed the wip-certificate-minimum-duration branch from 312e5dd to 65c2c17 Compare August 16, 2021 22:11

bruce-ricard reviewed Aug 18, 2021

View reviewed changes

pvaramballypivot and others added 15 commits August 18, 2021 10:01

Enforce CA minimum duration if configured

93b410f

Signed-off-by: Pablo Rodas <prodas@vmware.com> Signed-off-by: Brian Upton <bupton@vmware.com>

Print override flag in cert generation output

09d3a4b

Authored-by: Preethi Varambally <pvarambally@vmware.com> Authored-by: Brian Upton <bupton@vmware.com> Authored-by: Pablo Rodas <prodas@vmware.com> [#174516521] **[JIRA Issue]** Deprecated property for smoke test apps domain is required

Add integration tests for other regenerate APIs

b1dc092

Co-authored-by: Pablo Rodas <prodas@vmware.com> Co-authored-by: Preethi Varambally <pvarambally@vmware.com> [#174516521] **[JIRA Issue]** Deprecated property for smoke test apps domain is required

Remove logging for the moment

599bcf4

There isn't similar logging within CredHub, and we want to have more discussion before introducing a new logging pattern. Signed-off-by: Preethi Varambally <pvarambally@vmware.com>

Solved lint problems

f9fbbcd

Co-authored-by: Brian Upton <bupton@vmware.com>

Update generate cert documentation with minimum duration details

be0337d

Authored-by: Preethi Varambally <pvarambally@pivotal.io> Authored-by: Brian Upton <bupton@vmware.com> Authored-by: Pablo Rodas <prodas@vmware.com>

Update docs to include logic change for converge

b6c3d8d

Authored-by: Brian Upton <bupton@vmware.com> Authored-by: Preethi Varambally <pvarambally@pivotal.io>

Rename *minimumDuration fields, make 0 the default

05d6183

Authored-by: Brian Upton <bupton@vmware.com> Authored-by: Preethi Varambally <pvarambally@pivotal.io>

ystros force-pushed the wip-certificate-minimum-duration branch from baff130 to f2d31ca Compare August 18, 2021 17:03

ystros and others added 2 commits August 18, 2021 14:08

peterhaochen47 self-requested a review August 31, 2021 18:32

peterhaochen47 approved these changes Aug 31, 2021

View reviewed changes

bruce-ricard suggested changes Sep 20, 2021

View reviewed changes

bruce-ricard merged commit a64374f into cloudfoundry:main Sep 22, 2021

cf-gitbot removed the unscheduled label Sep 22, 2021

		private final Integer caMinimumDuration;
		private final Integer leafMinimumDuration;

Add logic to generate certificates using minimum durations #201

Add logic to generate certificates using minimum durations #201

Conversation

pvaramballypivot commented Jul 23, 2021 • edited Loading

cf-gitbot commented Jul 23, 2021

linux-foundation-easycla bot commented Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhaochen47 Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhaochen47 Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhaochen47 commented Aug 12, 2021 • edited Loading

ystros commented Aug 12, 2021

bruce-ricard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ystros Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ystros Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhaochen47 Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhaochen47 Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ystros Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Aug 16, 2021 • edited Loading

bruce-ricard commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ystros commented Aug 18, 2021

peterhaochen47 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruce-ricard Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

pvaramballypivot commented Jul 23, 2021 •

edited

Loading

linux-foundation-easycla bot commented Jul 23, 2021 •

edited

Loading

peterhaochen47 Aug 16, 2021 •

edited

Loading

peterhaochen47 Aug 17, 2021 •

edited

Loading

peterhaochen47 commented Aug 12, 2021 •

edited

Loading

ystros Aug 17, 2021 •

edited

Loading

ystros Aug 19, 2021 •

edited

Loading

peterhaochen47 Aug 16, 2021 •

edited

Loading

peterhaochen47 Aug 16, 2021 •

edited

Loading

ystros Aug 19, 2021 •

edited

Loading

linux-foundation-easycla bot commented Aug 16, 2021 •

edited

Loading

bruce-ricard Sep 20, 2021 •

edited

Loading