Re: [quicwg/base-drafts] Rework Retry packet (#1498)
ianswett <notifications@github.com> Fri, 29 June 2018 19:06 UTC
Return-Path: <noreply@github.com>
X-Original-To: quic-issues@ietfa.amsl.com
Delivered-To: quic-issues@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id A8414130E27 for <quic-issues@ietfa.amsl.com>; Fri, 29 Jun 2018 12:06:52 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -8.01
X-Spam-Level:
X-Spam-Status: No, score=-8.01 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=0.001, MAILING_LIST_MULTI=-1, RCVD_IN_DNSWL_HI=-5, SPF_PASS=-0.001, T_DKIMWL_WL_HIGH=-0.01] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=github.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 0R5uGrrArnXi for <quic-issues@ietfa.amsl.com>; Fri, 29 Jun 2018 12:06:50 -0700 (PDT)
Received: from out-15.smtp.github.com (out-15.smtp.github.com [192.30.254.198]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 8ED83130E26 for <quic-issues@ietf.org>; Fri, 29 Jun 2018 12:06:50 -0700 (PDT)
Date: Fri, 29 Jun 2018 12:06:49 -0700
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1530299210; bh=/RkDGj+jab4J7wmCHP3taLUn7zuK+otj729AZzbXw48=; h=Date:From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=oSEqAq+J+VSZZt1bLsDhoCqOXxm3MwI4wXPDND0em1lOr/c0bUm2MWOnKjtDaWLI+ SSHYdqkarzseNQ/H19qb3z1mXqLH6W6B7cgeExM6ZoZ0wiVuSoBmdraXnrj1PuZJ7P ZeKIVVRxYDH+inCQwAW9sRdBXVfoQZ4434B4iHOY=
From: ianswett <notifications@github.com>
Reply-To: quicwg/base-drafts <reply+0166e4abc47445cd54144d923a0378a84b83950a416ebb3792cf00000001174e454992a169ce14138c09@reply.github.com>
To: quicwg/base-drafts <base-drafts@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <quicwg/base-drafts/pull/1498/review/133337121@github.com>
In-Reply-To: <quicwg/base-drafts/pull/1498@github.com>
References: <quicwg/base-drafts/pull/1498@github.com>
Subject: Re: [quicwg/base-drafts] Rework Retry packet (#1498)
Mime-Version: 1.0
Content-Type: multipart/alternative; boundary="--==_mimepart_5b368349a9ff1_1b1d3fd945ab2f80125596"; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: list
X-GitHub-Sender: ianswett
X-GitHub-Recipient: quic-issues
X-GitHub-Reason: subscribed
X-Auto-Response-Suppress: All
X-GitHub-Recipient-Address: quic-issues@ietf.org
Archived-At: <https://mailarchive.ietf.org/arch/msg/quic-issues/dAQzbCA6PeEPBcKZyosL5w1a96E>
X-BeenThere: quic-issues@ietf.org
X-Mailman-Version: 2.1.26
List-Id: Notification list for GitHub issues related to the QUIC WG <quic-issues.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/quic-issues/>
List-Post: <mailto:quic-issues@ietf.org>
List-Help: <mailto:quic-issues-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 29 Jun 2018 19:06:53 -0000
ianswett commented on this pull request. This looks like an improvement overall and clarifies some points. > +| Version (32) | ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +|DCIL(4)|SCIL(4)| ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +| Destination Connection ID (0/32..144) ... ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +| Source Connection ID (0/32..144) ... ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +| ODCIL(8) | Original Destination Connection ID (*) | ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +| Retry Token (*) ... ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +~~~ +{: #retry-format title="Retry Packet"} + +A Retry packet (shown in {{retry-format}}) only the invariant portion of the "A Retry packet only" uses? > + 0 1 2 3 + 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 ++-+-+-+-+-+-+-+-+ +|1| 0x7e | ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +| Version (32) | ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +|DCIL(4)|SCIL(4)| ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +| Destination Connection ID (0/32..144) ... ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +| Source Connection ID (0/32..144) ... ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +| ODCIL(8) | Original Destination Connection ID (*) | ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +| Retry Token (*) ... I'm realizing the lack of length means one can't 'coalesce' a RETRY and another packet type in a single datagram. I can't think why this is a practical issue, but it wouldn't surprise me if there was a use for it at some point, so I think it makes sense to add a length. > + +Retry Token: + +: An opaque token that the server can use to validate the client's address. + +The server populates the Destination Connection ID with the connection ID that +the client included in the Source Connection ID of the Initial packet. + +The server includes a connection ID of its choice in the Source Connection ID +field. The client MUST use this connection ID in the Destination Connection ID +of subsequent packets that it sends. + +A Retry packet does not include a packet number and cannot be explictly +acknowledged by a client. + +A server MUST only send a Retry in response to a client Initial packet. I think what Martin has is good, but I can imagine a DDoS device doing what you said, especially if it kept a small amount of state to ensure it didn't sent too many RETRY packets to a given IP/port. So this could be a SHOULD instead of a MUST. > +The server populates the Destination Connection ID with the connection ID that +the client included in the Source Connection ID of the Initial packet. + +The server includes a connection ID of its choice in the Source Connection ID +field. The client MUST use this connection ID in the Destination Connection ID +of subsequent packets that it sends. + +A Retry packet does not include a packet number and cannot be explictly +acknowledged by a client. + +A server MUST only send a Retry in response to a client Initial packet. + +If the Original Destination Connection ID field does not match the Destination +Connection ID from the most recent the Initial packet it sent, clients MUST +discard the packet. This prevents an off-path attacker from injecting a Retry +packet with a bogus new Source Connection ID. I think "with a bogus new Source Connection ID." is unnecessary here, since any off-path injection is bad. > Token Length: : A variable-length integer specifying the length of the Token field, in bytes. - It may be zero if no token is present. Initial packets sent by the server + This value is zero if no token is present. Initial packets sent by the server MUST include a zero-length token. I think this would be clearer as "Initial packets sent by the server must specify a zero token length." > +Note that if the server sends a HelloRetryRequest, the client will send a second +Initial packet with a CRYPTO frame with an offset starting at the end of the +CRYPTO stream in the first Initial. "end of the CRYPTO stream" makes it sound like it's after the stream has ended, ie: been FIN'd. Possibly "with an offset starting after the CRYPTO data in the first Initial." > @@ -624,10 +724,18 @@ server, it MUST discard any packet it receives with a different Source Connection ID. -#### Tokens +### Tokens + +If the client has an unused token that it received in a NEW_TOKEN frame on a nit: Unnecessary "that" > @@ -624,10 +724,18 @@ server, it MUST discard any packet it receives with a different Source Connection ID. -#### Tokens +### Tokens + +If the client has an unused token that it received in a NEW_TOKEN frame on a +previous connection to what it believes to be the same server, it includes that it SHOULD include that? > -If the client has a suitable token available from a previous connection, it -SHOULD populate the Token field. +A client MUST NOT reuse a token if it believes that its point of network By saying unused in the previous paragraph, you imply tokens should never be used more than once. But this paragraph seems to imply it's ok if you haven't changed IP. Which do you intend? > @@ -649,85 +757,27 @@ Note: the packet is that the client might have received the token in a previous connection using the NEW_TOKEN frame, and if the server has lost state, it might be unable to validate the token at all, leading to connection failure if - the packet is discarded. + the packet is discarded. A server might encode tokens that it provides with might -> MAY? And I'd suggest "that it provides" -> "provided" > -The first Initial packet contains a packet number of 0. Each packet sent after -the Initial packet is associated with a packet number space and its packet -number increases monotonically in that space (see {{packet-numbers}}). +The first Initial packet sent by either endpoint contains a packet number of SHOULD contain? > -The first Initial packet contains a packet number of 0. Each packet sent after -the Initial packet is associated with a packet number space and its packet -number increases monotonically in that space (see {{packet-numbers}}). +The first Initial packet sent by either endpoint contains a packet number of +0. The packet number increases monotonically thereafter. Initial packets are in MUST increase -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/quicwg/base-drafts/pull/1498#pullrequestreview-133337121
- Re: [quicwg/base-drafts] Rework Retry packet (#14… janaiyengar
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Christian Huitema
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Nick Banks
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Kazuho Oku
- Re: [quicwg/base-drafts] Rework Retry packet (#14… ekr
- [quicwg/base-drafts] Rework Retry packet (#1498) Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… MikkelFJ
- Re: [quicwg/base-drafts] Rework Retry packet (#14… MikkelFJ
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Nick Banks
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Nick Banks
- Re: [quicwg/base-drafts] Rework Retry packet (#14… ekr
- Re: [quicwg/base-drafts] Rework Retry packet (#14… ianswett
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Mike Bishop
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… MikkelFJ
- Re: [quicwg/base-drafts] Rework Retry packet (#14… MikkelFJ
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… ianswett
- Re: [quicwg/base-drafts] Rework Retry packet (#14… ianswett
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Christian Huitema
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Mike Bishop
- Re: [quicwg/base-drafts] Rework Retry packet (#14… MikkelFJ
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… MikkelFJ
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Mike Bishop
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Marten Seemann
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Christian Huitema
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Marten Seemann
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Nick Banks
- Re: [quicwg/base-drafts] Rework Retry packet (#14… MikkelFJ
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Nick Banks
- Re: [quicwg/base-drafts] Rework Retry packet (#14… MikkelFJ
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Nick Banks
- Re: [quicwg/base-drafts] Rework Retry packet (#14… MikkelFJ
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Nick Banks
- Re: [quicwg/base-drafts] Rework Retry packet (#14… MikkelFJ
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Nick Banks
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Nick Banks
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson
- Re: [quicwg/base-drafts] Rework Retry packet (#14… Martin Thomson