Skip to content

Ensure correct encryption key is used for new session format#6385

Merged
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/session-encryption-update
May 20, 2022
Merged

Ensure correct encryption key is used for new session format#6385
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/session-encryption-update

Conversation

@mitchellhenke
Copy link
Contributor

Previously, we were referencing the attribute encryptor, which uses the attribute_encryption_key, but we should not use that for session encryption!

This does not affect production as this encryption format is not enabled yet

@mitchellhenke mitchellhenke requested a review from jmhooper May 19, 2022 18:12
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 191 to 193
Copy link
Contributor

Choose a reason for hiding this comment

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

this feel familiar, I know we left a comment about a ruby openssl version that explained why previously... what if we moved this inside AesEncryptor class? (and/or also added a comment?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, I had forgotten we already have both here, so I'm guessing this is not strictly necessary (I copied it from SessionEncryptor)

Copy link
Contributor

Choose a reason for hiding this comment

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

yayyyy good job past us!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out, I've removed the truncation from this. The default test session_encryption_key is also longer than 32, so it works as intended.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/session-encryption-update branch from d94e6a1 to 3252585 Compare May 19, 2022 18:37
changelog: Internal, Security, Only decrypt PII bundle when needed and limit usage of KMS encryption to needed use cases
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/session-encryption-update branch from 3252585 to 58ba86b Compare May 20, 2022 11:53
@mitchellhenke mitchellhenke marked this pull request as ready for review May 20, 2022 12:08
@mitchellhenke mitchellhenke merged commit ceb9ae7 into main May 20, 2022
@mitchellhenke mitchellhenke deleted the mitchellhenke/session-encryption-update branch May 20, 2022 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants