Convert profile attributes to JSON #599

pkarman · 2016-10-22T13:59:24Z

This is part 2 of #498

Please review and merge #598 first to reduce the changes here.

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

How:

This change does not implement any actual encryption. That will follow
in a separate change.

This change includes:

schema modifications
user flow for collecting passphrase at IdV review
internal flow for decrypt PII at login for use in session

pkarman · 2016-10-22T14:10:01Z

app/services/pii/encryption_error.rb

@@ -0,0 +1,4 @@
+module Pii


this class will be exercised once #498 is complete

pkarman · 2016-10-22T14:10:42Z

app/services/pii/encryptor.rb

@@ -0,0 +1,21 @@
+module Pii


yes, this is not encryption per se. Just Base64 encoding. The class name is a placeholder for the concept of encrypting. Actual encryption will happen in #498

pkarman · 2016-10-22T14:12:24Z

spec/lib/feature_management_spec.rb

+        expect(FeatureManagement.use_kms?).to eq(true)
+      end
+
+      xit 'throws exception when attempting to fetch PII signing key' do


KeyMaker class is not present in this PR but will be in #498

monfresh · 2016-10-22T19:04:30Z

598 has been merged. Wanna rebase?

pkarman · 2016-10-22T21:06:20Z

thanks @monfresh -- rebased

pkarman · 2016-10-24T14:31:19Z

@monfresh @jessieay you reviewed this when it was part of #498. Please take another look. I addressed all the comments you left on that other PR.

#498 (comment)
#498 (comment)

zachmargolis · 2016-10-24T14:00:07Z

app/services/attribute_asserter.rb

    self.user = user
    self.service_provider = service_provider
    self.authn_request = authn_request
+    @decrypted_pii = decrypted_pii


I noticed in many other places we prefix instance variables with _, so this would be @_decrypted_pii -- should we be doing this here?

I have used the leading underscore for memoized private instance variables that should not be accessed directly anywhere else in the code except for via the memoized method. That doesn't apply here, since we do want to use the instance var elsewhere. I can make it a private accessor if that would be clearer.

Private accessor sounds good to me, I didn't see the difference between memoized instance variables vs otherwise

done in 51dd5df

Should we convert the other initialized variables into instance variables for consistency?

zachmargolis · 2016-10-24T14:32:48Z

app/controllers/idv/review_controller.rb

@@ -68,5 +76,13 @@ def submit_applicant
      idv_attempter.increment
      resolution
    end
+
+    def password
+      params.require(:user)[:password]


Should this be either .require(:password)[:password] or .require(user: :password)[:user][:password] ?

I feel like I tried a half dozen different combinations to get what I wanted, which is:

do not raise an exception if the param is missing

do return nil if the param is missing

We want the error to come from the valid_password? method, not the controller param fetcher.

Ok, I think I understand.

I think we want:

params.fetch(:user, {})[:password].presence

The methods from strong parameters .require and .permit are only useful if we're passing entire hashes around (ex User.create(params)), but in this case, we just want a single key, so I think we can avoid that approach and be explicit.

zachmargolis · 2016-10-24T14:49:50Z

app/models/profile.rb

+    attr_name_sym = method_sym.to_s.gsub(/=\z/, '').to_sym
+    if plain_pii.members.include?(attr_name_sym)
+      return plain_pii[attr_name_sym] if arguments.empty?
+      plain_pii[attr_name_sym] = arguments.first


Since this is imitating writing an attribute, should we do anything to mark the record as dirty?

good question.

PII must always be encrypted before saving, and because encryption requires the user passphrase, we can't make it a before_save hook or equivalent. So the only attribute that really should be marked as dirty is encrypted_pii and marking it dirty would be technically incorrect until after encrypt_pii is explicitly called.

I'm starting to think that the "convenience magic" of the method_missing stuff maybe isn't worth the complexity it creates. These two do the same thing:

profile.plain_pii[:ssn] = '1234' profile.ssn = '1234'

and while I personally love the terse-ness and error checking of the 2nd form, the non-magical-ness of the 1st form might be friendlier to the code maintainer. Thoughts?

I am very wary of method_missing and believe we should minimize Ruby dark magic, big fan of profile.plain_pii[:ssn] = '1234'!

How would you feel about this instead of method_missing etc:

Pii::Attributes.new.members.each do |attr| delegate attr, to: :plain_pii setter = "#{attr}=".to_sym delegate setter, to: :plain_pii end

