🔐 `Marketplace`: Encrypt `Order#delivery_address`! #1169

zspencer · 2023-03-02T20:33:25Z

So, I totally didn't think about how delivery addresses are PII and probably should not be stored in plaintext! Womp. Womp. Womp.

Now they ain't!

We'll want to delete the release:after_build bits after a prod deploy.

KellyAH · 2023-03-03T06:10:41Z

We'll want to delete the release:after_build bits after a prod deploy.

why? because that rake task is only temporarily needed to migrate the old prod data? And we don't need to keep it around because it won't ever be reused... right?

KellyAH · 2023-03-03T06:11:54Z

Aren't we also storing the contact person's email address too after every stripe order... is that PII (personal identifiable information) too that needs to be encrypted?

KellyAH · 2023-03-03T06:21:48Z

db/migrate/20230302202459_marketplace_encrypt_order_delivery_address.rb

+class MarketplaceEncryptOrderDeliveryAddress < ActiveRecord::Migration[7.0]
+  def change
+    rename_column :marketplace_orders, :delivery_address, :deprecated_delivery_address
+    add_column :marketplace_orders, :delivery_address_ciphertext, :text


lockbox gem says it's possible to encrypt the existing column here.... but maybe it's statement is deceptive. It's steps looks like you'd still need to create a new column for encrypted data. migrate the data, and drop the column of unencrypted data. 🙄

Ah, the migrating syntax is neat! I didn't realize it existed; but yea, that would save us the rename_column step and having to write the find_each!

It's also likely far safer in high-throughput cases; because you would still be able to access the delivery_address while it's being migrated; where as with this there is a moment in time where delivery_address is nil until the data migration completes.

I'll definitely do it next time!

KellyAH · 2023-03-03T06:43:21Z

lib/tasks/release.rake

+      next unless order.deprecated_delivery_address.present?
+
+      order.update(delivery_address: order.deprecated_delivery_address, deprecated_delivery_address: nil)
+    end


I see.. this is the prod data migration.....

So.. I assume this rake task runs BEFORE the delivery_address column is renamed in 20230302202459_marketplace_encrypt_order_delivery_address.rb

Ah, let me step ya through the Heroku deploy process:

Heroku checks out the git repository, and starts a build which creates a new image and installs gems, node packages, etc.

Heroku launches a release container using the new image, and runs the release command defined in the Procfile, which runs the rails db:migrate and release:after_build` tasks in sequence

Launches new web and worker containers using the new image.

Begins routing traffic to the new web containers

Stops routing traffic to the old web containers

Shuts down the old web and worker containers.

So the database has the new columns prior to running the rake task, and before deploying new web and worker containers. this means that the existing web and worker containers will still be running the code which references delivery_address, which could result in sql query errors. We could install strong_migrations to catch risky operations (like renames); but in this particular case I felt like it was safe enough given our traffic.

That said, there's no reason not to use the Lockbox.migrate strategy; which eliminates the risk and seems pretty dang cool!

Created issue #1177 to install strong_migrations.

My work team uses it and it is really handy.

KellyAH · 2023-03-03T06:58:46Z

app/furniture/marketplace/order.rb

@@ -11,7 +11,7 @@ class Order < Record
    has_many :ordered_products, inverse_of: :order, foreign_key: :cart_id
    has_many :products, through: :ordered_products, inverse_of: :orders

-    attribute :delivery_address, :string
+    has_encrypted :delivery_address


What's the order of operations for everything this code change does?

Seems like in this change...

:delivery_address is being renamed to deprecated_delivery_address

data in deprecated_delivery_address is migrated to delivery_address_ciphertext

data in deprecated_delivery_address is replaced with nil

And in later PR... I assume this happens?:
4. deprecated_delivery_address is dropped.
5. delivery_address_ciphertext is renamed delivery_address so has_encrypted :delivery_address in Order model still works.

Yes except step 5 never happens; because the Lockbox infers that delivery_address_ciphertext is the database name for the encrypted delivery_address attribute.

zspencer · 2023-03-03T20:08:07Z

We'll want to delete the release:after_build bits after a prod deploy.

why? because that rake task is only temporarily needed to migrate the old prod data? And we don't need to keep it around because it won't ever be reused... right?

Yep!

Aren't we also storing the contact person's email address too after every stripe order... is that PII (personal identifiable information) too that needs to be encrypted?

Good catch. This is part of why we have the AuthenticationMethod table, which should have encrypt the contact_location field and give it a blind_index. I don't think I want to bundle that into this PR; but I may wind up making a second one!

- #1136 - #831 So, I totally didn't think about how delivery addresses are PII and probably should not be stored in plaintext! Womp. Womp. Womp. Now they ain't! We'll want to delete the `release:after_build` bits after a prod deploy.

@KellyAH

@KellyAH found Lockbox's guide to migrating data: https://github.com/zinc-collective/convene/pull/1169/files#r1124067175 And it's way better!

zspencer · 2023-03-03T20:14:43Z

@KellyAH - I swapped to Lockbox.migrate since it was pretty straightforward, and gives us an example of doing it "the safer way" for future.

- #831 - #1136 Merge and deploy immediately after #1169

KellyAH

🚢

* 🔐 `Marketplace`: Step 2 of `Lockbox` migration - #831 - #1136 Merge and deploy immediately after #1169

zspencer added 🔐 security Relates to security vulnerabilities 🧹 refactor Includes non-behavioral changes labels Mar 2, 2023

zspencer assigned anaulin, KellyAH and daltonrpruitt and unassigned anaulin, KellyAH and daltonrpruitt Mar 2, 2023

zspencer requested review from a team, anaulin, daltonrpruitt and KellyAH March 2, 2023 20:35

zspencer force-pushed the marketplace/collect-delivery-address-on-cart branch from 267d6f8 to 4ef019d Compare March 2, 2023 20:38

zspencer changed the title ~~🔐 Marketplace: Encrypt delivery addresses!~~ 🔐 Marketplace: Encrypt Order#delivery_address! Mar 2, 2023

KellyAH reviewed Mar 3, 2023

View reviewed changes

zspencer added 2 commits March 3, 2023 12:09

🔐 Marketplace: Encrypt delivery addresses!

0727632

- #1136 - #831 So, I totally didn't think about how delivery addresses are PII and probably should not be stored in plaintext! Womp. Womp. Womp. Now they ain't! We'll want to delete the `release:after_build` bits after a prod deploy.

Marketplace: Use Lockbox.migrate for safety

2fdd8a3

@KellyAH found Lockbox's guide to migrating data: https://github.com/zinc-collective/convene/pull/1169/files#r1124067175 And it's way better!

zspencer force-pushed the marketplace/collect-delivery-address-on-cart branch from 4ef019d to 2fdd8a3 Compare March 3, 2023 20:13

zspencer requested a review from KellyAH March 3, 2023 20:14

zspencer added a commit that referenced this pull request Mar 3, 2023

🔐 Marketplace: Step 2 of Lockbox migration

68479ca

- #831 - #1136 Merge and deploy immediately after #1169

zspencer mentioned this pull request Mar 3, 2023

🔐 Marketplace: Step 2 of Lockbox migration #1175

Merged

KellyAH mentioned this pull request Mar 3, 2023

🛠️ Neighborhood: Install strong_migrations to catch risky operations #1177

Closed

KellyAH approved these changes Mar 3, 2023

View reviewed changes

zspencer merged commit aec3c30 into main Mar 3, 2023

zspencer deleted the marketplace/collect-delivery-address-on-cart branch March 3, 2023 20:31

zspencer added a commit that referenced this pull request Mar 3, 2023

🔐 Marketplace: Step 2 of Lockbox migration (#1175)

92ba5ed

* 🔐 `Marketplace`: Step 2 of `Lockbox` migration - #831 - #1136 Merge and deploy immediately after #1169

zspencer removed the 🧹 refactor Includes non-behavioral changes label Mar 3, 2023

zspencer added this to the 1.0 - Andromeda milestone May 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔐 `Marketplace`: Encrypt `Order#delivery_address`! #1169

🔐 `Marketplace`: Encrypt `Order#delivery_address`! #1169

zspencer commented Mar 2, 2023 •

edited

Loading

KellyAH commented Mar 3, 2023 •

edited

Loading

KellyAH commented Mar 3, 2023

KellyAH Mar 3, 2023 •

edited

Loading

zspencer Mar 3, 2023 •

edited

Loading

KellyAH Mar 3, 2023

zspencer Mar 3, 2023

KellyAH Mar 3, 2023 •

edited

Loading

zspencer Mar 3, 2023

KellyAH Mar 3, 2023 •

edited

Loading

zspencer Mar 3, 2023

zspencer commented Mar 3, 2023 •

edited

Loading

zspencer commented Mar 3, 2023

KellyAH left a comment

🔐 Marketplace: Encrypt Order#delivery_address! #1169

🔐 Marketplace: Encrypt Order#delivery_address! #1169

Conversation

zspencer commented Mar 2, 2023 • edited Loading

KellyAH commented Mar 3, 2023 • edited Loading

KellyAH commented Mar 3, 2023

KellyAH Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

zspencer Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

KellyAH Mar 3, 2023

Choose a reason for hiding this comment

zspencer Mar 3, 2023

Choose a reason for hiding this comment

KellyAH Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

zspencer Mar 3, 2023

Choose a reason for hiding this comment

KellyAH Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

zspencer Mar 3, 2023

Choose a reason for hiding this comment

zspencer commented Mar 3, 2023 • edited Loading

zspencer commented Mar 3, 2023

KellyAH left a comment

Choose a reason for hiding this comment

🔐 `Marketplace`: Encrypt `Order#delivery_address`! #1169

🔐 `Marketplace`: Encrypt `Order#delivery_address`! #1169

zspencer commented Mar 2, 2023 •

edited

Loading

KellyAH commented Mar 3, 2023 •

edited

Loading

KellyAH Mar 3, 2023 •

edited

Loading

zspencer Mar 3, 2023 •

edited

Loading

KellyAH Mar 3, 2023 •

edited

Loading

KellyAH Mar 3, 2023 •

edited

Loading

zspencer commented Mar 3, 2023 •

edited

Loading