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

Send alert messages on handshake failures #62

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

zinid
Copy link
Contributor

@zinid zinid commented Jan 9, 2024

When SSL_do_handshake() fails we should send data even when SSL_ERROR_WANT_WRITE is not reported (because a final error such as "unsupported protocol" is reported).

If we don't do it, we don't send handshake alerts toward the peer and as a consequence:

  • We formally violate RFC 5246, appendix E.1:

    If server supports (or is willing to use) only versions greater
    than client_version, it MUST send a "protocol_version" alert message
    and close the connection.

  • We are complicating the debugging of the protocol.
  • We don't give a hint to a peer on how to proceed with the error.

This commit fixes it.

When SSL_do_handshake() fails we should send data even when
SSL_ERROR_WANT_WRITE is not reported (because a final error
such as "unsupported protocol" is reported).

If we don't do it, we don't send handshake alerts toward the peer and
as a consequence:

- We formally violate RFC 5246, appendix E.1:
  > If server supports (or is willing to use) only versions greater
  > than client_version, it MUST send a "protocol_version" alert message
  > and close the connection.
- We are complicating the debugging of the protocol.
- We don't give a hint to a peer on how to proceed with the error.

This commit fixes it.
@coveralls
Copy link

Coverage Status

coverage: 57.285% (+0.3%) from 56.971%
when pulling f83322c on zinid:send-handshake-failure
into 4ae9adf on processone:master.

@alexeyshch alexeyshch merged commit 82d4e5b into processone:master Jan 9, 2024
5 checks passed
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.

3 participants