Skip to content
This repository has been archived by the owner on Oct 27, 2018. It is now read-only.

Security changes whitelisting #22

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Security changes whitelisting #22

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e0a0ac9
Adding CORS Support to yaml
gaurim17 Jan 18, 2016
c8dde1b
Adding security changes
gaurim17 Jan 20, 2016
64c0afc
security changes
gaurim17 Jan 20, 2016
e1b90a2
security changes for whitelisting the keys
gaurim17 Jan 20, 2016
8d05ea4
security changes for whitelistingkeys
gaurim17 Jan 20, 2016
796829f
security changes for whitelistingkeys
gaurim17 Jan 20, 2016
616ff46
security changes for whitelistingkeys
gaurim17 Jan 20, 2016
8d5061f
security changes for whitelistingkeys
gaurim17 Jan 20, 2016
9b8b46f
Code refactoring
gaurim17 Jan 29, 2016
cd02f9e
security changes for whitelistingkeys
gaurim17 Jan 29, 2016
60cd81d
security changes for whitelistingkeys
gaurim17 Jan 29, 2016
f7548f6
security changes for CORS
gaurim17 Jan 29, 2016
cb9f24b
security changes for whitelistingkeys
gaurim17 Jan 29, 2016
1f1ba3b
security changes for whitelistingkeys
gaurim17 Jan 29, 2016
34f71e0
Uncommenting the default keys sent by bucky
gaurim17 Feb 22, 2016
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
19 changes: 19 additions & 0 deletions config/default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ influxdb:
# Acceptable version are: '0.8' and '0.9'
version: '0.9'

useWhitelistedKeys: false
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename this onlyAcceptWhitelistedKeys.


modules:
# The modules just get require'd in, so they don't have to be in the Bucky project.
Expand All @@ -36,3 +37,21 @@ modules:
# - ./modules/statsd
# - ./modules/openTSDB
# - ./modules/influxdb

whitelistedKeys:
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

Uncommenting atleast 1 of the keys should fix that issue. This is similar to having to uncomment atleast 1 collectors.

# Uncomment the keys that you'd like to whitelist
Copy link

Choose a reason for hiding this comment

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

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"

# - file.page.navigationStart
# - file.page.unloadEventStart
# - file.page.unloadEventEnd
# - file.page.fetchStart
# - file.page.domainLookupStart
# - file.page.domainLookupEnd
# - file.page.connectStart
# - file.page.connectEnd
# - file.page.requestStart
# - file.page.responseStart
# - file.page.responseEnd
# - file.page.domLoading
# - file.page.domInteractive
# - file.page.domContentLoadedEventStart
# - file.page.toJSON
18 changes: 17 additions & 1 deletion modules/collectors.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,29 @@ _ = require 'underscore'

load = require "../lib/load"
modules = require("config").modules
useWhitelistedKeys = require("config").useWhitelistedKeys
whitelistedkeys = ''

module.exports = ({app, logger, config}, next) ->
collectorHandler = (collectors) ->
arrOfVals = []
return (req, res) ->
res.send(204, '')
if useWhitelistedKeys
for fields of req.body
if (arrOfVals.indexOf( fields ) == -1)
arrOfVals.push( fields )
if arrayEqual(arrOfVals, whitelistedkeys)
res.send(204, '')
else
res.send(406, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

else
res.send(204, '')

for coll in collectors
coll(req.body, {req, res})

logger.log "Loading collectors: #{ modules.collectors.join(', ') }"
whitelistedkeys = "#{modules.whitelistedKeys}".split(',')
Copy link
Contributor

Choose a reason for hiding this comment

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

modules.whitelistedKeys should already be an array, no? Why convert it into a string only to convert it right back?

Copy link
Author

Choose a reason for hiding this comment

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

Its a comma separated list.

Copy link
Contributor

Choose a reason for hiding this comment

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

The values are coming from the YAML parser as an array: https://github.com/HubSpot/BuckyServer/pull/22/files#diff-fe7044f2ecd69c76ce484ad03fabc12fR41

They will only become a comma separated list if you cast them into a string, as you're doing with the string interpolation.

Copy link
Author

Choose a reason for hiding this comment

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

So if I do modules.whitelistedKeys, the output I get is, [ [Getter/Setter], [Getter/Setter] ]

Copy link
Contributor

Choose a reason for hiding this comment

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

The console will log it like that, but if you try _.contains(modules.whitelistedKeys, 'file.page.fetchStart') for example, you'll see it works as you expect.

Copy link
Author

Choose a reason for hiding this comment

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

Good to know. Changes done.


collectors = {}
collPromises = []
Expand All @@ -38,3 +51,6 @@ module.exports = ({app, logger, config}, next) ->
collector[path] = collectorHandler(hls)

next collector

arrayEqual = (a, b) ->
a.length is b.length and a.every (elem, i) -> elem is b[i]
2 changes: 1 addition & 1 deletion server.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ configWrapper = require './lib/configWrapper'
load = require './lib/load'

MODULES = config.modules

loadLogger = ->
if MODULES.logger
load(MODULES.logger, {config})
Expand Down Expand Up @@ -62,7 +63,6 @@ loadApp = (logger, loadedConfig) ->
moduleGroups = {}
loadModuleGroup = (group) ->
moduleGroups[group] = {}

if MODULES[group]
_.map MODULES[group], (name) ->
logger.log "Loading #{ group } Module", name
Expand Down