Skip to content

feat: reject revoked mTLS client certificates from local CRL files - #4197

Open
pardeep-singh wants to merge 1 commit into
maziyarpanahi:masterfrom
pardeep-singh:pardeep/issue3922-mtls-local-crl
Open

pardeep-singh wants to merge 1 commit into
maziyarpanahi:masterfrom
pardeep-singh:pardeep/issue3922-mtls-local-crl

Conversation

@pardeep-singh

Copy link
Copy Markdown
Contributor

Pull Request

Description

Fixes #3922.

On master, openmed/service/mtls.py checks the client chain against the CA
bundle but takes no revocation input. A compromised client certificate keeps
working until it expires. #832 left out online CRL and OCSP checks, and #3580
leaves out CRL hosting, so nothing handled an offline, operator-supplied CRL.

This PR adds local CRL files to REST mTLS. OpenMed verifies each CRL's
signature against the configured CA bundle at startup. On every request it
rejects revoked certificates and checks CRL freshness before principal mapping
runs. Invalid or stale CRLs fail closed in production and are reported in the
dev and test profiles. Errors and logs use only fixed codes.

Type of Change

  • 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)
  • Documentation update
  • Code refactoring
  • Performance improvement
  • Test addition/improvement

Changes Made

  • openmed/service/mtls.py: Add LocalCRLChecker, which loads PEM or DER
    CRL files, verifies each signature against a bundle CA with a matching
    subject and cRLSign usage, and checks every certificate in the verified
    chain by serial number. Add three settings:
    OPENMED_SERVICE_MTLS_CRL_FILES, OPENMED_SERVICE_MTLS_CRL_GRACE_SECONDS
    and OPENMED_SERVICE_MTLS_CRL_FAIL_CLOSED. ServiceMTLS runs the check
    inside chain verification, before _principal_for. Add the
    client_certificate_revoked (401), crl_stale (503) and crl_invalid
    codes. MTLSAuthenticationError now carries a status code (default 401).
    verify_client_certificate_chain takes an optional crl_checker, so the
    gRPC listener can reuse the same check later.
  • tests/unit/service/test_mtls_crl.py: 40 new tests. They build a
    synthetic CA, client certificates and CRLs at test time.
  • docs/serving/mtls.md: Document the settings, the error codes, profile
    behavior, what "reported" means, and how to produce and rotate a CRL with
    OpenSSL (placeholder paths).
  • CHANGELOG.md: Add an Unreleased Security entry.

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this change with different models/inputs

New tests cover:

  • a revoked synthetic certificate is rejected with client_certificate_revoked
    and an unrevoked one still authenticates, both directly, through the REST
    app, and through the trusted-proxy header;
  • a revoked intermediate CA rejects its clients;
  • PEM and DER CRLs, and multiple CRL files;
  • crl_invalid for a CRL signed by the wrong key, a CRL from a CA outside the
    bundle, a corrupted signature, a malformed file, a delta CRL and a missing
    file;
  • crl_stale beyond the grace period, acceptance within it, and a CRL that
    goes stale while the service runs (injected clock);
  • production refuses to start with an invalid CRL and returns 503 for a stale
    one; dev and test log one value-free warning and let the request
    proceed; a revoked certificate is still rejected in report-only mode;
  • the principal mapper is never called for a revoked certificate;
  • profile defaults, and rejecting OPENMED_SERVICE_MTLS_CRL_FAIL_CLOSED=false
    outside dev and test;
  • negative controls: the subject CN, SAN, full subject, serial number (decimal,
    hex, upper-case hex and colon-separated hex) and the CRL directory path never
    appear in responses, headers, exception text or captured logs.

Before the fix, the new test file failed at collection with an ImportError
for the new symbols (0 of 40 ran). After the fix, all 40 pass. With the
revocation call removed from the code path, 14 of the 40 fail, so the tests
exercise the check itself.

PYTHONPATH=$PWD .venv/bin/python -m pytest tests/unit/service/test_mtls_crl.py -q
# before: 1 error during collection (ImportError)
# after:  40 passed

PYTHONPATH=$PWD .venv/bin/python -m pytest tests/unit/service/test_mtls_crl.py tests/unit/service/test_mtls_auth.py -q
# 49 passed

PYTHONPATH=$PWD .venv/bin/python -m pytest tests/unit/service -q
# 973 passed, 5 skipped (onnx not installed)

PYTHONPATH=$PWD .venv/bin/python -m pytest tests/unit/test_docs_publication.py tests/unit/test_brand_system.py tests/unit/test_pages_manifest.py tests/unit/test_doctests.py tests/unit/test_troubleshooting_docs.py -q
# 178 passed, 2 skipped

ruff check . && ruff format --check .
# All checks passed! / 2885 files already formatted

PYTHONPATH=$PWD .venv/bin/python scripts/security/check_effect_paths.py --check
# effect_paths_verified

Full suite (pytest tests/ -q): 29576 passed, 185 skipped, 2 failed,
18 errors. All 20 failures and errors are in
tests/unit/security/test_vulnerability_scan_gate.py. That file fails the same
way (2 failed, 23 passed, 18 errors) on a clean export of the base commit, so
these failures already exist on master and are not caused by this change.

Documentation

  • I have updated the documentation accordingly
  • I have added docstrings to new functions/classes
  • I have updated the CHANGELOG.md

Code Quality

  • I ran make format, make lint, and make format-check
  • For Swift/OpenMedKit changes, I ran make format-swift and make lint-swift
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

Dependencies

  • I have not added any new dependencies
  • OR I have added new dependencies and they are justified because: ____

mtls.py already imports cryptography. The CRL code uses the same package.

Checklist

Notes

  • Where the check runs. The check runs in the application, after
    cryptography verifies the chain and before principal mapping. It does not
    use ssl.VERIFY_CRL_CHECK_LEAF. OpenMed does not own the TLS context:
    certificates arrive through the ASGI TLS extension or a trusted proxy
    header, and Uvicorn has no CRL flag. One check covers both paths.
  • Production detection. This follows OPENMED_PROFILE, which defaults to
    prod, the same way auth.py and runtime.py read it. Only dev and
    test are report-only. prod, fast, low_resource and custom profiles
    fail closed. OPENMED_SERVICE_MTLS_CRL_FAIL_CLOSED=true opts a development
    profile into fail-closed. Setting it to false outside dev and test is a
    startup error, so production has no fail-open switch.
  • Invalid CRL in production refuses startup. An invalid CRL file raises
    crl_invalid when the app is built, like an invalid CA bundle does today.
    The error names only the file's 1-based position, never the path. Freshness
    changes over time, so crl_stale is checked per request and returns 503.
    The client cannot fix a stale CRL, and a 503 lets load balancers and alerts
    notice it. A revoked certificate returns 401.
  • What "reported" means. In dev and test, each problem produces one
    WARNING on the openmed.service.mtls logger. The record holds only the
    code (and the file position for an invalid file), plus an openmed_code
    attribute. Invalid files are skipped. Stale CRLs are still used. Requests
    proceed. Revoked certificates are always rejected.
  • Scope of the check. Each certificate in the verified chain is checked
    against CRLs signed by its issuer. A certificate whose issuer has no
    configured CRL is not checked. Freshness is checked only for the CRLs that
    apply to the chain, so a stale CRL for one CA does not block clients of
    another CA.
  • Extra hardening. Delta CRLs, CRLs without nextUpdate, PEM files with
    more than one CRL block, and files over 64 MiB are rejected as
    crl_invalid. A delta CRL lists only recent changes, so treating it as a
    full list could accept certificates revoked in the base CRL. CRL load errors
    are raised from None, so tracebacks do not carry OS error text that names
    the path.
  • Out of scope. Online OCSP, CRL distribution-point fetching and reload
    without restart are not included, as the issue boundary states. A future
    lastUpdate time is not rejected.
  • mkdocs is not installed in my environment, so I did not run a strict docs
    build. The new in-page anchor uses the default slug for its heading.

Related Issues

Closes #3922
Related to #832, #3580

This branch has not been deployed

No deployments
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.

Reject revoked mTLS client certificates from an operator-supplied local CRL

1 participant