Skip to content

Merge changes from tls-channel to prevent accidentally calling SSLEng… #1726

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

Merged
merged 5 commits into from
Jun 10, 2025

Conversation

vbabanin
Copy link
Member

@vbabanin vbabanin commented Jun 5, 2025

See related bug report: marianobarrios/tls-channel#197
The PR from upstream has been manually merged: marianobarrios/tls-channel#201

JAVA-5797

…ine.beginHandshake more than once.

JAVA-5797
@rozza rozza requested a review from Copilot June 5, 2025 08:42
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR merges upstream changes to prevent unintended duplicate calls to SSLEngine.beginHandshake by replacing the negotiated flag with separate handshakeStarted and handshakeCompleted flags.

  • Replaces the "negotiated" flag with "handshakeStarted" and "handshakeCompleted".
  • Adds a guard to prevent multiple invocations of SSLEngine.beginHandshake while retaining backward compatibility.

@vbabanin vbabanin marked this pull request as ready for review June 5, 2025 23:25
@vbabanin vbabanin requested review from rozza and Copilot June 6, 2025 05:46
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR merges upstream changes from the tls-channel project to prevent unintended SSLEngine renegotiation by introducing explicit handshake state tracking.

  • Replace the single negotiated flag with handshakeStarted and handshakeCompleted for clearer handshake lifecycle management.
  • Add a guard in doHandshake() to skip engine.beginHandshake() on subsequent calls when not forcing a renegotiation.
  • Ensure initSessionCallback is invoked once per completed handshake.
Comments suppressed due to low confidence (4)

driver-core/src/main/com/mongodb/internal/connection/tlschannel/impl/TlsChannelImpl.java:162

  • Consider adding a Javadoc comment explaining the role of the handshakeStarted flag to clarify its lifecycle and prevent future confusion.
private boolean handshakeStarted = false;

driver-core/src/main/com/mongodb/internal/connection/tlschannel/impl/TlsChannelImpl.java:164

  • Consider documenting the handshakeCompleted flag, indicating that it tracks whether the TLS handshake has finished to improve code readability.
private volatile boolean handshakeCompleted = false;

driver-core/src/main/com/mongodb/internal/connection/tlschannel/impl/TlsChannelImpl.java:531

  • Consider adding a unit test for the force=true handshake path after handshakeCompleted to verify that the handshake restarts correctly and doesn’t skip necessary steps.
if (!force && handshakeCompleted) {

driver-core/src/main/com/mongodb/internal/connection/tlschannel/impl/TlsChannelImpl.java:162

  • [nitpick] You might consolidate handshakeStarted and handshakeCompleted into a single enum state (e.g., NOT_STARTED, IN_PROGRESS, COMPLETED) to reduce complexity and prevent inconsistent flag usage.
private boolean handshakeStarted = false;


@Test
@DisplayName("should not call beginHandshake more than once during TLS session establishment")
void shouldNotCallBeginHandshakeMoreThenOnceDuringTlsSessionEstablishment() throws Exception {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test case, as upstream didn't include one to cover this change.

@vbabanin vbabanin self-assigned this Jun 6, 2025
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

LGTM!

@vbabanin vbabanin merged commit a7a3df5 into mongodb:main Jun 10, 2025
52 of 54 checks passed
vbabanin added a commit to vbabanin/mongo-java-driver that referenced this pull request Jun 10, 2025
…ine (mongodb#1726)

- Perform handshake after marking handshake started.
- Add an integration test case, as upstream didn't include one to cover this change.

JAVA-5797
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.

2 participants