From ceb09ce99ad080ae804d322804b22e3e8f0be005 Mon Sep 17 00:00:00 2001 From: webzwo0i Date: Thu, 1 Oct 2020 11:39:01 +0200 Subject: [PATCH] security: Support proxy with rate limiting and include CI test coverage for nginx rev proxy (#4373) Previously Etherpad would not pass the correct client IP address through and this caused the rate limiter to limit users behind reverse proxies. This change allows Etherpad to use a client IP passed from a reverse proxy. Note to devs: This header can be spoofed and spoofing the header could be used in an attack. To mitigate additional *steps should be taken by Etherpad site admins IE doing rate limiting at proxy.* This only really applies to large scale deployments but it's worth noting. --- .travis.yml | 12 +++++++++ src/node/handler/PadMessageHandler.js | 5 ++-- src/package-lock.json | 10 ++++++++ src/package.json | 3 ++- tests/ratelimit/Dockerfile.anotherip | 4 +++ tests/ratelimit/Dockerfile.nginx | 2 ++ tests/ratelimit/nginx.conf | 26 +++++++++++++++++++ tests/ratelimit/send_changesets.js | 25 +++++++++++++++++++ tests/ratelimit/testlimits.sh | 36 +++++++++++++++++++++++++++ 9 files changed, 120 insertions(+), 3 deletions(-) create mode 100644 tests/ratelimit/Dockerfile.anotherip create mode 100644 tests/ratelimit/Dockerfile.nginx create mode 100644 tests/ratelimit/nginx.conf create mode 100644 tests/ratelimit/send_changesets.js create mode 100644 tests/ratelimit/testlimits.sh diff --git a/.travis.yml b/.travis.yml index cf7307501..2fab80ceb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -99,6 +99,18 @@ jobs: - "npm install -g etherpad-load-test" script: - "tests/frontend/travis/runnerLoadTest.sh" + - name: "Test rate limit" + install: + - "docker network create --subnet=172.23.42.0/16 ep_net" + - "docker build -f Dockerfile -t epl-debian-slim ." + - "docker build -f tests/ratelimit/Dockerfile.nginx -t nginx-latest ." + - "docker build -f tests/ratelimit/Dockerfile.anotherip -t anotherip ." + - "docker run -p 8081:80 --rm --network ep_net --ip 172.23.42.1 -d nginx-latest" + - "docker run --name etherpad-docker -p 9000:9001 --rm --network ep_net --ip 172.23.42.2 -e 'TRUST_PROXY=true' epl-debian-slim &" + - "docker run --rm --network ep_net --ip 172.23.42.3 --name anotherip -dt anotherip" + - "./bin/installDeps.sh" + script: + - "cd tests/ratelimit && bash testlimits.sh" notifications: irc: diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 1094fe837..9e0e349ce 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -181,10 +181,11 @@ exports.handleMessage = async function(client, message) var env = process.env.NODE_ENV || 'development'; if (env === 'production') { + const clientIPAddress = remoteAddress[client.id]; try { - await rateLimiter.consume(client.handshake.address); // consume 1 point per event from IP + await rateLimiter.consume(clientIPAddress); // consume 1 point per event from IP }catch(e){ - console.warn("Rate limited: ", client.handshake.address, " to reduce the amount of rate limiting that happens edit the rateLimit values in settings.json"); + console.warn("Rate limited: ", clientIPAddress, " to reduce the amount of rate limiting that happens edit the rateLimit values in settings.json"); stats.meter('rateLimited').mark(); client.json.send({disconnect:"rateLimited"}); return; diff --git a/src/package-lock.json b/src/package-lock.json index 59f124154..356b7455e 100644 --- a/src/package-lock.json +++ b/src/package-lock.json @@ -1810,6 +1810,16 @@ "resolved": "https://registry.npmjs.org/etag/-/etag-1.8.1.tgz", "integrity": "sha1-Qa4u62XvpiJorr/qg6x9eSmbCIc=" }, + "etherpad-cli-client": { + "version": "0.0.9", + "resolved": "https://registry.npmjs.org/etherpad-cli-client/-/etherpad-cli-client-0.0.9.tgz", + "integrity": "sha1-A+5+fNzA4EZLTu/djn7gzwUaVDs=", + "dev": true, + "requires": { + "async": "*", + "socket.io-client": "*" + } + }, "etherpad-require-kernel": { "version": "1.0.9", "resolved": "https://registry.npmjs.org/etherpad-require-kernel/-/etherpad-require-kernel-1.0.9.tgz", diff --git a/src/package.json b/src/package.json index 24eeb53de..18f42c395 100644 --- a/src/package.json +++ b/src/package.json @@ -81,7 +81,8 @@ "set-cookie-parser": "^2.4.6", "superagent": "^3.8.3", "supertest": "4.0.2", - "wd": "1.12.1" + "wd": "1.12.1", + "etherpad-cli-client": "0.0.9" }, "engines": { "node": ">=10.13.0", diff --git a/tests/ratelimit/Dockerfile.anotherip b/tests/ratelimit/Dockerfile.anotherip new file mode 100644 index 000000000..5b9d1d21a --- /dev/null +++ b/tests/ratelimit/Dockerfile.anotherip @@ -0,0 +1,4 @@ +FROM node:alpine3.12 +WORKDIR /tmp +RUN npm i etherpad-cli-client +COPY ./tests/ratelimit/send_changesets.js /tmp/send_changesets.js diff --git a/tests/ratelimit/Dockerfile.nginx b/tests/ratelimit/Dockerfile.nginx new file mode 100644 index 000000000..ba8dd358f --- /dev/null +++ b/tests/ratelimit/Dockerfile.nginx @@ -0,0 +1,2 @@ +FROM nginx +COPY ./tests/ratelimit/nginx.conf /etc/nginx/nginx.conf diff --git a/tests/ratelimit/nginx.conf b/tests/ratelimit/nginx.conf new file mode 100644 index 000000000..97f0a9e00 --- /dev/null +++ b/tests/ratelimit/nginx.conf @@ -0,0 +1,26 @@ +events {} +http { + server { + access_log /dev/fd/1; + error_log /dev/fd/2; + location / { + proxy_pass http://172.23.42.2:9001/; + proxy_set_header Host $host; + proxy_pass_header Server; + # be careful, this line doesn't override any proxy_buffering on set in a conf.d/file.conf + proxy_buffering off; + proxy_set_header X-Real-IP $remote_addr; # http://wiki.nginx.org/HttpProxyModule + proxy_set_header X-Forwarded-For $remote_addr; # EP logs to show the actual remote IP + proxy_set_header X-Forwarded-Proto $scheme; # for EP to set secure cookie flag when https is used + proxy_set_header Host $host; # pass the host header + proxy_http_version 1.1; # recommended with keepalive connections + # WebSocket proxying - from http://nginx.org/en/docs/http/websocket.html + proxy_set_header Upgrade $http_upgrade; + proxy_set_header Connection $connection_upgrade; + } + } + map $http_upgrade $connection_upgrade { + default upgrade; + '' close; + } +} diff --git a/tests/ratelimit/send_changesets.js b/tests/ratelimit/send_changesets.js new file mode 100644 index 000000000..cf0377cae --- /dev/null +++ b/tests/ratelimit/send_changesets.js @@ -0,0 +1,25 @@ +try{ + var etherpad = require("../../src/node_modules/etherpad-cli-client"); + //ugly +} catch { + var etherpad = require("etherpad-cli-client") +} +var pad = etherpad.connect(process.argv[2]); +pad.on("connected", function(){ + + setTimeout(function(){ + setInterval(function(){ + pad.append("1"); + }, process.argv[3]); + },500); // wait because CLIENT_READY message is included in ratelimit + + setTimeout(function(){ + process.exit(0); + },11000) +}); +// in case of disconnect exit code 1 +pad.on("message", function(message){ + if(message.disconnect == 'rateLimited'){ + process.exit(1); + } +}) diff --git a/tests/ratelimit/testlimits.sh b/tests/ratelimit/testlimits.sh new file mode 100644 index 000000000..778348dcc --- /dev/null +++ b/tests/ratelimit/testlimits.sh @@ -0,0 +1,36 @@ +#!/usr/bin/env bash + +#sending changesets every 101ms should not trigger ratelimit +node send_changesets.js http://127.0.0.1:8081/p/BACKEND_TEST_ratelimit_101ms 101 +if [[ $? -ne 0 ]];then + echo "FAILED: ratelimit was triggered when sending every 101 ms" + exit 1 +fi + +#sending changesets every 99ms should trigger ratelimit +node send_changesets.js http://127.0.0.1:8081/p/BACKEND_TEST_ratelimit_99ms 99 +if [[ $? -ne 1 ]];then + echo "FAILED: ratelimit was not triggered when sending every 99 ms" + exit 1 +fi + +#sending changesets every 101ms via proxy +node send_changesets.js http://127.0.0.1:8081/p/BACKEND_TEST_ratelimit_101ms 101 & +pid1=$! + +#sending changesets every 101ms via second IP and proxy +docker exec anotherip node /tmp/send_changesets.js http://172.23.42.1:80/p/BACKEND_TEST_ratelimit_101ms_via_second_ip 101 & +pid2=$! + +wait $pid1 +exit1=$? +wait $pid2 +exit2=$? + +echo "101ms with proxy returned with ${exit1}" +echo "101ms via another ip returned with ${exit2}" + +if [[ $exit1 -eq 1 || $exit2 -eq 1 ]];then + echo "FAILED: ratelimit was triggered during proxy and requests via second ip" + exit 1 +fi