0RTT, Early data, session ticketing, key verification, stateless reset #147
Conversation
dr7ana
changed the title
dr7ana
force-pushed
the
0rtt
branch
3 times, most recently
from
65b8374 to
060c58b
Compare
dr7ana
changed the title
dr7ana
force-pushed
the
0rtt
branch
2 times, most recently
from
79bc67b to
e89c643
Compare
- By passing `opt::disable_key_verification` in calls to `Endpoint::{listen,connect}(...)` the application can decide whether to use key verification for inbound or outbound connections.
- can be toggled on/off, bespoke types for handling generation and storage - improvements, better handling, callbacks
dr7ana
force-pushed
the
0rtt
branch
5 times, most recently
from
667fa64 to
7244dff
Compare
- ping binaries added to test timing - more thoughtful compile time checking
| @@ -3,11 +3,13 @@ | |||
| #include <oxenc/base64.h> | |||
There was a problem hiding this comment.
(This comment isn't necessary for this PR) Now that we have three separate cpp files implementing the stuff in this header, I'm thinking we should split the header up into something like gnutls/crypto.hpp/gnutls/session.hpp/gnutls/creds.hpp to match the cpp split.
| } | ||
| } // namespace detail | ||
|
|
||
| struct connection_callbacks | ||
| { |
There was a problem hiding this comment.
The struct rename seems fine, but what is the point of moving them into internal.hpp when they are entirely local to connection.cpp?
There was a problem hiding this comment.
I wanted to aggregate all the callback declarations (including session and tls callbacks) internally, as they're defined regardless in the *.cpp files
| conn.reference_id()); | ||
| associate_cid(qcid, conn); | ||
|
|
||
| // We only hold one reset token per connection at a time! |
There was a problem hiding this comment.
A question (about the protocol, not the code): why is there only one at a time?
There was a problem hiding this comment.
I would think that it would be one per active connection ID (which is like default 8 per connection?)
There was a problem hiding this comment.
Talked about this off-band: My understanding is stateless reset is tied to the connection itself and renewed with every new DCID. That is why the cb defined by ngtcp2 is on_dcid_status
Whenever a the new connection id hook is invoked by ngtcp2, it copies the cid token to the current cid stored in the connection, overwriting the old one:
https://github.com/ngtcp2/ngtcp2/blob/67047340bc3f0cffd3217f8be649405a3927065c/lib/ngtcp2_conn.c#L7854-L7857
That's why there's a second map (reset_token_map and reset_token_lookup) to reference all the other quic cid's
| log::trace( | ||
| log_cat, "{} associating CID:{} to {}", conn.is_inbound() ? "SERVER" : "CLIENT", qcid, conn.reference_id()); | ||
|
|
||
| conn_lookup.emplace(qcid, conn.reference_id()); |
There was a problem hiding this comment.
Is there any possibility of a duplicate here that should be worried about?
There was a problem hiding this comment.
It should be many quic_cid's to one ConnectionID (default is 8:1), so I would guess not? Apologies if I'm misunderstanding
There was a problem hiding this comment.
Presumably there's an astronomically unlikely chance that there could be a collision, but I think CIDs are sent encrypted in both directions so a MITM shouldn't be able to force a collision, and either side of a connection intentionally making a new connection and re-using a CID is just messing up their own connection, so it should be fine. At least, if I understand the question correctly.
|
I've now reviewed this all and left various comments. I think this mostly looks good! |
- added RAII gnutls_datum object, used where needed. Must be careful, can double free if the memory is owned by another object or held within ngtcp2 - misc fixes from oxen-io#147
|
Closed and reopened #153 because github is stupid and broke updating this PR when trying to drop a commit |
- added RAII gnutls_datum object, used where needed. Must be careful, can double free if the memory is owned by another object or held within ngtcp2 - misc fixes from oxen-io#147
Scope:
opt::enable_0rtt_ticketingat the endpoint level, and will apply for all connections in/out of that endpoint.opt::disable_key_verificationat the connection level by passing the struct to calls toEndpoint::{listen,connect}(...)opt::disable_stateless_verificationat the endpoint level