> On 10 May 2023, at 21:46, Roman Arutyunyan <a...@nginx.com> wrote:
>
> Hi,
>
> On Tue, May 02, 2023 at 04:34:15PM +0400, Roman Arutyunyan wrote:
>> # HG changeset patch
>> # User Roman Arutyunyan <a...@nginx.com>
>> # Date 1682679819 -14400
>> # Fri Apr 28 15:03:39 2023 +0400
>> # Branch quic
>> # Node ID 43f0ceffa227a33e5c5ceb35b77f9a1f86dd2481
>> # Parent cdc41ec778ffae822fefce639e67f2f57e3667f0
>> QUIC: keep stream sockaddr and addr_text constant.
>>
>> HTTP and Stream variables $remote_addr and $binary_remote_addr rely on
>> constant client address, particularly because they are cacheable.
>> However, QUIC client may migrate to a new address. While there's no perfect
>> way to handle this, the proposed solution is to copy client address to QUIC
>> stream at stream creation. Previously, the address was only referenced,
>> which
>> could result in changing it while stream was active, which in turn would lead
>> to broken cached variables values, since address length is cached as well.
>
> While testing this, it was found that $remote_addr truncation happens at the
> QUIC level since the addr_text string is copied by value and retains the old
> length after migration. The new commit log:
>
> QUIC: keep stream sockaddr and addr_text constant.
>
> HTTP and Stream variables $remote_addr and $binary_remote_addr rely on
> constant client address, particularly because they are cacheable.
> However, QUIC client may migrate to a new address. While there's no perfect
> way to handle this, the proposed solution is to copy client address to QUIC
> stream at stream creation.
>
> The change also fixes truncated $remote_addr if migration happened while the
> stream was active. The reason is addr_text string was copied to stream by
> value.
>
> [..]
>
All series looks good for me.
For the record, reproduced with the following tests I intend to push later.
# HG changeset patch
# User Sergey Kandaurov <pluk...@nginx.com>
# Date 1683800560 -14400
# Thu May 11 14:22:40 2023 +0400
# Branch quic
# Node ID 9bd3e671cdf9f6f4883a77368458721dc2d2b5ac
# Parent 4eb8f6d9bd13a36cb46da6caebf1a2f1a6710f2d
Tests: basic QUIC migration tests.
diff --git a/lib/Test/Nginx/HTTP3.pm b/lib/Test/Nginx/HTTP3.pm
--- a/lib/Test/Nginx/HTTP3.pm
+++ b/lib/Test/Nginx/HTTP3.pm
@@ -92,6 +92,7 @@ sub init {
$self->{dcid} = Crypt::PRNG::random_bytes(18);
$self->{salt} = "\x38\x76\x2c\xf7\xf5\x59\x34\xb3\x4d\x17"
. "\x9a\xe6\xa4\xc8\x0c\xad\xcc\xbb\x7f\x0a";
+ $self->{ncid} = [];
$self->{early_data} = $early_data;
$self->retry();
@@ -277,6 +278,12 @@ sub path_response {
$self->{socket}->syswrite($self->encrypt_aead($frame, 3));
}
+sub ping {
+ my ($self) = @_;
+ my $frame = "\x01\x00\x00\x00";
+ $self->{socket}->syswrite($self->encrypt_aead($frame, 3));
+}
+
###############################################################################
# HTTP/3 routines
@@ -1481,6 +1488,11 @@ sub handle_frames {
]);
}
+ @frames = grep { $_->{type} eq 'NCID' } @$frames;
+ while (my $frame = shift @frames) {
+ push @{$self->{ncid}}, $frame;
+ }
+
my $ack = $self->{ack}[$level];
# stop tracking acknowledged ACK ranges
diff --git a/quic_migration.t b/quic_migration.t
new file mode 100644
--- /dev/null
+++ b/quic_migration.t
@@ -0,0 +1,134 @@
+#!/usr/bin/perl
+
+# (C) Sergey Kandaurov
+# (C) Nginx, Inc.
+
+# Tests for quic migration.
+
+###############################################################################
+
+use warnings;
+use strict;
+
+use Test::More;
+
+BEGIN { use FindBin; chdir($FindBin::Bin); }
+
+use lib 'lib';
+use Test::Nginx;
+use Test::Nginx::HTTP3;
+
+###############################################################################
+
+select STDERR; $| = 1;
+select STDOUT; $| = 1;
+
+eval { require Crypt::Misc; die if $Crypt::Misc::VERSION < 0.067; };
+plan(skip_all => 'CryptX version >= 0.067 required') if $@;
+
+plan(skip_all => '127.0.0.20 local address required')
+ unless defined IO::Socket::INET->new( LocalAddr => '127.0.0.20' );
+
+my $t = Test::Nginx->new()->has(qw/http http_v3/)
+ ->has_daemon('openssl')->plan(2);
+
+$t->write_file_expand('nginx.conf', <<'EOF');
+
+%%TEST_GLOBALS%%
+
+daemon off;
+
+events {
+}
+
+http {
+ %%TEST_GLOBALS_HTTP%%
+
+ ssl_certificate_key localhost.key;
+ ssl_certificate localhost.crt;
+ ssl_protocols TLSv1.3;
+
+ server {
+ listen 127.0.0.1:%%PORT_8980_UDP%% quic;
+ server_name localhost;
+
+ location / {
+ add_header X-IP $remote_addr;
+ }
+ }
+}
+
+EOF
+
+$t->write_file('openssl.conf', <<EOF);
+[ req ]
+default_bits = 2048
+encrypt_key = no
+distinguished_name = req_distinguished_name
+[ req_distinguished_name ]
+EOF
+
+my $d = $t->testdir();
+
+foreach my $name ('localhost') {
+ system('openssl req -x509 -new '
+ . "-config $d/openssl.conf -subj /CN=$name/ "
+ . "-out $d/$name.crt -keyout $d/$name.key "
+ . ">>$d/openssl.out 2>&1") == 0
+ or die "Can't create certificate for $name: $!\n";
+}
+
+$t->write_file('index.html', '');
+$t->run();
+
+###############################################################################
+
+# test that $remote_addr is not truncated after migration (ticket #2488),
+# to test, we migrate to another address large enough in text representation,
+# then send a request on the new path
+
+my $s = Test::Nginx::HTTP3->new();
+$s->new_connection_id(1, 0, "foobar1", "foobarbazbazzzzz");
+
+$s->{socket} = IO::Socket::INET->new(
+ Proto => "udp",
+ LocalAddr => '127.0.0.20',
+ PeerAddr => '127.0.0.1:' . port(8980),
+);
+$s->{scid} = "foobar1";
+$s->{dcid} = $s->{ncid}[0]{cid};
+$s->ping();
+
+my $frames = $s->read(all => [{ type => 'PATH_CHALLENGE' }]);
+my ($frame) = grep { $_->{type} eq "PATH_CHALLENGE" } @$frames;
+$s->path_response($frame->{data});
+
+$frames = $s->read(all => [{ sid => $s->new_stream(), fin => 1 }]);
+($frame) = grep { $_->{type} eq "HEADERS" } @$frames;
+is($frame->{headers}{'x-ip'}, '127.0.0.20', 'remote addr after migration');
+
+# test that $remote_addr is not truncated while in the process of migration;
+# the same but migration occurs on receiving a request stream itself,
+# which is the first non-probing frame on the new path;
+# previously this led to $remote_addr truncation in the following order:
+# - stream held original sockaddr/addr_text references on stream creation
+# - values were rewritten as part of handling connection migration
+# - stream was handled referencing rewritten values, with old local lengths
+# now sockaddr and addr_text are expected to keep copies on stream creation
+
+$s = Test::Nginx::HTTP3->new();
+$s->new_connection_id(1, 0, "foobar1", "foobarbazbazzzzz");
+
+$s->{socket} = IO::Socket::INET->new(
+ Proto => "udp",
+ LocalAddr => '127.0.0.20',
+ PeerAddr => '127.0.0.1:' . port(8980),
+);
+$s->{scid} = "foobar1";
+$s->{dcid} = $s->{ncid}[0]{cid};
+
+$frames = $s->read(all => [{ sid => $s->new_stream(), fin => 1 }]);
+($frame) = grep { $_->{type} eq "HEADERS" } @$frames;
+is($frame->{headers}{'x-ip'}, '127.0.0.1', 'remote addr on migration');
+
+###############################################################################
--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
