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

fix sporadic failure in specs with OpenSSL 1.1+. #8582

Merged
merged 2 commits into from
Dec 16, 2019

Conversation

rdp
Copy link
Contributor

@rdp rdp commented Dec 13, 2019

@ysbaddaden
Copy link
Contributor

Reading the linked OpenSSL issue, there is another solution that would work all the time, and transparently: insert a dummy SSL_read after the first SSL_shutdown. That would force a read and handle any pending session tickets, before a second SSL_shutdown.

src/openssl/ssl/context.cr Outdated Show resolved Hide resolved
@bcardiff
Copy link
Member

@ysbaddaden I can't find exactly where in the linked issue it stands about the dummy SSL_read.
Is that a read with zero bytes?

@Sija
Copy link
Contributor

Sija commented Dec 13, 2019

@ysbaddaden
Copy link
Contributor

Actually (3) at openssl/openssl#6904 (comment)

Or we try to avoid raising inside the BIO and try to properly report the Errno to OpenSSL, if that's possible?

@bcardiff
Copy link
Member

I would definitely prefer a transparent solution.
Since it requires a bit more of investigation, and due to the update in the CI to use bionic (and that uses openssl 1.1.1) I would say to

  1. Apply the suggestions by @Sija
  2. Merge this PR to be able to have a green master
  3. Iterate for 0.33 how to allow a better solution

That is unless, someone can make things work in the next couple of days.

I checked this PR on top of the CI using 0.32.0 and it works https://circleci.com/workflow-run/96ac4542-1977-40ca-b631-fa1c19f02033 .

@Sija
Copy link
Contributor

Sija commented Dec 13, 2019

openssl/openssl#7948 (comment) looks like a possible (albeit pretty hackish) solution.

@bcardiff bcardiff added this to the 0.33.0 milestone Dec 13, 2019
@rdp
Copy link
Contributor Author

rdp commented Dec 13, 2019

calling SSL_read "fixes it and works" if the client does it. However what I was hoping for in this PR is a fix for the server so that it "can work" with any generic client, including ones that don't call that (that just unidirectionally close, which some do)...if that makes sense... :)

@ysbaddaden
Copy link
Contributor

Ah yeah, I didnt properly read.

I guess that OpenSSL is behaving wrongly and shall be fixed. In the meantime, we can merge this, and/or disable TLS 1.3 for the failing spec, or add a dummy read in the spec to avoid the write then close behavior that triggers the issue.

@bcardiff bcardiff merged commit fe94936 into crystal-lang:master Dec 16, 2019
@bcardiff
Copy link
Member

Thanks @rdp for iterating here. When I checked if using bionic would break the new specs were not in master yet.

@bcardiff bcardiff modified the milestones: 0.33.0, 0.32.1 Dec 16, 2019
bcardiff pushed a commit that referenced this pull request Dec 16, 2019
* fix sporadic failure in specs with OpenSSL 1.1+. See openssl/openssl#6904

* don't use a keyword for a variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants