Encrypt PII at rest #498

pkarman · 2016-09-23T15:24:12Z

Why: All PII must be encrypted at rest.
Storing attributes in a single JSON blob makes this much easier.

How:

The encryption architecture is similar to a hotel. Each user gets their
own room. The hotel management has a master key to every room.
Inside each room is a safe, where the PII itself is stored,
and only the user has the combination to the safe. If the user loses
their room key, the management can replace it. If the user loses
the safe combination, the safe can never be opened.

All PII is encrypted at rest in 2 layers: once with a unique
private key encrypted with the user's own password, and once
with a server-wide private key. The user's private key
is enclosed within the server-wide-key encrypted payload.

Encryption happens at the time a Profile is saved. Each time the
user logs in, the PII is decrypted using the password the user
provides. The PII is then re-encrypted within the session, using
the server-wide private key (just one layer of encryption).
The PII can be decrypted as-needed during the session (as for
SAML responses and displaying profile information to the user)
using the server-wide key.

NOTE: This PR does not include the following features, which
will be implemented in a follow-on PR:

re-encrypt PII when password changed
mark profile.active=false when password reset
encrypt IdV session at rest

pkarman · 2016-09-27T20:35:46Z

app/services/pii/encryptor.rb

+    DELIMITER = '.'.freeze
+    DIGEST = OpenSSL::Digest::SHA256.new.freeze
+
+    # structure of the encrypted payload:


here's the encryption description

pkarman · 2016-09-30T02:53:07Z

@paultag @alex @bandrzej would love your 👀 on this one as well, especially app/services/pii/encryptor.rb

jessieay

Hello! Left some comments. Feel free to ignore any that do not make sense, I am new to this proj so trying to pick up the team practices and not sure what is a project-wide style or specific to this PR

jessieay · 2016-10-03T17:46:21Z

app/controllers/idv/review_controller.rb

@@ -9,12 +10,18 @@ def confirm_idv_steps_complete
      redirect_to idv_phone_path unless idv_phone_complete?
    end

+    def confirm_current_password
+      unless current_user.valid_password?(password)
+        flash[:error] = t('idv.errors.incorrect_password')


yess loving the use of I18n in this proj

jessieay · 2016-10-03T17:47:34Z

app/controllers/idv/review_controller.rb

@@ -9,12 +10,18 @@ def confirm_idv_steps_complete
      redirect_to idv_phone_path unless idv_phone_complete?
    end

+    def confirm_current_password
+      unless current_user.valid_password?(password)


as you know, I prefer if over unless in general.

If we could make this if current_user.invalid_password?(password) I think it would be easier to understand.

jessieay · 2016-10-03T17:50:02Z

app/controllers/idv/review_controller.rb

+    def password
+      params.require(:user)[:password]
+    rescue ActionController::ParameterMissing
+      ''


would it make sense to have some sort of error msg here?

the error message will be generated by the User model when attempting to validate the password. I.e. a missing password is treated like an empty string because nil is not allowed as an argument to valid_password?.

jessieay · 2016-10-03T17:51:42Z

app/forms/idv/profile_form.rb

    def ssn_is_unique
-      if Profile.where.not(user_id: @user.id).where(ssn: ssn).any?
+      if Profile.where.not(user_id: @user.id).where(ssn_signature: ssn_signature).any?


It may may sense to throw this line into its own method for understandability. Perhaps something like

def ssn_is_duplicate? Profile.where.not(user_id: @user.id).where(ssn_signature: ssn_signature).any? end

jessieay · 2016-10-03T17:53:23Z

app/models/profile.rb

+    @_plain_pii ||= Pii::Attributes.new
+  end
+
+  def method_missing(method_sym, *arguments, &block)


this in general makes me a tiny bit scared. what is the reason for it in this context?

Mostly backwards compatibility. E.g.

profile.ssn = '1234'

is more intuitive and used extensively already, rather than

profile.plain_pii[:ssn] = '1234'

jessieay · 2016-10-03T18:01:21Z

app/views/profile/index.html.slim

- if current_user.active_profile
-  - p = current_user.active_profile
+- if @active_profile
+  - p = @active_profile


interesting - why are we assigning p here rather than just using the ivar within this file?

I believe that was a designer shorthand, just to save keystrokes. Also, as is evident from this proposed change, the origin value of p might change, but its usage does not.

Shorthand should be removed - explicit objects please for self-documenting code.

jessieay · 2016-10-03T18:04:24Z

db/migrate/20160920144053_change_profile_to_json.rb

+        dob: profile.dob.to_s,
+        phone: profile.phone
+      }
+      profile.update!(encrypted_pii: pii.to_json, active: false)


Just to make sure -- defining Profile in here will ensure that we don't run into problems with failing AR validations later on, right? (if, for instance, this migration is run at a time when there are new AR validations but that column hasn't actually been added to the DB yet, so AR data updates will fail)

jessieay · 2016-10-03T18:16:16Z

spec/models/profile_spec.rb

@@ -4,11 +4,76 @@
  let(:user) { create(:user, :signed_up) }
  let(:another_user) { create(:user, :signed_up) }
  let(:profile) { create(:profile, user: user) }
-
-  subject { profile }
+  let(:pii) do


it looks like this is only being used in some tests. What is this project's philosophy on usage of let and before statements? I generally avoid them altogether but can see how they are helpful when there is setup that is shared for all specs in a file. Do we still use them if the setup is not shared?

let and before are used extensively on this project.

Typically, every context in a rspec has a before block that creates the described context.

They are indeed used a lot, but that doesn't mean we have to keep doing that. I've stopped using them, and I don't like before blocks when there is only one it block.

jessieay · 2016-10-03T18:18:37Z

spec/models/user_spec.rb


    before do
-      create(:profile, :active, :verified, first_name: 'Jane', user: user)
-      create(:profile, :verified, first_name: 'Susan', user: user)
+      profile1.save!


It looks like this is only needed because we want to make sure these records are created for the test. If that is the case, why not inline these let statements and avoid let and before in this context altogether? Seems like the use of let is adding unnecessary complexity for this one spec

jessieay · 2016-10-03T18:19:13Z

spec/services/pii/attributes_spec.rb

+describe Pii::Attributes do
+  describe '#new_from_hash' do
+    it 'initializes from plain Hash' do
+      pii = described_class.new_from_hash(first_name: 'Jane')


are we using described_class in this test suite?

I have been trying to use it to make it easier to rename classes, esp during early feature development.

alex · 2016-10-03T19:42:07Z

app/services/pii/encryptor.rb

+      plaintext_signature = sign(plaintext, private_key)
+      payload = join_segments(plaintext, plaintext_signature)
+      cek = cipher.random_key
+      join_segments(private_key.public_encrypt(cek), encrypt_payload(payload, cek))