Again, I'd be really happy with just writing profile.plain_pii.foobar everywhere since it makes for a really clear relation, but it may be easier to refactor the code with this delegation layer? Would love to get other folk's opinions.

(Also teeny nit, structs have a class-level .members so you could do Pii::Attributes.members without the .new)

pkarman · 2016-10-24T18:26:30Z

@zachmargolis thanks for the review. updates in 51dd5df

pkarman · 2016-10-24T18:46:00Z

per the codeclimate warning, I tried changing this:

if evaluator.pii
  evaluator.pii.each { |key, val| profile.plain_pii[key] = val }
end

to this:

evaluator.pii&.each { |key, val| profile.plain_pii[key] = val }

and got an error about "can't call each on a False::false value"

I'm in favor of disabling that rubocop rule. I've never seen that safe navigation syntax before.

UPDATE: I worked around the rule.

zachmargolis · 2016-10-25T13:25:47Z

app/models/profile.rb

-  def verified?
-    verified_at.present?
+  def plain_pii
+    @_plain_pii ||= Pii::Attributes.new


Let's address this later, but wanted to comment before I forget:

I'm not sure we should memoize this method in this way -- I think that this would allow pretending to read PII attributes without actually decrypting them. IMO if a profile has PII but it has not been decrypted, this should be nil or something so we get errors when try to access fields.

that's a good point. I am not happy with how the PII encryption is neither automatic in a hook, nor clearly a manual process.

We could get rid of plain_pii altogether to remove confusion. E.g.

profile = Profile.new( user: user ) pii_attrs = Pii::Attributes.new_from_hash( ssn: '1234' ) profile.encrypt_pii(passphrase, pii_attrs) profile.save! pii_decrypted = profile.decrypt_pii(passphrase) puts pii_decrypted == pii_attrs # -> true

You would just never handle un-encrypted PII aside from an explicit Pii::Attributes instance. The Profile only ever holds it encrypted.

That would make the tests a little more verbose but at least there would not be unexpected behaviour.

Thoughts?

@zachmargolis I have removed plain_pii method in d17b0a7

Yup, makes sense!

I would even be in favor of something like this (very verbose and separate but very clear)

profile = Profile.new( user: user ) pii_attrs = Pii::Attributes.new_from_hash( ssn: '1234' ) profile.encrypted_pii = pii_attrs.encrypt(passphrase, pii_attrs) profile.save! pii_decrypted = Pii::Attributes.decrypt(passphrase, profile.encrypted_pii)

Profile.encrypt_pii also fingerprints the ssn, which must be done in sync with whatever PII is being encrypted. So having a method to handle all the encryption-related stuff seems ok to me.

monfresh · 2016-10-25T19:51:49Z

app/controllers/profile_controller.rb

@@ -3,5 +3,7 @@ class ProfileController < ApplicationController
  layout 'card_wide'

  def index
+    cacher = Pii::Cacher.new(current_user, user_session)
+    @active_profile = cacher.fetch


Above, in saml_idp_auth_concern, you are doing the same thing, but the method is called decrypted_pii, whereas here you are saying the result represents an active profile. IIUC cacher.fetch returns a hash of attributes, so it seems like decrypted_pii is the correct term, and perhaps the instance variable here should be called that as well?

monfresh · 2016-10-25T20:38:11Z

app/services/idv/applicant.rb

+    def build_profile(applicant, user, password)
+      Profile.create_with_encrypted_pii(user, pii_from_applicant(applicant), password)
+    end
+  end


Reverse the order of these 2 methods so that they read from top-down?

monfresh · 2016-10-25T20:54:36Z

app/services/idv/applicant.rb

+
+    def build_profile(applicant, user, password)
+      Profile.create_with_encrypted_pii(user, pii_from_applicant(applicant), password)
+    end


build_profile implies calling new and not saving, but then create_with_encrypted_pii is called, which does save to the database. Perhaps this class should be renamed to ProfileFromApplicant and the build_profile method should be renamed to create, with the contents of the create_with_encrypted_pii from the Profile class? This will keep both the Profile class clean and keep the profile creation in one place.

pkarman · 2016-10-25T21:42:47Z

thanks @monfresh -- hit all your review feedback in 27fc889

monfresh · 2016-10-25T21:45:01Z

app/services/pii/cacher.rb

+      pii_json = decrypted_pii.to_json
+      encrypted_pii = encryptor.encrypt(pii_json)
+      user_session[:encrypted_pii] = encrypted_pii
+    end


I don't fully understand the purpose of this Cacher class. It seems like in order for the encrypted PII to be cached, the user must have an active profile. But if they already have an active profile, that means they already have encrypted PII in the DB, right? If so, why can't the encrypted PII be fetched directly from the DB without the session as a middleman?

From what I can tell, here are the sequence of events:

User with an active profile signs in

Their encrypted PII is fetched from the DB and stored in the session (after_action in users/sessions_controller)

Since their profile is already active, meaning they've already proofed, their PII is not going to be modified in this session

The encrypted PII is then fetched from the session and decrypted in 2 places: 1) the ProfileController in order to display to the user their verified attributes, 2) in SamlIdpController to assert attributes back to the SP

If this is correct, it looks like the caching is not necessary since the data is already available in the DB.

The PII may only be decrypted from the db with the user's passphrase. Since we only have the passphrase available to us at log in time, we must decrypt it then, and then store it re-encrypted w/o the passphrase for later use.

it's hard to see that in this PR because there is no actual encryption happening. All the crypto was yanked out to simplify the review process.

In a subsequent PR, the passphrase will actually be used for encryption, and there will be different encryption methods used for storage in the db vs storage in the session.

Ah, yes. I keep forgetting about that. I think some comments at the top of this class explaining why it's needed would be helpful.

monfresh · 2016-10-25T21:56:55Z

config/application.yml.example

@@ -86,6 +92,9 @@ production:
  participate_in_dap: 'false'
  password_pepper: 'rake secret'
  password_strength_enabled: 'true'
+  pii_passphrase: 'trust-but-verify'
+  pii_server_cek: 'rake secret 256 bits'
+  pii_signing_passphrase: 'trust-but-verify'


These 3 keys don't seem to be used in this PR. Move to another PR?

yup, that will go into a later PR. good catch. yanked in 61f6f79

jessieay

Oh shoot, reviewed this earlier but forgot to submit

jessieay · 2016-10-25T18:31:14Z

app/models/profile.rb

-      ssn: applicant.ssn,
-      phone: applicant.phone
-    )
+  def self.create_with_encrypted_pii(user, plain_pii, password)


thoughts on using named args here? I find them very helpful when dealing with more than 1 arg

jessieay · 2016-10-25T18:31:27Z

app/services/attribute_asserter.rb

@@ -15,10 +15,11 @@ class AttributeAsserter
    :phone
  ].freeze

-  def initialize(user, service_provider, authn_request)
+  def initialize(user, service_provider, authn_request, decrypted_pii)


named args?

jessieay · 2016-10-25T18:32:21Z

app/services/idv/session.rb

@@ -52,6 +55,8 @@ def complete_profile
      profile.verified_at = Time.zone.now
      profile.vendor = vendor
      profile.activate
+      # move from temp slot to 'permanent' slot
+      user_session[:encrypted_pii] = session.delete(:encrypted_pii)


put this in a method named move_from_temp_to_permanent instead of having comment?

jessieay · 2016-10-25T18:33:16Z

app/services/pii/attributes.rb

