Add Key Rotation to MessageEncryptor and MessageVerifier and simplify the Cookies middleware by mjc-gh · Pull Request #29716 · rails/rails

mjc-gh · 2017-07-08T00:32:48Z

Summary

This PR introduces ActiveSupport::KeyRotator which wraps KeyGenerator as well as MessageEncryptor and MessageVerifier and provides an interface for easily rotating between different encryption ciphers or message digests, salts, and secrets.

This PR also simplifies the cookies middleware and removes several of the internal legacy middlewares. Upgrading both signed and encrypted legacy cookies is now handled by rotation capable middlewares which each use a KeyRotator instance. Is also introduces a new interface for configuring the middleware.

Other Information

This feature spawned from discussions had in the Rails’ basecamp.

I still need to update the configuring and security markdown guides with details of this change. I plan on adding a new section the security guide detailing how secrets and salts can be rotated. I also want to add more rotation tests to railties/test/application/middleware/cookies_test.rb.

Any and all feedback is welcomed!

/cc @matthewd @kaspth

bdewater · 2017-07-09T14:24:13Z

Took a brief look at the code, looks nice 👏 One question: the rotator in it's current state seems to be for upgrading cookies and people using MessageEncryptor defaults, which is probably what most people need anyway. But passing in symbols and raising if it doesn't match a known scheme excludes people who use MessageEncryptor now with their own settings.

The shorthands for old and new defaults make sense, but it isn't is possible to pass in your own legacy scheme hash that is just passed on to create a new MessageEncryptor. Likewise "only :aes_gcm may be used for the primary scheme" seems very prescriptive and not in line with the 'provide sharp knives' philosophy. Should we make it a bit more general so it can rotate all configurations of MessageEncryptor?

kaspth

Made a focused review on how I'd like to bring the idea of key rotation to the fore for users (rather than focusing purely on our need to upgrade their cookies) 😊

Appreciate the work you've been doing so far! You seem really comfortable in the codebase 😄

kaspth · 2017-07-09T18:47:35Z

actionpack/lib/action_dispatch/railtie.rb

I find the scheme wording confusing. Isn't that what aes-256-gcm is? Also dislike the legacy wording since it doesn't clarify how people can use this to rotate their keys.

Ideally I'd like the config to be something like:

# config/initializers/rotations.rb # Here's some instructions on how to do cookie rotations right and why should be. # - Rotate your secret key base whenever a coworker leaves, etc. Rails.application.config.secrets.tap do |secrets| secrets.rotate_out secret_key_base: secrets.old_secret_key_base secrets.rotate_out cipher: 'aes-256-gcm' secrets.rotate_out :signed, digest: 'xxx' # We need some way to explain that a rotation targets either signed or encrypted messages. end Rails.application.config.action_dispatch.tap do |config| config.cookies.rotate_out signed_salt: config.old_signed_salt # It'll use the new config.signed_salt but add this as a fallback. end

With that API our new framework defaults could then contain:

# config/initializers/new_framework_defaults_5_2.rb # … # Use AES 256 GCM authenticated encryption in cookies and ActiveSupport::MessageEncryptor. # Rails.application.secrets.rotate_out cipher: 'aes-xx-cbc'

I agree with these wording changes.

The word "scheme" is unclear and could refer to several things really. It's probably best to just avoid that term altogether.

Removing the "legacy" wording is a good idea as well since this feature is more than just legacy support. Your configuration above is really nice and clean! I can rework my changes to introduce this style of configuration for this feature.

@kaspth any thoughts on what I should call a complete configuration for a cipher/digest, the salts, and a secret?

I'm thinking I'm just going to call it an encryptor_config and verifiy_config within the code but am open to something more fitting here!

How about just verifiers and encryptors? We should support passing a message verifier or encryptor instance to rotate_out, which I forgot to add above.

I'm still on the fence about the _out suffix. Do think you think just config.secrets.rotate would be clear enough to imply that it's the old value that goes there?

I like the idea of passing an encryptor or verifier directly. This would be for more advanced uses and would be great to support.

Under the hood, I think we can just have these instances be passed to KeyRotator and have the class able to insert arbitrary encryptors or verifiers into its chain. Modifying KeyRotator to support this should somewhat address @bdewater's feedback about making KeyRotator more versatile in general.

I also think the config.secrets.rotate syntax should be clear enough. I feel the "rotating out" concept is implied by the feature as a whole.

mjc-gh · 2017-07-09T20:25:05Z

Thanks @bdewater and @kaspth for the feedback!

As far as :cipher options go for the KeyRotator, I think I'm going to drop the symbol usage and just use the same strings MessageEncryptor (and thus OpenSSL) would expect. For instance, instead of using cipher: :aes_gcm we can just use cipher: 'aes-256-gcm'.

One question: the rotator in it's current state seems to be for upgrading cookies and people using MessageEncryptor defaults, which is probably what most people need anyway. But passing in symbols and raising if it doesn't match a known scheme excludes people who use MessageEncryptor now with their own settings.

I agree with this overall, @bdewater. Opening up KeyRotator to work with potentially any cipher option for MessageEncrytor would be a really nice feature. What would the expectation here for using the KeyRotator without a defined preset? The tricky part here is whether or not the supplied secret string should be passed through a KeyGenerator or not.

Maybe, in general, we can leave the :secret option as. It's mostly meant to mirror secret_key_base and should be used with a KeyGenerator and salt for deriving actual keys. Maybe we can then add a alternative :key option to the configuration which is not run through a KeyGenerator is passed directly to either MessageEncryptor or MessageVerifier. This sort of option would be meant for more advanced users who are generating sufficiently random and long keys on their own. The :key option would then not require a :salt option since no key derivation is occurring.

I'm going to rework this class some more such that all potential uses are easily supported in a clear and understandable API. It's far to versatile of a feature to leave for just preset options. Thanks very much for your feedback here!

Likewise "only :aes_gcm may be used for the primary scheme" seems very prescriptive and not in line with the 'provide sharp knives' philosophy.

This comment is actually out-of-date now. Prior to submitting this PR I had limits in place with what could be used as the primary_scheme. My original thinking here is that security APIs should be very easy to use correctly. Maybe we can explore this aspect a bit elsewhere with warnings or something...?

bdewater · 2017-07-10T13:43:59Z

Maybe, in general, we can leave the :secret option as. It's mostly meant to mirror secret_key_base and should be used with a KeyGenerator and salt for deriving actual keys. Maybe we can then add a alternative :key option to the configuration which is not run through a KeyGenerator is passed directly to either MessageEncryptor or MessageVerifier. This sort of option would be meant for more advanced users who are generating sufficiently random and long keys on their own. The :key option would then not require a :salt option since no key derivation is occurring.

This sounds like a great approach! Better defaults while still allowing a configuration to fully preserve existing behaviour for interoperability 👍 My worry with combining MessageEncryptor with secret_key_base and no opt-out is that changing the key base could break more than just existing sessions, e.g. when you're using it to exchange encrypted messages across different (web/mobile/etc) apps that expect AES-256-CBC with HMAC-SHA1 and a long key.

bdewater · 2017-07-10T14:56:05Z

Also a heads-up: you'll want to add the frozen string magic comment to new files, see #29728 :)

rafaelfranca · 2017-07-10T21:06:45Z

railties/lib/rails/application.rb

The Railtie component can't use ActionDispatch constants. We should inverse the dependency.

I had a feeling this was the case. I'm going to rework how the various configuration defaults are set, fix the dependency issues, and remove the top-level CookieSecurity constant.

rafaelfranca · 2017-07-10T21:08:23Z

actionpack/lib/action_dispatch/railtie.rb

This is a normal hash, so how config.action_dispatch["action_dispatch.secret_key_base"] would return something?

rafaelfranca · 2017-07-10T21:10:11Z

actionpack/lib/action_dispatch/cookie_security.rb

I'm not a fan of this top level constant. This class seems to be private API so maybe move to inside the actionpack/lib/action_dispatch/middleware/cookies.rb file?

rafaelfranca · 2017-07-10T21:19:22Z

activesupport/lib/active_support/key_rotator.rb

Should not the key rotator wrap the legacy logic automatically? Like, why would someone use the key rotator and use decrypt_and_verify without rescuing the exception and calling decrypt_and_verify_with_legacy_encryptors?

This is how I originally designed the KeyRotator class. The problem was, in the cookie middleware parse methods, we need to know when the primary encryptor/verifier fails so that the cookie can be updated.

I experimented with having decrypt_and_verify accept an optional block that is called when the primary encryptor/verifier fails. It didn't do the trick though because you end up having to deserialize before doing the actual cookie update.

Definitely open to ideas on how we can have decrypt_and_verify and verify signal to their caller that the primary encryptor or verifier were not used. This would definitely result in a simpler and easier to use API as you have suggested.

maybe we could invert that. Implement a decrypt_and_verify_without_legacy_encryptors (or just exposing the primary_encryptor for those who want control when the primary encryptor could not be used.

kaspth · 2017-07-17T18:20:24Z

I think using both key and secret wording seems confusing. How would users know which has passed through a key_generator and which has not?

I'm having trouble remembering right now for instance 😅

Perhaps it could be generated_key?

mjc-gh · 2017-07-17T20:53:33Z

I think using both key and secret wording seems confusing. How would users know which has passed through a key_generator and which has not?

I'm having trouble remembering right now for instance 😅

Perhaps it could be generated_key?

Maybe raw_key or even raw_key_material is a better name for what it actually is?

This option will also be clearly documented in that it is used as an actual cipher key and should be sufficiently long, random, and properly encoded. That is, it should probably be a raw byte string from SecureRandom or something similar. It should not be encoded as a hex string or otherwise either, which can potentially lead to losses in entropy.

mjc-gh · 2017-08-01T01:15:00Z

With some great advice from @kaspth, I reworked this PR and added the rotation features directly to the MessageEncryptor and MessageVerifier classes with the help of some small mixins. Overall this approach feels a lot better and it addresses a lot of the feedback raised in this PR.

There are still some failing tests with regards to "upgrading" secret_token HMAC cookies to encrypted cookies. I should be able to easily handle this case in the cookies middleware directly with a legacy verifier in the EncryptedKeyRotatingCookieJar middleware class. I also need to add some more docs and update the configuration guide with notes on several new cookie middleware configuration options. These options should give a high degree of flexibility to developers and will enable them to use different digests for signed cookies (like SHA256) and different ciphers for encrypted cookies.

As always, any and all feedback is welcomed!

rafaelfranca · 2017-08-14T19:26:57Z

actionpack/lib/action_dispatch.rb

Is this needed? I tried to find where this constant is defined and could not.

Yikes, this is stale code. I can remove it.

rafaelfranca · 2017-08-14T19:29:07Z

actionpack/lib/action_dispatch/middleware/cookies.rb

do/end here. If needed it is better to extract to a local variable than using { to change the precedence.

rafaelfranca · 2017-08-14T19:29:51Z

actionpack/lib/action_dispatch/middleware/cookies.rb

It is not clear why you need to use return here. It seems it is because this block is called more than once. So maybe the method should be clear about that.

The verify method (and the decrypt_and_verify method) call an optional block when a rotated instance is used to verify the message. We need to rewrite the cookie's value when this occurs and this optional block is utilized to signal that.

The tricky part is we need actually write the deserialize message so within this block deserialize is called and the cookie is updated. The return is there so the parse method returns the deserialized value from within the block. If we didn't return here, the outer deserialize would also get invoked and deserialization would effectively occur twice.

I do agree that return here is a little confusing. I'm definitely open to a better way of signal to the caller of verify or decrypt_and_verify when a rotated instance is used so that we can better handle this case.

rafaelfranca · 2017-08-14T19:33:05Z

activesupport/lib/active_support/messages/rotator.rb

This will make all the constants inside it to be not documented.

kaspth · 2017-09-14T19:03:00Z

actionpack/lib/action_dispatch/railtie.rb

Looks like we're always calling this with new.cookies_rotations, then there's no point to the namespace handling in the Configuration class and we should just move up Rotation to RotationConfiguration.

Should I convert this class to a Singleton? Or should I make rotate as well as the signed and encrypted arrays into class/module methods?

Nope, let's keep it a regular class. Assigning it to Rails.application.config makes it a "singleton", i.e. accessible across the app.

kaspth · 2017-09-14T19:03:40Z

actionpack/lib/action_dispatch/railtie.rb

These now need to be yanked.

kaspth · 2017-09-14T19:05:02Z

actionpack/lib/action_dispatch/railtie.rb

Let's move this to the alongside the other _salts up top and skip the ||= for =.

kaspth · 2017-09-14T19:17:27Z

actionpack/lib/action_dispatch/railtie.rb

There's a part of me that thinks we should turn:

config.action_dispatch.signed_cookie_salt = "signed cookie" config.action_dispatch.encrypted_cookie_salt = "encrypted cookie" config.action_dispatch.encrypted_signed_cookie_salt = "signed encrypted cookie" config.action_dispatch.use_authenticated_cookie_encryption = false

into:

config.action_dispatch.cookies = ActiveSupport::OrderedOptions.new config.action_dispatch.cookies.signed_salt = "signed cookie" config.action_dispatch.cookies.encrypted_salt = "encrypted cookie" config.action_dispatch.cookies.encrypted_signed_salt = "signed encrypted cookie" # These three are old 👆, but these three haven't shipped 👇. config.action_dispatch.cookies.authenticated_encrypted_salt = "authenticated encrypted cookie" config.action_dispatch.cookies.authenticated_encryption = false config.action_dispatch.cookies.rotations = ActiveSupport::Messages::RotationConfiguration.new

We'd need to deprecate the old ones. Though I think it's highly unlikely that many apps have overriden those default salts, so the hurt isn't too huge. Either that or we just reassign for something so trivial.
It's probably out of scope for this, so let me look into that separately.

Though by the way, why do we need the authenticated_encrypted_cookie_salt? We could just as well reuse the encrypted_signed_cookie_salt?

When I added AEAD support I wanted to use a different salt value as this is a best practice when deriving keys for different uses.

kaspth · 2017-09-14T19:37:23Z

Yup, let's move forward with that configuration API :)

mjc-gh · 2017-09-14T20:36:15Z

Remember that it can't just be ** it has to be **options so we can pass them onto verifier.verified(*args, **options) etc.

Good call on this. I can update those calls and even add a test case for it.

mjc-gh · 2017-09-14T20:54:21Z

Yup, and it does! After the credentials thing, we're now validating Rails.application.secret_key_base when it's called with this:

Nice to "centralized" method for validating the configuration!

kaspth · 2017-09-17T11:51:10Z

activesupport/lib/active_support/message_encryptor.rb

constructor reads like a JavaScript word, I'd say the same arguments as +new+.

kaspth · 2017-09-17T11:57:50Z

activesupport/lib/active_support/messages.rb

Isn't this a bit overkill?

Had a feeling it would be. I can just remove this and require the class as needed.

kaspth · 2017-09-17T17:24:22Z

Looks like the documentation update is missing and then we need to do some commit squashing. Perhaps the Active Support changes and then the cookie changes are two separate commits? Unless you can spot a division that's more git history apt 😊

mjc-gh · 2017-09-17T18:14:31Z

@kaspth yup, the docs is next up for me. Sounds good on the squash commit. That division makes the most sense to me.

mjc-gh · 2017-09-18T02:57:11Z

Thought some more about the RotationConfiguration interface and I think we should now allow :encrypted and :signed rotations to be defined at the same time. The options differ slightly and I think we might run into some problems as a result.

Additionally, allowing both encrypted and signed cookies to have the same configuration would lead to the same salt and thus the same key being used for different security features which is not a good idea security-wise. Thoughts @kaspth? Should be easy to remove this small part of the class.

Edit: It's probably OK to define both at the same time if just a :secret is provided and no salts are directly provided. I suppose this may actually be the most common use case (ie rotating the secret_key_base while using the default salts).

mjc-gh · 2017-09-20T23:57:14Z

Updated the guides and the documentation some more.

I hope it's OK that I reworked the session cookie section of the Security guide a bit. Just felt odd at this point to have that section lead with legacy details. I'd rather explain security for the current version upfront then explain differences in prior versions after.

Want to just review it again with fresh eyes and then squash the commits.

kaspth · 2017-09-23T19:43:54Z

Let's use the action_dispatch.use_authenticated_cookie_encryption option in the middleware then squash the commits.

I'm going to be out of the country the week after this. So this has to be wrapped up soon. If you can get to the squashing tomorrow then great. Otherwise I'll merge and do some follow up codewise then 😊

mjc-gh · 2017-09-23T20:33:42Z

Now I'm actually a bit confused on how action_dispatch.use_authenticated_cookie_encryption should work. Setting this value to false should lead to CBC cookies being used, correct?

For Rails 5.2 this value will default to true and CBC cookie upgrading will be added as a rotation (because the CBC salts are still set and upgrade_legacy_hmac_aes_cbc_cookies? is true).

I wrote a test case for use_authenticated_cookie_encryption to false and it's failing. I think somewhere between this PR and the AEAD cookie PR the EncryptedCookieJar become to simplified. Should be easy to fix this though but I want to make sure I'm clear on what the role of use_authenticated_cookie_encryption exactly is.

Both classes now have a rotate method where new instances are added for each call. When decryption or verification fails the next rotation instance is tried.

mjc-gh · 2017-09-23T21:28:10Z

Squashed the commits. I think I broke something with the use_authenticated_cookie_encryption change. Will try to fix that ASAP.

Using the action_dispatch.cookies_rotations interface, key rotation is now possible with cookies. Thus the secret_key_base as well as salts, ciphers, and digests, can be rotated without expiring sessions.

kaspth · 2017-09-24T16:58:22Z

Great! The build looked fine and green to me 😄👏

mjc-gh · 2017-09-24T17:25:21Z

One last thing, @kaspth: what about having the cookies middleware use CBC encryption when use_authenticated_cookie_encryption is set to false? I can add a test case and small patch to the cookies middleware to support this in a separate PR if need be.

kaspth · 2017-09-24T17:32:05Z

What's it doing currently? I think false and nil should probably work the same for that config (depending on which Rails version the app was generated on).

mjc-gh · 2017-09-24T17:36:34Z

Currently it's just responsible for enabling the upgrade behavior, which rotates CBC to GCM. I feel setting it to false should also lead to CBC being used. The following patch should suffice:

diff actionpack/lib/
diff --git a/actionpack/lib/action_dispatch/middleware/cookies.rb b/actionpack/lib/action_dispatch/middleware/cookies.rb
index b383164..dbe46be 100644
--- a/actionpack/lib/action_dispatch/middleware/cookies.rb
+++ b/actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -599,9 +599,16 @@ class EncryptedKeyRotatingCookieJar < AbstractCookieJar # :nodoc:
       def initialize(parent_jar)
         super
 
-        key_len = ActiveSupport::MessageEncryptor.key_len(encrypted_cookie_cipher)
-        secret = request.key_generator.generate_key(request.authenticated_encrypted_cookie_salt, key_len)
-        @encryptor = ActiveSupport::MessageEncryptor.new(secret, cipher: encrypted_cookie_cipher, serializer: SERIALIZER)
+        if request.use_authenticated_cookie_encryption
+          key_len = ActiveSupport::MessageEncryptor.key_len(encrypted_cookie_cipher)
+          secret = request.key_generator.generate_key(request.authenticated_encrypted_cookie_salt, key_len)
+          @encryptor = ActiveSupport::MessageEncryptor.new(secret, cipher: encrypted_cookie_cipher, serializer: SERIALIZER)
+        else
+          key_len = ActiveSupport::MessageEncryptor.key_len("aes-256-cbc")
+          secret = request.key_generator.generate_key(request.encrypted_cookie_salt, key_len)
+          sign_secret = request.key_generator.generate_key(request.encrypted_signed_cookie_salt)
+          @encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret, cipher: "aes-256-cbc", serializer: SERIALIZER)
+        end
 
         request.cookies_rotations.encrypted.each do |rotation_options|
           @encryptor.rotate serializer: SERIALIZER, **rotation_options

Edit: made a branch and can submit a PR for it: https://github.com/rails/rails/compare/master...mikeycgto:actiondispatch-use-aead-encrypted-cookies-patch?expand=1

kaspth · 2017-09-24T19:48:38Z

Had a bit of a change of heart on the rotate API as I realized that neither the verifier or encryptor ever mentions key generators or salts. They're only meant to work with generated secrets, so I didn't like to add the extra support there after all.

Think we'll have to go through Rails.application.key_generator somehow.

The cookies middleware then felt a little strange, so I chose to nix the :salt and :key_generator support for now. I wasn't happy with how it felt at the moment. Perhaps this is good enough for 5.2, then we can let it evolve a bit on its own once its out.

The updated code is in 36888b9..38308e6

mjc-gh · 2017-09-24T20:32:50Z

Sounds good @kaspth. The changes in the documentation and Message::KeyRotator look good to me. For the best to keep this as simplified as possible 👍

Let me know if I should submit my use_authenticated_cookie_encryption PR. I feel like the expectation would be if this is false, CBC should be used for cookies.

mjc-gh force-pushed the active-support-key-rotator branch 6 times, most recently from 857fdde to c7405d2 Compare July 8, 2017 17:35

kaspth reviewed Jul 9, 2017

View reviewed changes

rafaelfranca reviewed Jul 10, 2017

View reviewed changes

mjc-gh force-pushed the active-support-key-rotator branch from c7405d2 to 523987c Compare August 1, 2017 00:55

mjc-gh changed the title ~~Add ActiveSupport::KeyRotator and simplify the Cookies middleware~~ Add Key Rotation to MessageEncryptor and MessageVerifier and simplify the Cookies middleware Aug 1, 2017

mjc-gh force-pushed the active-support-key-rotator branch 2 times, most recently from db12569 to 28aef6b Compare August 1, 2017 01:02

mjc-gh force-pushed the active-support-key-rotator branch 5 times, most recently from 4f8726c to cdb3774 Compare August 9, 2017 14:32

mjc-gh force-pushed the active-support-key-rotator branch from cdb3774 to a5db533 Compare August 9, 2017 16:56

rafaelfranca reviewed Aug 14, 2017

View reviewed changes

rafaelfranca added this to the 5.2.0 milestone Aug 14, 2017

mjc-gh mentioned this pull request Aug 16, 2017

Add credentials using a generic EncryptedConfiguration class #30067

Merged

4 tasks

mjc-gh force-pushed the active-support-key-rotator branch from 3fdee6b to 4a64ada Compare August 19, 2017 20:43

kaspth reviewed Sep 14, 2017

View reviewed changes

kaspth reviewed Sep 17, 2017

View reviewed changes

Add key rotation message Encryptor and Verifier

39f8ca6

Both classes now have a rotate method where new instances are added for each call. When decryption or verification fails the next rotation instance is tried.

mjc-gh force-pushed the active-support-key-rotator branch from d76f4be to 39de46a Compare September 23, 2017 21:20

Add key rotation cookies middleware

8b0af54

Using the action_dispatch.cookies_rotations interface, key rotation is now possible with cookies. Thus the secret_key_base as well as salts, ciphers, and digests, can be rotated without expiring sessions.

mjc-gh force-pushed the active-support-key-rotator branch from 39de46a to 8b0af54 Compare September 24, 2017 16:25

kaspth merged commit 36888b9 into rails:master Sep 24, 2017

bogdanvlviv mentioned this pull request Sep 24, 2017

Fix links to RotationConfiguration class [ci skip] #30696

Closed

This was referenced Oct 10, 2017

Update security guide for signed cookie rotations #30847

Merged

Use SHA256 instead of SHA1 #25204

Closed

mjc-gh mentioned this pull request Oct 3, 2018

Add rotated_secret_key_bases credentials config #33632

Closed

p8 mentioned this pull request Jul 13, 2021

MessageEncryptor broke compatibility between 3.1 and 3.2+ #22195

Closed

jg23497 mentioned this pull request May 6, 2025

Default to SHA-256 for cookie signing #55017

Closed

4 tasks

Conversation

mjc-gh commented Jul 8, 2017

Summary

Other Information

Uh oh!

bdewater commented Jul 9, 2017

Uh oh!

kaspth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjc-gh commented Jul 9, 2017

Uh oh!

bdewater commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdewater commented Jul 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjc-gh Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspth commented Jul 17, 2017

Uh oh!

mjc-gh commented Jul 17, 2017

Uh oh!

mjc-gh commented Aug 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspth commented Sep 14, 2017

Uh oh!

mjc-gh commented Sep 14, 2017

Uh oh!

mjc-gh commented Sep 14, 2017

bdewater commented Jul 10, 2017 •

edited

Loading

mjc-gh Jul 10, 2017 •

edited

Loading

mjc-gh commented Sep 18, 2017 •

edited

Loading

mjc-gh commented Sep 24, 2017 •

edited

Loading

mjc-gh commented Sep 24, 2017 •

edited

Loading