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

feat: add cert rotator #869

Merged
merged 3 commits into from
Jun 16, 2023
Merged

Conversation

binbin-li
Copy link
Collaborator

@binbin-li binbin-li commented Jun 5, 2023

Description

What this PR does / why we need it:

This is a following PR of #831. Add cert-rotator in Ratify to support rotating TLS related certificates automatically. The overall design doc can be found here: https://hackmd.io/@akashsinghal/BySB4VbHh

  1. We adopt the cert-controller used in Gatekeeper. The cert-controller checks the validation of certificates every 12 hours and generates a new certificate that is valid for 10 years 90 days before the old certificate expires.
  2. Upgrade GK versions from 3.10.0/3.11.0 to 3.11.0/3.12.0 as cert-controller only works for the externaldata.gatekeeper.sh/v1beta1.
  3. Add access permissions to providers and secrets for Ratify as it needs to list/update them.
  4. Add featureFlag CERT_ROTATION so that this feature is behind FF.
  5. Add Ratify_NAME env var to Ratify as cert-rotator needs it for validation and cert generation.
  6. Updated generate-tls-certs.sh so that the generated certs can pass the cert-rotator validation.
  7. Add blocker for tlsCertWatcher so that cert-rotator will not start until certWatcher is ready.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Fixes #834

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Helm Chart Change (any edit/addition/update that is necessary for changes merged to the main branch)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

Adding a new e2e test which will be provided an expiring cert in deployment, which will trigger a cert rotatation immediately.

Checklist:

  • Does the affected code have corresponding tests?
  • Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have appropriate license header?

Post Merge Requirements

  • MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Patch coverage: 18.18% and project coverage change: -0.48 ⚠️

Comparison is base (c13f848) 55.03% compared to head (7cab6f8) 54.55%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #869      +/-   ##
==========================================
- Coverage   55.03%   54.55%   -0.48%     
==========================================
  Files          80       81       +1     
  Lines        4739     4797      +58     
==========================================
+ Hits         2608     2617       +9     
- Misses       1848     1896      +48     
- Partials      283      284       +1     
Impacted Files Coverage Δ
cmd/ratify/cmd/serve.go 43.75% <0.00%> (-0.94%) ⬇️
httpserver/server.go 6.12% <0.00%> (-0.13%) ⬇️
pkg/featureflag/featureflag.go 100.00% <ø> (ø)
pkg/manager/manager.go 3.59% <0.00%> (-1.25%) ⬇️
pkg/utils/podinfo.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@binbin-li binbin-li force-pushed the add-cert-controller branch 3 times, most recently from 0cc0f2f to 9d80500 Compare June 5, 2023 15:01
charts/ratify/values.yaml Outdated Show resolved Hide resolved
@binbin-li binbin-li force-pushed the add-cert-controller branch 3 times, most recently from d6b5213 to c69efa3 Compare June 6, 2023 14:42
@susanshi
Copy link
Collaborator

susanshi commented Jun 7, 2023

As discussed offline, lets validate if Ratify throws error on startup when customer provides a expired cert.

@binbin-li binbin-li force-pushed the add-cert-controller branch 2 times, most recently from 56ce934 to e7f0521 Compare June 7, 2023 01:37
@binbin-li binbin-li changed the title feat: add cert rotator [WIP] feat: add cert rotator Jun 7, 2023
@binbin-li binbin-li force-pushed the add-cert-controller branch 9 times, most recently from 25ca8c8 to ee401fc Compare June 12, 2023 07:12
Copy link
Collaborator

@susanshi susanshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks great Binbin, could you provide more details on logs and test coverage?

Have we covered cases for:

  • rotation customer provided cert
  • rotation for ratify auto generated cert
  • Are there any customer provided cert that we don't support rotation that we should document?
  • Once a cert is rotated by the rotator, what does ratify log look like?
  • Have we validated ratify 's configuration matrix with cert rotater disabled?
    thanks!

docs/reference/usage.md Outdated Show resolved Hide resolved
pkg/featureflag/featureflag.go Show resolved Hide resolved
pkg/manager/manager.go Outdated Show resolved Hide resolved
@binbin-li
Copy link
Collaborator Author

PR looks great Binbin, could you provide more details on logs and test coverage?

Have we covered cases for:

  • rotation customer provided cert
  • rotation for ratify auto generated cert
  • Are there any customer provided cert that we don't support rotation that we should document?
  • Once a cert is rotated by the rotator, what does ratify log look like?
  • Have we validated ratify 's configuration matrix with cert rotater disabled?
    thanks!
  1. For customer provided/ratify generated certs, cert-rotator will behave in the same way. If they're invalid, rotator will refresh invalid certs, and generate a log like:
    image
    And if it's valid, it will generate a log like:
    image

  2. Actually if customer provided any incompatible/invalid certs, cert-rotator will just auto-rotate it. But I can add some docs on the cert generated by cert-rotator so that customers can follow.

  3. I'll add a basic notation test with rotator disabled.

@binbin-li binbin-li force-pushed the add-cert-controller branch 13 times, most recently from 13915d1 to 2a325ee Compare June 15, 2023 14:33
}

@test "cert rotator test" {
helm uninstall ratify --namespace gatekeeper-system
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to use helm upgrade which works without providing --set featureFlags.RATIFY_CERT_ROTATION=true flag. But it always failed to deploy Ratify pod with it. I checked logs which shows the pod is already up but helm regards it as not ready. What's more interesting is that the same helm upgrade command works on my local machine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh that's interesting. Maybe there's another resource other than the pod that is failing to be upgraded due some cpu/mem limitation? Either way I'm personally ok with your solution.

@binbin-li binbin-li enabled auto-merge (squash) June 15, 2023 15:06
Signed-off-by: Binbin Li <libinbin@microsoft.com>
Copy link
Collaborator

@akashsinghal akashsinghal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@binbin-li binbin-li merged commit 207af0c into ratify-project:main Jun 16, 2023
12 of 14 checks passed
bspaans pushed a commit to bspaans/ratify that referenced this pull request Oct 17, 2023
Signed-off-by: Binbin Li <libinbin@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support TLS cert rotation in Ratify.
3 participants