Skip to content

Commit

Permalink
Improved path challenge handling.
Browse files Browse the repository at this point in the history
1) We are willing to reply to up to 5 outstanding challenges
on a connection, as it is legal for a client to send more than
one.  We limit to 5 to prevent excessive challenging.

2) We now accept path challege responses on any network path,
in accordance with RFC 9000 section 8.2.3.
  • Loading branch information
rthalley committed Mar 11, 2024
1 parent ae282aa commit f8eee22
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 21 deletions.
53 changes: 32 additions & 21 deletions src/aioquic/quic/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@
"1": tls.Epoch.ONE_RTT,
}
MAX_EARLY_DATA = 0xFFFFFFFF
MAX_REMOTE_CHALLENGES = 5
MAX_LOCAL_CHALLENGES = 5
SECRETS_LABELS = [
[
None,
Expand Down Expand Up @@ -197,14 +199,14 @@ class QuicConnectionState(Enum):
TERMINATED = 4


@dataclass
class QuicNetworkPath:
addr: NetworkAddress
bytes_received: int = 0
bytes_sent: int = 0
is_validated: bool = False
local_challenge: Optional[bytes] = None
remote_challenge: Optional[bytes] = None
def __init__(self, addr: NetworkAddress, is_validated: bool = False):
self.addr: NetworkAddress = addr
self.bytes_received: int = 0
self.bytes_sent: int = 0
self.is_validated: bool = is_validated
self.local_challenge_sent: bool = False
self.remote_challenges: Deque[bytes] = deque()

def can_send(self, size: int) -> bool:
return self.is_validated or (self.bytes_sent + size) <= 3 * self.bytes_received
Expand Down Expand Up @@ -308,6 +310,7 @@ def __init__(
self._host_cid_seq = 1
self._local_ack_delay_exponent = 3
self._local_active_connection_id_limit = 8
self._local_challenges: Dict[bytes, QuicNetworkPath] = {}
self._local_initial_source_connection_id = self._host_cids[0].cid
self._local_max_data = Limit(
frame_type=QuicFrameType.MAX_DATA,
Expand Down Expand Up @@ -1996,7 +1999,7 @@ def _handle_path_challenge_frame(
self._quic_logger.encode_path_challenge_frame(data=data)
)

context.network_path.remote_challenge = data
context.network_path.remote_challenges.append(data)

def _handle_path_response_frame(
self, context: QuicReceiveContext, frame_type: int, buf: Buffer
Expand All @@ -2012,16 +2015,16 @@ def _handle_path_response_frame(
self._quic_logger.encode_path_response_frame(data=data)
)

if data != context.network_path.local_challenge:
try:
network_path = self._local_challenges.pop(data)
except KeyError:
raise QuicConnectionError(
error_code=QuicErrorCode.PROTOCOL_VIOLATION,
frame_type=frame_type,
reason_phrase="Response does not match challenge",
)
self._logger.debug(
"Network path %s validated by challenge", context.network_path.addr
)
context.network_path.is_validated = True
self._logger.debug("Network path %s validated by challenge", network_path.addr)
network_path.is_validated = True

def _handle_ping_frame(
self, context: QuicReceiveContext, frame_type: int, buf: Buffer
Expand Down Expand Up @@ -2738,6 +2741,14 @@ def _update_traffic_key(
cipher_suite=cipher_suite, secret=secret, version=self._version
)

def _add_local_challenge(self, challenge: bytes, network_path: QuicNetworkPath):
self._local_challenges[challenge] = network_path
while len(self._local_challenges) > MAX_LOCAL_CHALLENGES:
# Dictionaries are ordered, so pop the first key until we are below the
# limit.
key = next(iter(self._local_challenges.keys()))
del self._local_challenges[key]

def _write_application(
self, builder: QuicPacketBuilder, network_path: QuicNetworkPath, now: float
) -> None:
Expand Down Expand Up @@ -2772,22 +2783,22 @@ def _write_application(
self._handshake_done_pending = False

# PATH CHALLENGE
if (
not network_path.is_validated
and network_path.local_challenge is None
):
if not (network_path.is_validated or network_path.local_challenge_sent):
challenge = os.urandom(8)
self._add_local_challenge(
challenge=challenge, network_path=network_path
)
self._write_path_challenge_frame(
builder=builder, challenge=challenge
)
network_path.local_challenge = challenge
network_path.local_challenge_sent = True

# PATH RESPONSE
if network_path.remote_challenge is not None:
while len(network_path.remote_challenges) > 0:
challenge = network_path.remote_challenges.popleft()
self._write_path_response_frame(
builder=builder, challenge=network_path.remote_challenge
builder=builder, challenge=challenge
)
network_path.remote_challenge = None

# NEW_CONNECTION_ID
for connection_id in self._host_cids:
Expand Down
42 changes: 42 additions & 0 deletions tests/test_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from aioquic.quic import events
from aioquic.quic.configuration import SMALLEST_MAX_DATAGRAM_SIZE, QuicConfiguration
from aioquic.quic.connection import (
MAX_LOCAL_CHALLENGES,
STREAM_COUNT_MAX,
NetworkAddress,
QuicConnection,
Expand Down Expand Up @@ -1703,6 +1704,47 @@ def test_handle_path_challenge_frame(self):
self.assertEqual(server._network_paths[1].addr, ("1.2.3.4", 1234))
self.assertTrue(server._network_paths[1].is_validated)

def test_handle_path_challenge_response_on_different_path(self):
with client_and_server() as (client, server):
# client changes address and sends some data
client.send_stream_data(0, b"01234567")
for data, addr in client.datagrams_to_send(now=time.time()):
server.receive_datagram(data, ("1.2.3.4", 2345), now=time.time())

# check paths
self.assertEqual(len(server._network_paths), 2)
self.assertEqual(server._network_paths[0].addr, ("1.2.3.4", 2345))
self.assertFalse(server._network_paths[0].is_validated)
self.assertEqual(server._network_paths[1].addr, ("1.2.3.4", 1234))
self.assertTrue(server._network_paths[1].is_validated)

# server sends PATH_CHALLENGE and receives PATH_RESPONSE on the 1234
# path instead of the expected 2345 path.
for data, addr in server.datagrams_to_send(now=time.time()):
client.receive_datagram(data, SERVER_ADDR, now=time.time())
for data, addr in client.datagrams_to_send(now=time.time()):
server.receive_datagram(data, ("1.2.3.4", 1234), now=time.time())

# check paths; note that the order is backwards from the prior test
# as receiving on 1234 promotes it to first in the list
self.assertEqual(server._network_paths[0].addr, ("1.2.3.4", 1234))
self.assertTrue(server._network_paths[0].is_validated)
self.assertEqual(server._network_paths[1].addr, ("1.2.3.4", 2345))
self.assertTrue(server._network_paths[1].is_validated)

def test_local_path_challenges_are_bounded(self):
with client_and_server() as (client, server):
for i in range(MAX_LOCAL_CHALLENGES + 2):
server._add_local_challenge(
int.to_bytes(i, 8, "big"), QuicNetworkPath(f"1.2.3.{i}")
)
self.assertEqual(len(server._local_challenges), MAX_LOCAL_CHALLENGES)
for i in range(2, MAX_LOCAL_CHALLENGES + 2):
self.assertEqual(
server._local_challenges[int.to_bytes(i, 8, "big")].addr,
f"1.2.3.{i}",
)

def test_handle_path_response_frame_bad(self):
with client_and_server() as (client, server):
# server receives unsolicited PATH_RESPONSE
Expand Down

0 comments on commit f8eee22

Please sign in to comment.