TLS test failures #2331
TLS test failures #2331
Conversation
|
@jcrist @mariusvniekerk do either of you have time to check out these test failures? They're breaking tests currently. |
|
Also cc @pitrou |
|
You should be able to pinpoint when the failures started to occur? Is it on a specific changeset? |
|
I haven't tried doing this yet. They didn't show up during any PR and now they fail consistently, so I don't think that they snuck in as some intermittent failures do. My guess is that some dependency changed upstream. |
|
Do you |
|
By the way, I don't think those failures are intermittent. They should even be reproducible locally, with the right software setup. |
|
@pitrou Pretty certain these are related to openssl 1.1 vs 1.0 |
|
@mariusvniekerk do you have a proof of that? e.g. |
|
yep, and the test failures are around TLS 1.3 ciphers See: https://travis-ci.org/dask/distributed/jobs/449255237#L523 Our issue seems to be mozilla/server-side-tls#191 (comment) |
|
The cipher tests should be easy to relax a bit, but the certification reject test shouldn't fail. |
|
it does "warn" about it failing, so it is aware of that but doesn't raise? distributed/distributed/comm/tcp.py Lines 475 to 481 in 6ae7b9e |
|
Right, but it should raise in that case. The warning is in the case the handshake error is because of transient network issues. But if the certificate is reject, that's a permanent error and it should raise an exception. So you must inspect the exception to decide what to do with it. |
|
We should change the test to catch and reraise the exception (within that test) if present to see what it is, we could still be raising something but pytest just says it's not |
|
Marius, you now have push rights to this PR
…On Thu, Nov 8, 2018 at 8:48 AM Marius van Niekerk ***@***.***> wrote:
We should change the test to catch and reraise the exception if present to
see what it is, we could still be raising *something* but pytest just
says it's not EnvironmentError. Want to just rule that one out first.
I'm not sure if i have push rights to this pr
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2331 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASszFWlJhYF3riDybMex57SM2z5z1QSks5utDY4gaJpZM4YGD7B>
.
|
|
Tests seem to pass. The Python 2 failure was due to the |
|
To the extent that I'm able to judge this seems fine to me. There were a couple of test failures but I think that they are intermittent and unrelated. @mariusvniekerk is there any additional work that you think should be done here? |
| @@ -329,6 +333,13 @@ def connect(self, address, deserialize=True, **connection_args): | |||
| stream = yield client.connect(ip, port, | |||
| max_buffer_size=MAX_BUFFER_SIZE, | |||
| **kwargs) | |||
| # Under certain circumstances tornado will have a closed connnection with an error and not raise | |||
There was a problem hiding this comment.
Perhaps it would be worth reporting the issue to the Tornado bug tracker.
There was a problem hiding this comment.
I don't know, but it's still worth reporting. I generally find it annoying when people don't report issues upstream.
When python is compiled against openssl 1.1 we have TLS1.3 ciphers in the mix as well. We should treat these appropriately
This deals with TLS errors that cannot be recovered.
|
I removed my initial dummy commit and added a couple small style changes. I hope that now that the first commit belongs to @mariusvniekerk that the "Squash and merge" feature will appropriately assign ownership to him |
|
This does mean a force push though, so you may want to |
* Check ciphers less aggresively for TLS1.3 ciphers When python is compiled against openssl 1.1 we have TLS1.3 ciphers in the mix as well. We should treat these appropriately * Attempt to mitigate the failed connection case * Introduced a FatalCommClosedError This deals with TLS errors that cannot be recovered. * Relaxed forced cipher test
Do not merge