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

tcp-connection-endpoint.js does not correctly handle the VBus-over-TCP handshake #93

Closed
pdbjjens opened this issue Oct 9, 2023 · 16 comments

Comments

@pdbjjens
Copy link

pdbjjens commented Oct 9, 2023

Current behaviour
Currently the tcp-connection-endpoint.js always answers to the commands CHANNEL and PASS with +OK
In the case of the CHANNEL command, this leads to endless connection request loops if the requested channel is unknown in tcp-connection-endpoint.js
Requested behavior
The CHANNEL command should respond with -ERROR: "Error: Channel not available" if the requested channel is not included in options.channels
The PASS command should respond with -ERROR: "Error: Password mismatch" if the password is not matching. For this password check either the default VBus password "vbus" should be used or a new option options.password should be implemented.
Proposal
The requested behaviour can easily be added to tcp-connection-endpoint.js by extending the code in function processLine() L151 and L163:
NOTE: The password check is performed against the default password here. Of course it would be desirable to add a new option options.password to tcp-connection-endpoint.js and check against that.

            const processLine = function(line) {
                let md;
                if ((md = /^CONNECT (.*)$/.exec(line))) {
                    connectionInfo.viaTag = md [1];
                    callback(null, '+OK');
                } else if ((md = /^PASS (.*)$/.exec(line))) {
                    connectionInfo.password = md [1];
                    if (connectionInfo.password == 'vbus') {
                        callback(null, '+OK');
                   } else {
                       callback(new Error('Password mismatch'));
                   }
               } else if ((md = /^CHANNELLIST$/.exec(line))) {
                    const response = _this.channels.reduce((memo, channel, index) => {
                        if (channel) {
                            memo.push('*' + index + ':' + channel);
                        }
                        return memo;
                    }, []).join('\r\n');

                    callback(null, response + '\r\n+OK');
                } else if ((md = /^CHANNEL (.*)$/.exec(line))) {
                    connectionInfo.channel = md [1];
                    let channelAvailable = false;
                    for (let key in _this.channels) {
                        const channel = _this.channels[key];
                        if (channel && (key == connectionInfo.channel)) {
                            channelAvailable = true;
                            break;
                        }
                    }
                     if (channelAvailable) {
                        callback(null, '+OK');
                   } else {
                       callback(new Error('Channel not available'));
                   }
               } else if ((md = /^QUIT$/.exec(line))) {
                    callback(null, '+OK', false);
                } else if ((md = /^DATA$/.exec(line))) {
                    callback(null, '+OK', true);
                } else {
                    callback(new Error('Unknown command'));
                }
            };
@danielwippermann
Copy link
Owner

Hi @pdbjjens !

Thanks for that suggestion! I'll take a look at this and get back to you!

Best regards, Daniel

@danielwippermann
Copy link
Owner

Hi @pdbjjens !

I have adapted your suggestion. Thanks again!

Best regards,
Daniel

@pdbjjens
Copy link
Author

Hello @danielwippermann
Thank you for your quick response and the prompt realization of the requested functionality. This will fix a problem with an endless loop in my implementation of a serial-to-tcp vbus gateway adapter for ioBroker. https://github.com/pdbjjens/ioBroker.vbus-gw
(Btw. this adapter is based on your example serial-to-tcp.js. Of course I included credits to you and also your copyright and license note in the ioBroker adapter. I hope that is alright with you.)

Alas, due to some other priorities I will not be able to test the new functionality until mid November. Anyway, I will come back to you here to report on the outcome of my tests.

@danielwippermann
Copy link
Owner

HI @pdbjjens !

(Btw. this adapter is based on your example serial-to-tcp.js. Of course I included credits to you and also your copyright and license note in the ioBroker adapter. I hope that is alright with you.)

Yeah, that's perfectly fine for me!

Have a nice weekend, Daniel

@pdbjjens
Copy link
Author

@danielwippermann
Hi Daniel,
I finally managed to test your modifications to tcp-connection-endpoint.js to correctly handle the VBus-over-TCP handshake which you published with v0.28.0.
It works great in my environment. Thank you very much for that.

However, I have one issue regarding how the password is passed to tcp-connection-endpoint.js:
Right now you pass the password to be checked in the options object. This means that it is the same for all connections and channels. I think it might be better to pass the password in options.channels. Thus you could have a different password for each channel, which might be needed if you have multiple serial vbus'es connected to the vbus-gateway (serial-to-tcp) which not necessarily have the same password.

Currently I use a workaround in my vbus-gateway by requiring all vbus passwords to be the same. However I would appreciate it very much if you could implement a "password-per-channel" in one of your next releases.

Best Regards, Jens

@danielwippermann
Copy link
Owner

danielwippermann commented Jan 19, 2024

Hi @pdbjjens !

