Feature: ssl abstraction #1999

slaff · 2019-12-12T21:31:59Z

This is the initial draft for a SSL abstraction that will allow Sming users to change seamlessly between different SSL implementations and without the need to recompile the whole framework.

This PR is still Work-In-Progress(WIP).

Related to #1993 and #1713.

We now have a single ssl Component which contains code to use the selected implementation. It generates a library variant as required, including when SSL is disabled. As the public interface is fixed, no recompilation of the framework is required when switching between implementations.

mikee47 · 2019-12-13T13:55:40Z

Following on from slaff#40

Instead of global functions, how about a factory class instead with createExtension(), createContext(), etc? This could be a global pointer SslImplementation* sslImpl which is null if SSL isn't enabled, then we could get rid of need for a dummy SSL component.

Further, let's shift everything into the project. Get rid of ENABLE_SSL, etc. then add this to the Project's component.mk file:

COMPONENT_DEPENDS += AxtlsSsl

The application then explicitly registers an SSL implementation:

// We'd like to use AXTLS thankyouverymuch.
static AxtlsSsl sslImpl;

void init()
{
  ...
  // Internally this sets a pointer to the implementation
  sming::ssl::register(&sslImpl);
  ...
}

If no implementation is registered, or it's null, then no SSL is used.

Initially I had axtls-8266 and the AxtlsSsl implementation in one component. But then decided against it. axtls-8266 is containing Ssl functions and cryptographic functions, that are quite useful. If someone wants to use the cryptographic functions from axtls-8266 but prefer to have dummy ssl implementation having AxtlsSsl as a separate component makes this possible.

With the above approach, we can add multiple SSL Components to a project and pick the one to use, whilst allowing access to the code from other libraries. That means there's no problem having axtls-8266 contained within AxtlsSsl which I feel is the correct approach.

Future work ~~might~~ will include additional abstractions for the crypto stuff to avoid project's being tied to any specific implementation.

~~Finally, now that AxtlsSsl is fully decoupled from the Sming framework it can be moved into Libraries.~~ Nope, part of the main framework even though it's decoupled.

mikee47

@slaff Apologies for the length of this review. I think you're much more familar with SSL and web protocols in general than me so happy you've taken the lead on this one. Happy to help with any of this stuff.

Sming/Components/AxtlsSsl/src/SslCertificateImpl.h

Sming/Components/AxtlsSsl/src/SslConnectionImpl.h

Sming/Components/AxtlsSsl/src/SslContextImpl.h

mikee47 · 2019-12-13T15:48:26Z

Sming/Core/Network/Ssl/SslCrypto.h

+extern "C" {
+#endif
+
+void hmac_md5(const uint8_t* msg, int length, const uint8_t* key, int key_len, uint8_t* digest);


Too many parameters for an interface function, not strongly typed, no length for digest parameter.

I'd lose the extern "C" as a minimum, can always use an inline or templated wrapper on top of the library implementation(s).

All these MD5 functions are provided in the ESP ROM. This is a good place to bring them out, only need to use axtls for Host support. See also #1999.

Sming/Core/Network/TcpConnection.cpp

Sming/Core/Network/TcpConnection.h

samples/Echo_Ssl/app/application.cpp

samples/HttpClient/app/application.cpp

mikee47 · 2019-12-13T16:56:16Z

Is there any accepted standard for C++ crypto/SSL library interfaces? I've found https://github.com/weidai11/cryptopp and there are likely boost libraries around.

General question: What sort of SSL/crypto code might we want to run on Sming? What's it written for? How can we simplify porting?

Also re. namespaces. A quick Sming search reveals ContentType, MallocCount, FSTR, NanoTime, PolledTimer, Json, RingTone, Profiling. So it seems logical that we'll pick Sming::Ssl, reserving lowercase convention for the STL.

If we end up namespacing everything in Sming (then using aliases for backward-compatibility) then it might even end up being Sming::Tcp::Ssl. It would then be logical to have all the classes renamed such as Tcp::Client, Tcp::Connection, etc. And would we have Tcp::Http::Connection or just Http::Connection?

Sming/Core/Network/Ssl/Interface/SslExtension.h

Sming/Components/AxtlsSsl/src/SslContextImpl.cpp

Sming/Components/AxtlsSsl/src/SslExtensionImpl.h

Sming/Components/AxtlsSsl/src/SslContextImpl.cpp

slaff · 2019-12-18T20:10:07Z

@mikee47 if you want take this PR and develop it further. I like your suggestions a lot. With the speed and spare time that I have it will take me ages to finish it (or at least not before February, 2020).

mikee47 · 2019-12-18T20:26:15Z

@slaff OK. If you want to push any outstanding stuff to your branch we can work from there?

slaff · 2019-12-19T07:36:48Z

If you want to push any outstanding stuff

Just take it as it is. Create a clone in your own repository of this branch. After that close this PR and open a new PR with your branch.

mikee47 · 2019-12-20T16:58:32Z

@slaff Could you rebase your PR? Made some progress.

slaff · 2019-12-20T17:31:16Z

Could you rebase your PR?

Yep, should be ready for you.

slaff · 2019-12-21T20:39:37Z

The lwirax compatibility layer is now probably surplus to requirements - we don't need two compatibility layers - so remove this and work with axtls-8266 directly.

Lwirax is the "glue" between LWIP raw (async) mode and axTLS. We still need it.

mikee47 · 2019-12-21T20:58:35Z

The lwirax compatibility layer is now probably surplus to requirements - we don't need two compatibility layers - so remove this and work with axtls-8266 directly.

Lwirax is the "glue" between LWIP raw (async) mode and axTLS. We still need it.

I've integrated the necessary bits into ConnectionImpl.cpp. If you could rebase when #2009 is merged and I'll push to your repo. from https://github.com/mikee47/Sming/tree/feature/ssl/no-lwirax

slaff · 2019-12-21T20:58:47Z

@mikee47 Thanks a lot for the Session.cpp/h implementation! With your help the SSL abstraction might arrive earlier than expected :)

slaff · 2019-12-21T21:01:28Z

If you could rebase when #2009 is merged

Done.

slaff · 2019-12-21T21:16:10Z

fix make cs - todo: how to make this pluggable?

We can just add .cs file with 0 bytes to every directory where the the coding style rules should be applied. The coding style rules should be valid for the requested directories AND their sub-directories. And then with a simple find $SMING_HOME/../ -name '.cs' -type f command we should be able to get a list of all directories where the CS rules should be applied.

mikee47 · 2019-12-21T21:31:55Z

@slaff Thanks! That would never have ocurred to me in a milllon years. LOL.

I'm looking at BearSSL now using the esp8266-arduino directly seems best.

mikee47 · 2019-12-21T21:42:41Z

Codacy warning: should have used delete[] instead of delete. Will fix it in next PR.

mikee47 · 2019-12-22T08:12:51Z

@slaff Have you any pointers for an SSL server sample application? As far as I can see the samples are all client-focussed.

slaff · 2019-12-22T11:51:19Z

Have you any pointers for an SSL server sample application?

Take one of the HttpServer samples and try to make the following changes: https://github.com/SmingHub/Sming/wiki/How-to-use-SSL-in-HttpServer

mikee47 · 2019-12-27T14:38:22Z

We need a mechanism for applications to configure all the SSL session options. At present we have a few items stored within an HttpRequest: sslOptions, sslFingerprints and sslKeyCertPair. These all end up in the Session anyway,so a better way I think would be to replace those with a single callback delegate, something like this:

using SslInitDelegate = Delegate<void(Ssl::Session& session, void* param)>.

Applications then set:

request->onSslInit(mySslInitCallback, &request).

Or something similar. This gets called by the SSL layer before a connection is created.

It's difficult to tell what additional options we might want to set in the future, so something like this means we only need to update Session to support those and expose them for application use.

…ompile the complete network stack.

…at we might want to use axtls-8266 as crypto library without having to use it as a SSL implementation.

