Skip to content

Conversation

@Dav-11
Copy link

@Dav-11 Dav-11 commented Nov 26, 2024

Description

This PR adds the possibility to create wireguard VPNs as an alternative to the currently available L2TP-IPsec tunnels.

Changes in details:

  • System VM template changes:
    • Install Wireguard (leave it disabled)
    • Add placeholder file /etc/wireguard/wg0.conf
    • Add systemd service to restart wg-quick@wg0 if /etc/wireguard/wg0.conf is edited
  • DB changes: 2 new tables are introduced
    • wireguard_vpn -> contains details on each tunnel (similar to the remote_access_vpn table)
    • wireguard_vpn_peer -> contains details on each peer for the tunnel (similar to vpn_users table)
    • Preliminary DBML for the database:
    Table wireguard_vpn {
      id bigint [primary key]
      uuid varchar(40)
      domain_id bigint [Ref: > domain.id]
      account_id bigint [Ref: > account.id]
      display boolean
      state varchar(32)
      vpn_server_addr_ip bigint [Ref: > user_ip_address.id]
      vpn_server_addr_port bigint
      ip4_enabled boolean
      ip4_int_address varchar
      ip4_range varchar
      ip6_enabled boolean
      ip6_int_address varchar
      ip6_range varchar
      public_key varchar
      private_key varchar
      conf_file_name varchar
    }
    
    Table wireguard_vpn_peer {
      id bigint [primary key]
      vpn_id bigint [Ref: > wireguard_vpn.id]
      domain_id bigint [Ref: > domain.id]
      account_id bigint [Ref: > account.id]
      name string
      display boolean
      state varchar(32)
      uuid bigint
      ip4_address varchar
      ip6_address varchar
      public_key varchar
      allowed_ips text
      split_tunnel boolean
    }
    
    • API changes
      • Add {Create|List|Update|Delete}WgVpn
        • CreateWgVpn
          • params:
            • publicipid (required|int64):
            • ip4enable (bool): default is true
            • ip4range (string): IPv4 network to use internally for the vpn (CIDR notation)
            • ip6enable (bool): default is false
            • ip6range (string): IPv6 network to use internally for the vpn (CIDR notation)
            • openfirewall (bool): If firewall rule for source/end public port is automatically created , default= true
            • fordisplay (bool): default is true
            • accountid (int64):
            • domainid (int64):
          • flow:
          sequenceDiagram
          
            actor user
          
            participant mgmt as management server
          
            box VR
            participant uc as update_config.py
            participant conf as /etc/wireguard/wg0.conf
            participant iptables as /etc/iptables/rules.vX
            participant wg_srv as [email protected]
          
            end
          
          
            user ->> mgmt : (1) CreateWgVpn
            activate mgmt
            mgmt ->> mgmt: (2) gen server keys
            mgmt ->> mgmt: (3) save to DB
            mgmt ->> uc: (4) enable_wg(wg0.json)
            activate uc
            uc ->> conf: (5) generate
            uc ->> iptables: (6) regen
            uc ->> wg_srv: (7) systemctl start
            uc -->>- mgmt: (8) ok
            mgmt -->>- user: (9) ok
          
          Loading
        • ListWgVpn:
          • params:
            • id (int64)
            • networkid (int64)
            • page (int)
            • pagesize (int)
            • publicipid (int64)
            • domainid (int64)
            • accountid (int64)
            • listall (bool): If set to false, list only resources belonging to the command's caller, default= false
          • response element values:
            • id (int64)
            • networkid (int64)
            • publicipid (int64)
            • publicip (int64)
            • publicport (int)
            • accountid (int64)
            • domainid (int64)
            • fordisplay (bool)
            • ip4enable (bool)
            • ip4range (bool)
            • ip6enable (bool)
            • ip6range (string)
            • state (string)
            • publickey (string)
        • DeleteWgVpn
          • params:
            • id (required|int64)
      • Add {Create|List|Update|Delete}WgPeer
        • CreateWgPeer
          • params:
            • wgvpnid (required|int64):
            • publickey (string): if not provided, the server will generate the keys for the user, but save only the public one in the DB
            • domainid (int64):
            • accountid (int64):
            • fordisplay (bool):
            • splittunnel (bool): if false, the config file will be generated to pass all the peer traffic (0.0.0.0/0) through the vpn
          • flow:
            sequenceDiagram
          
              actor user
          
              participant mgmt as management server
          
              box VR
              participant uc as update_config.py
              participant conf as /etc/wireguard/wg0.conf
              participant wg_srv as [email protected]
          
              end
          
              user ->> mgmt : (1) CreateWgPeer
              activate mgmt
              mgmt ->> mgmt: (2) [OPT] gen keys
              mgmt ->> mgmt: (3) gen addresses 
              mgmt ->> mgmt: (4) save to DB 
              mgmt ->> uc: (5) add_wg_user(wg0_user.json)
              activate uc
              uc ->> conf: (6) regenerate 
              uc ->> wg_srv: (7) systemctl restart 
              uc -->>- mgmt: (8) ok
              mgmt -->>- user: (9) wg0.conf
          
          Loading
        • ListWgPeer
          • params:
            • id (int64)
            • vpnid (int64)
            • page (int)
            • pagesize (int)
            • domainid (int64)
            • accountid (int64)
            • listall (bool): If set to false, list only resources belonging to the command's caller, default=false
          • response element values:
            • id (int64)
            • vpnid (int64)
            • state (string)
            • publickey (string)
            • splittunnel(bool)
            • accountid (int64)
            • domainid (int64)
            • fordisplay (bool)
            • ip4address (string)
            • ip6address (string)
        • DeleteWgPeer
          • params:
            • id (required|int64)
        • GetWgConfigForPeer
          • params:
            • peerid (required|int64)
          • response values:
            • configfile (string): file .config for the peer (private key is omitted)
            • qrcode
  • UI changes
    • Add "Enable Wireguard VPN" button in VPN tab of IP address
    • Add peer management page.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

