Reality scanner: target-derived SNI + hardening (follow-up to #695) - #700
Merged
ImMohammad20000 merged 4 commits intoJul 16, 2026
Merged
Conversation
added 2 commits
July 15, 2026 14:26
The scanner no longer takes a separate SNI and no longer pulls the inbound's first serverName. The SNI is derived from the target itself: - hostname target -> the hostname is its own SNI, validated against it - bare IP target -> handshake, discover the domain from the presented certificate (CN or first non-wildcard SAN), then re-validate the cert against that discovered domain; the result reports it as discovered from the IP Removes RealityScanRequest.sni and the serverNames->SNI dialog wiring; adds RealityScanResult.sni_discovered. Multi-target scanning is kept (each target derives its own SNI). Adds sniLabel/sniDiscovered locales.
…emaphore) - Bound the TLS handshake with an absolute deadline (non-blocking socket + select loop) so a slow-drip peer can no longer keep do_handshake() running forever, and run scans on a dedicated ThreadPoolExecutor so a lingering scan thread can never starve the event loop's shared pool (getaddrinfo / other to_thread work). - Key the concurrency semaphore per running event loop via a WeakKeyDictionary, removing the cross-loop "bound to a different event loop" RuntimeError. - Preserve the certificate reason when the permissive fallback also fails, handle UnicodeError from over-long DNS labels gracefully instead of a 502, and gate the discovered SNI through a hostname sanity check.
WalkthroughThe REALITY scanner removes caller-provided SNI overrides, derives SNI from target hosts or certificates, performs deadline-based TLS probing, exposes discovery status in results, and displays discovered SNI in the dashboard with localized labels. ChangesREALITY SNI discovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RealityScanDialog
participant scanRealityTarget
participant _scan_sync
participant _tls_probe
participant Certificate
RealityScanDialog->>scanRealityTarget: submit target and timeout
scanRealityTarget->>_scan_sync: run scan in bounded executor
_scan_sync->>_tls_probe: probe target TLS
_tls_probe->>Certificate: discover usable SNI when needed
Certificate-->>_tls_probe: return hostname
_tls_probe-->>_scan_sync: return TLS and SNI state
_scan_sync-->>scanRealityTarget: return scan result
scanRealityTarget-->>RealityScanDialog: return result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/utils/reality_scan.py`:
- Around line 271-281: Update the certificate identity extraction logic to
inspect DNS names from the SUBJECT_ALTERNATIVE_NAME extension before evaluating
the subject CN. Return the first valid non-wildcard hostname from the SANs, and
only fall back to the existing CN validation when no usable DNS SAN identity
exists; adjust tests in the related unit test module to cover SAN precedence and
CN fallback.
- Around line 674-675: Update the feasibility calculation in _group_probe to
require an explicitly confirmed X25519-family group, rather than treating
unknown x25519, post_quantum, and curve values as acceptable. Ensure
result["feasible"] is false when group probing leaves these fields unknown,
while preserving the existing TLS 1.3, HTTP/2, and certificate validity
requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6dd5340-f567-42ab-b0f8-aad310901c4f
📒 Files selected for processing (11)
app/models/reality_scan.pyapp/operation/core.pyapp/utils/reality_scan.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/features/core-editor/components/xray/reality-scan-dialog.tsxdashboard/src/features/core-editor/components/xray/xray-inbounds-section.tsxdashboard/src/service/reality-scan.tstests/test_reality_scan_unit.py
Prefer non-wildcard DNS SANs over the certificate CN when discovering a bare-IP target's server name, and fall back to the CN only when the certificate carries no DNS SANs. This matches OpenSSL hostname verification, which ignores the CN whenever any dNSName SAN is present, so a CN-derived name no longer fails re-validation on an otherwise valid target. Require a confirmed X25519-family key exchange for feasibility instead of only rejecting a confirmed non-X25519 curve. A group probe that cannot determine the curve now marks the target not feasible with an explicit reason rather than passing on an unverified assumption.
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_reality_scan_unit.py (1)
211-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate certificate generation boilerplate.
The certificate creation logic here is nearly identical to
_self_signed_der(defined around line 170). You can avoid this duplication by updating the helper to conditionally add the SAN extension, which keeps the tests focused and easier to maintain.♻️ Proposed refactor
Update this test to use the helper:
- key = rsa.generate_private_key(public_exponent=65537, key_size=2048) - name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "cn-only.example.com")]) - now = datetime.datetime.now(datetime.timezone.utc) - cert = ( - x509.CertificateBuilder() - .subject_name(name) - .issuer_name(name) - .public_key(key.public_key()) - .serial_number(x509.random_serial_number()) - .not_valid_before(now - datetime.timedelta(days=1)) - .not_valid_after(now + datetime.timedelta(days=90)) - .sign(key, hashes.SHA256()) - ) - assert rs._first_usable_name(cert.public_bytes(serialization.Encoding.DER)) == "cn-only.example.com" + der = _self_signed_der("cn-only.example.com", []) + assert rs._first_usable_name(der) == "cn-only.example.com"And update
_self_signed_der(around line 170) to support an emptysanslist sincecryptographyenforces a non-empty list forSubjectAlternativeName:def _self_signed_der(cn: str, sans: list[str]) -> bytes: key = rsa.generate_private_key(public_exponent=65537, key_size=2048) name = x509.Name([ x509.NameAttribute(NameOID.COMMON_NAME, cn), x509.NameAttribute(NameOID.ORGANIZATION_NAME, "Test Org") ]) now = datetime.datetime.now(datetime.timezone.utc) builder = ( x509.CertificateBuilder() .subject_name(name) .issuer_name(name) .public_key(key.public_key()) .serial_number(x509.random_serial_number()) .not_valid_before(now - datetime.timedelta(days=1)) .not_valid_after(now + datetime.timedelta(days=90)) ) if sans: builder = builder.add_extension( x509.SubjectAlternativeName([x509.DNSName(s) for s in sans]), critical=False ) cert = builder.sign(key, hashes.SHA256()) return cert.public_bytes(serialization.Encoding.DER)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_reality_scan_unit.py` around lines 211 - 225, Update _self_signed_der to accept an empty sans list and add the SubjectAlternativeName extension only when sans is non-empty; then replace the duplicated certificate-building logic in test_first_usable_name_falls_back_to_cn_without_san with a call to _self_signed_der using the CN and an empty SAN list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_reality_scan_unit.py`:
- Around line 211-225: Update _self_signed_der to accept an empty sans list and
add the SubjectAlternativeName extension only when sans is non-empty; then
replace the duplicated certificate-building logic in
test_first_usable_name_falls_back_to_cn_without_san with a call to
_self_signed_der using the CN and an empty SAN list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05c08913-8b0c-4ac0-a806-8f75c26aeead
📒 Files selected for processing (2)
app/utils/reality_scan.pytests/test_reality_scan_unit.py
🚧 Files skipped from review as they are similar to previous changes (1)
- app/utils/reality_scan.py
Let _self_signed_der skip the SubjectAlternativeName extension when no SANs are given, and collapse the three inline no-SAN certificate builders onto it, addressing the PR PasarGuard#700 review nitpick.
Contributor
Author
|
@coderabbitai full review 
|
|
The Comment image shows a "brain expansion" style meme with two panels (a smug face and a determined face reaching toward each other), a classic meme format used to depict escalating power/hype. Overlaid on the two panels are three small images: a chaotic glowing "energy skull/face" icon, a photo of Benedict Cumberbatch as Doctor Strange, and the CodeRabbit rabbit logo in the top-right corner — playfully suggesting CodeRabbit is being "summoned" for a powerful full review.
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/utils/reality_scan.py`:
- Around line 253-261: Update _looks_like_hostname in app/utils/reality_scan.py
(lines 253-261) to reject IP literals and validate each label using IDNA/LDH
hostname rules, including allowed characters, hyphen placement, and trailing-dot
handling. Add coverage in tests/test_reality_scan_unit.py (lines 479-483) for
invalid characters, leading or trailing hyphens, repeated trailing dots, and
numeric IP literals; the tests site requires direct additions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf90edfb-74bf-4789-a512-30d5b18ee74a
📒 Files selected for processing (11)
app/models/reality_scan.pyapp/operation/core.pyapp/utils/reality_scan.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/features/core-editor/components/xray/reality-scan-dialog.tsxdashboard/src/features/core-editor/components/xray/xray-inbounds-section.tsxdashboard/src/service/reality-scan.tstests/test_reality_scan_unit.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #695, which added the REALITY target scanner. This reworks it so the
SNI is derived purely from the target, adds bare-IP domain discovery, and hardens
the scan engine.
What changed
New flow
www.microsoft.com:443): resolved to an IP, the hostname issent as the SNI, and the cert is validated against it.
1.1.1.1:443): no SNI is sent the scanner reads the cert theIP presents, discovers the domain from it (CN / first non-wildcard SAN), re-validates
against that, and returns
sni_discovered: truewith the discovered name.longer pulls
serverNames). Multiple targets can be scanned at once; each derives itsown SNI. A target is suitable when: TLS 1.3 + HTTP/2 (ALPN) + an X25519-family key
exchange + a valid certificate.
API —
POST /api/core/reality-scan(auth:cores:read){ "target": "host[:port]", "timeout": 1-20 }(the separatesnifield wasremoved; it's derived from the target).
sni_discovered: bool; otherwise unchanged (target, host, ip, port, sni, feasible, tls13, tls_version, h2, alpn, x25519, post_quantum, curve, h3, cert_valid, cert_subject, cert_issuer, not_after, server_names[], latency_ms, reason).Hardening
select), so a slow-drip peer can't keep
do_handshake()running forever; scans run on adedicated
ThreadPoolExecutorso a lingering scan thread can't starve the event loop'sshared pool (DNS / other
to_thread).WeakKeyDictionary), removing thecross-loop
RuntimeError.gracefully (a reason, not a 502); discovered SNI gated through a hostname check.
Tests: 70 unit tests (handshake-deadline, per-loop semaphore, discovery, error paths)
plus opt-in live-network tests.
Summary by CodeRabbit