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

net: sockets: tls: Implement DTLS Connection ID socket option #36738

Closed

Conversation

rlubos
Copy link
Contributor

@rlubos rlubos commented Jul 5, 2021

This PR implements DTLS Connection ID socket option which allows to configure the use of Connection ID extension for DTLS session. Opening the PR early as a RFC, to collect some feedback on the option format.

In the current implementation, it's assumed that the application generates the CID value, and passes it to a socket with setsockopt(). An alternative that I've considered was to provide only the CID length with the option, and allowing the secure socket implementation to randomize the CID value before the handshake. I've chosen the former however, as it gives the option more flexibility (for example using a predefined CID value for testing).

While the implementation on the Zephyr side can be considered as complete, I've encountered a few issues with mbed TLS itself:

  • There's a bug in mbed TLS that causes NULL pointer dereference, if some extra configs are not enabled. The issue is already reported in the upstream mbed TLS repository, and fix submitted (see Bugfix: Fix the usage of ssl context after it's nullified Mbed-TLS/mbedtls#3991). As a workaround for the issue, one can enable CONFIG_MBEDTLS_SSL_EXPORT_KEYS along with CONFIG_MBEDTLS_SSL_DTLS_CONNECTION_ID to prevent the NULL pointer dereference.

  • Currently, mbed TLS uses an old CID extension identifier, as the value changed between different CID spec drafts. In result, the extension record is not recognized by other parties, like Leshan or Wireshark. Again, a fix for the issue is already posted, still on a PR though (see Allow configuring MBEDTLS_TLS_EXT_CID at compile time Mbed-TLS/mbedtls#4413). As a workaround for the issue, you need to update the MBEDTLS_TLS_EXT_CID value manually for now:

diff --git a/mbedtls/include/mbedtls/ssl.h b/mbedtls/include/mbedtls/ssl.h
index 7815ad9..2600d7d 100644
--- a/mbedtls/include/mbedtls/ssl.h
+++ b/mbedtls/include/mbedtls/ssl.h
@@ -413,7 +413,7 @@
 /* The value of the CID extension is still TBD as of
  * draft-ietf-tls-dtls-connection-id-05
  * (https://tools.ietf.org/html/draft-ietf-tls-dtls-connection-id-05) */
-#define MBEDTLS_TLS_EXT_CID                        254 /* TBD */
+#define MBEDTLS_TLS_EXT_CID                        53 /* TBD */
 
 #define MBEDTLS_TLS_EXT_ECJPAKE_KKPP               256 /* experimental */

Given the above, I think we shouldn't rush with this PR, and postpone its integration until the aforementioned issues are fixed on the mbed TLS side (and upmerged into Zephyr fork).

With the above workarounds, one can simply enable the CID usage for the DTLS session with a single setsockopt() call:

		int8_t cid[4] = {0xde, 0xad, 0xbe, 0xef};

		ret = setsockopt(client_ctx->sock_fd, SOL_TLS, TLS_DTLS_CONNECTION_ID,
				 cid, sizeof(cid));

or

		ret = setsockopt(client_ctx->sock_fd, SOL_TLS, TLS_DTLS_CONNECTION_ID,
				 NULL, 0);

if we don't want to specify own CID on the Zephyr part. The latter will result in an empty CID record, notifying the PR that the Zephyr part is ready to process the peer's CID, but does not want to use one.

Note, that the PR carries an extra commit, fixing the HS on non-blocking sockets. It's already posted as a separate PR, but I've included it here as it was needed for testing with LwM2M.

The TLS/DTLS handshake in most cases is a blocking process, therefore
the underlying socket should be in a blocking mode to prevent busy
looping in the handshake thread. Fix this by clearing the O_NONBLOCK
flag on the underlying socket before the handshake, and restoring it
afterards.

Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
Add Kconfig option for `config-tls-generic.h` to enable DTLS Connection
ID extension.

Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
@rlubos rlubos added the RFC Request For Comments: want input from the community label Jul 5, 2021
@rlubos rlubos self-assigned this Jul 5, 2021
@rlubos rlubos mentioned this pull request Jul 5, 2021
Implement TLS_DTLS_CONNECTION_ID socket option, which enables to use
Connection ID extension for the DTLS session.

The option allows to set the value and the length of the CID to use with
`setsockopt()` function. Setting the CID length to 0, enables the
extension but does not send the own CID to the peer, as described in the
specification.

Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
@boaks
Copy link

boaks commented Jul 5, 2021

Some general comments:

During the specification process, the way the MAC is calculated has changed with draft-ietf-tls-dtls-connection-id, 09. In order to provide a migration path, a new hello extension id has been emitted for the new MAC, see IANA, Hello-Extensions. With that, 53 for the old MAC for a interim time, 54 with the new MAC.

Californium (CoAP/DTLS 1.2 base for Leshan) will be adapted within the next week before the upcoming 3.0.0 release. I will support the old value 53 with the old MAC and the 54 with the new MAC. I guess mbedtls will also adapt.

With that: really cool that this PR has been prepared!
And yes, postpone it a little in order to use directly the new extension value and MAC will help to prevent some trouble.

In the current implementation, it's assumed that the application generates the CID value, and passes it to a socket with setsockopt().

Sometimes I'm not sure, how people use UDP. So, in my experience, I open a UDP socket and potentially communicate with a couple of other peers. Assuming "co-located servers", and maybe not aware of that co-location, requires separate UDP-Sockets on the client-side. So, if the CID is passed in, that socket seems to be only useful for one other peer, but not to exchange messages with more peers.
Is that the intention? To use a UDP socket only for one destination?

@rlubos
Copy link
Contributor Author

rlubos commented Jul 5, 2021

Is that the intention? To use a UDP socket only for one destination?

That's the assumption for the DTLS socket, which this option is intended for. Currently, we allocate a single mbedtls_ssl_context per socket, which effectively limits the number of active DTLS sessions on a socket to one. I think that it should be technically possible to support multiple DTLS sessions on a single socket, however, I'm afraid that it'd complicate the logic a lot (which is already quite complex for DTLS).

Anyway, assuming that we might want to implement multiple DLTS session handling on a single socket in the future, perhaps it would indeed be a better idea to specify only CID length by the option, and let the DTLS socket randomize CID before handshake? This should be more future-proof solution, and it seems that Leshan Demo Server for instance already works in this way. You seem to have a great knowledge about this extension, what'd be your recomendation?

@boaks
Copy link

boaks commented Jul 5, 2021

That's the assumption for the DTLS socket, which this option is intended for.

That's not bad, it should just be clear. I'm not sure, how many device will send messages to more than a few server (e.g. LwM2M Bootstrap- and Data-Management-Server) and so the drawback is small. For clients I prefer doing it that way (1 socket 1 peer), otherwise errors resulting from co-located servers may be hard to detect.

This should be more future-proof solution, and it seems that Leshan Demo Server for instance already works in this way

Yes, there I have implemented it as callback in Californium.

You seem to have a great knowledge about this extension

Recently I was added to the list of authors of that upcoming DTLS connection ID RFC, Version 10 (Achim Kraus). The implementation in Eclipse/Californium (that's what Eclipse/Leshan uses) was developed by me. For the server-side it's mandatory to provide more than one CID, otherwise it will only be able to serve for one client. Using a callback enables also to use CIDs with variable length, if the internal encoding of the CID supports that (outside of the RFC, e.g. 1 byte as length, or in CoAP style, first nibble as length, if 15, length in the next nibble + 15).

what'd be your recomendation?

I would recommend to try to add a similar callback as Californium, but for that I have to talk with the mbedtls developers. I'm not sure, if that will be successful (and currently I'm too busy with Californium 3.0). Until that, I think using that approach providing a single CID for a client-socket will do it. Please obey, that there is also a case, where the client uses a empty "CID". That means, the client provides to send records to the server using a CID requested by the server, but will not use it receiving records from the server. Either try to support it, or document, that only "none-empty CID" can be used.

@rlubos
Copy link
Contributor Author

rlubos commented Jul 6, 2021

Until that, I think using that approach providing a single CID for a client-socket will do it.

Ok, thank you for the feedback, let's keep it as it is for now then. I'm expecting the PR is going to sit there for a while, so I might change this in the feature if needed.

Please obey, that there is also a case, where the client uses a empty "CID". That means, the client provides to send records to the server using a CID requested by the server, but will not use it receiving records from the server. Either try to support it, or document, that only "none-empty CID" can be used.

Actually, this is already supported, I've tried to explain this in the socket option description:

/** Socket option to set DTLS Connection ID to be used for the DTLS session.
 *  The option accepts an byte array, holding the CID to use.
 *  Setting an empty CID (option length set to 0) indicates that the socket is
 *  willing to handle CID from a peer, but does not specify its own CID.
 */

Perhaps it's not clear enough?

@boaks
Copy link

boaks commented Jul 6, 2021

Perhaps it's not clear enough?

Clearly my bad, I missed to view the PR before writing that. Sorry for that.

@boaks
Copy link

boaks commented Jul 6, 2021

Is it easy/possible to test this PR with a "Thingy91"?
(I have setup the toolchain to compile the serial_lte_modem end of last year, maybe it's required to update it. I used the nRF connect SDK 1.4.1 setup).

Checkout the PR and compile a CoAP/DTLS example for the "Thingy91"?
Or is more required?

@rlubos
Copy link
Contributor Author

rlubos commented Jul 7, 2021

Is it easy/possible to test this PR with a "Thingy91"?

It should be possible, but not that easy I'm afraid.

First off, the sample should be built within NCS, as vanilla Zepyr does not contain the modem library to handle networking on nRF91. So you'd need to checkout the latest sdk-nrf master branch, and call west update to update Zephyr.

Next, you need to cherry-pick the commits from this PR into the local Zephyr instance. Finally, the sample would need extra configs to work with nRF91 and offloading (I'm assuming the lwm2m_client from Zephyr tree, connecting to public Leshan):

CONFIG_NEWLIB_LIBC=y
CONFIG_HEAP_MEM_POOL_SIZE=16384

# # Network
CONFIG_NETWORKING=y
CONFIG_NET_NATIVE=n
CONFIG_NET_IPV6=n
CONFIG_NET_CONFIG_NEED_IPV6=y
CONFIG_NET_IPV4=y
CONFIG_NET_SOCKETS=y
CONFIG_NET_SOCKETS_OFFLOAD=y
CONFIG_NET_SOCKETS_OFFLOAD_TLS=n

# LTE link control
CONFIG_LTE_LINK_CONTROL=y
CONFIG_LTE_AUTO_INIT_AND_CONNECT=y

# Modem library
CONFIG_NRF_MODEM_LIB=y

CONFIG_NET_CONFIG_PEER_IPV4_ADDR="23.97.187.154"

I'm having some LTE connectivity issues right now, so I cannot confirm 100% it's all that is needed though.

And finally, in the sample itself, you need to set the TLS_DTLS_CONNECTION_ID socket option, as shown in the original post, and enable CONFIG_MBEDTLS_SSL_EXPORT_KEYS and CONFIG_MBEDTLS_SSL_DTLS_CONNECTION_ID in the config.

@boaks
Copy link

boaks commented Jul 7, 2021

It should be possible, but not that easy I'm afraid.

Yes, I think, everyone is afraid, that using something fresh developed in a new different context is not that easy :-).

Thanks for all the advice and hints. Hope I find some time at the weekend.

@boaks
Copy link

boaks commented Jul 18, 2021

I was able to apply the PR to my setup and it compiles.
Using ncs 1.6.0 the "main" of the lwm2m client contains CONFIG_LWM2M_DTLS_SUPPORT and modem_key_mgmt_write. For me this looks more, that this takes the DTLS support of the modem, and not the one of this PR. Or am I wrong?

@rlubos
Copy link
Contributor Author

rlubos commented Jul 19, 2021

I was able to apply the PR to my setup and it compiles.
Using ncs 1.6.0 the "main" of the lwm2m client contains CONFIG_LWM2M_DTLS_SUPPORT and modem_key_mgmt_write. For me this looks more, that this takes the DTLS support of the modem, and not the one of this PR. Or am I wrong?

You're right, the NCS LwM2M sample uses DTLS from the modem by default. In order to use DTLS implementation from Zephyr you need to disable TLS offloading with the following option:

CONFIG_NET_SOCKETS_OFFLOAD_TLS=n

Then you need some extra configs to configure mbedTLS (I guess the configuration could be copied from the upstream sample).

Additionally, it'd be needed to register DTLS psk/id with tls_credential_add instead of modem_key_mgmt_write, as the latter works explicitly with the modem DTLS only. Otherwise there should be no differences.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Sep 18, 2021
@rlubos rlubos removed the Stale label Sep 20, 2021
@boaks
Copy link

boaks commented Oct 14, 2021

@rlubos

Two updates:

mbedtls - Allow configuring MBEDTLS_TLS_EXT_CID at compile time or mbedtls - 2.x backport: Allow configuring MBEDTLS_TLS_EXT_CID at compile time enables to build mbedtls now without modyfing the sources.

And mbedtls-PR - CID update to RFC 9146 is prepared, but not sure, when that could be merged.

FMPOV, the deprecated variant with an external compile time switch will currently be best way, at least until
the mbedtls-PR is unblocked. With that, either use Californium 2.6.5 or 3.0.0-RC1 and enable DTLS_SUPPORT_DEPRECATED_CID in the Configuration.

@rlubos
Copy link
Contributor Author

rlubos commented Oct 14, 2021

@boaks Thank you for the news! It's good to hear that the RFC is near completion.

It should be pretty straightforward to add Kconfig option which allows to use deprecated CID value. It seems however we'd need to wait for the mbed TLS relase to get configurable CID on the mbed TLS side - we''ve just updated recently to 3.0.0 in Zephyr.

@boaks
Copy link

boaks commented Oct 14, 2021

wait for the mbed TLS release

Yes, I missed that a mbedtls release will be required.
I was just happy to get the couple of lines merged :-).

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@rlubos
Copy link
Contributor Author

rlubos commented Mar 10, 2022

I'm currently waiting for zephyrproject-rtos/mbedtls#35 (mbed TLS 3.1 update), once we get this in I plan to revive this PR and pull it out of draft.

@boaks
Copy link

boaks commented Mar 10, 2022

The main issue here is mbedTLS.
In that project the implementation in 2.x was based on an early version of RFC9146 (to come) and the assigned IANA code points were not used.

At that point I started November 2020 mbedtls - Please adjust MBEDTLS_TLS_EXT_CID to 53 and since Summer 2021, it's now possible to use 53.

However, the draft RFC9146 has undergone a "cryptographic hygiene" cleanup (starting September 2020, with tls - mailing list ) and so the MAC has changed (again) and in order to support time-limited backwards compatibility (time to migrate), also a new IANA code point 54 was assigned.

All that is reflected and implemented in the open source project Eclipse/Californium (and so also in Eclipse/Leshan) with the version 3.0.0 last year.

For mbedtls it is still on the way, see mbedtls - Update DTLS CID implementation to comply with the "final" draft. and PR 5061. I don't know, when this will be ready. Using the deprecated code point 53 and the deprecated MAC is only recommended for early tests.

An alternative, but not at the modem-socket level, is using Eclipse/tinyDtls CID and the modem socket in UDP.

I've adapted the nRF9160 UDP sample to use tinyDtls and for me it works quite well. You can run from battery over a (small) couple of weeks, depending on the number of messages you want. RTT from LTE-M PSM is fast, about 1-2s (average, depending on the network provider). I still plan to publish that thingy 91 / tinydtls sampel client, but for now, I'm busy again with Californium.

Let me end:
Sometimes I ask my self, why so many companies want to use that stuff, but prefer to wait instead of act. Maybe I'm wrong, but running from battery with mqtt doesn't make too much sense. Having the most companies spent there resources in that, it's hard to make progress with CoAP/DTLS CID.

@boaks
Copy link

boaks commented Mar 22, 2022

Just to mention:
RFC9146 is published.

plskeggs added a commit to plskeggs/mbedtls that referenced this pull request Apr 13, 2022
Per zephyrproject-rtos/zephyr#36738.

Updated CID value with latest from:
https://www.iana.org/assignments/tls-extensiontype-values/
      tls-extensiontype-values.xhtml#tls-extensiontype-values-1

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
plskeggs added a commit to plskeggs/mbedtls-1 that referenced this pull request Apr 15, 2022
Per zephyrproject-rtos/zephyr#36738.

Updated CID value with latest from:
  https://www.iana.org/assignments/tls-extensiontype-values/
tls-extensiontype-values.xhtml#tls-extensiontype-values-1

Enable use of SNI without x509.

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
plskeggs added a commit to plskeggs/mbedtls-1 that referenced this pull request Apr 15, 2022
Updated CID value with latest from:
https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml.

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
plskeggs added a commit to plskeggs/mbedtls that referenced this pull request Apr 16, 2022
Per zephyrproject-rtos/zephyr#36738.

Updated CID value with latest from:
  https://www.iana.org/assignments/tls-extensiontype-values/
tls-extensiontype-values.xhtml

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
plskeggs added a commit to plskeggs/mbedtls that referenced this pull request Apr 18, 2022
Per zephyrproject-rtos/zephyr#36738.

Updated CID value with latest from:
  https://www.iana.org/assignments/tls-extensiontype-values/
tls-extensiontype-values.xhtml

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
frkv pushed a commit to frkv/sdk-mbedtls that referenced this pull request May 13, 2022
Per zephyrproject-rtos/zephyr#36738.

Updated CID value with latest from:
  https://www.iana.org/assignments/tls-extensiontype-values/
tls-extensiontype-values.xhtml

ref: NCSDK-15193

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Signed-off-by: Frank Audun Kvamtrø <frank.kvamtro@nordicsemi.no>
anangl pushed a commit to nrfconnect/sdk-mbedtls that referenced this pull request May 18, 2022
Per zephyrproject-rtos/zephyr#36738.

Updated CID value with latest from:
  https://www.iana.org/assignments/tls-extensiontype-values/
tls-extensiontype-values.xhtml

ref: NCSDK-15193

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Signed-off-by: Frank Audun Kvamtrø <frank.kvamtro@nordicsemi.no>
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label May 22, 2022
@boaks
Copy link

boaks commented May 23, 2022

zephyrproject-rtos/mbedtls/pull/35 is merged. I tested mbedtls 3.1 against Eclipse/Californium and it works well. Looking forward to see this PR continued.

Edited:
Sorry, I was mislead by some old binaries with that PR 5061. The 3.1 still uses the proprietary extension value 254 and the old MAC ...

@github-actions github-actions bot removed the Stale label May 24, 2022
plskeggs added a commit to plskeggs/mbedtls-1 that referenced this pull request Jun 2, 2022
Updated CID value with latest from:
https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml.

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Vge0rge pushed a commit to Vge0rge/sdk-mbedtls that referenced this pull request Jun 9, 2022
Per zephyrproject-rtos/zephyr#36738.

Updated CID value with latest from:
  https://www.iana.org/assignments/tls-extensiontype-values/
tls-extensiontype-values.xhtml

ref: NCSDK-15193

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Signed-off-by: Frank Audun Kvamtrø <frank.kvamtro@nordicsemi.no>
joerchan pushed a commit to joerchan/sdk-mbedtls that referenced this pull request Jun 9, 2022
Per zephyrproject-rtos/zephyr#36738.

Updated CID value with latest from:
  https://www.iana.org/assignments/tls-extensiontype-values/
tls-extensiontype-values.xhtml

ref: NCSDK-15193

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Signed-off-by: Frank Audun Kvamtrø <frank.kvamtro@nordicsemi.no>
joerchan pushed a commit to joerchan/sdk-mbedtls that referenced this pull request Jun 9, 2022
Per zephyrproject-rtos/zephyr#36738.

Updated CID value with latest from:
  https://www.iana.org/assignments/tls-extensiontype-values/
tls-extensiontype-values.xhtml

ref: NCSDK-15193

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Signed-off-by: Frank Audun Kvamtrø <frank.kvamtro@nordicsemi.no>
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jul 23, 2022
@rlubos rlubos removed the Stale label Jul 25, 2022
mbolivar-nordic pushed a commit to mbolivar-nordic/sdk-mbedtls that referenced this pull request Sep 6, 2022
Per zephyrproject-rtos/zephyr#36738.

Updated CID value with latest from:
  https://www.iana.org/assignments/tls-extensiontype-values/
tls-extensiontype-values.xhtml

ref: NCSDK-15193

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Signed-off-by: Frank Audun Kvamtrø <frank.kvamtro@nordicsemi.no>
(cherry picked from commit fcf5f41)
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Sep 24, 2022
@github-actions github-actions bot closed this Oct 9, 2022
@boaks
Copy link

boaks commented Nov 29, 2022

Right now mbtls starts to support RFC 9146.

Mbed-TLS/mbedtls#6264 is merged.

Maybe the right time to reopen it?

mbolivar-nordic pushed a commit to mbolivar-nordic/sdk-mbedtls that referenced this pull request Feb 20, 2023
Per zephyrproject-rtos/zephyr#36738.

Updated CID value with latest from:
  https://www.iana.org/assignments/tls-extensiontype-values/
tls-extensiontype-values.xhtml

ref: NCSDK-15193

Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
Signed-off-by: Frank Audun Kvamtrø <frank.kvamtro@nordicsemi.no>
(cherry picked from commit fcf5f41)
(cherry picked from commit 6330645)
@plskeggs plskeggs mentioned this pull request Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Modules area: Networking RFC Request For Comments: want input from the community Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants