New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catching the PROXY protocol error #11

Closed
artmatsak opened this Issue May 13, 2015 · 23 comments

Comments

Projects
None yet
2 participants
@artmatsak
Copy link

artmatsak commented May 13, 2015

We have a Node.js application behind ELB that uses proxywrap in strict mode. The application crashes intermittently with the following error:

events.js:87
      throw Error('Uncaught, unspecified "error" event.');
            ^
Error: Uncaught, unspecified "error" event.
    at Error (native)
    at Socket.emit (events.js:87:13)
    at net.js:459:14
    at process._tickCallback (node.js:355:11)

We have a strong suspicion that this is caused by non-PROXY requests somehow hitting the server because we get the same error if we run proxywrap in strict mode locally and then hit it with a non-PROXY request.

Now, how can we catch that error event, apparently emitted by the socket object here, and prevent it from crashing the whole application?

@cusspvz

This comment has been minimized.

Copy link
Member

cusspvz commented May 13, 2015

Hi @jamix2 ,

can you please provide a chunk of you source-code where you're implementing findhit-proxywrap?

We are using it on our production servers without any problems until date, also on strict mode.
That doesn't seems to come from findhit-proxywrap, since any falsy message that cames from it will be converted into 'PROXY protocol error' here. (such as undefined, null and so on)

We also have a test unit covering the case you're providing.

Which version of findhit-proxywrap are you using?

Hope i can help you, production errors are an headache... x)

@artmatsak

This comment has been minimized.

Copy link
Author

artmatsak commented May 13, 2015

Thank you for a prompt reply @cusspvz! We are using version 0.3.8. Here is an ultra minimal application to reproduce the issue:

var http = require("http");
var proxywrap = require('findhit-proxywrap');

var proxied_http = proxywrap.proxy(http, { strict: true });
var http_server = proxied_http.createServer();

var port = 8080;
http_server.listen(port, function() {
  console.log("server listening on port " + port);
});

Run the application locally and then enter localhost:8080 in the browser. You will see the following in the console:

$ node strict-test.js 
server listening on port 8080
events.js:87
      throw Error('Uncaught, unspecified "error" event.');
            ^
Error: Uncaught, unspecified "error" event.
    at Error (native)
    at Socket.emit (events.js:87:13)
    at net.js:451:14
    at process._tickCallback (node.js:355:11)

As you can see, there is no mention of 'PROXY protocol error'. The question is, can this kind of error be caught in our code and acted upon?

@cusspvz

This comment has been minimized.

Copy link
Member

cusspvz commented May 14, 2015

Strange... let me dig on it.

@cusspvz

This comment has been minimized.

Copy link
Member

cusspvz commented May 14, 2015

Well, just fixed it on #12 with an option called ignoreStrictExceptions.
Defaults to false, you should set it to true.

I'm merging it now and bumping version so you could deploy it to your production servers.

Thanks!! :)

@cusspvz cusspvz closed this in #12 May 14, 2015

@artmatsak

This comment has been minimized.

Copy link
Author

artmatsak commented May 14, 2015

Thank you for addressing this. If I understand it correctly, ignoreStrictExceptions will make proxywrap discard non-PROXY requests without terminating the app when in strict mode, which is great.

That said, would it be possible to enable listening for the error event so we can see where the non-PROXY requests are coming from? That would leave processing of the protocol error to the client code, which would either terminate the app or log the error, hopefully along with the origin IP.

After all, it's really weird to be getting those non-PROXY requests from behind ELB.

@cusspvz

This comment has been minimized.

Copy link
Member

cusspvz commented May 14, 2015

If I understand it correctly, ignoreStrictExceptions will make proxywrap discard non-PROXY requests without terminating the app when in strict mode, which is great.

ignoreStrictExceptions will destroy socket without specifying exception, that will prevent node from running .emit( 'error', exception ) internally, said that, it won't launch an uncaught exception in case you are not listening for socket.on( 'error', ... ).

That said, would it be possible to enable listening for the error event so we can see where the non-PROXY requests are coming from?

Exactly, but for that you should keep ignoreStrictExceptions false.

After all, it's really weird to be getting those non-PROXY requests from behind ELB.

That never happened with us, since our production SG are configured to allow only connections from ELB. Maybe you should do the same. :)

Thank you for addressing this.

You're welcome. We are open to issues since we can improve this module to avoid future problems on our app, thats why open-source rocks!! :)

If you want to hear more about our modules, follow me or keep in touch with findhit organization. :)

@artmatsak

This comment has been minimized.

Copy link
Author

artmatsak commented May 14, 2015

That said, would it be possible to enable listening for the error event so we can see where the non-PROXY requests are coming from?

Exactly, but for that you should keep ignoreStrictExceptions false.

OK, so if we do keep ignoreStrictExceptions false, where exactly do we attach the event listener to catch that error?

That never happened with us, since our production SG are configured to allow only connections from ELB. Maybe you should do the same. :)

We do, too, as per Elastic Beanstalk's default configuration. That's what's strange about it.

If you want to hear more about our modules, follow me or keep in touch with findhit organization. :)

Done!

@cusspvz

This comment has been minimized.

Copy link
Member

cusspvz commented May 14, 2015

We do, too, as per Elastic Beanstalk's default configuration. That's what's strange about it.

I was thinking about it for a while, could it be related somehow to instance monitoring service?

