-
Notifications
You must be signed in to change notification settings - Fork 2
Encrypting sensitive data on demand #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
a500c74 to
7582833
Compare
|
|
||
| // Mask sensitive attributes, remove null | ||
| const maskSensitiveAttributes = (key: string, value: JSONObject): JSONObject | string | undefined => { | ||
| const maskSensitiveAttributes = (key: string, value: any): JSONObject | string | undefined => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't define types as any. It has to be JSONObject
| */ | ||
| if (!!this.cipherKey) { | ||
| const cipher = createCipheriv( | ||
| 'aes-256-ecb', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to use aes-256-ecb ? We should have the algorithm as an optional setting, with aes-256-cbc as a default.
An IV would need to be created for each encryption.
The response should be an object:
{
iv: 'xxxxxx',
encrypted: 'abcdef'
}When value is an array or an object, the String(value) won't return anything usable. For theses cases, the value should be stringified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to understand the reason behind using encryption?
We also need to understand if this is PII compliant. Encrypted values are decryptable if you have the encryption key.
If what we want is to be able to filter logs based on a known value, maybe hashing the value (with a non-reversible algorithm) is good enough and improves compliance?
| { | ||
| "name": "@serverless-guru/logger", | ||
| "version": "1.0.7", | ||
| "version": "1.0.8", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment for another PR.
This should be automated.
| */ | ||
| if (!!this.cipherKey) { | ||
| const cipher = createCipheriv( | ||
| 'aes-256-ecb', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to understand the reason behind using encryption?
We also need to understand if this is PII compliant. Encrypted values are decryptable if you have the encryption key.
If what we want is to be able to filter logs based on a known value, maybe hashing the value (with a non-reversible algorithm) is good enough and improves compliance?
We have security concern for sensitive data to be pull in log. The logger masking is not fully help for debugging as we don't know the payload identifier when they are masked. So instead of mask the sensitive attributes, we can keep securing them with encryption tool as well as let them to be searchable on debugging.