Security changes whitelisting #22
Conversation
| whitelistedkeys: | ||
| - key1 | ||
| - key2 | ||
| - key3 |
There was a problem hiding this comment.
The whitelistedkeys part should be commented out as an 'example'
|
Work in progress |
|
(this addresses #20) |
|
This one is ready to go in, waiting on review |
|
Could we check for the presence of whitelistedKeys and use that as the trigger to filter keys instead of having to use a separate boolean? |
|
So you need atleast one of those keys commented out for the server to start up, else it gives parse errors. |
| @@ -36,3 +37,21 @@ modules: | |||
| # - ./modules/statsd | |||
| # - ./modules/openTSDB | |||
| # - ./modules/influxdb | |||
|
|
|||
| whitelistedKeys: | |||
| # Uncomment the keys that you'd like to whitelist | |||
There was a problem hiding this comment.
A note here would be helpful. "The example below are timing metrics that Bucky includes by default. You will need to whitelist these if you use the default configuration of Bucky and enable whitelisting"
|
@zackbloom could you review this and check if its ok to merge this? |
|
|
||
| module.exports = ({app, logger, config}, next) -> | ||
| collectorHandler = (collectors) -> | ||
| return (req, res) -> | ||
| if useWhitelistedKeys | ||
| if not _.every(_.keys(req.body), (v) -> _.contains(whitelistedkeys, v)) | ||
| res.send(406, '') |
There was a problem hiding this comment.
We should probably respond with a meaningful error message here. I'm also not sure if it wouldn't make sense to accept the keys which do match the whitelist, rather than returning an error, in the interest of being able to use the BuckyClient software which automatically generates keys.
| @@ -36,3 +37,23 @@ modules: | |||
| # - ./modules/statsd | |||
| # - ./modules/openTSDB | |||
| # - ./modules/influxdb | |||
|
|
|||
| whitelistedKeys: | |||
There was a problem hiding this comment.
In trying to run this, it looks like this file is invalid YAML in this current form. I think you need to fix the indentation perhaps?
There was a problem hiding this comment.
Uncommenting atleast 1 of the keys should fix that issue. This is similar to having to uncomment atleast 1 collectors.
|
@zackbloom I have uncommented the default keys sent by bucky in the yaml. It should work in its current form. Please check and merge if everything looks right. |
|
@zackbloom wanted to check with you if you need anything from us after the last comment? Or is this ready to merge? |
No description provided.