Skip to content
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.

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

Update the zcrypto dependency to bring in TLS 1.3 support #507

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

phillip-stephens
Copy link
Contributor

Add a description of your changes here.

How to Test

Add brief instructions on how to test your changes.

Notes & Caveats

If necessary, explain the motivation for this PR, and note any caveats that apply to your changes or future work that will be needed.

Issue Tracking

Add a link to the relevant GitHub issue(s) if the pull request resolves it.

@phillip-stephens
Copy link
Contributor Author

Looks like we'll need to update the zschema definitions in zcrypto for the integration tests to pass. @developStorm would you have the bandwidth to take a look at this?

zakird and others added 3 commits March 21, 2025 11:36
The old Read method was incorrectly implemented as ReadFull, causing it to wait indefinitely for a new packet that may never arrive under certain conditions.
@developStorm developStorm marked this pull request as ready for review March 23, 2025 06:46
@developStorm
Copy link
Member

@phillip-stephens Thanks for the heads up, fixed zschemas!

ZGrab on this branch is currently tracking an unmerged commit of ZCrypto zmap/zcrypto#412 to allow the integration test to pass. We should merge the ZCrypto PR first before proceeding with this one.

Additionally, the Python integration test now pulls schemas from a specific version of ZCrypto instead of the latest main commit (see the requirements.txt change). This ensures it aligns with the ZCrypto version specified in go.mod.

The test failure took longer to resolve due to an issue with the mssql module that was difficult to identify. The tdsConnection.Read() method was incorrectly implemented as ReadFull, causing it to wait indefinitely for a new packet that will never arrive. The issue only became evident under specific packet sizes and input buffering configurations - this happened to be the case with Server Hello packets in the upgraded TLS library, where an increase in size triggered the flawed TDS packet handling. e165e17 fixed this issue by ensuring the method no longer tries to fill up the input buffer by waiting for new TDS packets, aligning with the expected behavior of Read().

I also patched the integration test and Docker setup a bit with 9f512c2 to improve robustness.

@developStorm developStorm self-assigned this Mar 23, 2025
@developStorm developStorm enabled auto-merge (rebase) March 23, 2025 07:20
@developStorm developStorm disabled auto-merge March 23, 2025 07:21
Copy link
Member

@developStorm developStorm left a comment

Choose a reason for hiding this comment

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

Oh I guess you probably can't approve this PR because you created it @phillip-stephens. I'll just approve this and you can rebase/merge it if it looks good.

@developStorm developStorm linked an issue Mar 23, 2025 that may be closed by this pull request
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.

zgrab2 with tls1.3
3 participants