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

Update of PR #327 (OpenVPN3 support) #506

Closed
wants to merge 8 commits into from

Conversation

facboy
Copy link
Contributor

@facboy facboy commented Jun 6, 2022

@lstipakov i merged master back into your PR #327 and addressed some of the comments from @selvanair.

is there interest in getting this merged?

lstipakov and others added 4 commits February 28, 2020 14:35
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.

Signed-off-by: Lev Stipakov <[email protected]>
@selvanair
Copy link
Collaborator

IIRC, when the original PR was submitted, ovpnagent was not available publicly. Has that changed?

@facboy
Copy link
Contributor Author

facboy commented Jun 7, 2022

Um, when you say available publicly, do you mean as a binary? Or to be built, from source?

The source is available here: https://github.com/OpenVPN/openvpn3/tree/master/openvpn/ovpnagent

I built OpenVPN3 locally (on Win11). Using openvpn-gui with OpenVPN3 requires ovpnagent and omicliagent and a registry key allowing the former to point to the latter.

I had to fix a couple of small issues: OpenVPN/openvpn3#218

@facboy facboy force-pushed the ovpn3-merge branch 2 times, most recently from 8792b45 to c8f7aef Compare June 7, 2022 11:39
@lstipakov
Copy link
Member

ovpnagent is now available in source (as @facboy) mentioned and also binary form (GitHub Actions artifacts, for example https://github.com/OpenVPN/openvpn3/suites/6374869439/artifacts/231032114).

The interest in merging this is a bit less than before, since with (coming) dco-win most of functionality will be moved from openvpn2/3 into kernel. That said, I think we could review and merge it - assuming it is useful for community and doesn't complicate code much.

@lstipakov
Copy link
Member

Since we have switched to CMake:

  • do we really need those travis changes? I don't think we use travis anymore.

  • @facboy could you add required changed to CMakePresets.json? For example, add something like "ovpn3-x64-release-ossl3" - this way we ensure that openvpn3-related code is part of our build automation

  • @selvanair I don't have write access to this repo and cannot give an approval to run workflow (aka github actions build automation). Could you grant this approval (without merging yet :) ?

@selvanair
Copy link
Collaborator

The source is available here: https://github.com/OpenVPN/openvpn3/tree/master/openvpn/ovpnagent

Thanks. I think it was closed source in 2020 when the original PR was submitted.

@facboy
Copy link
Contributor Author

facboy commented Jun 7, 2022

could you add required changed to CMakePresets.json? For example, add something like "ovpn3-x64-release-ossl3" - this way we ensure that openvpn3-related code is part of our build automation

done

@facboy facboy force-pushed the ovpn3-merge branch 3 times, most recently from 654be36 to 73c7cba Compare June 7, 2022 17:59
@lstipakov
Copy link
Member

Yay, it works :) I was able to connect to corp vpn with dco-win driver using openvpn-gui and openvpn3 ovpnagent/omicliagent.

Prerequisites:

  • add omi_exe_path key at HKLM\SOFTWARE\OpenVPN of type REG_SZ pointing to the location of omicliagent.exe - the latter one could be downloaded from GHA.
  • install ovpnagent.exe as a service (which can also be downloaded from GHA):
    c:\Users\lev\Projects\ovpn3-build\ovpn3\core\out\build\x64-Debug-dco\openvpn\ovpnagent\win>ovpnagent.exe install
    Note that running ovpnagent.exe in a foreground will not work - CreateProcessAsUser() will fail.

One small thing I noticed is that settings dialog height has grown by 40px even if ENABLE_OVPN3 is not set. Can we not do it (and resize programmatically instead?) for existing configurations?

@facboy
Copy link
Contributor Author

facboy commented Jun 8, 2022

hmm...i suspect that is beyond me!

i believe if you run ovpnagent as admin it works - that's how i observe the logs when debugging.

@selvanair
Copy link
Collaborator

hmm...i suspect that is beyond me!

I guess this is in reference to constructing the controls at run-time.

Can't we use preprocessor directives in resource files? Like:

#if ENABLE_OVPN3
ID_DLG_ADVANCED DIALOGEX 6, 18, 252, 320
#else
ID_DLG_ADVANCED DIALOGEX 6, 18, 252, 280
#endif

and similar for the new groupbox and buttons. And, remove the conditional hiding of those controls in options.c

@facboy
Copy link
Contributor Author

facboy commented Jun 8, 2022

i think this does it.

@lstipakov
Copy link
Member

I am revamping #327 based on @facboy's work - will rebase on top of master, remove merge commits and squash fixes.

This also works:

#ifdef ENABLE_OVPN3
#define ADVANCED_DIALOG_HEIGHT 320
#else
#define ADVANCED_DIALOG_HEIGHT 280
#endif

and then

/* Advanced Dialog */
ID_DLG_ADVANCED DIALOGEX 6, 18, 252, ADVANCED_DIALOG_HEIGHT

@lstipakov
Copy link
Member

I updated #327 and added @facboy as a co-author, let's close this one and continue there.

@facboy
Copy link
Contributor Author

facboy commented Jun 10, 2022

kk, closed.

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.

3 participants