Conversation
8b6c0f7 to
0b88575
Compare
0b88575 to
2440bcb
Compare
| end | ||
|
|
||
| def fetch_string | ||
| user_session[:decrypted_pii] |
There was a problem hiding this comment.
Can you help me understanding why the logic below is necessary? It looks like SessionEncryptor#kms_encrypt_pii! should prevent user_session[:encrypted_pii] from being an issue.
There was a problem hiding this comment.
Slacked with Mitchell and see where I got hung up. The session encryptor encrypts the user_session[:encrypted_pii] value, but never actually decrypts it. It leaves this to the session encryptor so we don't need to run KMS on every request for a proofed user.
There was a problem hiding this comment.
Attempted to clarify this better in cfea392
7b62800 to
edc6e4a
Compare
d71abf0 to
897da44
Compare
orenyk
left a comment
There was a problem hiding this comment.
I got a little lost in the weeds in the actual encryption stuff (hoping to take a closer look later) but in the meantime here are some minor style comments/suggestions. Great work!
fd55bf8 to
dfcb52c
Compare
7da78b8 to
94421de
Compare
lib/session_encryptor.rb
Outdated
| # rubocop:disable Layout/LineLength | ||
| SENSITIVE_REGEX = %r{FAKEY|MIDDLEFAKER|MCFAKERSON|1111111111111|GREAT FALLS|1938-10-06|2099-12-31|314-555-1212} | ||
| # rubocop:enable Layout/LineLength |
There was a problem hiding this comment.
Is it worth doing something similar to what was done in FakeAnalytics, referencing the constant from which these values are generated?
identity-idp/spec/support/fake_analytics.rb
Lines 23 to 31 in 0e8c7e1
There was a problem hiding this comment.
Thank you that's a great catch, I had forgotten we had them in a constant.
SessionEncryptor being in lib/ means it can't reference things outside of lib/ so I moved it into our constants file in e11e91946, but I'm not sure if that's the best place
There was a problem hiding this comment.
When I was using it for spec/factories/profiles.rb in #6229, I had felt it was kinda weird to reference the constant where it was at, so I like the idea of having a single constant of fake PII in a common location like that.
23c5c6d to
7e8361f
Compare
…ndle when needed changelog: Internal, Security, Only decrypt PII bundle when needed and limit usage of KMS encryption to needed use cases Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Oren Kanner <oren.kanner@gsa.gov>
7e8361f to
9cda15d
Compare
9cda15d to
bba056d
Compare
In the past the IDP used the `Encryption::Encryptors::SessionEncryptor` to encrypt sessions as a whole. This tool was used by the unfortunately named `SessionEncryptor` which acts as a serializer for the session store. The `Encryption::Encryptors::SessionEncryptor` was also used for encrypting PII temporarily while it was queued for letter sending. Two changes led to `Encryption::Encryptors::SessionEncryptor` being unused: - #6315 enabled partial session encryption which made the session encryptor sophisticated enough that it justified its own logic for encrypting elements instead of depending on `Encryption::Encryptors::SessionEncryptor` - #6211 replaced the `Encryption::Encryptors::SessionEncryptor` that was used for encrypting letter PII with a new encryptor built specifically for encrypting background arguments With these changes the `Encryption::Encryptors::SessionEncryptor` no longer has a caller. This commit removes it. [skip changelog]
In the past the IDP used the `Encryption::Encryptors::SessionEncryptor` to encrypt sessions as a whole. This tool was used by the unfortunately named `SessionEncryptor` which acts as a serializer for the session store. The `Encryption::Encryptors::SessionEncryptor` was also used for encrypting PII temporarily while it was queued for letter sending. Two changes led to `Encryption::Encryptors::SessionEncryptor` being unused: - #6315 enabled partial session encryption which made the session encryptor sophisticated enough that it justified its own logic for encrypting elements instead of depending on `Encryption::Encryptors::SessionEncryptor` - #6211 replaced the `Encryption::Encryptors::SessionEncryptor` that was used for encrypting letter PII with a new encryptor built specifically for encrypting background arguments With these changes the `Encryption::Encryptors::SessionEncryptor` no longer has a caller. This commit removes it. [skip changelog]
Currently, we double encrypt the entire web session for a user. When saving a session, first it is converted to JSON, then encrypted with AES, then with KMS, and then put into Redis. The reverse process happens when the session is loaded.
This safe-by-default method is straightforward conceptually and in implementation, but imposes costs and friction in other ways. We have added more and more to session storage, and KMS can only encrypt 4kB per API call, which leads to us chunking the string and having to make multiple calls to KMS. Certain values we store can be quite large, which inflates the session size significantly in some contexts. The KMS API calls now take up a sizeable portion of our network and server time.
This method also places KMS in the critical path since it is used on most requests, including the home page.
From a security perspective, it means we decrypt sensitive data even when it won't be used. The most relevant part in this regard is the PII bundle, which we only need to KMS decrypt when we are displaying it to the user or passing it to the service provider.
To meet our security requirements and ensure that sensitive data continues to be KMS encrypted while vastly reducing our KMS usage otherwise, this PR attempts to split the session into sensitive and less sensitive data. The entire session is still AES encrypted, but the sensitive parts are KMS encrypted still. These changes aim to handle PII in two ways:
Pii::Cacherinterface that we've moved our PII bundle handling towards using (Create common interface for accessing PII bundle in session #6054, Do not save additional PII to session when not needed #6072, Move more PII bundle usage to the common interface in Pii::Cacher #6273, Rename reactivate account session :personal_key key to be more descriptive #6283).Alerting has been added if we detect a potentially sensitive key not being KMS encrypted as it should be, and sends the session structure without values to help troubleshoot if it does. In tests and development, it will raise exceptions if a sensitive key is not encrypted properly.
This change is not backwards compatible on its own, and will require deploying the code that contains the ability to parse both formats (without writing the new format immediately). Writing the new format to Redis is currently behind a configuration flag that we can switch on if and when we feel comfortable. If the new format is problematic after being enabled, the switch can be turned off with full backwards compatibility.
Document
Discussion Thread