Thanks for the suggestion. But putting the password into the channels does not really work, since the VBus-over-TCP protocol requires to send the password first and the channel later since the order of commands it pretty much set in stone.

But I have an alternative suggestion: a new constructor option verifyPassword which is an async function that gets executed instead of comparing a fixed password directly. And that verifyPassword function could then alter the channels array to a matching set of channels that the user corresponding to the password may have access to.

Basically something like this:

const endpoint = new TcpConnectionEndpoint({
  async verifyPassword(password) {
    if (password === 'secret') {
      this.channels = [ 'VBusSecret1' ];
    } else if (password === 'password') {
      this.channels = [ 'VBusPassword1' ];
    } else {
      throw new Error('Password mismatch');
  },

  channels: [],
});

Best regard, Daniel

EDIT: changed initial passwordVerifier naming to verifyPassword

@pdbjjens
Copy link
Author

@danielwippermann
Thank you for your proposal. While thinking about the ramifications of my request to support different passwords for each of the vbus'es of the vbus-gateway the question occurred to me: how realistic is this idea? For instance, does a DL3 when working as a vbus-gw allow different passwords for each connected vbus? If not, I would not implement this in the vbus-gateway adapter either.
Greetings, Jens

@danielwippermann
Copy link
Owner

Hi @pdbjjens !

For instance, does a DL3 when working as a vbus-gw allow different passwords for each connected vbus? If not, I would not implement this in the vbus-gateway adapter either.

No, a single DL3 uses the same password for all its VBus channels.

I have prepared a backwards-compatible extension to the TcpConnectionEndpoint class that adds verifyViaTag, verifyPassword, verifyChannel and verifyDataMode callbacks to be able to react to the CONNECT, PASS, CHANNEL and DATA commands and their parameters respectively and be able to reject that input (e.g. after looking up credentials in a database or denying DATA command without a password).

I'll land it on a feature branch today for further discussion / testing.

Best regards,
Daniel

@danielwippermann
Copy link
Owner

@pdbjjens
Copy link
Author

@danielwippermann
Dear Daniel, thank you for your effort to provide a solution to my proposal. However, after looking into this problem and your solution more closely, my feeling is that I will stick with the single password for all channels. I do not really see an impelling use case which would warrant the effort to put the logic for connection management into the gateway. I think my gateway adapter does not need to be "smarter" than a DL3 ;-). I hope you don't mind if I will not use and test the async callbacks in your feature branch.

Nevertheless I dare to propose another addition to tcp-connection-endpoint.
Currently tcp-connection-endpoint silently negotiates the connections with the requesting tcp devices and only issues an event when the connection is established. If an error occurs during the negotiation due to wrong password or non-existant channel the gateway gets no knowledge about these errors. I think it might be useful to issue an error event in case that a connection is denied so that the gateway is informed about the reason for the denial and can at least log these error events.

Cheers, Jens

@danielwippermann
Copy link
Owner

Hi @pdbjjens !

Great suggestion. But I won't name it error, since that name is treated specially and might trigger program-wide unhandledError messages. But I'll model something to behave like Node.js's own connectionAttemptFailed event: https://nodejs.org/dist/latest-v21.x/docs/api/net.html#event-connectionattemptfailed

Best regards,
Daniel

@danielwippermann
Copy link
Owner

Hi @pdbjjens !

I have added the event in 54f2f93

Best regards,
Daniel

@pdbjjens
Copy link
Author

Hi @danielwippermann
Wow, again that was a superfast response. It is really a pleasure to work with you.
I am pleased to tell that my ioBroker vbus gateway now works as expected:

