Skip to content

Conversation

@deekshithaby
Copy link

Proposed change

This PR fixes Python 3.12 compatibility issues in the Nettacker project. Python 3.12 removed the deprecated ssl.wrap_socket() function, causing 2 tests to fail with AttributeError.

This PR updates both the source code and tests to use the modern ssl.SSLContext().wrap_socket() approach, which is the recommended method for SSL/TLS connections in Python 3.10+.

Changes:

  • Replaced ssl.wrap_socket() with ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT).wrap_socket() in nettacker/core/lib/ssl.py and nettacker/core/lib/socket.py
  • Added server_hostname parameter for proper SSL/TLS certificate validation
  • Updated test mocks in tests/core/lib/test_ssl.py and tests/core/lib/test_socket.py to patch the correct SSL method
  • All 214 tests now pass on Python 3.12

Testing:

  • Ran full test suite on Python 3.12.3
  • Before: 212 passed, 2 failed
  • After: 214 passed, 0 failed

Type of change

  • Bugfix (non-breaking change which fixes an issue)

Checklist

  • I've followed the contributing guidelines
  • I've run make test, all tests passed locally

- Replace deprecated ssl.wrap_socket() with ssl.SSLContext().wrap_socket()
- Update test mocks to use ssl.SSLContext.wrap_socket instead of ssl.wrap_socket
- Add server_hostname parameter to wrap_socket calls for proper SSL/TLS handling
- Fixes compatibility issues with Python 3.12 where ssl.wrap_socket was removed
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 10, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Improved TLS/SSL connection handling by using an explicit TLS client context with Server Name Indication (SNI) support for more reliable secure connections.
  • Tests

    • Updated tests to reflect the new TLS handshake behavior and verify SNI-aware wrapping during connections.

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

Walkthrough

Replace module-level ssl.wrap_socket calls with an explicit TLS client SSLContext (ssl.PROTOCOL_TLS_CLIENT) and use context.wrap_socket(..., server_hostname=host) in core socket/ssl code; tests updated to patch/assert ssl.SSLContext.wrap_socket. Previous non-TLS fallback behavior is preserved.

Changes

Cohort / File(s) Summary
Core SSL/socket changes
nettacker/core/lib/socket.py, nettacker/core/lib/ssl.py
Replace ssl.wrap_socket(...) with ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) and context.wrap_socket(..., server_hostname=host). Set check_hostname=False and verify_mode=ssl.CERT_NONE as before. Maintain fallback to non-TLS path on wrap failure.
Tests updated for SSLContext API
tests/core/lib/test_socket.py, tests/core/lib/test_ssl.py
Update monkeypatch/patch targets from ssl.wrap_socket to ssl.SSLContext.wrap_socket. Update assertions to expect server_hostname=HOST passed to the wrapped call.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Reasoning: Small, homogeneous API migration across a few files and corresponding tests.
  • Review focus:
    • Ensure fallback behavior when context.wrap_socket raises matches previous behavior.
    • Confirm intentional disabling of hostname/certificate verification.
    • Verify tests patch the correct bound method (SSLContext.wrap_socket) and assert server_hostname usage.

Suggested reviewers

  • arkid15r

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: replacing ssl.wrap_socket with SSLContext for Python 3.12 compatibility.
Description check ✅ Passed The description is directly related to the changeset, detailing the specific motivation, changes made, and testing results.
✨ 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.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1cbd140 and 5ade02b.

📒 Files selected for processing (1)
  • tests/core/lib/test_ssl.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical

Files:

  • tests/core/lib/test_ssl.py
tests/**/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

tests/**/test_*.py: Place tests under tests/ and name files as test_*.py
Use pytest (with pytest-asyncio, xdist) for tests

Files:

  • tests/core/lib/test_ssl.py
tests/**

📄 CodeRabbit inference engine (AGENTS.md)

Mirror package layout in tests (e.g., tests/core/, tests/lib/, etc.)

Files:

  • tests/core/lib/test_ssl.py

Tip

✨ Issue Enrichment is now available for GitHub issues!

CodeRabbit can now help you manage issues more effectively:

  • Duplicate Detection — Identify similar or duplicate issues
  • Related Issues & PRs — Find relevant issues and PR's from your repository
  • Suggested Assignees — Find the best person to work on the issue
  • Implementation Planning — Generate detailed coding plans for engineers and agents
Disable automatic issue enrichment

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c16a253 and 6d47186.

📒 Files selected for processing (4)
  • nettacker/core/lib/socket.py (1 hunks)
  • nettacker/core/lib/ssl.py (1 hunks)
  • tests/core/lib/test_socket.py (2 hunks)
  • tests/core/lib/test_ssl.py (3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Use 4-space indents in Python code
Limit lines to 99 characters (ruff/ruff-format/isort profile=black)
Module and file names should use lower_snake_case
Function and variable names should use lower_snake_case
Class names should use PascalCase
Constants should use UPPER_SNAKE_CASE
Keep functions small and add type hints where practical

Files:

  • nettacker/core/lib/ssl.py
  • tests/core/lib/test_ssl.py
  • tests/core/lib/test_socket.py
  • nettacker/core/lib/socket.py
nettacker/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Add docstrings for public APIs in the nettacker package

Files:

  • nettacker/core/lib/ssl.py
  • nettacker/core/lib/socket.py
nettacker/core/**

📄 CodeRabbit inference engine (AGENTS.md)

Place core libraries under nettacker/core/

Files:

  • nettacker/core/lib/ssl.py
  • nettacker/core/lib/socket.py
tests/**/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

tests/**/test_*.py: Place tests under tests/ and name files as test_*.py
Use pytest (with pytest-asyncio, xdist) for tests

Files:

  • tests/core/lib/test_ssl.py
  • tests/core/lib/test_socket.py
tests/**

📄 CodeRabbit inference engine (AGENTS.md)

Mirror package layout in tests (e.g., tests/core/, tests/lib/, etc.)

Files:

  • tests/core/lib/test_ssl.py
  • tests/core/lib/test_socket.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: OWASP/Nettacker PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T19:20:58.332Z
Learning: Maintain coverage (pytest configured with --cov=nettacker); add tests for new features and bug fixes
🔇 Additional comments (3)
tests/core/lib/test_socket.py (1)

144-154: LGTM!

Test correctly updated to patch ssl.SSLContext.wrap_socket and verify the server_hostname parameter is passed, aligning with the new SSLContext-based implementation.

tests/core/lib/test_ssl.py (2)

182-194: LGTM!

Test correctly updated to patch ssl.SSLContext.wrap_socket and verify the server_hostname parameter is passed.


438-440: LGTM!

Assertion correctly updated to expect the server_hostname parameter in the wrap_socket call.

- Add context.check_hostname = False to allow scanning self-signed certificates
- Add context.verify_mode = ssl.CERT_NONE to match original ssl.wrap_socket behavior
- Maintains backward compatibility for security scanner use cases
@deekshithaby
Copy link
Author

@securestep9 - Testing Results

I've thoroughly tested the changes on Python 3.12 with real scanning:

✅ Full Nettacker scan completed successfully
✅ All 117 modules loaded correctly
✅ SSL weak version detection: Working
✅ SSL weak cipher detection: Working
✅ Port scanning: All 1005 requests completed
✅ Report generation: HTML reports created successfully
✅ Zero Python 3.12 compatibility errors

The SSL functionality is operating properly with the SSLContext changes.

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.

1 participant