Protocol decoder not always working with TCP proxy

[getlarge]

There is still an issue in the the protocol decoder when Aedes broker is used behind a TCP proxy which handles SSL termination.
The packet is splitted in two, on one side the protocol proxy header and the TCP buffer on the other.
Any help to solve this would be appreciated.

Originally posted by @getlarge in https://github.com/moscajs/aedes/issues/338#issuecomment-577841457

[robertsLando]

@mcollina Any suggestion for this?

[mcollina]

I think this was solved in #334. Or maybe not, I do not have time to dig into this. Please send a PR if interested.

[robertsLando]

@mcollina @getlarge Is the author of that PR so I think this is not fixed

[robertsLando]

@getlarge Could you create an example to reproduce this bug?

[getlarge]

Sure, but it's not so simple to reproduce.
It would require to start an app with Aedes + MQTT client and have Nginx or HAProxy running with some SSL certs.
Should i provide a docker-compose file for this ?

[robertsLando]

I think a docker compose would be fine :)

[getlarge]

Now i'm confused, i created a test repo for this case and it seems to work well.
So the issue might be specific to a Nginx configuration point and / or maybe docker-compose networks since i saw this issue in this context.
I will try to figure how to reproduce it.

[robertsLando]

@getlarge Let me know

[getlarge]

@robertsLando So after some more tests, it seems the bug appears "randomly" ( because i don't understand why), that's why i didn't catch it yesterday.
As you can see on the screenshot, sometimes the new connection triggers protocolDecoder once, and some other time it triggers it twice.
It might happen due to the condition in nextBatch ? Because nextBatch would be triggered twice before connack is sent ?

[robertsLando]

@getlarge In latest commit nextBatch call in client init has been removed. Coould you use the version from master to test this again? Just run npm install moscajs/aedes/#master

[getlarge]

I already use the github repo in package.json, and when the image is built all dependencies are installed.
I will try to confirm.

[robertsLando]

@getlarge So you are not using npm install aedes (that installs latest npm version 0.40.1) but are you cloning it from github?

With npm install moscajs/aedes/#master you use latest code available on master instead latest published version

[getlarge]

Yes i cloned it from github to have the decodeProtocol hook.

[robertsLando]

The fix has landed yesterday so if you are sure you are running it from the master now that is not the problem. Let's wait if @mcollina has any other suggestion

[getlarge]

In fact i'm going to rename the issue, since the problem occurs ( it seems less often ) when using a simple TCP connection ( with no SSL ).

[getlarge]

The only trick i found to avoid this issue, is to remove the else condition when no data has been extracted from the broker.decodeProtocol.
So in this case the client sends the connection packet several times ( in fact 2 times, it seems to be constant ) and the connack is only sent when the buffer is composed of the proxy header and the mqtt packet.
Not sure this is the right way to handle this bug, because the drawback is that if you assign broker.trustProxy = true, your client must always be behind a proxy ( or inject proxy header itself ) to handle connection.
Any suggestion ?

[robertsLando]

@getlarge When protocol decoder fails, what is the buffer received?

The only trick i found to avoid this issue, is to remove the else condition when no data has been extracted from the broker.decodeProtocol

Here: https://github.com/moscajs/aedes/blob/master/lib/client.js#L74 ?

[getlarge]

var {data} that's the TCP Buffer (containing only the MQTT packet) extracted without the proxy protocol header and it should be present when a client connects behind a TCP proxy.
What is causing the issue is that data is empty because only the proxy protocol header was received from buf ( even if i don't know why, maybe the size of the client.conn.read buffer ? Or Nginx proxy splitting the buffer ).

In fact after some thoughts and tests, this seems fine to fix the issue

var { data, isProxy } = client.broker.decodeProtocol(client, buf)

if (data) {
  client._parser.parse(data)
} else if (!isProxy) {
  client._parser.parse(buf)
}

And i would like to suggest to change the condition above this block to
if (!client.connackSent && buf)
Like this even clients which are not behind a proxy would also get their connection information in client.connDetails.

Related to this, i'm currently trying to get a reliable source for the proxy server IP to achieve this #338
Then i will send a PR.

[robertsLando]

@gnought Thoughts on this?

[robertsLando]

Like this even clients which are not behind a proxy would also get their connection information in client.connDetails.

@getlarge Shouldn't also clients connected using websockets get them?

[getlarge]

@robertsLando Yes clients using websockets would, ( from socket._socket ) and if broker.trustProxy = true, they would get in priority information from headers set by the proxy server.

[robertsLando]

What is causing the issue is that data is empty because only the proxy protocol header was received from buf ( even if i don't know why, maybe the size of the client.conn.read buffer ? Or Nginx proxy splitting the buffer ).

Could it be a protocol or nginx related behaviour to firstly send a packet with empty data field? Should check specs about them to check if this is normal

About if (!client.connackSent && buf), I upvote this so like you said even clients which are not behind a proxy would also get their connection information in client.connDetails, the client.broker.trustProxy check is already done in protocol-decoder

Anyway this would add a little more complexity to packet parsing, maybe add an option to broker connectionDetails would be better?

connectionDetails: When trustProxy is false this option allows to force protocol parsing to get connection details in preConnect

I think most users doesn't mind to get connection details from their clients

cc @mcollina

[getlarge]

It could be related to the specs, but then why this issue would appear randomly ?
"Sometimes" the MQTT packet and the Proxy header are concatenated in the same buffer and "sometimes" they are separated.
I can't see any clarification in Proxy Protocol spec neither in Nginx Docs.

I tried to reproduce the NGINX proxy behaviour with 2 TCP servers to write a test ( one for the Aedes and one acting as a Proxy ) but the issue never seems to happen.
So yes it might comes from NGINX...

[getlarge]

@robertsLando following the change with protocolDecoder, you forgot to implement the condition changes that fixes this issue.
Unless it was on purpose ?
Also we suggested that client.broker.trustProxy should not need be true to go through broker.protocolDecoder. Should i make a PR for this ?

[robertsLando]

Yes go for it! Daniel

…

On 6 Feb 2020, at 19:34, getlarge ***@***.***> wrote:  @robertsLando following the change with protocolDecoder, you forgot to implement the condition changes that fixes this issue. Unless it was on purpose ? Also we suggested that client.broker.trustProxy should not need be true to go through broker.protocolDecoder. Should i make a PR for this ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[robertsLando]

@getlarge Any news?

[getlarge]

@robertsLando I saw the discussion on #365 and thought it might be smarter to wait until it goes into aedes to bring the change.

[robertsLando]

If you want to join that discussion you are welcome, gnought made some changes in his “next” branch that requires some thoughts about protocol decoder. Daniel

…

On 11 Feb 2020, at 19:32, getlarge ***@***.***> wrote:  @robertsLando I saw the discussion on #365 and thought it might be smarter to wait until it goes into aedes to bring the change. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[robertsLando]

@getlarge Could we close this now?

[getlarge]

yes