Skip to content

Conversation

Gldywn
Copy link

@Gldywn Gldywn commented Aug 15, 2025

❗ Important Announcements

Click here for more details:

⚠️ Please Note: We do not accept all types of pull requests, and we want to ensure we don't waste your time. Before submitting, make sure you have read our pull request guidelines: Pull Request Rules

🚫 Please Avoid Unnecessary Pinging of Maintainers

We kindly ask you to refrain from pinging maintainers unless absolutely necessary. Pings are for critical/urgent pull requests that require immediate attention.

📋 Overview

  • What problem does this pull request address?

    • Uptime Kuma currently checks certificate expiry but not revocation. This is largely because Node.js default HTTPS/TLS stack does not perform revocation validation (no OCSP/CRL by default), so revocation signals were historically missing at runtime. This means revoked certs could appear “healthy” until clients fail. This PR is a first, incremental step toward revocation awareness as requested in #1254.
  • What features or functionality does this pull request introduce or enhance?

    • This PR adds certificate revocation checks for HTTPS monitors (when ignoreTls=false) using the new dependency hardened-https-agent.
    • The HardenedHttpsAgent:
        1. Validates OCSP stapled responses if provided by the server.
        1. If no stapling is present, extracts the OCSP responder URL (AIA) from the certificate and performs a direct OCSP HTTP query, validating signatures in both cases.
      • The applied policy is OCSP “mixed” with failHard=false, meaning if OCSP is unavailable (no stapling, no responder URL, or fetch errors), the monitor does not fail.
      • It also works through configured proxies (HTTP/HTTPS/SOCKS5), via HardenedHttpsValidationKit instead of HardenedHttpsAgent.
        • ⚠️ Note: OCSP direct lookups currently bypass the configured proxy (auxiliary AIA HTTP fetch). In egress‑restricted networks this may fail but with OCSP “mixed” and failHard=false, monitors do not flip DOWN.
          • More at “Proxy behavior and OCSP direct lookups” below.

Roadmap

  • Step 1 (this PR): Introduce hardened-https-agent and enable OCSP validation with the Mixed policy (failHard=false).
  • Step 2 (next PRs): First, add classic CRL support to the upstream library hardened-https-agent (automatic AIA CRL extraction, fetch, and query). Then open a PR here to bump the dependency and enable CRL checks in Kuma.
  • Step 3 (needs discussion): Add a new Settings screen ("Certificate Revocation Policy") to configure desired behavior: fail‑hard on/off, OCSP mixed vs stapling vs direct, classic CRL, CRLSet/CRLite, custom CA, etc.

Proxy behavior and OCSP direct lookups

When a proxy is enabled, the HardenedHttpsValidationKit attaches to the same agent used by the monitor (HttpProxyAgent/HttpsProxyAgent/SocksProxyAgent), so the main HTTPS connection benefits from revocation checks even when proxied.
When the server does not include OCSP stapling, a secondary AIA OCSP HTTP request is performed via a basic fetch and currently does not use the proxy. In locked‑down egress environments, that fetch may be blocked; this is acceptable for this first step because the default policy is permissive (OCSP “mixed”, failHard=false) to avoid false negatives.

  • As an optional mitigation (if preferred by maintainers), we can temporarily disable hardened validation when a proxy is attached until proxy‑aware OCSP direct requests land.
  • The planned improvement is to add proxy support to the OCSP direct flow by passing the same proxy agent to the OCSP client upstream and enabling it here later.

🛠️ Type of change

  • 🐛 Bugfix (a non-breaking change that resolves an issue)
  • ✨ New feature (a non-breaking change that adds new functionality)
  • ⚠️ Breaking change (a fix or feature that alters existing functionality in a way that could cause issues)
  • 🎨 User Interface (UI) updates
  • 📄 New Documentation (addition of new documentation)
  • 📄 Documentation Update (modification of existing documentation)
  • 📄 Documentation Update Required (the change requires updates to related documentation)
  • 🔧 Other (please specify)

📄 Checklist

  • 🔍 My code adheres to the style guidelines of this project.
  • 🦿 I have indicated where (if any) I used an LLM for the contributions
  • ✅ I ran ESLint and other code linters for modified files.
  • 🛠️ I have reviewed and tested my code.
  • 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • ⚠️ My changes generate no new warnings.
  • 🤖 My code needed automated testing. I have added them (this is an optional task).
  • 📄 Documentation updates are included (if applicable).
  • 🔒 I have considered potential security impacts and mitigated risks.
  • 🧰 Dependency updates are listed and explained.
  • 📚 I have read and understood the Pull Request guidelines.

📷 Screenshots or Visual Changes

Dashboard Event

I've tested several live domains and connection paths (direct HTTP(S), local HTTP proxy and SOCKS5 proxy) to manually validate the validation behavior. In the screenshots you can see that revoked.badssl.com appears UP even though it's obviously revoked, that's because it's revoked via CRL and as said before this PR only implements OCSP-based revocation.
PS: I collected test targets from https://crt.sh/test-websites.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

I am reviewing this despite it being a draft.
The code looks good and I would merge it as is 😊

Tests for this would be amazing (this way we can see if the planned PRs/features break something accidentally).
Because of where this sits, the simplest way is likely an end to end test 🤔

This is not a must though.

@Gldywn
Copy link
Author

Gldywn commented Aug 25, 2025

Thanks for the review! I had kept it in draft on purpose until I got a first round of feedback 😅

Tests for this would be amazing (this way we can see if the planned PRs/features break something accidentally).
Because of where this sits, the simplest way is likely an end to end test 🤔

Absolutely, I’ll add an E2E test suite for this PR that rely on stable, real-world targets (like badssl.com and a few known-good hosts). I had already planned to add classic CRL support to hardened-https-agent in the coming days, so once that lands, I’ll bump the dependency here, and update this PR accordingly (if it’s ok for you to have this here as well) 👍

@CommanderStorm
Copy link
Collaborator

Shure. What works for you

@Gldywn Gldywn force-pushed the feature/hardened-https-ocsp branch from d41f683 to 7197221 Compare September 5, 2025 15:52
@Gldywn
Copy link
Author

Gldywn commented Sep 5, 2025

Hey @CommanderStorm, just pushed the latest changes to this PR.
For now, I’ve disabled attaching the HardenedHttpsValidationKit whenever a proxy is enabled. Even if the risk is low, the kit currently makes auxiliary validation requests (e.g., OCSP direct/AIA lookups) that may not use the same proxied networking path, and in tightly locked‑down enterprise networks that can be blocked or causing noisy logs. My plan is to add a custom fetcher hook to the kit so these lookups go through the same proxy/agent, then re‑enable hardened validation for proxied connections.

I also added a full end‑to‑end test suite to lock in the TLS behaviors introduced here.

This is ready for (re)review, if it looks good, feel free to merge, and I’ll follow up with a separate PR to add classic CRL support and the proxy‑aware fetcher so we can safely re‑enable the validation kit with proxies.

@Gldywn Gldywn marked this pull request as ready for review September 5, 2025 16:04
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.

Dealing with revoked certificates
2 participants