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

Add basic SSL support for micropython #40

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

fgervais
Copy link
Contributor

@fgervais fgervais commented Dec 9, 2020

This is a partial support as the CA certificate is not validated. This
is so as this functionality is not implemented in all micropython ports.

For example, it isn't currently supported in the esp32 port although a
draft of the feature has been published:
micropython/micropython#5998

As the CA functionality is not there, we cannot use this parameter
do decide when to switch to SSL mode as done in the CPython version.
Instead we enable SSL when connecting to port 443 or 8443 which are
well known ports used for TLS communications.

One more thing is that this library relies on short reads to get data
from the socket as it always ask for the max buffer length when reading.
In micropython, the interface provided for SSL socket is only the one
of a stream which doesn't allow short reads. To go around this we change
the socket to non blocking which has the side-effect of allowing
short reads. However we then need to do manual polling and timeout on
the socket which we do here.

This change got introduced in the python/mp split commit:
81dfc16

Having it default to 1 is fine on the clear text port but doesn't
authenticate when connecting to the server through SSL.

The server will ack the token packet but won't reply anything.

Setting it back to 0 by default which mirrors what blynklib.py has.
This is a partial support as the CA certificate is not validated. This
is so as this functionality is not implemented in all micropython ports.

For example, it isn't currently supported in the esp32 port although a
draft of the feature has been published:
micropython/micropython#5998

As the CA functionality is not there, we cannot use this parameter
do decide when to switch to SSL mode as done in the CPython version.
Instead we enable SSL when connecting to port 443 or 8443 which are
well known ports used for TLS communications.

One more thing is that this library relies on short reads to get data
from the socket as it always ask for the max buffer length when reading.
In micropython, the interface provided for SSL socket is only the one
of a stream which doesn't allow short reads. To go around this we change
the socket to non blocking which which has the side-effect of allowing
short reads. However we then need to do manual polling and timeout on
the socket which we do here.
blynklib_mp.py Outdated
@@ -5,6 +5,7 @@
__version__ = '0.2.6'

import usocket as socket
import ussl as ssl
Copy link
Collaborator

Choose a reason for hiding this comment

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

ussl import can affect library usage on
some micropython ports ..
I suggest create separate file
blynklib_mp_ssl.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about another split. I mean I think even the first split was a bad idea because the common code never gets in sync anymore.

What about I import it inline in _get_socket() just before using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or.. I can have the new blynklib_mp_ssl.py which has a new class SSLConnection which inherit from class Connection.

The compromise is that people using the SSL version will need to upload 2 files instead of 1. Is that bad?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pushed new commits showing the concept with the new SslConnection class.

Blynk can still be used as usual for clear text transmissions. with no additional imports or anything.

For SSL, it is used like so:

import blynklib_mp as blynklib
import blynklib_mp_ssl as blynklib_ssl

ssl_connection = blynklib_ssl.SslConnection(token, port=443, log=print)
blynk = blynklib.Blynk(secret.BLYNK_AUTH, connection=ssl_connection)

We also don't need to switch on the port used.

What do you think?

blynklib_mp.py Outdated Show resolved Hide resolved
blynklib_mp.py Outdated Show resolved Hide resolved
blynklib_mp.py Outdated Show resolved Hide resolved
Now instead of Blynk _is_ a Connection, Blynk _uses_ a Connection.

This allows for Blynk to use different types of Connection.
This can be used like so:

ssl_connection = SslConnection(secret.BLYNK_AUTH, port=443)
blynk = blynklib.Blynk(token, connection=ssl_connection)
@antohaUa
Copy link
Collaborator

François, let's do NOT forget about users and well know python Zen aphorisms
"Simple is better than complex"
"Although practicality beats purity"

I know that it is just philosophy but ...

If iamgine that we use pure cPython your solution looks great and I always raise both hands for classes inheritance libraries patterns etc
But for micropython we have a little bit other things - resource limitations and not so good documentations, not supported modules ...

User frequently frustrated even with single lib installation procedure. What to talk about complex scenarion with two libs and inheritance + running script
So my position still the same - we should leave CPython and micropython libs as is for backward compatibility and do not affect user experience.
SSL micropython lib I suggest to add as separate library. Within this lib we can cut all functional related to NON ssl connections etc. Main goal small ssl oriented micropython lib - users if they want ssl and have support of ussl will be able to use it.

I know there will be code duplication maybe sync issues etc - but this is mycropython )) It is just on the way for stabilization/standartization
Hope after 1-2 year all will be synced and for example ussl port will be everywhere.

As so-author of course put your copyright to new ssl module and let's start use it.
Waiting for your final commit - hope my arguments will be accepted by you. Thx in advance.

@fgervais
Copy link
Contributor Author

I'm not sure I understand.

So my position still the same - we should leave CPython and micropython libs as is for backward compatibility and do not affect user experience.

With the current PR they are the same. The micropython library work the same internally and is used the same way. No backward compatibility problem.

It is still used as shown in README.md i.e.

import blynklib_mp as blynklib

blynk = blynklib.Blynk(BLYNK_AUTH)

Also the micropython lib does not import anything new.

There is a small new blynklib_mp_ssl.py library as you requested.

I feel the PR as it stands is well aligned with you requests.

@antohaUa
Copy link
Collaborator

To be honest schema provided by you will work ...
but we add additional complexity into existing mp library without enhancing it's functionality just increasing it's size that also may be critical for low memory systems.

New SSL oriented separate module will give more profit:
All users from the beginning know if their HW supports ussl and is they want to use ssl connection.
So they simply will choose what lib should be written to HW memory common MP module or MP ssl module.

Yeap we have code duplication - but I am temporary ok with this - want to wait untill ussl will be accepted and used across all new HW.
Until this time SSL lib for micropython is just POC or prototype - better to place it as separate module without modifying existing libraries.

@fgervais
Copy link
Contributor Author

Alright I see your position.

I'm sorry I cannot comply with these requests.

This PR can be dismissed, thank you.

@antohaUa
Copy link
Collaborator

François, sorry from my side also - lib active development was finished by me year+ ago.
Thus main focus for now only stability and support. Sometimes this can block new features ...
Anyway as I said previously thanks for your investigation and contribution - hope WE can be lucky with next PRs acceptance.

For now I will leave this branch - it might be useful for users.

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.

2 participants