nutechsoftware / alarmdecoder-webapp

Web application for interfacing with the AlarmDecoder family of devices.
MIT License
49 stars 50 forks source link

Twilio notification setup error #53

Closed twistedstream closed 5 years ago

twistedstream commented 5 years ago

Per my forum post, I'm trying to set up Twilio notifications with the AD webapp. When I complete the setup and click the Save & Test button, I get this error flashed at the top of the page:

Error sending test notification: 'module' object has no attribute 'TwilioRestException'

Doing a little digging, I think the error is coming from this line of code. Here's that line plus the two following it:

except twilio.TwilioRestException as e:
   current_app.logger.info('Event Twilio Notification Failed: {0}' . format(e))
   raise Exception('Twilio Notification Failed: {0}' . format(e))

Looking at the twilio-python library code at the version used by AD, it appears that the class being referenced should be twilio.rest.TwilioRestException and not twilio.TwilioRestException. If this is true, then this is a syntax error, which is swallowing the actual error from Twilio library.

But this code hasn't changed in the AD codebase for over 4 years. I'm finding it hard to believe I'm the only one who's stumbled upon this issue. The other "issue" is that the version of the twilio-python library itself is four years old (current version is 6.22.1 and the one in AD is 3.7.3). It hasn't changed since the initial Twilio SMS support commit.

This seems like a good opportunity for me to tinker and submit a PR that fixes the issue and perhaps updates the Twilio library. But I'd love some feedback first to make sure I'm not chasing a red herring.

endlesscoil commented 5 years ago

Yup, sounds like you're on the right track. That's 3 major revisions behind and it doesn't surprise me at all that the API has changed. I say go for it!

twistedstream commented 5 years ago

@endlesscoil: Right. It’s quite likely the Twilio HTTP REST API itself has changed, which is probably what the actual error is. Regardless, it appears we’ve been handling REST errors incorrectly since the initial Twilio commit (i.e. using the incorrect namespace for the TwilioRestException class). And it seems odd that this wasn’t noticed before.

I’m also quite curious to see what the underlying error is coming back from Twilio as I’m pretty sure I’ve provided the correct config. Seems like I wouldn’t be the only one seeing this issue.

endlesscoil commented 5 years ago

It looks like this is where the breaking change happened. I think the reason we haven't run into this issue before is because most users are likely just using the pre-installed raspi image without updating any pypi packages.

I'm also curious as to what the issue is. I'd wager it's another incompatibility with the library. Regardless, it's ancient and should be updated for the current twilio version.

twistedstream commented 5 years ago

@endlesscoil:

I think the reason we haven't run into this issue before is because most users are likely just using the pre-installed raspi image without updating any pypi packages.

Interesting... I was assuming I was doing the same since my AD webapp is running on a pi that's been flashed with the latest AlarmDecoder Raspbian Stretch 9.0 PiBakery build. But when I check it appears that AD is using a much newer version of the library:

pi@AlarmDecoder:/opt/alarmdecoder-webapp $ sudo pip show twilio
Name: twilio
Version: 6.19.0
Summary: Twilio API client and TwiML generator
Home-page: https://github.com/twilio/twilio-python/
Author: Twilio
Author-email: help@twilio.com
License: UNKNOWN
Location: /usr/local/lib/python2.7/dist-packages
Requires: PyJWT, six, requests, pytz
Required-by:

I'm assuming AD is using the above version since the requirements.txt allows for any version >= 3.7.3. But perhaps I'm not checking the version right. Is AD running in its own Python virtualenv that's using different versions of the libraries than what's installed globally?

And perhaps you are saying that there are users out there who are just running an old version of the pi image who aren't updating the global py libraries?

f34rdotcom commented 5 years ago

Likely this change will break people using the old Raspbian 7 image. That image is very old and is no longer getting security updates so I am not sure much can be done other than rebuilding packages from source. This wont matter unless the "many" people who have this older image still try to upgrade it. Likely it wont break more than this section and the system will keep running. I certainly can download and deploy the old image and test on the dev branch.

Did you say you tested your code change on the latest Raspbian 9 image I made and it works? Or did it give any more details as you noted it would hopefully provide. I am not up to speed on twilio so if you can test this code that would be most most excellent.