public_encrypt uses PKCS1v1.5 mode padding (per https://ruby-doc.org/stdlib-2.0.0/libdoc/openssl/rdoc/OpenSSL/PKey/RSA.html#method-i-public_encrypt), PKCS1v1.5 should be avoided in new applications, and OAEP should be used instead (I'm not sure the API for this in Ruby offhand)

thanks @alex. Looks like the relevant constant is OpenSSL::PKey::RSA::PKCS1_OAEP_PADDING based on what I see in https://github.com/LaunchKey/launchkey-ruby/blob/master/lib/launchkey/rsa_key.rb

alex · 2016-10-03T19:44:11Z

app/services/pii/encryptor.rb

+    end
+
+    def sign(text, private_key = key_maker.server_key)
+      encode(private_key.sign(DIGEST, encode(text)))


The Ruby docs don't say, but I'm pretty sure sign uses PKCS1v1.5 mode padding, which should be avoided in enw applications, and PSS (Probabilistic Signature Scheme) used isntead.

https://github.com/jondistad/openssl_rsa_pss_verify makes me think that the ruby openssl bindings don't yet support PSS. I'll keep looking.

ruby/openssl#75

alex · 2016-10-03T19:44:41Z

I only skimmed the raw crypto, I haven't reviewed the overall design in any real detail.

alex · 2016-10-03T19:56:29Z

Sounds plausible!

On Mon, Oct 3, 2016 at 3:55 PM, Peter Karman notifications@github.com
wrote:

@pkarman commented on this pull request.

In app/services/pii/encryptor.rb
#498:
end
def decrypt(ciphertext, password)
 user_payload = decrypt_with_key(ciphertext, key_maker.server_key)
 return unless user_payload
 user_private_key_pem, user_ciphertext = split_into_segments(user_payload)
 user_private_key = rsa_key(user_private_key_pem, password)
 decrypt_with_key(user_ciphertext, user_private_key)
end
DHS/NIST algorithm

def encrypt_with_key(plaintext, private_key)
 plaintext_signature = sign(plaintext, private_key)
 payload = join_segments(plaintext, plaintext_signature)
 cek = cipher.random_key
 join_segments(private_key.public_encrypt(cek), encrypt_payload(payload, cek))
thanks @alex https://github.com/alex. Looks like the relevant constant
is OpenSSL::PKey::RSA::PKCS1_OAEP_PADDING based on what I see in
https://github.com/LaunchKey/launchkey-ruby/blob/master/
lib/launchkey/rsa_key.rb

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#498, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAADBDOZjmscnWRYAZyQyuUAK_kiTfdoks5qwV3HgaJpZM4KFGRq
.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: D1B3 ADC0 E023 8CA6

alex · 2016-10-04T14:48:34Z

(For your awareness, I intend to do a more complete review of this tomorrow)

monfresh · 2016-10-05T02:10:52Z

app/controllers/concerns/idv_session.rb

+    idv_session.resolution = resolution
+    idv_session.question_number = 0
+    idv_session.profile_from_applicant(idv_session.applicant, password)
+  end


Is this related to this PR? If not, move to a separate PR?

It's related in the sense that reek complained of too many methods in the controller, so a slight refactor was necessary to get codeclimate passing.

monfresh · 2016-10-05T02:13:37Z

app/controllers/idv/review_controller.rb

-      idv_session.question_number = 0
-      idv_session.profile_from_applicant(idv_session.applicant)
+    def invalid_password?
+      !current_user.valid_password?(password)
    end
  end
 end


These changes here don't seem to be related to encrypting PII. Do you mind removing the changes that are not specific to encryption to make this PR smaller and easier to review?

see comment above about reek.

IIUC, by adding the password confirmation feature to this controller, it made the controller too big for reek, so you had to move some methods into concerns/idv_session.rb and services/idv/session.rb right? My question is about this confirm_current_password feature. It doesn't seem related to PII encryption, but I could be wrong. If it's not related, then several changes related/caused by this feature could be moved to a different PR.

Since the user's password is used in order to encrypt their PII, we must collect that password at the moment we intend to encrypt it. Likewise, we use their password at log-in time to decrypt.

Ah, that makes sense.

**Why**: Requires less space and we are already one-way hashing.

pkarman · 2016-10-22T14:02:40Z

I am splitting this PR into 3 different PRs to make reviewing easier.

#598 is a small refactor to satisfy codeclimate
#599 is the model change and UX change to collect passphrase during IdV

This PR will, once the other 2 are merged, reflect only the encryption code itself, as most of the comment threads here are about the encryption model and implementation.

pkarman · 2016-10-24T14:09:48Z

@alex OpenSSL verify() implemented in fe3e3e2

@alex @monfresh aes-256-gcm implemented in bfa7704

pkarman · 2016-10-24T14:41:30Z

I am aware of test failures and merge conflict. I will fix these once #599 is merged.

pkarman · 2016-10-26T18:00:23Z

Will create a new PR once encryption model is finalized.

pkarman added status - ready for review status - do not merge labels Sep 23, 2016

pkarman added this to the Sprint 15 milestone Sep 23, 2016

pkarman self-assigned this Sep 23, 2016

pkarman mentioned this pull request Sep 23, 2016

Store PII as JSON #491

Closed

pkarman force-pushed the encrypt-pii branch 6 times, most recently from f5f73d3 to 9db84d1 Compare September 23, 2016 21:40

pkarman removed the status - do not merge label Sep 23, 2016

pkarman force-pushed the encrypt-pii branch from 9db84d1 to 6ccc85c Compare September 23, 2016 22:02

pkarman commented Sep 27, 2016

View reviewed changes

pkarman force-pushed the encrypt-pii branch 2 times, most recently from 0465b3c to e205d20 Compare September 29, 2016 16:39

pkarman mentioned this pull request Sep 29, 2016

Encrypt session at rest #516

Closed

pkarman force-pushed the encrypt-pii branch from e205d20 to ceb053b Compare September 29, 2016 18:45

This was referenced Sep 29, 2016

Deactivate profile on pw reset #517

Closed

[WIP] Reencrypt pii on pw change #518

Closed

jessieay reviewed Oct 3, 2016

View reviewed changes

alex reviewed Oct 3, 2016

View reviewed changes

pkarman force-pushed the encrypt-pii branch from e1d8025 to 82eb091 Compare October 3, 2016 21:53

monfresh reviewed Oct 5, 2016

View reviewed changes

monfresh suggested changes Oct 5, 2016

View reviewed changes

Peter Karman added 16 commits October 22, 2016 08:01

Store SSN signature digest only

bc2bc4c

**Why**: Requires less space and we are already one-way hashing.

review responses

5b99812

codeclimate fixes

85c9318

use OpenSSL::PKey::RSA::PKCS1_OAEP_PADDING for encrypt/decrypt

be32a02

Refactor Profile into Pii::Attributes and Idv::Applicant

6d6e8ad

Idv::Applicant tests

a19ad27

a riddle, wrapped in a mystery, inside an enigma

151e04c

add comment about Digital Envelope Encryption Model

2be48d9

remove missing password parameter check

8ff0f07

Tests specifically for Digital Envelope Encryption Model (DEEM)

2f1c232

codeclimate fix

a92c8b4

zap duplicate method introduced in bad merge conflict

ce12f27

new encryption algorithm with no stored AES CEK

d1ed019

random(?) file load order can result in spurious superclass error

8a0a40c

fix double negative (hard to read)

3b767f0

replace RSAEncryptor with EnvelopeEncryptor in prep for KMS

684b033

pkarman force-pushed the encrypt-pii branch from 7160402 to 684b033 Compare October 22, 2016 13:01

This was referenced Oct 22, 2016

Convert profile attributes to JSON #599

Merged

Refactor IdV controllers to DRY up some methods #598

Merged

pkarman added the status - do not merge label Oct 22, 2016

Peter Karman added 2 commits October 22, 2016 16:37

use OpenSSL verify API for signatures

fe3e3e2

Refactor Pii::Cipher to use aes-256-gcm

bfa7704

pkarman force-pushed the encrypt-pii branch from 0232267 to bfa7704 Compare October 22, 2016 21:59

pkarman closed this Oct 26, 2016

pkarman mentioned this pull request Nov 3, 2016

Implement NIST encryption model #650

Merged

pkarman deleted the encrypt-pii branch November 14, 2016 19:57

Encrypt PII at rest #498

Encrypt PII at rest #498

Conversation

pkarman commented Sep 23, 2016 • edited

Choose a reason for hiding this comment

pkarman commented Sep 30, 2016

jessieay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessieay Oct 3, 2016 • edited

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

pkarman Oct 3, 2016 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex commented Oct 3, 2016

alex commented Oct 3, 2016

@pkarman commented on this pull request.

DHS/NIST algorithm

alex commented Oct 4, 2016

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

pkarman commented Oct 22, 2016

pkarman commented Oct 24, 2016

pkarman commented Oct 24, 2016

pkarman commented Oct 26, 2016

pkarman commented Sep 23, 2016 •

edited

jessieay Oct 3, 2016 •

edited

pkarman Oct 3, 2016 •

edited