Skip to content

Fixes propagation of errors during TLS handshake #316

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

Conversation

artemredkin
Copy link
Collaborator

Motivation:
Right now we only handle one type of SSL error: .handshakeFailed,
but in reality a multitude of errors can happen, for example, remote
party might just close connection that will in turn raise
.uncleanShutdown error, that will be dropped on the floor and
users will only get non-descriptive NoResult error.

Modifications:
Handle all types of SSL errors during handshake instead of just one.
Adds a test.

Result:
Closes #313

@artemredkin artemredkin added this to the 1.2.2 milestone Nov 11, 2020
@artemredkin artemredkin requested a review from Lukasa November 11, 2020 12:24
@artemredkin artemredkin changed the title Fixes propagation of errors during TLS handshakre Fixes propagation of errors during TLS handshake Nov 11, 2020
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Why don't other errors want to do the same things here?

@artemredkin artemredkin force-pushed the fix_ssl_handshakre_error_propagation branch from b505fe5 to f2872ed Compare November 11, 2020 12:40
@artemredkin
Copy link
Collaborator Author

Great question, in test we actually produce two errors, first is connection reset by peer and second is uncleanShutdown, could there be a case were we get IOError and no SSLError? I've updated the PR to handle all errors for now

@artemredkin artemredkin force-pushed the fix_ssl_handshakre_error_propagation branch from f2872ed to a4c7fad Compare November 11, 2020 13:55
Motivation:
Right now we only handle one type of SSL error: `.handshakeFailed`,
but in reality a multitude of errors can happen, for example, remote
party might just close connection that will in turn raise
`.uncleanShutdown` error, that will be dropped on the floor and
users will only get non-descriptive `NoResult` error.

Modifications:
Handle all types of SSL errors during handshake instead of just one.
Adds a test.

Result:
Closes swift-server#313
@artemredkin artemredkin force-pushed the fix_ssl_handshakre_error_propagation branch from a4c7fad to 0202d0b Compare November 11, 2020 13:56
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Seems sensible to me. I think we should be pessimistic about errors and assume weird things can happen.

@artemredkin artemredkin merged commit 236b1de into swift-server:main Nov 11, 2020
@artemredkin artemredkin deleted the fix_ssl_handshakre_error_propagation branch November 11, 2020 15:50
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.

NoResult error on errors before TaskHandler is added
2 participants