Ekr editorial 17 3 #2164

ekr · 2018-12-13T22:12:44Z

No description provided.

draft-ietf-quic-transport.md

mikkelfj · 2018-12-13T22:25:43Z

draft-ietf-quic-transport.md

@@ -1616,7 +1619,8 @@ Token field of its Initial packet.
 A token allows a server to correlate activity between the connection where the
 token was issued and any connection where it is used.  Clients that want to
 break continuity of identity with a server MAY discard tokens provided using the
-NEW_TOKEN frame.  Tokens obtained in Retry packets MUST NOT be discarded.
+NEW_TOKEN frame.  Tokens obtained in Retry packets MUST NOT be discarded
+during connection establishment (they will not be used with new connections).



How about a positive: A token obtained in a Retry packet must be used immediately during the connection attempt and cannot be used in subsequent connection attempts.

What @mikkelfj said.

The parenthetical doesn't really make sense to me - it's true, but I don't see how it is connected to the primary statement.

mikkelfj · 2018-12-13T22:27:54Z

draft-ietf-quic-transport.md

+Path validation tests that packets (PATH_CHALLENGE) can be both sent
+to and received (PATH_RESPONSE) from a peer on the path.  Importantly,
+it validates that the packets received from the migrating endpoint do
+not carry a spoofed source address.



But that isn't exactly true since MITM can spoof the source address of these packets if it can guess the packet content (as you pointed out).

Sure.. I didn't change this text, I just added the parentheticals to make it clearer. This PR is essentially editorial

draft-ietf-quic-transport.md

mikkelfj · 2018-12-13T22:31:26Z

draft-ietf-quic-transport.md

+A new address is considered valid when A PATH_RESPONSE frame is received
+that meets the following criteria:
+
+- It contains that was sent in a previous PATH_CHALLENGE. Receipt of an


Suggested change

- It contains that was sent in a previous PATH_CHALLENGE. Receipt of an

- It contains the data that was sent in a previous PATH_CHALLENGE. Receipt of an

mikkelfj · 2018-12-13T22:34:39Z

draft-ietf-quic-transport.md

+  corresponding PATH_CHALLENGE was sent. If a PATH_RESPONSE frame is
+  received from a different remote address than the one to which the
+  PATH_CHALLENGE was sent, path validation is considered to have failed,
+  even if the data matches that sent in the PATH_CHALLENGE.


Suggested change

even if the data matches that sent in the PATH_CHALLENGE.

even if the PATH_RESPONSE data matches the PATH_CHALLENGE data.

I think this is fine

mikkelfj · 2018-12-13T22:37:35Z

draft-ietf-quic-transport.md

-successful, even if the data matches the PATH_CHALLENGE.  This doesn't result in
-path validation failure, as it might be a result of a forwarded packet (see
-{{off-path-forward}}) or misrouting.
+Note that receipt on a different local address doesn't result in path


Suggested change

Note that receipt on a different local address doesn't result in path

Note that receipt on a different local address does not result in path

Prefer as-is

@janaiyengar insists on avoiding contractions.

mikkelfj · 2018-12-13T22:41:19Z

draft-ietf-quic-transport.md

+A stateless reset will be interpreted by a recipient as a packet with
+a short header.  For the packet to appear as valid, the Random Bits
+field needs to include at least 182 bits of data (or 24 bytes, less
+the two fixed bits). This is intended to allow for a destination


Suggested change

the two fixed bits). This is intended to allow for a destination

the two fixed bits). This is intended to allow for a Destination

mikkelfj · 2018-12-13T22:41:37Z

draft-ietf-quic-transport.md

+a short header.  For the packet to appear as valid, the Random Bits
+field needs to include at least 182 bits of data (or 24 bytes, less
+the two fixed bits). This is intended to allow for a destination
+connection ID of the maximum length permitted, with a minimal packet


Suggested change

connection ID of the maximum length permitted, with a minimal packet

Connection ID of the maximum length permitted, with a minimal packet

I'll let MT weigh in on this. This is existing text.

Our convention is that the term "connection ID" is not capitalized, but the fields named "Connection ID" are.

This is a field, so the capitalization is correct.

draft-ietf-quic-transport.md

Co-Authored-By: ekr <ekr@rtfm.com>

MikeBishop

Mostly improvements or neutral. As a general comment, you appear to be rewrapping your changed paragraphs to a different width than we've been using for these documents, which makes more appear to have changed than actually did.

MikeBishop · 2018-12-13T22:37:00Z

draft-ietf-quic-transport.md

-on the path.  Importantly, it validates that the packets received from the
-migrating endpoint do not carry a spoofed source address.
+Path validation tests that packets (PATH_CHALLENGE) can be both sent
+to and received (PATH_RESPONSE) from a peer on the path.  Importantly,


These are not packets, and the position of the parenthetical phrase is inconsistent between the two options.

I agree, this likely confuses things more than it helps.

draft-ietf-quic-transport.md

MikeBishop · 2018-12-13T22:40:33Z

draft-ietf-quic-transport.md

@@ -2294,7 +2301,7 @@ coalesced (see {{packet-coalesce}}) to facilitate retransmission.

 ## Stateless Reset {#stateless-reset}

-A stateless reset is provided as an option of last resort for an endpoint that
+A stateless reset is provided as an option of last resort for a server that


No, SR is bilateral, even if we expect servers to use them more frequently.

"A client MUST NOT include an original connection ID, a stateless reset token, or
a preferred address. A server MUST treat receipt of any of these transport
parameters as a connection error of type TRANSPORT_PARAMETER_ERROR."

It does appear that a client can send one in NCID, but that's a real inconsistency.

OCID is only applicable to servers because it's about Retry

Preferred Address is only applicable to servers and contains an SRT

SRT is useless if it's ever exchanged in the clear, which the client's TPs are

After the initial CID, the client can provide an SRT and the behavior is bilateral. There was a discussion about enabling the client to supply an SRT for the initial CID after the handshake, but there wasn't great appetite for the added complexity. If the server cares about getting an SRT, it just starts using the next CID as soon as it gets one.

It'd all be a lot simpler if we just went with client and server :-)

MikeBishop · 2018-12-13T22:42:39Z

draft-ietf-quic-transport.md

-have negotiated a packet protection scheme with a larger minimum AEAD expansion.
+A stateless reset will be interpreted by a recipient as a packet with
+a short header.  For the packet to appear as valid, the Random Bits
+field needs to include at least 182 bits of data (or 24 bytes, less


I appreciate that you're removing the suggestion that these should be "random" despite the name of the field. However, it seems worthwhile to keep "unpredictable" at least.

The previous graf dicates the contents. This graf is about how the recipient will interpret them, and the recipient has no good way of knowing if they are random or unpredictable or whatever.

MikeBishop · 2018-12-13T22:43:30Z

draft-ietf-quic-transport.md

+The design of a Stateless Reset is such that without knowing the
+stateless reset token it is indistinguishable from a valid packet.
+This means that if a server sends a Stateless Reset to another server,
+that might trigger the sending of a Stateless Reset in response, which


Can we get rid of one of these "that"s?

feel free to suggest.

"This means that if a server sends a Stateless Reset to another server, it might receive another Stateless Reset in response. As a result...."

One fewer: "If a server sends a Stateless Reset to another server, it might receive another Stateless Reset in response."

MikeBishop · 2018-12-13T22:45:38Z

draft-ietf-quic-transport.md

-The first Handshake packet sent by a server contains a packet number of 0.
-Handshake packets are their own packet number space.  Packet numbers are
-incremented normally for other Handshake packets.
+


Extra line seems unnecessary here.

Suggested change

MikeBishop · 2018-12-13T22:46:45Z

draft-ietf-quic-transport.md

-incremented normally for other Handshake packets.
+
+Handshake packets are their own packet number space, and thus
+the first Handshake packet sent by a server contains a packet number of 0.


Is this a requirement? Last I knew, some implementations were choosing to just keep a counter incrementing across all packet spaces, and that was okay. The key point is that (Initial,X) and (Handshake,X) are different packets, but IIRC there's no requirement that each space start at zero.

" The CRYPTO frame can be sent in different packet number spaces. The
sequence numbers used by CRYPTO frames to ensure ordered delivery of
cryptographic handshake data start from zero in each packet number
space.
"

That's the offset of the CRYPTO stream, not the packet numbers.

My mistake. Here:

https://tools.ietf.org/html/draft-ietf-quic-transport-16#section-12.3

" This enforces cryptographic separation between the data sent in the
different packet sequence number spaces. Each packet number space
starts at packet number 0. Subsequent packets sent in the same
packet number space MUST increase the packet number by at least one."

I think that we're operating on the assumption that you start sending from 0, but there is no strict requirement because it's unenforceable. Loss.

MikeBishop · 2018-12-13T22:48:42Z

draft-ietf-quic-transport.md

+that the original_connection_id transport parameter is present and
+correct; otherwise, it validates that the transport parameter is
+absent.  A client MUST treat a failed validation as a connection error
+of type TRANSPORT_PARAMETER_ERROR.


Adding the MUST to the validate-is-present fork seems unnecessary, but if you're going to, isn't it also a MUST to validate-is-absent?

yes I agree. What I'm trying to do is remove language which is apparently descriptive but is actually normative. I.e., "the agent does X". That's a requirement

Then "otherwise, it MUST validate that...."

I'm OK with the existing or as Mike suggests.

Hah, yes. Suggested above.

martinthomson

Thanks for these. Nits only.

martinthomson · 2018-12-14T05:10:07Z

draft-ietf-quic-transport.md

@@ -1616,7 +1619,8 @@ Token field of its Initial packet.
 A token allows a server to correlate activity between the connection where the
 token was issued and any connection where it is used.  Clients that want to
 break continuity of identity with a server MAY discard tokens provided using the
-NEW_TOKEN frame.  Tokens obtained in Retry packets MUST NOT be discarded.
+NEW_TOKEN frame.  Tokens obtained in Retry packets MUST NOT be discarded
+during connection establishment (they will not be used with new connections).



What @mikkelfj said.

The parenthetical doesn't really make sense to me - it's true, but I don't see how it is connected to the primary statement.

martinthomson · 2018-12-14T05:10:50Z

draft-ietf-quic-transport.md

-on the path.  Importantly, it validates that the packets received from the
-migrating endpoint do not carry a spoofed source address.
+Path validation tests that packets (PATH_CHALLENGE) can be both sent
+to and received (PATH_RESPONSE) from a peer on the path.  Importantly,


I agree, this likely confuses things more than it helps.

martinthomson · 2018-12-14T05:12:40Z

draft-ietf-quic-transport.md

-successful, even if the data matches the PATH_CHALLENGE.  This doesn't result in
-path validation failure, as it might be a result of a forwarded packet (see
-{{off-path-forward}}) or misrouting.
+Note that receipt on a different local address doesn't result in path


@janaiyengar insists on avoiding contractions.

martinthomson · 2018-12-14T05:13:32Z

draft-ietf-quic-transport.md

+the number of packets it sends to this new address until the address
+is validated (see {{migrate-validate}}). A server in the closing state
+MAY instead choose to discard packets received from a new source
+address.


The change to the fill-column is making this harder to review than I'd like. I guess that's just a consequence of insisting on wrapping at <80...

I tried rewrapping it for him, but that makes all the comments outdated, unfortunately. I think this will need a rewrap just before merge instead, unfortunately.

martinthomson · 2018-12-14T05:14:18Z

draft-ietf-quic-transport.md

@@ -2294,7 +2301,7 @@ coalesced (see {{packet-coalesce}}) to facilitate retransmission.

 ## Stateless Reset {#stateless-reset}

-A stateless reset is provided as an option of last resort for an endpoint that
+A stateless reset is provided as an option of last resort for a server that


martinthomson · 2018-12-14T05:15:35Z

draft-ietf-quic-transport.md

+The design of a Stateless Reset is such that without knowing the
+stateless reset token it is indistinguishable from a valid packet.
+This means that if a server sends a Stateless Reset to another server,
+that might trigger the sending of a Stateless Reset in response, which


One fewer: "If a server sends a Stateless Reset to another server, it might receive another Stateless Reset in response."

martinthomson · 2018-12-14T05:16:14Z

draft-ietf-quic-transport.md

-The first Handshake packet sent by a server contains a packet number of 0.
-Handshake packets are their own packet number space.  Packet numbers are
-incremented normally for other Handshake packets.
+


Suggested change

martinthomson · 2018-12-14T05:17:10Z

draft-ietf-quic-transport.md

-incremented normally for other Handshake packets.
+
+Handshake packets are their own packet number space, and thus
+the first Handshake packet sent by a server contains a packet number of 0.


I think that we're operating on the assumption that you start sending from 0, but there is no strict requirement because it's unenforceable. Loss.

martinthomson · 2018-12-14T05:17:43Z

draft-ietf-quic-transport.md

+that the original_connection_id transport parameter is present and
+correct; otherwise, it validates that the transport parameter is
+absent.  A client MUST treat a failed validation as a connection error
+of type TRANSPORT_PARAMETER_ERROR.


I'm OK with the existing or as Mike suggests.

martinthomson · 2018-12-14T05:18:37Z

draft-ietf-quic-transport.md

@@ -5326,7 +5338,7 @@ An accompanying transport parameter registration (see
 specification needs to describe the format and assigned semantics of any fields
 in the frame.

-Expert(s) are encouraged to be biased towards approving registrations unless
+Expert(s) SHOULD be biased towards approving registrations unless


Not really an interoperability requirement.

Suggested change

Expert(s) SHOULD be biased towards approving registrations unless

Expert(s) are encouraged to be biased towards approving registrations unless

janaiyengar · 2018-12-15T00:35:53Z

draft-ietf-quic-transport.md

-validation as a connection error of type TRANSPORT_PARAMETER_ERROR.
+If the client received and processed a Retry packet, it MUST validate
+that the original_connection_id transport parameter is present and
+correct; otherwise, it validates that the transport parameter is


Suggested change

correct; otherwise, it validates that the transport parameter is

correct; otherwise, it MUST validate that the transport parameter is

janaiyengar · 2018-12-15T00:38:27Z

draft-ietf-quic-transport.md

-containing data that was sent in a previous PATH_CHALLENGE. Receipt of an
-acknowledgment for a packet containing a PATH_CHALLENGE frame is not adequate
-validation, since the acknowledgment can be spoofed by a malicious peer.
+A new address is considered valid when A PATH_RESPONSE frame is received


Suggested change

A new address is considered valid when A PATH_RESPONSE frame is received

A new address is considered valid when a PATH_RESPONSE frame is received

janaiyengar · 2018-12-15T00:39:43Z

draft-ietf-quic-transport.md

-sent. If a PATH_RESPONSE frame is received from a different remote address than
-the one to which the PATH_CHALLENGE was sent, path validation is considered to
-have failed, even if the data matches that sent in the PATH_CHALLENGE.
+- It is from the same remote address as that to which the


Suggested change

- It is from the same remote address as that to which the

- It is sent from the same remote address to which the

martinthomson · 2018-12-19T06:13:00Z

Merged manually.

ekr added 3 commits December 12, 2018 08:44

some more editorial work

ef973fe

Some more cleanup

1dcac01

More editorial work

841e8c3

mikkelfj reviewed Dec 13, 2018

View reviewed changes

mikkelfj and others added 2 commits December 13, 2018 14:47

Update draft-ietf-quic-transport.md

ade5b40

Co-Authored-By: ekr <ekr@rtfm.com>

Apply suggestions from code review

cc14d24

Co-Authored-By: ekr <ekr@rtfm.com>

MikeBishop reviewed Dec 13, 2018

View reviewed changes

martinthomson reviewed Dec 14, 2018

View reviewed changes

MikeBishop force-pushed the ekr_editorial_17_3 branch from 5f6331f to cc14d24 Compare December 15, 2018 00:29

janaiyengar reviewed Dec 15, 2018

View reviewed changes

Review comments

ceba5d1

martinthomson closed this Dec 19, 2018

	- It contains that was sent in a previous PATH_CHALLENGE. Receipt of an
	- It contains the data that was sent in a previous PATH_CHALLENGE. Receipt of an

	even if the data matches that sent in the PATH_CHALLENGE.
	even if the PATH_RESPONSE data matches the PATH_CHALLENGE data.

	Note that receipt on a different local address doesn't result in path
	Note that receipt on a different local address does not result in path

	the two fixed bits). This is intended to allow for a destination
	the two fixed bits). This is intended to allow for a Destination

	connection ID of the maximum length permitted, with a minimal packet
	Connection ID of the maximum length permitted, with a minimal packet

	Expert(s) SHOULD be biased towards approving registrations unless
	Expert(s) are encouraged to be biased towards approving registrations unless

	correct; otherwise, it validates that the transport parameter is
	correct; otherwise, it MUST validate that the transport parameter is

	A new address is considered valid when A PATH_RESPONSE frame is received
	A new address is considered valid when a PATH_RESPONSE frame is received

	- It is from the same remote address as that to which the
	- It is sent from the same remote address to which the

Ekr editorial 17 3 #2164

Ekr editorial 17 3 #2164

Conversation

ekr commented Dec 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeBishop left a comment

Choose a reason for hiding this comment

MikeBishop Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar Dec 15, 2018 • edited Loading

Choose a reason for hiding this comment

martinthomson commented Dec 19, 2018

MikeBishop Dec 13, 2018 •

edited

Loading

janaiyengar Dec 15, 2018 •

edited

Loading