Skip to content

Drop plaintext columns#1169

Merged
amoose merged 1 commit intomasterfrom
pek-drop-plaintext-columns
Apr 6, 2017
Merged

Drop plaintext columns#1169
amoose merged 1 commit intomasterfrom
pek-drop-plaintext-columns

Conversation

@pkarman
Copy link
Copy Markdown
Contributor

@pkarman pkarman commented Mar 3, 2017

Why: Encrypted attributes have now propagated through all AWS environments.

@pkarman pkarman self-assigned this Mar 3, 2017
@pkarman pkarman requested a review from monfresh March 3, 2017 22:41
Copy link
Copy Markdown
Contributor

@monfresh monfresh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but we can't merge this until we run a rake task on all the servers that encrypts the OTP secret from the existing plain text otp_secret_key

@monfresh
Copy link
Copy Markdown
Contributor

monfresh commented Mar 3, 2017

I believe this is what needs to be run:

User.where.not(otp_secret_key: nil).find_in_batches.with_index do |users, batch|
  users.each do |user|
    encrypted_attribute = EncryptedAttribute.new_from_decrypted(user.otp_secret_key)
    execute "UPDATE users SET encrypted_otp_secret_key='#{encrypted_attribute.encrypted}' WHERE id=#{user.id}"
  end
end

@pkarman Can you confirm?

@pkarman
Copy link
Copy Markdown
Contributor Author

pkarman commented Mar 3, 2017

that looks correct to me. Didn't realize that wasn't already done as part of the migration.

@pkarman
Copy link
Copy Markdown
Contributor Author

pkarman commented Mar 3, 2017

see #1170

@monfresh
Copy link
Copy Markdown
Contributor

monfresh commented Apr 1, 2017

I just remembered that we need to merge and deploy this before we launch. We can't have these plain text columns in the production database.

@pkarman
Copy link
Copy Markdown
Contributor Author

pkarman commented Apr 3, 2017

These plaintext columns will be ignored in any database. The code ignores them, so no data is read/written. We can launch w/o dropping them.

@monfresh
Copy link
Copy Markdown
Contributor

monfresh commented Apr 3, 2017

Agreed. It was a brain fart on my part. I confused the ability to see what's in the DB when all you have access to is the DB, with the ability to see the plaintext values when you have access to the Rails console. The DB will indeed not populate those columns, but if you have access to the Rails console, you can look up the decrypted values, which is expected.

@pkarman
Copy link
Copy Markdown
Contributor Author

pkarman commented Apr 3, 2017

the task has been run in qa and int and is irrelevant for the higher envs that have never had user data. I think this can be merged to master now. I will reconcile conflicts.

**Why**: Encrypted attributes have now propagated through all AWS environments.
@pkarman pkarman force-pushed the pek-drop-plaintext-columns branch from 7be9a48 to d4cc455 Compare April 3, 2017 15:18
Copy link
Copy Markdown
Contributor

@monfresh monfresh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Copy Markdown
Contributor

@amoose amoose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in QA

@amoose amoose merged commit 85b7636 into master Apr 6, 2017
@amoose amoose deleted the pek-drop-plaintext-columns branch April 6, 2017 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants