TLS test failures by mrocklin · Pull Request #2331 · dask/distributed

mrocklin · 2018-11-01T09:29:38Z

Do not merge

mrocklin · 2018-11-02T10:21:44Z

@jcrist @mariusvniekerk do either of you have time to check out these test failures? They're breaking tests currently.

mrocklin · 2018-11-03T16:56:14Z

Also cc @pitrou

pitrou · 2018-11-03T16:58:10Z

You should be able to pinpoint when the failures started to occur? Is it on a specific changeset?

mrocklin · 2018-11-03T17:00:36Z

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.

pitrou · 2018-11-03T17:01:29Z

Do you conda list as part of CI jobs? That's very useful for cases like this, so that you can compare versions.

pitrou · 2018-11-03T17:02:07Z

By the way, I don't think those failures are intermittent. They should even be reproducible locally, with the right software setup.

mariusvniekerk · 2018-11-08T13:21:27Z

@pitrou Pretty certain these are related to openssl 1.1 vs 1.0

pitrou · 2018-11-08T13:22:27Z

@mariusvniekerk do you have a proof of that? e.g. conda list output?

mariusvniekerk · 2018-11-08T13:25:50Z

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)

pitrou · 2018-11-08T13:27:43Z

The cipher tests should be easy to relax a bit, but the certification reject test shouldn't fail.

mariusvniekerk · 2018-11-08T13:30:26Z

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

    
           except EnvironmentError as e: 
        
               # The handshake went wrong, log and ignore 
        
               logger.warning("Listener on %r: TLS handshake failed with remote %r: %s", 
        
                              self.listen_address, address, 
        
                              getattr(e, "real_error", None) or e) 
        
           else: 
        
               raise gen.Return(stream)

pitrou · 2018-11-08T13:39:26Z

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.

mariusvniekerk · 2018-11-08T13:48:39Z

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 EnvironmentError. Want to just rule that one out first.
I'm not sure if i have push rights to this pr

mrocklin · 2018-11-08T14:15:13Z

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> .

mrocklin · 2018-11-08T19:03:37Z

Tests seem to pass. The Python 2 failure was due to the print(..., flush=True) syntax in debug code.

mrocklin · 2018-11-08T21:15:18Z

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?

pitrou · 2018-11-08T21:26:58Z

            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


Perhaps it would be worth reporting the issue to the Tornado bug tracker.

@mrocklin I think this gets us working for now.
@pitrou I don't think tornado is tested a python built against openssl 1.1+ atm?

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.

mrocklin · 2018-11-08T23:34:34Z

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

mrocklin · 2018-11-08T23:35:39Z

This does mean a force push though, so you may want to git reset --hard before pulling/pushing if there is any more to do

* 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

mrocklin mentioned this pull request Nov 1, 2018

Adaptive: recommend close workers when any are idle #2330

Merged

mrocklin changed the title ~~A trivial change to test CI~~ TLS test failures Nov 3, 2018

This was referenced Nov 3, 2018

Add preload script to conf #2325

Merged

Expand async docs #2293

Merged

pitrou reviewed Nov 8, 2018

View reviewed changes

mariusvniekerk mentioned this pull request Nov 8, 2018

StreamClosedError not raised when connecting to a TLS server when using incorrect certificates tornadoweb/tornado#2533

Open

mariusvniekerk and others added 8 commits November 8, 2018 18:21

Add more logging for this test

68b9670

Check ciphers less aggresively for TLS1.3 ciphers

4939f89

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

88130fc

Introduced a FatalCommClosedError

4fcb1fd

This deals with TLS errors that cannot be recovered.

Relaxed forced cipher test

6c07897

Remove debug code

8f7b549

raise bare exception

ca90f8a

avoid internal import ssl

1021ced

mrocklin force-pushed the test-ci branch from 3ba9728 to 1021ced Compare November 8, 2018 23:33

mrocklin merged commit 3fed641 into dask:master Nov 9, 2018

mrocklin deleted the test-ci branch November 9, 2018 12:37

Uh oh!

Uh oh!

Conversation

mrocklin commented Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrocklin commented Nov 2, 2018

Uh oh!

mrocklin commented Nov 3, 2018

Uh oh!

pitrou commented Nov 3, 2018

Uh oh!

mrocklin commented Nov 3, 2018

Uh oh!

pitrou commented Nov 3, 2018

Uh oh!

pitrou commented Nov 3, 2018

Uh oh!

mariusvniekerk commented Nov 8, 2018

Uh oh!

pitrou commented Nov 8, 2018

Uh oh!

mariusvniekerk commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Nov 8, 2018

Uh oh!

mariusvniekerk commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Nov 8, 2018

Uh oh!

mariusvniekerk commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrocklin commented Nov 8, 2018 via email

Uh oh!

mrocklin commented Nov 8, 2018

Uh oh!

mrocklin commented Nov 8, 2018

Uh oh!

pitrou Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

mariusvniekerk Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

pitrou Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

mariusvniekerk Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

mrocklin commented Nov 8, 2018

Uh oh!

mrocklin commented Nov 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mrocklin commented Nov 1, 2018 •

edited

Loading

mariusvniekerk commented Nov 8, 2018 •

edited

Loading

mariusvniekerk commented Nov 8, 2018 •

edited

Loading

mariusvniekerk commented Nov 8, 2018 •

edited

Loading