[PLAT-639] Add credentials obfuscation #33

Antoninbln · 2019-08-07T13:55:34Z

Purpose of this PR

To be clean with sentry implementation, we need to obfuscate some critical information in logs such as TOKEN=MY_TOKEN or "token": "My token".

Changes

New obfuscation mode has been added : for example password=My-super-password in log.msg is now obfuscated by default ;
Parameters of SensitiveDataStream has changed :
- fragments -> Example : (password|mdp);
- patterns -> Example : [{ regex: YOUR_REGEX, substitute: SUBSTITUTION_CONTENT }];
Add the possibility to provide an "obfuscation config" to obfuscate whatever you want. To do this you need to use :

const newLogger = init({
   logger: { sensitiveDataFragment, sensitiveDataPattern },
});

Update documentation

…ffuscation

codecov · 2019-08-07T13:55:39Z

Codecov Report

Merging #33 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #33   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines          39     44    +5     
=====================================
+ Hits           39     44    +5

Impacted Files	Coverage Δ
index.js	`100% <ø> (ø)`	⬆️
streams/sensitive-data.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9648ed5...d673f48. Read the comment docs.

codecov · 2019-08-07T13:55:39Z

Codecov Report

Merging #33 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #33   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines          39     42    +3     
=====================================
+ Hits           39     42    +3

Impacted Files	Coverage Δ
index.js	`100% <ø> (ø)`	⬆️
streams/sensitive-data.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9648ed5...507fb00. Read the comment docs.

ygrandmaitre

LGTM some comment about default regexp

ygrandmaitre · 2019-08-07T16:43:55Z

streams/sensitive-data.js

+        {
+          regex: new RegExp(`${this.fragments}=([\\w-]*)`, 'ig'), // @Match mdp=My-super-password
+          substitute: `$1=${this.replacer}`,
+        },


I think we should only have the first regexp by default to be iso with the actual version of the logger.
And the second regex should only be used in nestor-api

ygrandmaitre · 2019-08-07T16:46:27Z

README.md

+const sensitiveDataPattern = [
+  {
+    regex: YOUR_NEW_REGEX,
+    substitute: SUBSTITUTION_CONTENT,


I'm not sure we need to change the substitute I think we can forced to be SENSITIVE_DATA

Indeed, it isn't realy about SENISITIVE_DATA but more about the whole pattern replacement such as :

"password":"WHATEVER" -> "$1":"WHATEVERE" which needs dots and quotes.

Althought password=whatever - > $1=WHATEVER doesn't need.

ThierryPot · 2019-08-07T16:48:57Z

streams/sensitive-data.js

+          regex: new RegExp(`"${this.fragments}":"([^"]*)"`, 'ig'), // @Match "mdp":"My super password"
+          substitute: `"$1":"${this.replacer}"`,
+        },
+        {


I would remove this case from the default regexp to keep the same behaviour as before.

ThierryPot · 2019-08-07T16:49:46Z

README.md

+const sensitiveDataPattern = [
+  {
+    regex: YOUR_NEW_REGEX,
+    substitute: SUBSTITUTION_CONTENT,


Do we really need to specify a substitute? the default one should be enough? Don't you think?

Substitute is indeed replacement pattern like "$1":"__SENSITIVE_DATA__", and it is not the same depending on cases, sometimes we need quotes, for other ones we don't necessarily need.

If it is just to avoid double quote in the equal case, I keep thinking it is not necessary.

ThierryPot · 2019-08-07T16:51:43Z

streams/sensitive-data.js

+    let sanitized = input;
+
+    // Apply replace on input looping through patterns array
+    for (let pattern of this.patterns) {


All the logging process is done synchronously, we must ensure this array has not too many items because it could lead to performance issues.
If we don't do anything in code we must at least warn the users in the documentation.

I just updated README because for the moment there is no usage of this functionality. But if you prefer I can update code.

ygrandmaitre

LGTM

…entials" This reverts commit 608c4a3, reversing changes made to 9648ed5.

PLAT-639 Add offuscation for credentials & some config to customize o…

d673f48

…ffuscation

Antoninbln requested a review from a team August 7, 2019 13:55

Antoninbln changed the title ~~[PLAT-639] Add credentials offuscation~~ [PLAT-639] Add credentials obfuscation Aug 7, 2019

PLAT-639 Add sensitive data management in README

830f1db

ygrandmaitre reviewed Aug 7, 2019

View reviewed changes

ThierryPot reviewed Aug 7, 2019

View reviewed changes

Antonin Blin added 3 commits August 8, 2019 11:08

PLAT-639 add 2 more cases in tests

c074cb1

PLAT-639 Increment major version

0dccac8

PLAT-639 Add review fixes

507fb00

ygrandmaitre approved these changes Aug 8, 2019

View reviewed changes

Antoninbln merged commit 608c4a3 into master Aug 8, 2019

Antoninbln pushed a commit that referenced this pull request Aug 9, 2019

Revert "Merge pull request #33 from transcovo/PLAT-639-offuscate-cred…

757539c

…entials" This reverts commit 608c4a3, reversing changes made to 9648ed5.

Antoninbln mentioned this pull request Aug 9, 2019

TECH revert credentials obfuscation #34

Merged

JulienJourdain deleted the PLAT-639-offuscate-credentials branch December 16, 2019 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PLAT-639] Add credentials obfuscation #33

[PLAT-639] Add credentials obfuscation #33

Antoninbln commented Aug 7, 2019 •

edited

Loading

codecov bot commented Aug 7, 2019

codecov bot commented Aug 7, 2019 •

edited

Loading

ygrandmaitre left a comment

ygrandmaitre Aug 7, 2019

ygrandmaitre Aug 7, 2019

Antoninbln Aug 8, 2019

ThierryPot Aug 7, 2019

ThierryPot Aug 7, 2019

Antoninbln Aug 8, 2019 •

edited

Loading

ThierryPot Aug 8, 2019

ThierryPot Aug 7, 2019

Antoninbln Aug 8, 2019

ygrandmaitre left a comment

[PLAT-639] Add credentials obfuscation #33

[PLAT-639] Add credentials obfuscation #33

Conversation

Antoninbln commented Aug 7, 2019 • edited Loading

Purpose of this PR

Changes

codecov bot commented Aug 7, 2019

Codecov Report

codecov bot commented Aug 7, 2019 • edited Loading

Codecov Report

ygrandmaitre 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

Antoninbln Aug 8, 2019 • edited Loading

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

ygrandmaitre left a comment

Choose a reason for hiding this comment

Antoninbln commented Aug 7, 2019 •

edited

Loading

codecov bot commented Aug 7, 2019 •

edited

Loading

Antoninbln Aug 8, 2019 •

edited

Loading