* Review changes 1.2 Omit `virtual` from overridden destructors Change `decrypted` parameter a `pbuf*&` (instead of pbuf**) Remove default parameter values from implementation Use enums in preference to #define * Rationalise header files, remove `Interface` sub-directory and `SslStructs.h`, delete `DummySsl` Component * Add Ssl namespace. * Add `Ssl::Session` to contain and manage session information Note: Must free tcp *after* context * Move all the SSL stuff into a separate Component, using a variant to select the implementation. * Add debug print support for Connection, simplify classes. Add `Ssl::Connection::printTo()` Move code into header Add `CipherSuite` definitions.

* fix `make cs` - todo: how to make this pluggable? * Update axtls-8266 to head, and copy compat read/write functions * Integrate read/write routines from lwirax Rework and simplify read/write routines working directly with axTLS. lwirax not required - fd_client is cast to a ConnectionImpl* Don't need to create `extension` on heap, just make it a member of `Session` Create certificate object on demand * Rename Session methods and remove redundant tcp parameter

* Fix codacy warning - used `delete` instead of `delete[]`. * Fix whole-file patching to include sub-directories * Fix pgmspace.h flies Add __cplusplus guard to Core/pgmspace.h so it can be #included from C source Add missing #includes to sys/pgmspace.h * Remove superfluous Ssl:: qualifiers * Move validators into Session, add `beginHandshake()` and `endHandshake()` methods. * Remove `addSslValidator` and `pinCertificate` methods from TcpClient. HttpClientConnection uses Ssl::Session methods instead. * Add `Connection::freeCertificate()` and use `freeKeyCertAfterHandshake` setting to determine lifetime. For client applications this means the certificate remains available for the completion callback. * Simplify axtls port_read() - don't need intermediate buffer * Rename AXTLS files and classes using `Ax` prefix, remove Connection::calcWriteSize() method. * Move read method into Connection, add readTcpData(), writeTcpData() and encrypt() methods Using same algorithm for BR * Fix validator (wrong method called for PKI) * Move keyCert handling into AxContext, add Connection::getErrorString() method * Free tcp buf in Connection, prior to allocating return buffer * Add AxError strings Using FlashString Map with minor bugfix * Add Bear SSL implementation * Handle handshake completion and verification using callback to Session. Context contains session reference plus tcp. Don't use error return from decrypt() to detect handshake Session resumption handled within Session class * Working with Basic_SSL sample * Get rid of Extension class, move settings into Session. * Sort X509 callback * Simplify Context interface Don't need parameters or `setKeyCert` method, just pull required settings from session Define `Option` enum for session * Fixup `SSL_DEBUG` handling and add alert code string table * Fix 16K block handling - don't attempt to reassemble tcp data * Move Alert/CipherSuite stuff into separate modules, add comment headers * Dynamically allocate buffer according to fragmentSize setting * Fix AXTLS abnormal termination * Move error string stuff into separate module and incorporate AX alert codes * Add callback mechanism for configuring SSL session * Add overload for KeyCertPair::assign() to handle Strings * Copy `make_certs.sh` into ssl Component, keep generated certificates in `cert` directory * Add instance pointer to TCP connection debug messages Cannot debug server code because 2 connections get created (with or without SSL) * Refactor TcpConnection::write Simplify and get rid of inner loop * Increase host TCP_MSS to 1390 * Add `TcpConnection::trySend()` * Implement SSL server connections using HttpServer_ConfigNetwork sample Load certificate and private key as binary data from flash SSL Init handler moved into TcpConnection class Session initialised when TCP connection is accepted (not on listen) Don't cache connection state in Session, just complicates things * Add `getAlert` method for introspection getErrorString and getAlert methods should probably go into the factory (stateless) * Add BrServerConnection class Split out into separate modules with common base BrConnection class

* Don't generate certificate if SSL is disabled * Invoke `sslInitSession` from within `sslCreateSession`. * Enable information/debug messages only when SSL_DEBUG is defined * Further simplify TcpConnection::write * Add certificate DN parsing for BR implementation Revise interface to use standard DN and RDN values