@@ -0,0 +1,32 @@
+module Pii
+  Attributes = Struct.new(
+    :first_name, :middle_name, :last_name,


rubocop might freak out if we do this, but I find lists like this easier to read when each arg is on its own line.

agreed on both counts. I dislike fighting with rubocop more than I dislike grouped args.

jessieay · 2016-10-25T18:34:19Z

app/services/pii/cacher.rb

+      decrypted_pii = profile.decrypt_pii(password)
+      pii_json = decrypted_pii.to_json
+      encrypted_pii = encryptor.encrypt(pii_json)
+      user_session[:encrypted_pii] = encrypted_pii


it seems like this method is doing more than save -- would it make sense to put parts of this in other (mayve private) methods so it's clearer what is happening?

jessieay · 2016-10-25T18:35:22Z

app/views/idv/sessions/new.html.slim

@@ -28,7 +28,7 @@ h2.pb0.heading = t('idv.titles.session.basic')
    .mt0.mb3.pb-tiny.border-bottom.border-teal = t('profile.index.dob')
    = f.input :dob, label: t('idv.form.dob'), required: true,
      pattern: '(0[1-9]|1[012])/(0[1-9]|1[0-9]|2[0-9]|3[01])/[0-9]{4}',
-      input_html: { class: 'dob', value: idv_profile_form.dob.try(:strftime, '%m/%d/%Y'),
+      input_html: { class: 'dob', value: idv_profile_form.dob,


why no more strftime ?

because dob is no longer a DateTime object. It is a String.

pkarman · 2016-10-25T22:57:19Z

thanks @jessieay -- feedback in dcf10d8

pkarman · 2016-10-25T23:25:51Z

@jessieay named args change in 1c88161

note that reek has a TooManyMethods rule so refactoring to increase number of methods can sometimes hit that rule.

pkarman · 2016-10-26T00:19:32Z

@jessieay @monfresh thanks for your comments. They weren't in the form of an official GH "review" so I am not sure if you approve yet of this PR. Please comment/review as appropriate. Thanks.

jessieay

Left some smaller comments but nothing that blocks PR approval. I will hold off on merging to give @monfresh a chance to re-look. 🌵

jessieay · 2016-10-26T00:24:10Z

app/controllers/idv/review_controller.rb

@@ -68,5 +76,13 @@ def submit_applicant
      idv_attempter.increment
      resolution
    end
+
+    def password


super minor detail, but I usually put methods in order of dependencies (so password below where it is used, which here is valid_password?). I find this makes diffs more readable because you read from top to bottom and see why each thing is used.

done in f6fffdb

jessieay · 2016-10-26T00:25:29Z

app/forms/idv/profile_form.rb


    def initialize(params, user)
      @user = user
-      profile.attributes = params.select { |key, _val| respond_to?(key.to_sym) }
+      params.each do |key, value|


I might consider putting this activity into a method rather than initialize -- seems like a lot of data parsing for calling .new All I usually expect to see in initialize is ivar assignment

done in f6fffdb

jessieay · 2016-10-26T00:34:46Z

app/services/attribute_asserter.rb

-    self.authn_request = authn_request
+  URI_PATTERN = Saml::Idp::Constants::REQUESTED_ATTRIBUTES_CLASSREF
+
+  def initialize(options)


ah so you'd rather use hash than ruby named args? I kind of prefer named args but not a big deal: https://robots.thoughtbot.com/ruby-2-keyword-arguments#keyword-arguments-vs-options-hash

I agree, am a big fan of named arguments, then we get bonuses like errors when there are extra/typoed arguments

Since we know we're on a recent version of Ruby and AFAIK are not planning on switching platforms (ex JRuby) I think there's a huge upside to named arguments

done in f6fffdb

jessieay · 2016-10-26T00:37:42Z

spec/controllers/idv/review_controller_spec.rb

+      allow(subject).to receive(:confirm_idv_steps_complete).and_return(true)
+      allow(subject).to receive(:confirm_idv_attempts_allowed).and_return(true)
+      idv_session.params = user_attrs.merge(phone_confirmed_at: Time.zone.now)
+      allow(subject).to receive(:idv_session).and_return(idv_session)


I think this is a good example of why before and let statements are tricky. When reading this diff, I have no idea what idv_session is, am just trusting it's some thing (factory? mock? stub? string?) above in the spec file ☘

If the test setup were all in each test, there'd be more duplication but I could see what was going on much more easily

happy to see a refactor of this, maybe in a separate PR?

monfresh · 2016-10-26T04:10:34Z

app/services/idv/session.rb

+      profile = Idv::ProfileFromApplicant.create(applicant, current_user, password)
+      self.profile_id = profile.id
+      cache_encrypted_pii(password)
+      profile
    end


From what I can tell, the return value (the profile) is not used when this method is called. Perhaps the profile_id assignment should be in its own method: set_applicant_profile_id, then instead of calling idv_session.profile_from_applicant(idv_session.applicant, password) in init_questions_and_profile, you would call both of these methods:

idv_session.set_applicant_profile_id(idv_session.applicant, password) idv_session.cache_encrypted_pii(password)

What do you think?

done in f6fffdb

pkarman · 2016-10-26T14:56:22Z

thanks for all the comments. I think I've hit everything.

@monfresh comments added to Cacher in 652f01386bffb28bda061f560f7cc9dbd4bfd406

monfresh · 2016-10-26T15:22:40Z

keys/pii_signing.key.enc

+0anypRH3z3wSpO1R/5qG1YUZOQlr46CSCUxwOv0RuPMmyAxAvvEzlsVb2CwikCGD
+O2L4KQ+tOCBfjFf+OYcxV3Z3WrasR77xfgCxA3x2ePlpQSzeHKt3pXy229URMGl3
+GCJlM8uk66/rwARsfIDVrDYy5QzYBBWYrBIqCKzyGQn023+sW8kejRSTqF525CBt
+-----END RSA PRIVATE KEY-----


Is this key used in this PR?

no but it will be in the next PR

I will remove it here

monfresh · 2016-10-26T15:34:16Z

app/controllers/profile_controller.rb

@@ -3,5 +3,7 @@ class ProfileController < ApplicationController
  layout 'card_wide'

  def index
+    cacher = Pii::Cacher.new(current_user, user_session)
+    @decrypted_pii = cacher.fetch


Every time the profile is accessed, and every time a SAML request is made, this will decrypt the PII and create a hash from them. Instead of repeating this action on every request, would it make sense to store the decrypted PII in the user_session so that it can be reused?

it's never ok to store PII decrypted.

subsequent PRs will encrypt the entire session as well.

monfresh · 2016-10-26T16:08:39Z

LGTM. Wanna squash?

**Why**: All PII must be encrypted at rest. Storing attributes in a single JSON blob makes this much easier. **How**: This change does not implement any actual encryption. That will follow in a separate change. This change includes: * schema modifications * user flow for collecting passphrase at IdV review * internal flow for decrypt PII at login for use in session

pkarman · 2016-10-26T16:58:14Z

@monfresh all squashed.

pkarman added the status - ready for review label Oct 22, 2016

pkarman mentioned this pull request Oct 22, 2016

Encrypt PII at rest #498

Closed

pkarman changed the title ~~Convert profile attributes to encrypted JSON~~ Convert profile attributes to JSON Oct 22, 2016

pkarman commented Oct 22, 2016

View reviewed changes

app/services/pii/encryption_error.rb

@@ -0,0 +1,4 @@

module Pii

Copy link

Contributor Author

pkarman Oct 22, 2016

Choose a reason for hiding this comment

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

this class will be exercised once #498 is complete

pkarman commented Oct 22, 2016

View reviewed changes

pkarman force-pushed the encrypt-pii-part2 branch 2 times, most recently from 851f92a to 795e9d9 Compare October 22, 2016 21:05

pkarman force-pushed the encrypt-pii-part2 branch from 795e9d9 to 7bd86ac Compare October 24, 2016 14:27

pkarman force-pushed the encrypt-pii-part2 branch from 7bd86ac to b1d8ea1 Compare October 24, 2016 14:32

zachmargolis reviewed Oct 24, 2016

View reviewed changes

pkarman force-pushed the encrypt-pii-part2 branch from 3cb411a to cbb4edb Compare October 24, 2016 19:02

zachmargolis reviewed Oct 25, 2016

View reviewed changes

zachmargolis approved these changes Oct 25, 2016

View reviewed changes

This was referenced Oct 25, 2016

Deactivate profile on pw reset #517

Closed

[WIP] Reencrypt pii on pw change #518

Closed

monfresh reviewed Oct 25, 2016

View reviewed changes

pkarman force-pushed the encrypt-pii-part2 branch from d4a9f00 to 27fc889 Compare October 25, 2016 21:42

pkarman self-assigned this Oct 25, 2016

monfresh reviewed Oct 25, 2016

View reviewed changes

jessieay reviewed Oct 25, 2016

View reviewed changes

jessieay approved these changes Oct 26, 2016

View reviewed changes

monfresh reviewed Oct 26, 2016

View reviewed changes

pkarman force-pushed the encrypt-pii-part2 branch from e907a03 to 0c0e30b Compare October 26, 2016 16:57

pkarman merged commit 94f3587 into master Oct 26, 2016

pkarman deleted the encrypt-pii-part2 branch October 26, 2016 17:44

Convert profile attributes to JSON #599

Convert profile attributes to JSON #599

Conversation

pkarman commented Oct 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfresh commented Oct 22, 2016

pkarman commented Oct 22, 2016

pkarman commented Oct 24, 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

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 24, 2016

pkarman commented Oct 24, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmargolis Oct 25, 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

pkarman commented Oct 25, 2016

Choose a reason for hiding this comment

pkarman Oct 25, 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

jessieay left a 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

pkarman commented Oct 25, 2016

pkarman commented Oct 25, 2016

pkarman commented Oct 26, 2016

jessieay left a 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

pkarman commented Oct 26, 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

monfresh commented Oct 26, 2016

pkarman commented Oct 26, 2016

pkarman commented Oct 22, 2016 •

edited

pkarman commented Oct 24, 2016 •

edited

zachmargolis Oct 25, 2016 •

edited

pkarman Oct 25, 2016 •

edited