Offloaded TLSSocket and BG96 support for it #11357

kivaisan · 2019-08-28T06:41:32Z

Description

Some external modems have an internal TLSSocket implementation which can be used instead of mbedtls based TLSSocket. Using offloaded TLSSocket can result in significantly reduce ROM usage.

Offloaded TLSSocket can be enabled by enabling "nsapi.offload-tlssocket" and the used network stack (e.g. cellular modem's CellularStack class) must support the setsockopt's defined in nsapi_types.h.

Compared to original mbedtls based TLSSocket, offloaded TLSSocket brings in one significant API limitation. Offloaded TLSSocket requires setting of certificates and keys after open() and before connect() calls, where mbedtls based TLSSocket allows setting these before open() call.

This PR also includes a reference implementation for BG96 cellular modem.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@AnttiKauppila @kjbracey-arm

Release Notes

Some external modems have an internal TLSSocket implementation which can be used instead of mbedtls based TLSSocket. Using offloaded TLSSocket can result in significantly reduce ROM usage.

Offloaded TLSSocket can be enabled by enabling "nsapi.offload-tlssocket" and the used network stack (e.g. cellular modem's CellularStack class) must support the setsockopt's defined in nsapi_types.h.

Compared to original mbedtls based TLSSocket, offloaded TLSSocket brings in one significant API limitation. Offloaded TLSSocket requires setting of certificates and keys after open() and before connect() calls, where mbedtls based TLSSocket allows setting these before open() call.

This PR also includes a reference implementation for BG96 cellular modem.

ciarmcom · 2019-08-28T07:00:26Z

@kivaisan, thank you for your changes.
@AnttiKauppila @kjbracey-arm @ARMmbed/mbed-os-test @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

AnttiKauppila · 2019-08-28T07:09:05Z

features/cellular/framework/AT/AT_CellularStack.h

@@ -118,6 +119,7 @@ class AT_CellularStack : public NetworkStack, public AT_CellularBase {
        bool started; // socket has been opened on modem stack
        bool tx_ready; // socket is ready for sending on modem stack
        bool rx_avail; // socket has data for reading on modem stack
+        bool tls_socket;


Where is this used

I'll add similar description as other fields. This is internally used by AT_CellularStack to define if socket is offloaded TLSSocket.

AnttiKauppila · 2019-08-28T07:10:04Z

features/cellular/framework/targets/QUECTEL/BG96/QUECTEL_BG96_CellularStack.cpp

-        if ((_at.get_last_error() == NSAPI_ERROR_OK) && err) {
-            if (err == BG96_SOCKET_BIND_FAIL) {
-                socket->id = -1;
+        if (socket->tls_socket) {


What is the value of this when using MbedTLS for TLSSocket?

It is set to false for mbedtls based TLSSocket because in that case socket looks like a normal TCP socket from CellularStack point of view.

AnttiKauppila · 2019-08-28T07:18:32Z

Will this work with MbedTLS anymore or should you encapsulate the BG96 SSL code inside MBED_CONF_NSAPI_OFFLOAD_TLSSOCKET macro?

michalpasztamobica · 2019-08-28T07:08:57Z

TESTS/netsocket/tls/tlssocket_endpoint_close.cpp

@@ -48,6 +48,10 @@ static nsapi_error_t _tlssocket_connect_to_daytime_srv(TLSSocket &sock)
        return err;
    }

+    TEST_ASSERT_EQUAL(NSAPI_ERROR_OK, sock.set_root_ca_cert(tls_global::cert));
+
+    sock.set_timeout(10000); // Set timeout for case TLSSocket does not support peer closed indication


Don't we also need this in other functions connecting to various servers, for example here?

I think there is some room for little refactoring in here to use common method for opening the server connection in different test cases, but I think that should be done as a separate task.

michalpasztamobica · 2019-08-28T07:17:25Z

features/netsocket/TLSSocket.cpp

@@ -46,5 +48,71 @@ TLSSocket::~TLSSocket()
     */
    close();
 }
+#endif


Please, add the // MBEDTLS_SSL_CLI_C for readability.

michalpasztamobica · 2019-08-28T07:22:45Z

features/netsocket/TLSSocket.h

+     *
+     * @param root_ca Root CA Certificate in any Mbed TLS-supported format.
+     * @param len     Length of certificate (including terminating 0 for PEM).
+     * @return        0 on success, negative error code on failure.


Should we stick to the NSAPI_ERROR_OK convention, rather than disclose its value 0?

Good finding. I'll provide a fix.

michalpasztamobica · 2019-08-28T07:40:24Z

features/netsocket/mbed_lib.json

@@ -65,6 +65,10 @@
        "socket-stats-max-count": {
            "help": "Maximum number of socket statistics cached",
            "value": 10
+        },
+        "offload-tlssocket" : {


Just a thought here, please reject if "offload" is in fact a widely used name for what you are trying to achieve :)
To me it sounds like "offload" is a potential gain for the user, as they offload the memory and processor (and probably increase the load on the external modem?) but doesn't exactly indicate the main choice that user makes with this flag.
I wonder if something like "external" (from mbed-os perspective) or "embedded" would better suit the purpose?
Are there any reasons, different than offloading the main core, that would make users switch this on?

@AnttiKauppila , @kjbracey-arm, @lauri-piikivi what do you think about the term "offloaded"? The same term was already used for DNS:

mbed-os/features/cellular/mbed_lib.json

Line 24 in 4e10e98

"offload-dns-queries" : {

kivaisan · 2019-08-28T13:43:48Z

Will this work with MbedTLS anymore or should you encapsulate the BG96 SSL code inside MBED_CONF_NSAPI_OFFLOAD_TLSSOCKET macro?

If offloaded TLSSocket is enabled, mbedtls socket cannot be used. This was discussed and at the moment there is no requirement to support both at the same time. Supporting both at the same time would require API break, or a different class name for offloaded TLSSocket.

0xc0170 · 2019-08-29T11:49:09Z

@kivaisan Is this targeting 5.14 ? It has not been labeled. Please let us know asap.

If it is, we should have reviews done today to get into CI

AnttiKauppila · 2019-08-29T12:01:26Z

Will this work with MbedTLS anymore or should you encapsulate the BG96 SSL code inside MBED_CONF_NSAPI_OFFLOAD_TLSSOCKET macro?

If offloaded TLSSocket is enabled, mbedtls socket cannot be used. This was discussed and at the moment there is no requirement to support both at the same time. Supporting both at the same time would require API break, or a different class name for offloaded TLSSocket.

The question was if this still work with MbedTLS like previously (not simultaneously with offload)

kivaisan · 2019-08-29T12:29:16Z

The question was if this still work with MbedTLS like previously (not simultaneously with offload)

Yes. Offloading is an optional feature and is disabled by default. In case of mbedtls based TLSSocket cellularstack handles the socket as a normal TCP socket.

kivaisan · 2019-08-29T12:30:38Z

@kivaisan Is this targeting 5.14 ? It has not been labeled. Please let us know asap.

If it is, we should have reviews done today to get into CI

@AnttiKauppila , what do you think?

AnttiKauppila · 2019-08-29T15:39:26Z

This is not in hurry for 5.14

AriParkkila · 2019-08-30T08:29:55Z

TESTS/netsocket/tls/tlssocket_handshake_invalid.cpp

    SKIP_IF_TCP_UNSUPPORTED();
    TLSSocket sock;
    TEST_ASSERT_EQUAL(NSAPI_ERROR_OK, sock.open(NetworkInterface::get_default_instance()));
    TEST_ASSERT_EQUAL(NSAPI_ERROR_OK, sock.set_root_ca_cert(tls_global::cert));
-    TEST_ASSERT_EQUAL(NSAPI_ERROR_AUTH_FAILURE,
-                      sock.connect("google.com", 443)); // 443 is https port.
+    TEST_ASSERT_EQUAL(NSAPI_ERROR_AUTH_FAILURE, sock.connect("www.mtvuutiset.fi", https_port));


Use a local/CI host.

Sorry, I didn't understand your comment. Could you please describe more?

@AriParkkila , was there something you requested for this?

How about using e.g. example.com here instead of www.mtvuutiset.fi.

Example.com does not work either. Probably uses some CA root already included in the BG96 modem. But I removed mtvuutiset.fi as well. I think those other handshake tests verifies the needed functionality.

0xc0170 · 2019-09-03T12:07:53Z

@kivaisan Please add Release notes section above for [X] Functionality change

0xc0170 · 2019-09-03T12:28:16Z

I would also suggest adding more details to the commits. There are one liners for functionality changes, provide more information there.

0xc0170

Commit msg update and release notes section provide

kivaisan · 2019-09-10T05:36:38Z

I updated commit messages (and also squashed some) and also added some release note description.

0xc0170 · 2019-09-25T10:18:00Z

@AnttiKauppila Please approve this functionality change

kivaisan · 2019-09-25T10:33:54Z

Add possibility to use network stack's internal TLS socket instead of mbedtls based TLS socket. This requires support from network stack (for example cellular modem driver) and reference BG96 implementation is provided.

how to use it? Is it via config or? Can we provide more details to the release notes?

I updated release notes and also description a bit about usage.

SeppoTakalo · 2019-09-25T11:29:47Z

Please switch the content of your Release Notes and Description sections as it makes much more sense to have full explanation in public facing documentation instead of closed PRs.

Also, add some notes about security implications, because offloading is a potential security risk, if modem is connected by unprotected wires to main CPU. It allows man-in-the-middle attacks using just soldering iron and TTL-serial adapter.

SeppoTakalo · 2019-09-25T12:53:05Z

Please update the design document as well: https://github.com/ARMmbed/mbed-os/blob/master/docs/design-documents/features/connectivity/TLSSocket%20-%20Design%20document.md

0xc0170

To answer @SeppoTakalo requests above

kivaisan · 2019-09-26T12:35:05Z

Please switch the content of your Release Notes and Description sections as it makes much more sense to have full explanation in public facing documentation instead of closed PRs.

I disagree. I think description should be more detailed info than release note entry.

Also, add some notes about security implications, because offloading is a potential security risk, if modem is connected by unprotected wires to main CPU. It allows man-in-the-middle attacks using just soldering iron and TTL-serial adapter.
Please update the design document as well: https://github.com/ARMmbed/mbed-os/blob/master/docs/design-documents/features/connectivity/TLSSocket%20-%20Design%20document.md

I think note about security concern can be part of this documentation, not commit description.

0xc0170 · 2019-10-01T13:57:53Z

@kivaisan We are improving the PR template, there will be more subsections. We should provide more details in the release notes as we have provided so far.

I'll schedule CI now

0xc0170 · 2019-10-01T13:58:13Z

CI started

mbed-ci · 2019-10-01T14:52:14Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

AnttiKauppila · 2019-10-08T11:38:37Z

@adbridge Why is this marked now for 6.0? What is the reasoning and who demanded this?

ciarmcom requested review from AnttiKauppila, kjbracey and a team August 28, 2019 07:00

ciarmcom added the needs: review label Aug 28, 2019

AnttiKauppila reviewed Aug 28, 2019

View reviewed changes

michalpasztamobica reviewed Aug 28, 2019

View reviewed changes

AriParkkila reviewed Aug 30, 2019

View reviewed changes

AriParkkila approved these changes Aug 30, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Sep 3, 2019

0xc0170 requested changes Sep 3, 2019

View reviewed changes

michalpasztamobica approved these changes Sep 10, 2019

View reviewed changes

0xc0170 added needs: review and removed needs: work labels Sep 10, 2019

0xc0170 approved these changes Sep 10, 2019

View reviewed changes

AnttiKauppila approved these changes Sep 25, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Sep 25, 2019

0xc0170 requested changes Sep 25, 2019

View reviewed changes

0xc0170 self-requested a review September 26, 2019 13:52

0xc0170 added needs: review release-version: 5.15.0-rc1 and removed needs: work labels Sep 26, 2019

0xc0170 approved these changes Oct 1, 2019

View reviewed changes

adbridge added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 needs: CI ready for merge and removed release-version: 5.15.0-rc1 needs: review needs: CI labels Oct 3, 2019

adbridge merged commit eeb033f into ARMmbed:master Oct 3, 2019

adbridge removed the ready for merge label Oct 3, 2019

adbridge added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Oct 10, 2019

adbridge mentioned this pull request Nov 18, 2019

Document Socket API functions return values. #11808

Merged

kivaisan mentioned this pull request Dec 12, 2019

Update offloaded TLSSocket to TLSSocket design #12089

Merged

Offloaded TLSSocket and BG96 support for it #11357

Offloaded TLSSocket and BG96 support for it #11357

Uh oh!

Conversation

kivaisan commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Aug 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnttiKauppila Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kivaisan Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnttiKauppila commented Aug 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kivaisan commented Aug 28, 2019

Uh oh!

0xc0170 commented Aug 29, 2019

Uh oh!

AnttiKauppila commented Aug 29, 2019

Uh oh!

kivaisan commented Aug 29, 2019

Uh oh!

kivaisan commented Aug 29, 2019

Uh oh!

AnttiKauppila commented Aug 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Sep 3, 2019

Uh oh!

0xc0170 commented Sep 3, 2019

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

kivaisan commented Sep 10, 2019

Uh oh!

0xc0170 commented Sep 25, 2019

Uh oh!

kivaisan commented Sep 25, 2019

Uh oh!

SeppoTakalo commented Sep 25, 2019

Uh oh!

SeppoTakalo commented Sep 25, 2019

Uh oh!

kivaisan commented Aug 28, 2019 •

edited

Loading

AnttiKauppila Aug 28, 2019 •

edited

Loading

kivaisan Aug 28, 2019 •

edited

Loading

kivaisan commented Sep 26, 2019 •

edited

Loading