@boring-cyborg
Copy link

boring-cyborg bot commented Nov 26, 2024

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

@weizhouapache weizhouapache added this to the 4.21.0 milestone Nov 26, 2024
@weizhouapache
Copy link
Member

added to 4.21.0 milestone

@Dav-11
please let us know when it is ready for review

@Dav-11
Copy link
Author

Dav-11 commented Nov 26, 2024

Ok thank you

@GutoVeronezi
Copy link
Contributor

@Dav-11
Thanks for the PR

We already have APIs to manage VPN and introducing more APIs would increase the complexity of the system. Have you checked/considered the existent APIs?

@Dav-11
Copy link
Author

Dav-11 commented Nov 27, 2024

@GutoVeronezi
we did check the existing APIs, the wg behaviour is similar to the remote access vpn.
But the remote access vpn APIs do not have some field I need:

  • createRemoteAccessVpn

    • request:

      field I need current req felds
      If I need to enable ipv4/ipv6 or both now does not exist
      optionally the range for ipv4/ipv6 only ipv4 but not explicitly specified (iprange)
    • response:

      field I need current felds
      public key does not exists
  • addVpnUser

    • request:
      field I need current felds
      public key exists username and password that are not needed
    • response:
      field I need current felds
      public key exists username and password that are not needed
      ip v4 or/and v6 to use for the peer n.a.

Also, right now the remote-access vpn is not tied to the vpn server, but it seems like that all the vpn server made by a user have the same vpn_users. It would be better for wg to have the user/peer tied to the server instead.

I think It would be possible to use the same APIs by adding some fields (both in response and request) and a selector field for the vpn type (l2tp or wireguard). It would also require also to change the params validation based on the value of the vpn_type field. This would change the current APIs.

We thought that it would have been better to create new APIs, but if it is preferred to change the current one instead we could try to propose a way to do it that way.

