Secure tunnel with Multiplexing #78

sbSteveK · 2022-12-02T22:03:43Z

Secure Tunnel full refactor and Multiplexing integration

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…connection complete

bretambrose

First pass on everything except secure_tunneling.c For that one I need to read it on its own without any diff, it's too overwhelming otherwise.

include/aws/iotdevice/private/serializer.h

bretambrose · 2023-01-31T17:19:03Z

include/aws/iotdevice/private/serializer.h

-    struct aws_byte_buf payload;
+enum aws_secure_tunnel_protocol_buffer_wire_type {
+    AWS_SECURE_TUNNEL_PBWT_VARINT = 0,            /* int32, int64, uint32, uint64, sint32, sint64, bool, enum */
+    AWS_SECURE_TUNNEL_PBWT_64_BIT = 1,            /* fixed64, sfixed64, double */


If these are all floating point, I'd make that part of the name

The way protocol buffers work, type 1 and type 5 refer to the size of what comes next and we handle casting those bits as one of the types in the comment. They're not always doubles/floats and can also be ints and uints. I think we should leave them as is but don't feel strongly about it and am open to naming them something else if you do.

FWIW, we also don't use either in our encoding/decoding for secure tunnel.

I feel like our implementation should not care about protocol buffers in any way. It's... frustrating that someone is defining an abstract network wire protocol in terms of a specific technology.

bretambrose · 2023-01-31T17:19:07Z

include/aws/iotdevice/private/serializer.h

+    AWS_SECURE_TUNNEL_PBWT_LENGTH_DELIMINTED = 2, /* string, bytes, embedded messages, packed repeated fields */
+    AWS_SECURE_TUNNEL_PBWT_START_GROUP = 3,       /* groups (deprecated) */
+    AWS_SECURE_TUNNEL_PBWT_END_GROUP = 4,         /* groups (deprecated) */
+    AWS_SECURE_TUNNEL_PBWT_32_BIT = 5,            /* fixed32, sfixed32, float */


If these are all floating point, I'd make that part of the name

include/aws/iotdevice/private/secure_tunneling_operations.h

source/secure_tunneling_operations.c

bretambrose · 2023-01-31T21:12:34Z

source/secure_tunneling_operations.c

+            stream_id = secure_tunnel->config->service_id_3_stream_id;
+        } else {
+            /* service_id doesn't match any existing service id*/
+            AWS_LOGF_DEBUG(


Should this be a fatal protocol error?

Tricky. It's possible the user uses a service id that isn't one the current secure tunnel supports. The protocol/specs for secure tunnel doesn't explicitly say anything about what to do in this scenario so I'm just letting it slide with a debug message. I could maybe swap it to a Warn level message instead of Debug.

bretambrose · 2023-01-31T21:14:20Z

source/secure_tunneling_operations.c

+ * Check the outbound stream start service id (or lack of one for V1) and set the secure tunnel and stream start
+ * message's stream id to the next value.
+ */
+static int s_aws_secure_tunnel_operation_message_set_next_stream_id(


Comments above apply to this function too.

Also, I just don't understand the semantics of this function. It takes a service id (as specified in a message) and then uses a different, larger stream id (written back into that message) than what is assigned to that service id. Larger context of why this is happening would be helpful.

The Destination device connects, receives service ids (multiplexing) from the service, and each of the (up to 3) service id's stream ids are set to 0 (unused).

When a Source device connects on the other end of the tunnel, it starts a stream by sending a service id along with whatever number they want as the stream id.

When the function above (s_aws_secure_tunnel_operation_message_set_stream_id) receives a stream start message, it checks against the service ids, and sets that service id's stream id to the received one from the stream start.

This function here (s_aws_secure_tunnel_operation_message_set_next_stream_id ) is only called from Source mode sending a Stream Start message and it gets the next stream id to use and sets a its own stream id to the same one so it can send messages and receive messages on the right stream id.

source/secure_tunneling_operations.c

include/aws/iotdevice/secure_tunneling.h

bretambrose · 2023-02-15T18:01:57Z

include/aws/iotdevice/secure_tunneling.h

 AWS_IOTDEVICE_API
-const struct aws_secure_tunnel_options *aws_secure_tunnel_options_storage_get(
-    const struct aws_secure_tunnel_options_storage *storage);
+int aws_secure_tunnel_stream_start(


Can this be replaced by a start followed by a send_message?

Can these functions marked don't-use be moved to a private header? Private header APIs are accessible from tests.

Stream Start is a specific operation that's processed differently than a message. It obtains the stream id differently and I'm assuming this will need to be expanded further for V3.

I thought about moving them to a private header but we still need to support this in a way that prevents breaking existing use in CPP because it was exposed. This one feels like a gray area in terms of how we want to handle it but for now, I'll leave it here since it needs to be accessible by the CPP client.

source/serializer.c

include/aws/iotdevice/private/secure_tunneling_operations.h

source/secure_tunneling_operations.c

bretambrose · 2023-02-15T19:56:34Z

source/secure_tunneling.c

+            case AWS_STOT_STREAM_START:
+                if ((*current_operation->vtable->aws_secure_tunnel_operation_set_next_stream_id_fn)(
+                        current_operation, secure_tunnel)) {
+                    error_code = aws_last_error();


Same here, it's hard to conceive of a sane protocol that continues under invariant breakage (or what should be an invariant)

bretambrose · 2023-02-15T19:56:47Z

source/secure_tunneling.c

+
+                if ((*current_operation->vtable->aws_secure_tunnel_operation_assign_stream_id_fn)(
+                        current_operation, secure_tunnel)) {
+                    error_code = aws_last_error();


Same here, it's hard to conceive of a sane protocol that continues under invariant breakage (or what should be an invariant)

bretambrose · 2023-02-15T19:58:21Z

source/secure_tunneling.c

+                break;
+        }
+
+        s_complete_operation(secure_tunnel, current_operation, AWS_OP_SUCCESS, NULL);


This seems weird. We're guaranteed to fully flush an application data message?

Actually I think I understand now. In mqtt5 we're using raw channel messages of a fixed size. Here we're sending var-length websocket frames and the websocket impl is responsible for breaking up into channel messages.

source/secure_tunneling.c

TwistedTwigleg

Review part 1:

I have not looked at secure_tunneling.c, secure_tunneling_operations.c, nor serializer.c yet - but I will look at those next. So far it looks good though! Will look at the files mentioned on my next review pass.

bin/securetunnel/main.c

tests/secure_tunnel_tests.c

tests/secure_tunneling_tests.c

TwistedTwigleg

Review Part 2: Some minor questions and nitpick/debatable things that might be worth looking into (or not!) - but they are all minor and the rest looks good.

source/serializer.c

source/secure_tunneling_operations.c

source/secure_tunneling.c

sbSteveK added 26 commits September 20, 2022 09:43

Add connection_error_code and API to access it for error checking on …

fc2425c

…connection complete

encoder/decoder refactor

04e8055

Secure Tunnel Multiplex WIP

98545b5

WIP on hold to work on mqtt5 canary

da4a6bb

more wip saves

97f1731

revamp api and operations

6caf6d5

refactored message handling

6ea5be8

continued implementation of operations and tasks

b7a5b00

merged with current main

4f7d3c0

stash

b1d85b3

test connection

449195c

progress before removing channel and slot for websocket use

d7c4216

stable before checking slot

6dee25f

pings updated to work with secure tunnel

5b6f269

cleanup

39dcd7f

encoding WIP

1ea9dbe

encoding of outbound messages

16572d1

cleaning up

7ee40ae

clean up

ca98b92

more clean up

968c684

merge with latest main

34a3fc0

clean up

d75b11a

stream reset added

94082ac

logging of error added

8a452c8

handling of the on_connection_complete callback

c890820

typo

ee05c21

bretambrose reviewed Jan 31, 2023

View reviewed changes

sbSteveK added 3 commits February 7, 2023 14:21

code review changes WIP

1b6a666

wip

f3a3993

pr wip

e24924b

sbSteveK added 3 commits February 14, 2023 10:29

add an on_stopped callback

539da71

give options_storage its allocator to use during cleanup

176e38b

added /bin files

05d2604

bretambrose reviewed Feb 15, 2023

View reviewed changes

sbSteveK added 10 commits February 15, 2023 16:20

code review fixes wip

82b3179

use aws_byte_buf_write_to_capacity

71a082f

removed unused variable

96c1ad3

convert service ids to hash table

04358b5

fix hash table for service ids

a731303

cleanup old service id

4cb2261

more cr fixes/changes

5e6c377

add termination callback specific user data

b28da64

vtable expanded for testing

0b6027c

expand vtable for testing

c7accba

TwistedTwigleg reviewed Feb 20, 2023

View reviewed changes

sbSteveK added 11 commits February 20, 2023 14:04

simplify websocket creation

35fe644

add encoding for service ids on outbound message

2d9ee53

fix for length on multiple service ids

fa2b9de

cr changes

8fbe49a

remove old tests

c7b2adb

fix race condition

ec73eee

secure tunnel tests

bde1c01

Clean Up

3736917

unused test parameters (void)

99f35b0

ignore specific warning

f0bbb07

add logging

e7abc0d

TwistedTwigleg approved these changes Feb 22, 2023

View reviewed changes

code review changes

0c3cd36

sbSteveK merged commit 09ded2b into main Feb 23, 2023

sbSteveK deleted the secure-tunnel-multiplex branch February 23, 2023 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secure tunnel with Multiplexing #78

Secure tunnel with Multiplexing #78

sbSteveK commented Dec 2, 2022

bretambrose left a comment

bretambrose Jan 31, 2023

sbSteveK Feb 7, 2023

bretambrose Feb 15, 2023

bretambrose Jan 31, 2023

bretambrose Jan 31, 2023

sbSteveK Feb 8, 2023

bretambrose Jan 31, 2023

sbSteveK Feb 3, 2023

bretambrose Feb 15, 2023

sbSteveK Feb 16, 2023

bretambrose Feb 15, 2023

bretambrose Feb 15, 2023

bretambrose Feb 15, 2023

TwistedTwigleg left a comment

TwistedTwigleg left a comment •

edited

Loading

Secure tunnel with Multiplexing #78

Secure tunnel with Multiplexing #78

Conversation

sbSteveK commented Dec 2, 2022

bretambrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TwistedTwigleg left a comment

Choose a reason for hiding this comment

TwistedTwigleg left a comment • edited Loading

Choose a reason for hiding this comment

TwistedTwigleg left a comment •

edited

Loading