RDKBNETWOR-76 : Add WireGuard VPN manager with IPv4/IPv6 support - #5
sameerunnisa9 wants to merge 2 commits into
Conversation
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new RdkVpnManager component that manages WireGuard tunnels via a TR-181 data model, including syscfg persistence and a helper shell script for bringing up/down interfaces and syncing peers.
Changes:
- Adds a new
vpnmanagerCCSP component (ssp_* glue) that registers a TR-181 data model and connects to the CCSP message bus. - Implements TR-181 middle-layer DML + backend manager for WireGuard interface and peer/tunnel configuration (IPv4/IPv6, ports, keys).
- Adds packaging/build integration, systemd service unit, XML data model, and
vpn_config.shscript.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| source/VpnManager/ssp_messagebus_interface.h | Declares message bus engagement/teardown hooks. |
| source/VpnManager/ssp_messagebus_interface.c | Implements CCSP message bus init/register logic and callbacks. |
| source/VpnManager/ssp_main.c | Adds daemon entrypoint, signal handling, and bus/component startup. |
| source/VpnManager/ssp_internal.h | Defines component IDs/paths and internal interfaces. |
| source/VpnManager/ssp_global.h | Provides global CCSP/ANSC includes for the component. |
| source/VpnManager/ssp_action.c | Creates/engages/cancels CCSP controller + data model registration. |
| source/VpnManager/Makefile.am | Builds the vpnmanager binary and links middle-layer library. |
| source/TR-181/middle_layer_src/vpn_manager_internal.h | Internal WireGuard DM object and helper macros/constants. |
| source/TR-181/middle_layer_src/vpn_manager_internal.c | WireGuard DM object creation/init/remove and public key retrieval. |
| source/TR-181/middle_layer_src/vpn_manager_dml.h | Declares TR-181 DML getters/setters/commit/rollback for WireGuard. |
| source/TR-181/middle_layer_src/vpn_manager_dml.c | Implements TR-181 DML logic and commit/apply behavior. |
| source/TR-181/middle_layer_src/vpn_manager_dml_apis.h | Declares backend “CosaDml_*” APIs and helpers used by DML. |
| source/TR-181/middle_layer_src/vpn_manager_dml_apis.c | Implements syscfg-backed persistence, wg show status, config generation, and peer sync. |
| source/TR-181/middle_layer_src/plugin_main.h | Plugin version/export definitions for TR-181 integration. |
| source/TR-181/middle_layer_src/plugin_main.c | Registers DML functions and initializes backend manager via plugin API. |
| source/TR-181/middle_layer_src/plugin_main_apis.h | Defines backend manager + WireGuard data structures and enums. |
| source/TR-181/middle_layer_src/plugin_main_apis.c | Implements backend manager create/init/remove lifecycle. |
| source/TR-181/middle_layer_src/Makefile.am | Builds middle-layer static library for the component. |
| source/TR-181/Makefile.am | Adds TR-181 subdir build wiring. |
| source/Makefile.am | Adds TR-181 and VpnManager subdirs to build. |
| Makefile.am | Top-level automake wiring to build source/. |
| files/vpn_config.sh | Shell helper to generate wg0.conf, bring interface up/down, and sync peers. |
| configure.ac | Autotools configuration to generate Makefiles for new subdirs. |
| config/RdkVpnManager.xml | TR-181 data model XML definition for X_RDK_Wireguard and Tunnel table. |
| config/RdkVPNManager.service | Systemd unit to start the vpnmanager daemon. |
Suppressed comments (2)
source/TR-181/middle_layer_src/vpn_manager_dml_apis.c:99
VpnDmlInitialize()is declared to returnANSC_STATUSbut falls off the end without returning a value, which is a build error with-Werror=return-type.
{
pMyObject->WireguardPort = atoi(string);
}
}
}
files/vpn_config.sh:99
- Same issue as above: this logs the config file contents (including secrets) and also uses
WG_CONFIG_FILEwithout$. Redact secrets before logging or avoid logging the config entirely.
echo "*********************************" >> $WG_DEBUG_FILE
cat WG_CONFIG_FILE >> $WG_DEBUG_FILE
echo "*********************************" >> $WG_DEBUG_FILE
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mhughesacn
left a comment
There was a problem hiding this comment.
Hi @sameerunnisa9 : Please will you change the header line in these new files:
"If not stated otherwise in this file or this component's Licenses.txt file"
change "Licenses.txt" to "LICENSE" as the older form is no longer used.
The files should scan clean on the update.
Thank you
MartinH, RDK CMF Compliance Team
Reason for change: Introduce RdkVpnManager to manage WireGuard tunnels over
TR-181 (X_RDK.Wireguard), including interface bring-up/tear-down, peer
configuration, ListenPort, and IPv4/IPv6 address and AllowedIPs handling via
vpn_config.sh and syscfg (wireguard_enabled, wireguard_local_ipv4,
wireguard_local_ipv6, wireguard_subnet).
Test Procedure:
1. Enable WireGuard via TR-181 and verify wg0.conf is created with IPv4 and IPv6 Address, ListenPort, and PrivateKey.
2. Create a peer tunnel and confirm PublicKey, Endpoint, and IPv4/IPv6 AllowedIPs are written correctly.
3. Bring the tunnel UP/DOWN and verify syscfg wireguard_enabled and firewall-restart are updated.
4. Confirm public key retrieval and tunnel status through the data model.
Testing Done : Results are captured in RDKBNETWOR-76
Risks: None.
Signed-off-by: Sameerunnisa S <sameerunnisa.s@telekom-digital.com>
47c5c8b to
694636c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 13 comments.
Suppressed comments (7)
source/TR-181/middle_layer_src/vpn_manager_dml.c:807
insis declared but never used, which will fail compilation under-Wall -Werror(-Wunused-variable).
}
ULONG ins = pWireGuardTu->InstanceNumber;
pWireGuardTu->Enable = FALSE;
source/VpnManager/ssp_main.c:60
- On
setsid()failure,daemonize()exits with status 0, which can mask startup failures. This should exit non-zero.
if (setsid() < 0)
{
CcspTraceInfo(("Error demonizing (setsid)! %d - %s\n", errno, strerror(errno)));
exit(0);
}
files/vpn_config.sh:61
- In the UP command, this
catuses the literal filenameWG_CONFIG_FILEinstead of the$WG_CONFIG_FILEvariable, so the debug log won’t show the actual config (and may fail if no such file exists).
elif [ "$COMMAND" = "UP" ]; then
echo "*********************************" >> $WG_DEBUG_FILE
cat WG_CONFIG_FILE >> $WG_DEBUG_FILE
echo "*********************************" >> $WG_DEBUG_FILE
files/vpn_config.sh:99
- In the SYNC command, this
catuses the literal filenameWG_CONFIG_FILEinstead of the$WG_CONFIG_FILEvariable, so the debug log won’t show the actual config (and may fail if no such file exists).
elif [ "$COMMAND" = "SYNC" ]; then
echo "*********************************" >> $WG_DEBUG_FILE
cat WG_CONFIG_FILE >> $WG_DEBUG_FILE
echo "*********************************" >> $WG_DEBUG_FILE
source/VpnManager/ssp_messagebus_interface.c:75
- If
component_idorpathis NULL, the function logs an error but continues and passes NULL intoCCSP_Message_Bus_Init/Register_Path, which can crash or mis-register the component. This should fail fast and return an error status.
if ( ! component_id || ! path )
{
CcspTraceError((" !!! ssp_Mbi_MessageBusEngage: component_id or path is NULL !!!\n"));
}
source/VpnManager/ssp_main.c:51
- On fork failure,
daemonize()exits with status 0, which makes service managers treat startup as successful even though daemonization failed. This should exit non-zero.
This issue also appears on line 56 of the same file.
case -1:{
// Error
CcspTraceInfo(("Error daemonizing (fork)! %d - %s\n", errno, strerror(
errno)));
exit(0);
break;}
source/VpnManager/ssp_main.c:274
SIGKILLcannot be caught or handled; registering a handler for it is ineffective and can confuse maintenance/troubleshooting.
signal(SIGSEGV, sig_handler);
signal(SIGBUS, sig_handler);
signal(SIGKILL, sig_handler);
signal(SIGFPE, sig_handler);
signal(SIGILL, sig_handler);
65eb9f3 to
960725f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are multiple confirmed build-breaking issues (missing returns and -Werror-triggering constructs/format strings) plus functional/script bugs and a sensitive-key logging issue that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (14)
source/TR-181/middle_layer_src/vpn_manager_dml_apis.c:99
- VpnDmlInitialize() has return type ANSC_STATUS but currently falls off the end without returning a value, which is undefined behavior and will typically fail the build with -Werror.
if (0 == syscfg_get(NULL, "Wireguard_Port", string, sizeof(string)))
{
pMyObject->WireguardPort = atoi(string);
}
}
}
source/TR-181/middle_layer_src/vpn_manager_dml_apis.c:117
- Assignment inside the if-condition (fp = popen(...)) will trigger -Wall warnings (often treated as errors via -Werror). Split the assignment from the condition to avoid build breaks.
if (fp = popen("wg show", "r"))
{
source/TR-181/middle_layer_src/vpn_manager_dml_apis.c:456
- sprintf(buf, "%d", val) is the wrong format for ULONG and can fail the build under -Wformat/-Werror. Prefer snprintf with %lu.
ANSC_STATUS
CosaDml_WireGuardTunnelSetRemotePort(ULONG tuIns, ULONG val)
{
char syscfg_var[MAX_SIZE +1]={0};
char buf[BUF_SIZE]={0};
sprintf(buf, "%d", val);
snprintf(syscfg_var, sizeof(syscfg_var),WIREGUARDTU_PARAM_REMPORT, tuIns);
syscfg_set(NULL,syscfg_var,buf);
source/TR-181/middle_layer_src/vpn_manager_dml.c:305
- sprintf(buf, "%d", uValue) uses the wrong format for ULONG and may fail compilation under -Wformat/-Werror. Use snprintf with %lu.
{
pMyObject->WireguardPort = uValue;
sprintf(buf, "%d", uValue);
syscfg_set(NULL, "Wireguard_Port", buf);
syscfg_commit();
source/TR-181/middle_layer_src/vpn_manager_internal.c:42
- WireGuard_GetPublicKey(): fgets() return value isn’t checked and keylen can be 0, making pubKey[keylen-1] an underflow. The function also doesn’t return an ANSC_STATUS on success, which is undefined behavior and can break builds.
fgets(pubKey,64,fPtr);
keylen = strlen(pubKey);
if(pubKey[keylen-1] == '\n')
pubKey[keylen-1] = '\0';
source/TR-181/middle_layer_src/plugin_main.c:308
- Assignment in the if-condition (tmpSubsystemPrefix = ...) will typically trigger -Wall warnings and can break the build under -Werror. Split assignment from condition.
if ( tmpSubsystemPrefix = g_GetSubsystemPrefix(g_pDslhDmlAgent) )
{
AnscCopyString(g_SubSysPrefix_Irep, tmpSubsystemPrefix);
}
source/TR-181/middle_layer_src/vpn_manager_dml_apis.h:35
- CosaDml_WireGuardGetStatus() is declared twice in this header, which is redundant and can confuse maintenance. Keep a single declaration.
ANSC_STATUS
CosaDml_WireGuardGetStatus(DML_VPN_IF_CFG_STATUS *st);
source/VpnManager/ssp_action.c:162
- ssp_engage() returns ANSC_STATUS_SUCCESS even if RegisterCcspDataModel fails, which hides initialization failures from callers and supervision logic.
}
return ANSC_STATUS_SUCCESS;
}
source/VpnManager/ssp_messagebus_interface.c:75
- ssp_Mbi_MessageBusEngage() logs when component_id/path are NULL but continues anyway, which will likely crash in CCSP_Message_Bus_Init / CCSP_Message_Bus_Register_Path. Return failure immediately on invalid inputs.
if ( ! component_id || ! path )
{
CcspTraceError((" !!! ssp_Mbi_MessageBusEngage: component_id or path is NULL !!!\n"));
}
source/VpnManager/ssp_messagebus_interface.c:95
- bus_handle is a handle/pointer type; logging it with %x is undefined and will warn/fail under -Wformat/-Werror. Use %p with a (void*) cast.
CcspTraceInfo(("INFO: bus_handle: 0x%8x \n", bus_handle));
source/VpnManager/ssp_main.c:353
- SIGKILL cannot be caught/handled; registering a handler for it is ineffective and may hide that shutdown cleanup isn’t happening as expected. Remove the SIGKILL handler registration.
signal(SIGSEGV, sig_handler);
signal(SIGBUS, sig_handler);
signal(SIGKILL, sig_handler);
signal(SIGFPE, sig_handler);
files/vpn_config.sh:61
- The script uses
cat WG_CONFIG_FILE(missing$), so it won’t actually dump the wg0.conf contents to the debug log.
elif [ "$COMMAND" = "UP" ]; then
echo "*********************************" >> $WG_DEBUG_FILE
cat WG_CONFIG_FILE >> $WG_DEBUG_FILE
echo "*********************************" >> $WG_DEBUG_FILE
files/vpn_config.sh:86
- In create_tun, the Endpoint line is appended to the debug log instead of wg0.conf, so peers won’t have an Endpoint configured.
if [ -z "$3" ] || [ -z "$4" ]; then
echo "No end point or remote needed as its configure as server" >> $WG_DEBUG_FILE
else
echo "Endpoint = $3:$4" >> $WG_DEBUG_FILE
fi
files/vpn_config.sh:92
- The script writes the pre-shared key value into $WG_DEBUG_FILE ("PSK $7"), which is sensitive material and should not be logged.
echo "PresharedKey = $7" >> $WG_CONFIG_FILE
echo "PSK $7" >> $WG_DEBUG_FILE
else
echo "NO PSK $7" >> $WG_DEBUG_FILE
wg-quick down wg0; wg-quick up wg0
- Files reviewed: 25/25 changed files
- Comments generated: 2
- Review effort level: Lite
| v_secure_system(VPN_CONFIG_SCRIPT " enable %s %d %s %ld", pMyObject->LocalIP, netMastToCIDR(pMyObject->Subnet), pMyObject->LocalIPv6, pMyObject->WireguardPort); | ||
| for (i = 0; i < 5; i++) | ||
| { | ||
| PDML_VPN_TUN_CFG pTunnel = &(wireGuard->WireGuardTu[i]); | ||
|
|
||
| if (!pTunnel->Enable) | ||
| continue; | ||
|
|
||
| CcspTraceInfo(("%s %d - Creating Tunnel through the script. \n", __FUNCTION__, __LINE__)); | ||
| v_secure_system(VPN_CONFIG_SCRIPT " create_tun %s %s %ld %s %s %s", | ||
| pTunnel->PeerPublicKey, pTunnel->RemoteEndPoint, | ||
| pTunnel->RemotePort,pTunnel->RemoteIP,pTunnel->RemoteIPv6, | ||
| pTunnel->PSKEnable ? pTunnel->PreSharedKey:"\0"); |
| wg genkey | tee $WG_PRIV_KEY_FILE | wg pubkey | tee $WG_PUB_KEY_FILE | ||
| echo "Key file not found....Generating it." >> $WG_DEBUG_FILE | ||
| fi | ||
| PrivKey=$(<$WG_PRIV_KEY_FILE) |
WireGuard VPN manager with IPv4/IPv6 support
Reason for change: Introduce RdkVpnManager to manage WireGuard tunnels over
TR-181 (X_RDK.Wireguard), including interface bring-up/tear-down, peer
configuration, ListenPort, and IPv4/IPv6 address and AllowedIPs handling via
vpn_config.sh and syscfg (wireguard_enabled, wireguard_local_ipv4,
wireguard_local_ipv6, wireguard_subnet).
Test Procedure:
Testing Done : Results are captured in RDKBNETWOR-76
Risks: None.