Please let me know.

@weizhouapache
Copy link
Member

Currently the remote access vpn feature is implemented via strongswan which has lot of limitations.
it is a good idea to setup vpn with different software.

@Dav-11
can you consider a VPN provider framework in the early stage of your development ?
I know some users are favorite of wireguard. however it also has a few limitations: https://www.wireguard.com/known-limitations/
some other users might be interested in OpenVPN.

@Dav-11
Copy link
Author

Dav-11 commented Nov 27, 2024

@weizhouapache
An Idea could be to create a genric API for VPN and then allow different providers like plugins.
This way we could allow for other to add also openvpn or future VPN providers more easilly.

Would this be better ?

@weizhouapache
Copy link
Member

@weizhouapache An Idea could be to create a genric API for VPN and then allow different providers like plugins. This way we could allow for other to add also openvpn or future VPN providers more easilly.

Would this be better ?

yes @Dav-11
makes sense.

@codecov
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 4.04%. Comparing base (8a2c0f3) to head (b0fecdd).

❗ There is a different number of reports uploaded between BASE (8a2c0f3) and HEAD (b0fecdd). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (8a2c0f3) HEAD (b0fecdd)
unittests 1 0
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #9977       +/-   ##
============================================
- Coverage     15.80%   4.04%   -11.77%     
============================================
  Files          5627     392     -5235     
  Lines        492343   32177   -460166     
  Branches      59694    5679    -54015     
============================================
- Hits          77828    1301    -76527     
+ Misses       405992   30728   -375264     
+ Partials       8523     148     -8375     
Flag Coverage Δ
uitests 4.04% <ø> (ø)
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Dav-11
Copy link
Author

Dav-11 commented Nov 27, 2024

ok, I'll think of a way to modify tha proposed new APIs to allow such beaviour.

@weizhouapache
Copy link
Member

ok, I'll think of a way to modify tha proposed new APIs to allow such beaviour.

cool, thanks @Dav-11
it is a new feature I expected for several years..
looking forward to more changes

@GutoVeronezi
Copy link
Contributor

We thought that it would have been better to create new APIs, but if it is preferred to change the current one instead we could try to propose a way to do it that way.

Yes, reusing the existing APIs would reduce the maintenance efforts in the medium and long term. Also, it would prepare the code for new VPN providers, like it was commented.

@rohityadavcloud
Copy link
Member

rohityadavcloud commented Nov 27, 2024

@Dav-11 thanks for the PR - looks very exciting. I'm yet to review/test/try the PR, can you raise a doc PR?

