ioos / pyoos

A Python library for collecting Met/Ocean observations
GNU Lesser General Public License v3.0
34 stars 33 forks source link

SSL error being thrown by HADS collector #77

Closed cordrey closed 7 years ago

cordrey commented 7 years ago

The following error is occurring when using HADS collector:

  File "getHads_tj.py", line 72, in <module>
    respCollect = hadsData.collect()
  File "build/bdist.macosx-10.7-x86_64/egg/pyoos/collectors/hads/hads.py", line 94, in collect
  File "build/bdist.macosx-10.7-x86_64/egg/pyoos/collectors/hads/hads.py", line 102, in raw
  File "build/bdist.macosx-10.7-x86_64/egg/pyoos/collectors/hads/hads.py", line 124, in _get_metadata
  File "/Applications/anaconda/envs/pyoos_testing/lib/python2.7/site-packages/requests-2.13.0-py2.7.egg/requests/api.py", line 110, in post
    return request('post', url, data=data, json=json, **kwargs)
  File "/Applications/anaconda/envs/pyoos_testing/lib/python2.7/site-packages/requests-2.13.0-py2.7.egg/requests/api.py", line 56, in request
    return session.request(method=method, url=url, **kwargs)
  File "/Applications/anaconda/envs/pyoos_testing/lib/python2.7/site-packages/requests-2.13.0-py2.7.egg/requests/sessions.py", line 488, in request
    resp = self.send(prep, **send_kwargs)
  File "/Applications/anaconda/envs/pyoos_testing/lib/python2.7/site-packages/requests-2.13.0-py2.7.egg/requests/sessions.py", line 609, in send
    r = adapter.send(request, **kwargs)
  File "/Applications/anaconda/envs/pyoos_testing/lib/python2.7/site-packages/requests-2.13.0-py2.7.egg/requests/adapters.py", line 497, in send
    raise SSLError(e, request=request)
requests.exceptions.SSLError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:661)
cordrey commented 7 years ago

Adding verify=False to affected requests.post seems to work but not sure if that is the best solution. Example:

        resp = requests.post(self.metadata_url, data={'state'    : 'nil',
                                                      'hsa'      : 'nil',
                                                      'of'       : '1',
                                                      'extraids' : " ".join(station_codes),
                                                      'data'     : "Get Meta Data"},verify=False)
emiliom commented 7 years ago

Thanks for reporting this solution, @cordrey! I'm running into the same error. The last line looks a tad different, but the error type is the same:

requests.exceptions.SSLError: ("bad handshake: Error([('SSL routines', 'ssl3_get_server_certificate', 'certificate verify failed')],)",)

I'll report back if your solution (adding verify=False) worked for me. If it does, I'll push a commit to pyoos.

lukecampbell commented 7 years ago

Can someone post an example? After that I could take a look at it. Generally, we don't want to ignore certificate errors, instead we'd like to work with the site owners to correct any sort of certificate validation errors that could exist.

lukecampbell commented 7 years ago

On a hunch I ran the SSL Server Test on hands.ncep.noaa.gov and I did find a bunch of certificate problems. This one is very likely because the certificate chain is incomplete.

The server is sending it's own signed certificate but not sending the intermediate certificates tracing back to the certificate authority. This is a very common problem that I see a lot. The server should be sending the server certificate and then the intermediate certificate.

If you visit it with your browser, chances are you'll still see a green padlock because you've visited a site with the intermediate certificate at some point so the browser caches these. Software, such as python, doesn't have it's own certificate database it just has the system trusted certificate authority root certificates. So, pyoos, in this case, has never seen the intermediate certificate and can't verify that the server's certificate is valid.

screen shot 2017-03-07 at 10 44 07 am

screen shot 2017-03-07 at 10 44 20 am

lukecampbell commented 7 years ago

https://nginx.org/en/docs/http/configuring_https_servers.html#chains

emiliom commented 7 years ago

Generally, we don't want to ignore certificate errors, instead we'd like to work with the site owners to correct any sort of certificate validation errors that could exist.

Sounds great. But for those of us running operational data harvesting, we typically can't wait for the site owner to respond and address the issue. Thanks for starting to do the leg work!

I've started to look into the pyoos hads collector code so I can add the verify=False. I can make it a user option (passed as an optional argument on the collect method call) rather than a hard-wired default.

Can someone post an example? After that I could take a look at it.

Code chunk:

from pyoos.collectors.hads.hads import Hads

hadsData = Hads()
hadsData.station_codes = ['346F229A']
hadsData.start_time = startdt_utc  # a datetime
responseCollect = hadsData.collect()

Error stack:

Traceback (most recent call last):
*(CLIPPED LINES JUST BEFORE THE CALL TO pyoos)*

File "siso_hads_harvest.py", line 43, in get_hads_data
  responseCollect = hadsData.collect()

File "miniconda/envs/myenv1/lib/python2.7/site-packages/pyoos/collectors/hads/hads.py", line 94, in collect
  metadata, raw_data = self.raw()
File "miniconda/envs/myenv1/lib/python2.7/site-packages/pyoos/collectors/hads/hads.py", line 102, in raw
  metadata      = self._get_metadata(station_codes)
File "miniconda/envs/myenv1/lib/python2.7/site-packages/pyoos/collectors/hads/hads.py", line 124, in _get_metadata
  'data'     : "Get Meta Data"})
File "miniconda/envs/myenv1/lib/python2.7/site-packages/requests/api.py", line 111, in post
  return request('post', url, data=data, json=json, **kwargs)
File "miniconda/envs/myenv1/lib/python2.7/site-packages/requests/api.py", line 57, in request
  return session.request(method=method, url=url, **kwargs)
File "miniconda/envs/myenv1/lib/python2.7/site-packages/requests/sessions.py", line 475, in request
  resp = self.send(prep, **send_kwargs)
File "miniconda/envs/myenv1/lib/python2.7/site-packages/requests/sessions.py", line 585, in send
  r = adapter.send(request, **kwargs)
File "miniconda/envs/myenv1/lib/python2.7/site-packages/requests/adapters.py", line 477, in send
  raise SSLError(e, request=request)
requests.exceptions.SSLError: ("bad handshake: Error([('SSL routines', 'ssl3_get_server_certificate', 'certificate verify failed')],)",)
daf commented 7 years ago

Very much for a verify=False to get pyoos users unstuck from this semi-common situation, with the caveat that a warning is displayed to stderr if possible detailing the situation, as a reminder to revisit your code using pyoos when the upstream service is (hopefully) corrected.

lukecampbell commented 7 years ago

I can get behind and support users from the view point that they need to get up and running again. However, in my experience when we ignore errors and write a TODO to get back to it later, we typically don't. It's not fair to punish users for a mistake by the server owner, though.

If anyone has a good idea to ensure that someone can reach out to the server owner to correct the issue, I can coordinate and help them correct the issue.

This isn't a terribly critical application where verifying authenticity of a server is vital for operations. However, I do lose sleep at night because I've personally been responsible for coding work around after work around for issues just like this, and I'm just waiting for the day where all of them come back and bite me.

emiliom commented 7 years ago

I totally get your concern, @lukecampbell. Though I'm plenty guilty of pushing things to a TODO time that never arrives.

Also, thanks for your input, @daf!

If anyone has a good idea to ensure that someone can reach out to the server owner to correct the issue, I can coordinate and help them correct the issue.

Sorry. I've never interacted with humans on the HADS system/site.

FYI, I've already made the bulk of the code changes on the pyoos hads collector to enable an optional verify argument to be passed to the collect method. The default will still be True if the argument isn't passed. Of course, that default could later be changed to False if there's no headway with HADS changing its certificate problems ...

I'll try to test this late today, and report back hopefully via a PR. Which, if it's merged, should lead to a new minor pyoos release, so that this update as well as the previous hads fix from mid December are made visible and available to users??

lukecampbell commented 7 years ago

Thanks @emiliom

cordrey commented 7 years ago

Thanks guys for following up on this. I don't have enough experience with SSL issues yet to be of much help with contacting the server ops but maybe this link I found while trying to get my system running will help for contact info:

http://www.nws.noaa.gov/os/notification/scn16-51hads.htm

emiliom commented 7 years ago

Alrighty, see PR #78

daf commented 7 years ago

Fixed in #78

@cordrey you can now specify verify=False as a kwarg to the HADS collect and raw methods.

mwengren commented 7 years ago

@emiliom @cordrey @lukecampbell Can one of you test again the HADS collector without the verify=False setting? NWS believes they've resolved the SSL certificate issue and are looking for us to confirm.

lukecampbell commented 7 years ago

I looked at it this morning, the certificate chain is still broken.

SSL Server Test_ hads.ncep.noaa.pdf

cordrey commented 7 years ago

I just tested and still getting original error: requests.exceptions.SSLError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:661)

mwengren commented 7 years ago

Ok, thanks for testing it out. This will be a wait and see whether they are able to get their servers configured properly. Unfortunately, it will have to go through a lengthy RFC process because it is an NWS production system. I'll report back when I get another update.

emiliom commented 7 years ago

Understood. @mwengren, thanks for following through and reporting back to us!

@cordrey, did you install the updated pyoos release and confirm that the verify=False option works for you? It's working just fine for me in an operational data-ingest application (ie, harvesting about hourly, 24/7)

lukecampbell commented 7 years ago

I think @mwengren asked to test it without using your patch @emiliom.

emiliom commented 7 years ago

@lukecampbell, my patch provides a user-optional mechanism to sidestep certificate checking. If the user doesn't pass the verify=False argument value, the default will be True, which will test the certificate. No need to use an old pyoos release!

mwengren commented 7 years ago

@emiliom @cordrey Can one of you test again the HADS collector with verify=False? We got another notification from NCEP to test on our side since they believe they fixed the issue. Thanks!

emiliom commented 7 years ago

@mwengren I use the pyoos HADS collector with collect(verify=False) operationally in a harvesting job that runs every two hours. It's been working continuously w/o issues. So that should be test enough!

But maybe what you meant to ask was testing of verify=True, or w/o the verify argument, which defaults to True?

daf commented 7 years ago

@emiliom yes, I believe that's what he meant.

mwengren commented 7 years ago

Whoops, sorry yeah. We want verify=True tested.

emiliom commented 7 years ago

I won't have time to test this (I'm at a conference), but I'd point you to this code chunk I posted above (Mar 7), that you can use for testing. verify=True is the default that will be used if the verify arg is not passed.

daf commented 7 years ago

This codeblock is now working without verify=False!

mwengren commented 7 years ago

Great, thanks. That confirms my test with the SSL checker. I'll let them know to close.

cordrey commented 7 years ago

Also just tested and works with default settings.

lukecampbell commented 7 years ago

The SSL test passes (with a C), the certificate chain problems have been resolved. Thanks for this.

lukecampbell commented 7 years ago

capture

emiliom commented 7 years ago

Cool!