Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

genSignedCert should check for expiration of cert in next n days and ensure new self signed cert is provisioned if this happens #124

Closed
2 tasks done
sheldonhull opened this issue Aug 9, 2024 · 1 comment · Fixed by #125
Assignees
Labels

Comments

@sheldonhull
Copy link
Contributor

sheldonhull commented Aug 9, 2024

current behavior

  • genSignedCert in charts/dsv-injector/templates/webhook.yaml generates a cert if the user doesn't provide their own.
  • If the releasename-tls is found, it defaults to this.

So the basics of current behavior

{{- $tlsCert := genSelfSignedCert (include "dsv.dnsname" .) nil (list (include "dsv.dnsname" .) (include "dsv.name" .)) (default 365 .Values.webhookCertExpireDays | int) -}}
{{- $tlsSecret := lookup "v1" "Secret" .Release.Namespace (printf "%s-tls" (include "dsv.name" .)) -}}
  • $tlsCert == self signed cert always generated on run (it's a helm function)

  • $tlsSecret is a lookup to find the "CurrentReleaseName-tls".

  • The clientConfig: value for the webhook:

    clientConfig:
{{- if eq .Values.service.type "ExternalName" }}
      caBundle: {{ .Values.caBundle }}
{{- else if $tlsSecret }}
      caBundle: {{ $tlsSecret.data.cert }}
{{- else }}
      caBundle: {{ $tlsCert.Cert | b64enc }}

Logic explained:

  • eq .Values.service.type "ExternalName" is set, then this takes precedence (for when service is running external to kubernetes). The default in charts/dsv-injector/values.yaml is set to type: ClusterIP. This means unless this is changed, the default behavior will fall through to checking if existing cert found in namespace for this release (aka someone installed their own cert before.
  • caBundle is not set by default, but could be passed in with the helm install command if desired the caBundle must be a base64 string containing a PEM-encoded certificate chain that validates the certificate per charts/dsv-injector/values.yaml.
  • Else if existing $tlsSecret that was looked up is found, then it uses this. That doesn't check if it's valid or not, so it will just reuse what's there.
  • else if no existing cert (meaning fresh install/or expired cert was deleted) then it uses the self signed cert from helm function genSelfSignedCert.
  • The logic expects caBundle to only be used when designating an external service.

improvement to behavior

  • The check against {{- else if $tlsSecret }} should check if exists, but also check that the tls secret cert expiration <= in days from recreateSelfSignedCertThreshold.
  • recreateSelfSignedCertThreshold will default in helm values to 90 days.
  • webhookCertExpireDays should be exposed in the values.yaml with default of 365, rather than default set in the webhook.yaml so it's more visible
  • This same check should be on the secret, which creates the cert files separated in: charts/dsv-injector/templates/webhook.yaml. This should be modified to also have the same check for expiration.
data:
{{- if $tlsSecret }}
  cert.pem: {{ $tlsSecret.data.cert }}
  key.pem: {{ $tlsSecret.data.key }}
{{- else }}
  cert.pem: {{ $tlsCert.Cert | b64enc }}
  key.pem: {{ $tlsCert.Key | b64enc }}
{{- end }}

Looking Into

  • should this be gated behind a SelfSignedCertRegeneration to avoid impact to custom provided cert. ANSWER: The logic expects caBundle to only be used when designating an external service. Otherwise genSelfSignedCert is what's used.
  • we load custom provided cert via config map with DSV_CERT I believe. Validate if this impacts anything here, since it's not exposed as a helm input for this process currently, but expected to be done on app loading. ANSWERED below.

How this Relates to the DSV_CERT and loading in injector

  • cmd/injector/main.go caused some confusion initially as a container wouldn't have knowledge of "${HOME}". I backtracked this though and recall now why this is set.

So yes it was always finding it (else it would error as fatal/termination of Run.
But nothing was checking the expiration of the cert.

related to AB#590946

@sheldonhull sheldonhull self-assigned this Aug 9, 2024
sheldonhull added a commit that referenced this issue Aug 9, 2024
…creation

Related to #124

Add expiration check for self-signed certificates in `charts/dsv-injector/templates/webhook.yaml`.

* Add a new variable `recreateSelfSignedCertThreshold` with a default of 90 days.
* Add a check for the expiration of the existing cert in the next n days.
* Update the logic to generate a new self-signed cert if the existing cert is expiring within `recreateSelfSignedCertThreshold` days.
* Update the secret cert value mapping to use the `$tlsCert` value based on it meeting the expiration check requirement.

Expose `webhookCertExpireDays` and `recreateSelfSignedCertThreshold` in `charts/dsv-injector/values.yaml`.

* Expose `webhookCertExpireDays` with a default of 365 days.
* Add `recreateSelfSignedCertThreshold` with a default of 90 days.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/DelineaXPM/dsv-k8s/issues/124?shareId=XXXX-XXXX-XXXX-XXXX).
@sheldonhull
Copy link
Contributor Author

Adjusted behavior in PR since couldn't get cert auto renewal to be detected. Closing PR will mark this done.

sheldonhull added a commit that referenced this issue Aug 14, 2024
…allation (#125)

Related to #124

- Improve helm chart install and upgrade directions to help with reset
of the self signed cert.
- Add a check for the expiration of the existing cert in the next n days
and log this.
- Align the secret type to tls type.
- Expose `webhookCertExpireDays` with a default of 365 days.
- Chart doesn't try to recreate secret, instead gives kubectl command to
do this as hook life cycles aren't a straight forward to do this without
errors I can't easily test for.

fixes
[AB#590946](https://thycotic.visualstudio.com/4a89362e-1361-424f-a291-a8f57c2a8991/_workitems/edit/590946)
fixes #124

---

For more details, open the [Copilot Workspace
session](https://copilot-workspace.githubnext.com/DelineaXPM/dsv-k8s/issues/124?shareId=1fba5ec4-ec09-46f8-8477-05bc72386546).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants