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

Support for OpenVPN3 #327

Merged
merged 1 commit into from
Jun 15, 2022
Merged

Support for OpenVPN3 #327

merged 1 commit into from
Jun 15, 2022

Conversation

lstipakov
Copy link
Member

This adds optional support for using OpenVPN3 client as an alternative to openvpn2.

Just replacing one client with another will not work:

 - OpenVPN3 doesn't use interactive service, it uses
"agent" service with completely different protocol. OpenVPN GUI
needs to talk to agent using HTTP and JSON.

 - OpenVPN3 management interface realtime notifications must be
explicitly turned on in order for GUI to work.

To enable using openvpn3, one needs to build openvpn-gui with
./configure --enable-ovpn3 option. This also requires json-c library.

To switch betweet openvpn2 and openvpn3, see "OpenVPN Engine"
radiobutton group in Settings -> Advanced dialog.

Management interface - enabled OpenVPN3 client will be added soon
to openvpn3 repo (along with existing "cli" test client). Also agent
service will be opensourced in near future.

@mattock
Copy link
Member

mattock commented Jan 9, 2020

Finally open source OpenVPN 3 is moving towards becoming a real alternative to OpenVPN 2. With this PR merged there are not that many blockers in front of having an OpenVPN 3-based open source Windows installer. 👍

@cron2
Copy link
Contributor

cron2 commented Jan 9, 2020 via email

@mattock
Copy link
Member

mattock commented Jan 9, 2020

@cron2 technically you're correct. But getting more people to actually use a fully open source version of OpenVPN 3 is a good thing.

In my opinion the "make OpenVPN 3 a real open source project" part took way too long. Based on GitHub discussions (openvpn3, openvpn3-linux) it is finally gaining some traction. Maybe at some point a non-OpenVPN Inc. developer community will also form around it.

@lstipakov
Copy link
Member Author

on Windows, with wintun merged, OpenVPN 2 should be able to do whatever OpenVPN 3 can (and with the same speed)

The keywords here are should and with the same speed.

Unfortunately this is not the case at the moment. When testing against openvpn3 backend with kernel acceleration, I got up to 1.8Gbit/s with openvpn3 client and 1.2Gbit/s with openvpn2 client (both wintun). As you see, the difference is significant.

I haven't looked deeper into that - it should be indeed possible to make openvpn2 up to speed with openvpn3. There must be something what ASIO does better than openvpn2 event loop.

Besides performance, having openvpn3 as an option in community client will give the openvpn3 project larger exposure, which I think it needs.

@mattock
Copy link
Member

mattock commented Apr 16, 2020

@selvanair any thoughts on the actual changes?

@selvanair
Copy link
Collaborator

@selvanair any thoughts on the actual changes?

I haven't got a chance to look at this carefully. Will do.

@mattock
Copy link
Member

mattock commented Aug 12, 2020

@selvanair if you can do this soonish we could get this out in 2.5.0. Optional OpenVPN 3.x support in the "official" OpenVPN GUI version would be nice imho.

@selvanair
Copy link
Collaborator

@selvanair if you can do this soonish we could get this out in 2.5.0. Optional OpenVPN 3.x support in the "official" OpenVPN GUI version would be nice imho.

Everyone is putting me on notice these days, I must be slacking..
I don't have a setup to test this with OpenVPN3 so I can only check that the changes wont break anything. Will do that but I'm not convinced this is ready for merging into master

(i) We won't be distributing the GUI with this enabled in 2.5.0, right?
(ii) Where can one find this "ovpnagent" service referred to here?

Unless we can provide instructions on how to install OpenVPN3 and the agent, I think we could merge this into a development branch and move to master only when ready for community use.

Copy link
Collaborator

@selvanair selvanair left a comment

Choose a reason for hiding this comment

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

I haven't tested with OpenVPN3 (could not find any info on how to build the agent) but here are some comments.
We conditionally check the version of openvpn2 on startup (openvpn.exe --version) which is probably okay as this assumes OpenVPN2 is installed even when the engine is set to OpenVPN3.

manage.c Outdated
if (o.ovpn_engine == OPENVPN_ENGINE_OVPN3)
{
ManagementCommand(c, "log on all", NULL, regular);
ManagementCommand(c, "state on all", NULL, regular);
Copy link
Collaborator

@selvanair selvanair Aug 13, 2020

Choose a reason for hiding this comment

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

We do log on all and state on in OnReady(), so is this required? Looks like this would have no effect except to add some delay.

openvpn.c Outdated Show resolved Hide resolved
@@ -1928,28 +1988,44 @@ StartOpenVPN(connection_t *c)
/* Try to open the service pipe */
if (!IsUserAdmin() && InitServiceIO (&c->iserv))
{
Copy link
Collaborator

@selvanair selvanair Aug 13, 2020

Choose a reason for hiding this comment

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

If run as admin or if InitServiceIO() fails, we will fall back to starting openvpn2 directly irrespective of the engine setting. It would be better to error out in that case.

More as a side note: The admin check was added when we used to support vista because of an obscure named pipe bug that could have led to a privilege escalation. With vista no longer supported, we should probably get rid of this restriction.

res/openvpn-gui-res-en.rc Outdated Show resolved Hide resolved
@facboy
Copy link
Contributor

facboy commented May 22, 2022

this seems to have atrophied somewhat. it merges on to master relatively easily (some things have been added to the Advanced dialog so it needs moving around), but it doesn't actually work against openvpn3 3.8-master (ie built off github master). ovpnagent logs a PAGE NOT FOUND error.

EDIT: turns out PAGE NOT FOUND is because i disabled OPENVPN_AGENT_START_PROCESS based on a comment on another issue. now ovpnagent successfully receives the request from openvpn-gui but it still doesn't work. openvpn-gui logs nothing in the status window and gives up after a bit.

@facboy
Copy link
Contributor

facboy commented May 23, 2022

ok, turns out the part that launches omiclient does not quote some of the file arguments, so on my system this results in it failing to launch.

Copy link
Contributor

@facboy facboy left a comment

Choose a reason for hiding this comment

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

apart from the indentation lgtm. i suspect there are some line-ending differences with my branch but i'm not sure if that's because there are incorrect line-endings in my branch, or on this branch.

openvpn.c Outdated

/* Ignore pushed route-method when service is in use */
const wchar_t* extra_options = L" --pull-filter ignore route-method";
size += wcslen(extra_options);

_sntprintf_0(startup_info, L"%ls%lc%ls%ls%lc%.*hs", c->config_dir, L'\0',
Copy link
Contributor

Choose a reason for hiding this comment

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

the indentation on these lines is wrong now.

@facboy
Copy link
Contributor

facboy commented Jun 13, 2022

lgtm

This adds optional support for using OpenVPN3 client
as an alternative to openvpn2.

Just replacing one client with another will not work:

 - OpenVPN3 doesn't use interactive service, it uses
"agent" service with completely different protocol. OpenVPN GUI
needs to talk to agent using HTTP and JSON.

 - OpenVPN3 management interface realtime notifications must be
explicitly turned on in order for GUI to work.

To enable using openvpn3:

 - use any of *-ovpn3 presets (cmake build system)

 - ./configure --enable-ovpn3 (mingw)

To switch betweet openvpn2 and openvpn3, see "OpenVPN Engine"
radiobutton group in Settings -> Advanced dialog.

OnReady() implementation was slighly changed - "log all on"
replaced with "log on all" - according to management interface
documentation this is the right way to do it, and also OpenVPN3
only supports "on all" order.

Management interface - enabled OpenVPN3 client (omiclient.exe) and
agent (ovpnagent.exe) are now part of openvpn3 repo.

Co-authored-by: Christopher Ng <[email protected]>
Signed-off-by: Christopher Ng <[email protected]>
Signed-off-by: Lev Stipakov <[email protected]>
@lstipakov
Copy link
Member Author

I've pushed hopefully final version, changes from previous are -

  • removed ovpn3-specific code to enable logging
  • changed order from "log all on" to "log on all" - this is according to documentation and only supported way by openvpn3

There is a small bug in openvpn3 code - SUCCESS: password is correct string is missing \r\n and due to that OnReady() callback was never called. I've fixed that, hope this will be in openvpn3 master later this week. Meanwhile the fix is available on my repo (lstipakov/openvpn3@972b32d)

diff --git a/manage.c b/manage.c
index 1320b73..cfd96f9 100644
--- a/manage.c
+++ b/manage.c
@@ -272,12 +272,6 @@ OnManagement(SOCKET sk, LPARAM lParam)
                 ManagementCommand(c, c->manage.password, NULL, regular);
                 *c->manage.password = '\0';

-                if (o.ovpn_engine == OPENVPN_ENGINE_OVPN3)
-                {
-                    ManagementCommand(c, "log on all", NULL, regular);
-                    ManagementCommand(c, "state on all", NULL, regular);
-                }
-
                 continue;
             }

diff --git a/openvpn.c b/openvpn.c
index 12be5d6..81d457d 100644
--- a/openvpn.c
+++ b/openvpn.c
@@ -117,8 +117,8 @@ void
 OnReady(connection_t *c, UNUSED char *msg)
 {
     ManagementCommand(c, "state on", NULL, regular);
-    ManagementCommand(c, "log all on", OnLogLine, combined);
-    ManagementCommand(c, "echo all on", OnEcho, combined);
+    ManagementCommand(c, "log on all", OnLogLine, combined);
+    ManagementCommand(c, "echo on all", OnEcho, combined);
     ManagementCommand(c, "bytecount 5", NULL, regular);
 }

@facboy
Copy link
Contributor

facboy commented Jun 14, 2022

lgtm. been running your patched ovpn3 and this for the last few days.

Copy link
Collaborator

@selvanair selvanair left a comment

Choose a reason for hiding this comment

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

I have not tested using OpenVPN3, but the changes look sane, and use of OpenVPN 2.0 is not affected as far as I can see.

@lstipakov
Copy link
Member Author

Looks like this is ready for merge (/me looks at @selvanair, @mattock and @cron2).

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

Successfully merging this pull request may close these issues.

5 participants