Add Windows support#102
Conversation
TODOs: __main__.py : make the "if WINDOWS" checks nicer; somehow fix fork support (multiprocessing? struggling soo far); maybe enhance the tool with DNS NRPT support - it would be nice for INC split configurations win.py : maybe move the transformation between IPvXAddresses and PS argument here? powershell.py : do smarter support for PS detection (PS7 is called pwsh.exe); add comment regarding the "we keep a hidden powershell running in the background and feed him commands over STDIN; interlaving them with echo NULL; and reading the JSON-serialized responses back over STDOUT, delimted by NULL" idea; improve error handling if possible (without risking deadlock by reading just STDIN and not STDOUT) -- probably parse the output of each command separately from its "echo $?" status check The idea is looking for collabolators and testers. credits for original windows-port attempt go to https://github.com/bersbersbers
| class NrptProvider: | ||
| @abstractmethod | ||
| def add_nrtp(self, namespace, servers): | ||
| """NRPT Rule addition. |
There was a problem hiding this comment.
Is it NRPT or NRTP? And what is it?
|
|
||
| @abstractmethod | ||
| def lock_hosts_file(self, hostf): | ||
| """Lock the hosts file.""" |
There was a problem hiding this comment.
Why is this method abstract, rather than using the POSIX implementation (fcntl.flock(hostf, fctnl.LOCK_EX)?
| class PythonOsProcessProvider(ProcessProvider): | ||
| def kill(self, pid, signal=SIGTERM): | ||
| os.kill(pid, signal) | ||
|
|
||
| def pid(self): | ||
| return os.getpid() | ||
|
|
||
| def is_alive(self, pid): | ||
| try: | ||
| os.kill(pid, 0) | ||
| return True | ||
| except ProcessLookupError: | ||
| return False |
There was a problem hiding this comment.
How about put this in generic.py, rather than creating a new portable.py? (And let's just call it OsProcessProvider rather than add the seemingly-redundant Python to the name.)
|
|
||
| def do_connect(env, args): | ||
| global providers | ||
| global platform |
There was a problem hiding this comment.
I don't understand this. platform is already a global, imported with from sys import platform
| # erase all addresses (needed on Windows to clean-up the adapter) | ||
| # TODO: also cleanup DNS? | ||
| # TODO: also cleanup routes? | ||
| providers.route.remove_address(env.tundev) |
There was a problem hiding this comment.
What is the purpose of removing the preexisting addresses from the VPN adapter when connecting?
From OpenConnect development, we already know that if another (down) adapter is using the same address… we have to clean it up before we can assign it to the VPN adapter. In v9.0, we'll do this automatically: https://gitlab.com/openconnect/openconnect/-/blob/master/tun-win32.c#L228
But I don't know what issue is being solved by removing the previously assigned addresses when connecting. 🤷♂️
| ad=orig_netaddr, nm=env.netmask, nml=env.netmasklen, nmi=env.network.netmask)) | ||
| assert env.network.netmask == env.netmask | ||
| # first/lowest IP-addr in the range should be internal GW (seen in Windows vpnc-script.js) | ||
| env.via = next(env.network.hosts()) |
There was a problem hiding this comment.
https://gitlab.com/openconnect/vpnc-scripts/-/commit/b3dec790951444d5e5f5c6659ca8b51569207b03 🤔
- This won't work if
env.networkis /32.n=ipaddress.ip_network('192.168.12.25/32'); next(n.hosts())→StopIteration. - If
env.networkisn't /32, then in order to matchvpnc-script-win.js, it should be the second host IP address, not the first.
How about this?
from itertools import islice
lowest_hosts = islice(env.network.hosts(), 0, 2)
env.via = lowest_hosts[1] if len(lowest_hosts) == 2 else env.network.network_address| def initGlobalProviders(): | ||
| global providers | ||
|
|
||
| if "provider" in globals(): | ||
| return | ||
|
|
||
| # Set platform-specific providers | ||
| providers = slurpy() | ||
| for pn, pv in get_default_providers().items(): | ||
| try: | ||
| if isinstance(pv, Exception): | ||
| raise pv | ||
| providers[pn] = pv() | ||
| except Exception as e: | ||
| print("WARNING: Couldn't configure {} provider: {}".format(pn, e), file=stderr) | ||
|
|
||
| def main(args=None, environ=os.environ): | ||
| global providers | ||
|
|
||
| try: | ||
| p, args, env = parse_args_and_env(args, environ) | ||
|
|
||
| # Set platform-specific providers | ||
| providers = slurpy() | ||
| for pn, pv in get_default_providers().items(): | ||
| try: | ||
| if isinstance(pv, Exception): | ||
| raise pv | ||
| providers[pn] = pv() | ||
| except Exception as e: | ||
| print("WARNING: Couldn't configure {} provider: {}".format(pn, e), file=stderr) | ||
|
|
There was a problem hiding this comment.
I don't understand why this is being changed.
2 participants
Closes #41