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

Introduce basic support for close_read and close_write. #743

Merged
merged 3 commits into from Apr 30, 2024

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Apr 22, 2024

Introduce SSLSocket#close_read and SSLSocket#close_write + relevant test.

#close_read is a no-op and exists to match the expected interface of IO.

#close_write uses #stop which in testing appears to do what we want.

Fixes #609.

@ioquatix ioquatix requested a review from rhenium April 22, 2024 08:52
@ioquatix ioquatix added this to the v3.3.0 milestone Apr 22, 2024
@rhenium
Copy link
Member

rhenium commented Apr 24, 2024

Looks good to me for TLS 1.3 connections. I'm wondering if #close_write should raise an error if it's called on a TLS 1.2 connection because it can be misleading. What do you think?

close_notify (which is sent by SSLSocket#stop = SSL_shutdown()) works differently in TLS 1.2 or older protocol versions. TLS 1.2 (https://www.rfc-editor.org/rfc/rfc5246#section-7.2.1) says:

The other party MUST respond with a close_notify alert of its own and close down the connection immediately, discarding any pending writes. It is not required for the initiator of the close to wait for the responding close_notify alert before closing the read side of the connection.

@ioquatix
Copy link
Member Author

IMHO, I think it's better just to do nothing. Raising an error will be more trouble for the user, and of little value. In other words, no other implementation of close_write can emit an error like this, so it's unexpected.

@rhenium
Copy link
Member

rhenium commented Apr 26, 2024

I don't see how #close_write could be used in a meaningful way on a TLS 1.2 connection, so users might not to want to call it at all. But I don't have a strong preference on it and I think both are OK.

Could you add some rdoc comments on the behavior difference?

@ioquatix
Copy link
Member Author

What do you think of the updated documentation?

lib/openssl/ssl.rb Outdated Show resolved Hide resolved
lib/openssl/ssl.rb Show resolved Hide resolved
@ioquatix
Copy link
Member Author

@rhenium thanks for your detailed review. Can you check it again?

@rhenium rhenium merged commit a7f2d22 into master Apr 30, 2024
106 checks passed
@rhenium rhenium deleted the close-read-write branch April 30, 2024 14:51
@rhenium
Copy link
Member

rhenium commented Apr 30, 2024

Looks good to me. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Missing shutdown, close_write and close_read.
2 participants