Skip to content

Refactor SSL shutdown process #385

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 2 commits into from
Feb 5, 2021

Conversation

fantix
Copy link
Member

@fantix fantix commented Jan 30, 2021

Changes made to the SSL protocol implementation:

EOF

If the peer shuts down its writing end of the TCP connection (transport.write_eof() or socket.shutdown(SHUT_WR)) without sending a graceful SSL CLOSE NOTIFY (ssl_sock.unwrap()), we will now close the connection immediately, discard any data in the send buffer.

  • Application's eof_received() will be called in asyncio, but uvloop won't do so after this PR.
  • Application's connection_lost() will be called with parameter None in asyncio. Q: should we use put a ConnectionResetError instead?

Refs RFC 8446, Section 6.1:

Either party MAY initiate a close of its write side of the connection by sending a "close_notify" alert. Any data received after a closure alert has been received MUST be ignored. If a transport-level close is received prior to a "close_notify", the receiver cannot know that all the data that was sent has been received.

Note: It is assumed that closing the write side of a connection reliably delivers pending data before destroying the transport.

In asyncio, eof_received() should be treated as a sign of "the peer's pending data is reliably delivered". But in case we received an EOF on the TCP connection, we don't know if the peer has flushed all the pending data or not, so we shouldn't call eof_received() on the SSL level. On the other hand, as many software is sending TCP EOF before CLOSE_NOTIFY, we don't want to be too picky to fail connection_lost() with an exception. So I think the proposed behavior is appropriate.

Clean Shutdown

The shutdown process is now refactored to be more strict to follow the API.

After transport.close() is called:

  • The only protocol callback that will be triggered is connection_lost().
  • None of data_received(), get_buffer(), buffer_updated(), pause_writing(), resume_writing() will ever be called again. This means application data received after transport.close() will be discarded silently.
  • Calling pause_reading() or resume_reading() has no effect - the shutdown process will procceed unaffected.
  • In the future, we could add a feature by implementing write_eof() in addition to transport.close() to receive trailing data during the shutdown process.

Other Fixes

  • Use _call_soon_handle() internally when possible.
  • Typo fix: clear _handshake_timeout_handle correctly.
  • Refactored internal methods to be more atomic and easy to reason about.
  • Bugfix: not all buffered data is flushed in the shutdown process if the peer's TCP receiving buffer is full.
  • Bugfix: the connection is closed too early before the CLOSE NOTIFY is sent when the peer's TCP receiving buffer is full.

@fantix fantix force-pushed the ssl-shutdown-while-pause-reading branch from 692dd88 to 3fab31c Compare February 1, 2021 01:05
@fantix fantix force-pushed the ssl-shutdown-while-pause-reading branch from 3fab31c to 59a69e6 Compare February 1, 2021 04:49
@fantix fantix marked this pull request as ready for review February 1, 2021 05:51
@fantix fantix requested a review from 1st1 February 1, 2021 05:51
Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I don't know the code intimately well anymore, so I trust you with this change! Great work.

@fantix
Copy link
Member Author

fantix commented Feb 3, 2021

Thanks! I'll create a bpo!

Co-authored-by: Yury Selivanov <[email protected]>
@fantix
Copy link
Member Author

fantix commented Feb 5, 2021

OK this PR does fix bpo-39951, I'm merging it now.

@fantix fantix merged commit 98e113e into MagicStack:master Feb 5, 2021
@fantix fantix deleted the ssl-shutdown-while-pause-reading branch February 5, 2021 15:27
@fantix fantix mentioned this pull request Feb 5, 2021
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