Audit log entry for login event captures wrong ip address

[knolleary]

Current Behavior

Logged into a project on ff cloud. Noticed the audit log entry contains a 192.168.... address which is not very helpful

Expected Behavior

The IP address should reflect the user logging in - not some internal ip address.

As Node-RED is providing the log entry content we will need to raise an upstream issue.

Steps To Reproduce

No response

Environment

• FlowForge version: 0.4
• Node-RED: 2.2.2

[knolleary]

@Steve-Mcl please take a look at this for 0.7. Given the IP address in question is coming from NR, there may be something we need to modify in how NR captures the IP address. There are usually http headers that can be used to get the original ip address of the client.

[Steve-Mcl]

I was looking into / testing this yesterday. We are deriving the IP from...

msg.ip = req.ip || (req.headers && req.headers['x-forwarded-for']) || (req.connection && req.connection.remoteAddress) || undefined;

... in the (node-red source).

Reading many articles, many pointed to this small lib (request-ip) where the user ip is determined by the following order...

1. X-Client-IP
2. X-Forwarded-For (Header may return multiple IP addresses in the format: "proxy 1 IP, proxy 2 IP, client IP, ", so we take the the last one.) //equivalent of express req.ip
3. CF-Connecting-IP (Cloudflare)
4. Fastly-Client-Ip (Fastly CDN and Firebase hosting header when forwared to a cloud function)
5. True-Client-Ip (Akamai and Cloudflare)
6. X-Real-IP (Nginx proxy/FastCGI)
7. X-Cluster-Client-IP (Rackspace LB, Riverbed Stingray)
8. X-Forwarded, Forwarded-For and Forwarded (Variations of Login updates #2)
9. appengine-user-ip (Google App Engine)
10. req.connection.remoteAddress
11. req.socket.remoteAddress
12. req.connection.socket.remoteAddress
13. req.info.remoteAddress
14. Cf-Pseudo-IPv4 (Cloudflare fallback)
15. request.raw (Fastify)

However, as many articles (example) state, to reliably get the client IP you need to add app.set('trust proxy', true) to the express init code. The express docs for this go into a little more detail.

I suspect in order to maximise the chance of capturing the correct client IP we will need a combination of the above.

@hardillb @knolleary with your knowledge of the cloud architecture, could you advise please?

[Steve-Mcl]

@knolleary a direct question for you please (node-red related)

There are 2 approaches and I believe the best chance of success would be to implement both, but partly hidden (disabled by default) in settings js.

1. Enable app.set('trust proxy', trustProxy) if trustProxy has a value set in settings.js

   • trustProxy: null null/undefined/missing ==> no change / same as now NR v2
   • trustProxy: true ==> app.set('trust proxy', true / false)
     • // if true, the client’s IP address is understood as the left-most entry in the X-Forwarded-For header.
     • // If false, the app is understood as directly facing the client and the client’s IP address is derived from req.socket.remoteAddress. This is the default setting.
   • trustProxy: 'loopback' ==> app.set('trust proxy', 'loopback') // specify a single subnet
   • trustProxy: 'loopback, 123.123.123.123' ==> app.set('trust proxy', 'loopback, 123.123.123.123') // specify a subnet and an address
   • trustProxy: '' 'loopback, linklocal, uniquelocal' ==>app.set('trust proxy', 'loopback, linklocal, uniquelocal') // specify multiple subnets as CSV
   • trustProxy: ['loopback', 'linklocal', 'uniquelocal'] ==> app.set('trust proxy', ['loopback', 'linklocal', 'uniquelocal']) // specify multiple subnets as an array
   • trustProxy: () => { (ip) => { return ip === '127.0.0.1' } ==> // function - Custom trust implementation.

2. Add request-ip as a dependency to node-red

   • This will replace the current "get client IP" logic in node-red) .
   • Reasoning : as a dependency, any improvements to client IP detection will be gained without changes to node-red

Please let me know your thoughts on this approach

[Steve-Mcl]

After some testing, I am moving away from adding trustProxy setting and using the existing mechanism of settings.httpServerOptions . We can do this in FF today e.g...

    httpServerOptions: { 
        trustProxy: true
    },

or

    httpServerOptions: { 
        trustProxy: 'whatever,whatever2'
    },

As for request=ip package - this is working well as a middleware & gives us the benefit of future enhancements without NR modification.

That leaves the questions...

1. Should the use of request-ip package be optional (i.e. behind a setting in settings.js)
   • IMO - no, just use it as is.
2. For FF what would we use for trustProxy

[hardillb]

We should just use trustProxy: true then we don't have to worry about the IP address of the proxy and use the exisitng httpServerOptions in the settings.js (we would modify the template in the nr-launcher

[Steve-Mcl]

Thanks for the reply @hardillb but i am having a proper "eats shoots and leaves" moment

Can I paraphrase you to make sure I understand?...

"We should just use trustProxy: true via the existing httpServerOptions and not bother with the request-ip package" - Correct?

[hardillb]

Yes, There shouldn't be a need for an extra package iirc

[Steve-Mcl]

@hardillb ok, thanks for confirming.

I will go with this for now.

I do think the minimal client IP detection logic we have in node-red ATM would benefit from this small package but we will see.

We can always revisit 👍

[Steve-Mcl]

PR ready for review: FlowFuse/nr-launcher#45