Skip to content
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

Proxy and ip decoder #334

Merged
merged 8 commits into from
Dec 16, 2019
Merged

Conversation

getlarge
Copy link
Member

Hey,
I'm using this project very often and i noticed that having a nice ipAddress property might be useful for me and other users ( to use in preConnect hook for example ).
Since it's important to have a correct information, i thought it would be interesting to add a trustProxy property to the broker. Then if aedes is placed behind load balancer / proxy, we could use http headers (x-real-ip / x-forwarded-for ) in case of websocket connection or proxy protocol in case of tcp socket.
So i implemented proxy-protocol-js to test incoming client's buffer and to try retrieving source ip address.

I've been adding an example and some tests.

Please let me know if this feature is useful and not out of the scope of aedes.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks and good work! I've left a few note.

Indeed, it will be a good feature to add.

examples/proxy/index.js Show resolved Hide resolved
lib/client.js Outdated Show resolved Hide resolved
test/connect.js Outdated Show resolved Hide resolved
test/connect.js Outdated Show resolved Hide resolved
lib/client.js Outdated Show resolved Hide resolved
lib/client.js Outdated Show resolved Hide resolved
lib/client.js Outdated Show resolved Hide resolved
…decodeProtocol (by default) if broker.trustProxy & before client.connackSent; update tests and doc
lib/protocol-decoder.js Outdated Show resolved Hide resolved
lib/protocol-decoder.js Outdated Show resolved Hide resolved
lib/protocol-decoder.js Outdated Show resolved Hide resolved
@mcollina
Copy link
Collaborator

Good work!

A couple of things:

  1. CI is failing, can you take a look?
  2. If the proxy packet is split into two separate chunks, would all this logic work?

@getlarge
Copy link
Member Author

Concerning the CI, i noticed that test failures happened with node v12.13.1 and the same happens locally on my computer and they also appear on the master branch.
For now i don't see where to start debugging, but i will dig into it.

For the split, you think of one chunk containing the proxy protocol and the other containing the mqtt packet ? Before the parsing ?
Currently, this is what happened after proxyProtocol.parse, where the "clean" tcp packet is extracted in the data property of the result, which can be sent to mqttPacket.parser.

@mcollina
Copy link
Collaborator

I mean if the proxy protocol header is split into 2 chunks. Are there some guarantees this cannot happen?

@getlarge
Copy link
Member Author

Quote from : https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt
"Both [ proxy protocol v1 and v2] formats are designed to fit in the smallest TCP segment that any TCP/IP host is required to support (576 - 40 = 536 bytes). This ensures that the whole
header will always be delivered at once when the socket buffers are still empty
at the beginning of a connection".

Still it might depend on the proxy configuration i suppose.

There are no guarantee that this cannot happen, so the connection to aedes should be dropped or delayed until the proxy header is complete ?

@mcollina
Copy link
Collaborator

Given on that text, I think this is ok and I was worrying for nothing.

test/basic.js Outdated Show resolved Hide resolved
examples/proxy/package.json Show resolved Hide resolved
lib/client.js Outdated Show resolved Hide resolved
lib/protocol-decoder.js Outdated Show resolved Hide resolved
test/meta.js Outdated Show resolved Hide resolved
test/basic.js Outdated Show resolved Hide resolved
examples/proxy/index.js Outdated Show resolved Hide resolved
test/basic.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants