Keep /dev/random open for seeding #6432

paulidale · 2018-06-06T23:36:48Z

Modify the DEVRANDOM seed source so that the files are kept open persistently.

This allows operation inside a chroot environment without having the random
device present.

documentation is added or updated
tests are added or updated

Following from #5330

paulidale · 2018-06-06T23:40:17Z

At present, there is no test and it isn't clear how an automated test could be written.

A question that needs to be address is should the files be kept open for the entire list of DEVRANDOM seed sources or just one? This PR opts for the former, although they are initially opened as required rather than on startup.

t-j-h · 2018-06-06T23:40:32Z

I think there should be a mechanism to ask for these files to be closed for an application that wants to do that - i.e. not have a leaking file descriptor ...

paulidale · 2018-06-06T23:47:39Z

That's fair enough. The chroot use case would suggest that keeping them open by default and having the option to close them is the way to go. This means introducing a new API to change the keep open behaviour.

t-j-h · 2018-06-07T00:14:34Z

Or an API to provide for a cleanup separately - either way (change the open behaviour or allow for a specific cleanup call) there needs to be something new in place. And I don't think you have access to "conf" for handling that and I wouldn't suggest an environment variable to control it either.

paulidale · 2018-06-07T01:35:52Z

Added option to enable/disable the keep open semantics.
Still no automated test.

Travis failures are unrelated.

paulidale · 2018-06-07T03:24:50Z

Which is the best lock to use to guard the set to close mode call? rand_meth_lock or a new one?

kroeckx · 2018-06-07T07:08:58Z

crypto/rand/rand_unix.c

+            FILE *fp = devrandom_files[i];
+
+            if (fp == NULL) {
+                if ((fp = fopen(devrandom_paths[i], "rb")) == NULL)


Since this is the _unix file, it might be useful to use open() instead of fopen() ...

The existing code used fopen(3) and I stayed with that. I think @richsalz avoided open(2) deliberately. What do others think? I'm happy to change it.

There is also an issue with the reading code later -- if less than the full asked for amount is received, it is ignored. This is perplexing. Shouldn't whatever is read be used and accounted for? Unfortunately, the code is sufficiently complex to not be easily understandable. @mspncp any suggestion here?

Considering we don't use any of the more advanced stdio functions, care about line endings and turn off buffering, I'd say that preferring FILE pointers over file descriptors is a moot point, and if using file descriptors gives us an advantage (such as fstat mentioned elsewhere), it might as well be changed. One unnecessary layer removed...

As for the bytes read being dropped unless it's all the bytes needed, I'm frankly not sure about that. Totally spaced on that last time I looked at this. That could surely be changed, no? Question is if it's something for this PR... (at the very least, it should be made a separate commit)

Shouldn't whatever is read be used and accounted for?

I don't see any reason to discard the random data, just because it's less than the requested amount. Just grab anything you can get, add it to the pool, and drain the next source ;-).

You can use fileno() if you stay with FILE * and want to call fstat()

kroeckx · 2018-06-07T07:10:37Z

crypto/rand/rand_unix.c

