Allow user to locally obfuscate secret in a keystore #5687

ph · 2017-11-22T17:04:36Z

This PR allow users to define sensitive information into an obfuscated data store on disk instead of having them defined in plaintext in the yaml configuration.

This add a few users facing commands:

# create new keystore to disk 
beat keystore create

# add a new key to the store.
beat keystore add output.elasticsearch.password

# remove a key from the store
beat keystore remove output.elasticsearch.password

# list the configured keys without the sensitive information
beat keystore list

The current implementation doesn't allow user to configure the secret with a custom password, this will come in future improvements of this feature.

How to use it

You can reference keys from the keystore using the same syntax that we use for the environment variables.

password: "${output.elasticsearch.password}"

When we unpack the configuration we will resolve the variables using the following priority:

Keystore
Environment variable
variable expansion

TODO before review

Fix the checker that prevent the tests from passing on travis :D
Manual testing
Split the library update vs this PR, update of the library was needed for a few encryption and terminal improvement.
fix the suite, the make testsuite fails locally for me.
movee the CLI test into libbeat
~~[ ] raise an error if we try override a config defined in the yaml file. (Another PR)~~
add testing around the merging of configs
Add doc (Done in another PR)
implement templating

This is the initial version, I don't expect it to be pluggable with every system, but some of the things are in place to make it pluggable.(ie: interface/factory)

closes: #3982

ph · 2017-11-22T17:11:41Z

libbeat/keystore/file_keystore_test.go

+		panic(err)
+	}
+	return fmt.Sprint(path, string(os.PathSeparator), "keystore")
+}


Self note: Add test for bad or corrupt keystore.

ph · 2017-11-22T17:12:11Z

libbeat/paths/paths.go

@@ -8,6 +8,7 @@
 //
 // path.data - Contains things that are expected to change often during normal
 // operations (“registry” files, UUID file, etc.)
+
 //


self note: remove this line, thank you vim.

andrewkroh

Nice job figuring out the AES-GCM stuff.

andrewkroh · 2017-11-22T21:30:27Z

libbeat/cmd/keystore.go

+	return b.Keystore()
+}
+
+// GenKeystoreCmd initialize the Keystore command to manage the Keystore


s/GenKeystoreCmd/genKeystoreCmd/ (probably you haven't linted yet since it's not marked for review and I'm jumping the gun)

I have the linter on as I code, It used to be called GenKeystoreCmd, but I've renamed it since it doesn't need to be exported in any way. Not sure why my linter didn't catch it.

I run:

Enabled Linters: ['gofmt', 'golint', 'go vet']

andrewkroh · 2017-11-22T21:34:11Z

libbeat/cmd/keystore.go

+			flagStdin, _ := cmd.Flags().GetBool("stdin")
+
+			if len(args) == 0 {
+				fmt.Fprintln(os.Stderr, "Failed to create the secret no key provided")


How about Failed to create the secret: no key provided?

andrewkroh · 2017-11-22T21:38:51Z

libbeat/keystore/file_keystore.go

+// Version of the keystore format, will be added at the beginning of the file
+var version = []byte("v1")
+
+// FileKeystore Allows to store key / secrets pair securely into an encrypted local file


Since these comments are rendered in our godoc pages can you please add a period to the end of the sentence. https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences

andrewkroh · 2017-11-22T21:41:28Z

libbeat/keystore/file_keystore.go

+	temporaryPath := fmt.Sprintf("%s.tmp", k.Path)
+	defer func() {
+		if _, err := os.Stat(temporaryPath); os.IsExist(err) {
+			os.Remove(temporaryPath)


I would only register a defer like this after you have "os.Create()ed" or "os.OpenFile()ed". But based on the code you have I think you could add a single os.Remove() call to the error handling for the failed os.Rename().

andrewkroh · 2017-11-22T21:43:07Z

libbeat/keystore/file_keystore.go

+		}
+	}()
+
+	if _, err := os.Stat(temporaryPath); os.IsExist(err) {


You should do this check before adding the above defer or else the file you're asking them to delete will be cleaned up already.

I've removed the defer, so this will also fix this.

andrewkroh · 2017-11-22T22:24:00Z

libbeat/keystore/secure_string.go

+	return s.value, nil
+}
+
+// String custom string implementation to make sure we don't bleed this struct into a string


What about GoString()? Do we need to implement that too to cover %#v or does String() cover that case too? https://golang.org/pkg/fmt/#GoStringer

I believe we also need to cover that, I will add a test case for it.

andrewkroh · 2017-11-22T22:30:06Z

libbeat/keystore/file_keystore.go

+
+	_, err := os.Stat(k.Path)
+
+	if os.IsNotExist(err) && !override {


os.Stat is going to return a nil error if the file exists. So should the logic be if err == nil && !override?

I've changed the logic, it should if the File exist and we don't want to override.

if os.IsExist(err) && !override { return ErrAlreadyExists }

Using os.IsExists() is for checking the error and a nil error will be returned if the file exists. See the implementation for a better understanding.

andrewkroh · 2017-11-22T22:51:45Z

libbeat/keystore/keystore.go

+	// Create Allow to create an empty keystore
+	Create(override bool) error
+
+	// Exists check if the current keystore is persisted


Based on this description maybe the method should be IsPersisted(). And then the implementation would take into account the dirty flag.

andrewkroh · 2017-11-22T22:52:34Z

libbeat/keystore/keystore.go

+	}
+
+	if config.Path == "" {
+		config.Path = fmt.Sprint(dataPath, string(os.PathSeparator), "keystore")


Use filepath.Join().

andrewkroh · 2017-11-22T22:55:06Z

libbeat/cmd/instance/beat.go

+	store, err := keystore.Factory(b.Config.Keystore, b.Config.Path.Data)
+
+	if err != nil {
+		return fmt.Errorf("Could not initialize the keystore: %v", err)


Use a lower cased error message. https://github.com/golang/go/wiki/CodeReviewComments#error-strings

ruflin · 2017-11-26T22:08:36Z

NOTICE.txt

@@ -3581,7 +3581,7 @@ SOFTWARE.

 --------------------------------------------------------------------
 Dependency: golang.org/x/crypto
-Revision: 9419663f5a44be8b34ca85f08abc5fe1be11f8a3
+Revision: 9f005a07e0d31d45e6656d241bb5c0f2efd4bc94


Could you update the revision of the dependencies in an separate PR? This would make sure the version change does not break other things and it would make this PR much smaller.

Agree, I've marked as a TODO in the PR description.

- Added: golang.org/x/crypto/pbkdf2 (Key derivation function) - Added: golang.org/x/crypto/ssh/terminal (Request a password securely on the terminal) - Updated: golang.org/x/sys/unix (required by terminal) - Updated: golang.org/x/sys/windows (required by terminal) Required by: elastic#5687

ph · 2017-11-28T19:24:34Z

@ruflin @andrewkroh @urso I have updated this PR with the following:

Support templating in the configuration
Created commits for the update of the vendored libraries and a corresponding PR at Update Go vendored library for the keystore feature #5735
Extracted the code I've created to check if a settings was already set by another config into a PR to the ucfg repo Allow to diff two configurations and return the difference in the keys. go-ucfg#93
removed the progress and the wip in the title.

If you review #5735 I will rebase this PR this will make it easier to review.

Thanks

ph · 2017-11-28T20:20:13Z

@joshbressers This is the PR for the keystore implementation in filebeat, its closer to the kibana implementation than the LS version.

ph · 2017-11-28T20:24:26Z

jenkins test this please

- Added: golang.org/x/crypto/pbkdf2 (Key derivation function) - Added: golang.org/x/crypto/ssh/terminal (Request a password securely on the terminal) - Updated: golang.org/x/sys/unix (required by terminal) - Updated: golang.org/x/sys/windows (required by terminal) Required by: #5687

ph · 2017-11-29T01:45:33Z

Rebased with master, the vendored files are now gone! :)

ruflin

Skimmed through the code more on a high level. Looks good to me so far, I think it will be also helpful to play around with this feature. And we should do some follow up PR's which also touch other projects but would be overkill to be added to this PR.

ruflin · 2017-11-29T03:46:17Z

libbeat/cmd/instance/beat.go

@@ -413,6 +421,15 @@ func (b *Beat) configure() error {

 	b.Beat.Config = &b.Config.BeatConfig

+	store, err := keystore.Factory(b.Config.Keystore, b.Config.Path.Data)
+	keystore.OverwriteConfigOpts(store)


I suggest you check the error before overwriting the config?

ruflin · 2017-11-29T03:49:38Z

libbeat/cmd/keystore.go

+
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "Error initializing beat: %s\n", err)
+		os.Exit(1)


I initially wanted to comment on why you exit here instead of returning an error and handling it up stream. But I saw that this kind of a common pattern now in cmd package. @exekias Do we need to exit here or could we also return and error? In the past we tried to reduce the places of Exit to 1 place to make testing easier.

Most of theses cmd are tested using the python framework, no? In this case exiting early with os.Exit(1) make sense? There is an [helper in cobra](https://github.com/spf13/cobra/blob/e5f66de850af3302fbe378c8acded2b0fa55472c/cobra/cmd/helpers.go#L63 to do that.

We should some helpers like Fatalf and Fatal that log and do os.Exit(1).

btw. the log.Fatal(f) methods do print the message + do os.Exit(1). It's common practice to use this one in main function on simple tools.

+1 for returning errors and having top-level do the exit handling :)

As cobra requires a Run function one can wrap it like this:

func runWith(fn func (args []string) error) func(*cobra.Command, []string) { return func(_ *cobra.Command, args []string) { if err := fn(args); err != nil { fmt.Error(err) os.Exit(1) } } } ... Run: runWith(func(args []string) error { return errors.New("TODO") })

yeah, and doing this through the logger has the benefit that the logger can flush and sync before exiting.

@urso @andrewkroh I can make the changes on the keystore cmd, we can create another issue to refactor the other command to have the same flow or execution, sound like plan?

Actually, lets do a PR with the runWith change and slowly refactor commands, I think its better to split concerns in multiple PR.

Sorry, I missed the notification here, but I totally agree with @urso, those run wrappers are +:100:

ruflin · 2017-11-29T03:58:28Z

libbeat/common/bytes_test.go

+	assert.NoError(t, err)
+
+	// unlikely to get 2 times the same results
+	assert.False(t, bytes.Equal(v1, v2))


using assert.NotEqual(t, string(v1[:]), string(v2[:])) would probably give a nicer error message.

ruflin · 2017-11-29T03:59:37Z

libbeat/common/terminal/terminal.go

+	"strings"
+)
+
+// ReadInput Capture user input for a question


A bit scary that we now have interactive terminal support :-D

Yes, but all the command can be used without requiring interactive calls, so we are still friendly for orchestration.

ruflin · 2017-11-29T04:00:17Z

libbeat/common/terminal/terminal.go

+	reader := bufio.NewReader(os.Stdin)
+
+	input, err := reader.ReadString('\n')
+


I assume @andrewkroh is not going to like these newlines even thought it's not after a { ;-) Often the err check is directly after without a newline.

ruflin · 2017-11-29T04:02:04Z

libbeat/keystore/file_keystore.go

+// NewFileKeystore returns an new File based keystore or an error, currently users cannot set their
+// own password on the keystore, the default password will be an empty string. When the keystore
+// is initialied the secrets are automatically loaded into memory.
+func NewFileKeystore(keystoreFile string) (Keystore, error) {


As there are potentially coming more key stores in the future, I'm thinking if we should put each in it's own package. keystore/file/... for this one, so this would be just New. BTW that is refactoring that could also be done in a second step.

Agree with that, since the pluggable part of the keystore is not complete, I think we should do it when we add more.

ruflin · 2017-11-29T04:06:28Z

libbeat/keystore/file_keystore.go

+		return fmt.Errorf("cannot encrypt the keystore: %v", err)
+	}
+
+	f, err := os.OpenFile(temporaryPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, filePermission)


We should really abstract out on how we write files from the filebeat registry and winlobeat registry: https://github.com/elastic/beats/blob/master/filebeat/registrar/registrar.go#L230 There are strange issues that can happen on shutdown and I remember @andrewkroh fixed on of those in winlogbeat. Having the logic on one place would make sure fixes are applied everywhere.

Also see the safeFileRotate helper: https://github.com/elastic/beats/blob/master/filebeat/registrar/registrar.go#L251

@ph Could we create a follow up meta issue to track such things we should cleanup after getting this PR in. Happy to help with abstracting out this logic.

@ruflin I have created #5755 for a specific follow up.

@ruflin Also create this meta issue #5757 to do a bit more work.

ph · 2017-12-19T17:44:45Z

@tsg Ready for another round of testing!

tsg · 2017-12-20T00:11:31Z

libbeat/cmd/instance/beat.go

+	// We have to initialize the keystore before any unpack or merging the cloud
+	// options.
+	keystoreCfg, _ := cfg.Child("keystore", -1)
+	pathData, _ := cfg.String("path.data", -1)


I wonder if path.data (typically /usr/share/beatname/data is the right place for the keystore. Alternatively we could consider path.config (typically /etc/beatname)?

We should probably follow what ES/KB did, do you know?

Also, I guess we'd want to check the permissions on the file like we do with the YAML config files.

Right, /etc/beatname make more sense, I haven't checked ES/KB I will verify that and make the change if needed.

We do check for permission https://github.com/elastic/beats/pull/5687/files#diff-33a534765f797732e272dc741db414f4R233 if StrictPerm(0600) is True.

tsg · 2017-12-20T00:12:58Z

@ph did you do any tests of the commands on Windows? Just very slightly worried about the terminal.Prompt calls and how they are implemented on Windows.

ph · 2017-12-20T01:13:06Z

@tsg Concerning windows, I've tested it early and it was OK, but I will do another complete run just to be on the safe side.

ph · 2017-12-20T01:42:45Z

@tsg small issue on windows powershell, will get a fix for that. Good point, I am still assuming more compatibility than I should coming from the jvm.

ph · 2017-12-20T02:32:55Z

@tsg Windows is all good and happy now with the exact same behavior as unix. ssh/terminal for readPassword handle windows gracefully

ph · 2017-12-20T15:52:50Z

@tsg I've addressed your concerns, adding log debug when successfully resolving keys from the keystore and where the keystore is loaded. Also I've changed the default path of the keystore to ${path.config}/${beatname}.keystore instead of ${path.data}/keystore to reflect Elasticsearch installation.

ph · 2017-12-20T16:36:28Z

jenkins test this please

tsg · 2017-12-20T21:42:26Z

@ph The last Jenkins run is almost green, but there's a failure on Windows that looks related to the PR:

16:50:16 System testing filebeat
16:55:25 S..S.........S.............................................FSSSSSS..................................S............S......SS......S...............
16:55:25 ======================================================================
16:55:25 FAIL: Test that we correctly do string replacement with values from the keystore
16:55:25 ----------------------------------------------------------------------
16:55:25 Traceback (most recent call last):
16:55:25   File "C:\Users\jenkins\workspace\elastic+beats+pull-request+multijob-windows\beat\filebeat\label\windows\src\github.com\elastic\beats\filebeat\tests\system\test_keystore.py", line 36, in test_keystore_with_present_key
16:55:25     proc.check_kill_and_wait(1)
16:55:25   File "C:\Users\jenkins\workspace\elastic+beats+pull-request+multijob-windows\beat\filebeat\label\windows\src\github.com\elastic\beats\filebeat\tests\system\../../../libbeat/tests/system\beat\beat.py", line 91, in check_kill_and_wait
16:55:25     return self.check_wait(exit_code=exit_code)
16:55:25   File "C:\Users\jenkins\workspace\elastic+beats+pull-request+multijob-windows\beat\filebeat\label\windows\src\github.com\elastic\beats\filebeat\tests\system\../../../libbeat/tests/system\beat\beat.py", line 80, in check_wait
16:55:25     exit_code, actual_exit_code)
16:55:25 AssertionError: Expected exit code to be 1, but it was 2
16:55:25

I triggered another Travis run on Filebeat as well.

tsg

LGTM, if the test failure is solved.

This PR allow users to define sensitive information into an obfuscated data store on disk instead of having them defined in plaintext in the yaml configuration. This add a few users facing commands: beat keystore create beat keystore add output.elasticsearch.password beat keystore remove output.elasticsearch.password beat keystore list The current implementation doesn't allow user to configure the secret with a custom password, this will come in future improvements of this feature.

ph · 2017-12-20T21:56:27Z

@tsg I've pushed a new commit for the test on windows, I will monitor the greeness of it. The Appveyor tests are also failing for winlogbeat, but this should not be related to anything this PR touch,.

dol · 2018-01-05T23:29:10Z

libbeat/keystore/file_keystore.go

+
+	// Stretch the user provided key
+	password, _ := k.password.Get()
+	passwordBytes := pbkdf2.Key(password, salt, iterationsCount, keyLength, sha512.New)


Consider replacing pbkdf2 with a never key derivation function like scrypt or even Argon2. Argon2 was very recently merged into golang.org/x/crypto/ .
Some resources about the topic:
https://www.linkedin.com/pulse/top-password-hashing-schemes-employ-today-chintan-jain-mba-cissp
https://download.libsodium.org/doc/password_hashing/
https://core.trac.wordpress.org/ticket/39499
https://gitlab.com/cryptsetup/cryptsetup/issues/119

In addition I would recommend to store the key derivation function and the iteration along side with the VERSION|SALT|IV|PAYLOAD.
If you decide to change the iteration count or the key derivation function you don't need to increase the file format version.
This is kind of similar to the https://secure.php.net/manual/en/function.password-hash.php. In your case the the key would be left out.

With this implementation it's possible to increase the key derivation parameters to prevent faster CPU/GPU's. To achieve this the maintainer needs to alter the key derivation parameters from time to time and if a change is detected from the key derivation parameters stored in the storage file the encrypted payload needs to be decrypted and encrypted with the new parameters.
Similar to https://secure.php.net/manual/en/function.password-needs-rehash.php

To reduce code duplication I would extract the passwordBytes generation passwordBytes := pbkdf2.Key(password, salt, iterationsCount, keyLength, sha512.New) to it own private function.

Btw: The rest of the encryption part looks good to me.

@dol I've created the following issues:

#6015
#6016

I will do the change right away to add the function in the format and check for argon2.

ph · 2018-01-07T18:43:02Z

Good suggestion I will do a follow up on that.

On Fri, Jan 5, 2018 at 6:29 PM Dominic ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In libbeat/keystore/file_keystore.go <#5687 (comment)>: > + // randomly generate the salt and the initialization vector, this information will be saved + // on disk in the file as part of the header + iv, err := common.RandomBytes(iVLength) + + if err != nil { + return nil, err + } + + salt, err := common.RandomBytes(saltLength) + if err != nil { + return nil, err + } + + // Stretch the user provided key + password, _ := k.password.Get() + passwordBytes := pbkdf2.Key(password, salt, iterationsCount, keyLength, sha512.New) Consider replacing pbkdf2 with a never key derivation function like scrypt or even Argon2. Argon2 was very recently merged into golang.org/x/crypto/ . Some resources about the topic: https://www.linkedin.com/pulse/top-password-hashing-schemes-employ-today-chintan-jain-mba-cissp https://download.libsodium.org/doc/password_hashing/ https://core.trac.wordpress.org/ticket/39499 https://gitlab.com/cryptsetup/cryptsetup/issues/119 In addition I would recommend to store the key derivation function and the iteration along side with the VERSION|SALT|IV|PAYLOAD. If you decide to change the iteration count or the key derivation function you don't need to increase the file format version. This is kind of similar to the https://secure.php.net/manual/en/function.password-hash.php. In your case the the key would be left out. With this implementation it's possible to increase the key derivation parameters to prevent faster CPU/GPU's. To achieve this the maintainer needs to alter the key derivation parameters from time to time and if a change is detected from the key derivation parameters stored in the storage file the encrypted payload needs to be decrypted and encrypted with the new parameters. Similar to https://secure.php.net/manual/en/function.password-needs-rehash.php To reduce code duplication I would extract the passwordBytes generation passwordBytes := pbkdf2.Key(password, salt, iterationsCount, keyLength, sha512.New) to it own private function. Btw: The rest of the encryption part looks good to me. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5687 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAACgIZqPS5LnJi_-FzjyI2BdzmSYzG7ks5tHrBHgaJpZM4Qnuc3> .

-- ph

ph added in progress Pull request is currently in progress. libbeat labels Nov 22, 2017

ph force-pushed the feature/keystore branch from d141137 to 85e2b36 Compare November 22, 2017 17:08

ph commented Nov 22, 2017

View reviewed changes

ph changed the title ~~Allow user to locally obfuscate secret in a keystore~~ [WIP] Allow user to locally obfuscate secret in a keystore Nov 22, 2017

ph mentioned this pull request Nov 22, 2017

Go vet complains on composite literal uses unkeyed fields #5690

Closed

ph closed this Nov 22, 2017

ph reopened this Nov 22, 2017

ph mentioned this pull request Nov 22, 2017

Go vet complains on composite literal uses unkeyed fields #5691

Merged

andrewkroh reviewed Nov 22, 2017

View reviewed changes

ph force-pushed the feature/keystore branch 2 times, most recently from 7662b76 to 515280d Compare November 23, 2017 19:24

ph mentioned this pull request Nov 24, 2017

Inconsistent behavior of make check locally and on travis. #5704

Closed

ruflin reviewed Nov 26, 2017

View reviewed changes

ph mentioned this pull request Nov 28, 2017

Update Go vendored library for the keystore feature #5735

Merged

ph force-pushed the feature/keystore branch from 26cd53a to 5910ba9 Compare November 28, 2017 19:14

ph changed the title ~~[WIP] Allow user to locally obfuscate secret in a keystore~~ Allow user to locally obfuscate secret in a keystore Nov 28, 2017

ph added review and removed in progress Pull request is currently in progress. labels Nov 28, 2017

ph mentioned this pull request Nov 28, 2017

Keystore: Allow user to set their own password on the keystore #5737

Closed

2 tasks

ph force-pushed the feature/keystore branch from 5910ba9 to f59b4f3 Compare November 29, 2017 01:44

ruflin reviewed Nov 29, 2017

View reviewed changes

ph mentioned this pull request Nov 29, 2017

Refactor how we write support file to disk (Registry, Keystore) #5755

Closed

ph force-pushed the feature/keystore branch 4 times, most recently from b5445db to 4599714 Compare December 19, 2017 16:04

tsg reviewed Dec 20, 2017

View reviewed changes

ph force-pushed the feature/keystore branch from 44a99a3 to 5232ccf Compare December 20, 2017 02:31

ph force-pushed the feature/keystore branch 3 times, most recently from 8864815 to 569ee8b Compare December 20, 2017 15:49

tsg approved these changes Dec 20, 2017

View reviewed changes

ph added 2 commits December 20, 2017 16:53

Changelog

f34a28a

ph force-pushed the feature/keystore branch from 85e39bf to f34a28a Compare December 20, 2017 21:53

tsg merged commit 0f09311 into elastic:master Dec 22, 2017

dol reviewed Jan 5, 2018

View reviewed changes

dedemorton mentioned this pull request Jan 11, 2018

Beats doc updates in 6.x #5632

Closed

37 tasks

dedemorton mentioned this pull request Jan 25, 2018

Add docs about keystore #6173

Merged

roncohen mentioned this pull request Jan 26, 2018

Test and document keystore functionality from beats for APM Server elastic/apm-server#559

Closed


		_, err := os.Stat(k.Path)

		if os.IsNotExist(err) && !override {

		reader := bufio.NewReader(os.Stdin)

		input, err := reader.ReadString('\n')

Allow user to locally obfuscate secret in a keystore #5687

Allow user to locally obfuscate secret in a keystore #5687

Conversation

ph commented Nov 22, 2017 • edited Loading

How to use it

TODO before review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh 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

andrewkroh Nov 23, 2017 • edited Loading

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

ph Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

ph commented Nov 28, 2017

ph commented Nov 28, 2017

ph commented Nov 28, 2017

ph commented Nov 29, 2017

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph Nov 29, 2017 • edited Loading

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

ph Nov 29, 2017 • edited Loading

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

ph commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsg commented Dec 20, 2017

ph commented Dec 20, 2017

ph commented Dec 20, 2017

ph commented Dec 20, 2017

ph commented Dec 20, 2017

ph commented Dec 20, 2017

tsg commented Dec 20, 2017

tsg left a comment

Choose a reason for hiding this comment

ph commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph commented Jan 7, 2018 via email

ph commented Nov 22, 2017 •

edited

Loading

andrewkroh Nov 23, 2017 •

edited

Loading

ph Nov 27, 2017 •

edited

Loading

ph Nov 29, 2017 •

edited

Loading

ph Nov 29, 2017 •

edited

Loading