2024-01-21 10:29:39.406 - info: vbus-gw.0 (6424) starting. Version 0.0.4 in C:/iob-dev/ioBroker.vbus-gw/.dev-server/default/node_modules/iobroker.vbus-gw, node: v18.16.0, js-controller: 5.0.12
2024-01-21 10:29:39.433 - info: vbus-gw.0 (6424) listen port: 7053
2024-01-21 10:29:39.434 - info: vbus-gw.0 (6424) discovery port: 80
2024-01-21 10:29:39.436 - debug: vbus-gw.0 (6424) serialPortsTab: [{"path":"COM1","channel":0,"baudrate":9600,"vbusPassword":"1234"},{"path":"COM5","channel":1,"baudrate":9600,"vbusPassword":"vbus"}]
2024-01-21 10:29:39.437 - info: vbus-gw.0 (6424) serial port path: COM1
2024-01-21 10:29:39.438 - info: vbus-gw.0 (6424) serial port channel: 0
2024-01-21 10:29:39.439 - info: vbus-gw.0 (6424) serial port baudrate: 9600
2024-01-21 10:29:39.439 - info: vbus-gw.0 (6424) vbus password: 1234
2024-01-21 10:29:39.440 - info: vbus-gw.0 (6424) serial port path: COM5
2024-01-21 10:29:39.441 - info: vbus-gw.0 (6424) serial port channel: 1
2024-01-21 10:29:39.442 - info: vbus-gw.0 (6424) serial port baudrate: 9600
2024-01-21 10:29:39.442 - info: vbus-gw.0 (6424) vbus password: vbus
2024-01-21 10:29:39.444 - info: vbus-gw.0 (6424) Opening serial port COM1...
2024-01-21 10:29:39.496 - info: vbus-gw.0 (6424) Opening serial port COM5...
2024-01-21 10:29:39.645 - info: vbus-gw.0 (6424) Opening TCP endpoint...
2024-01-21 10:29:39.725 - info: vbus-gw.0 (6424) Channels: ["VBus 0: COM1","VBus 1: COM5"]
2024-01-21 10:29:39.777 - info: vbus-gw.0 (6424) Starting discovery web service...
2024-01-21 10:29:39.817 - info: vbus-gw.0 (6424) Starting discovery broadcast service...
2024-01-21 10:29:39.835 - info: vbus-gw.0 (6424) Waiting for connections...
2024-01-21 10:30:16.972 - debug: vbus-gw.0 (6424) Connection request from 192.168.1.102 with password 1234 ...
2024-01-21 10:30:16.987 - info: vbus-gw.0 (6424) Negotiated connection with 192.168.1.102 for channel 1...
2024-01-21 10:30:16.987 - debug: vbus-gw.0 (6424) Select serial port "COM5"...
2024-01-21 10:30:16.987 - info: vbus-gw.0 (6424) Accepted connection with 192.168.1.102
2024-01-21 10:30:33.764 - info: vbus-gw.0 (6424) Rejected connection with 192.168.1.154 due to Error: Channel not available...
2024-01-21 10:31:22.369 - debug: vbus-gw.0 (6424) Connection request from 192.168.1.154 with password 1234 ...
2024-01-21 10:31:22.400 - info: vbus-gw.0 (6424) Negotiated connection with 192.168.1.154 for channel 1...
2024-01-21 10:31:22.400 - debug: vbus-gw.0 (6424) Select serial port "COM5"...
2024-01-21 10:31:22.400 - info: vbus-gw.0 (6424) Accepted connection with 192.168.1.154
2024-01-21 10:32:14.541 - info: vbus-gw.0 (6424) Closing connection to 192.168.1.154
2024-01-21 10:32:23.660 - info: vbus-gw.0 (6424) Rejected connection with 192.168.1.154 due to Error: Password mismatch...

Thank you again for your excellent support. I am looking forward to your publishing the connectionAttemptFailed event modification on npm, since I can only include the resol-vbus library if it is available on npm.

Cheers, Jens

@danielwippermann
Copy link
Owner

Hi @pdbjjens !

Thank you for the kind feedback!

The version 0.29.0 has been published to the NPM registry:

https://www.npmjs.com/package/resol-vbus/v/0.29.0

Best regards,
Daniel

@pdbjjens
Copy link
Author

Hi @danielwippermann
Thank you for publishing the modification on npm so quickly.
I tried to install it and it works just perfectly with my ioBroker vbus-gw. Thanks again for your great support.

However, I have one more question regarding your example serial-to-tcp gateway.
For the vbus device discovery it identifies as DL2 although in principle it could support more than one channel. An application e.g the resol service center would try to connect with the gw on channel 0. If the gw is configured for more than one vbus, the other channels would not be selectable by the application. Wouldn't it make sense if the gw would identify as DL3?
Since I have borrowed the code from your example, my vbus-gw also identifies as DL2 although it supports up to 3 vbus'es.
If you would recommend to identify the vbus-gw as DL3, what would be the dummy id data for a DL3? I mean the follwing web reply content:

const webReplyContent = [
    'vendor = "RESOL"',
    'product = "DL2"',
    'serial = "001E66000000"',
    'version = "2.1.0"',
    'build = "201311280853"',
    'name = "DL2-001E66000000"',
    'features = "vbus,dl2"',

And one more question regarding the DL3 in general.
Afaik the DL3 channel 0 (the directly connected sensors) were not accessible via tcp on DL3Channel0 and thus were also not supported by your resol-vbus lib. Is this still true?

Have a good day, Jens

@danielwippermann
Copy link
Owner

Hi @pdbjjens !

Thanks, good catch! I changed that in commit d176d65 to (partially) emulate a DL3.

Afaik the DL3 channel 0 (the directly connected sensors) were not accessible via tcp on DL3Channel0 and thus were also not supported by your resol-vbus lib. Is this still true?

Yes, afaik that still is true. Channel 0 is part of the downloadable recorded data, but is not accessible as a "live" bus using VBus-over-TCP.

Best regards, Daniel

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

No branches or pull requests

2 participants