+        for (i = 0; bytes_needed > 0 && i < OSSL_NELEM(devrandom_paths); i++) {
+            FILE *fp = devrandom_files[i];
+
+            if (fp == NULL) {


I would like to see a call to fstat(), and check S_ISCHR(), to see if the file is still open and still the type of file we expect.

Again, I'm happy to change this. Should we trust the configuration setting or should we check?

The reason I'm asking this is that I know that there is software that part as becoming a daemon will close all files. So they might have closed our file without us knowing about it, and it's very likely that something will reuse that file descriptor at a later point, say a network socket or log file. I want to avoid reading from an unexpected file. I guess we could cache the whole struct stat and check that some of the fields match.

That's a good enough reason for me. I'll add the check.

kroeckx · 2018-06-07T07:11:50Z

I prefer to only have 1 open file, instead of the 2 files you currently do.

levitte · 2018-06-07T07:54:06Z

Does that mean only reading the seed from one source, or closing them directly after seeding except for the last one, which is kept open for reseeding?

paulidale · 2018-06-07T08:07:40Z

There will only be more than one open file if the first (or second -- the list is three long from memory) opens but doesn't deliver enough bytes. The first entry is set to /dev/urandom which is unlikely to fail.

There is also a concern with only opening one file at a time: if the read ever fails, the file is then closed and reopened which isn't possible in a chroot(2) environment.

I'm happy to change things to only open one at a time or to open all up front but I'd like a consensus first.

paulidale · 2018-06-07T08:11:28Z

@levitte as I understand it, the current code will read from /dev/urandom (which won't fail) and the others in the list will never be opened or read from.

mspncp · 2018-06-07T08:15:55Z

@levitte as I understand it, the current code will read from /dev/urandom (which won't fail) and the others in the list will never be opened or read from.

Since this is the case anyway, there is no need to have this complicated opening logic. I would just use the first handle that can be opened successfully. Since /dev/urandom preceeds /dev/random in the list, it does not really make sense to consider a situation where the first device fails but the second one succeeds.

paulidale · 2018-06-07T23:26:31Z

I'm hesitant to only keep one FD open. If, after the chroot(2), there is a failure then there are no other possible sources.

paulidale · 2018-06-08T04:40:46Z

Updated to use FDs not FILE *.
The code opens both descriptors still.

mspncp

To be continued...

mspncp · 2018-06-08T06:15:35Z

crypto/rand/rand_lib.c

+    rand_pool_set_close_seed_files(close);
+}
+
+int RAND_get_close_seed_files(void)


As a person who likes self describing function names in imperative style (CLASSNAME_do_this(), CLASSNAME_enable_that()), I would prefer to have a more descriptive function name than "this is the setter of some internal variable called close_seed_files. Even more since

There is no need for a setter/getter pair. This feature is under control of the application and nobody else should be changing it anyway. So there is no need for the application to query the current value.

The term "seed files" is used nowhere else and is unnecessarily general; everywhere else we are talking about random device(s). Using such a general term just hides information to the programmer, which has no clue what precisely this function is doing unless he looks up the documentation or the source code.

My suggestion: Just drop the getter and rename the setter to something like

void RAND_keep_devrandom_open(int keep); void RAND_keep_random_devices_open(int keep); /* <= my favorite choice */

or the other way around

void RAND_close_devrandom_after_use(int close); void RAND_close_random_devices_after_use(int close);

edit: I forgot to commit a pendiing edit before sending off the review.

I often admin I'm rubbish with API names :) I'll change it.

Please don't use devrandom in the names. Sure, this is currently very Unix centric, but should this become implemented on other platforms as well, devrandom is as foreign a name as it gets.

This function exists solely to disable a workaround for a very unix-ish problem, namely that /dev/random might not be available after chroot(). So it is very unlikely that it will ever be used in a more general context.

Please don't use devrandom in the names.

Does this include random_devices, too, or only devrandom?

devrandom specifically. random_devices is fine in my mind.

Renamed to RAND_keep_random_devices_open and removed the getter.

mspncp · 2018-06-08T06:23:22Z

crypto/rand/rand_unix.c

+/*
+ * Verify that the FD associated with the random source is an open character
+ * device.
+ */


Just a suggestion:

/* * Verify that the file descriptor associated with the random source is * still valid and refers to a character device. The rationale for doing * this is the fact that it is not uncommon for daemons to close all open * file handles when daemonizing. So the handle might have been closed or * even reused for opening another file. */

@EmericBr, would you mind checking whether nginx closes all open handles when chrooting? Because this would mean that this pull request would not work for your issue #5330 as expected (see my previous post).

See also #6432 (comment)

Yes, that would be a problem :(

Additionally and mostly as a reminder to myself, I need to check that Linux on the S390 uses character devices for its alternative random seed device nodes. I think other Unix based operating systems do, but I'll also check what I've got available too.

The alternative I suggested was to cache the result of fstat() and then check that the st_dev, st_ino, st_mode and st_rdev fields are the same.

Clarification: this is not about an unnamed application. This is about haproxy.

Emeric is the maintainer of the SSL subsystem in haproxy, I myself am working on haproxy as well. I reproduced this first-hand in haproxy.

In the other thread I linked to the following haproxy troubleshooting session (where Sander, Emeric and myself confirmed the issue):
Haproxy 1.8 with OpenSSL 1.1.1-pre4 stops working after 1 hour

Sander runs an older kernel without getrandom functionality, Emeric cross-compiles without kernels headers (so the raw getrandom syscall doesn't work), and I had the kernel but not the libc, so it started working for me only with the raw getrandom syscall feature in -pre6.

I can see now that the nginx issue was not caused by chroot(), but by a permission problem (after nginx downgraded privileges). I'd argue that there's a possibility that keeping the FD open would have helped the nginx case as-well, depending on the initialization sequence - but let's focus on what we actually know. I apologize for the confusion my nginx comment caused.

Even more specifically, chroot(2) has to be called after rand is initialized.

Haproxy deliberately calls RAND_bytes() before chroot()ing to initialize OpenSSL's random number generator. Otherwise this would never have worked in the first place if I understand correctly.

As for the question of this being required or not or being post 1.1.1

In my opinion, if this doesn't hit 1.1.1, the entire discussion is quite useless, considering the breakage, the fact that 1.1.1 will be LTS and the post 1.1.1 release schedule ... unless you are willing to backport such a change to 1.1.1 post release?

This is about haproxy.

Cool! Thanks for clarification!

unless you are willing to backport such a change to 1.1.1 post release?

My argumentation in #6432 (comment) was to hold back the pull request as long as there is no concrete evidence of a regression yet, but of course keeping the option to backport it as bugfix to 1.1.1 if this would become necessary after the creation of the stable branch. Now since it has been clarified by you that haproxy is indeed affected by this regression, I would say it counts as bugfix and there is no need to hold back the pr.

What about adding this but with the default being not keeping the FD open?
Since haproxy is already making an effort to start the RAND system, would it be possible to modify it to support the new API to maintain the FDs open?

I guess making it not work by default in this instance isn't ideal.

dot-asm · 2018-06-11T12:52:27Z

crypto/rand/rand_unix.c

+            devrandom_files[n].mode = st.st_mode;
+            devrandom_files[n].rdev = st.st_rdev;
+        } else {
+            devrandom_files[n].fd = -1;


Formally speaking there is possibility for resource leak here. In case fd != -1 while return value from fstat(fd) is.

Good catch!

Now I see why you were saying 'Formally speaking'. Looking at the error codes listed in stat(2) there really seems to be only a hypothetical chance that fstat() might fail in this situation. :-)

ENOMEM would be possible. I'll fix.

It doesn't matter what is error condition or how impossible it is. Mere fact of checking the return value in given sequence formally makes it a possible resource leak.

dot-asm · 2018-06-11T13:02:47Z

doc/man3/RAND_add.pod

@@ -54,6 +57,11 @@ should consider using L<RAND_load_file(3)> instead.

 RAND_seed() is equivalent to RAND_add() with B<randomness> set to B<num>.

+RAND_keep_random_devices_open() use used to control the resource use
+of the random device seed sources. Some seed sources maintain open file
+descriptors which allow them to operate in chroot(2) environment without


"chroot(2) environment" is misleading, because "environment" gives an impression of something where you can start multiple processes. While the only thing that is effective in the context of proposed modification is operation within single process boundary that itself called chroot(2). Even more specifically, chroot(2) has to be called after rand is initialized.

paulidale · 2018-06-12T00:22:16Z

I've made the requested changes. The default behaviour is still to keep the device FDs open.

mspncp · 2018-06-12T07:31:12Z

@paulidale, I have some suggestions for enhancements. For simplicity, I pushed them to my repository, separated in three commits, see suggestions-for-pr-6432. To briefly review the changes, visit mspncp#1, which extends your pull request. To get a local copy, do

git fetch https://github.com/mspncp/openssl.git suggestions-for-pr-6432

Feel free to use (and squash) the suggestions if you like.

dot-asm · 2018-06-12T09:51:58Z

doc/man3/RAND_add.pod

@@ -54,6 +57,12 @@ should consider using L<RAND_load_file(3)> instead.

 RAND_seed() is equivalent to RAND_add() with B<randomness> set to B<num>.

+RAND_keep_random_devices_open() use used to control the resource use of
+the random device seed sources. Some seed sources are able to maintain
+open file descriptors which allow them to operate in chroot(2) jails


I'd even go further saying current process's chroot jail.

dot-asm · 2018-06-12T10:02:25Z

As for not sending file descriptor. Basically rationale is following. It's only appropriate if API can be used in multiple situations. And problem is that suggested interface is usable only in one very particular scenario. While if you pass file descriptor it can used even in other scenarios. For example process that does chroot can pass file descriptor to children (by telling which one is it), which would make it possible to run chroot-ed children. They would naturally have to be modified to identify the file descriptor and pass it down to their copies of libcrypto...

mspncp · 2018-06-21T21:11:37Z

Which this?

I meant the This at the end of CHANGES, line 12

file descriptors open rather than reopening them on each access. This

Unfortunately I did not comment on the 'Files changed' page, but on the commit's page. That's why it came out so strange.

mspncp · 2018-06-21T21:12:42Z

But never mind, it's not important.

paulidale · 2018-06-21T21:15:22Z

This is on a new line now.

paulidale · 2018-06-21T22:32:30Z

Approvals needed from @levitte and @mattcaswell

My concerns have been met

levitte · 2018-06-22T08:32:12Z

(I'm happy with the doc changes, leaving approval of the rest to others)

mattcaswell · 2018-06-22T09:10:09Z

doc/man3/RAND_add.pod

+by default, which allows such sources to operate in a chroot(2) jail
+without the associated device nodes being available. When the B<keep> argument
+is zero, this call disables the retention of file descriptors.  Conversely,
+a non-zero argument enables the retention of file descriptors.


As I understand it this only has an effect the next time we get more entropy from the seed sources, i.e. calling RAND_keep_random_devices_open(0), would not immediately close already open file descriptors. Should we say something about that?

RAND_keep_random_devices_open(0), would not immediately close already open file descriptors.

False, the file descriptors are closed immediately in rand_pool_keep_random_devices_open().

Ah - right! My misunderstanding. In that case, do we need to say that this happens immediately?

The normal use case would be to call RAND_keep_random_devices_open() only once during initialization before the first randomness is generated (in which case it makes no difference how it's implemented). You normally don't flip this feature on and off. So maybe better add a sentence that this should be called during early initialization (unless one is happy with the default).

I've added a sentence and rebased to avoid merge conflicts.

mspncp · 2018-06-25T04:27:18Z

doc/man3/RAND_add.pod

+the B<keep> argument is zero, this call disables the retention of file
+descriptors. Conversely, a non-zero argument enables the retention of
+file descriptors. This function is usually called during initialization
+and it takes effect immediately.


Oh, sorry for not asking earlier: shouldn't we also mention that this feature is enabled by default?

Earlier in the paragraph: Some seed sources maintain open file
descriptors by default, ... Isn't that enough?

Ah ok, I forgot about that. The early bird is not quite awake yet. Thanks for the reminder!

paulidale · 2018-06-25T22:18:06Z

@mattcaswell your approval is the missing piece at the moment :)

mattcaswell

Approved assuming the typo is fixed.

mattcaswell · 2018-06-26T08:56:59Z

doc/man3/RAND_add.pod

@@ -54,6 +57,15 @@ should consider using L<RAND_load_file(3)> instead.

 RAND_seed() is equivalent to RAND_add() with B<randomness> set to B<num>.

+RAND_keep_random_devices_open() is used to control file descriptor


descriptors?

It's "file descriptor use", but maybe @paulidale should break the line after "use", otherwise more readers will stumble over it.

Or "file descriptor usage"?

Oh..right. My mistake. Actually the line break is probably not that big a deal because it won't necessarily be there when this gets rendered as a man page.

…tly. This allows operation inside a chroot environment without having the random device present. A new call, RAND_keep_random_devices_open(), has been introduced that can be used to control file descriptor use by the random seed sources. Some seed sources maintain open file descriptors by default, which allows such sources to operate in a chroot(2) jail without the associated device nodes being available.

paulidale · 2018-06-26T21:16:37Z

Thanks to all, the final documentation change has been made and everything is merged.

…tly. This allows operation inside a chroot environment without having the random device present. A new call, RAND_keep_random_devices_open(), has been introduced that can be used to control file descriptor use by the random seed sources. Some seed sources maintain open file descriptors by default, which allows such sources to operate in a chroot(2) jail without the associated device nodes being available. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from #6432)

kroeckx · 2018-06-26T22:01:44Z

I really didn't have time to look at this. It seems that that /dev/urandom and /dev/random are now always opened by default even if it's never read. I guess I would prefer to only open them when no system call is available, but I can live with the current situation.

paulidale · 2018-06-26T23:21:10Z

I'd be a little uncomfortable about introducing this kind of dependencies between the seed sources. It complicates the code and simpler is better here.

If desirable, I can raise an issue for future consideration -- either add the exception to the code or document the behaviour and suggesting not enabling this source if it is a concern.

@bwelling

Fixes openssl#7022 In pull request openssl#6432 a change was made to keep the handles to the random devices opened in order to avoid reseeding problems for applications in chroot environments. As a consequence, the handles of the random devices were leaked at exit if the random generator was not used by the application. This happened, because the call to RAND_set_rand_method(NULL) in rand_cleanup_int() triggered a call to the call_once function do_rand_init, which opened the random devices via rand_pool_init(void). Thanks to GitHub user @bwelling for reporting this issue.

@bwelling

Fixes #7022 In pull request #6432 a change was made to keep the handles to the random devices opened in order to avoid reseeding problems for applications in chroot environments. As a consequence, the handles of the random devices were leaked at exit if the random generator was not used by the application. This happened, because the call to RAND_set_rand_method(NULL) in rand_cleanup_int() triggered a call to the call_once function do_rand_init, which opened the random devices via rand_pool_init(). Thanks to GitHub user @bwelling for reporting this issue. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #7023)

Commit c7504ae (pr openssl#6432) fixed a regression for applications in chroot environments, which compensated the fact that the new OpenSSL CSPRNG (based on the NIST DRBG) now reseeds periodically, which the previous one didn't. Now the reseeding could fail in the chroot environment if the DEVRANDOM devices were not present anymore and no other entropy source (e.g. getrandom()) was available. The solution was to keep the file handles for the DEVRANDOM devices open by default. In fact, the fix did more than this, it opened the DEVRANDOM devices early and unconditionally in rand_pool_init(), which had the unwanted side effect that the devices were opened (and kept open) even in cases when they were not used at all, for example when the getrandom() system call was available. Due to a bug (issue openssl#7419) this even happened when the feature was disabled by the application. This commit removes the unconditional opening of all DEVRANDOM devices. They will now only be opened (and kept open) on first use. In particular, if getrandom() is available, the handles will not be opened unnecessarily. This change does not introduce a regression for applications compiled for libcrypto 1.1.0, because the SSLEAY RNG also seeds on first use. So in the above constellation the CSPRNG will only be properly seeded if it is happens before the forking and chrooting. Fixes openssl#7419

Commit c7504ae (pr #6432) fixed a regression for applications in chroot environments, which compensated the fact that the new OpenSSL CSPRNG (based on the NIST DRBG) now reseeds periodically, which the previous one didn't. Now the reseeding could fail in the chroot environment if the DEVRANDOM devices were not present anymore and no other entropy source (e.g. getrandom()) was available. The solution was to keep the file handles for the DEVRANDOM devices open by default. In fact, the fix did more than this, it opened the DEVRANDOM devices early and unconditionally in rand_pool_init(), which had the unwanted side effect that the devices were opened (and kept open) even in cases when they were not used at all, for example when the getrandom() system call was available. Due to a bug (issue #7419) this even happened when the feature was disabled by the application. This commit removes the unconditional opening of all DEVRANDOM devices. They will now only be opened (and kept open) on first use. In particular, if getrandom() is available, the handles will not be opened unnecessarily. This change does not introduce a regression for applications compiled for libcrypto 1.1.0, because the SSLEAY RNG also seeds on first use. So in the above constellation the CSPRNG will only be properly seeded if it is happens before the forking and chrooting. Fixes #7419 Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #7437)

Commit c7504ae (pr #6432) fixed a regression for applications in chroot environments, which compensated the fact that the new OpenSSL CSPRNG (based on the NIST DRBG) now reseeds periodically, which the previous one didn't. Now the reseeding could fail in the chroot environment if the DEVRANDOM devices were not present anymore and no other entropy source (e.g. getrandom()) was available. The solution was to keep the file handles for the DEVRANDOM devices open by default. In fact, the fix did more than this, it opened the DEVRANDOM devices early and unconditionally in rand_pool_init(), which had the unwanted side effect that the devices were opened (and kept open) even in cases when they were not used at all, for example when the getrandom() system call was available. Due to a bug (issue #7419) this even happened when the feature was disabled by the application. This commit removes the unconditional opening of all DEVRANDOM devices. They will now only be opened (and kept open) on first use. In particular, if getrandom() is available, the handles will not be opened unnecessarily. This change does not introduce a regression for applications compiled for libcrypto 1.1.0, because the SSLEAY RNG also seeds on first use. So in the above constellation the CSPRNG will only be properly seeded if it is happens before the forking and chrooting. Fixes #7419 Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #7437) (cherry picked from commit 8cfc197)

paulidale force-pushed the persist_random branch from 1650463 to 3f5c921 Compare June 6, 2018 23:37

paulidale mentioned this pull request Jun 6, 2018

Document possible DRBG reseed issues #6388

Closed

1 task

kroeckx reviewed Jun 7, 2018

View reviewed changes

paulidale force-pushed the persist_random branch 2 times, most recently from 68fd6ce to 8e89666 Compare June 8, 2018 01:30

mspncp requested changes Jun 8, 2018

View reviewed changes

paulidale force-pushed the persist_random branch 4 times, most recently from 5eb1641 to 75c0697 Compare June 10, 2018 22:45

dot-asm reviewed Jun 11, 2018

View reviewed changes

dot-asm reviewed Jun 12, 2018

View reviewed changes

paulidale force-pushed the persist_random branch from 211a85f to c8305f9 Compare June 21, 2018 21:15

mattcaswell reviewed Jun 22, 2018

View reviewed changes

paulidale force-pushed the persist_random branch from 1a651c8 to df79de7 Compare June 24, 2018 21:34

mspncp reviewed Jun 25, 2018

View reviewed changes

mattcaswell approved these changes Jun 26, 2018

View reviewed changes

mattcaswell added the approval: done This pull request has the required number of approvals label Jun 26, 2018

paulidale force-pushed the persist_random branch from df79de7 to 04df958 Compare June 26, 2018 21:02

paulidale closed this Jun 26, 2018

paulidale deleted the persist_random branch June 26, 2018 21:16

mspncp mentioned this pull request Jun 26, 2018

SSL_new returns NULL because openssl fails to re-open /dev/Xrandom when in chroot. #5330

Closed

mspncp mentioned this pull request Aug 21, 2018

Don't open random devices while cleaning up. #7023

Closed

This was referenced Oct 18, 2018

rand_unix.c: open random devices on first use only #7437

Closed

Conditionally open random devices on initialisation. #7429

Closed

		@@ -54,6 +57,15 @@ should consider using L<RAND_load_file(3)> instead.

		RAND_seed() is equivalent to RAND_add() with B<randomness> set to B<num>.

		RAND_keep_random_devices_open() is used to control file descriptor

Keep /dev/random open for seeding #6432

Keep /dev/random open for seeding #6432

Conversation

paulidale commented Jun 6, 2018

paulidale commented Jun 6, 2018

t-j-h commented Jun 6, 2018

paulidale commented Jun 6, 2018

t-j-h commented Jun 7, 2018

paulidale commented Jun 7, 2018

paulidale commented Jun 7, 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

kroeckx commented Jun 7, 2018

levitte commented Jun 7, 2018

paulidale commented Jun 7, 2018 • edited

paulidale commented Jun 7, 2018

mspncp commented Jun 7, 2018

paulidale commented Jun 7, 2018 • edited

paulidale commented Jun 8, 2018

mspncp left a comment

Choose a reason for hiding this comment

mspncp Jun 8, 2018 • edited

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

paulidale commented Jun 12, 2018

mspncp commented Jun 12, 2018

Choose a reason for hiding this comment

dot-asm commented Jun 12, 2018

mspncp commented Jun 21, 2018

mspncp commented Jun 21, 2018

paulidale commented Jun 21, 2018

paulidale commented Jun 21, 2018

levitte commented Jun 22, 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

paulidale commented Jun 25, 2018

mattcaswell 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

paulidale commented Jun 26, 2018

kroeckx commented Jun 26, 2018 via email

paulidale commented Jun 26, 2018

paulidale commented Jun 7, 2018 •

edited

paulidale commented Jun 7, 2018 •

edited

mspncp Jun 8, 2018 •

edited