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

TLS connection model #1700

Closed
djphoenix opened this issue Jan 3, 2017 · 13 comments
Closed

TLS connection model #1700

djphoenix opened this issue Jan 3, 2017 · 13 comments
Labels

Comments

@djphoenix
Copy link
Contributor

And hello again!

So after net module was rewrote with plain LWIP, the next target is secure connections. And there are very complex things to do.

Everyone knows what is espconn... So, without it, we can't uniform secure and plain connections anymore. And we should find a simple, stable and reusable solution to get it back.

Proposal

This is my look on problem. Comments are welcome.

  1. Now we are have CLIENT_SSL_ENABLE definition and tls module. I propose to merge them together, so tls module enables SSL globally.
  2. Since tls module are entry point for SSL, write connection wrapper in it (that establishes random pool, CA store, verification, contexts, and more).
  3. Define something like tls_pcb *tcp_pcb_wrap_tls(tcp_pcb *pcb) that wraps plain TCP connection into TLS and manages callbacks like original. So we achieve unified callbacks for receive, sent, disconnection/error callbacks. Also, of course, define send method for encrypted connection.

After this part, there will be an options like add custom certificate validation (e.g. against public key), and more. But outgoing connection "like it works before" is main goal.

@jmattsson
Copy link
Member

  1. Sounds good to me!
  2. It might be better to have such a wrapper outside, in e.g. app/tlswrap so that it can easily be used by e.g. the http client.
  3. Unified is good.

@pjsg you looked at the old-SDK TLS/CA stuff, are there any key points you learned from that which we should make sure we're aware of right from the get-go?

@djphoenix
Copy link
Contributor Author

@jmattsson agree for move wrapper code outside lua-module, I was think about it anyway, but forget to note it.

@pjsg
Copy link
Member

pjsg commented Jan 5, 2017

The old cert stuff was horrible -- and I never managed to make the client cert stuff work right.

The fact that you had to put the certs directly into flash (on a 4k boundary) and then point the code at them was nasty.

There wasn't a way of generating a key pair, so you had to construct the cert/private key on another system and then insert it into the firmware image. It was then partially hidden which was a good thing. The trouble is that you can never hide secrets properly on the ESP8266.

Also, there was no support for building an SSL/TLS server -- this would have been useful to be able to build an HTTPS server.

@marcoskirsch
Copy link

Also, there was no support for building an SSL/TLS server -- this would have been useful to be able to build an HTTPS server.

So this will be made possible?

@djphoenix
Copy link
Contributor Author

djphoenix commented Jan 5, 2017

The fact that you had to put the certs directly into flash (on a 4k boundary) and then point the code at them was nasty.

For mbedTLS it is not necessary.

So this will be made possible?

Yep, but step by step.

@marcelstoer
Copy link
Member

marcelstoer commented Jan 5, 2017

  1. Now we are have CLIENT_SSL_ENABLE definition and tls module. I propose to merge them together, so tls module enables SSL globally.

👍 but I (again) propose to do it the other way around.

SSL is a cross-cutting concern i.e. the implications go beyond a single module. It feels thus more logical to me to enable SSL via a global flag. It's like choosing between integer or floating point support. The fact that this cross-module SSL support is implemented by adding a dedicated module and a wrapper is an implementation detail in my opinion.

Disclaimer: I'm of course biased because I primarily look at this as the maintainer of the cloud builder. I had to jump through a few hoops to give the users this SSL flag and include the TLS module transparently behind the scenes without it ever showing up on the modules list or the stats.

@jmattsson
Copy link
Member

  1. Now we are have CLIENT_SSL_ENABLE definition and tls module. I propose to merge them together, so tls module enables SSL globally.

👍 but I (again) propose to do it the other way around.

Yes, this does make more sense. Should be trivial to do, in the tls.c file just:

#ifdef CLIENT_SSL_ENABLE
# define LUA_USE_MODULES_TLS
#endif

before including module.h and the module macro should do its thing.

@djphoenix
Copy link
Contributor Author

Check out my tlswrap branch for some progress

@stgoddv
Copy link

stgoddv commented Oct 13, 2017

Anyone knows if today the secure parameter of MQTT runs mqtt over tls 1.2?
I think that this issue was discussed on 2016, and the conclusion was that the libraries at that time
could only stand for tls 1.1. So the conclusion was that mqtt over tls 1.2 was not possible.
Then, it appears that the net module enabled tls1.2 but no one confirmed the compatibility of
tls 1.2 with mqtt. Anyone knows if mqtt now runs over tls1.2? Thanks!

@stale
Copy link

stale bot commented Jul 21, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 21, 2019
@nwf
Copy link
Member

nwf commented Jul 21, 2019

It has, by now, long been the case that TLS1.2 is enabled in our mbedTLS configuration and that mqtt secure connections will attempt to use it. As usual, the use of ECC certificates helps with memory pressure, but the use of LFS is essentially mandatory.

@nwf nwf closed this as completed Jul 21, 2019
@nwf
Copy link
Member

nwf commented Apr 19, 2020

I would very much like to dust this off. I'm sorry I closed it; that was in error or out of incompetence on my part. @djphoenix's tlswrap branch would be an excellent thing to land, especially in light of #3006 .

@nwf nwf reopened this Apr 19, 2020
@stale stale bot removed the stale label Apr 19, 2020
@stale
Copy link

stale bot commented Jun 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 2, 2021
@stale stale bot closed this as completed Jun 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants