-
-
Notifications
You must be signed in to change notification settings - Fork 583
#5609 Encrypt sensitive Personally Identifiable Information (PII) #5626
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
eugeniojimenes
wants to merge
2
commits into
rubyforgood:main
Choose a base branch
from
eugeniojimenes:5609-encrypt-sensitive-pii
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| # Active Record encryption keys for local development. | ||
| # | ||
| # These are throwaway, non-production keys, committed on purpose so that a fresh clone boots | ||
| # without any setup. Production and staging read the same variables from the real environment. | ||
| AR_ENCRYPTION_PRIMARY_KEY=WSUDLoAAbKSnldNQe4YXVoC5WTdBiiFt | ||
| AR_ENCRYPTION_KEY_DERIVATION_SALT=6dlX6mQqooFztaeIOxOBHJ9SmynoWaXC |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| # Active Record encryption keys for the test suite. | ||
| # | ||
| # These are throwaway, non-production keys, committed on purpose: CI has no .env file, and dotenv | ||
| # does not load .env.development in the test environment. | ||
| AR_ENCRYPTION_PRIMARY_KEY=3WdNnb4TBItgQxzJd5CUdk2g2v03UvzZ | ||
| AR_ENCRYPTION_KEY_DERIVATION_SALT=SggkroBqG4UVZINKNAaURapc6PREICJ5 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # Keys come from the environment everywhere, so there is no env-specific branch here. Development | ||
| # and test read them from the committed .env.development and .env.test, which dotenv loads before | ||
| # initializers run. Production and staging get them from the real environment, and ENV.fetch has no | ||
| # default, so a missing key fails the boot loudly instead of silently writing data that cannot be | ||
| # read back later. | ||
| ActiveRecord::Encryption.configure( | ||
| primary_key: ENV.fetch("AR_ENCRYPTION_PRIMARY_KEY"), | ||
| key_derivation_salt: ENV.fetch("AR_ENCRYPTION_KEY_DERIVATION_SALT"), | ||
|
|
||
| # Bridge for migrating existing plaintext rows: lets reads of not-yet-backfilled | ||
| # columns work instead of raising. Flip to false (follow-up) after prod backfill. | ||
| support_unencrypted_data: true | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| class EncryptDatesOfBirth < ActiveRecord::Migration[8.0] | ||
| def up | ||
| # date -> text: encrypted values are string blobs and won't fit a date column. | ||
| # | ||
| # `using` pins the serialization format. Without it Postgres falls back to the date output | ||
| # function, which follows the server's DateStyle setting: under `SQL, MDY` the same date is | ||
| # written as "03/04/1990", which Ruby then reads back as April 3rd. This rewrite is in place | ||
| # and irreversible, so the format cannot be left to a server setting. | ||
| safety_assured do | ||
| change_column :children, :date_of_birth, :text, using: "to_char(date_of_birth, 'YYYY-MM-DD')" | ||
| change_column :authorized_family_members, :date_of_birth, :text, using: "to_char(date_of_birth, 'YYYY-MM-DD')" | ||
| end | ||
| end | ||
|
|
||
| def down | ||
| safety_assured do | ||
| change_column :children, :date_of_birth, :date, using: "date_of_birth::date" | ||
| change_column :authorized_family_members, :date_of_birth, :date, using: "date_of_birth::date" | ||
| end | ||
| end | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| class BackfillEncryptedPii < ActiveRecord::Migration[8.0] | ||
| # `encrypts` only encrypts new writes, so existing rows stay plaintext until re-saved. | ||
| # This re-encrypts them in place. `record.encrypt` uses update_columns (no validations, | ||
| # callbacks, timestamps or paper_trail), and relies on | ||
| # config.active_record.encryption.support_unencrypted_data = true to read the plaintext. | ||
| # | ||
| # No wrapping transaction: each row commits on its own, so a big table doesn't hold one long | ||
| # transaction/lock, and a run that fails halfway keeps the rows it already encrypted. | ||
| disable_ddl_transaction! | ||
|
|
||
| MODELS = [ | ||
| Partners::Child, | ||
| Partners::AuthorizedFamilyMember, | ||
| Partners::Family, | ||
| Partners::Profile, | ||
| ProductDriveParticipant | ||
| ].freeze | ||
|
|
||
| def up | ||
| MODELS.each do |model| | ||
| model.find_in_batches do |batch| | ||
| batch.reject { |record| encrypted?(record) }.each(&:encrypt) | ||
| sleep(0.1) # throttle prod DB load (~40-50k rows); matches BackfillItemReportingCategoryField | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def down | ||
| raise ActiveRecord::IrreversibleMigration | ||
| end | ||
|
|
||
| private | ||
|
|
||
| # `encrypt` writes unconditionally, and the encryption is non-deterministic, so re-encrypting an | ||
| # already-encrypted row rewrites it with fresh ciphertext: same value, wasted write. Skipping | ||
| # those rows is what lets a run that failed halfway be re-run and only do what is left. | ||
| # | ||
| # Ciphertext is opaque to Postgres, so there is no scope for this: the check is per record. | ||
| # A blank value counts as done, since there is nothing to encrypt. | ||
| def encrypted?(record) | ||
| record.class.encrypted_attributes.all? do |attribute| | ||
| value = record.read_attribute_before_type_cast(attribute) | ||
| value.blank? || record.encrypted_attribute?(attribute) | ||
| end | ||
| end | ||
| end | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| require "rails_helper" | ||
| require Rails.root.join("db/migrate/20260703191552_backfill_encrypted_pii") | ||
|
|
||
| RSpec.describe BackfillEncryptedPii, type: :migration do | ||
| let(:connection) { ActiveRecord::Base.connection } | ||
| let(:migration) { described_class.new } | ||
|
|
||
| let(:family) { create(:partners_family) } | ||
| let(:child) { create(:partners_child, family: family) } | ||
| let(:participant) { create(:product_drive_participant) } | ||
| let(:profile) { create(:partner_profile) } | ||
|
|
||
| # Rows as production holds them: written before `encrypts` existed, so still plaintext. | ||
| # They have to be seeded with raw SQL, since the models would encrypt them on the way in. | ||
| def write_plaintext(record, values) | ||
| assignments = values.map { |column, value| "#{column} = #{connection.quote(value)}" }.join(", ") | ||
| connection.execute("UPDATE #{record.class.table_name} SET #{assignments} WHERE id = #{record.id}") | ||
| record.reload | ||
| end | ||
|
|
||
| def stored(record, column) | ||
| connection.select_value("SELECT #{column} FROM #{record.class.table_name} WHERE id = #{record.id}") | ||
| end | ||
|
|
||
| it "encrypts the rows that are still plaintext, keeping their values" do | ||
| write_plaintext(child, date_of_birth: "1990-03-04") | ||
| write_plaintext(family, guardian_phone: "555-1234") | ||
| write_plaintext(participant, phone: "555-9876", email: "donor@example.com") | ||
| write_plaintext(profile, primary_contact_email: "contact@example.com", pick_up_phone: "555-4444") | ||
|
|
||
| migration.up | ||
|
|
||
| expect(stored(child, :date_of_birth)).to start_with('{"p":') | ||
| expect(stored(family, :guardian_phone)).to start_with('{"p":') | ||
| expect(stored(participant, :email)).to start_with('{"p":') | ||
| expect(stored(profile, :primary_contact_email)).to start_with('{"p":') | ||
|
|
||
| expect(child.reload.date_of_birth).to eq(Date.new(1990, 3, 4)) | ||
| expect(family.reload.guardian_phone).to eq("555-1234") | ||
| expect(participant.reload.phone).to eq("555-9876") | ||
| expect(participant.reload.email).to eq("donor@example.com") | ||
| expect(profile.reload.primary_contact_email).to eq("contact@example.com") | ||
| expect(profile.pick_up_phone).to eq("555-4444") | ||
| end | ||
|
|
||
| it "leaves rows that are already encrypted untouched" do | ||
| # The factories write through the models, so these rows are encrypted already. Encryption is | ||
| # non-deterministic: if the migration re-encrypted them, the ciphertext would come out different. | ||
| ciphertext = stored(child, :date_of_birth) | ||
|
|
||
| expect { migration.up }.not_to change { stored(child, :date_of_birth) }.from(ciphertext) | ||
| end | ||
|
|
||
| it "picks up where a failed run stopped" do | ||
| encrypted_child = child | ||
| plaintext_child = create(:partners_child, family: family) | ||
| write_plaintext(plaintext_child, date_of_birth: "1985-12-31") | ||
| ciphertext = stored(encrypted_child, :date_of_birth) | ||
|
|
||
| migration.up | ||
|
|
||
| expect(stored(encrypted_child, :date_of_birth)).to eq(ciphertext) | ||
| expect(plaintext_child.reload.date_of_birth).to eq(Date.new(1985, 12, 31)) | ||
| end | ||
|
|
||
| it "leaves a missing value null" do | ||
| write_plaintext(child, date_of_birth: nil) | ||
|
|
||
| migration.up | ||
|
|
||
| expect(stored(child, :date_of_birth)).to be_nil | ||
| end | ||
| end |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we find only non-encrypted rows to handle the case where a run might fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, and my "idempotent" comment was wrong: idempotent in the value, not in the work.
record.encryptwrites unconditionally (it just callsupdate_columns), and our encryption is non-deterministic, so re-encrypting an already-encrypted row rewrites it with new ciphertext. Same value, wasted write. A failed run redid the whole table.There's no way to filter this in SQL. Ciphertext is opaque to Postgres, and Rails has no scope for it (
extend_queriesonly helps deterministic attributes, and ours are all non-deterministic). Matching the payload prefix withNOT LIKE '{"p":%'would work, but it hardcodes Rails' internal JSON format into a migration.So I used the public API instead, skipping the row before the write:
encrypted?usesrecord.encrypted_attribute?, and treats a blank value as done. We still read every row, which is one sequential scan, but we only write the ones that need it. That's the expensive part (WAL, replication), so a re-run after a failure only does what's left.Covered in
spec/migrations/backfill_encrypted_pii_spec.rb, including already-encrypted rows staying byte for byte unchanged.