where exactly do we attach the event listener to catch that error?

By listening for socket connection after server initialization.

server.on( 'connection', function ( socket ) {
  socket.on( 'error', function ( err ) {
    // ...
  });
});

If I was with your scenario in hands, i would prefer to ignore proxywrap caused exceptions (thats why i've fixed it up by adding an option) and find out why you're receiving non-proxy-based connections.

Notice that even if ignoreStrictExceptions you could listen for other errors.

If you're interested on gathering errors from production servers, i would propose you to use snitcher, which is also developed by us and used in production on findhit. :)

If you want to hear more about our modules, follow me or keep in touch with findhit organization. :)

Done!

Thanks! :)

@artmatsak

This comment has been minimized.

Copy link
Author

artmatsak commented May 14, 2015

We do, too, as per Elastic Beanstalk's default configuration. That's what's strange about it.

I was thinking about it for a while, could it be related somehow to instance monitoring service?

Yes, that's what I'm inclined to think as well. We'll poke around.

where exactly do we attach the event listener to catch that error?

By listening for socket connection after server initialization.

Ah, beautiful! We added the listener and it looks like we won't even need the ignoreStrictExceptions setting because the PROXY errors are now caught and the app doesn't terminate.

In the error handler, though, the socket object is already in a destroyed state so it's impossible to get the remote address for logging purposes. Are there any solutions to this you are aware of?

@cusspvz

This comment has been minimized.

Copy link
Member

cusspvz commented May 14, 2015

In the error handler, though, the socket object is already in a destroyed state so it's impossible to get the remote address for logging purposes. Are there any solutions to this you are aware of?

As the connection isn't initialized with PROXY Protocol header, it won't replace remoteAddress and remotePort even if you have overrideRemote set as true. Although you should be able to get and log which IP is coming on socket even if it is on a destroyed state. What is socket.remoteAddress returning?

@artmatsak

This comment has been minimized.

Copy link
Author

artmatsak commented May 14, 2015

What is socket.remoteAddress returning?

undefined :( I do a dump of the whole socket object in the error handler and the remoteAddress property isn't even there.

@cusspvz

This comment has been minimized.

Copy link
Member

cusspvz commented May 14, 2015

Could you please try the same with overrideRemote set as false? Just to have sure...

@artmatsak

This comment has been minimized.

Copy link
Author

artmatsak commented May 15, 2015

Could you please try the same with overrideRemote set as false? Just to be sure...

That has no effect unfortunately.

@cusspvz

This comment has been minimized.

Copy link
Member

cusspvz commented May 15, 2015

You're using http right?
Are you accessing req.socket.remoteAddress?

@artmatsak

This comment has been minimized.

Copy link
Author

artmatsak commented May 15, 2015

Here is the updated minimal code:

var http = require("http");
var proxywrap = require('findhit-proxywrap');

var proxied_http = proxywrap.proxy(http, {
  strict: true,
  overrideRemote: false
});
var http_server = proxied_http.createServer();

var port = 8080;
http_server.listen(port, function() {
  console.log("server listening on port " + port);
});

http_server.on('connection', function (socket) {
  socket.on('error', function (err) {
    console.log(socket.remoteAddress);
  });
});

Upon launching the server and hitting it with a non-PROXY request on port 8080, the error handler outputs undefined for socket.remoteAddress.

@cusspvz

This comment has been minimized.

Copy link
Member

cusspvz commented May 15, 2015

Don't know why, but inspite of closures are my last resource, they are an awesome resource! 💃

Hope this can help you to find the culprit:

var http = require("http");
var proxywrap = require('findhit-proxywrap');

var proxied_http = proxywrap.proxy(http, {
  strict: true,
  overrideRemote: false
});
var http_server = proxied_http.createServer();

var port = 8080;
http_server.listen(port, function() {
  console.log("server listening on port " + port);
});

http_server.on('connection', function (socket) {
  var ip = socket._getpeername().address;
  socket.on('error', function (err) {
    console.log( 'error: blame ' + ip);
  });
});
@artmatsak

This comment has been minimized.

Copy link
Author

artmatsak commented May 15, 2015

Of course! Store all the necessary socket info on the connection stage and reuse it in the error handler. Sometimes we have to be reminded of the basics :)

Thank you for all your help, I think we are now well-equipped to find out what is causing the issue!

@cusspvz

This comment has been minimized.

Copy link
Member

cusspvz commented May 15, 2015

You're welcome, glad to help! Cheers ;)

@artmatsak

This comment has been minimized.

Copy link
Author

artmatsak commented May 22, 2015

We now have a strong feeling that the intermittent PROXY protocol errors are related to #13. We'd like to get the latest goodies into production, including the #13 fix and .header in the error object for better error reporting. Can we expect a version tag for those soon please?

@cusspvz

This comment has been minimized.

Copy link
Member

cusspvz commented May 22, 2015

Done! :)

@artmatsak

This comment has been minimized.

Copy link
Author

artmatsak commented May 22, 2015

Thank you!

@artmatsak

This comment has been minimized.

Copy link
Author

artmatsak commented Jun 10, 2015

Just a quick update - that indeed was #13. After we updated to 0.3.10, the odd PROXY protocol errors were gone for good. Thanks!

@cusspvz

This comment has been minimized.

Copy link
Member

cusspvz commented Jun 10, 2015

I'm happy for that! You're welcome! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment