Skip to content

Fix SSL test_preauth_data_to_tls_server #6508

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
youknowone merged 1 commit into from
Dec 25, 2025
Merged

Fix SSL test_preauth_data_to_tls_server #6508

youknowone merged 1 commit into from
Dec 25, 2025

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Dec 25, 2025 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • Bug Fixes
    • Improved error detection and reporting for non-TLS data received during SSL/TLS handshake initiation, providing clearer error messages when connection issues occur.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 25, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

A new PreauthData error variant was added to the SslError enum to represent non-TLS data received before TLS handshake completion. The conversion logic from rustls errors and to Python exceptions was updated to handle this new variant.

Changes

Cohort / File(s) Summary
SSL Error Variant Addition
crates/stdlib/src/ssl/compat.rs		 Added PreauthData variant to SslError enum; mapped rustls::Error::InvalidMessage to SslError::PreauthData during error conversion; added PreauthData arm in SslError-to-Python exception conversion to produce SSLError with library=None and reason "before TLS handshake with data"

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Fix SSL error handling #6351 — Modifies create_ssl_error_with_reason to accept optional library parameter, which is used by this PR when converting PreauthData to Python exceptions with library=None
  • Fix install ujson #6502 — Adds another SslError variant (Timeout) in the same file using similar rustls-to-SslError and SslError-to-Python conversion patterns
  • Fix openssl build and shared ssl/error.rs #6456 — Modifies SSL error handling and exception conversion paths in the same ssl/compat.rs code

Poem

🐰 Before the handshake blooms so bright,
Non-TLS whispers in the night,
PreauthData catches them with care,
Mapping rustls errors through the air—
All with library set to nil, fair!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing SSL pre-authentication data handling by adding a new SslError variant and related conversions.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aae6bf5 and f12a547.

📒 Files selected for processing (1)
  • crates/stdlib/src/ssl/compat.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

  • crates/stdlib/src/ssl/compat.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Run snippets and cpython tests (windows-latest)
🔇 Additional comments (3)
crates/stdlib/src/ssl/compat.rs (3)

394-395: LGTM! Clean error variant addition.

The new PreauthData variant is well-documented and follows Rust conventions. The placement within the enum is logical, grouping it with other protocol-level errors.

567-575: LGTM! Consistent error conversion implementation.

The conversion of PreauthData to a Python exception follows the established pattern in this codebase. Setting library=None is appropriate for this protocol-level error, consistent with the Syscall error handling.

1259-1263: The code correctly maps rustls::Error::InvalidMessage to SslError::PreauthData during handshake. The InvalidMessage error during the TLS handshake phase reliably indicates non-TLS data (such as HTTP requests sent to TLS servers), as valid TLS ClientHello/ServerHello messages would parse successfully. The distinct handling from the general case (which maps InvalidMessage to Eof via from_rustls) is intentional: during handshake, any parsing failure indicates pre-authentication data before TLS began; after handshake, it indicates a protocol violation. The error message "before TLS handshake with data" and the PreauthData enum documentation (line 395: "Non-TLS data received before handshake completed") accurately reflect this distinction.

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 and usage tips.

@youknowone youknowone marked this pull request as ready for review December 25, 2025 04:14
@youknowone youknowone merged commit 49dbbbd into RustPython:main Dec 25, 2025
13 checks passed
@youknowone youknowone deleted the ssl branch December 25, 2025 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@youknowone