Subject: [quicwg/base-drafts] [transport] Add FNV Hash on Long Header (#678)

From: Alexis La Goutte <[email-masked]>
Date: Tue, 04 Jul 2017 03:29:37 -0700

You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/678

-- Commit Summary --

  * [transport] Add FNV Hash on Long Header

-- File Changes --

    M draft-ietf-quic-transport.md (10)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/678.patch
https://github.com/quicwg/base-drafts/pull/678.diff

-- 
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/678

---

From: Alexis La Goutte <[email-masked]>
Date: Tue, 04 Jul 2017 03:31:06 -0700

thanks to @tatsuhiro-t for the pointer when write wireshark dissector

Open to review 
* may be change name => only hash ?
* don't fix a length
...

-- 
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/678#issuecomment-312844249

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 04 Jul 2017 18:32:49 -0700

kazuho commented on this pull request.

> @@ -367,6 +367,10 @@ version negotiation and 1-RTT keys are established.
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                          Payload (*)                        ...
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                                                               |
++                        [FNV Hash(64)]                         +
+|                                                               |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Isn't the FNV1 hash expected to be appended to the packet (not the header)?

[Section 6.1 of QUIC-TLS](https://quicwg.github.io/base-drafts/draft-ietf-quic-tls.html#rfc.section.6.1) states:
> The sender then calculates the integrity check over the entire packet, starting from the type field. The output of the hash is appended to the packet.

-- 
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/678#pullrequestreview-47938739

---

From: ekr <[email-masked]>
Date: Wed, 05 Jul 2017 03:21:15 +0000 (UTC)

ekr commented on this pull request.

Kazuho's comment matches my understanding

-- 
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/678#discussion_r125548464

---

From: MikkelFJ <[email-masked]>
Date: Wed, 05 Jul 2017 05:21:27 +0000 (UTC)

mikkelfj commented on this pull request.

I agree, it is at the end, and it is FNV-1a, not FNV. Also, it can be seen as outside the format similar to how encryption tags are added for protected packets.

-- 
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/678#discussion_r125557027

---

From: Alexis La Goutte <[email-masked]>
Date: Wed, 05 Jul 2017 01:09:10 -0700

alagoutte commented on this pull request.

@kazuho Move to section 6 ? ( Frames and Frame Types)

@mikkelfj may be rename hash or integrity check (like section 5.4)

-- 
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/678#discussion_r125578914

---

From: MikkelFJ <[email-masked]>
Date: Wed, 05 Jul 2017 03:24:43 -0700

@alagoutte as I understand it, the QUIC TLS document is currently scheduled for redesign where some details such as FNV should move to transport - @martinthomson ?

-- 
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/678#discussion_r125607066

---

From: ianswett <[email-masked]>
Date: Wed, 05 Jul 2017 09:12:32 -0700

ianswett commented on this pull request.

In GQUIC, the hash is right before the payload, which was a bad idea, and may be why this PR proposed that.

We should definitely put it at the end for IETF QUIC.

-- 
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/678#discussion_r125689519

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 06 Jul 2017 06:33:17 +0000 (UTC)

Maybe my comment has caused confusion here. My apologies.

The PR proposes to add a field named "FNV Hash" _after_ Payload. So the location at where the hash should be stored might not be at stake.

OTOH, simply adding a "FNV Hash" field contradicts with the expectation that the hash value will be part of the Payload. For example, 5.4 states:
> The payload of cleartext packets also includes an integrity check, which is described in [QUIC-TLS].

The case is also the same of the AEAD tag.

So while I am not against creating a field to mark where the hash and tag needs to be stored (though I am not sure if it is necessary to do so), I think that such a PR should also require adjustments to the text of the drafts as well.

-- 
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/678#discussion_r125820618

---

From: Igor Lubashev <[email-masked]>
Date: Wed, 12 Jul 2017 23:46:25 +0000 (UTC)

igorlord commented on this pull request.

> @@ -410,6 +414,12 @@ Payload:
 : Octets from 17 onwards (the rest of QUIC packet) are the payload of the
   packet.
 
+FNV Hash:
+
+: If Packet Type is unprotected ( Version Negociation, Client Initial,
+  Client/Server Cleartext, Public Reset ) a 64-bit FNV-1a hash is present.

You should also update the language on the STREAM frame description of D flag.

Presently, it says: "When set to 0, this field indicates that the Stream Data field extends to the end of the packet.".  

You may want to clarify that in case of a plaintext packet, D=0 indicates that Stream Data field extends up to the FVN-1a hash field.

-- 
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/678#pullrequestreview-49649215

---

From: Mike Bishop <[email-masked]>
Date: Mon, 24 Jul 2017 10:31:08 -0700

Given the WG decision to replace the FNV-1a hash with AES-GCM encryption (#693), I believe this change is no longer applicable.  If I've missed something, feel free to reopen.

-- 
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/678#issuecomment-317495968

---

From: Mike Bishop <[email-masked]>
Date: Mon, 24 Jul 2017 17:31:09 +0000 (UTC)

Closed #678.

-- 
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/678#event-1176562040