Add support for datagram packing #1918

hanno-becker · 2018-08-03T16:39:04Z

Summary: This PR adds support for stacking multiple records in a single datagram, based on the ongoing work to support outgoing handshake fragmentation in #1939. The first new commit is 5aa4e2c.

`mbedtls_ssl_get_record_expansion()` is supposed to return the maximum difference between the size of a protected record and the size of the encapsulated plaintext. It had the following two bugs: (1) It did not consider the new ChaChaPoly ciphersuites, returning the error code #MBEDTLS_ERR_SSL_INTERNAL_ERROR in this case. (2) It did not correctly estimate the maximum record expansion in case of CBC ciphersuites in (D)TLS versions 1.1 and higher, in which case the ciphertext is prefixed by an explicit IV. This commit fixes both bugs.

hanno-becker · 2018-08-07T13:40:31Z

This is on hold until the test failures in #1879 have been resolved.

hanno-becker · 2018-08-10T08:01:51Z

All CI interoperability failures on the underlying #1879 have been understood and seen as not caused by an issue in Mbed TLS; see IOTSSL-2401. Testing this again.

hanno-becker · 2018-08-10T08:02:01Z

retest

hanno-becker · 2018-08-10T20:58:42Z

@mpg @AndrzejKurek Do you think you will have time to review this?

mpg · 2018-08-11T20:48:49Z

Yes, I'll review this next week!

hanno-becker · 2018-08-13T08:36:57Z

The introduction of datagram packing significantly changes the nature of the proxy tests in ssl-opt.sh since the UDP proxy works on the basis of datagrams: where it was previously capable of reordering or dropping some messages from flights, this is no longer possible if the entire flight is sent within a single datagram - as is the default behavior now. For example, without further changes to the proxy, or without the possibility to disable datagram packing, it is no longer possible to simulate a delayed CCS message. This is also the reason why we see the CI succeeding albeit it fails for the underlying fragmentation PR #1879.

I believe we should consider ways around this, for example

Allow static enabling/disabling of datagram packing at compile time.
Allow dynamic enabling/disabling of datagram packing at run time.
Improve the UDP proxy by having it dissect datagrams into records prior to performing its other operations.

Leaving tests aside, the only practical reason I see for allowing to disable datagram packing are potential interoperability issues with peers that happen to have problems receiving packed datagrams, but this seems quite unlikely.

Going through the options, the following are the consequences to the test system for each of them:

For static enabling/disabling of datagram packing, we'd need another build + test in all.sh. Some tests in ssl-opt.sh would need to be disabled for datagram packing (such as the one for delayed CCS's).
For dynamic enabling/disabling of datagram packing, this could be controlled through a new parameter to ssl_client2 and ssl_server2, so that we could (a) disable datagram packing for the tests conflicting with it, and (b) duplicate tests which are meaningful with and without datagram packing.
When the UDP proxy not only supports packing but also splitting of datagrams, we'd need to turn that on in those tests that conflict with datagram packing, and duplicate the tests which are meaningful with and without datagram packing.

@mpg What do you think?

mpg · 2018-08-13T11:00:46Z

@hanno-arm I'll reply to those question later because I have to go now, but I wanted to let you know I've put up a new PR with the fragmentation work here: #1918 It has a cleaner history, fixes for a few bugs, more tests for MTU settings, and should pass CI (as it doesn't have the problematic interop tests yet).

I'd like to suggest you rebase your work on the new branch of mine, in order to have a cleaner history (currently there are a number of WIP/temporary/revert commits in the history) and the bug fixes.

hanno-becker · 2018-08-13T11:14:15Z

Thanks @mpg, I will do the rebase in the meantime.

mpg · 2018-08-14T09:16:41Z

Leaving tests aside, the only practical reason I see for allowing to disable datagram packing are potential interoperability issues with peers that happen to have problems receiving packed datagrams, but this seems quite unlikely.

I'm not sure. IIRC, OpenSSL starts by sending packed datagrams, then if it has to retransmit tries unpacked, then it if has to retransmit again, it starts lowering the "estimated handshake MTU" and fragmenting if necessary. I don't think I've seen it stated anywhere, but I always assumed this was in order to enhance compatibility with peers that might not support datagram packing. However perhaps I'm misinterpreting, and even if I'm not, perhaps this is no longer a concern these days.

Anyway, I'm inclined to go for option 2 (dynamic setting of this behaviour) unless you think that will make the code too complex.

If we don't got for option 2, I quite dislike option 1 (static setting) as we have too many build options to test already, but before we go for option 3 I'd like to suggest option 4: find a peer that doesn't pack (I think you mentioned recently that openssl s_server -debug is such a peer) and use it for the tests that need it, so that we don't have to make the proxy smarter for now.

hanno-becker · 2018-08-14T09:24:50Z

@mpg Thank you for your feedback. I don't expect it to be difficult to enable/disable datagram packing dynamically, and I'll do the corresponding change. In the meantime, I implemented datagram splitting on the UDP proxy, which I think is a feature worth having in any case, complementing the datagram packing ability of the UDP proxy that we already have; in conjunction, both would e.g. allow to reorder records within a single datagram, which is not something you would practically expect to happen, but still worth testing, as it tests that we don't make make any ordering assumptions from the fact that records come within the same datagram.

mpg · 2018-08-14T09:32:01Z

Aw, github is now showing commit in the wrong order. Apparently it does this on purpose because it's "a space for discussion" - well I find it much easier to discuss a series of commits when it's presented in its logical order.

Anyway, reviewers beware of the github interface. Thankfully git log --patch --reverse is our reliable friend.

mpg · 2018-08-14T10:03:43Z

@hanno-arm Thanks for reworking the history! However I'm afraid you didn't quite rebase on top of #1939, but rather cherry-picked it or something. As a result, all the commits from #1939 are present with a different commit ID, which means when both PRs are merged, the history will contains "duplicate" commits with the same title and date but different hashes, which I tend to find pretty annoying when searching the history later on.

I fixed the issue by reset-harding my branch to 8c75a5d6a06f then force-pushing it, so you don't have anything do to here, but I wanted to let you know.

In the future, tempted to base a branch on two other branches (that are also going to be merged independently), we should base on one and merge the other in, in order to avoid near-duplicate commits in the overall history.

hanno-becker · 2018-08-24T10:30:12Z

@mpg I have pushed some commits to address your comments, and left a comment on your point in regards to the length check in ssl_encrypt_buf().

This commit introduces some tests to ssl-opt.sh checking that setting the MFL limits the MTU to MFL + { Maximum Record Expansion }.

…king

hanno-becker · 2018-08-24T11:02:54Z

@mpg Tests checking the MFL effectively restricting the MTU are currently failing, I am investigating.

The negotiated MFL is always the one suggested by the client, even if the server has a smaller MFL configured locally. Hence, in the test where the client asks for an MFL of 4096 bytes while the server locally has an MFL of 512 bytes configured, the client will still send datagrams of up to ~4K size.

hanno-becker · 2018-08-24T11:16:09Z

One of the tests I just added to ssl-opt.sh had wrong expectations, I removed it in 69ca0ad.

mpg · 2018-08-24T12:00:47Z

tests/ssl-opt.sh

@@ -5020,6 +5020,10 @@ run_test    "DTLS fragmenting: server only (max_frag_len)" \
            -c "found fragmented DTLS handshake message" \
            -C "error"

+# With the MFL extension, the server has no way of forcing


I think you just re-discovered our biggest issue with the MFL extension :) Fortunately the IETF has a replacement underway: https://tools.ietf.org/html/draft-ietf-tls-record-limit-03#section-3

mpg

Thanks for making the requested changes! I'm happy with the result.

mpg · 2018-08-24T22:20:20Z

Note: all.sh -k -r -m passed locally on the latest commit 69ca0ad

simonbutcher

I'm okay with the PR -as-is- but I'd like to document our use of --insecure in gnutls-cli. I don't want to delay merging this PR, so @hanno-arm, can you please raise a secondary PR to fix this? (Assuming you don't disagree!). If you don't agree, please reply to this PR.

simonbutcher · 2018-08-25T18:06:20Z

tests/ssl-opt.sh

@@ -5459,35 +5515,31 @@ run_test    "DTLS fragmenting: gnutls server, DTLS 1.0" \
            -c "fragmenting handshake message" \
            -C "error"

-# gnutls-cli always tries IPv6 first, and doesn't fall back to IPv4 with DTLS
-requires_ipv6
 requires_config_enabled MBEDTLS_SSL_PROTO_DTLS


I think we need to have a comment here that explains and justifies the use of --insecure here and below.

@sbutcher-arm I don't mind changing this here, but the change was introduced by @mpg in #1939, and if your feedback on that PR leads to a 'fork' of #1939 being created anyway, I think it's better to add the change you requested here in that new PR, too.

Please liaise with @mpg and do what you think is best. I'd prefer it if you could do it, as I think you'll have more time than @mpg, but I'll leave it to you to decide. Just do it as a new PR, so I can get this one in without more rework.

Force using IPv4 in the GNU_CLI SRTP tests, as introduced for other tests in Mbed-TLS#1918.

Force using IPv4 in the GNU_CLI SRTP tests, as introduced for other tests in Mbed-TLS#1918. Signed-off-by: Johan Pascal <johan.pascal@belledonne-communications.com>

Force using IPv4 in the GNU_CLI SRTP tests, as introduced for other tests in Mbed-TLS#1918.

Force using IPv4 in the GNU_CLI SRTP tests, as introduced for other tests in Mbed-TLS#1918. Signed-off-by: Johan Pascal <johan.pascal@belledonne-communications.com>

Hanno Becker added 2 commits August 3, 2018 10:07

Adapt ChangeLog

4481464

hanno-becker added enhancement mbed TLS team DO-NOT-MERGE component-tls labels Aug 3, 2018

hanno-becker changed the title ~~[DRAFT] Datagram packing~~ [DRAFT] Add support for datagram packing Aug 3, 2018

hanno-becker force-pushed the datagram_packing branch from 58b29ac to 3f24fda Compare August 6, 2018 11:27

hanno-becker changed the title ~~[DRAFT] Add support for datagram packing~~ Add support for datagram packing Aug 6, 2018

hanno-becker added the needs-work label Aug 6, 2018

hanno-becker requested review from mpg and AndrzejKurek August 10, 2018 14:54

hanno-becker added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Aug 10, 2018

hanno-becker force-pushed the datagram_packing branch from 3fa6a52 to 390d64f Compare August 14, 2018 07:25

hanno-becker force-pushed the datagram_packing branch from 390d64f to 85f4225 Compare August 14, 2018 09:20

hanno-becker force-pushed the datagram_packing branch 2 times, most recently from 138648f to c5438d8 Compare August 14, 2018 14:38

Hanno Becker added 2 commits August 24, 2018 11:48

ssl-opt.sh: Add tests checking that MFL implies bounds on MTU

c92b5c8

This commit introduces some tests to ssl-opt.sh checking that setting the MFL limits the MTU to MFL + { Maximum Record Expansion }.

Merge branch 'iotssl-165-dtls-hs-fragmentation-new' into datagram_pac…

6b6f602

…king

mpg reviewed Aug 24, 2018

View reviewed changes

mpg approved these changes Aug 24, 2018

View reviewed changes

k-stachowiak approved these changes Aug 24, 2018

View reviewed changes

AndrzejKurek approved these changes Aug 24, 2018

View reviewed changes

simonbutcher reviewed Aug 25, 2018

View reviewed changes

simonbutcher added approved Design and code approved - may be waiting for CI or backports approved for design and removed needs-design-approval needs-review Every commit must be reviewed by at least two team members, labels Aug 25, 2018

hanno-becker mentioned this pull request Aug 28, 2018

DTLS fragmentation and reordering support: Minor rework #1988

Merged

simonbutcher merged commit 69ca0ad into Mbed-TLS:development Aug 29, 2018

JanneKiiskila mentioned this pull request Sep 10, 2018

Hardware acceleration is not functional ARMmbed/mbed-os#6545

Closed

RonEld pushed a commit to RonEld/mbedtls that referenced this pull request Jan 15, 2019

Force IPv4 in gnutls_cli srtp tests

7eb96c7

Force using IPv4 in the GNU_CLI SRTP tests, as introduced for other tests in Mbed-TLS#1918.

RonEld pushed a commit to RonEld/mbedtls that referenced this pull request Sep 26, 2019

Force IPv4 in gnutls_cli srtp tests

67a5e44

Force using IPv4 in the GNU_CLI SRTP tests, as introduced for other tests in Mbed-TLS#1918.

jeannotlapin pushed a commit to jeannotlapin/mbedtls that referenced this pull request Apr 23, 2020

Force IPv4 in gnutls_cli srtp tests

26702e9

Force using IPv4 in the GNU_CLI SRTP tests, as introduced for other tests in Mbed-TLS#1918.

ycyang1229 pushed a commit to ycyang1229/mbedtls that referenced this pull request Sep 1, 2020

Force IPv4 in gnutls_cli srtp tests

9040c05

Force using IPv4 in the GNU_CLI SRTP tests, as introduced for other tests in Mbed-TLS#1918.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for datagram packing #1918

Add support for datagram packing #1918

hanno-becker commented Aug 3, 2018 •

edited

Loading

hanno-becker commented Aug 7, 2018

hanno-becker commented Aug 10, 2018 •

edited

Loading

hanno-becker commented Aug 10, 2018

hanno-becker commented Aug 10, 2018

mpg commented Aug 11, 2018

hanno-becker commented Aug 13, 2018

mpg commented Aug 13, 2018

hanno-becker commented Aug 13, 2018

mpg commented Aug 14, 2018

hanno-becker commented Aug 14, 2018

mpg commented Aug 14, 2018

mpg commented Aug 14, 2018

hanno-becker commented Aug 24, 2018

hanno-becker commented Aug 24, 2018

hanno-becker commented Aug 24, 2018

mpg Aug 24, 2018

mpg left a comment

mpg commented Aug 24, 2018

simonbutcher left a comment

simonbutcher Aug 25, 2018

hanno-becker Aug 26, 2018

simonbutcher Aug 27, 2018

Add support for datagram packing #1918

Add support for datagram packing #1918

Conversation

hanno-becker commented Aug 3, 2018 • edited Loading

hanno-becker commented Aug 7, 2018

hanno-becker commented Aug 10, 2018 • edited Loading

hanno-becker commented Aug 10, 2018

hanno-becker commented Aug 10, 2018

mpg commented Aug 11, 2018

hanno-becker commented Aug 13, 2018

mpg commented Aug 13, 2018

hanno-becker commented Aug 13, 2018

mpg commented Aug 14, 2018

hanno-becker commented Aug 14, 2018

mpg commented Aug 14, 2018

mpg commented Aug 14, 2018

hanno-becker commented Aug 24, 2018

hanno-becker commented Aug 24, 2018

hanno-becker commented Aug 24, 2018

mpg Aug 24, 2018

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg commented Aug 24, 2018

simonbutcher left a comment

Choose a reason for hiding this comment

simonbutcher Aug 25, 2018

Choose a reason for hiding this comment

hanno-becker Aug 26, 2018

Choose a reason for hiding this comment

simonbutcher Aug 27, 2018

Choose a reason for hiding this comment

hanno-becker commented Aug 3, 2018 •

edited

Loading

hanno-becker commented Aug 10, 2018 •

edited

Loading