opnsense / core

OPNsense GUI, API and systems backend
https://opnsense.org/
BSD 2-Clause "Simplified" License
3.25k stars 724 forks source link

Possible Wireguard Widget bug #7812

Closed johannfenech closed 1 week ago

johannfenech commented 3 weeks ago

Describe the bug

It seems that the new Wireguard widget is not displaying all Wireguard connections (please refer to https://forum.opnsense.org/index.php?topic=42436.0)

To Reproduce

Steps to reproduce the behavior:

  1. Have 3 Wireguard connections (WG0, WG1 and WG2)
  2. Add a wireguard widget to the lobby (OPNsense 24.7.2-amd64)
  3. Only WG1 and WG2 statuses are displayed

Expected behavior Expected behaviour is to see all 3 Wireguard connections.

Describe alternatives you considered Based on the discussion in the forum link above, initially, it was assumed that the issue was related to WG0, however following the discussion I have renamed WG0 to WG1, WG1 to WG2, and WG2 to WG3

Did some debugging and

console.log (header)) just before super.updateTable('wgTunnelTable', [[header, row]], tunnel.publicKey);

returns this :

                <div style="display: flex; justify-content: space-between; align-items: center;">
                    <div style="display: flex; align-items: center;">
                        <i class="fa fa-exchange text-success wireguard-interface" style="cursor: pointer;"
                            data-toggle="tooltip" title="Online">
                        </i>
                        &nbsp;
                        <span><b>wg0 (NordVPN_Instance_XX_1) </b></span>
                    </div>
                </div>
                <div style="display: flex; justify-content: space-between; align-items: center;">
                    <div style="display: flex; align-items: center;">
                        <i class="fa fa-exchange text-success wireguard-interface" style="cursor: pointer;"
                            data-toggle="tooltip" title="Online">
                        </i>
                        &nbsp;
                        <span><b>wg1 (NordVPN_Instance_XX_2) </b></span>
                    </div>
                </div>

                <div style="display: flex; justify-content: space-between; align-items: center;">
                    <div style="display: flex; align-items: center;">
                        <i class="fa fa-exchange text-success wireguard-interface" style="cursor: pointer;"
                            data-toggle="tooltip" title="Online">
                        </i>
                        &nbsp;
                        <span><b>wg2 (NordVPN_Instance_XX_3) </b></span>
                    </div>
                </div>
    }

So the Spans are being generated correctly

Screenshots Refer to forum thread plus below

ss1 ss2

Relevant log files

If applicable, information from log files supporting your claim.

Additional context

None

Environment

OPNsense 24.7.2-amd64 FreeBSD 14.1-RELEASE-p3 OpenSSL 3.0.14

OPNsense-bot commented 3 weeks ago

Thank you for creating an issue. Since the ticket doesn't seem to be using one of our templates, we're marking this issue as low priority until further notice.

For more information about the policies for this repository, please read https://github.com/opnsense/core/blob/master/CONTRIBUTING.md for further details.

The easiest option to gain traction is to close this ticket and open a new one using one of our templates.

johannfenech commented 2 weeks ago

To put this to bed once and for all, in my specific case the problem was that I have two tunnels with the exact same public key so when this gets executed

super.updateTable('wgTunnelTable', [[header, row]], tunnel.publicKey);

The second instance, overwrites the first instance coz they have the same index (tunnel.publicKey).

My workaround I added an index to the forEach loop and the following :

let modifiedPublicKey = `${tunnel.publicKey}-${index}`;

    // Update the HTML table with the sorted rows
    super.updateTable('wgTunnelTable', [[header, row]], modifiedPublicKey);[/font]

So the full function would look like :

 tunnels.forEach((tunnel, index) => { // 'index' gives you the current position in the loop
    let header = `
        <div style="display: flex; justify-content: space-between; align-items: center;">
            <div style="display: flex; align-items: center;">
                <i class="fa ${tunnel.statusIcon} wireguard-interface" style="cursor: pointer;"
                    data-toggle="tooltip" title="${tunnel.connected ? this.translations.online : this.translations.offline}">
                </i>
                &nbsp;
                <span><b>${tunnel.ifname}</b></span>
            </div>
        </div>`;
    let row = `
        <div>
            <span><a href="/ui/wireguard/general#peers">${tunnel.name}</a> | ${tunnel.allowed_ips}</span>
        </div>
        <div>
            ${tunnel.latest_handshake_fmt
                ? `<span>${tunnel.latest_handshake_fmt}</span>
                   <div style="padding-bottom: 10px;">
                       <i class="fa fa-arrow-down" style="font-size: 13px;"></i>
                       ${tunnel.rx}
                       |
                       <i class="fa fa-arrow-up" style="font-size: 13px;"></i>
                       ${tunnel.tx}
                   </div>`
                : `<span>${this.translations.disconnected}</span>`}
        </div>`;

    // Add '-0', '-1', etc. to the public key using the index
    let modifiedPublicKey = `${tunnel.publicKey}-${index}`;

    // Update the HTML table with the sorted rows
    super.updateTable('wgTunnelTable', [[header, row]], modifiedPublicKey);
});

To further confirm, I have also restored my original configuration with the first interface called WG0, and the widget still works.

working

J

Monviech commented 1 week ago

@fichtner I have to construct it out of if+public-key or my own configuration in wireguard would fail now xD. Example:

<div class="flextable-row" id="Wireguard_wg1_9R__2v5469cduYQKGR__4iQWmth__lcdxSbx8Cm9EHeLwU__" style="opacity: 1; background-color: initial;">
<div class="flextable-row" id="Wireguard_wg1_67dHcTQOQeClqSNN2FjqoeA3nhSSlsnKiN__o3Hxokio__" style="opacity: 1; background-color: initial;">
<div class="flextable-row" id="Wireguard_wg1_X9xODnpCvr5rwQiykEzoXXlgpv8KjnylzdRaQvCImmE__" style="opacity: 1; background-color: initial;">

Without public-key here, all rows would be "Wireguard_wg1" and conflict.

fichtner commented 1 week ago

Ok fine.

johannfenech commented 1 week ago

@Monviech why not use just ${index} ?

his is how I got mine working -> `

    // Generate HTML for each tunnel
        tunnels.forEach((tunnel, index) => { // 'index' gives you the current position in the loop
    let header = `
            <div style="display: flex; justify-content: space-between; align-items: center;">
                <div style="display: flex; align-items: center;">
                    <i class="fa ${tunnel.statusIcon} wireguard-interface" style="cursor: pointer;"
                        data-toggle="tooltip" title="${tunnel.connected ? this.translations.online : this.translations.offline}">
                    </i>
                    &nbsp;
                    <span><b>${tunnel.ifname}</b></span>
                </div>
            </div>`;
        let row = `
            <div>
                <span><a href="/ui/wireguard/general#peers">${tunnel.name}</a> | ${tunnel.allowed_ips}</span>
            </div>
            <div>
                ${tunnel.latest_handshake_fmt
                    ? `<span>${tunnel.latest_handshake_fmt}</span>
                       <div style="padding-bottom: 10px;">
                           <i class="fa fa-arrow-down" style="font-size: 13px;"></i>
                           ${tunnel.rx}
                           |
                           <i class="fa fa-arrow-up" style="font-size: 13px;"></i>
                           ${tunnel.tx}
                       </div>`
                    : `<span>${this.translations.disconnected}</span>`}
            </div>`;

        // Update the HTML table with the sorted rows
        super.updateTable('wgTunnelTable', [[header, row]], index);
    });

`

fichtner commented 1 week ago

the index shifts as new instances are added or old ones removed which can create widget to display weirdly