Fix certificate error for IERS data download on Windows #10434

noc0lour · 2020-06-03T10:58:53Z

Across different machines Windows often has intermittent problems of fetching data from datacenter.iers.org due to the CA certificate missing on Windows. (visiting the webpage via internet explorer will temporarily fetch the certificate, but that's rather clumsy and unreliable).

Description

Use certifi certificate store for reliable CA root certificate fetching on all platforms.

Fixes #9640

Close #10448

noc0lour · 2020-06-03T16:34:56Z

It seems that accessing URLs in travis is not always stable and sometimes times out. Other than that the amount of tests working in travis is increased. In the Windows test even the ftp test for downloading the IERS dataset works (XPASS), weirdly it still fails on Linux.

pllim · 2020-06-03T16:36:40Z

Yes, they are flaky. And the more jobs try to access them concurrently, the flakier they get as some servers try to block them from spamming. Not sure what we can do about it.

mhvk · 2020-06-03T20:47:30Z

I don't really feel competent to review this, but am unclear about one thing: where should this belong in our dependencies? Since IERS files are required for quite a bit of essential use of astropy, is certifi effectively a required dependency on windows? Or just something to make travis work? It will certainly have to be documented in docs/install.rst.

noc0lour · 2020-06-03T21:35:35Z

@mhvk This is specifically to Windows, the travis test case on Windows also exhibits this behaviou, but it's not specific to travis.

To get the CA certificates in urllib/Python for Windows:
Essentially either users & systems will have to install required CA certificates on windows by means of using the certmanager or visiting datacenter.iers.org via internet explorer to auto-fetch is from a windows (update?) server. The next best thing is using certifi which is providing an independent CA store.

I've lost the original issue on bugs.python.org where this was discussed in more detail, but the answer on this https://bugs.python.org/issue36011 confirms the problem with the certificate storage on Windows and ways to solve it.

I'm happy to iterate on adding documentation to install/changelog as needed.

mhvk · 2020-06-03T22:31:44Z

OK, so on Windows certifi effectively becomes a required dependency. This probably is not much of a problem, since many users likely have it anyway (on my Debian system, requests has pulled it in), but it needs some thought on how to have OS-specific dependencies. cc @astrofrog
(and of course ties in with the more general discussion about what to do about the exploding number of dependencies in all - #10342.

saimn · 2020-06-04T02:16:05Z

With the current version of the PR, this makes certifi a dependency not only for windows, and it should be listed in install_requires. If it is not too cumbersome to make it optional, window-only, then it is possible to use markers to make it a dependency only for windows:
https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-platform-specific-dependencies

noc0lour · 2020-06-04T08:17:36Z

In the current approach I deliberately included it for Linux and Windows as otherwise it would require to write OS specific codepaths later and is not really elegant nor readable.
Altthough if it is agreed upon to be the best way forward I can implement that as the patch here is rather short anyway.

On Linux the system-installed certifi package (at least on Fedora & Gentoo) actually points to the system CA certificates. Although in a virtualenv it points tho the locally installed CA certificates.

.travis.yml

pllim · 2020-06-04T22:28:09Z

Just so we have something we can compare against, I did a remote data run on Windows over at #10448 . Its log does show urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1076)> among some other errors that are unrelated to certificates.

EDIT: To be more specific...

UNEXPECTED EXCEPTION: AstropyWarning('failed to download ftp://anonymous:mail%40astropy.org@gdc.cddis.eosdis.nasa.gov/pub/products/iers/finals2000A.all and https://datacenter.iers.org/data/9/finals2000A.all, using local IERS-B: <urlopen error Unable to open any source! Exceptions were {\'ftp://anonymous:mail%40astropy.org@gdc.cddis.eosdis.nasa.gov/pub/products/iers/finals2000A.all\': URLError("ftp error: error_temp(\'425 Security: Bad IP connecting.\')"), \'https://datacenter.iers.org/data/9/finals2000A.all\': URLError(SSLCertVerificationError(1, \'[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self signed certificate in certificate chain (_ssl.c:1076)\'))}>')

E               urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self signed certificate in certificate chain (_ssl.c:1076)>

This one is actually from astropy.readthedocs.io :

E           ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1076)

pllim

Here's my plan:

Cherry-pick the relevant fix to EXP: Testing on Windows with remote data #10448 to convince myself that this really works.
See if there is a way to only require certifi for Windows and not other OS.

Todo for this PR:

Address existing review comments.
- Add text to docs/install.rst
- Remove remote data Windows job from PR CI. Instead create one as cron job.
Wait for next steps based on my investigation above.

How does this sound? Thanks!

setup.cfg

pllim · 2020-06-05T16:25:22Z

I'll ponder the need for change log and milestone later. Given that this is a bug, it can be backported but then again, it's been there forever and our Windows user base is rather small. 🤷

UPDATE: I decided against backport due to introduction of new dependency for Windows.

pllim · 2020-06-06T00:45:02Z

So, reporting back on my work at #10448 that is built upon the fix here:

It is definitely possible to make certifi solution for Windows only. (You can ignore the OSX failures over there; they are not related to certs and I don't think there is a need to fix those.)
However, even on Windows, once you got that certificate, you don't really need certifi anymore, so I don't think it should be mandatory dependency even for Windows. And there is also no guarantee that this will not become a problem for other OSes in the future.
So, I am thinking maybe to make this solution something that user can toggle on and off, maybe in astropy.utils.conf and it will be off by default and we explicitly turn it on when we run remote data tests for Windows.

What do y'all think?

mhvk · 2020-06-06T01:15:21Z

@pllim - thanks so much for the investigation. I feel somewhat incompetent here, so don't really know what to suggest - and will defer to @noc0lour for what will help windows users most - except that whatever we do should be documented clearly!

noc0lour · 2020-06-06T13:11:49Z

@pllim I fully agree that for now the fix is only necessary on Windows and also not always. I will implement the fix to target only Windows OS via the requirements specification in setup and checking for the OS/availability of certifi in the utils code.

As this is a annoying road block for new users (which with a entry in a FAQ etc can be resolved quickly), I would rather opt to default a possible configuration option to on or make it non-optional for Windows entirely. As certifi is used by requests we will not have an issue of it going away/getting outdated anytime soon.

I made this patch primarily not to fix tests or anything but to make it easier to get started on Windows (no error on the first run) for new users.

pllim · 2020-06-06T14:43:28Z

Thanks for your inputs, @noc0lour ! Looking forward to your next commit(s).

noc0lour · 2020-06-15T08:07:53Z

Implemented the certifi solution to be only used on the windows platform checked with os.name == 'nt'. If we have a more elegant solution for these imports I'm happy to adopt it.

pllim

Thank you very much for following up and your continued patience! I think about this a bit more and...

I am still not convinced that requiring it for Windows blindly is the best way forward, especially given the note about multiprocessing and the note about OpenSSL. The problem this PR is trying to solve does not seem to affect a lot of people, but the fix affects all Windows users going forward. As you have mentioned, a FAQ might be sufficient for beginner users even if this fix is not the default behavior? I still think that making it a non-default configurable item the safest route, as that won't change any existing behavior at all. Would be nice to have more opinions about this.
Depending on the decision above, the change log would be under API change or new feature, but regardless, I have set the milestone.
Additional text about certifi needs to be in docs/install.rst.
Some tests will fail for this new cron job unless their fixes are copied over from EXP: Testing on Windows with remote data #10448 for astropy/coordinates/tests/test_solar_system.py and docs/coordinates/spectralcoord.rst.

astropy/utils/data.py

pllim · 2020-09-18T21:26:55Z

I wasn't the best reviewer for this PR, resulting in it getting stale. I do think we need to find a way forward with this one way or another. @embray , any ideas?

noc0lour · 2020-09-21T08:22:31Z

@pllim at least from my side I just did not have the time (yet) to follow up with a distinct implementation to put things into configuration etc. Current state could be used by anyone interested to implement the configuration to go into the implemented codepath.

pllim

I can confirm that the utils/tests/test_misc.py failure I see on Windows complaining about SSL cert is gone with this patch! 🎉

Found some typos and more minor comments, but I think we're really close now. Thanks for your patience! 😸

CHANGES.rst

astropy/utils/data.py

astropy/utils/misc.py

docs/install.rst

docs/utils/data.rst

pllim · 2021-04-19T17:40:33Z

@embray , any chance we can wrap this up before end of April 2021?

embray · 2021-04-22T10:01:50Z

@pllim will do!

ref: astropy#10434 (review)

embray · 2021-04-22T11:00:49Z

Somehow an unrelated commit got in here when I rebased...

Initialize a SSL context with the CA file taken from certifi which collects the Mozilla CA certificates.

this is consistent with how the 'requests' package handles root CA certificates: https://requests.readthedocs.io/en/master/user/advanced/#ca-certificates

…ly when installed but is not required adds an option allow_insecure to allow downloads over unverified SSL connections to succeed with a warning, as well as an ssl_context argument which allows additional optional settings for configuring SSL connections

ref: astropy#10434 (review)

timeout to 5 seconds however, when it does fail it appears to be raising a URLError, not socket.timeout

automatically re-tried with allow_insecure=True

embray · 2021-04-22T11:04:07Z

Ok. Everything should be fixed now. @pllim I addressed all your comments. Also fixed the changelog entry to the new format.

embray · 2021-04-22T11:26:28Z

Bleh, there are some test failures because I changed a v to a V in the error message :)

allow_insecure=True.

pllim · 2021-04-27T20:00:44Z

astropy/utils/tests/test_data.py

@@ -1723,7 +1745,7 @@ def info(self):
        def read(self, length=None):
            return self.reader.read(length)

-    monkeypatch.setattr(urllib.request, "build_opener", mockurl_builder)
+    monkeypatch.setattr(astropy.utils.data, "_build_urlopener", mockurl_builder)


I remember seeing monkeypatching of astropy.utils.data over at astroquery, so this is a heads up to astroquery people: @keflavich , @bsipocz , @ceb8

pllim

LGTM. I tested this again and seems fine on Windows. Thanks, all!

embray · 2021-04-29T17:02:32Z

Thank you 🙏

pllim · 2021-05-05T21:56:36Z

For future reference, this broke astroquery's test monkeypatching downstream, see astropy/astroquery#2059 . FYI.

astropy-bot bot added the utils label Jun 3, 2020

pllim added utils.iers Windows labels Jun 3, 2020

pllim requested review from mhvk, eteq and pllim June 3, 2020 16:22

astrofrog reviewed Jun 4, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

pllim reviewed Jun 5, 2020

View reviewed changes

setup.cfg Show resolved Hide resolved

pllim mentioned this pull request Jun 6, 2020

EXP: Testing on Windows with remote data #10448

Closed

noc0lour force-pushed the fix_certificate_error_windows branch 2 times, most recently from e0e2053 to 9833363 Compare June 15, 2020 08:04

pllim added this to the v4.2 milestone Jun 16, 2020

pllim reviewed Jun 16, 2020

View reviewed changes

astropy/utils/data.py Outdated Show resolved Hide resolved

pllim reviewed Jan 19, 2021

View reviewed changes

embray force-pushed the fix_certificate_error_windows branch from 909eb2b to 892afe3 Compare April 22, 2021 10:49

embray added a commit to noc0lour/astropy that referenced this pull request Apr 22, 2021

utils: remove the default_allow_insecure config option for now

854db8c

ref: astropy#10434 (review)

noc0lour and others added 11 commits April 22, 2021 13:01

utils: Use certifi on Windows systems as reliable CA certificate store

a88d96f

Initialize a SSL context with the CA file taken from certifi which collects the Mozilla CA certificates.

utils: insert HTTPSHandler also if ftp_tls is True

ad2c966

utils: use certifi for SSL requests on all platforms

0f4e2a7

this is consistent with how the 'requests' package handles root CA certificates: https://requests.readthedocs.io/en/master/user/advanced/#ca-certificates

utils: use get_readable_fileobj for find_api_page

4fab759

utils: add a note in docs on how to deal with certificate errors

6c12092

utils: remove the default_allow_insecure config option for now

28560f0

ref: astropy#10434 (review)

tests: the download in this test is a little slow sometimes, so bump the

6623262

timeout to 5 seconds however, when it does fail it appears to be raising a URLError, not socket.timeout

Address @pllim nitpicks ;)

