Jayich-Lab / tray-launcher

A launcher for Windows that resides in the taskbar for managing .bat scripts.
MIT License
7 stars 1 forks source link

Port being accidentally connected by other applications; safety #35

Closed danhuaxu closed 2 years ago

fanmingyu212 commented 2 years ago

I think it should default to a number within 1000 - 9999 that is not used (https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml might be helpful). The user can set an env variable that overrides the default port.

fanmingyu212 commented 2 years ago

If you are thinking about safety in the aspect of some other application may connect to the opened port, and accidentally run/stop some shell scripts, I think it is pretty unlikely.

To avoid this problem, encryption is likely needed which is too much for such a program to go into. The port should probably only accept connections from the local computer though (I believe you can choose an host/interface when opening the port to only listen from localhost).

jayich commented 2 years ago

yeah... let's not get bogged down in high-level security. Just make clear notes about what may be problematic, and move on. If there are simple preventative measures, great, implement, but avoid time consuming fixes.

danhuaxu commented 2 years ago

I think it should default to a number within 1000 - 9999 that is not used (https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml might be helpful). The user can set an env variable that overrides the default port.

I don't know how to set an env variable (I will look it up!) but if that means hard-coding a port number to the program, I am a little concerned: different computers will surely have different ports available.

There is a way to let the OS pick an available port for us, and that's what I am using right now. But the issue is that the client won't know what this port number is... https://github.com/Jayich-Lab/tray-launcher/issues/34

danhuaxu commented 2 years ago

If you are thinking about safety in the aspect of some other application may connect to the opened port, and accidentally run/stop some shell scripts, I think it is pretty unlikely.

To avoid this problem, encryption is likely needed which is too much for such a program to go into. The port should probably only accept connections from the local computer though (I believe you can choose an host/interface when opening the port to only listen from localhost).

yeah... let's not get bogged down in high-level security. Just make clear notes about what may be problematic, and move on. If there are simple preventative measures, great, implement, but avoid time consuming fixes.

Okay, I will get the basic functions down first and worry about these later.

Yes, I have set to use a port that only listens to the localhost.

jayich commented 2 years ago

It is unclear you should worry about security features ~ever. We just don't have the ability to support security issues outsid our local network.

fanmingyu212 commented 2 years ago

See how labrad handles this: https://github.com/labrad/pylabrad/blob/4a1f3bbc2e6fcb979f91296351d03355b8f88863/labrad/protocol.py#L519 gets ports and other constants from https://github.com/labrad/pylabrad/blob/4a1f3bbc2e6fcb979f91296351d03355b8f88863/labrad/constants.py.

os.environ may be helpful.

danhuaxu commented 2 years ago

See how labrad handles this: https://github.com/labrad/pylabrad/blob/4a1f3bbc2e6fcb979f91296351d03355b8f88863/labrad/protocol.py#L519 gets ports and other constants from https://github.com/labrad/pylabrad/blob/4a1f3bbc2e6fcb979f91296351d03355b8f88863/labrad/constants.py.

os.environ may be helpful.

Awesome, it's good to have reference that comes directly from our lab. Thank you!

danhuaxu commented 2 years ago

@fanmingyu212 Hey Mingyu, I checked how the labrad handles constants, and just want to confirm with you if my understanding is correct: in https://github.com/labrad/pylabrad/blob/4a1f3bbc2e6fcb979f91296351d03355b8f88863/labrad/constants.py, for example, MANAGER_PORT = int(os.environ.get('LABRADPORT', 7682)) is referring to a system-wise environment variable, which is visible to all programs. If an environment variable named LABRADPORT exists, the above code will return whatever value being stored in this env variable. But if such env variable doesn't exist, it will return the default value 7682.

So I created an env variable called TRAY_LAUNCHER_PORT with value 7689 (this port is available on the computer miltiades in 1702A) by manually going into the control panel. For the time being, this works, but I am uncertain how I am going to do this on other computers - Do you know how labrad automatically sets its system env variables so they show up in the control panel?

A bigger concern is that the port I am using on miltiades right now is possibly unavailable on other computers. The issue is that I am still hard-coding the port number and I doubt if setting such an env variables helps, unless we ask the end user to change it once they find out that 7689 is not available on their computers. If the Python program could modify system env variables, this could be easily solved (setting os.environ[TRAY_LAUNCHER_PORT] = available_port_number_assigned_by_OS in my server program wouldn't affect my client program, the scope being an issue).

I am afraid that I don't understand your suggestion correctly so the above might just all be nonsense. But any thoughts?

fanmingyu212 commented 2 years ago

I think your understanding is correct. labrad uses LABRADPORT env variable if it exists, and otherwise uses 7682.

With your example, the program will open a port at TRAY_LAUNCHER_PORT (or 7689 if the env variable is not defined). Commands gets the port number with the same way (if TRAY_LAUNCHER_PORT is defined it tries to connect to it, and otherwise 7689).

I think it is fine for the end user to change the port number if the port number is occupied. It just needs to show the end user that the port is not available. I am not sure the best way to do it, but one simple way is to display a disabled action in the menu saying "Port xxxx is not available". Maybe a better way to do it is to show a modified tray icon (design idea could be similar to this) if the port is occupied, and have a tooltip (https://doc.qt.io/qt-5/qsystemtrayicon.html#toolTip-prop) that tells the user port xxxx is not available.

For a random port number, the chance that it is occupied is fairly low (my computer has ~15 ports used between 1000 to 9999 right now). The default port of labrad has never been occupied on any computer I worked on yet, so the chance of the default port blocked is pretty low. As the user can change the port to any number, I don't think this will be an issue at all.

An alternative to env variables is to have a command line argument --port when starting tray_launcher.

Using an available port and saving it in a file does not seem like the best approach to me. Accidental removing the file could cause a problem, for example.

fanmingyu212 commented 2 years ago

7684-7686 could be good - https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml?search=7684

danhuaxu commented 2 years ago

Displaying a warning is reasonable. Cool.

Ok, I will take 7686.

And another question. Is it possible to create a system environment variable with a python program? How did labrad create those env variables in the control panel?

fanmingyu212 commented 2 years ago

It is possible for a python program to create system environment variables, but I think there may not be a python stdlib method to do that. I believe one needs to use subprocess to run a windows command to create the environment variable - generally a python program should not modify the environment variables so there is not a simple function to do that.

LabRAD does not create those env variables (the users need to create them manually). That is why they have to use a default value for ports (such as https://github.com/labrad/pylabrad/blob/4a1f3bbc2e6fcb979f91296351d03355b8f88863/labrad/constants.py#L54).

danhuaxu commented 2 years ago

So should the tray launcher do that? Creating a system env variable so if our default port is occupied, the user can switch to another port slightly more easily?

Having an additional --port just looks a bit cumbersome to me because you will need it everytime you run a command.

fanmingyu212 commented 2 years ago

I don't think the tray launcher should create an environment variable. Probably we only need to include a short paragraph in the readme about the port and environment variable.

I agree --port is cumbersome and a default port that can be overriden by env variable is probably the best way.

The last command in https://stackoverflow.com/a/59489965/4696323 describes how to set an env variable in python if you are interested (which I don't think is the way to go).

danhuaxu commented 2 years ago

Ok I will definitely mention the port number we are using in the README. If this eventually becomes a real concern, we can add the line as a last resort. Thank you!