SqueakSSL: Drain pending handshake data after connection success (required for TLS 1.3)

[LinqLover]

The LibreSSL implementation already does this. Windows and macOS versions of the plugin are currently pinned to TLS 1.2 but can benefit from this patch in the future, too.

On the image side, this change means that code must no longer rely on primitiveConnect returning 0 for testing whether the connection has established but test SQSSL_PROP_SSLSTATE analogously to for primitiveAccept. While this is a breaking change, it is one that was already dictated by TLS 1.3 in the past few years and has caused a red bar on Linux. Platforms that do not use TLS 1.3 (Windows, macOS) are not affected by this change.

Will be complemented by SqueakSSL-Core-ct.37/SqueakSSL-Tests-ct.29 (Squeak inbox).

[LinqLover]

⚠️ Before merging this change, it should be tested on Windows/macOS/OBSD, too!

[krono]

This definitely would need an increment in the Version number.

[LinqLover]

Why? This does not introduce any backward compatibility per se. For older TLS versions, primitiveConnect keeps returning 0. If at all, we should have introduced a new version number when adding support for TLS 1.3 to the Unix VM.

[krono]

It changes the contract. If the Image-side needs changes due to chages in the plugin, the version must be updated.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR aligns sqConnectSSL (and sqAcceptSSL on iOS) return values with the need to drain any pending handshake output after a successful TLS connection, which becomes important with TLS 1.3-style handshakes.

Changes:

• Win32 + Unix OpenSSL: return the amount of pending handshake output copied into dstBuf instead of returning a fixed success code.
• iOS: return ssl->outLen from sqConnectSSL and sqAcceptSSL after a successful handshake.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
platforms/win32/plugins/SqueakSSL/sqWin32SSL.c	Returns pending handshake output via sqCopyDescToken after connect succeeds.
platforms/unix/plugins/SqueakSSL/sqUnixOpenSSL.inc	Returns pending handshake output via sqCopyBioSSL after connect succeeds.
platforms/unix/plugins/SqueakSSL/sqOBSDOpenSSL.inc	Same as Unix OpenSSL change for the OBSD variant.
platforms/iOS/plugins/SqueakSSL/sqMacSSL.c	Returns ssl->outLen after connect/accept success, to expose pending handshake output.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[krono]

Where does copilot come from?

[LinqLover]

It changes the contract. If the Image-side needs changes due to chages in the plugin, the version must be updated.

Okay, I see your point. The contract was effectively already changed by 2018 when OpenSSL supported OpenSSL 1.1.1 and our unix plugin started automatically using it. Since then, the change in the contract primitiveConnect has been that primitiveConnect would not drain all data from the handshake, leaving callers that control the stream themselves with an incomplete handshake. The change in this PR restores the previous expectation of draining the full handshake by doing an alternative contract change and returning something else than just 0 from primitiveConnect. So, yes, if you want to, we can increase the version (I don't know how). But I don't think that the image code needs to check this version for a fallback.

[LinqLover]

Where does copilot come from?

From my GitHub account, I'm still testing this (a number of times it already provided me C noob with helpful comments, though not too often). It's not a setting of this repository.

[krono]

I have not consented my code to be subjected to copilot.

[johnmci]

As far as I remember code contributions to squeak or the open source vm require the author to agree to https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/Cog/LICENSE

Which doesn't allow the author to restrict how or who or what examines his code

[krono]

I would have expected this to be discussed (eg mainling list) instead of being silently applied.

[LinqLover]

@krono @johnmci I have started a new issue #769 for this where we can discuss the use of GitHub Copilot Reviews without further polluting the comments of this PR. :-)

[krono]

ok

[LinqLover]

It changes the contract. If the Image-side needs changes due to chages in the plugin, the version must be updated.

Okay, I see your point. The contract was effectively already changed by 2018 when OpenSSL supported OpenSSL 1.1.1 and our unix plugin started automatically using it. Since then, the change in the contract primitiveConnect has been that primitiveConnect would not drain all data from the handshake, leaving callers that control the stream themselves with an incomplete handshake. The change in this PR restores the previous expectation of draining the full handshake by doing an alternative contract change and returning something else than just 0 from primitiveConnect. So, yes, if you want to, we can increase the version (I don't know how). But I don't think that the image code needs to check this version for a fallback.

Please ignore the above. I double-checked. With this patch, a slightly older trunk image cannot establish a SSL connection because the previous termination condition result = 0 no longer works. Is this the kind of backward compatibility that we are interested in or do we only care about the other direction (older VM, newer image)? I'm always confused. If compatibility matters here, what would be the best way to preserve that?

1. Make the change backward-compatible by making sqConnectSSL return 0 if already connected (not necessarily good style)?
2. Only activate the new behavior if the image version is newer than X or a VM flag is set?
3. Add a second version of the primitive with the new behavior and keep the old one for compatibility?
4. others?

No idea how this kind of thing is typically done in the VM. Advice welcome!