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.

Sign up for GitHub

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

Jump to bottom

Added possibility to configure interface bindings #231

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Added possibility to configure interface bindings #231

wants to merge 7 commits into from

Conversation

elluisian
Copy link
Contributor

@elluisian elluisian commented Aug 21, 2024 •
edited
Loading

UPDATE, Monday, February 17th 2025

This commit implements the feature requests reported on the issues #43 and #227.
It is now possible to make the server listen only on certain interfaces, instead of all of them.
This not only increases security, but it also allows the "typical SSH usage", that is, SSH server + public key authentication + local port forwarding + VNC server listening on localhost.

OLD MESSAGE

When applied, this commit tries to implement the feature requests reported on the issues #43 and #227. Basically, with this, it is now possible to make the server listen only on certain addresses. This increases security and allows the typical "SSH usage", that is, SSH + publickey auth + local port forwarding + VNC listening on localhost only.

To achieve this:

  • on droidvnc-ng.c, vncServerStart was modified in order to provide a further "jstring listenIf" parameter. It uses rfbScreenInfo*'s listenInterface property.

  • A TableRow was added in order to allow users to input the desired address to use ("Listening Address").

  • On droidvnc-ng.c, the method vncServerGetListenInterface was introduced in order to track if the server is currenctly set to listen to 0.0.0.0 or not (this is used to properly show what addresses are available to use on the UI).

Please note that, if an invalid address/host is set, by default, it is assumed the address 0.0.0.0 is requested.

@bk138
Copy link
Owner

bk138 commented Aug 22, 2024

I think we should aim for "interface names", not "IP addresses" in the UI. Also, what happens if the interface the server is using the IP of goes down?

@elluisian
Copy link
Contributor Author

elluisian commented Aug 23, 2024 •
edited
Loading

@bk138 Hi, sorry for the late response.
Actually, I think I've used "listening address" as label... not IP address, also, technically, one specifies a single address of an interface to listen to (except for 0.0.0.0 which is a special address which stands for "active addresses of the available interfaces").

As for what happens when the interface is down... I'm sorry, I didn't check for that when I made the fix, I've checked while commenting this though.

I've made the server listening to my wifi IP Address and, if I disable wifi while the server is running, it simply stops (that is, the server gracefully stops and you can restart it, I guess this is the default behaviour you programmed?), if I do the same thing but when listening on localhost, nothing happens, the server continues to listen, same thing if using 0.0.0.0.

@bk138
Copy link
Owner

bk138 commented Aug 27, 2024

What I mean is that we should not bother the user to enter IP addresses in the UI but present choices like "Wifi interface", LAN, WAN etc and handle the device->address resolution under the hood.

@elluisian
Copy link
Contributor Author

elluisian commented Aug 27, 2024 •
edited
Loading

How about a spinner with the current up interfaces plus, the possibility to define a custom IP address?
Something like:

Any
Wifi
Cellular
Loopback
...
Custom IP

It seems XVnc has a -interface option in order to customize the ip address.

@bk138
Copy link
Owner

bk138 commented Aug 27, 2024

Yes, but why would the user want to set a custom IP?

@elluisian
Copy link
Contributor Author

Ehr... eh eh, that's actually a good question... because... why not? :P
I know, stupid answer... the only use-case I can think of is if one wants to use a loopback address which is DIFFERENT FROM 127.0.0.1 (for example, 127.0.0.124).
I understand this is a far-fetched use-case... but still, considering it is relatively easy to make, why not?
All that is needed is to simply make the spinner and an edittext, similar to the one in my commit, only, this time, it is enabled only if "custom ip" is chosen.

@bk138
Copy link
Owner

bk138 commented Aug 27, 2024

Let's keep it as simple as needed: https://en.wikipedia.org/wiki/KISS_principle :-)

@elluisian
Copy link
Contributor Author

Ok, done, now it should be better, let me know what you think.

@elluisian elluisian force-pushed the configurable-network-binding branch from 1a1571c to be567d2 Compare August 27, 2024 19:25
bk138
bk138 requested changes Aug 30, 2024
View reviewed changes
Copy link
Owner

@bk138 bk138 left a comment

Choose a reason for hiding this comment

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

Some comments. Before delving into details, the two grand topics are:

  • what's the strategy when a listen interface goes down while the server is running?
  • what's the strategy when the server is started wit a listen interface that's not up?

If solutions for these are too hard to be found, the details are kinda moot...

@elluisian elluisian force-pushed the configurable-network-binding branch 10 times, most recently from f60d76c to c7845a4 Compare September 1, 2024 20:50
@elluisian elluisian changed the title Implemented configurable listening address Implemented configurable interface bindings Sep 1, 2024
@elluisian elluisian force-pushed the configurable-network-binding branch 2 times, most recently from f133256 to beba90d Compare September 30, 2024 16:51
@elluisian
Copy link
Contributor Author

Waiting for review @bk138, of course, when you have time.

@elluisian elluisian requested a review from bk138 October 4, 2024 09:51
@bk138
Copy link
Owner

bk138 commented Oct 18, 2024

@elluisian I'll try to take a look ASAP - will take some time due to the PR's size.

@freebrowser1
Copy link

So, do I understand this is implemented ?
The user interface does not show any option to select network interface and the possibility to use localhost only, so this VNC is only accessible via an SSH tunnel, which is more secure.
Opening droidVNC in a public wifi is insecure as it is sniffable.

@bk138
Copy link
Owner

bk138 commented Feb 12, 2025

@freebrowser1 No, it's not. I didn't find time for a thorough review yet.

@elluisian
Copy link
Contributor Author

@freebrowser1 Unfortunately not, I'm waiting for it too.
@bk138 Do you need a hand by any chance?
I can rebase some of the commits, since some of these were tries and not actual solutions.

@elluisian elluisian changed the title Implemented configurable interface bindings Added possibility to configure interface bindings Feb 12, 2025
@bk138
Copy link
Owner

bk138 commented Feb 12, 2025

@elluisian thanks for the offer - yes you can rebase, that would help. I have not forgotten about this PR, otherwise I would have closed it. It's simply a matter of urgencies/priorities :-)

@elluisian
res: Added needed text entries for interfaces
176bc32 
- Added the entry "main_activity_settings_listening_interface", used as
  label for the spinner which will determine the used interface;
- Added the entry "main_activity_settings_listenif_spin_any", used as
  label for the spinner entry that indicates the usage of "any interface";
@elluisian
res: Added UI for interface choice
0934fa7 
- Added a section (label + spinner) needed to choose the interface to
  use ("any" included);
@elluisian
Constants+Defaults: Added values for use with IFs
b9e1269 
- Added constant PREFS_KEY_SETTINGS_LISTEN_INTERFACE, to store the
  chosen interface to use;
- Added listenInterface property with value "loopback" to Defaults;
@elluisian
java: Added ifaceutils package
3171d6d 
Added ifaceutils package which contains classes that will be helpful
  in detecting and managing NICs (Network Interface Card).

- INetIfData, NetIfData and NetIfDataDecorator:
These represent a single NIC, the last one can be used to customize the
name of the interface;

- IfCollector:
As the name suggests, this class simply collects all the available
interfaces at once;

- NetworkInterfaceTester:
As the name suggests, this class uses IfCollector and listens for
network changes;
@elluisian
java: Added ListenIfAdapter and another decorator
628679f 
- ListenIfAdapter is an adapter class that will be used with the Spinner
  present in MainActivity in order to choose what interface to use;

- NetIfDataDefaultNameDecorator simply gives friendly name to interfaces
  "loopback" and "any" (these are taken from strings.xml);
@elluisian
Utils: Added methods to retrieve IPs and NICs
63a2afd 
- Added method getIPv4ForInterface(NetworkInterface) and
  getIPv4ForInterface(String) which returns a
  list of ip4 addresses given a NetworkInterface instance/String name of
interface;

- Added method getAvailableNICs(Boolean, Boolean) and getAvailableNICs()
  which return a list of NetworkInterface instances that represents
available NICs on the system;
@elluisian
Main+Boot+droidvnc-nc: Added interface binding
8d49c47 
- droidvnc-ng.c:
Method Java_net_christianbeier_droidvnc_1ng_MainService_vncStartServer
modified to accept a futher "jstring listenIf" parameter.
This way, it is possible to use rfbScreen->listenInterface property to
perform interface binding;

- MainActivity:
Now it makes use of ListenIfAdapter, allowing to change the used
interface (default is loopback);

- MainService:
Now it makes use of NetworkInterfaceTester and IfCollector to detect the
state of the network and stop itself, if needed;

- OnBootReceiver:
Now it correctly detects whether an interface is disabled/non-existent and if so, it
DOESN't start the service, moreover, it sends a notification about the
problem.
@elluisian elluisian force-pushed the configurable-network-binding branch from d37cd48 to 8d49c47 Compare February 16, 2025 23:54
@elluisian
Copy link
Contributor Author

elluisian commented Feb 17, 2025 •
edited
Loading

Hi @bk138 , I've improved the pull request, from 11 commits to just 7, moreover, they are a bit more "organized".
I've also improved some of the code and comments, in particular, I've introduced the ifaceutils package, in order to modularize the classes that relate to NIC detection.
Of course, if there is still some problem, let me know and I will look into it.

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

@bk138 bk138 Awaiting requested review from bk138

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elluisian @bk138 @freebrowser1