Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements SSL connection #20

Merged
merged 4 commits into from Oct 21, 2019
Merged

Implements SSL connection #20

merged 4 commits into from Oct 21, 2019

Conversation

Smankusors
Copy link
Contributor

@Smankusors Smankusors commented Oct 10, 2019

There's some small example I included on README.md file.

Also the certificate verification works with blynk-cloud.com (included _blynk-cloudcom.crt from blynk-library) and Let's Encrypt certificates :)

@antohaUa
Copy link
Collaborator

antohaUa commented Oct 10, 2019

Smankusors,
Many thanks for your SSL solution.
I just need to check micropython ssl compatibility and soon will return to you with some review comments.
But I think we are very close to final solution.

@Smankusors
Copy link
Contributor Author

Smankusors commented Oct 10, 2019

Yeah that is something I'm worried about, especially the certificate verification :/

@antohaUa
Copy link
Collaborator

antohaUa commented Oct 11, 2019

Smankusors ,
Unfortunately I have found that within micropython wrap_socket "..some or all keyword arguments above may be not supported"
http://docs.micropython.org/en/latest/library/ussl.html
We are limited in micropython support ((

But ssl feature very important and I suggest:

  1. place copy of blynklib with ssl modifications to separate folder "ssl"
  2. place crt file as "blynk-cloudcom.crt" also to "ssl" new dir
  3. Also I propose to use not ssl but ssl_cert flag that means that certificate can be defined by user and if it was defined - then we should try use ssl connection mode.
    smth like:
    self._state = self.CONNECTING
    self._socket = socket.socket()
    self._socket.connect(socket.getaddrinfo(self.server, self.port)[0][4])
    self._set_socket_timeout(self.SOCK_TIMEOUT)
    if self.ssl_cert:
    self._set_socket_timeout(self.SOCK_SSL_TIMEOUT)
    self.log('Using SSL socket...')
    self._socket = ssl.wrap_socket(sock=self._socket, ca_certs=self.ssl_cert)

Note! SOCK_SSL_TIMEOUT: We need this to handle all timeout related events, but ssl needs more time for connect so instead of default 0.05 we should use other value 1sec or 0.5 sec

If you are ok with this plan - lets modify existing pull request. For new folder I will create README by myself - so don't worry here.

And again thx a lot for all your investigations as I said ssl important feature and all your findings are very and very helpful.

@Smankusors
Copy link
Contributor Author

Smankusors commented Oct 11, 2019

Hmm what is this new folder ssl for? For development? Hmm I think I propose to create a new branch for SSL implementation. Then when it's ready, we merge it to master.

For ssl_cert, if the user wants to use Let's Encrypt or blynk-cloud.com, they don't need to set custom ca_certs. Well for micropython.... to be honest I never touched it... so I have no idea how to implement this for micropython 😖

@antohaUa
Copy link
Collaborator

antohaUa commented Oct 11, 2019

Hmm what is this new folder ssl for?

Main lib designed for cPython and micropython support thus I can't place code there. Within SSL dir will be cPython with SSL feature. You can create base and then I will remove not needed micropython staff

propose to create a new branch for SSL implementation

You see unfortunately most of users do not use branches. Even code sometimes just copied from web pages )) If we will have correct REAMDE in ssl folder and annotations in main README thinks for end users will be more simple and clear.

so I have no idea how to implement this for micropython

Here is no actions for micropython. As i said in prev comment we are blocked with wrap_socket call limitations. So focus should be pointed just to cpython ssl implementation

if the user wants to use Let's Encrypt or blynk-cloud.com, they don't need to set custom ca_certs

This is true. But library can be used for custom local servers and put hardcoded certificate names probably not correct way if we want flexibility of code usage. Here you see we are balancing between usability and flexibility.

@Smankusors
Copy link
Contributor Author

Smankusors commented Oct 11, 2019

Main lib designed for cPython and micropython support thus I can't place code there. Within SSL dir will be cPython with SSL feature. You can create base and then I will remove not needed micropython staff

Hmm so the main program code will be something like this?

import blynk
blynk = blynklib.ssl.Blynk(BLYNK_AUTH)

This is true. But library can be used for custom local servers and put hardcoded certificate names probably not correct way if we want flexibility of code usage. Here you see we are balancing between usability and flexibility.

Oh yeah I forgot to say that the ssl_cert will be "optional". How about that?

@antohaUa
Copy link
Collaborator

antohaUa commented Oct 11, 2019

ssl_cert will be "optional".

Yeap, definitely right. You can place it as keyword parameter with default None value
and condition
if self.ssl_cert:
self._set_socket_timeout(self.SOCK_SSL_TIMEOUT)
self.log('Using SSL socket...')
self._socket = ssl.wrap_socket(sock=self._socket, ca_certs=self.ssl_cert)

will cover only case when ssl_cert was defined

blynk = blynklib.ssl.Blynk(BLYNK_AUTH)

Not sure... I was thinking about copy -paste from ssl folder to root. Library name will be the same.
But maybe you have any other suggestions? We can discuss them...

@Smankusors
Copy link
Contributor Author

Smankusors commented Oct 11, 2019

and condition
if self.ssl_cert:
self._set_socket_timeout(self.SOCK_SSL_TIMEOUT)
self.log('Using SSL socket...')
self._socket = ssl.wrap_socket(sock=self._socket, ca_certs=self.ssl_cert)

will cover only case when ssl_cert was defined

Ehm... if we create a new folder specially for SSL... why not always wrap the socket?

blynk = blynklib.ssl.Blynk(BLYNK_AUTH)

Not sure... I was thinking about copy -paste from ssl folder to root. Library name will be the same.
But maybe you have any other suggestions? We can discuss them...

or.... create a new class that extends the Blynk class?

@antohaUa
Copy link
Collaborator

antohaUa commented Oct 11, 2019

Ehm... if we create a new folder specially for SSL... why not always wrap the socket?

Yeap this was in my mind too, but not pronounced )) Sure we need to do this. Port should be changed , socket will be only ssl wrapped , msg_id =0 etc.
And now I understood your question about certificate )) - yeap we can place default to keyword parameter aka ssl_cert='blynk-cloudcom.crt' and if user want this value can be changed on init.

Micropython library staff i will remove by myself - as I said don't worry here.

@Smankusors
Copy link
Contributor Author

Smankusors commented Oct 11, 2019

ouh, I really don't have idea about this. Currently I extended the Blynk class into BlynkSSL. But if I put it in the other folder, then how can I extend from Blynk? I tried relative import but it needs to be package (which requires __init__.py file). I really don't want to copy paste, because it's redundant 😕

This is my implementation so far :

class BlynkSSL(Blynk):
    def __init__(self, token, port=443, ssl_cert=None, **kwargs):
        Blynk.__init__(self, token, port=port, **kwargs)
        import os
        if ssl_cert is None:
            self.ssl_cert = os.path.dirname(__file__) + "/_blynk-cloudcom.crt"
        else:
            self.ssl_cert = ssl_cert

    def _get_socket(self):
        try:
            self._state = self.CONNECTING
            self._socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
            self._socket.connect((self.server, self.port))
            self.log('Using SSL socket...')
            import ssl
            sslContext = ssl.create_default_context()
            sslContext.verify_mode = ssl.CERT_REQUIRED
            sslContext.load_verify_locations(cafile=self.ssl_cert)
            self._socket = sslContext.wrap_socket(sock=self._socket, server_hostname=self.server)
            self.log('Connected to blynk server')
        except FileNotFoundError as f_exc:
            raise f_exc
        except Exception as g_exc:
            raise BlynkError('Connection with the Blynk server failed: {}'.format(g_exc))

@antohaUa
Copy link
Collaborator

antohaUa commented Oct 15, 2019

Hi Smankusors,
I have intention to split libraries to cPython and mPython version.
You can commit ssl changes to https://github.com/blynkkk/lib-python/blob/master/blynklib_cp.py
Later mPython version aslo will be extracted to blynklib_mp.py

crt file please add to separate "certfificate" folder. Msg id was already corrected by me. So just please add socket wrapper and related staff. Also let's use ssl_cert=None by default if this parameter will be defined by user - only then we will switch to ssl connection mode.

About extended classes mentioned by you in prev comment : this is really good solution to software systems. If we have hardware endpoints then we should think about usability, compatibility etc.
Let's try move forward with library splitting approach.

Thx in advance for your patience.

@Smankusors
Copy link
Contributor Author

Smankusors commented Oct 15, 2019

Also let's use ssl_cert=None by default if this parameter will be defined by user - only then we will switch to ssl connection mode.

wait, how about if the user not self signed the certificate but using Let's encrypt? The public certificate usually already built in on the installed system

@antohaUa
Copy link
Collaborator

antohaUa commented Oct 15, 2019

Not understood what do you mean ... Do you have example or suggestion what we need to change?

@Smankusors
Copy link
Contributor Author

Smankusors commented Oct 15, 2019

Not understood what do you mean ... Do you have example or suggestion what we need to change?

on line 220 in this pull request, I call create_default_context() without any params. The SSLContext will then choose to trust system default CA certificate as mentioned in the documentation. Then later in the line 223, I call load_verify_locations with cafile param to load the blynk-cloud.com cert too into the SSLContext.

Note that if you're using Let's encrypt, you don't need to supply cafile param, because it's already installed on your system. That's why on the beginning, I'm using ssl param to determine if the user wants to use ssl socket or not.

@antohaUa
Copy link
Collaborator

antohaUa commented Oct 15, 2019

As I see from documentations for python2/3 we can define cafile parameter directly within create_default_context call. And there is no need in load_verify_locations line.

Still I don't wont to hardcodes and multiple parameters - so suggestion - lets use ssl_cert=None as default for none ssl mode if this parameter will have path defined we will use crt file if ssl_cert='' or "default" or smth like this lets use system default CA

@Smankusors
Copy link
Contributor Author

Smankusors commented Oct 16, 2019

Hmm which do you think is better? String "default" or const int SSL_CERT_DEFAULT? Or just blank string?

@antohaUa
Copy link
Collaborator

antohaUa commented Oct 16, 2019

For me ssl_cert="default" looks more user friendly.

Smankusors added 2 commits Oct 16, 2019
* move ssl implementation to `blynklib_cp.py`
* move cert to certificate folder
@Smankusors
Copy link
Contributor Author

Smankusors commented Oct 18, 2019

alright it's done, how about it?

@antohaUa
Copy link
Collaborator

antohaUa commented Oct 20, 2019

Hi Smankusors !
One important thing should be discussed.
For me hardcoded certificate name inside code still looks as not good enough solution.
I mean user can define file name through the ssl_cert parameter we should not store it inside library.

For default value I was thinking we were discussing system’s default CA certificates:

"ssl.create_default_context( ....

cafile, capath, cadata represent optional CA certificates to trust for certificate verification, as in SSLContext.load_verify_locations(). If all three are None, this function can choose to trust the system’s default CA certificates instead."

Also according the same documentation( https://docs.python.org/3/library/ssl.html) we can use create_default_context directly with cafile argument . So sslContext.load_verify_locations call will be not necessary.

And last one comment related to README file - we now use ssl_cert parameter - not ssl.
Please correct this
Thx in advance for your understanding. We really were close to final merge )

1 similar comment
@antohaUa
Copy link
Collaborator

antohaUa commented Oct 20, 2019

Hi Smankusors !
One important thing should be discussed.
For me hardcoded certificate name inside code still looks as not good enough solution.
I mean user can define file name through the ssl_cert parameter we should not store it inside library.

For default value I was thinking we were discussing system’s default CA certificates:

"ssl.create_default_context( ....

cafile, capath, cadata represent optional CA certificates to trust for certificate verification, as in SSLContext.load_verify_locations(). If all three are None, this function can choose to trust the system’s default CA certificates instead."

Also according the same documentation( https://docs.python.org/3/library/ssl.html) we can use create_default_context directly with cafile argument . So sslContext.load_verify_locations call will be not necessary.

And last one comment related to README file - we now use ssl_cert parameter - not ssl.
Please correct this
Thx in advance for your understanding. We really were close to final merge )

README.md Show resolved Hide resolved
blynklib_cp.py Outdated Show resolved Hide resolved
blynklib_cp.py Outdated Show resolved Hide resolved
sslContext = ssl.create_default_context()
sslContext.verify_mode = ssl.CERT_REQUIRED
if (self.ssl_cert == "default"):
caFile = os.path.dirname(__file__) + "/certificate/_blynk-cloudcom.crt"
Copy link
Collaborator

@antohaUa antohaUa Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use "default" not as hardcode for blynk cloud certificate but as system’s default CA certificates
It means calling ssl.create_default_context(caFile=None)
According this document https://docs.python.org/3/library/ssl.html

Copy link
Contributor Author

@Smankusors Smankusors Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't do.
The user don't have the blynk cloud certificate in the first place. They must dig this certificate deep on the blynk-lib for C (ESP8266). Without this, the user can't connect to the blynk-cloud.com unless they have this file.

blynklib_cp.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
blynklib_cp.py Outdated Show resolved Hide resolved
* update README.md
* move imports to the top
* fix some flow when wrapping ssl socket
@antohaUa antohaUa merged commit 62fb5ee into blynkkk:master Oct 21, 2019
@Smankusors
Copy link
Contributor Author

Smankusors commented Oct 21, 2019

alright, thanks for the merge 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants