Upgrade openssl-1.1.0e for Node-v8 (DO NOT LAND THIS) #11828

shigeki · 2017-03-13T15:10:50Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

crypto, tls

This PR is a big patch and please don't review this immediately.

It is time to make a decision if we upgrade OpenSSL-1.1.0 in Node-v8. It breaks API/ABI compatibilities with 1.0.2 so that it needs semver-major in this upgrade.

I submit this patch for intending to show and demonstrate that upgrading to 1.1.0 can be done in all platforms. CI results has already shown in https://ci.nodejs.org/job/node-test-commit/8424/ and they are green except FreeBSD failure due to Jenkins error.

Before we make the decision, I think that the following three major issues should be discussed.

Support end difference between Node LTS and OpenSSL

Node-v8 LTS Maintenance End will be in 2020-04. According to the current support policy of OpenSSL of https://www.openssl.org/policies/releasestrat.html , it says that
```
Version 1.0.2 will be supported until 2019-12-31 (LTS).
Version 1.1.0 will be supported until 2018-08-31.
```
If we continue to use openssl-1.0.2, Node-v8 LTS support should be end on 2019-12-31 about 3 months earlier than our plan as we did in v0.12.
1.1.0 will be support end in the next year. A new OpenSSL-1.1.1 is now under development and expected to be released near future with new feature of TLS1.3. It is API/ABI compatible with 1.1.0 so we can deploy it with semver-minor. A new OpenSSL release will be supported at least two years. If LTS is specified to it, it will be five years support.
If we deployed 1.1.0 in Node-v8, we need to make semver-minor upgrade of OpenSSL until to meet its support end of 2020-04.
FIPS Support

The current FIPS support is provided only for 1.0.2. There was an announcement of a sponsor to support FIPS compliance with OpenSSL 1.1 in
https://www.openssl.org/blog/blog/2016/07/20/fips/ but it is not yet to known when new FIPS support is released.

If FIPS support is required in Node, we have to continue to use 1.0.2 until new FIPS support is released.
New Build Requirement (nasm) on Windows

In order to build OpenSSL in Windows with asm support, NASM is officially required. https://github.com/openssl/openssl/blob/OpenSSL_1_1_0-stable/NOTES.WIN#L20-L23
```
- Netwide Assembler, a.k.a. NASM, available from http://www.nasm.us,
  is required if you intend to utilize assembler modules. Note that NASM
  is the only supported assembler. The Microsoft provided assembler is NOT
  supported.
```
It was noted even in 1.0.2 and we have been ignoring this requirement. 1.1.0 begins to checks its requirement during build. We can avoid it with our own risk but I don't think it is a good way to go. As it is inevitable, we should decide when and what version of Node the new requirement starts.

This PR adds an experimental support of OpenSSL-1.1.0e with a new flag --use-openssl11 in configure and contines to use 1.0.2 by default. I think that it leads API complexities when we add a new feature specific to 1.1.0 and agree that it is not a good solution.

My preference is that we stay on 1.0.2 in Node-v8 and deploy 1.1.0 or 1.1.1 someday later in v9 or v10 with replacing 1.0.2. If FIPS support is not released at that time. we separate a new branch with 1.0.2 only to have FIPS support.

CC @nodejs/crypto , @nodejs/lts

mhdawson · 2017-03-13T15:26:44Z

I agree with @shigeki's recommendation to stick with 1.0.2 in Node version 8. I'd prefer we have more time for the update to settle in master before it goes into a release and we are not that far from cutting version 8.

indutny · 2017-03-13T17:23:45Z

I agree on this too. It seems that 1.1.0 is getting more security updates than 1.0.2 at the moment, I'd say that it should make Node 8 safer too.

This extracts and adds all source files of openssl-1.1.0e.tar.gz into deps/openssl/openssl110.

This is a floating patch against openssl-1.1.0 in order to have asm files of *.S which are before pre-processed. Node.js needs them in its repository because pre-processing is to be made in the build of Node binaries on each architecture platforms. This patch should be submitted to the upstream.

This commit has a new binding scheme in builing OpenSSL-1.1.0 library with Node. OpenSSL-1.1.0 uses a new build system with perl for various supported platforms. See `Configurations/README` and `Configurations/README.design` in the OpenSSL source for details. In order to build OpenSSL library without perl in the build of Node for various supported platforms, platform dependent files (e.g. asm and header files ) are pre-generated and stored into the `config/arch` directory. - Makefile and generate.pl Makefile has supported platform list and generates and copies platform dependent files (e.g. asm files) into arch directory with generate.pl. Platform dependent gypi files also created obtaining build information from `configdata.pm` that is generated with `Configure` in the OpenSSL build system. For Windows, `Configure` generates makefile that is only available to nmake command. `Makefile_VC-WIN32` and `Makefile_VC-WIN64A` are made created by hand for the use of GNU make. If make rules or targets are changed in the version up of OpenSSL, they should be also updated. Theses are usually used in upgrading openssl-1.1.0. - gyp and gypi files openssl.gyp has two targets of openssl and openssl-cli referred from node.gyp. They includes asm and no_asm gypi files with arch dependent gypi according to its build options and platforms . The gyp data which is common with asm and no_asm are stored in openssl_common.gypi. - header files bn_conf.h, dso_conf.h and opensslconf.h are platform dependent in the OpenSSL sources. They are replaced with *.h.tmpl files to include the file in the `../../../config/` and referred to each arch files that depends on asm and no-asm option.

This updates all platform dependent files for OpenSSL-1-1.0e with using `make` command in the config directory.

This adds a new option to build Node with OpenSSL-1.1.0. The configure also check build evnviroments if asm features are supported.

PR-URL: nodejs#8491

Fix lint errors in nodejs#8491 and add more fixes of build failures due to API changes in OpenSSL-1.1.0.

This add a new rules to install header files of OpenSSL-1.1.0 in case of use-openssl110 option. This also includes the fix of mkssldef.py due to the change of a format of ordinal files.

This fixes that tests can run and pass on both built with OpenSSL-1.0.2 and 1.1.0. Only the test of `test/parallel/test-tls-econnreset.js` is skipped because there are no way to leads TLS handshake failure and send ECONNRESET from the tls client to tls server with using OpenSSL-1.1.0.

This always enables use_openssl110 flag true. It has fallback to add no_asm option on Windows when nasm is not installed. This is a tentative fix for running on CI.

sgallagher · 2017-03-14T12:46:07Z

A few concerns I will note:

OpenSSL 1.0.2 will terminate support in Dec. 2019, but Node.js v8 will be supported until Apr. 2020. There's a four-month gap during which the Node.js security team will be responsible for maintenance of OpenSSL itself (at least within the context of Node.js usage).
As a distribution packager, I like the idea of at least having a configure flag that enables (experimental or otherwise) support for OpenSSL 1.1.0. This would also help mitigate the above bullet point, as once we reach OpenSSL 1.0.x EOL, it would still be possible to switch over to 1.1.1 or 1.1.2 (whichever is appropriate at the time).

gibfahn · 2017-03-14T13:02:13Z

OpenSSL 1.0.2 will terminate support in Dec. 2019, but Node.js v8 will be supported until Apr. 2020. There's a four-month gap during which the Node.js security team will be responsible for maintenance of OpenSSL itself (at least within the context of Node.js usage).

@sgallagher See @shigeki's comment about ending support for Node 8 when its OpenSSL support ends:

Node-v8 LTS Maintenance End will be in 2020-04. According to the current support policy of OpenSSL of https://www.openssl.org/policies/releasestrat.html , it says that

Version 1.0.2 will be supported until 2019-12-31 (LTS).
Version 1.1.0 will be supported until 2018-08-31.
If we continue to use openssl-1.0.2, Node-v8 LTS support should be end on 2019-12-31 about 3 months earlier than our plan as we did in v0.12.

bricss · 2017-03-14T13:20:21Z

As Node.js goes to semver-major everybody should expect breaking changes, deprecations would not work anymore and etc. If not, they should stick to previous LTS versions until they update their projects.
Nobody will jump up to Node.js 8 on the next day after release, in production. I believe, it shall be updated to OpenSSL 1.1.0, then it will be easier to support and switch over to future 1.1.x versions.

shigeki · 2017-03-14T14:54:43Z

@gibfahn I would like ask you to use Node in your package with our builtin openssl library not using a shared library to meet our support policy.

For example, It is shown in CVE-2016-7056 of outdated openssl-1.0.1 that the distributor will not fix it. See it in https://access.redhat.com/security/cve/cve-2016-7056 and https://bugzilla.redhat.com/show_bug.cgi?id=1412120 . Even if it is a moderate severity, we do not accept its risk. That is why we follow the upstream support policy.

shigeki · 2017-03-14T14:57:52Z

@sgallagher I replied to your comment. Sorry for @gibfahn.

sgallagher · 2017-03-14T15:07:37Z

@shigeki
We (Fedora) cannot ship the version of OpenSSL bundled with Node.js because it doesn't meet U.S. legal requirements. (Specifically, it includes a variety of encryption protocols that are encumbered with U.S. patents forbidding Fedora or Red Hat Enterpirse Linux from shipping them without paying royalties, which Fedora is incapable of doing). As such, in Fedora we have to strip out these patented components from the tarball. (We cannot even ship them in the source RPM distribution).

That leaves us with two options:

Link against the shared library provided by Fedora which has been properly vetted by Fedora's security and Legal teams
Attempt to replicate their work and strip out only the offending pieces of the OpenSSL code bundled into Node.js

The latter is unrealistic in a volunteer project like Fedora.

I should also note that Fedora follows OpenSSL upstream's recommendations. We generally carry the latest upstream releases within 24 hours. Red Hat Enterprise Linux makes its own decisions about which patches to carry, weighing the costs of maintenance, testing, certification and customer inconvenience. Since I (and my fellow co-maintainers) do not have the resources to maintain our own embedded version of OpenSSL, our only option is to trust that Red Hat makes reasonable decisions on which patches to backport on those platforms.

sgallagher · 2017-03-14T15:11:11Z

@shigeki BTW, for Red Hat, an issue of "moderate" severity usually translates to "It's not worth the hassle to customers to push out a fix solely for this, but it will usually be included the next time an update for a more severe issue is released."

shigeki · 2017-03-14T15:40:35Z

@sgallagher Thanks for clarification. I understand your situation. Crypto libraries have a long history against patents and government laws to meet local legal restrictions. We currently make a high priority on the upstream's support policy.

sam-github · 2017-04-24T21:18:49Z

@nodejs/lts would like this to get discussed again so we commit if possible to the major that will contain the updated openssl, so we don't get stuck on an unsupported Openssl in LTS, see meeting notes for nodejs/Release#201

shigeki · 2017-04-25T11:13:19Z

I think that the most issue is that we have to support both 1.0.2 and 1.1.x as long as fips is needed.
It gets much more complicated for us to keep two versions of openssl that have many API/ABI incompatibilities.

bricss · 2017-04-25T16:57:32Z

Why not just stick with OpenSSL 1.1.x ❓

sam-github · 2017-04-26T20:15:16Z

@shigeki

isn't 1.1.x supposed to get FIPS certification, eventually?
are you proposing we would have to compile and link against either 1.0 or 11, depending on ./configure --openssl-fips=... being specified? That would be awful. Or are you talking about how node 8.x and below would be on 1.0, and (maybe) 9.x and above would be on 1.1?

shigeki · 2017-04-27T10:22:32Z

@sam-github

isn't 1.1.x supposed to get FIPS certification, eventually?

I wrote it in the comment above as

FIPS Support

The current FIPS support is provided only for 1.0.2. There was an announcement of a sponsor to
support FIPS compliance with OpenSSL 1.1 in
https://www.openssl.org/blog/blog/2016/07/20/fips/ but it is not yet to known when new FIPS support is released.

are you proposing we would have to compile and link against either 1.0 or 11, depending on ./configure --openssl-fips=... being specified? That would be awful. Or are you talking about how node 8.x and below would be on 1.0, and (maybe) 9.x and above would be on 1.1?

In this PR, both openssl-1.0.2 and 1.1.0 are included and --openssl-fips flag in configure use 1.0.2 as

+    if options.openssl_fips:
+      warn('openssl-1.1.0 does not support FIPS. Use 1.0.2 instead.')
+      variables['use_openssl110'] = 'false'

Another idea I wrote in the above for the target of Node-v9 or v10 is

If FIPS support is not released at that time. we separate a new branch with 1.0.2 only to have FIPS support.

This also can be workable for Node-v8.

mhdawson · 2017-05-09T20:56:21Z

I'm wondering why this is on the agenda. I think we agreed to stick with 1.0.2 for Node version 8 and then upgrade in Node version 9. Is this to get consensus on upgrading in Node version 9 or something else ?

mhdawson · 2017-05-11T22:58:23Z

This was discussed in the last CTC meeting but it was unclear why it was on the agenda.

@shigeki, @sam-github I took the action to get us to clarify the recommendation so that we can take that back to the CTC.

I'd suggest that we upgrade to openssl v1.1 for Node version 9. This means that if you need FIPs support before the cert for v1.1 completes you would have to stick with Node version 8.

My rational would be:

Since we had to shorten the lifespan of Node version 8 due to the EOL of 1.02, there can be no new LTS Node version that reasonably supports 1.02 anyway
If you want FIPs support I think there is a high likelihood that you also want an LTS release.
Upgrading in 9 would give us more runway for testing out shaking out the issues before the next LTS
Its unlikely that the cert for 1.1 will complete before Node version 10 which means that its unlikely that we'll end up without an LTS version that includes 1.1 at the time 1.1 can be used for FIPs.

mhdawson · 2017-05-16T21:23:21Z

@shigeki sorry to ping you again, but it would be great if we had your feedback in time for the CTC meeting tomorrow.

shigeki · 2017-05-17T01:04:23Z

Sorry, I missed this. I think it does not need more discussion to stick OpenSSL-1.0.2 in Node-v8.
OpenSSL-1.1.1 is coming soon after TLS1.3 is standardized as https://www.openssl.org/blog/blog/2017/05/04/tlsv1.3/ and it would be a candidate for adopt to Node-v9 or v10 according to its release date.

I will remove the ctc-agenda tag and close this. If anyone has any new issues to discuss on upgrading openssl in Node-v8. Please open a new issue.

gibfahn · 2017-06-03T14:12:27Z

@shigeki now that 8.0.0 is out, do you plan to reopen this PR, or raise a new one? It probably makes sense to get OpenSSL 1.1 into master well before Node 9.

shigeki · 2017-06-06T11:05:06Z

@gibfahn I'm planning to open a new PR after releasing OpenSSL-1.1.1. It is expected be released soon after TLS1.3 is finalized.

danbev · 2017-06-09T13:24:49Z

I think that it leads API complexities when we add a new feature specific to 1.1.0 and agree that it is not a good solution.

@shigeki I been thinking about this and have a initial suggestion for further work and I'd be interested in hearing yours and others thought on it. I opened #13575 for it.

davidben · 2017-08-14T22:15:52Z

src/node_crypto.cc

+  return 1;
+}
+
+static const char *SSL_SESSION_get0_hostname(const SSL_SESSION *s) {


I believe this one is no longer needed.

davidben · 2017-08-14T22:15:58Z

src/node_crypto.cc

+    *tick = s->tlsext_tick;
+}
+
+static int SSL_SESSION_has_ticket(const SSL_SESSION *s) {


davidben · 2017-08-14T22:17:19Z

src/node_crypto.cc

+
+#define SSL_get_tlsext_status_type(ssl) (ssl->tlsext_status_type)
+
+// It doesn't do exactly the same, but works.


It does not do the same thing and leaks memory. HMAC_CTX_cleanup followed by HMAC_CTX_init would probably work. But, see comment later on...

davidben · 2017-08-14T22:19:39Z

src/node_crypto.h

@@ -496,7 +491,11 @@ class Hmac : public BaseObject {
  ~Hmac() override {
    if (!initialised_)
      return;
-    HMAC_CTX_cleanup(&ctx_);
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+    free(ctx_);


This leaks memory. It does not call HMAC_CTX_cleanup.

davidben · 2017-08-14T22:21:28Z

src/node_crypto.h

@@ -515,10 +514,15 @@ class Hmac : public BaseObject {
      : BaseObject(env, wrap),
        initialised_(false) {
    MakeWeak<Hmac>(this);
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+    ctx_ = reinterpret_cast<HMAC_CTX *>(malloc(sizeof(HMAC_CTX)));


This will break 1.0.2. It needs to call HMAC_CTX_init(ctx_) to zero-initialize everything.

davidben · 2017-08-14T22:23:21Z

src/node_crypto.h

  }

  ~SignBase() override {
    if (!initialised_)
      return;
-    EVP_MD_CTX_cleanup(&mdctx_);
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+    free(mdctx_);


davidben · 2017-08-14T22:29:17Z

src/node_crypto.cc

@@ -3708,11 +3822,11 @@ void Hmac::HmacInit(const char* hash_type, const char* key, int key_len) {
  if (md == nullptr) {
    return env()->ThrowError("Unknown message digest");
  }
-  HMAC_CTX_init(&ctx_);
+  HMAC_CTX_reset(ctx_);


This one is pretty much redundant with the one at line 3985. The state here is really confusing. Since you're paying for a heap-allocated EVP_MD_CTX anyway, simpler would be to merge the initialised_ bit into whether ctx_ is null. That is:

HmacInit has a CHECK_EQ(nullptr, ctx_). It then sets ctx_ = HMAC_CTX_new() before HMAC_Init_ex. (TBH, OpenSSL didn't do a great job of cleaning up the API while they were opaquifying. HMAC_CTX_new and HMAC_Init_ex should have been the same operation.

All the if (!initialised_) checks before if (!ctx_)

In HmacDigest, rather than reset and clearing initialised_, do HMAC_CTX_free(ctx_); and ctx_ = nullptr;.

davidben · 2017-08-14T22:30:18Z

src/node_crypto.cc

@@ -3849,8 +3963,8 @@ bool Hash::HashInit(const char* hash_type) {
  const EVP_MD* md = EVP_get_digestbyname(hash_type);
  if (md == nullptr)
    return false;
-  EVP_MD_CTX_init(&mdctx_);
-  if (EVP_DigestInit_ex(&mdctx_, md, nullptr) <= 0) {
+  EVP_MD_CTX_init(mdctx_);


davidben · 2017-08-14T22:30:45Z

src/node_crypto.cc

@@ -4001,8 +4115,8 @@ SignBase::Error Sign::SignInit(const char* sign_type) {
  if (md == nullptr)
    return kSignUnknownDigest;

-  EVP_MD_CTX_init(&mdctx_);
-  if (!EVP_SignInit_ex(&mdctx_, md, nullptr))
+  EVP_MD_CTX_init(mdctx_);


davidben · 2017-08-14T22:30:51Z

src/node_crypto.cc

@@ -4207,8 +4321,8 @@ SignBase::Error Verify::VerifyInit(const char* verify_type) {
  if (md == nullptr)
    return kSignUnknownDigest;

-  EVP_MD_CTX_init(&mdctx_);
-  if (!EVP_VerifyInit_ex(&mdctx_, md, nullptr))
+  EVP_MD_CTX_init(mdctx_);


davidben · 2017-08-14T22:34:17Z

(I ended up here from #14761. Although I missed this PR was actually closed already? I'm a little confused which PR is the plan for making Node work with 1.1.0 since the other one is out of date. Anyway, I've left some comments on this one since it seems to be the most up-to-date version of the original PR.)

shigeki · 2017-08-18T15:52:38Z

@davidben Thanks for reviewing. Yes, this was old PR and still in WIP and it was closed as we wait for FIPS support of OpenSSL-1.1.x. But it is good to have preparations for the future upgrading so I will submit a new WIP PR against 1.1.0f with including your comments after next week.

davidben · 2017-08-18T16:45:15Z

Sounds good. Sorry about the confusion. I'm happy to review it or even put it together myself if it's helpful.

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 13, 2017

shigeki added discuss Issues opened for discussions and feedbacks. openssl Issues and PRs related to the OpenSSL dependency. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 13, 2017

shigeki added this to the 8.0.0 milestone Mar 13, 2017

mscdex added the wip Issues and PRs that are still a work in progress. label Mar 13, 2017

shigeki and others added 10 commits March 14, 2017 09:43

deps: add openssl-1.1.0e sources

107d344

This extracts and adds all source files of openssl-1.1.0e.tar.gz into deps/openssl/openssl110.

deps: update all openssl110e configs

a51b158

This updates all platform dependent files for OpenSSL-1-1.0e with using `make` command in the config directory.

build: add new flag to build with openssl110

878968a

This adds a new option to build Node with OpenSSL-1.1.0. The configure also check build evnviroments if asm features are supported.

crypto,tls,test: Make it build using openssl 1.1.0

89d748f

PR-URL: nodejs#8491

tls,crypto: fix lint err, api changes openssl110

f1b3da8

Fix lint errors in nodejs#8491 and add more fixes of build failures due to API changes in OpenSSL-1.1.0.

tools: add install rules and fix for openssl110

03ebc23

This add a new rules to install header files of OpenSSL-1.1.0 in case of use-openssl110 option. This also includes the fix of mkssldef.py due to the change of a format of ordinal files.

build: temporary enabled --use_openssl110 for CI

45dc7f7

This always enables use_openssl110 flag true. It has fallback to add no_asm option on Windows when nasm is not installed. This is a tentative fix for running on CI.

shigeki force-pushed the upgrade_openssl110e branch from 642d992 to 45dc7f7 Compare March 14, 2017 00:47

richardlau mentioned this pull request Mar 14, 2017

Discussion: OpenSSL 1.1.0 planning #4270

Closed

shigeki mentioned this pull request Mar 14, 2017

Node-v8 LTS support end on 2019-12-31 to meet EOSL of openssl-1.0.2 nodejs/Release#186

Closed

richardlau mentioned this pull request Mar 24, 2017

Make it build using openssl 1.1.0 #8491

Closed

2 tasks

mscdex mentioned this pull request Mar 30, 2017

ed25519 keys not working mscdex/ssh2#352

Closed

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

sam-github added the ctc-agenda label Apr 24, 2017

Trott mentioned this pull request May 1, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-05-03 nodejs/CTC#117

Closed

Trott mentioned this pull request May 8, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-05-10 nodejs/CTC#120

Closed

shigeki closed this May 17, 2017

shigeki removed the ctc-agenda label May 17, 2017

gibfahn mentioned this pull request Jun 3, 2017

crypto.scrypt() #8417

Closed

danbev mentioned this pull request Jun 9, 2017

WIP: crypto: support multiple Crypto versions/libraries #13575

Closed

4 tasks

gibfahn mentioned this pull request Jun 15, 2017

libcrypto.so.1.0.0 error #13185

Closed

davidben mentioned this pull request Aug 14, 2017

crypto: account for OpenSSL 1.1.0 function signature change. #14761

Closed

2 tasks

davidben reviewed Aug 14, 2017

View reviewed changes


		#define SSL_get_tlsext_status_type(ssl) (ssl->tlsext_status_type)

		// It doesn't do exactly the same, but works.

Upgrade openssl-1.1.0e for Node-v8 (DO NOT LAND THIS) #11828

Upgrade openssl-1.1.0e for Node-v8 (DO NOT LAND THIS) #11828

Conversation

shigeki commented Mar 13, 2017

Checklist

Affected core subsystem(s)

mhdawson commented Mar 13, 2017

indutny commented Mar 13, 2017

sgallagher commented Mar 14, 2017

gibfahn commented Mar 14, 2017

bricss commented Mar 14, 2017 • edited Loading

shigeki commented Mar 14, 2017

shigeki commented Mar 14, 2017 • edited Loading

sgallagher commented Mar 14, 2017 • edited Loading

sgallagher commented Mar 14, 2017

shigeki commented Mar 14, 2017

sam-github commented Apr 24, 2017

shigeki commented Apr 25, 2017

bricss commented Apr 25, 2017

sam-github commented Apr 26, 2017

shigeki commented Apr 27, 2017

mhdawson commented May 9, 2017

mhdawson commented May 11, 2017 • edited Loading

mhdawson commented May 16, 2017 • edited Loading

shigeki commented May 17, 2017

gibfahn commented Jun 3, 2017

shigeki commented Jun 6, 2017

danbev commented Jun 9, 2017

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

davidben commented Aug 14, 2017

shigeki commented Aug 18, 2017

davidben commented Aug 18, 2017

bricss commented Mar 14, 2017 •

edited

Loading

shigeki commented Mar 14, 2017 •

edited

Loading

sgallagher commented Mar 14, 2017 •

edited

Loading

mhdawson commented May 11, 2017 •

edited

Loading

mhdawson commented May 16, 2017 •

edited

Loading