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 2 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
27 changes: 27 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,33 @@ values by `__SENSITIVE_DATA__` string. This feature is enabled by default but
you can skip this (not recommanded) by setting the environment variable
`LOGGER_USE_SENSITIVE_DATA_STREAM` to `false`.

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

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

Moreover, you can also 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,
},
});
```

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).
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
28 changes: 25 additions & 3 deletions streams/sensitive-data.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,34 @@ 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__';

// If a pattern is provided
if (patterns.length) {
this.patterns = patterns;
} else {
// If no pattern provided, then add 2 default regexes
this.patterns = [
{
regex: new RegExp(`"${this.fragments}":"([^"]*)"`, 'ig'), // @Match "mdp":"My super password"
substitute: `"$1":"${this.replacer}"`,
},
{

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

];
}
}
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
31 changes: 30 additions & 1 deletion test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,15 +154,20 @@ 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__');
expect(message.cmd).to.equal(
'command docker build password=__SENSITIVE_DATA__'
);
});
});
});
Expand All @@ -182,7 +187,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 +197,30 @@ 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.deep.equal([
{
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