Define transport parameters #122

martinthomson · 2017-01-04T10:43:45Z

This builds on @MikeBishop's work in #99 in a big way. I have stolen a lot of text from that.

Again, it's the tippy top of a bunch of changes. #113 comes before this.

Closes #99.

MikeBishop

This takes a big step toward requiring the use of TLS (and therefore requiring a new full version of the QUIC spec to use anything else) rather than just defining a new version identifier to be QUIC + other-crypto-protocol and a document describing how the integration would work.

I think either path is reasonable, but I had understood us to be on the latter path, and this PR changes that. That should probably be something the WG as a whole discusses in depth.

MikeBishop · 2017-01-04T18:26:17Z

draft-ietf-quic-transport.md

+different packet format.
+
+Between different versions the following things are guaranteed to remain
+constant are:


Too many "are"s.

MikeBishop · 2017-01-04T18:29:53Z

draft-ietf-quic-transport.md

-first QUIC packet from the client to the server MUST carry handshake information
-as data on Stream ID 1.
+establishment latency.  QUIC allocates stream 1 for the cryptographic handshake,
+which uses TLS.


This seems to be explicitly rejecting the separation of a modular crypto protocol and a definition of how TLS is used as that module. We should discuss that change in the WG more generally.

Sure, the principle I'm operating on here is that this version of quic uses TLS. That doesn't preclude the use of other cryptographic handshake protocols for other versions. Given that no other alternative exists, building in the ability to change out this piece of the protocol doesn't seem worth the additional effort.

I don't believe that the goal was to have TLS be baked into this version of QUIC. There's no reason this version of QUIC cannot work with a future version of TLS and/or a different crypto mechanism. The reason that no alternative exists at the moment is because the wg is scoped to be only working on TLS, similar to the wg working only on HTTP as an application.

I'm following the principles that I thought we had agreed: this version uses TLS (and only TLS) and a new version would be needed to change that. We have a clear description of the interface between the two protocols that makes it a relatively easy process to forklift TLS. What we don't need is some abstract definition of a "crypto handshake protocol" and then a specific embodiment of that. That makes the protocol much harder to read and reason about.

My plan is to remove the entire "requirements on a crypto handshake protocol" section. It's redundant with other more concrete text. Any replacement crypto can then just provide the same services that TLS does (or not, because those services are not relevant in that context).

My thought was something like this:

RFC QQQQ: Uses a crypto protocol on Stream 1, gets keys from it, uses the keys to provide an encrypted transport protocol like so....

RFC QQQT: If your crypto protocol is TLS 1.3, here's what happens on Stream 1 and when the keys are handed to QUIC. RFC QQQQ using TLS 1.3 is defined as QUIC version T.

RFC QQQF in the far future: If your crypto protocol is Foo, here's what happens on Stream 1 and when the keys are handed to QUIC. RFC QQQQ using Foo is defined as QUIC version F.

So my understanding actually was that it should be possible to change the crypto protocol without changing the version. However, that would mean you probably need another negotiation mechanism for the crypto protocol...? I guess that doesn't make sense. If we define this version to always use TLS (it doesn't have to be 1.3. though, right? should probably also be possible to support higher versions...), then we could probably move the text on requirement for a crypto protocol in the applicability statement (which we just started to write today; expect to see a -00 draft latest by next week).

Yes, @MikeBishop already observed that since we don't have a negotiation for crypto protocol, this version necessarily uses TLS. And you are right to observe that any version of TLS will do, though you don't get all the properties you need from versions earlier than 1.3.

I wonder if we need explicit language saying that 1.2 isn't good enough, some stacks could negotiate 1.2, which could actually work.

definitely a +1 to being explicit about >= 1.3.. being overly clever for the reader is not a good thing here.

also +1 on binding a version of the protocol to TLS with text that acknowledges security versioning is done as protocol versioning.

+1 on >= 1.3, but I don't think this document should be tied to TLS.

(a bit late to the discussion, but here are some thoughts from a large p2p project)

It is pretty important to preserve the ability to swap to a different crypto handshake protocol, in case problems are found, or for some extremely sensitive applications. Specs tend to bake and solidify, so doors you close now may remain shut forever.

Be careful with crypto agility-- negotiating handshake protocols can be a big security problem. Crypto agility without the ruthless removal of older protocols (and necessary rejection of old peers) has been known to cause massive problems (yes, hence everyone here asking for TLS >= 1.3, 👍 ).

Differentiate "crypto extensibility" from "crypto agility", as agl puts it here (https://www.imperialviolet.org/2016/05/16/agility.html):

Firstly I'd like to separate extensibility from agility. A protocol is extensible if you can add features to it without having to update every implementation at the same time—which is generally impossible. Cryptographic agility depends on having extensibility, at least if you ever want to use something that wasn't designed into a protocol at the beginning.

Protocols should be extensible: the world keeps changing and no design is going to be perfect for all time. But extensibility is much harder in practice than it sounds.

I personally think QUIC should have extensibility regarding its crypto encryption protocol (and thus keep the "what is needed from a crypto protocol" section of the spec), but i also think crypto agility maybe should not be encouraged (i.e. negotiation is not necessary at all, only extensibility (a code specifying which protocol is being used may be sufficient). Negotiation/agility is not required because a meta-protocol that "negotiates which crypto protocol to use" could just as easily be the one used by those who require it/are fine to saddle on the security implications of crypto agility).

In practice, it's ok to have two peers/a client+server pair know what crypto protocol they will use to connect with each other (eg latest browsers using TLS 1.3, or even a "TLS >= 1.3 negotiation protocol", all ipfs peers using TLS 1.3 or CurveCP).

and definitely IPFS would have to violate that right away and do a custom QUIC that supports other non TLS 1.3 protocols :( ideally we shouldnt have to deviate ever.

But again, baking in TLS as the only option at this point is the sort of thing that can "protocol ossify" and be quite difficult to upgrade from. I dont think you lose much (anything?) by keeping a way to specify which protocol the connection is using, which this version says SHOULD be TLS 1.3, but which leaves the door open to implementors / users / future protocol designers to upgrade smoothly to something else, without requiring a full new QUIC version (harder to do).

also +1 on binding a version of the protocol to TLS with text that acknowledges security versioning is done as protocol versioning.

I don't think this solves the problem. It's a good idea when everyone agrees which protocol is the right one. But consider that there has been a lot of suspicious activity even in standards bodies, and we may see a lot more in the coming years. Already it is the case that many well-known cryptographers/security people prefer other crypto primitives (eg djb crypto) or different crypto handshake protocols. Diversity of choice i think will be increasingly important. This only needs extensibility though, not agility.

i don't know if this is still relevant (or whether TLS 1.3 has absorbed DTLS 1.3 as far as QUIC is concerned), but i think it may be relevant to keep the door open to use DTLS 1.3 for unreliable "channels" (stream, but it's not really a stream). I dont think most standard users of QUIC will need unreliable streams, and you may have dropped it from the spec by this point, but i know users like WebRTC and IPFS would greatly benefit from muxing both unreliable channels and reliable streams, and use TLS for reliable and DTLS (or something else) for unreliable.

Sorry, there's a lot of feedback here, much that isn't up to date with everything going on in the WG. If you'd like me to separate any of these comments into other issues, please lmk.

janaiyengar

Thanks much for the work! I agree with Mike's comment. I think the intended goal for this document was to keep it independent of TLS, though the wg was clearly chartered to only work on TLS.

janaiyengar · 2017-01-06T00:30:53Z

draft-ietf-quic-transport.md

@@ -298,6 +298,28 @@ significant bit is referred to as bit 0.  Hexadecimal notation is used for
 describing the value of fields.


+## Versions


I think this belongs in its own section, outside "Packet Types and Formats," since it applies to the entire document, and is independent of Packet Types.

janaiyengar · 2017-01-06T00:35:39Z

draft-ietf-quic-transport.md

+with an existing connection, packets without a VERSION bit MUST be discarded.
+
+While there might be similarities between different versions of this protocol,
+implementations have to assume that a version that it does not support uses a


Suggest removing the clause "While there might be similarities between different versions of this protocol"

janaiyengar · 2017-01-06T00:38:06Z

draft-ietf-quic-transport.md

+
+* the location and size of the Flags field,
+
+* the location and value of the VERSION bit in the Flags field,


Not sure what it means to say that "the value of the VERSION bit" is "guaranteed to remain constant". I think location is adequate.

janaiyengar · 2017-01-06T00:40:32Z

draft-ietf-quic-transport.md

+
+* the location and value of the VERSION bit in the Flags field,
+
+* the existence of a 64-bit field following the Flags field, and


Connection ID is an optional field, so its existence is not guaranteed. Suggestion: "the location and size of the Connection ID field" seems adequate.

It's only optional AFTER version negotiation is possible.

janaiyengar · 2017-01-06T00:51:21Z

draft-ietf-quic-transport.md

+A QUIC connection begins with a client sending a handshake packet. Until the
+cryptographic handshake ({{handshake}}) produces 1-RTT keys, packets sent by a
+client MUST include the version in the packet header.  This allows the server to
+identify early packets and to enable version negotiation.


By "early packets", do you mean 0-RTT data? This would be a significant change. This bit is not intended to be conflated with 0-RTT data. The version bit is only set until the client hears back from the server with the version bit off, so generally, the first RTT, but it does not directly have anything to do with 1-RTT or 0-RTT keys. (In Google QUIC without Stateless Rejects, the first RTT of a cold connect would have no server config, and the second RTT would carry a fully formed CHLO with 0-RTT data, encrypted using 1-RTT keys. Version negotiation however would have completed during the first RTT.)

We had a long discussion about that in relation to the TLS draft. Most relevant is this comment from Dragana: #43 (comment) If I don't make this change, it isn't possible for the packet protection to work out which keys to use on a given packet.

Yes, this is different from Google QUIC, but that's not a downside. Google QUIC relied very heavily on trial decryption. This change would mean no need for trial decryption at all.

janaiyengar · 2017-01-06T01:23:53Z

draft-ietf-quic-transport.md

-  TCID parameter is received, an endpoint MAY choose to not send the connection
-  ID on subsequent packets.
+: The truncated connection identifier parameter indicates that packets sent to
+  the peer can omit the connection ID.  This can be used by an endpoint where


"sent to the peer" --> "sent to the endpoint" (since line 781 says "An endpoint MAY use the following transport parameters"

janaiyengar · 2017-01-06T01:24:23Z

draft-ietf-quic-transport.md

-  TCID parameter is received, an endpoint MAY choose to not send the connection
-  ID on subsequent packets.
+: The truncated connection identifier parameter indicates that packets sent to
+  the peer can omit the connection ID.  This can be used by an endpoint where


"can omit" --> "MAY omit"

janaiyengar · 2017-01-06T01:31:12Z

draft-ietf-quic-transport.md

+
+A client cannot use or rely upon any other transport parameter that was enabled
+in a previous connection.  This includes those defined in this document.  A
+client MUST assume that the transport parameter was absent, unless the


Are you saying that it's ok for the client to cache and use flow control limits and stream limits from a previous connection, but not other params? That seems reasonable to me, but I'd make that more explicit.

Yeah, I need to rewrite this a little. Actually, there is an alternative we should discuss. I will raise a separate issue on this point.

janaiyengar · 2017-01-06T01:34:00Z

draft-ietf-quic-transport.md

+
+An endpoint MUST ignore transport parameters that it does not support.
+
+The definition of new transport parameters can be used to negotiate new protocol


Nit: "The definition of new transport parameters can be used..." --> "New transport parameters can be used..."

janaiyengar · 2017-01-06T01:36:41Z

draft-ietf-quic-transport.md

+behavior.  Endpoints that support features that deviate from this specification
+MUST assume that a peer will operate as described in this document unless it
+provides an explicit signal of support.  A new transport parameter could be used
+to provide this signal.


Suggestion: I don't think the last two sentences add any value. I would remove them and combine this para with the previous one: "New transport parameters can be used to negotiate new protocol behavior. An endpoint MUST ignore transport parameters that it does not support."

janaiyengar · 2017-01-06T05:11:32Z

draft-ietf-quic-transport.md

@@ -633,7 +632,7 @@ on the two endpoints agreeing on a version.
 A QUIC connection begins with a client sending a handshake packet. Until the
 cryptographic handshake ({{handshake}}) produces 1-RTT keys, packets sent by a
 client MUST include the version in the packet header.  This allows the server to
-identify early packets and to enable version negotiation.
+identify 0-RTT packets and to enable version negotiation.


I would simply remove the last sentence. I don't think that the server should be using the version bit to infer anything about what packets are being sent.

I guess that we still disagree about the necessity for marking 0-RTT packets then. I will remove the sentence in the interests of not blocking the entire PR. Expect this to come up again.

janaiyengar · 2017-01-06T05:12:54Z

draft-ietf-quic-transport.md

+the client for 0-RTT connections.
+
+The mandatory transport parameters for stream and connection flow control and
+concurrent stream limits MUST be remembered by the client.  The idle timeout value


I think MAY is adequate here -- I don't see the need for requiring a client to remember these parameters (which cannot be enforced anyways.)

The problem with "MAY" is that we don't establish defaults. If the client decides not to remember it has no idea how much flow control window it can use or how many streams it can open. So we either need to define defaults (maybe 0-RTT defaults) or we do this as a MUST.

janaiyengar · 2017-01-06T05:14:29Z

draft-ietf-quic-transport.md

+
+Transport parameter values could change as a result of completing the handshake.
+The client MUST respect the new values when the handshake completes.  This
+introduces some potential problems:

 * A client might exceed a newly declared connection or stream flow control limit
  with 0-RTT data.  If this occurs, the client ceases transmission as though the


perhaps "the client MUST cease"

janaiyengar

There's plenty of useful stuff here that can go into an uncontroversial PR on Transport Params, which I've tried to mark out. I've suggested pieces to separate into PRs for discussion.

janaiyengar · 2017-01-12T06:11:03Z

draft-ietf-quic-tls.md

-selected supports that protocol.
+TLS uses Application Layer Protocol Negotiation (ALPN) {{!RFC7301}} to select an
+application protocol.  The application-layer protocol MAY restrict the QUIC
+versions that it can operate over.  Servers MUST select an application protocol


I would leave out what the application protocol MAY do from this doc

I actually think this is appropriate, because it's describing how app-protocol selection works. TLS already enables you to advertise lists of things you support, not all of which are rational combinations. This is describing how TLS should handle ALPN in QUIC.

However, if you wanted to move part of this text to the -transport document and describe app-protocol selection independent of the crypto protocol in use, I wouldn't object.

Having considered this, I think you're right. This makes sense in the context of describing how ALPN ought to work for QUIC. SGTM.

janaiyengar · 2017-01-12T06:11:34Z

draft-ietf-quic-tls.md

+If the server cannot select a compatible combination of application protocol and
+QUIC version, it MUST abort the connection. A client MUST abort a connection if
+the server picks an incompatible combination of QUIC version and ALPN
+identifier.


I would leave this para out, since it's not for TLS to do. I think this belongs in the HTTP mapping doc.

As before. It is for TLS to do, since the server will run into this while trying to process ALPN, but you're correct that it's going to be an issue for any crypto protocol.

Yes, you're right. I hadn't fully processed ALPN and the new world order, but I think it makes sense when considering how TLS would use ALPN with QUIC.

janaiyengar · 2017-01-12T06:34:18Z

draft-ietf-quic-transport.md

@@ -629,10 +628,9 @@ QUIC's connection establishment begins with version negotiation, since all
 communication between the endpoints, including packet and frame formats, relies
 on the two endpoints agreeing on a version.

-A QUIC connection begins with a client sending a handshake packet.  Until
-packets are protected by 1-RTT keys (see {{handshake}}), packets sent by a


This threw me off, since I thought we had agreed that this text change was to be in a separate PR that would get discussed, since it changes text from simple version negotiation to combining it with crypto keys. Especially since we have no agreement on making TLS part of the transport doc, I think this text ought to be reverted (I've posted on the commit of this PR as well.)

janaiyengar · 2017-01-12T06:36:25Z

draft-ietf-quic-transport.md

+      TransportParameter parameters<30..2^16-1>;
+   } TransportParameters;
+~~~
+{: #figure-transport-parameters title="Definition of TransportParameters"}


I would separate this description of transport params out into a non-controversial separate PR. (starting from line 697)

Why would it be non-controversial - this is the entire point of this PR?

Defining a structure to hold transport params, describing the various params, what to do at 0-RTT, are all basically non-controversial. This is mostly what is described from here and below, and I think that can be separated.

janaiyengar · 2017-01-12T06:38:39Z

draft-ietf-quic-transport.md

+attacker can be detected.
+
+The client includes two fields in the transport parameters that it includes in
+the TLS ClientHello:


remove mention of TLS ClientHello

janaiyengar · 2017-01-12T06:40:01Z

draft-ietf-quic-transport.md

+{{version-negotiation}}) that is performed prior to the cryptographic handshake.
+
+Inclusion of these values in an extension provides integrity protection for
+these values.  As a result, modification of version negotiation packets by an


nit: "Inclusion of these values in an extension" --> "The handshake protocol"

janaiyengar · 2017-01-12T06:44:43Z

draft-ietf-quic-transport.md

+validate the initial_version value.
+
+A server that does not maintain state for early packets uses a different
+process. If the initial and negotiated versions are the same, a stateless server


Why are we discussing stateless servers? Version negotiation is part of the QUIC connection, and it is not expected that the server would shoot a version negotiation packet and just drop the connection. At any rate, this is again a function change from what is, and I think belongs in a separate PR.

janaiyengar · 2017-01-12T06:46:54Z

draft-ietf-quic-transport.md

 ### Crypto Handshake Protocol Features

+(TODO: Remove or migrate this section.)


Remove this from this PR.

janaiyengar · 2017-01-12T06:48:19Z

draft-ietf-quic-transport.md


-#### Encoding
+Definitions for each of the defined transport parameters are included in


I would remove the para above from the transport params PR, and modulo comments below, add the rest of the text into a non-controversial transport params PR.

martinthomson · 2017-02-07T09:04:40Z

@janaiyengar, I think that this is basically there. I had to pick some defaults for transport parameters in order to deal with the conclusion to #126. I think that's a terrible decision, but I'm clearly in the rough. I picked very conservative defaults (gmail-level concurrency, yay) so that constrained servers don't get too badly screwed by this decision.

I moved the transport-requirements up and scrubbed it a little. As a result this also fixes #9.

This is now getting too big - at the point that this starts fixing multiple issues, I think that we have to either commit to fixing niggles in follow-up changes, or abandon all the work. I'd rather go with the former if at all possible.

This changes the definition for transport parameters from "MUST remember" as I originally wrote up, to the frankly insane consensus position from the Tokyo interim. Closes #126

janaiyengar

One minor change requested, but LGTM.

martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -tls -transport labels Jan 4, 2017

martinthomson requested review from MikeBishop and janaiyengar January 4, 2017 10:43

martinthomson mentioned this pull request Jan 4, 2017

Source address token #120

Merged

MikeBishop reviewed Jan 4, 2017

View reviewed changes

martinthomson mentioned this pull request Jan 6, 2017

QUIC-TLS Extension and version negotiation #99

Closed

janaiyengar reviewed Jan 6, 2017

View reviewed changes

martinthomson mentioned this pull request Jan 6, 2017

Separate transport parameters for 0-RTT #126

Closed

martinthomson force-pushed the transport_parameters_mt branch from ee4739d to e70735c Compare January 6, 2017 03:32

janaiyengar reviewed Jan 6, 2017

View reviewed changes

martinthomson force-pushed the transport_parameters_mt branch 3 times, most recently from 2b6271d to f457732 Compare January 10, 2017 23:54

janaiyengar requested changes Jan 12, 2017

View reviewed changes

This was referenced Jan 12, 2017

Decouple QUIC version and ALPN #12

Closed

Transport parameter advertisements #49

Closed

janaiyengar added the needs-discussion An issue that needs more discussion before we can resolve it. label Jan 14, 2017

MikeBishop mentioned this pull request Jan 14, 2017

Version Negotiation + TLS #97

Closed

This was referenced Jan 15, 2017

Version downgrade vulnerability #121

Closed

Remove SETTINGS[_ACK] #181

Closed

This was referenced Jan 24, 2017

Mismatch between version negotiation in TLS layer and in QUIC layer #206

Closed

QUIC extensibility #210

Closed

Kill SCUP #225

Closed

martinthomson removed the needs-discussion An issue that needs more discussion before we can resolve it. label Feb 7, 2017

martinthomson force-pushed the transport_parameters_mt branch from dbea96b to ca264eb Compare February 7, 2017 00:11

martinthomson mentioned this pull request Feb 7, 2017

COPTs as empty transport parameters #116

Closed

martinthomson force-pushed the transport_parameters_mt branch from 54b749a to b8a310f Compare February 7, 2017 02:01

martinthomson added 21 commits February 20, 2017 14:52

Add a stub iana section, fix dead references

54b4a61

Remove COPT, add rule about unknown extensions

58b44bf

Reference WINDOW_UPDATE properly

69682c8

Remove extra sentence on ignoring unknown parameters

c6f4388

editorial

4e553f5

Correct reference

c003df8

Move crypto requirements to head of section

ff697d3

Wrap note

0a927e4

Remove QUIC-Crypto reference

b251df5

Transport parameter are available when the handshake completes

64e9772

Simplify the section on new parameters

081ef5e

Add IANA considerations

e5d5bf4

Add transport parameters to the handshake interface in TLS

665a59a

Fix formatting

895a68e

Proofread version negotiation and correct errors

06a8935

Use hex, add initial registry contents

10be7da

Transport parameters during 0-RTT are either remembered or defaults

e671b0d

This changes the definition for transport parameters from "MUST remember" as I originally wrote up, to the frankly insane consensus position from the Tokyo interim. Closes #126

Fix long line

bb4f04d

Rename transport parameters, closes #300

8922744

Cryptographic handshake, blah, blah

8e5a538

Jana's review comments

0fada3c

martinthomson force-pushed the transport_parameters_mt branch from e6c17bc to 0fada3c Compare February 20, 2017 03:55

Correct line length

686aad7

janaiyengar approved these changes Feb 21, 2017

View reviewed changes

Fix case

1ffe4e9

martinthomson merged commit 8ec5114 into master Feb 21, 2017

martinthomson deleted the transport_parameters_mt branch February 21, 2017 23:17

martinthomson mentioned this pull request Feb 24, 2017

Remove references to QUIC Crypto #9

Closed

martinthomson mentioned this pull request Apr 27, 2017

Version negotiation gaps #89

Closed

		@@ -298,6 +298,28 @@ significant bit is referred to as bit 0. Hexadecimal notation is used for
		describing the value of fields.


		## Versions


		* the location and size of the Flags field,

		* the location and value of the VERSION bit in the Flags field,


		* the location and value of the VERSION bit in the Flags field,

		* the existence of a 64-bit field following the Flags field, and


		An endpoint MUST ignore transport parameters that it does not support.

		The definition of new transport parameters can be used to negotiate new protocol

		### Crypto Handshake Protocol Features

		(TODO: Remove or migrate this section.)


		#### Encoding
		Definitions for each of the defined transport parameters are included in

Define transport parameters #122

Define transport parameters #122

Conversation

martinthomson commented Jan 4, 2017

MikeBishop 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

jbenet Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

janaiyengar 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson commented Feb 7, 2017

janaiyengar left a comment

Choose a reason for hiding this comment

jbenet Jan 19, 2017 •

edited

Loading