(WIP) networking: Add support for OpenVPN #19363

subhoghoshX · 2023-09-21T07:49:55Z

pkg/networkmanager/dialogs-common.jsx

pkg/networkmanager/interfaces.js

pkg/networkmanager/openvpn.jsx

subhoghoshX · 2023-10-03T12:59:08Z

Greatly simplified config/cert generation, fetching to host machine and file upload tests.

subhoghoshX · 2023-10-04T12:13:33Z

353 ms to 140 ms = 60.4% reduction
1050 ms to 225 ms (in fast 3G) = 78.6% reduction

I guess the refactoring was worth it...

subhoghoshX · 2023-10-29T16:41:14Z

Generating Deffie-Hellman params on the CI can be an interesting flake as generating the (prime) numbers can take anywhere from less than a second to more than a minute...

martinpitt

I had a first coarse look at the import and config file structure. I'm rather relieved to see that there is nothing fundamentally weird -- just some rather shallow and easy cleanups (see threads below). I did not look into the design aspect of this, that needs to wait until Garrett is back. I figure he has some ideas/opinions about the dialog and the optional import, and how to "merge" it with Wireguard to have a single "Add VPN" button. But let's get the technical underpinnings right in the meantime.

Thanks, this is great work!

martinpitt · 2023-10-30T14:37:29Z

pkg/networkmanager/network-interface.jsx

@@ -144,6 +145,9 @@ export const NetworkInterfacePage = ({
    }

    function disconnect() {
+        if (active_connection.Vpn === true)


Can it be truthy without being true? What other values can this have?

martinpitt · 2023-10-30T14:39:29Z

pkg/networkmanager/network-interface.jsx

+        let isChecked = !!(dev && dev.ActiveConnection);
+        if (active_connection && active_connection.Vpn === true)
+            isChecked = true;
+        else if (!active_connection)
+            isChecked = false;


This is redundant and hard to understand. The if overwrites the initial isChecked in most cases. Please simplify using ?? and the duplicated/triplicated checks, and write as a single const or an else branch (for "active non-VPN" connection). This could also use a comment which cases exist.

martinpitt · 2023-10-30T14:40:36Z

pkg/networkmanager/network-main.jsx

            <NetworkAction buttonText={_("Add VPN")} type='wg' />
+            <NetworkAction buttonText={_("Add OpenVPN")} type='openvpn' />


This needs design. We deliberately called the initial button "VPN", not "Wireguard", as we anticipated that we're going to have more types. This somehow needs to get folded into "Add VPN".

martinpitt · 2023-10-30T14:42:15Z

pkg/networkmanager/openvpn.jsx

+    const configFile = "/tmp/temp.ovpn"; // TODO: use a random name for temporary files
+    await cockpit.file("/tmp/ovpn-to-json.py").replace(


You don't have to write that into a file at all. Build a string, and run it through pkg/lib/python.js's spawn().

martinpitt · 2023-10-30T14:43:54Z

pkg/networkmanager/openvpn.jsx

+
+// TODO: clean it
+async function ovpnToJSON(ovpn) {
+    const configFile = "/tmp/temp.ovpn"; // TODO: use a random name for temporary files


I would recommend to not create that file name on the JS side, it's too much churn. Let the Python script create it using NamedTemporaryFile, process it, read it, and return the JSON on its stdout (as it already does).

martinpitt · 2023-10-30T15:19:36Z

pkg/networkmanager/openvpn.jsx

+with open("${configFile}", "w") as ovpn_file:
+    ovpn_file.write("""${ovpn}""")
+
+subprocess.run(["nmcli", "con", "import", "--temporary", "type", "openvpn", "file", "${configFile}"], stdout=subprocess.DEVNULL)


Please try if you can do file - or file /dev/stdin and pipe the ${ovpn} data into the process; then you don't need the configFile temp file at all. Otherwise use a NamedTemporaryFile.

martinpitt · 2023-10-30T15:20:10Z

pkg/networkmanager/openvpn.jsx

+os.remove("/tmp/ovpn-to-json.py")
+subprocess.run(["nmcli", "con", "del", "temp"], stdout=subprocess.DEVNULL)
+`);
+    const json = await cockpit.spawn(["python", "/tmp/ovpn-to-json.py"], { superuser: 'try' });


I think you want require here, for a better error message.

martinpitt · 2023-10-30T15:21:28Z

pkg/networkmanager/openvpn.jsx

+        const caPath = `${user.home}/.cert/${caFileName}-ca.pem`;
+        const userCertPath = `${user.home}/.cert/${certFileName}-cert.pem`;
+        const userKeyPath = `${user.home}/.cert/${keyFileName}-key.pem`;


Is ~/.cert an official NM or recommended OpenVPN path? That sounds rather sad, this stuff belongs into ~/.config/networkmanager or something ovpn specific.

martinpitt · 2023-10-30T15:24:05Z

pkg/networkmanager/openvpn.jsx

+            <Name idPrefix={idPrefix} iface={iface} setIface={setIface} />
+            <FormFieldGroup
+                header={
+                    <FormFieldGroupHeader titleText={{ text: _("Automatic") }}


This should probably be called "Import"?

martinpitt · 2023-10-30T15:25:13Z

test/verify/check-networkmanager-openvpn

+        # increasing productivity
+        keys_dir = "/etc/openvpn/server"
+        m1.execute("touch .hushlogin")


What now? 😁

subhoghoshX · 2023-12-11T11:41:05Z

✅ I've seen this flake https://cockpit-logs.us-east-1.linodeobjects.com/pull-19363-20231211-103844-f3eb448f-fedora-38-networking/log.html#40 couple times by now, but it doesn't happen locally. I'll dig into it. - Fixed now!

✅ Another flake is https://cockpit-logs.us-east-1.linodeobjects.com/pull-19363-20231212-130944-2f459d18-fedora-38-networking/log.html#40-2. When creating diffie-hellman params it times out sometimes, because it can take anywhere between less than a second to more than a minute. - Fixed! It generates a lot faster now with the -dsaparam option.

✅ Yet another flake is this one https://cockpit-logs.us-east-1.linodeobjects.com/pull-19363-20231212-115530-795d6a37-fedora-38-networking/log.html#40-1I'm debugging it at the moment. I reproduces fine in the CI (rarely locally). - Fixed!

❌ Huh another: https://cockpit-logs.us-east-1.linodeobjects.com/pull-19363-20231214-081835-ddd8075b-fedora-39-networking/log.html#41-1. There are many variations of it. It's a "heisenbug". Every time I add debug logs it vanishes 🤯

subhoghoshX · 2023-12-15T15:27:24Z

I had a first coarse look at the import and config file structure. I'm rather relieved to see that there is nothing fundamentally weird...

Well the most fundamentally weird thing is still that temporarily importing a connection with nmcli, accessing a .nmconnection file (which you questioned whether it's a defined API of NM, it seems to be, but with the recent netplan changes in ubuntu, in #19724, I can't guarantee anymore), creating a config file and cleaning up the mess. I don't like the whole approach overall.

On Monday I found this KDE bug report and this commit closing it that uses a shared object exposed by NM. It's in C++, over the weekend I'll see if can do something similar in Python.

cockpituous reviewed Sep 21, 2023

View reviewed changes

subhoghoshX changed the title ~~networking: Add support for OpenVPN~~ (WIP) networking: Add support for OpenVPN Sep 21, 2023

subhoghoshX added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Sep 22, 2023

subhoghoshX force-pushed the openvpn branch 3 times, most recently from ca446c9 to bec2e80 Compare September 28, 2023 15:16

subhoghoshX force-pushed the openvpn branch from bec2e80 to 9714efd Compare October 3, 2023 12:56

subhoghoshX force-pushed the openvpn branch from 9714efd to 0ff099e Compare October 4, 2023 12:09

subhoghoshX force-pushed the openvpn branch from 0ff099e to 5f07b02 Compare October 6, 2023 07:32

subhoghoshX force-pushed the openvpn branch 5 times, most recently from a687536 to f97e6d7 Compare October 17, 2023 10:21

subhoghoshX force-pushed the openvpn branch from f97e6d7 to b779535 Compare October 27, 2023 06:48

subhoghoshX force-pushed the openvpn branch from b779535 to 18ad7ff Compare October 29, 2023 16:41

martinpitt requested changes Oct 30, 2023

View reviewed changes

subhoghoshX force-pushed the openvpn branch from 18ad7ff to 5a7c571 Compare October 30, 2023 15:55

subhoghoshX force-pushed the openvpn branch 2 times, most recently from ea225de to f3eb448 Compare December 11, 2023 10:37

subhoghoshX force-pushed the openvpn branch 6 times, most recently from 54629f4 to cf54eac Compare December 13, 2023 07:04

subhoghoshX force-pushed the openvpn branch 7 times, most recently from 58c0996 to ddd8075 Compare December 14, 2023 08:18

networking: Add support for OpenVPN

9a71bcb

subhoghoshX force-pushed the openvpn branch from ddd8075 to 9a71bcb Compare December 15, 2023 05:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(WIP) networking: Add support for OpenVPN #19363

(WIP) networking: Add support for OpenVPN #19363

subhoghoshX commented Sep 21, 2023 •

edited

Loading

subhoghoshX commented Oct 3, 2023

subhoghoshX commented Oct 4, 2023

subhoghoshX commented Oct 29, 2023

martinpitt left a comment

martinpitt Oct 30, 2023

martinpitt Oct 30, 2023

martinpitt Oct 30, 2023

martinpitt Oct 30, 2023

martinpitt Oct 30, 2023

martinpitt Oct 30, 2023

martinpitt Oct 30, 2023

martinpitt Oct 30, 2023

martinpitt Oct 30, 2023

martinpitt Oct 30, 2023

subhoghoshX commented Dec 11, 2023 •

edited

Loading

subhoghoshX commented Dec 15, 2023

		<NetworkAction buttonText={_("Add VPN")} type='wg' />
		<NetworkAction buttonText={_("Add OpenVPN")} type='openvpn' />

		const configFile = "/tmp/temp.ovpn"; // TODO: use a random name for temporary files
		await cockpit.file("/tmp/ovpn-to-json.py").replace(

(WIP) networking: Add support for OpenVPN #19363

Are you sure you want to change the base?

(WIP) networking: Add support for OpenVPN #19363

Conversation

subhoghoshX commented Sep 21, 2023 • edited Loading

subhoghoshX commented Oct 3, 2023

subhoghoshX commented Oct 4, 2023

subhoghoshX commented Oct 29, 2023

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhoghoshX commented Dec 11, 2023 • edited Loading

subhoghoshX commented Dec 15, 2023

subhoghoshX commented Sep 21, 2023 •

edited

Loading

subhoghoshX commented Dec 11, 2023 •

edited

Loading