Skip to content

[deploy] Fix and harden cert handling - #4387

Merged
imnasnainaec merged 7 commits into
masterfrom
cert-expiration-check
Oct 13, 2026
Merged

imnasnainaec merged 7 commits into
masterfrom
cert-expiration-check

Conversation

@imnasnainaec

@imnasnainaec imnasnainaec commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

The NUC and laptop certs in S3 could go stale without anyone noticing. The fixed 60-day renewal windows also break when Let's Encrypt moves to 64-day certs (Feb 2027) and 45-day certs (Feb 2028).

cert-manager (prod)          renews NUC certs at 92% of lifetime left
└─ TLS secret
   └─ monitor.py             pushes changed TLS secrets to S3
      └─ S3                  cert_check.yml alerts below 91% left
         └─ update_cert.py   NUC/laptop pulls below 88% left, only if newer
            └─ TLS secret    applied in place

Thresholds

Step Lifetime left Setting
Server renews 92% certRenewBeforePercentage in cert-proxy-server
Check alerts < 91% cert_check.yml
Client pulls < 88% certRenewBeforePercentage in cert-proxy-client
  • 92% keeps renewals at about 2 per hostname per week with 45-day certs, under Let's Encrypt's limit of 5.
  • The alert fires before clients start polling S3 for a renewal that hasn't landed.

cert_check.yml (new workflow)

  • Runs every 12 hours, or on demand.
  • For each NUC cert and the local.thecombine.app cert in S3, it fails if:
    • key.pem doesn't match cert.pem, or
    • less than 91% of the cert's lifetime is left.
  • The cert list mirrors combineCertProxyList in prod.yaml.

monitor.py (cert-proxy-server)

  • Watch:
    • Only kubernetes.io/tls secrets, so Helm release secrets no longer pass through the watch.
    • Ends every 30 minutes and has connect and read timeouts.
    • Restarts 30 seconds after it ends or fails, replaying every secret.
  • Push:
    • Skipped if the secret hasn't changed since its last successful push.
    • 60-second timeout and up to 5 attempts. A failed attempt logs its exit code, e.g. -9 for an OOM kill.
  • AwsBackup.push takes check_results and timeout.

update_cert.py (cert-proxy-client)

 if cert needs renewal and internet is reachable
-  pull cert.pem and key.pem
-  delete secret
-  create secret
+  pull cert.pem
+  if it doesn't expire later than the current cert
+    stop
+  pull key.pem
+  apply secret in place
  • "Needs renewal" means less than CERT_RENEW_BEFORE_PERCENTAGE percent of the lifetime is left. It replaces the fixed CERT_RENEW_BEFORE days.
    • A current cert that is missing or fails to parse also needs renewal.
  • With an in-place apply, the secret is never missing.
  • The manifest goes to kubectl apply -f - on stdin, so it's never written to disk.
    • run_cmd takes a new optional stdin_text.
  • If renewal stalls, the client keeps checking S3 but leaves its secret alone.

Charts

  • cert-proxy-server: renew-before: 1440h → renew-before-percentage: "92".
    • Needs cert-manager v1.16 or later. Prod has v1.18.1.
  • cert-proxy-client: env var CERT_RENEW_BEFORE → CERT_RENEW_BEFORE_PERCENTAGE.
    • The unused cert_renew_before value is removed.

Evidence

  • flake8, isort, black and mypy pass on maintenance/scripts.

  • helm template renders:

    • cert-manager.io/renew-before-percentage: "92"
    • CERT_RENEW_BEFORE_PERCENTAGE: "88"
  • The threshold and key checks from cert_check.yml, run against generated 90-day and 45-day certs:

    fresh cert             pass
    8% of lifetime gone    pass
    10% of lifetime gone   fail
    matching key           pass
    different key          fail
    
  • Not yet run:

    • cert_check.yml: workflow_dispatch needs the workflow on master.
    • update_cert.py against a cluster.

Merge Danger

Door: two-way

Every change reverts cleanly, and certs already issued are unaffected. One constraint: the combine_maint image and the cert-proxy-client chart must ship together. A new image with an old chart fails with KeyError: 'CERT_RENEW_BEFORE_PERCENTAGE' on every run.

Blast Radius: NUC/laptop TLS

  • Let's Encrypt issues NUC certs more often: every 7.2 days per hostname now, 3.6 days once 45-day certs arrive.
  • v3.0.0 laptops keep the fixed 60-day threshold.
    • No change while certs last 90 days.
    • With 64-day certs, the S3 cert drops below 60 days for about 22% of each renewal cycle. During that time these laptops pull and recreate their secret every 5 minutes.
    • Upgrade them before Feb 2027.
  • The combine-cert-proxy OOM restarts are fixed by the memory limits in Raise memory limit for the cert update jobs #4350, not here. This PR reduces the memory monitor.py uses.

🤖 Generated with Claude Code

This change is Reviewable

Summary by CodeRabbit

  • New Features

    • Certificate renewal timing is now based on the percentage of certificate lifetime remaining, with separate thresholds for certificate clients and servers.
    • Automated checks now monitor certificate locations for download failures, mismatched keys, and certificates nearing expiration.
  • Bug Fixes

    • Certificate uploads retry after failures, and unchanged certificates are not uploaded again.
    • Certificate updates stop when the available certificate would expire no later than the current one.

imnasnainaec and others added 4 commits October 5, 2026 14:43
Runs daily and fails if any NUC or local.thecombine.app cert stored in
S3 expires within 50 days. The cert-proxy-server renews certs with 60
days left, so this catches a stalled renewal before the NUCs and
laptops are left with an expired cert.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The watch on the TLS secrets had no timeout, so when its connection died
without being closed, monitor.py waited forever and never pushed renewed
certs to S3. The watch now ends every 30 minutes, has a read timeout to
detect a dead connection, and is restarted after any error. Each new
watch re-pushes all the monitored secrets.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A dropped connection to S3 failed a push and exited monitor.py, which
could leave a new key.pem in S3 next to an old cert.pem. Each push now
has a 60-second timeout and up to 5 attempts. If all of them fail, the
watch is restarted, which re-pushes all the monitored secrets.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Let's Encrypt is moving to 64-day certs in 2027 and 45-day certs in 2028,
which breaks the fixed 60-day renewal thresholds on both ends.

The cert-proxy-server now renews NUC certs with 92% of their lifetime
left (renew-before-percentage), so the certs in S3 stay fresh for NUCs
and laptops that go offline. update_cert.py checks S3 when 88% of its
cert's lifetime is left, replaces the secret only with a cert that
expires later, and applies it in place instead of deleting and
recreating it, so the secret is never missing.

monitor.py only watches TLS secrets, skips pushes of unchanged secrets,
logs the exit code of a failed push, and sets separate connect and read
timeouts on the watch.

The S3 cert check runs every 12 hours. It fails if a key does not match
its cert or if a cert has less than 91% of its lifetime left, which
warns of a stalled renewal before clients start polling S3.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@imnasnainaec imnasnainaec self-assigned this Oct 9, 2026
@imnasnainaec imnasnainaec added the 🟥High High-priority PR: please review this asap! label Oct 9, 2026
@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

📝 Walkthrough

Walkthrough

The changes use percentage-based certificate renewal thresholds, update certificate validation and application, add retries and duplicate-push checks to certificate monitoring, and introduce scheduled S3 certificate checks.

Changes

Certificate renewal

Layer / File(s) Summary
Configure percentage-based renewal
deploy/helm/cert-proxy-client/templates/*, deploy/helm/cert-proxy-client/values.yaml, deploy/helm/cert-proxy-server/templates/ingress-nuc.yaml, deploy/helm/cert-proxy-server/values.yaml
Client values and templates now use an 88% remaining-lifetime threshold. Server values and the ingress annotation now use a 92% threshold.
Evaluate and apply certificate updates
maintenance/scripts/update_cert.py
The update script evaluates remaining certificate lifetime against the configured percentage. It validates the S3 certificate before downloading its key and does not replace the current secret when the S3 certificate expires at or before the current one.

Certificate monitoring and push handling

Layer / File(s) Summary
Pass command options and retry AWS pushes
maintenance/scripts/maint_utils.py, maintenance/scripts/aws_backup.py, maintenance/scripts/monitor.py
run_cmd accepts text for standard input. AwsBackup.push forwards result-checking and timeout options. TlsSecret.push retries failed or timed-out pushes up to five times.
Restart watches and avoid duplicate pushes
maintenance/scripts/monitor.py
The monitor restarts failed or completed watches, applies watch and request timeouts, filters events to TLS secrets, and skips pushes when a secret’s key and certificate data match the last successfully pushed digest.

Scheduled certificate checks

Layer / File(s) Summary
Download and validate certificate pairs
.github/workflows/cert_check.yml
A scheduled or manually dispatched workflow checks four S3 certificate and key pairs. It reports download failures, mismatched public keys, and certificates with less than 91% of their lifetime remaining. The job fails if any check fails.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant update_cert.py
  participant Kubernetes
  participant S3
  update_cert.py->>Kubernetes: Read current TLS secret
  update_cert.py->>S3: Fetch certificate data
  update_cert.py->>update_cert.py: Validate validity and compare expiration
  update_cert.py->>S3: Fetch key data when the S3 certificate is newer
  update_cert.py->>Kubernetes: Apply TLS secret YAML
Loading

Merge Risk: 🟡 Moderate · up to cfc99

The certificate update Job can exit instead of renewing when the stored certificate is corrupt. It can also install a certificate that is not yet valid, which breaks TLS until that certificate's start date. Separately, the monitor can send rapid repeated requests to the API server when a watch closes early. These issues should be fixed before merge.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 4 files. (7 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main changes to certificate monitoring, renewal, retry, and validation behavior.

Full details: Docstring Coverage

Explanation

Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 4 files. (7 skipped: 7 unsupported.)



  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added deployment python github_actions Pull requests that update GitHub Actions code labels Oct 9, 2026
@imnasnainaec imnasnainaec changed the title Fix and harden cert handling [deploy] Fix and harden cert handling Oct 9, 2026
Comment thread maintenance/scripts/monitor.py Dismissed
Comment thread maintenance/scripts/update_cert.py Fixed
@codecov

codecov Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.26%. Comparing base (3f735c1) to head (d98757b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4387   +/-   ##
=======================================
  Coverage   76.26%   76.26%           
=======================================
  Files         305      305           
  Lines       11452    11452           
  Branches     1409     1409           
=======================================
  Hits         8734     8734           
  Misses       2324     2324           
  Partials      394      394           
Flag Coverage Δ
backend 87.55% <ø> (ø)
frontend 67.09% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

imnasnainaec and others added 2 commits October 9, 2026 14:36
The replay of failed pushes depends on each watch starting without a
resource version, which was not obvious from the comment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
update_cert.py wrote the generated secret manifest, which contains the
TLS private key, to a temp file before applying it. It now sends the
manifest to kubectl apply on stdin, using a new stdin_text parameter on
run_cmd.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@imnasnainaec
imnasnainaec marked this pull request as ready for review October 9, 2026 19:11
coderabbitai[bot]

This comment was marked as resolved.

monitor.py now waits before every new watch, not only after an error,
so a watch that keeps closing cleanly cannot cause a tight request loop.

update_cert.py now treats a current cert that fails to parse as needing
renewal instead of exiting, so a malformed secret gets replaced.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@jasonleenaylor jasonleenaylor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. The key-before-cert push order, the newer-expiry check and the in-place apply fit together well.

Comment thread .github/workflows/cert_check.yml
@imnasnainaec
imnasnainaec merged commit 1f1c80d into master Oct 13, 2026
24 of 26 checks passed
@imnasnainaec
imnasnainaec deleted the cert-expiration-check branch October 13, 2026 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment github_actions Pull requests that update GitHub Actions code 🟥High High-priority PR: please review this asap! python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants