bf: ARSN-57 log correct client ip

lib/policyEvaluator/requestUtils.js

@@ -7,8 +7,9 @@ const ipCheck = require('../ipCheck');
  * @return {string} - returns client IP from the request
  */
 function getClientIp(request, s3config) {
-    const clientIp = request.socket.remoteAddress;
     const requestConfig = s3config ? s3config.requests : {};
+    const remoteAddress = request.socket.remoteAddress;
+    const clientIp = requestConfig ? remoteAddress : request.headers['x-forwarded-for'] || remoteAddress;
     if (requestConfig) {
         const { trustedProxyCIDRs, extractClientIPFromHeader } = requestConfig;
         /**

lib/s3routes/routes.js

@@ -10,6 +10,8 @@ const routeOPTIONS = require('./routes/routeOPTIONS');
 const routesUtils = require('./routesUtils');
 const routeWebsite = require('./routes/routeWebsite');
 
+const requestUtils = require('../../lib/policyEvaluator/requestUtils');
+
 const routeMap = {
     GET: routeGET,
     PUT: routePUT,
@@ -67,7 +69,8 @@ function checkBucketAndKey(bucketName, objectKey, method, reqQuery,
     return undefined;
 }
 
-function checkTypes(req, res, params, logger) {
+// TODO: ARSN-59 remove assertions or restrict it to dev environment only.
+function checkTypes(req, res, params, logger, s3config) {
     assert.strictEqual(typeof req, 'object',
         'bad routes param: req must be an object');
     assert.strictEqual(typeof res, 'object',
@@ -114,6 +117,9 @@ function checkTypes(req, res, params, logger) {
     });
     assert.strictEqual(typeof params.dataRetrievalFn, 'function',
         'bad routes param: dataRetrievalFn must be a defined function');
+    if (s3config) {
+        assert.strictEqual(typeof s3config, 'object', 'bad routes param: s3config must be an object');
+    }
 }
 
 /** routes - route request to appropriate method
@@ -134,9 +140,10 @@ function checkTypes(req, res, params, logger) {
  *  values for whether queries are supported
  * @param {function} params.dataRetrievalFn - function to retrieve data
  * @param {RequestLogger} logger - werelogs logger instance
+ * @param {String} [s3config] - s3 configuration
  * @returns {undefined}
  */
-function routes(req, res, params, logger) {
+function routes(req, res, params, logger, s3config) {
     checkTypes(req, res, params, logger);
 
     const {
@@ -150,7 +157,7 @@ function routes(req, res, params, logger) {
     } = params;
 
     const clientInfo = {
-        clientIP: req.socket.remoteAddress,
+        clientIP: requestUtils.getClientIp(req, s3config),
         clientPort: req.socket.remotePort,
         httpCode: res.statusCode,
         httpMessage: res.statusMessage,

tests/unit/policyEvaluator/requestUtils.js

@@ -12,8 +12,7 @@ describe('requestUtils.getClientIp', () => {
     const testClientIp2 = '192.168.104.0';
     const testProxyIp = '192.168.100.2';
 
-    it('should return client Ip address from header ' +
-        'if the request comes via proxies', () => {
+    it('should return client Ip address from header if the request comes via proxies', () => {
         const request = new DummyRequest({
             headers: {
                 'x-forwarded-for': [testClientIp1, testProxyIp].join(','),
@@ -28,13 +27,9 @@ describe('requestUtils.getClientIp', () => {
         assert.strictEqual(result, testClientIp1);
     });
 
-    it('should not return client Ip address from header ' +
-        'if the request is not forwarded from proxies or ' +
-        'fails ip check', () => {
+    it('should return client Ip address from socket info if the request is not forwarded from proxies', () => {
         const request = new DummyRequest({
-            headers: {
-                'x-forwarded-for': [testClientIp1, testProxyIp].join(','),
-            },
+            headers: {},
             url: '/',
             parsedHost: 'localhost',
             socket: {
@@ -45,12 +40,11 @@ describe('requestUtils.getClientIp', () => {
         assert.strictEqual(result, testClientIp2);
     });
 
-    it('should not return client Ip address from header ' +
-        'if the request is forwarded from proxies, but the request ' +
+    it('should not return client Ip address from header if the request is forwarded from proxies, but the request ' +
         'has no expected header or the header value is empty', () => {
         const request = new DummyRequest({
             headers: {
-                'x-forwarded-for': ' ',
+                'x-forwarded-for': '',
             },
             url: '/',
             parsedHost: 'localhost',
@@ -61,4 +55,37 @@ describe('requestUtils.getClientIp', () => {
         const result = requestUtils.getClientIp(request, configWithProxy);
         assert.strictEqual(result, testClientIp2);
     });
+
+    it('should return client Ip address from header if the request comes via proxies and ' +
+        'no request config is available', () => {
+        const request = new DummyRequest({
+            headers: {
+                'x-forwarded-for': testClientIp1,
+            },
+            url: '/',
+            parsedHost: 'localhost',
+            socket: {
+                remoteAddress: testProxyIp,
+            },
+        });
+        const result = requestUtils.getClientIp(request, configWithoutProxy);
+        assert.strictEqual(result, testClientIp1);
+    });
+
+    it('should return client Ip address from socket info if the request comes via proxies and ' +
+        'request config is available and ip check fails', () => {
+        const dummyRemoteIP = '221.10.221.10';
+        const request = new DummyRequest({
+            headers: {
+                'x-forwarded-for': testClientIp1,
+            },
+            url: '/',
+            parsedHost: 'localhost',
+            socket: {
+                remoteAddress: dummyRemoteIP,
+            },
+        });
+        const result = requestUtils.getClientIp(request, configWithProxy);
+        assert.strictEqual(result, dummyRemoteIP);
+    });
 });