Subject: Re: [quicwg/base-drafts] What needs to be checked for address validation (#3327)

From: ekr <[email-masked]>
Date: Tue, 04 Feb 2020 05:40:47 -0800

ekr commented on this pull request.

> @@ -1814,10 +1814,13 @@ tokens that would be accepted by the server.  Only the server requires access to
 the integrity protection key for tokens.
 
 There is no need for a single well-defined format for the token because the
-server that generates the token also consumes it.  A token could include
-information about the claimed client address (IP and port), a timestamp, and any
-other supplementary information the server will need to validate the token in
-the future.
+server that generates the token also consumes it.  Tokens sent in Retry packets
+SHOULD include information that allows the server to verify that the source IP

I agree with @kazuho: suppose that one has a scheme where you store tokens, then the token wouldn't contain this information. Isn't the requirement here that the token be *bound* to an IP.

> @@ -1814,10 +1814,13 @@ tokens that would be accepted by the server.  Only the server requires access to
 the integrity protection key for tokens.
 
 There is no need for a single well-defined format for the token because the
-server that generates the token also consumes it.  A token could include
-information about the claimed client address (IP and port), a timestamp, and any
-other supplementary information the server will need to validate the token in
-the future.
+server that generates the token also consumes it.  Tokens sent in Retry packets
+SHOULD include information that allows the server to verify that the source IP
+address and port in client packets remains constant.  Servers MUST ensure that
+replay of tokens is prevented or limited.  For instance, servers might limit the
+time over which a token is accepted.  Tokens sent in NEW_TOKEN frames MAY omit
+the client port and allow for use over a longer period.  Tokens MAY include
+additional information about clients to further narrow applicability or reuse.

This is actually a pretty confusing situation that the document sees to handle badly.

Suppose that a client connects with address 1.2.3.4 and the server issues a NEW_TOKEN with address 1.2.3.4. When the client comes back it has 5.6.7.8. The server might accept the token for purposes of anti-DoS (e.g., it might otherwise send Retry), but should not accept it for the purposes of exempting the client from the anti-amplification limits. 

-- 
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/3327#discussion_r374672269

---

From: Martin Thomson <[email-masked]>
Date: Tue, 04 Feb 2020 07:54:22 -0800

martinthomson commented on this pull request.

maybe it doesn't use that word, but that is what it says

-- 
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/3327#discussion_r374757399

---

From: Martin Thomson <[email-masked]>
Date: Tue, 11 Feb 2020 16:20:17 -0800

@martinthomson pushed 2 commits.

4c848c6184e7eeefc37d1b9432c3434754b4ac5e  what needs to be checked for address validation
3fc2b7823c89d05cabd98a206b50722af2429dd5  Editorial changes

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3327/files/819e86a3694ced49df1bac649660b7e78f4407e3..3fc2b7823c89d05cabd98a206b50722af2429dd5

---

From: Martin Thomson <[email-masked]>
Date: Tue, 11 Feb 2020 16:21:27 -0800

@ekr and @kazuho would you mind taking a look at these changes.  I considered doing a follow-up, but I'm going to ask the chairs to re-run the consensus call for this instead.

-- 
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/3327#issuecomment-584937776

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 11 Feb 2020 17:20:12 -0800

kazuho commented on this pull request.

@martinthomson Thank you for the ping. LGTM modulo the point below.

> @@ -1829,10 +1829,21 @@ tokens that would be accepted by the server.  Only the server requires access to
 
 There is no need for a single well-defined format for the token because the
-server that generates the token also consumes it.  A token could include
-information about the claimed client address (IP and port), a timestamp, and any
-other supplementary information the server will need to validate the token in
-the future.
+server that generates the token also consumes it.  Tokens sent in Retry packets
+SHOULD include information that allows the server to verify that the source IP
+address and port in client packets remains constant.
+
+Servers might use tokens from NEW_TOKEN in deciding not to send a Retry packet,
+even if the client address has changed.  A token that was provided in
+NEW_TOKEN cannot be used for address validation if the client address is not the
+same, though servers MAY allow for the possibility of changes arising from new
+mappings at a NAT.

Would it make sense to change "though..." to something like: "unless the server can assume that the address change happened within the same network (e.g., NAT rebinding)."?

The reason I suggest this change is because client address might change due to reasons other than NAT, for example, DHCP or SLAAC.

-- 
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/3327#pullrequestreview-357132605

---

From: Martin Thomson <[email-masked]>
Date: Tue, 11 Feb 2020 17:30:11 -0800

I think that new mappings at a NAT are all I really want to allow for here, recognizing that this is likely indistinguishable from a new address from SLAAC or a DHCP lease.

We shouldn't really want a server to spray packets at unwitting hosts within the network just because another one of those hosts has a valid token, more so for large carrier-grade NATs.  But nor can we realistically prevent that, so this needs to be a little squishy.

-- 
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/3327#discussion_r377995957

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 11 Feb 2020 18:07:36 -0800

Thank you for the response. To me it seems that what we are actually looking for is an upper-case SHOULD that limits the scope of NEW_TOKEN tokens to one IP address. IMO that is the basis that we need to state. In addition, we can talk about when the scope can be wider, but that is less important.

I am not sure where the upper-case SHOULD should go into. It could be this section or [section 8.1.3](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#name-address-validation-for-futu). But assuming that it is the former, maybe we can replace this paragraph with something as simple as: "Tokens sent in NEW_TOKEN frames SHOULD include information that allows the server to verify if the client address is stable."

WDYT?

-- 
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/3327#discussion_r378005184

---

From: Martin Thomson <[email-masked]>
Date: Thu, 13 Feb 2020 21:21:18 -0800

(Oops, missed this comment.)

I didn't want to recommend that the IP address remain constant as NAT does not always limit itself to port changes over time.  However, I suspect that the right answer in that case is to not assume that the address is valid.  NATs that use multiple addresses often serve many clients, so the risk of amplification is heightened.

That said, I do like the brevity of what you have.  I'll use that, but I do want to keep the Retry sentence (that's important).

-- 
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/3327#discussion_r379254861

---

From: Martin Thomson <[email-masked]>
Date: Thu, 13 Feb 2020 21:24:35 -0800

55dfc6f01ac290121e8693e5fb66dea0c37e253b  A little less detail
01b60ac478f8476a016232d95b82b1d8603983ff  But 'stable' needs definition or it is just weaselly

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3327/files/3fc2b7823c89d05cabd98a206b50722af2429dd5..01b60ac478f8476a016232d95b82b1d8603983ff

---

From: ekr <[email-masked]>
Date: Fri, 14 Feb 2020 05:45:52 -0800

> @@ -1834,10 +1834,9 @@ SHOULD include information that allows the server to verify that the source IP
 address and port in client packets remains constant.
 
 Servers might use tokens from NEW_TOKEN in deciding not to send a Retry packet,
-even if the client address has changed.  A token that was provided in
-NEW_TOKEN cannot be used for address validation if the client address is not the
-same, though servers MAY allow for the possibility of changes arising from new
-mappings at a NAT.
+even if the client address has changed. Tokens sent in NEW_TOKEN frames SHOULD
+include information that allows the server to verify if the client address is
+stable, but might allow for different NAT bindings or ephemeral port selection.

I don't really understand the second sentence. What's an example of information that would allow the server to verify if the client address is stable.

-- 
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/3327#pullrequestreview-358948901

---

From: Martin Thomson <[email-masked]>
Date: Sun, 16 Feb 2020 15:10:53 -0800

== on IP and port would be one example, but the emphemeral port example clearly leads to just IP == IP.

Should we just say "IP MUST be the same" instead?  That's not good always (see CGNAT) and I want to avoid creating the impression that this is sufficient, even if it might be sufficient.

-- 
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/3327#discussion_r379941990

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 16 Feb 2020 15:53:44 -0800

Yeah one way of moving forward would be to state the intent along with how it might be implemented, rather than using RFC 2119 words for explaining how it should be implemented.

For example: "MUST validate that the network path between the peers have not changed (e.g., client's IP address is stable)."

-- 
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/3327#discussion_r379945192

---

From: Martin Thomson <[email-masked]>
Date: Sun, 16 Feb 2020 17:49:52 -0800

@martinthomson pushed 1 commit.

d9ccbdcbddfd77755d331cc3892f9e3c214f939e  Tighten requirement; explain it some

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3327/files/01b60ac478f8476a016232d95b82b1d8603983ff..d9ccbdcbddfd77755d331cc3892f9e3c214f939e

---

From: ekr <[email-masked]>
Date: Mon, 17 Feb 2020 09:45:11 -0800

> @@ -1834,9 +1834,18 @@ SHOULD include information that allows the server to verify that the source IP
 
 Servers might use tokens from NEW_TOKEN in deciding not to send a Retry packet,
-even if the client address has changed. Tokens sent in NEW_TOKEN frames SHOULD
-include information that allows the server to verify if the client address is
-stable, but might allow for different NAT bindings or ephemeral port selection.
+even if the client address has changed. Tokens sent in NEW_TOKEN frames MUST
+include information that allows the server to verify that the client IP address
+has not changed from when the token was issued.

I think this text would be a lot clearer if you moved the first sentence to the end and wrote it as:

"Servers might use tokens from NEW_TOKEN in deciding not to send a Retry packet,
even if the client address has changed. However, if the address has changed, the
server MUST adhere to the anti-amplification limits found in XXX"

-- 
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/3327#pullrequestreview-359895222

---

From: Martin Thomson <[email-masked]>
Date: Mon, 17 Feb 2020 21:22:19 -0800

9c5c738c1d0cd4c8c7ade6c1371bf81033d02c57  Take ekr's suggestion, remove note.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3327/files/d9ccbdcbddfd77755d331cc3892f9e3c214f939e..9c5c738c1d0cd4c8c7ade6c1371bf81033d02c57

---

From: Martin Thomson <[email-masked]>
Date: Mon, 17 Feb 2020 21:25:23 -0800

bc10a8ea253a7a50a38c420f7c5bbb06e0078a78  Tweak

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3327/files/9c5c738c1d0cd4c8c7ade6c1371bf81033d02c57..bc10a8ea253a7a50a38c420f7c5bbb06e0078a78

---

From: Martin Thomson <[email-masked]>
Date: Mon, 17 Feb 2020 21:27:24 -0800

That works.  I think that is better, though I want to be more precise about "address" vs. "IP address" here.  "Address" in this doc refers to IP+port, and I think that we do want to allow clients to choose different ephemeral ports or to be behind a NAT without absolutely requiring servers to use address validation.  I know that adds a bit of uncertainty to this, but it's a compromise.

I've replaced "address" with "IP address" and then replaced the long note with a shorter sentence explaining that IP alone is not perfect.  I think that we might need wider discussion on this one point.

-- 
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/3327#discussion_r380460915

---

From: ekr <[email-masked]>
Date: Tue, 18 Feb 2020 06:31:17 -0800

> @@ -1829,10 +1829,24 @@ tokens that would be accepted by the server.  Only the server requires access to

As you suggest in your comment, I think i would remove "port" here, as there are not that many applications where returning clients will have the same port.

-- 
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/3327#pullrequestreview-360383137

---

From: Martin Thomson <[email-masked]>
Date: Tue, 18 Feb 2020 17:56:38 -0800

As this is just for Retry, we can include port. 

-- 
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/3327#discussion_r381038185