I will have a few beta Ad2Pi-pHat boards over the next few months that need homes. If you need another board for testing outside of production DM me and I will see about getting one to you. 20181224_161848

twistedstream commented 5 years ago

@f34rdotcom: I'm on the latest Raspian 9 image. I haven't tested any changes yet. I'm just observing the errors. If I have some time this weekend, I'll attempt the fix described above. I'm assuming I'll need to test both the Twilio and TwiML notification types, right?

Is there a way to test changes like this outside of using an actual RPi, especially the one that's being used with my home alarm system? I found the alarmdecoder-docker repo, which looks promising. However, it still seems likes you us it with an actual RPi? I would have thought Docker would have been a way to run it on all on your workstation so you can test code changes locally. I must be misunderstanding something.

And as far as your beta Ad2Pi-pHat board offer, that sounds great! How should we DM to discuss details? Over IRC?

twistedstream commented 5 years ago

However, it still seems likes you us it with an actual RPi? I would have thought Docker would have been a way to run it on all on your workstation so you can test code changes locally. I must be misunderstanding something.

Disregard my above comment. I get it now. The idea is you host Docker on the RPi and run each service (eg. AD Webapp) as a container. And of course, since they are just images/containers, they could be run within Docker on your workstation as well.

f34rdotcom commented 5 years ago

Yep. I am running my home appliance on docker. Was a bit confusing at first but once you learn how to get into a docker dontainer and run 'ls' it gets easier. I may redo the docker part this year or fund development to improve it. Get all services in separate containers

On Sun, Jan 13, 2019, 9:02 PM Peter Stromquist <notifications@github.com wrote:

However, it still seems likes you us it with an actual RPi? I would have thought Docker would have been a way to run it on all on your workstation so you can test code changes locally. I must be misunderstanding something.

Disregard my above comment. I get it now. The idea is you host Docker on the RPi and run each service (eg. AD Webapp) as a container. And of course, since they are just images/containers, they could be run within Docker on your workstation as well.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nutechsoftware/alarmdecoder-webapp/issues/53#issuecomment-453902698, or mute the thread https://github.com/notifications/unsubscribe-auth/AB8rFBzZZTzLAgfOhDi6Giz6qn3Zyh7Zks5vDA9JgaJpZM4ZyIVY .

twistedstream commented 5 years ago

@f34rdotcom: So I made some progress with this issue. I started a PR, which all it does at the moment is use the new Twilio 6.x class names. That moved me past the syntax error occurring in the exception handler. Now the webapp is revealing the actual error:

Error sending test notification: HTTPSConnectionPool(host='api.twilio.com', port=443): Max retries exceeded with url: /2010-04-01/Accounts/MY_ACCOUNT_SID/Messages.json (Caused by SSLError(SSLError("bad handshake: Error([('SSL routines', 'tls_process_server_certificate', 'certificate verify failed')],)",),))

(where MY_ACCOUNT_SID is my Twilio Account SID)

This error seems to indicate an issue with SSL certificates on the RPi (or the SSL cert chain leading to api.twilio.com). However, when I curl that URL on the RPi it works. I think it may be an issue with the Python config itself or perhaps the version of the Requests library installed? Any guidance would be helpful. Thanks!

twistedstream commented 5 years ago

Oh yeah, I forgot to mention that it appears the latest AD pi build has the newer version of the Twilio Phython library installed (v6.19.0 to be exact). That must have been the latest version of the lib when the AD build was done. And this makes sense given the entry in the requirements.txt file, which will install the latest version of the lib greater than 3.7.3.

So all I’ve done in the PR is fix the AD code to call the newer Twilio API. I didn’t have to update any packages (eg via pip) or anything.

twistedstream commented 5 years ago

@f34rdotcom: Doing some more investigation on this. It seems as though Python SSL is broken with the latest AD build. Just invoking the requests package from the Python CLI produces the same error. This time I'm just doing a GET against https://www.alarmdecoder.com/:

$ python
Python 2.7.13 (default, Sep 26 2018, 18:42:22)
[GCC 6.3.0 20170516] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import requests
>>> response = requests.get("https://www.alarmdecoder.com/")
From cffi callback <function _verify_callback at 0x76c17430>:
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/OpenSSL/SSL.py", line 309, in wrapper
    _lib.X509_up_ref(x509)
AttributeError: 'module' object has no attribute 'X509_up_ref'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.7/dist-packages/requests/api.py", line 72, in get
    return request('get', url, params=params, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/requests/api.py", line 58, in request
    return session.request(method=method, url=url, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/requests/sessions.py", line 512, in request
    resp = self.send(prep, **send_kwargs)
  File "/usr/local/lib/python2.7/dist-packages/requests/sessions.py", line 622, in send
    r = adapter.send(request, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/requests/adapters.py", line 511, in send
    raise SSLError(e, request=request)
requests.exceptions.SSLError: HTTPSConnectionPool(host='www.alarmdecoder.com', port=443): Max retries exceeded with url: / (Caused by SSLError(SSLError("bad handshake: Error([('SSL routines', 'tls_process_server_certificate', 'certificate verify failed')],)",),))

When researching, I saw that sometimes this can happen if older versions of libraries are in use, but all the packages seem very current:

>>> requests.__version__
'2.19.1'
>>> import OpenSSL
>>> OpenSSL.__version__
'18.0.0'

Can you try these same commands in your environment to check to see if maybe this is just occuring on my RPi?

twistedstream commented 5 years ago

So I think I've uncovered the core issue:

In the above error output that resulted from calling requests.get, you may have noticed this line:

AttributeError: 'module' object has no attribute 'X509_up_ref'

That was caused by the OpenSSL package (which is called by requests, which is called by twilio) attempting to call a method in the cryptography package which didn't exist. This was because the cryptography package is one of the few being packages managed by the OS in the /usr/lib/python2.7/dist-packages directory (vs. the /usr/local/lib/python2.7/dist-packages where all the additional Python packages reside). It was quite old (v1.7.1) and missing the X509_up_ref method. This was the actual cause of the error. It had nothing to do with missing CA certs or the like.

To fix it, I had to first uninstall the old version of cryptography:

sudo pip uninstall cryptography 

Ideally, one would then just install the new version. But that would be too easy right? Unfortunately, the new version of cryptography somehow invokes the older version of pip managed by the OS in /usr/lib/python2.7/dist-packages but using a CLI switch it doesn't support (specifically --no-user). In the end, I had to uninstall that version of pip so only the newer version running in /usr/local/lib/python2.7/dist-packages would get used. The only way I figured out how to do that was to via the get-pip.py script:

curl https://bootstrap.pypa.io/get-pip.py -o get-pip.py
sudo python get-pip.py

Finally, I was able to install the latest version of cryptography:

sudo pip install cryptography

Now Twilio API calls are working and SMS messages are flowing 😄 🎉

So I think my PR is still valid. However, it doesn't fix any of the above Python dependency issues. That would have to be done in the AD RPi image.

twistedstream commented 5 years ago

@f34rdotcom: Just checking back to see if you’ve had a chance to take a closer look at this as well as the associated PR?

I’m running my pi on my own branch in order to keep Twilio working. Again, the real issue is the image itself, which needs some Python dependency gardening, per my notes above.

f34rdotcom commented 5 years ago

Thanks for the update on this. I am in here now getting ready to make a new master image and update master branch with lots of fixes etc. Today I am working on the README.md and ... what ever else I can cram in. In doing README.md I am documenting every step as found in the new PiBakery recipe. In that process it is the best opportunity to get this documented. Based upon having a clean image and running this recipe I will work on getting Twilio package instaled and working but I have no experience with Twilio or a test account :( so some help will be good.

twistedstream commented 5 years ago

@f34rdotcom I would love to help. Send me a DM on the AlarmDecoder forum and we can figure out the best way to test it.

f34rdotcom commented 5 years ago

Tried to find your user ID but I failed :(

twistedstream commented 5 years ago

Should be twistedstream

f34rdotcom commented 5 years ago

Ok this confirms the fix as well. https://www.raspberrypi.org/forums/viewtopic.php?p=1267544 I just added a patch to the README.md that "should" fix this but I still need to test as I build a new image.