Some questions & suggestions (in-line with @weizhouapache 's remarks)

  • Can you advise if this can allow a framework-plugin mechanism in the UI/API for end-users? For example, today it's Wireguard, what is somebody wants to also support openvpn or maybe others.

  • In a typical framework-plugin approach you wouldn't have VPN-specific APIs, but a configuration to select VPN provider/option but still use the same set of APIs (specifying the VPN provider) such as list/create/add/remove/delete etc to do common VPN operations - which are common across VPN types/options.

@Dav-11
Copy link
Author

Dav-11 commented Nov 28, 2024

@rohityadavcloud
Thank you,
I am back to the drawing board, we will compare ipsec, wg and openvpn to find which fields are common and which vary.

I would like to know how much can we edit the current APIs:

  • I suppose we can add fields, but can we make these fields required ? (breaking compatibility with the current API) ?
  • Can we delete fields or only deprecate them ?

Also do you think I should close this PR and open a new one with the new API proposal ? (I am sorry but this is the first PR i do for a community project)

@GutoVeronezi
Copy link
Contributor

@Dav-11, you can keep this PR and continue improving it, it is better for tracking.

I suppose we can add fields, but can we make these fields required ? (breaking compatibility with the current API) ?

Currently, we are not breaking compatibility (see #8970). Instead of marking a field as required, you could implement validations for the fields when one chooses Wireguard. We already do this kind of validation in some scenarios.

Can we delete fields or only deprecate them ?

Deleting fields would also break compatibility, so we are back to the first answer.

@rohityadavcloud
Copy link
Member

@Dav-11 to answer your questions, in addition to what Guto has already advised;

  • Adding new fields is acceptable to existing APIs, you can also make some fields/parameters of an API non-required or required depending on your use-case
  • If there are fields which may not be necessary, we mustn't delete them but
  • More general note: the VPN related APIs may accept the VPN provider (or similar) as an option, so the end user can specify which type of VPN they want; or this could be a property of the network where this is set (think updateNetwork? or a new API to configure/list VPN provider for the network). You want to explore an option that's intuitive and can then allow to support this easily in the UI.
  • Feel free to re-open a new PR or rework this one, this is all up to you.

Here are few more links for you that you may find useful from the open source hackerbook (CloudStack self-learning course material):
https://github.com/shapeblue/hackerbook/blob/main/2-dev.md#contributing-to-cloudstack
https://github.com/shapeblue/hackerbook/blob/main/hack/framework.md

@Dav-11
Copy link
Author

Dav-11 commented Mar 11, 2025

Hello @DaanHoogland I think I may have explained it poorly.

What I meant was to have a generic optional object config that is kept as a json and passed directly to the code that implements the specific vpn.

The vpn-specific code would unmarshal the JSON and validate the object.

The part about wg was just ana example of waht this json would be in the case of wireguard.

I am not certain a solution like this can be implemented in cloudstack, I have done something like this but I was using Go.

@DaanHoogland
Copy link
Contributor

@Dav-11 , sounds good, but I don't have a clear vision yet of how that would work for different FWs.

A name like implementationdata may be better than config as the suggests something very generic.

Implementations like this are there. Look for the Provider model.

@Dav-11
Copy link
Author

Dav-11 commented Mar 13, 2025

@DaanHoogland Thank you very much! No problem changing the parameter name, I agree that it is too generic.

I will look into the com.cloud.network.Network.Provider code and try to implement createRemoteAccessVpn to determine if what I want to do is feasible and how to proceed.

@DaanHoogland
Copy link
Contributor

@Dav-11 , I'm sure something went wrong with your branches. You are having 298 commits in this PR.

@Dav-11
Copy link
Author

Dav-11 commented Mar 14, 2025

Yes,
I tried to rebase the branch on master, but I have messed up.
On Monday I'll try to reset it.

@Dav-11
Copy link
Author

Dav-11 commented Mar 18, 2025

I resetted the PR and it closed the PR, will it be reopened with the first commit ?

@weizhouapache
Copy link
Member

@Dav-11
github says "there are no new commits on the branch".
if you push (or force-push) some changes to the branch, you/we might be able to reopen the PR.

@Dav-11
Copy link
Author

Dav-11 commented Mar 19, 2025

I made a commit just to keep this open.
Can I ask you if there is a way to align this branch with the main branch without having all the commits on main appear as commits of this branch ?

@Dav-11 Dav-11 reopened this Mar 19, 2025
@github-actions
Copy link

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@DaanHoogland
Copy link
Contributor

@Dav-11 , you can rebase your branch on main. In case it was off an older commit on main that would just be
git rebase bit it could be more complicated as I see you have conflicts now.

also apache rules require the license header in each source file.

@sureshanaparti
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@Dav-11
Copy link
Author

Dav-11 commented Jun 5, 2025

I am deeply sorry but I have been busy on other projects and it seems like it will be like that for the foreseeable future.
I will close the PR as I cannot keep working on it.

If anyone would like to keep on working on this and wants to know more about what we did (which is not much) I will happily answer to emails or in person if we meet at next CCC.

Sorry again and have a nice day.

@Dav-11 Dav-11 closed this Jun 5, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in Apache CloudStack 4.21.0 Jun 5, 2025
@blueorangutan
Copy link

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 13625

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

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants