networking: Add wireguard keepalive and preshared-key option in the gui #19521

subhoghoshX · 2023-10-24T08:23:16Z

fixes #19491

subhoghoshX · 2023-10-26T16:38:11Z

Finally green! Either the flakes introduced were bad or my brain is dead. It took far too long..

subhoghoshX · 2023-10-26T16:41:15Z

This flake is too common to ignore..

martinpitt

Thanks! This needs some design discussion first.

martinpitt · 2023-10-30T07:29:10Z

pkg/networkmanager/wireguard.jsx

+                if (peer.persistentKeepalive.trim()) {
+                    if (isNaN(Number(peer.persistentKeepalive)))
+                        throw cockpit.format(_("Peer #$0 has invalid persistent keepalive. It must be a number."), index + 1);


Probably it should also check for ≥ 1 and possibly some maximum? Also, can this be a float?

The dbus type for Keepalive is 'u', so it has to be >=0. Passing negative or float shows an error in the UI. Like below:

I opened an issue for a better error message at #19645.

0 (zero) means "not set" as usual.

I couldn't find a documented maximum value, but little experiment shows it can be in the range 0 to 65535 (i.e. 2^16 possible values).. after that it overflows without any error.

martinpitt · 2023-10-30T07:30:43Z

pkg/networkmanager/wireguard.jsx

@@ -324,6 +348,22 @@ export function WireGuardDialog({ settings, connection, dev }) {
                                    id={idPrefix + '-allowedips-peer-' + i}
                                />
                            </FormGroup>
+                            <FormGroup className='pf-m-6-col-on-md' label={_("Preshared key")} fieldId={idPrefix + '-presharedkey-peer-' + i} labelIcon={


I think the usual spelling is "pre-shared key"

Yes :) done.

martinpitt · 2023-10-30T07:31:50Z

pkg/networkmanager/wireguard.jsx

@@ -324,6 +348,22 @@ export function WireGuardDialog({ settings, connection, dev }) {
                                    id={idPrefix + '-allowedips-peer-' + i}
                                />
                            </FormGroup>
+                            <FormGroup className='pf-m-6-col-on-md' label={_("Preshared key")} fieldId={idPrefix + '-presharedkey-peer-' + i} labelIcon={
+                                <Popover
+                                    bodyContent={_("Preshared key should be a random 32 byte bas64 encoded.")}


Suggested change

bodyContent={_("Preshared key should be a random 32 byte bas64 encoded.")}

bodyContent={_("A base64 encoded string of 32 random bytes.")}

martinpitt · 2023-10-30T07:33:15Z

pkg/networkmanager/wireguard.jsx

@@ -324,6 +348,22 @@ export function WireGuardDialog({ settings, connection, dev }) {
                                    id={idPrefix + '-allowedips-peer-' + i}
                                />
                            </FormGroup>
+                            <FormGroup className='pf-m-6-col-on-md' label={_("Preshared key")} fieldId={idPrefix + '-presharedkey-peer-' + i} labelIcon={


I think this needs some UI work: I suppose it doesn't make sense to specify both a public/private key pair and a PSK? So perhaps there should either be a "key mode" selector at the top (symmetric or asymmetric), or one field should be disabled if the other has a value.

Both public/private key pair and PSK is used to make it more secure and quantum computer resistant. It is mentioned here https://www.wireguard.com/papers/wireguard.pdf.

Found this in the docs (https://www.wireguard.com/protocol/#key-exchange-and-data-packets), I think this is more accessible than the pdf above.

If an additional layer of symmetric-key crypto is required (for, say, post-quantum resistance), WireGuard also supports an optional pre-shared key that is mixed into the public key cryptography. When pre-shared key mode is not in use, the pre-shared key value used below is assumed to be an all-zero string of 32 bytes.

martinpitt · 2023-10-30T07:34:26Z

test/verify/check-networkmanager-wireguard

+        b.set_input_text("#network-wireguard-settings-keepalive-peer-0", "120")
+        psk = m1.execute("head -c 32 /dev/random | base64").strip()
+        b.set_input_text("#network-wireguard-settings-presharedkey-peer-0", psk)


Please also validate these settings in the API or with nmcli -- the UI doesn't reflect which key mode is being used, and which keepalive time.

Yes, good idea! Done, thanks!

I did it with wg because nmcli doesn't show peer information.

subhoghoshX · 2023-11-21T14:41:24Z

Removed the pixels to fix branch conflict. I'll get to the reviews tomorrow.

Update the tests accordingly. fixes cockpit-project#19491

subhoghoshX · 2023-11-22T13:32:45Z

Thanks! This needs some design discussion first.

It may indeed need some design discussion because adding another row makes it tall too quick and the separation between the peer fields gets confusing:

@garrett any suggestions?

garrett · 2023-11-23T09:43:28Z

A few thoughts about the latest screenshot:

Which are optional? Is preshared the only optional entry in peers?
Why are keys separate and so far apart?
What's the default keepalive? Should we pre-fill the value instead of leaving it blank? (So people would know if it needs to be adjusted or not.)
Are endpoint and keepalive related? (It appears that way here.)
It's hard to know which peer ends and the next starts. There's no separation, either with a divider or space. Everything's cramped, so we'd probably want divider lines between the items, I guess?
It feels like we're really overloading the modal and someone might have a lot of peers. Do we want to split the basic settings and peers?

subhoghoshX · 2023-11-27T12:02:45Z

Which are optional? Is preshared the only optional entry in peers?

Keepalive and Pre-shared key is completely optional and Endpoint is also (kind of) optional.
Only Public key and Allowed IPs is required.

Why are keys separate and so far apart?

No strong reason, it's just Public key is required so it's the first one, and Pre-shared key is optional, so it's one of the last.

What's the default keepalive? Should we pre-fill the value instead of leaving it blank? (So people would know if it needs to be adjusted or not.)

The default value is 0, which means "not set". Pre-filling it sounds better!

Are endpoint and keepalive related? (It appears that way here.)

They're separate.
Endpoint = the other machine/peer/server's IP address (or domain) with port. (e.g. 1.2.3.4:80)
Keepalive = Send an empty packet every specified seconds to keep the connection alive if a machine is behind a firewall or NAT.

It's just out of the 5 fields these two only take 1/4 of the width. So they're together. Everything else takes 1/2 of the horizontal space.

It's hard to know which peer ends and the next starts. There's no separation, either with a divider or space. Everything's cramped, so we'd probably want divider lines between the items, I guess?

It feels like we're really overloading the modal and someone might have a lot of peers. Do we want to split the basic settings and peers?

Yes, that's the issue it looks a little overloaded with the new row.

subhoghoshX force-pushed the wireguard-keepalive branch 6 times, most recently from 30ae48d to e161d67 Compare October 26, 2023 14:51

subhoghoshX force-pushed the wireguard-keepalive branch 4 times, most recently from 6f34fa7 to 9c11660 Compare October 26, 2023 17:37

subhoghoshX marked this pull request as ready for review October 27, 2023 05:58

martinpitt requested changes Oct 30, 2023

View reviewed changes

subhoghoshX marked this pull request as draft November 21, 2023 14:39

subhoghoshX force-pushed the wireguard-keepalive branch from 9c11660 to ede03ff Compare November 21, 2023 14:39

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

subhoghoshX force-pushed the wireguard-keepalive branch from ede03ff to a96c1c5 Compare November 22, 2023 07:17

networking: Add wireguard keepalive and preshared-key option in the gui

4a0865b

Update the tests accordingly. fixes cockpit-project#19491

subhoghoshX force-pushed the wireguard-keepalive branch from a96c1c5 to 4a0865b Compare November 22, 2023 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

networking: Add wireguard keepalive and preshared-key option in the gui #19521

networking: Add wireguard keepalive and preshared-key option in the gui #19521

subhoghoshX commented Oct 24, 2023 •

edited

Loading

subhoghoshX commented Oct 26, 2023

subhoghoshX commented Oct 26, 2023

martinpitt left a comment

martinpitt Oct 30, 2023

subhoghoshX Nov 22, 2023

martinpitt Oct 30, 2023

subhoghoshX Nov 22, 2023

martinpitt Oct 30, 2023

subhoghoshX Nov 22, 2023

martinpitt Oct 30, 2023

subhoghoshX Nov 22, 2023

subhoghoshX Nov 22, 2023

martinpitt Oct 30, 2023

subhoghoshX Nov 22, 2023

subhoghoshX Nov 22, 2023

subhoghoshX commented Nov 21, 2023

subhoghoshX commented Nov 22, 2023

garrett commented Nov 23, 2023

subhoghoshX commented Nov 27, 2023

	bodyContent={_("Preshared key should be a random 32 byte bas64 encoded.")}
	bodyContent={_("A base64 encoded string of 32 random bytes.")}

networking: Add wireguard keepalive and preshared-key option in the gui #19521

Are you sure you want to change the base?

networking: Add wireguard keepalive and preshared-key option in the gui #19521

Conversation

subhoghoshX commented Oct 24, 2023 • edited Loading

subhoghoshX commented Oct 26, 2023

subhoghoshX commented Oct 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhoghoshX commented Nov 21, 2023

subhoghoshX commented Nov 22, 2023

garrett commented Nov 23, 2023

subhoghoshX commented Nov 27, 2023

subhoghoshX commented Oct 24, 2023 •

edited

Loading