Skip to content
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

[PLAT-639] Add credentials obfuscation #33

Merged
merged 5 commits into from
Aug 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,3 +79,32 @@ you can skip this (not recommanded) by setting the environment variable
In addition, you can update the pattern on which to make the match with the
environment variable `LOGGER_SENSITIVE_DATA_PATTERN`. Its value must represent
a valid [capturing regular expression](https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Objets_globaux/RegExp#group_back).

You also can add custom filter :
```
const sensitiveDataFragment = '(pass|password)'; // Will obfuscate 'pass' and 'password' data

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

Moreover, you can add customize the way it replaces data :
```
const sensitiveDataPattern = [
{
regex: YOUR_NEW_REGEX,
substitute: SUBSTITUTION_CONTENT,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Antoninbln Antoninbln Aug 8, 2019

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

}
]; // Will replace data matching with new regex by substitute content

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

🚨 Process is synchronous, it means that `sensitiveDataPattern` can produce performance issues on large sizes.
5 changes: 4 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ function init(config) {
} else if (finalConfig.logger.hideSensitiveData) {
loggerConfig.streams.push({
level: loggerLevel,
stream: new SensitiveDataStream(finalConfig.logger.sensitiveDataPattern),
stream: new SensitiveDataStream(
finalConfig.logger.sensitiveDataFragment,
finalConfig.logger.sensitiveDataPattern
),
});
} else {
loggerConfig.streams.push({ level: loggerLevel, stream: process.stdout });
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "chpr-logger",
"version": "3.2.0",
"version": "4.2.0",
"description": "Logger for NodeJS application stack",
"main": "index.js",
"directories": {
Expand Down
21 changes: 18 additions & 3 deletions streams/sensitive-data.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,27 @@ const DEFAULT_SENSITIVE_DATA_FRAGMENTS =
'(mdp|password|authorization|token|pwd|auth)';

module.exports = class SensitiveDataStream {
constructor(fragments) {
constructor(fragments, patterns = []) {
this.fragments = fragments || DEFAULT_SENSITIVE_DATA_FRAGMENTS;
this.pattern = new RegExp(`"${this.fragments}":"([^"]*)"`, 'ig');
this.replacer = '__SENSITIVE_DATA__';

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

write(input) {
const sanitized = input.replace(this.pattern, '"$1":"__SENSITIVE_DATA__"');
let sanitized = input;

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

sanitized = sanitized.replace(pattern.regex, pattern.substitute);
}

return process.stdout.write(sanitized);
}
Expand Down
77 changes: 76 additions & 1 deletion test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,15 +154,64 @@ describe('index.js', () => {
req: {
token: 'My personal token',
},
cmd: 'command docker build password=test',
},
'Logging amazingly sensitive data!'
);

const message = logs.shift();

expect(message.password).to.equal('__SENSITIVE_DATA__');
expect(message.headers['accept-language']).to.equal('fr-FR');
expect(message.headers.authorization).to.equal('__SENSITIVE_DATA__');
expect(message.req.token).to.equal('__SENSITIVE_DATA__');
});

it('should replace good parts of string using custom replacement & fragment patterns', () => {
const sensitiveDataPattern = [
{
regex: new RegExp(`(password)=([\\w-]*)`, 'ig'),
substitute: `$1=__SENSITIVE_DATA__`,
},
];

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

newLogger.info(
{
password: 'My personal password',
pass: 'My other personal password',
headers: {
'accept-language': 'fr-FR', // unchanged
authorization: 'Bearer token',
},
req: {
token: 'My personal token',
},
cmd:
'command docker build --build-arg password=test-1234-abcd --build-arg password=test1234-abcd --build-arg password=TEST-1234-abcd ',
cmd2: 'command docker build --build-arg password=test',
},
'Logging amazingly sensitive data!'
);

const message = logs.shift();

// Default cases
expect(message.password).to.equal('__SENSITIVE_DATA__');
expect(message.headers['accept-language']).to.equal('fr-FR');
expect(message.headers.authorization).to.equal('__SENSITIVE_DATA__');
expect(message.req.token).to.equal('__SENSITIVE_DATA__');

// Custom cases
expect(message.cmd).to.equal(
'command docker build --build-arg password=__SENSITIVE_DATA__ --build-arg password=__SENSITIVE_DATA__ --build-arg password=__SENSITIVE_DATA__ '
);
expect(message.cmd2).to.equal(
'command docker build --build-arg password=__SENSITIVE_DATA__'
);
});
});
});
Expand All @@ -182,7 +231,7 @@ describe('index.js', () => {

it('should use the sensitive data stream with specific pattern fragments if set', () => {
const newLogger = init({
logger: { sensitiveDataPattern: '(password)' },
logger: { sensitiveDataFragment: '(password)' },
});

expect(newLogger.streams).to.have.lengthOf(1);
Expand All @@ -192,6 +241,32 @@ describe('index.js', () => {
expect(newLogger.streams[0].stream.fragments).to.equal('(password)');
});

it('should use the sensitive data stream with specific replacement patterns if set', () => {
const sensitiveDataPattern = [
{
regex: new RegExp(`(testing_string)=([\\w-]*)`, 'ig'),
substitute: `"$1":"__TESTING_REPLACEMENT__"`,
},
];

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

expect(newLogger.streams).to.have.lengthOf(1);
expect(newLogger.streams[0]).to.have.property('stream');

expect(newLogger.streams[0].stream).to.be.instanceOf(SensitiveDataStream);
expect(newLogger.streams[0].stream.patterns).to.have.lengthOf(2); // New pattern & default one
expect(newLogger.streams[0].stream.patterns[0]).to.deep.equal(
// Test the 1st pattern (the other one is the default one)
{
regex: /(testing_string)=([\w-]*)/gi,
substitute: `"$1":"__TESTING_REPLACEMENT__"`,
}
);
});

it('should use only the default stdout stream if LOGGER_USE_SENSITIVE_DATA_STREAM is false', () => {
const newLogger = init({
logger: { hideSensitiveData: false },
Expand Down