…r ones...

* Fix codacy issues * Fix BRSSL server `BrConnection::write` may write less than the amount requested, but that's fine. Also require bi-directional buffer for server operation. * Don't fail if send buffer full * Don't bother calculating DN hash until requested * Re-instate `writeTcpData()` 'message, but as debug.

mikee47 · 2020-01-02T15:59:30Z

OK, I've pushed some changes to your repo. Checked the documentation build and made some changes to that so it's all in the SSL component.

* Revert changes to FlashString * Documentation/comment updates/fixes * Trying to be consistent with deprecated types * Remove redundant code * Proposed simplification to `state` check in `TcpClient::close()` * Get rid of `sslSessionCacheSize` in HttpServerSettings, and associated `sslInitSession` override If necessary, this value can be changed in application's sslInit callback. * Move trivial methods into header

Sming/Components/ssl/include/Network/Ssl/Session.h

The purpose of this value is allow applications to limit the buffer size and provide some control over RAM usage. It is up to the SSL implementation/adapter how this is used. At present we have max_fragment_length https://tools.ietf.org/html/rfc6066 but this will change to record_size_limit https://tools.ietf.org/html/rfc8449

New features ------------------ - No-WiFi build option SmingHub#2004 - get more resources if your application is not using WIFI. - Multiple SSL adapters based on axTLS and BearSSL. SmingHub#1999 - Added basic Crypto support library SmingHub#2014 - Updates framework to build using GCC 9.2.0 toolchain for C++17. SmingHub#1825 - Modbus master SmingHub#1992 - Implemented Small String Optimisation (SSO). SmingHub#1951 - Webcam stream and sample webcam web server. SmingHub#1981 - Allow HTTP connections to ignore rejected body content SmingHub#1928 Improvements ------------------- - Some improvements to multipart parser SmingHub#2007 - Update ArduinoJson to 6.13.0 SmingHub#1979 - Added precaching from Arduino for ESP8266. SmingHub#1965 - Add support for 'Expect: 100-continue' in HTTP server. SmingHub#1931 - Upgrade to FlashString Library SmingHub#1974, SmingHub#2013 Bug fixes ------------- - Updated mqtt-codec to allow publish messages without payload. SmingHub#1976 - HttpConnection freed twice. SmingHub#1938 - Hangs at startup when custom heap enabled. SmingHub#1996 - Fix issues reported by valgrind SmingHub#2017 Breaking changes and Migration ------------------------------------------- - See our [dedicated page](https://sming.readthedocs.io/en/latest/upgrading/4.0-4.1.html) for migration from 4.0.0 to 4.1.0. All PRs scheduled for this release can be seen from [here](https://github.com/SmingHub/Sming/milestone/23)

slaff force-pushed the feature/ssl-abstraction branch from 68dde5f to 530bdd9 Compare December 12, 2019 21:37

mikee47 reviewed Dec 13, 2019

View reviewed changes

mikee47 reviewed Dec 16, 2019

View reviewed changes

Sming/Core/Network/Ssl/Interface/SslExtension.h Outdated Show resolved Hide resolved

mikee47 reviewed Dec 16, 2019

View reviewed changes

Sming/Components/AxtlsSsl/src/SslContextImpl.cpp Outdated Show resolved Hide resolved

mikee47 reviewed Dec 16, 2019

View reviewed changes

Sming/Components/AxtlsSsl/src/SslExtensionImpl.h Outdated Show resolved Hide resolved

mikee47 reviewed Dec 18, 2019

View reviewed changes

Sming/Components/AxtlsSsl/src/SslContextImpl.cpp Outdated Show resolved Hide resolved

mikee47 reviewed Dec 18, 2019

View reviewed changes

Sming/Components/AxtlsSsl/src/SslContextImpl.cpp Outdated Show resolved Hide resolved

mikee47 self-assigned this Dec 19, 2019

mikee47 mentioned this pull request Dec 19, 2019

OTA upgrade as a component #2006

Merged

slaff force-pushed the feature/ssl-abstraction branch from 705ea5c to bd24d6a Compare December 20, 2019 17:30

mikee47 assigned slaff Dec 21, 2019

slaff force-pushed the feature/ssl-abstraction branch from feb4966 to 2b38288 Compare December 21, 2019 21:01

SmingHub deleted a comment from kpishere Dec 25, 2019

slav-at-attachix and others added 21 commits January 2, 2020 16:30

Initial ideas... creating chaos for now...

9ee8a72

Trying to change SSL versions in application without the neede to rec…

78b8564

…ompile the complete network stack.

Moved AxtlsSsl to a separate component. The reasoning behind it is th…

c8d20ad

…at we might want to use axtls-8266 as crypto library without having to use it as a SSL implementation.

Coding style fixes.

7a00c36

Try to prevent rebuild of Sming when SSL implementation is changed.

bc4342f

Fixes to the other SSL samples.

2d73e4a

Added documentation.

5c576a8

Prevent framework rebuild. Thanks @mikee47.

d4d5c4d

Moved destructors to top as suggested.

2b49e6b

Ssl factory abstraction.

ee61452

Suggested changes part 1.

ac189f7

Suggested changes part 1.1

0fc9231

Removed unused headers.

3649e91

Made server mode to work for small pages. Still not working for bigge…

965517f

…r ones...

Documentation and small changes.

6a8922d

The SSL component should follow the coding style rules too.

54525a7

slaff force-pushed the feature/ssl-abstraction branch from e916c6f to 54525a7 Compare January 2, 2020 15:36

mikee47 reviewed Jan 2, 2020

View reviewed changes

Sming/Components/ssl/include/Network/Ssl/Session.h Outdated Show resolved Hide resolved

slaff merged commit 41d03e8 into SmingHub:develop Jan 3, 2020

slaff removed the 3 - Review label Jun 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: ssl abstraction #1999

Feature: ssl abstraction #1999

slaff commented Dec 12, 2019 •

edited

mikee47 commented Dec 13, 2019 •

edited

mikee47 left a comment

mikee47 Dec 13, 2019

mikee47 Dec 19, 2019

mikee47 commented Dec 13, 2019

slaff commented Dec 18, 2019

mikee47 commented Dec 18, 2019

slaff commented Dec 19, 2019

mikee47 commented Dec 20, 2019

slaff commented Dec 20, 2019

slaff commented Dec 21, 2019

mikee47 commented Dec 21, 2019

slaff commented Dec 21, 2019

slaff commented Dec 21, 2019

slaff commented Dec 21, 2019

mikee47 commented Dec 21, 2019

mikee47 commented Dec 21, 2019

mikee47 commented Dec 22, 2019

slaff commented Dec 22, 2019

mikee47 commented Dec 27, 2019

mikee47 commented Jan 2, 2020

Feature: ssl abstraction #1999

Feature: ssl abstraction #1999

Conversation

slaff commented Dec 12, 2019 • edited

mikee47 commented Dec 13, 2019 • edited

mikee47 left a comment

Choose a reason for hiding this comment

mikee47 Dec 13, 2019

Choose a reason for hiding this comment

mikee47 Dec 19, 2019

Choose a reason for hiding this comment

mikee47 commented Dec 13, 2019

slaff commented Dec 18, 2019

mikee47 commented Dec 18, 2019

slaff commented Dec 19, 2019

mikee47 commented Dec 20, 2019

slaff commented Dec 20, 2019

slaff commented Dec 21, 2019

mikee47 commented Dec 21, 2019

slaff commented Dec 21, 2019

slaff commented Dec 21, 2019

slaff commented Dec 21, 2019

mikee47 commented Dec 21, 2019

mikee47 commented Dec 21, 2019

mikee47 commented Dec 22, 2019

slaff commented Dec 22, 2019

mikee47 commented Dec 27, 2019

mikee47 commented Jan 2, 2020

slaff commented Dec 12, 2019 •

edited

mikee47 commented Dec 13, 2019 •

edited