Skip to content

Remove pyopenssl usage from certdir (SYN-7712)#4867

Merged
vEpiphyte merged 14 commits intomasterfrom
epiphyte/SYN-7712/remove_pyopenssl
Apr 9, 2026
Merged

Remove pyopenssl usage from certdir (SYN-7712)#4867
vEpiphyte merged 14 commits intomasterfrom
epiphyte/SYN-7712/remove_pyopenssl

Conversation

@vEpiphyte
Copy link
Copy Markdown
Contributor

@vEpiphyte vEpiphyte commented Apr 7, 2026

Summary

Remove the pyOpenSSL dependency and replace all certificate chain verification with a custom _verifyChain() implementation using the cryptography library directly. The cryptography dependency has been updated to >=46.0.7,<47.0.0.

  • Replaced pyOpenSSL X509Store/X509StoreContext usage in Crl._verify(), valCodeCert(), and valUserCert() with _verifyChain() and _verifyCertSignature()
  • _verifyCertSignature() uses Certificate.verify_directly_issued_by() for combined issuer name and signature verification
  • Removed the vestigial CertDir.valUserCert() API (no production callers)
  • Added _getCertName() helper to guard against certificates missing a Common Name subject field
  • Added comprehensive adversarial and edge-case test coverage (40 tests, up from 13)

Verification improvements over pyOpenSSL

  • Path length constraints enforced at every level of the chain per RFC 5280
  • Maximum chain depth limited to 8 to prevent excessive recursion
  • Cycle detection via SHA256 fingerprint tracking (pyOpenSSL relied on depth limits)
  • BasicConstraints required on all certificates in the chain, not just issuers
  • Root CA validity dates checked -- issuer time window is checked unconditionally at every level, including self-signed roots (pyOpenSSL skipped root CA validity)
  • Distinct error messages for cert vs issuer failures, including the certificate subject for diagnostics
  • Unhandled critical extensions rejected per RFC 5280 -- certificates with critical extensions not processed by _verifyChain are rejected. Handled critical extensions: BasicConstraints, ExtendedKeyUsage.
  • Lenient CRL checking -- missing CRLs for a given issuer are skipped rather than failing the entire chain. This fixes a bug where independent CA chains sharing a certdir would break if only some chains had CRLs.
  • Signature-based issuer matching via verify_directly_issued_by -- _verifyChain matches issuers by both subject name AND cryptographic signature in one operation, improving robustness when multiple CAs share the same CN.
  • RSA-only key policy enforced in _verifyCertSignature() with a clear error for unsupported key types
  • Empty subject certificates rejected with BadCertBytes instead of raw IndexError
  • Deceptive self-signed certs rejected -- a cert signed by its own key but with issuer != subject is not treated as a root and fails chain verification

Files changed

File Description
synapse/lib/certdir.py Core migration: remove pyOpenSSL, add _verifyCertSignature(), _verifyChain(), _getCertName(), rewrite Crl._verify() and valCodeCert(), remove valUserCert() and _unpackContextError()
synapse/tests/test_lib_certdir.py Remove pyOpenSSL usage, rewrite basic_assertions(), add 27 new adversarial/edge-case tests
synapse/tests/test_tools_docker_validate.py Add test for checkCRL with invalid data
synapse/tools/docker/validate.py Update stale pyopenssl comment
requirements.txt Remove pyOpenSSL, update cryptography to >=46.0.7,<47.0.0
pyproject.toml Same dependency changes

Test plan

  • python -m pytest synapse/tests/test_lib_certdir.py -v -- 38 tests pass
  • python -m pytest synapse/tests/test_tools_docker_validate.py -v
  • 100% patch coverage.
  • Verify no pyOpenSSL remnants.
  • Verify the removal of valUserCert has no regressions.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.73%. Comparing base (2989d72) to head (a7f4205).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4867   +/-   ##
=======================================
  Coverage   97.72%   97.73%           
=======================================
  Files         298      298           
  Lines       62831    62865   +34     
=======================================
+ Hits        61404    61441   +37     
+ Misses       1427     1424    -3     
Flag Coverage Δ
linux 97.66% <100.00%> (+<0.01%) ⬆️
linux_replay 93.44% <100.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Copy Markdown
Contributor

@invisig0th invisig0th left a comment

Choose a reason for hiding this comment

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

Looks really solid. A couple of style nits ;)

Comment thread synapse/lib/certdir.py Outdated
Comment thread synapse/lib/certdir.py
Comment thread synapse/lib/certdir.py Outdated
Comment thread synapse/tools/docker/validate.py Outdated
…ince a certificate without a CN would raise an IndexError. Update _verifyChain error messages to include the subject field for certificates presenting issues.
MichaelSquires
MichaelSquires previously approved these changes Apr 8, 2026
MichaelSquires
MichaelSquires previously approved these changes Apr 8, 2026
Cisphyx
Cisphyx previously approved these changes Apr 8, 2026
Comment thread synapse/lib/certdir.py Outdated
@vEpiphyte vEpiphyte requested a review from MichaelSquires April 9, 2026 13:55
@vEpiphyte vEpiphyte requested a review from Cisphyx April 9, 2026 13:55
@vEpiphyte vEpiphyte added this to the v2.238.0 milestone Apr 9, 2026
@vEpiphyte vEpiphyte merged commit 74b5dd1 into master Apr 9, 2026
6 checks passed
@vEpiphyte vEpiphyte deleted the epiphyte/SYN-7712/remove_pyopenssl branch April 9, 2026 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants