forked from mas-bandwidth/yojimbo
-
Notifications
You must be signed in to change notification settings - Fork 0
/
Copy pathWOULD BE NICE
186 lines (95 loc) · 8.34 KB
/
WOULD BE NICE
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
WOULD BE NICE
------------------
Client should be smart enough to know the expiry time on a connect token.
If the connect doesn't complete within the expiry time, give up.
Right now, it risks, worst case, hitting n servers, with timeout per-server, which if n is high could be worst case a lot higher than expiry time.
------------------
Make sure the last access time for the encryption manager is getting updated, even while using the cached encryption index in the client transport context.
------------------
Is there an attack where a pending client can take a connect token and dick around with pre-connect packets and states forever?
Perhaps there should be a time limit for pending client connects. If an encryption mapping is setup for a pending client, but that client doesn't connect within a certain amount of time (connect token timeout?) then blacklist that IP address?
But is this an effective attack? I'm not sure yet.
------------------
Add user data to the match info
------------------
Connect token can time out. Update the code to abort the client connect on connect token timeout, otherwise it's possible, when lots of addresses
are specified in the match data, for the client to keep trying to connect and timing out per-server (5 seconds), which would be painful if I extended the # of servers per-connect token past the current 8.
------------------
Add more tests to ensure address parse from string is robust.
Add safety padding to the buffer as well. This is an external input (inside connect token), so make sure it's robust.
------------------
Fix up the address class endianness. It's inconsistent. Should prefer values to be in local byte order vs. network.
Nowhere else in the library does network order (big endian), and in fact the library considers network order to be little endian.
------------------
Probably best to just store the IPv4 address as uint8_t ipv4[4].
Get little benefit for mantaining it in an integer. It has to be endian swapped to/from network byte order anyway on send/recv.
------------------
Maximum worst case IPv6 address is 39 bytes.
But considering IPv4 thru IPv6 it is 45 bytes.
If I add port number to it:
[ipv6]:port
This adds up to 2 + 1 + 5 = 8 chars
So the world case IPv6 string as text with port is 53 bytes.
This is frankly pretty huge, lets say we want 32 addresses to try in order for a connect token:
That's 32 * 53 = 1696 bytes!
However, if addresses were included as binary data, that would be just 128 bits = 16 bytes, + 2 bytes for port. So 18 bytes.
18 bytes each address, I could have 32 * 18 = 576, which is much more reasonable. Add some overhead to this for base64 encode in JSON
has overhead tending to 1.3333, so 32 addresses would fit into 767.99808 bytes.
Subtract this from a conservative modern MTU of 1500, lets assume we have 1400 payload for the connect token, this leaves:
633 bytes for potentially including some base64 user data to pass from the matcher to the dedicated server instance with the connect token.
More if the tokens are configured to have less servers per-token.
------------------
So it seems that the addresses should be specified in a binary format to save space, and this binary format should indicate, one byte, IPv4 or IPv6?
From this point of view, is there even any benefit to implementing the connect token as JSON now?
It could really just be a binary format written to from multiple languages, and it would be more efficient.
Hmmmmm
------------------
Should also include protocol id in the associated data for the AEAD of the connect token.
This would allow trivial rejects for connect tokens with different protocol ids.
Then we can remove the protocol id from the connect token, there is no need to duplicate it.
------------------
The additional data for the connect token AEAD should be converted to little endian format on big endian machines, it's currently passed in host byte order, so machines that are big endian will have problems generating or processing connect tokens.
>>>>>>> aff630801c6124043fc84cd342ecfadccb3cee4a
------------------
Packet processor max packet size is deceptive. You give it a max packet size, it adds encryption overhead to it.
It should be the other way around. Specify the maximum packet size you want to generate (total). Calculate how much packet payload data can be allowed while still hitting that maximum, vs. padding it out.
I think this is more honest, and it is what the user of this system would be expecting. It would also simplify the interface for the packet processor.
------------------
I notice that the allocators aren't checked on the server when a client is disconnected.
It seems that although I have silo'd clients into their own allocator, I don't think I've done enough work (yet) to ensure that a malicious client can't shit up an allocator after it quits.
I really should add some code to check that all allocations are cleaned up after the server cleans up an allocator slot.
This might require adding some code to the transport to invalidate packets in queues that belong to a particular allocator.
This seems a bit weird, but I think, I'm not really comfortable with the allocator setup until I can assert that a client has properly cleaned up all allocations made by its client slot allocator when the server resets that slot.
Same logic applies to message factories. Make sure all messages are cleaned up. This is an important security measure, to make sure the server isn't leaking memory from one client session to the next.
------------------
Client should also do the same checks on the allocator between each connect/disconnect.
This is less important on the client than the server, because it would only bring down the one client, but there should still be no memory leaks as the client connects and disconnects.
------------------
Definitely need more usage documentation and samples showing how to do stuff.
It's not immediately obvious how to use libyojimbo from the samples. Or, what exactly libyojimbo does or even what it's for?
There should definitely be some documentation from the point of view of a new user, how do you use this library? What does it do? etc.
------------------
Probably a good safety measure to verify that a message sent across a channel belongs to the message factory set on the channel.
It's an easy mistake to make, and would be hard to track down otherwise.
------------------
Would be nice if packet factories named each packet type as it is registered, so this could be used in debugging.
Same logic could apply to message factories as well.
------------------
Would be nice if each packet factory or message factory had an easy to access hash (uint64_t) that could be use to quickly test if two factories are compatible.
This hash also input into a protocol id hash now that it's 64 bit as well.
------------------
Add a unit test for user context to make sure it's working end-to-end.
------------------
Add tests that simulate each of the server-side client error conditions, eg. allocator, packet factory, message factory, connection, and then verify that after these errors happen, that client slot is cleared and a client can connect to it again, otherwise, possibility that I have error codes not getting cleared by accident on client disconnect.
------------------
Add functionality to connection and client/server to get the set of packet acks, so users can get acks and build their own systems on top of that.
The current system based around callbacks is unweildy.
------------------
Optimize the simulator to keep track of head and tail internally, so it can avoid scanning the entire 4096 entries on each receive call.
------------------
Really need to make the matcher HTTPS request asynchronous, can't have it blocking the main thread like it does now!
------------------
Implement QoS within the connection, eg. packet loss, latency measurements etc, and make this available to the user to query.
------------------
Clean up profile.cpp with helper methods. It's overly complicated for what it does!
------------------