Not working at all on node 0.12.2 about proxywrap

[cusspvz]

Hi @AbdelMat-CP
Did you tested with npm test?

[cusspvz]

MacBook-Pro-de-JM:findhit-proxywrap josemoreira$ n 0.12.2

     install : node-v0.12.2
       mkdir : /usr/local/n/versions/node/0.12.2
       fetch : https://nodejs.org/dist/v0.12.2/node-v0.12.2-darwin-x64.tar.gz
   installed : v0.12.2

MacBook-Pro-de-JM:findhit-proxywrap josemoreira$ node -v
v0.12.2
MacBook-Pro-de-JM:findhit-proxywrap josemoreira$ npm test

> findhit-proxywrap@0.3.8 test /Users/josemoreira/GitHub/findhit/findhit-proxywrap
> node node_modules/mocha/bin/mocha --globals setImmediate,clearImmediate --check-leaks --colors -t 10000 --reporter spec $(find test/* -name '*.test.js')



  PROXY Protocol regexp tests
    UNKNOWN
      ✓ localhost 
      ✓ ignore things uppon UNKNOWN 
      ✓ should fail if anything is behind UNKNOWN 
    TCP4
      ✓ basic 
      ✓ star.findhit.com example 
    TCP6
      ✓ localhost 
      ✓ FE80:0000:0000:0000:0202:B3FF:FE1E:8329 

  PROXY Protocol v1
    net
      ✓ Check socket is stablished correctly 
      ✓ Check with another socket parameters 
      ✓ Check with another socket parameters as a string 
      Should detect a malformed PROXY headers
        ✓ Header without IP's 
        ✓ non-proxy connection when in non-strict mode should not be destroyed #7 
        ✓ Restore emitted events after socket.destroy #5 


  13 passing (42ms)

Tests are passing trough 0.12.2, can you show up how are you implementing it?

[AbdelMat-CP]

sure !

Here is an extract of our code :

//...

var express = require('express');
var https = require('https');
var proxywrap = require('proxywrap');

var proxiedHttps = proxywrap.proxy(https, { strict: false });

//...

var app = express();

//...

app.get('/call/:script/:method', function (req, res) {
var msg = { script : req.params.script, method : req.params.method };
// This gives only the elb private ip...
console.log('connection remote ip :' + req.connection.remoteAddress);
// This gives also only the elb private ip...
console.log('socket remote ip :' + req.socket.remoteAddress);
dummy.DoStuff(msg, function (output) {
res.json(output);
});
});

//...

var server = proxiedHttps.createServer(app).listen(443, function () {
console.log('Endpoint HTTPS listening on port 443...');
});

tested with express 3.x on both node 0.10.28 and 0.12.2, works fine with 0.10, but fails (no public ip retrieved) in 0.12.2...

Thank you.

[AbdelMat-CP]

And yes tested with npm test on both version of node, and tests passes OK... but tests does not cover the retrieving of the 'remoteAddress' prop behind connection or socket object no ?

[cusspvz]

but tests does not cover the retrieving of the 'remoteAddress' prop behind connection or socket object no ?

I doesn't cover remoteAddress because it is optional, internally we use clientAddress.
Could it be because you're using https? Could you please test with http instead?

We currently use ELB ssl feature for providing https support to our instances which are running on top of http server (its more reliable because you don't have the ssl bottleneck on your instances CPUs, that allows faster responses from your app).

It would be great if you could create a test case where it fails so we can fix that together. :)

[cusspvz]

Just added tests for remoteAddress checking and it seems fine on v0.12.2.

[AbdelMat-CP]

Could it be because you're using https? Could you please test with http instead?

Yes it seems to work on http with 0.12.2, it's very troubling... is it something with object socket when on http vs tlsSocket when on https in the lib ? can you confirm that it doesn't work with https ? unfortunately we cannot change the creation of the server over https to use http...

Thank you.

[AbdelMat-CP]

So just to be clear enough, the 'req.connection.remoteAddress' (i.e. socket.remoteAddress or in this case tlsSocket.remoteAddress ?) does not seem to be overridden when used on https and node 0.12.2...

[cusspvz]

But you still got access to socket.clientAddress ?

[cusspvz]

As my last commit states, i've added some room for providing https and spdy tests in the future.
If you could spend some time playing around tests and make minor changes so it can provide a fully functional test suite for https, it would be great!!1

I had in mind that this was also testing http, but i was wrong, now tests are covering http as also.

[cusspvz]

@AbdelMat-CP did you got any results?

[AbdelMat-CP]

Sorry, but i probably don't understand what you want me to do ? is it doing some tests with https ? as i already said, we are trying to use your module with an https interface, and it seems to be not compatible with it on node 0.12.2 but works fine on 0.10.28.
to reproduce the case, just take the previous given sample code, and run it on a vm/instance/machine with node 0.12.2 behind a proxy protocol enabled lb... you will immediately see that the caller public ip is not retrieved (lb private ip instead...)

Thank you.

[cusspvz]

Sorry, but i probably don't understand what you want me to do ?

I've added support for http, but since i didn't had enough time, so I'm asking you if you would mind to spend some of yours trying to figure out how to support https testing.

That effort would allow us to dig on why https is failing on 0.12.2, actually we avoid to work on things that aren't being tested.

to reproduce the case, just take the previous given sample code, and run it on a vm/instance/machine with node 0.12.2 behind a proxy protocol enabled lb

I could reproduce that case but it would be easier if tests were already supporting https.

Due to my lack of time, and this isn't an emergency for our team, I will try to support https if you couldn't get into it, thats my last case scenario. 😞

Thank you for helping! Hope you can contribute to findhit-proxywrap. 😄

[AbdelMat-CP]

Hello,
Just to share with all users of this module, in case the same problem happens, we found a solution without modifying the lib, but in fact it seems to be a bad hack.

In HTTPS just do somethig like this to retrieve the overridden 'remoteAddress' :

var socket = req.socket._parent || req.socket;
ipAddress = socket.remoteAddress;

My knowledge of Javascript/node can't help anymore to explain why this works, but it works !

Thanks.

[cusspvz]

In HTTPS just do somethig like this to retrieve the overridden 'remoteAddress' :

Hi @AbdelMat-CP ,

you have an option on this lib to avoid remoteAddress override:

var http = require( 'findhit-proxywrap' ).proxy( require( 'http' ), { overrideRemote: false });

Thanks! :)

[AbdelMat-CP]

Hi,
Yes i know, but in fact 'clientAddress' and 'proxyAddress' are also available only via req.socket._parent and not via req.socket on HTTPS mode.

[cusspvz]

Hmm, makes sense, since socket is wrapped on a tls stream.
I will let this issue opened so i can study this case as soon as i have some free time.

[EyePulp]

We use the https module as well (node is our SSL terminator).

Following on with what @AbdelMat-CP already mentioned, we were not getting the proper remoteAddress in 0.12.0, but it was solved under 0.12.5 using his/her code:

var socket = req.socket._parent || req.socket;
var ipAddress = socket.remoteAddress;

Something obviously changed, but req.socket._parent seems to contain the proxywrap modified remoteAddress. Note we did try this fix under 0.12.0, but it didn't work.

Maybe this isn't really something proxywrap can fix (without a lot of hacks), but the behavior could definitely be documented as a special case for dealing with the https module, and making sure to use a node version >= 0.12.2 ?

[cusspvz]

@EyePulp could you please provide a test case?

[doubliez]

Hi,

I started using proxywrap recently but encountered the same issue with Node 6.3.1 trying to wrap the https module. In order to get the correct remoteAddress I indeed had to access the parent socket at req.socket._parent.

However I also needed to use proxywrap together with spdy (https://github.com/indutny/node-spdy) configured to use SSL. Same problem here as well, but I couldn't apply the same solution since req.socket._parent was undefined with spdy.

Digging into the req.socket object, I found a way to get the initial socket on which proxywrap has set the correct remoteAddress, though it's quite hacky:

var socket = req.socket._spdyState.parent._parent;
var ip = socket.remoteAddress;

I think this should definitely be documented somewhere, or fixed in a different way so that req.socket.remoteAddress contains the correct ip when doing SSL termination in Node and not at the ELB.

This can certainly help people trying to use proxywrap with https or spdy :)

[cusspvz]

spdy support added at v0.3.12
Please re-open in case you find this issue again. Thanks.