openv / vcontrold

:fire: vcontrold Daemon for control and logging of Viessmann® type heating devices
https://github.com/openv/openv/wiki
GNU General Public License v3.0
102 stars 55 forks source link

Restrict daemon to localhost #100

Closed corsac-s closed 2 years ago

corsac-s commented 2 years ago

Hi,

by default the vcontrold daemon will listen on 0.0.0.0:3002. I have to admit I'm unsure why there's a need for a daemon at all, but it's not really a good idea to have it listen on all IPs by default. Would it make sense to restrict it to localhost so not everyone on the current LAN (or even outside if routable IP is used) can talk to the optolink interface through vcontrold?

l3u commented 2 years ago

You can change that if you want? I mean, no harm meant – someone running this will know what he does – no?

corsac-s commented 2 years ago

You can change that if you want? I mean, no harm meant – someone running this will know what he does – no?

I might have missed something obvious then. How do you restrict the daemon to localhost for example (I'm guessing in vcontrold.xml but I couldn't find a documentation for all the fields and the provided example doesn't talk about host restrictions.

l3u commented 2 years ago

Oh, I didn't remember that correctly. You're right, one can as of now only define a port, not an IP address. I thought you could simply change 0.0.0.0 to e.g. 127.0.0.1 or 192.168.1.10 or such.

Being able to do so would actually be a meaningful feature.

l3u commented 2 years ago

Thinking it through a bit further, it's actually also a very good question why we do this server-client stuff at all …

My personal use-case for vcontrold is: It runs on a Raspberry Pi 1 which is attached via a homemade serial optolink adapter. Each five minutes (via cron), it gathers the data I want to know, processes it and presents it as HTML via lighttpd.

So the only thing the server does is to wait that vclient queries it from 127.0.0.1 in a five-minute-interval. Could as well do it directly, without a server …

corsac-s commented 2 years ago

I have the exact same use case and the same feeling as you (a simple client and a shared library to implement the protocol would work), but I guess some people do the data collection or the home automation on a separate, more powerful box and thus separate the clients and the servers.

I think it adds complexity but I guess it was done on purpose. I'm just not comfortable exposing that over the network myself.

l3u commented 2 years ago

Exactly: A meaningful approach would be to move the core functionality out of the server and – as you said – to have a shared library implementing it. This way, a simple (serverless) direct-query program could be implemented, for people like you and me (I really think this is the most common use-case). And the server could still exist – but only with the code needed to do the TCP/IP communication stuff. For the rest, it could use the shared library as well.

Sadly, this is beyond my (strictly speaking non-existant ;-) C programming skills … I only was the one cleaning up the code back then, formatting it properly and kicking out the old build system in favor of cmake. I never touched the code itself (or have a deeper insight in what it actually does).

Question is if it would not be better to implement such an approach from scratch (maybe in C++?). This way, one could also rework the (imo questionable) XML configuration (like having an INI style config for the server if there would be one and something more convenient like JSON for the commands definition) …

l3u commented 2 years ago

@speters Do you think such a change (only speaking of the shared library) would be meaningful and doable?

speters commented 2 years ago

Restriction of the interface vcontrold tries to bind to is basically possible with a few lines of code like shown in https://github.com/openv/vcontrold/pull/101

But I am unsure if it is worth the effort for this old piece of software, as it is flawed in so many places (with the configuration/datapoint address problems above all).

l3u commented 2 years ago

Wow, that was fast! If it doesn't break existing setups and works, I think this should be added.

However, I would leave the default config's bind tag empty, with the comment that if it stays empty, the socket will bind to all addresses (and e.g. with "set to 127.0.0.1 to only allow connections from localhost" or such). This way, we would keep the current default behavior.

Do you mean you doubt it's worth the effort to add this, or to split up communication and server? If you mean the latter, I also think that possibly a new implementation only (re-)using the communication logic maybe would be better.

corsac-s commented 2 years ago

But I am unsure if it is worth the effort for this old piece of software, as it is flawed in so many places (with the configuration/datapoint address problems above all).

I had the impression that this software was the reference one, but if there's another existing software which is recommended these days, I'd be happy for a pointer.

l3u commented 2 years ago

I can only say that I've been using vcontrold since 2015, 24/7, and it "just works". When you have set it up correctly, all the commands you need for your heating etc.

When one would do a rework/rewrite, I think that nowadays, one would do it in a different way, esp. speaking of the configuration (and also of stuff like mixing English and German in the code etc.).

I'd stick to vcontrold, at least, I didn't find anything better. Maybe, you want to try out the changes @speters added and see if this works for you?

speters commented 2 years ago

New feature merged into master. Use --listen/-L option to specify an address (e.g. 127.0.0.1 or localhost) to listen on. It can also be specified in the xml config.

ATTN: Still no new binary releases, no version bump as there is some work needed to move from Travis CI to Github workflow.