Security: limit Masterkey remote access #4017

pungme · 2017-07-14T15:39:44Z

add config option to specify list of ip addresses to allow masterKey to be used
This is good for doubling down the security, even if your masterKey is leaked, it still getting blocked based on ip address.
feel free to suggest! :-)
Closes Security: limit Masterkey remote access #1987

flovilmart · 2017-07-14T15:43:06Z

Hey Thanks for the PR, can you add the option also to the CLI options?

pungme · 2017-07-14T15:50:40Z

Hey @flovilmart thanks for a prompt feedback! I added CLI options as you suggested. let me know if there's anything else i should do.

codecov · 2017-07-14T16:37:17Z

Codecov Report

Merging #4017 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4017      +/-   ##
==========================================
+ Coverage    90.7%   90.73%   +0.02%     
==========================================
  Files         116      116              
  Lines        7889     7910      +21     
==========================================
+ Hits         7156     7177      +21     
  Misses        733      733

Impacted Files	Coverage Δ
src/cli/definitions/parse-server.js	`50% <ø> (ø)`	⬆️
src/defaults.js	`90% <ø> (ø)`	⬆️
src/ParseServer.js	`88.67% <100%> (+0.07%)`	⬆️
src/middlewares.js	`97.48% <100%> (+0.22%)`	⬆️
src/Config.js	`95.17% <100%> (+0.24%)`	⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`96.37% <0%> (-0.13%)`	⬇️
src/triggers.js	`90.9% <0%> (+0.5%)`	⬆️

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 e6cc820...358c136. Read the comment docs.

montymxb · 2017-07-14T17:12:49Z

@pungme this is fantastic! Having the capacity to restrict requests, even with an authorized master key, definitely important from a security standpoint.

flovilmart · 2017-07-14T17:17:38Z

src/middlewares.js

@@ -110,6 +110,11 @@ export function handleParseHeaders(req, res, next) {
  req.config = new Config(info.appId, mount);
  req.info = info;

+  const ip = req.ip;


hum, how does it work behind a LB? req.ip may be the LB's IP and not the original client IP right?

Through a reverse proxy, yeah the IP would probably be that of the balancer :/. Although in some cases the requesting IP would be passed along in the X-Forwarded-For header, not necessarily guaranteed however. To note this could be more than one IP, depending on how many proxies it passed through.

Perhaps we could check for that header and use it's value instead? When present that is.

Actually, express has a guide for running behind proxies. Allowing it to handle itself internally, req.ip would just return what it can deduce as the original client ip. All that would have to be done is to add the following to the express initialization code.

app.enable('trust proxy');

good question. let me rework a bit and test with our loadbalancer environment

Hey @flovilmart , I think @montymxb was right. We can handle the LB case with something like:

const ip = req.headers['x-forwarded-for'] || req.connection.remoteAddress || ... etc

But the question is, should we try to handle that on parse-server side or let user configure that by using app.enable('trust proxy'); in the beginning?

Let me know what you think.

benishak · 2017-07-15T10:12:09Z

a better security mechanism is to use a proxy and restrict CORS (Allow Origin)
https://stackoverflow.com/questions/3862813/how-can-i-use-an-http-proxy-with-node-js-http-client

flovilmart · 2017-07-15T13:59:19Z

CORS is only implemented in web browsers, and you may wanna restrict by Ip nonetheless. It isn’t better just different

montymxb · 2017-07-15T15:12:20Z

There is actually an npm cors package for connect/express, but cross origin requests, in the context of Access-Control-Allow-Origin as mentioned above, would restrict access to resources rather than filter actions performed on resources. We wouldn't to block someone from making a query, we simply want to prevent them from querying beyond their limitations.

The exception being CORS does feature Access-Control-Request-Headers, which allows you to filter out requests containing certain headers (like that for carrying the masterKey) to those who are specifically authorized to do so.

Still, I prefer what's being put together here, rather than to include another dependency.

benishak · 2017-07-15T17:36:22Z

yes I do actually like the idea, we only need to make sure that the IP is passed correctly when using a reverse proxy

benishak · 2017-07-15T18:05:53Z

maybe it is not big issue, most reserve proxy nginx, haproxy or even AWS ELB will set XFF header if configured correctly, we only need to improve the help message and let the user decide whether to use this feature or not, at the end it is not harmful ..

flovilmart · 2017-07-16T14:43:00Z

The trust proxy option can always be added we should be handling for the ‘worst’ scenario :)

miguel-s · 2017-07-17T08:14:52Z

@benishak as already mentioned, using CORS is a different issue that only really affects browsers. By generally restricting the ip we can make sure that, in case the master key leaks, no malicious requests get to the server, independently of origin (e.g. a node script).

@pungme it's probably better to not rely on app.enable('trust proxy'); and check for all sources of ip, like the x-forwarded-for header

pungme · 2017-07-17T10:10:02Z

Hey @flovilmart just updated the pull request based on your suggestions. Let me know what else I should work on. Thanks!

benishak · 2017-07-17T14:39:50Z

@miguel-s yes I know forget about CORS for now, I'm talking about this pull request is not harmful, we only need to improve the help message and tell the user if you want to use the masterKeyIps they need to make sure that their reverse proxy nginx, haproxy or elb is configured correctly if they are using one. If the reverse proxy is correctly configured the header x-forwarded-for will be set correctly.

pungme : just add a notice to the help message to the user that if they are using reverse proxy they need to make sure it is configured to set the x-forwarded-for header, maybe adding a small chapter in the docs about it would be also good

https://easyengine.io/tutorials/nginx/forwarding-visitors-real-ip/

montymxb · 2017-07-17T15:28:27Z

src/middlewares.js

@@ -110,7 +110,11 @@ export function handleParseHeaders(req, res, next) {
  req.config = new Config(info.appId, mount);
  req.info = info;

-  const ip = req.ip;
+  const ip = req.headers['x-forwarded-for'] ||


Quick thing, it is possible that this value may be multiple ips. Especially if the user passed through one or more proxies before getting to a balancer, (quick breakdown here).

Considering that if you have more than 1 IP, and they are each separated by a ,, you can just split the string apart by it's commas to and take the first 'origin' entry (or the only entry).

req.headers['x-forwarded-for'].split(',')[0]

montymxb · 2017-07-17T15:31:19Z

@pungme can you add some master key allowed/denied tests accounting for the x-forwarded-for header being set? Additionally can you also factor for multiple ips? I left a comment in the new code mentioning that a bit more.

nbering · 2017-07-17T16:11:55Z

Could this support a subnet range instead of a specific IP? Right now I have some code that runs in AWS Lambda to perform tasks against Parse Server using the rest API and the master key. It's part of the same private network, but would never come from a guaranteed specific IP address.

pungme · 2017-07-17T17:11:19Z

Hey @montymxb, thank you so much for your comment :-) I've updated the change as you suggested. Let me know if there are anything else I should change.

pungme · 2017-07-17T17:16:29Z

@nbering This could be the next step of this feature. it shouldn't be too hard to support that. But i'd prefer to getting this one done first :-)

nbering · 2017-07-17T18:03:25Z

Validating that the value is in fact an IP address would probably help to shim in subnet-based stuff later without breaking people - because a good value was enforced up-front.

There's IP address validation built into NodeJS core on the net module.

https://nodejs.org/dist/latest-v6.x/docs/api/net.html#net_net_isip_input

montymxb · 2017-07-18T15:07:46Z

@nbering I like the idea of being able to match up to a subnets, but I think for step one let's go with what we have here. Assuming this plays nice once merged, and we don't have any issues regarding forwarded-for I think it would be an excellent idea to have subnet matching built in later.

@pungme no problem! I like the test cases 👍 . Can't think of too much else besides the aformentioned ip validation via net.isIP(ip), particularly for the ips listed for master key access. It could be good to let them know in case they drop in an invalid IP by chance, and additionally to add a test case trying to add a 'bad' ip (you have to correct your existing entries). If you can sanitize that we should be pretty close to being ready here.

montymxb · 2017-07-18T16:37:30Z

Excellent! Well this looks pretty good to me. @flovilmart any thoughts or concerns on this as it stands?

flovilmart · 2017-07-18T17:09:26Z

src/middlewares.js

@@ -110,6 +110,15 @@ export function handleParseHeaders(req, res, next) {
  req.config = new Config(info.appId, mount);
  req.info = info;

+  const ip = (req.headers['x-forwarded-for'] && req.headers['x-forwarded-for'].split(',')[0]) ||


Maybe extract this into a function getClientIp and document each check?

flovilmart · 2017-07-18T17:10:03Z

One small nit for the ip extraction otherwise it’s great!

pungme · 2017-07-18T17:58:00Z

@flovilmart thanks a lot for your reply, I've changed as you suggested. Let me know if there are anything else 😊

flovilmart · 2017-07-23T16:26:26Z

Let's see how it goes!

pungme added 11 commits July 4, 2017 16:27

update choose_password to have the confirmation

8598a5b

Merge branch 'master' of https://github.com/ParsePlatform/parse-server

1dc64df

add comment mark

613cb11

First version, no test

b69692c

throw error right away instead of just use masterKey false

b5c2f7a

fix the logic

3538c89

move it up before the masterKey check

4b2c8e4

adding some test

2f55b45

typo

04bdd3e

remove the choose_password

4008703

newline

ff138aa

add cli options

4fe7d2d

remove trailing space

e43bfd2

flovilmart reviewed Jul 14, 2017

View reviewed changes

handle in case the server is behind proxy

8827646

montymxb reviewed Jul 17, 2017

View reviewed changes

add getting the first ip in the ip list of xff

ffc7b6f

sanity check the ip in config if it is a valid ip address

e5e06fd

flovilmart suggested changes Jul 18, 2017

View reviewed changes

split ip extraction to another function

b8a2343

trailing spaces

358c136

flovilmart approved these changes Jul 23, 2017

View reviewed changes

flovilmart merged commit 7e54265 into parse-community:master Jul 23, 2017

Uh oh!

Security: limit Masterkey remote access #4017

Security: limit Masterkey remote access #4017

Uh oh!

Conversation

pungme commented Jul 14, 2017 • edited by natanrolnik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flovilmart commented Jul 14, 2017

Uh oh!

pungme commented Jul 14, 2017

Uh oh!

codecov bot commented Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

montymxb commented Jul 14, 2017

Uh oh!

flovilmart Jul 14, 2017

Choose a reason for hiding this comment

Uh oh!

montymxb Jul 14, 2017

Choose a reason for hiding this comment

Uh oh!

montymxb Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pungme Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pungme Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benishak commented Jul 15, 2017

Uh oh!

flovilmart commented Jul 15, 2017

Uh oh!

montymxb commented Jul 15, 2017

Uh oh!

benishak commented Jul 15, 2017

Uh oh!

benishak commented Jul 15, 2017

Uh oh!

flovilmart commented Jul 16, 2017

Uh oh!

miguel-s commented Jul 17, 2017

Uh oh!

pungme commented Jul 17, 2017

Uh oh!

benishak commented Jul 17, 2017

Uh oh!

montymxb Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

montymxb commented Jul 17, 2017

Uh oh!

nbering commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pungme commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pungme commented Jul 17, 2017

Uh oh!

nbering commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

montymxb commented Jul 18, 2017

Uh oh!

montymxb commented Jul 18, 2017

Uh oh!

flovilmart Jul 18, 2017

Choose a reason for hiding this comment

Uh oh!

pungme Jul 18, 2017

Choose a reason for hiding this comment

Uh oh!

pungme commented Jul 14, 2017 •

edited by natanrolnik

Loading

codecov bot commented Jul 14, 2017 •

edited

Loading

montymxb Jul 14, 2017 •

edited

Loading

pungme Jul 14, 2017 •

edited

Loading

pungme Jul 14, 2017 •

edited

Loading

nbering commented Jul 17, 2017 •

edited

Loading

pungme commented Jul 17, 2017 •

edited

Loading

nbering commented Jul 17, 2017 •

edited

Loading

pungme commented Jul 18, 2017 •

edited

Loading