a593a82

Make it clear in the warning message that the download is being

49ad080

automatically re-tried with allow_insecure=True

Additional typo fix.

9cdabbb

embray force-pushed the fix_certificate_error_windows branch from a84237c to d77bf02 Compare April 22, 2021 11:02

More typo fixes and an additional warning about using

3feff1f

allow_insecure=True.

embray force-pushed the fix_certificate_error_windows branch from d77bf02 to 3feff1f Compare April 22, 2021 11:27

pllim reviewed Apr 27, 2021

View reviewed changes

pllim approved these changes Apr 27, 2021

View reviewed changes

pllim merged commit b27c921 into astropy:main Apr 27, 2021

This was referenced May 5, 2021

vo_conesearch errors with astropy-dev astropy/astroquery#2054

Closed

TST: Fix vo_conesearch test failures astropy/astroquery#2059

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix certificate error for IERS data download on Windows #10434

Fix certificate error for IERS data download on Windows #10434

noc0lour commented Jun 3, 2020 •

edited by pllim

Loading

noc0lour commented Jun 3, 2020

pllim commented Jun 3, 2020

mhvk commented Jun 3, 2020

noc0lour commented Jun 3, 2020 •

edited

Loading

mhvk commented Jun 3, 2020

saimn commented Jun 4, 2020

noc0lour commented Jun 4, 2020

This comment has been minimized.

pllim commented Jun 4, 2020 •

edited

Loading

pllim left a comment

pllim commented Jun 5, 2020 •

edited

Loading

pllim commented Jun 6, 2020

mhvk commented Jun 6, 2020

noc0lour commented Jun 6, 2020

pllim commented Jun 6, 2020

noc0lour commented Jun 15, 2020

pllim left a comment

pllim commented Sep 18, 2020

noc0lour commented Sep 21, 2020

pllim left a comment

pllim commented Apr 19, 2021

embray commented Apr 22, 2021

embray commented Apr 22, 2021

embray commented Apr 22, 2021

embray commented Apr 22, 2021

pllim Apr 27, 2021

pllim left a comment

embray commented Apr 29, 2021

pllim commented May 5, 2021

Fix certificate error for IERS data download on Windows #10434

Fix certificate error for IERS data download on Windows #10434

Conversation

noc0lour commented Jun 3, 2020 • edited by pllim Loading

Description

noc0lour commented Jun 3, 2020

pllim commented Jun 3, 2020

mhvk commented Jun 3, 2020

noc0lour commented Jun 3, 2020 • edited Loading

mhvk commented Jun 3, 2020

saimn commented Jun 4, 2020

noc0lour commented Jun 4, 2020

This comment has been minimized.

pllim commented Jun 4, 2020 • edited Loading

pllim left a comment

Choose a reason for hiding this comment

pllim commented Jun 5, 2020 • edited Loading

pllim commented Jun 6, 2020

mhvk commented Jun 6, 2020

noc0lour commented Jun 6, 2020

pllim commented Jun 6, 2020

noc0lour commented Jun 15, 2020

pllim left a comment

Choose a reason for hiding this comment

pllim commented Sep 18, 2020

noc0lour commented Sep 21, 2020

pllim left a comment

Choose a reason for hiding this comment

pllim commented Apr 19, 2021

embray commented Apr 22, 2021

embray commented Apr 22, 2021

embray commented Apr 22, 2021

embray commented Apr 22, 2021

pllim Apr 27, 2021

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

embray commented Apr 29, 2021

pllim commented May 5, 2021

noc0lour commented Jun 3, 2020 •

edited by pllim

Loading

noc0lour commented Jun 3, 2020 •

edited

Loading

pllim commented Jun 4, 2020 •

edited

Loading

pllim commented Jun 5, 2020 •

edited

Loading