From c752de10eea907a9a95744c303024d87497347a2 Mon Sep 17 00:00:00 2001 From: Moritz Fain Date: Wed, 26 Nov 2025 16:22:56 +0100 Subject: [PATCH 1/2] Management: add reload-push-options command This adds a new management command 'reload-push-options' that allows reloading the push options from the configuration file without restarting the server. This is useful for updating routes or DNS settings for new clients without dropping existing connections. The command supports an optional 'update-clients' argument. When provided, the server will also synchronize the new options to currently connected clients by: 1. Calculating the difference between old and new push options. 2. Sending '-instruction' (e.g. -route) to remove old options. 3. Sending new options via PUSH_UPDATE. This includes a comprehensive integration test suite in tests/reload_push_options. --- .gitignore | 1 + src/openvpn/manage.c | 29 ++ src/openvpn/manage.h | 3 +- src/openvpn/multi.c | 280 ++++++++++++++++++ src/openvpn/options.c | 2 + src/openvpn/options.h | 1 + src/openvpn/options_util.c | 7 +- src/openvpn/options_util.h | 3 + tests/reload_push_options/.gitignore | 6 + tests/reload_push_options/Dockerfile | 46 +++ tests/reload_push_options/README.md | 84 ++++++ .../reload_push_options/client-entrypoint.sh | 35 +++ tests/reload_push_options/configs/client.conf | 22 ++ .../configs/server.conf.default | 16 + tests/reload_push_options/docker-compose.yml | 73 +++++ tests/reload_push_options/run.sh | 277 +++++++++++++++++ .../scripts/client-entrypoint.sh | 18 ++ tests/reload_push_options/scripts/gen-keys.sh | 48 +++ .../reload_push_options/scripts/log-routes.sh | 13 + .../scripts/server-entrypoint.sh | 20 ++ 20 files changed, 980 insertions(+), 4 deletions(-) create mode 100644 tests/reload_push_options/.gitignore create mode 100644 tests/reload_push_options/Dockerfile create mode 100644 tests/reload_push_options/README.md create mode 100755 tests/reload_push_options/client-entrypoint.sh create mode 100644 tests/reload_push_options/configs/client.conf create mode 100644 tests/reload_push_options/configs/server.conf.default create mode 100644 tests/reload_push_options/docker-compose.yml create mode 100755 tests/reload_push_options/run.sh create mode 100755 tests/reload_push_options/scripts/client-entrypoint.sh create mode 100755 tests/reload_push_options/scripts/gen-keys.sh create mode 100755 tests/reload_push_options/scripts/log-routes.sh create mode 100755 tests/reload_push_options/scripts/server-entrypoint.sh diff --git a/.gitignore b/.gitignore index 04523af3ccc..8a712be5769 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,7 @@ out .vs .deps .libs +.cache Makefile Makefile.in aclocal.m4 diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index 6efa1001a80..96fb762d984 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -137,6 +137,8 @@ man_help(void) msg(M_CLIENT, "push-update-broad options : Broadcast a message to update the specified options."); msg(M_CLIENT, " Ex. push-update-broad \"route something, -dns\""); msg(M_CLIENT, "push-update-cid CID options : Send an update message to the client identified by CID."); + msg(M_CLIENT, "reload-push-options [update-clients] : Reload push options from config file for new clients."); + msg(M_CLIENT, " With 'update-clients': also update connected clients (add new, remove old)."); msg(M_CLIENT, "END"); } @@ -1723,6 +1725,33 @@ man_dispatch_command(struct management *man, struct status_output *so, const cha man_push_update(man, p, UPT_BY_CID); } } + else if (streq(p[0], "reload-push-options")) + { + if (man->persist.callback.reload_push_options) + { + bool update_clients = (p[1] && streq(p[1], "update-clients")); + bool status = (*man->persist.callback.reload_push_options)(man->persist.callback.arg, update_clients); + if (status) + { + if (update_clients) + { + msg(M_CLIENT, "SUCCESS: push options reloaded and sent to all clients"); + } + else + { + msg(M_CLIENT, "SUCCESS: push options reloaded from config file"); + } + } + else + { + msg(M_CLIENT, "ERROR: failed to reload push options"); + } + } + else + { + man_command_unsupported("reload-push-options"); + } + } #if 1 else if (streq(p[0], "test")) { diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h index dedcc155802..2dde104b953 100644 --- a/src/openvpn/manage.h +++ b/src/openvpn/manage.h @@ -50,7 +50,7 @@ #include "socket_util.h" #include "mroute.h" -#define MANAGEMENT_VERSION 5 +#define MANAGEMENT_VERSION 6 #define MANAGEMENT_N_PASSWORD_RETRIES 3 #define MANAGEMENT_LOG_HISTORY_INITIAL_SIZE 100 #define MANAGEMENT_ECHO_BUFFER_SIZE 100 @@ -198,6 +198,7 @@ struct management_callback bool (*remote_entry_get)(void *arg, unsigned int index, char **remote); bool (*push_update_broadcast)(void *arg, const char *options); bool (*push_update_by_cid)(void *arg, unsigned long cid, const char *options); + bool (*reload_push_options)(void *arg, bool update_clients); }; /* diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 2b944667063..b08bd4df9bc 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -34,6 +34,7 @@ #include "forward.h" #include "multi.h" #include "push.h" +#include "options_util.h" #include "run_command.h" #include "otime.h" #include "gremlin.h" @@ -4100,6 +4101,284 @@ management_get_peer_info(void *arg, const unsigned long cid) return ret; } +/** + * Check if an option string exists in a push_list. + */ +static bool +push_option_exists(const struct push_list *list, const char *option) +{ + const struct push_entry *e = list->head; + while (e) + { + if (e->enable && e->option && strcmp(e->option, option) == 0) + { + return true; + } + e = e->next; + } + return false; +} + +/* + * Helper to append to push list using specific GC. + */ +static void +push_list_add(struct push_list *list, const char *opt, struct gc_arena *gc) +{ + struct push_entry *e; + ALLOC_OBJ_CLEAR_GC(e, struct push_entry, gc); + e->enable = true; + e->option = opt; + + if (list->tail) + { + list->tail->next = e; + list->tail = e; + } + else + { + list->head = e; + list->tail = e; + } +} + +/** + * Find the index of an updatable option type for a given option string. + * @param option The option string to check (e.g., "route 10.0.0.0 255.0.0.0") + * @return Index into updatable_options[] or -1 if not found + */ +static ssize_t +find_updatable_option_index(const char *option) +{ + size_t len = strlen(option); + for (size_t i = 0; i < updatable_options_count; ++i) + { + size_t opt_len = strlen(updatable_options[i]); + if (len >= opt_len + && strncmp(option, updatable_options[i], opt_len) == 0 + && (option[opt_len] == '\0' || option[opt_len] == ' ')) + { + return (ssize_t)i; + } + } + return -1; +} + +/** + * Reload push options from the configuration file. + * This function re-reads the config file and updates the push_list + * that will be sent to new connecting clients. + * + * Thread safety: OpenVPN uses a single-threaded event loop, so this + * function runs sequentially with all other operations. + * + * @param arg Pointer to multi_context + * @param update_clients If true, update connected clients (add new, remove old) + * @return true on success, false on failure + */ +static bool +management_callback_reload_push_options(void *arg, bool update_clients) +{ + struct multi_context *m = (struct multi_context *)arg; + struct gc_arena gc = gc_new(); + bool ret = false; + + msg(M_INFO, "MANAGEMENT: Reloading push options from config file"); + + /* Check if we have a config file to reload from */ + if (!m->top.options.config) + { + msg(M_WARN, "MANAGEMENT: Cannot reload push options - no config file specified"); + goto cleanup; + } + + /* Save reference to old push_list for update comparison */ + struct push_list old_push_list = m->top.options.push_list; + + /* Create a temporary options structure to parse the config */ + struct options new_options; + CLEAR(new_options); + + /* Initialize the gc_arena for the new options */ + new_options.gc = gc_new(); + + /* Set up environment for config parsing */ + struct env_set *es = env_set_create(&gc); + unsigned int option_types_found = 0; + + /* Re-read the configuration file */ + read_config_file(&new_options, m->top.options.config, 0, + m->top.options.config, 0, M_WARN, + OPT_P_DEFAULT, &option_types_found, es); + + /* Validate we got a sensible result - if old list had entries but new is empty, + * this likely indicates a parsing error */ + if (old_push_list.head && !new_options.push_list.head) + { + msg(M_WARN, "MANAGEMENT: Config reload returned empty push list - aborting"); + gc_free(&new_options.gc); + goto cleanup; + } + + /* Create a new GC arena for the new push list */ + struct gc_arena new_push_list_gc = gc_new(); + struct push_list new_push_list = { NULL, NULL }; + + /* Copy each push entry from the parsed config to the new push_list + * using the new dedicated push_list_gc */ + const struct push_entry *e = new_options.push_list.head; + while (e) + { + if (e->enable && e->option) + { + /* Copy the option string to the new dedicated gc_arena */ + const char *opt = string_alloc(e->option, &new_push_list_gc); + push_list_add(&new_push_list, opt, &new_push_list_gc); + } + e = e->next; + } + + /* Free the temporary options gc_arena (parsed config) */ + gc_free(&new_options.gc); + + /* Update connected clients if requested */ + /* We do this BEFORE swapping the lists so we can compare old vs new */ + if (update_clients) + { + /* Calculate required buffer size: sum of all option lengths + separators */ + size_t opts_size = 0; + const struct push_entry *size_e = new_push_list.head; + while (size_e) + { + if (size_e->enable && size_e->option) + { + opts_size += strlen(size_e->option) + 2; /* option + ", " */ + } + size_e = size_e->next; + } + /* Add space for removal commands: "-type, " for each updatable option type */ + opts_size += updatable_options_count * 32; + /* Minimum size to avoid edge cases */ + if (opts_size < PUSH_BUNDLE_SIZE) + { + opts_size = PUSH_BUNDLE_SIZE; + } + + struct buffer opts = alloc_buf_gc(opts_size, &gc); + bool first = true; + int added = 0, removed = 0; + + /* Set of option types that have been removed/modified */ + bool *type_removed = gc_malloc(updatable_options_count * sizeof(bool), true, &gc); + + /* 1. Detect removed options and mark their types */ + const struct push_entry *old_e = old_push_list.head; + while (old_e) + { + if (old_e->enable && old_e->option) + { + if (!push_option_exists(&new_push_list, old_e->option)) + { + ssize_t type_idx = find_updatable_option_index(old_e->option); + if (type_idx >= 0) + { + type_removed[type_idx] = true; + removed++; + msg(D_PUSH, "MANAGEMENT: Removing: %s", old_e->option); + } + else + { + msg(M_WARN, "MANAGEMENT: Cannot remove option '%s' (not updatable)", old_e->option); + } + } + } + old_e = old_e->next; + } + + /* 2. Add removal commands for all marked types */ + for (size_t i = 0; i < updatable_options_count; ++i) + { + if (type_removed[i]) + { + if (!first) + { + buf_printf(&opts, ", "); + } + /* Send -type to remove all options of that type */ + buf_printf(&opts, "-%s", updatable_options[i]); + first = false; + } + } + + /* 3. Add new options AND re-add options belonging to removed types */ + const struct push_entry *new_e = new_push_list.head; + while (new_e) + { + if (new_e->enable && new_e->option) + { + bool should_send = false; + bool is_existing = push_option_exists(&old_push_list, new_e->option); + + /* Check if this option belongs to a type that was reset */ + bool type_was_reset = false; + ssize_t type_idx = find_updatable_option_index(new_e->option); + if (type_idx >= 0 && type_removed[type_idx]) + { + type_was_reset = true; + } + + /* Always send new options */ + if (!is_existing) + { + should_send = true; + added++; + msg(D_PUSH, "MANAGEMENT: Adding: %s", new_e->option); + } + /* Also resend options if their type was reset (because we sent -type) */ + else if (type_was_reset) + { + should_send = true; + msg(D_PUSH, "MANAGEMENT: Re-adding (type reset): %s", new_e->option); + } + + if (should_send) + { + if (!first) + { + buf_printf(&opts, ", "); + } + buf_printf(&opts, "%s", new_e->option); + first = false; + } + } + new_e = new_e->next; + } + + if (BLEN(&opts) > 0) + { + msg(M_INFO, "MANAGEMENT: Updating clients with push options (added=%d, removed=%d)", + added, removed); + management_callback_send_push_update_broadcast(m, BSTR(&opts)); + } + else + { + msg(M_INFO, "MANAGEMENT: No changes to send to clients"); + } + } + + /* Now replace the old push_list with the new one and free old memory */ + gc_free(&m->top.options.push_list_gc); + m->top.options.push_list_gc = new_push_list_gc; + m->top.options.push_list = new_push_list; + + msg(M_INFO, "MANAGEMENT: Push options reloaded successfully"); + ret = true; + +cleanup: + gc_free(&gc); + return ret; +} + #endif /* ifdef ENABLE_MANAGEMENT */ @@ -4125,6 +4404,7 @@ init_management_callback_multi(struct multi_context *m) cb.get_peer_info = management_get_peer_info; cb.push_update_broadcast = management_callback_send_push_update_broadcast; cb.push_update_by_cid = management_callback_send_push_update_by_cid; + cb.reload_push_options = management_callback_reload_push_options; management_set_callback(management, &cb); } #endif /* ifdef ENABLE_MANAGEMENT */ diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 4794315cc35..8a97374e0bb 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -807,6 +807,7 @@ init_options(struct options *o, const bool init_gc) if (init_gc) { gc_init(&o->gc); + gc_init(&o->push_list_gc); gc_init(&o->dns_options.gc); o->gc_owned = true; } @@ -942,6 +943,7 @@ uninit_options(struct options *o) if (o->gc_owned) { gc_free(&o->gc); + gc_free(&o->push_list_gc); gc_free(&o->dns_options.gc); } } diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 42db9caec6b..bd013dbae53 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -487,6 +487,7 @@ struct options in_addr_t server_bridge_pool_end; struct push_list push_list; + struct gc_arena push_list_gc; bool ifconfig_pool_defined; in_addr_t ifconfig_pool_start; in_addr_t ifconfig_pool_end; diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c index eba7d396011..5fd35eda0aa 100644 --- a/src/openvpn/options_util.c +++ b/src/openvpn/options_util.c @@ -193,7 +193,7 @@ atoi_constrained(const char *str, int *value, const char *name, int min, int max return true; } -static const char *updatable_options[] = { "block-ipv6", "block-outside-dns", +const char *updatable_options[] = { "block-ipv6", "block-outside-dns", "dhcp-option", "dns", "ifconfig", "ifconfig-ipv6", "push-continuation", "redirect-gateway", @@ -202,6 +202,8 @@ static const char *updatable_options[] = { "block-ipv6", "block-outside-dns", "route-metric", "topology", "tun-mtu", "keepalive" }; +const size_t updatable_options_count = sizeof(updatable_options) / sizeof(char *); + bool check_push_update_option_flags(char *line, int *i, unsigned int *flags) { @@ -232,8 +234,7 @@ check_push_update_option_flags(char *line, int *i, unsigned int *flags) } size_t len = strlen(&line[*i]); - int count = sizeof(updatable_options) / sizeof(char *); - for (int j = 0; j < count; ++j) + for (size_t j = 0; j < updatable_options_count; ++j) { size_t opt_len = strlen(updatable_options[j]); if (len < opt_len) diff --git a/src/openvpn/options_util.h b/src/openvpn/options_util.h index 5bb710a6e44..50988b1bb7f 100644 --- a/src/openvpn/options_util.h +++ b/src/openvpn/options_util.h @@ -108,4 +108,7 @@ bool apply_pull_filter(const struct options *o, char *line); */ bool check_push_update_option_flags(char *line, int *i, unsigned int *flags); +extern const char *updatable_options[]; +extern const size_t updatable_options_count; + #endif /* ifndef OPTIONS_UTIL_H_ */ diff --git a/tests/reload_push_options/.gitignore b/tests/reload_push_options/.gitignore new file mode 100644 index 00000000000..d174b9ac2c8 --- /dev/null +++ b/tests/reload_push_options/.gitignore @@ -0,0 +1,6 @@ +# Generated during tests +keys/ +results/ + + + diff --git a/tests/reload_push_options/Dockerfile b/tests/reload_push_options/Dockerfile new file mode 100644 index 00000000000..4677aa7349d --- /dev/null +++ b/tests/reload_push_options/Dockerfile @@ -0,0 +1,46 @@ +# Build OpenVPN from source +FROM debian:bookworm-slim AS builder + +RUN apt-get update && apt-get install -y \ + build-essential \ + autoconf \ + automake \ + libtool \ + pkg-config \ + libssl-dev \ + liblz4-dev \ + liblzo2-dev \ + libpam0g-dev \ + libcap-ng-dev \ + libnl-genl-3-dev \ + && rm -rf /var/lib/apt/lists/* + +WORKDIR /src +COPY . . + +RUN autoreconf -fvi && \ + ./configure --disable-systemd --disable-plugins && \ + make -j$(nproc) + +# Runtime image +FROM debian:bookworm-slim + +RUN apt-get update && apt-get install -y \ + libssl3 \ + liblz4-1 \ + liblzo2-2 \ + libcap-ng0 \ + libnl-genl-3-200 \ + iproute2 \ + iptables \ + netcat-openbsd \ + gettext-base \ + && rm -rf /var/lib/apt/lists/* + +COPY --from=builder /src/src/openvpn/openvpn /usr/local/sbin/openvpn + +# Copy default server config into the image +COPY tests/reload_push_options/configs/server.conf.default /etc/openvpn/server.conf.default + +RUN mkdir -p /dev/net && \ + mknod /dev/net/tun c 10 200 || true diff --git a/tests/reload_push_options/README.md b/tests/reload_push_options/README.md new file mode 100644 index 00000000000..3ec3e591e85 --- /dev/null +++ b/tests/reload_push_options/README.md @@ -0,0 +1,84 @@ +# reload-push-options Test Suite + +Integration tests for the `reload-push-options` management command. + +## Prerequisites + +- Docker & Docker Compose +- OpenSSL (for key generation) + +## Quick Start + +```bash +cd tests/reload_push_options +./run.sh +``` + +## What it tests + +| Test | Description | +|------|-------------| +| 1 | Basic reload without update-clients - existing clients unchanged | +| 2 | Update clients with new route added | +| 3 | Update clients with route removed | +| 4 | Update clients with all routes removed | +| 5 | Update clients with only new routes (complete replacement) | +| 6 | Update clients with mixed changes (add + remove) | +| 7 | New client receives updated config | +| 8 | Stress test with 500 routes | + +## Architecture + +``` +┌─────────────────────────────────────────────────┐ +│ Docker Network │ +│ 10.100.0.0/24 │ +│ │ +│ ┌──────────┐ ┌──────────┐ ┌──────────┐ │ +│ │ Server │ │ Client1 │ │ Client2 │ │ +│ │ .0.2 │◄───│ .0.10 │ │ .0.11 │ │ +│ │ │◄───│ │ │ │ │ +│ │ :7505 │ │ │ │ │ │ +│ │ (mgmt) │ │ │ │ │ │ +│ └──────────┘ └──────────┘ └──────────┘ │ +│ │ │ +└───────┼─────────────────────────────────────────┘ + │ + localhost:7505 (management interface) +``` + +## Manual Testing + +```bash +# Start the environment +docker compose up -d + +# Connect to management interface +nc localhost 7505 + +# Commands to try: +help +status +reload-push-options +reload-push-options update-clients +``` + +## Files + +- `docker-compose.yml` - Container orchestration +- `Dockerfile` - Builds OpenVPN from source +- `configs/server.conf.default` - Default server config (baked into image) +- `configs/client.conf` - Client config +- `keys/` - PKI (auto-generated) +- `scripts/` - Entrypoints and helpers +- `results/` - Test output and logs + +## How Config Updates Work + +1. Default server config (`server.conf.default`) is copied into the Docker image +2. On container start, `server-entrypoint.sh` restores it to `/etc/openvpn/server.conf` +3. During tests, config is updated inside the container via `docker compose exec` +4. This ensures each test run starts from a clean, known state + + + diff --git a/tests/reload_push_options/client-entrypoint.sh b/tests/reload_push_options/client-entrypoint.sh new file mode 100755 index 00000000000..638d76d4251 --- /dev/null +++ b/tests/reload_push_options/client-entrypoint.sh @@ -0,0 +1,35 @@ +#!/bin/bash +set -e + +CLIENT_NAME=${CLIENT_NAME:-client} +LOG_FILE="/var/log/openvpn/${CLIENT_NAME}.log" + +# Function to log routes +log_routes() { + echo "=== Routes at $(date -Iseconds) ===" >> "$LOG_FILE" + ip route show | grep -E "^(10\.|172\.|192\.168\.)" >> "$LOG_FILE" 2>/dev/null || echo "(no VPN routes)" >> "$LOG_FILE" + echo "" >> "$LOG_FILE" +} + +# Monitor route changes in background +monitor_routes() { + while true; do + ip monitor route 2>/dev/null | while read -r line; do + echo "[$(date -Iseconds)] ROUTE CHANGE: $line" >> "$LOG_FILE" + done + sleep 1 + done +} + +# Start route monitor +monitor_routes & + +# Log initial routes +echo "=== Client $CLIENT_NAME starting ===" > "$LOG_FILE" +log_routes + +# Start OpenVPN +exec /usr/local/sbin/openvpn --config /etc/openvpn/client.conf \ + --log-append "$LOG_FILE" \ + --verb 4 + diff --git a/tests/reload_push_options/configs/client.conf b/tests/reload_push_options/configs/client.conf new file mode 100644 index 00000000000..1ce2ea847bb --- /dev/null +++ b/tests/reload_push_options/configs/client.conf @@ -0,0 +1,22 @@ +# OpenVPN Client Config for reload-push-options testing +client +dev tun +proto udp +remote 10.100.0.2 1194 + +ca /etc/openvpn/keys/ca.crt +# cert and key are set via command line based on client name + +nobind +persist-key +persist-tun + +# Verbose logging +verb 4 + +# Allow scripts to run +script-security 2 + +# Script to log route changes +route-up /scripts/log-routes.sh +route-pre-down /scripts/log-routes.sh diff --git a/tests/reload_push_options/configs/server.conf.default b/tests/reload_push_options/configs/server.conf.default new file mode 100644 index 00000000000..2445b718d8e --- /dev/null +++ b/tests/reload_push_options/configs/server.conf.default @@ -0,0 +1,16 @@ +dev tun +proto udp +port 1194 +server 10.8.0.0 255.255.255.0 +ca /etc/openvpn/keys/ca.crt +cert /etc/openvpn/keys/server.crt +key /etc/openvpn/keys/server.key +dh /etc/openvpn/keys/dh.pem +keepalive 10 120 +persist-key +persist-tun +management 0.0.0.0 7505 +verb 4 +log /var/log/openvpn.log +${PUSH_OPTIONS} + diff --git a/tests/reload_push_options/docker-compose.yml b/tests/reload_push_options/docker-compose.yml new file mode 100644 index 00000000000..69668c8850a --- /dev/null +++ b/tests/reload_push_options/docker-compose.yml @@ -0,0 +1,73 @@ +services: + server: + build: + context: ../.. + dockerfile: tests/reload_push_options/Dockerfile + cap_add: + - NET_ADMIN + devices: + - /dev/net/tun:/dev/net/tun + networks: + vpn_net: + ipv4_address: 10.100.0.2 + volumes: + - ./keys:/etc/openvpn/keys:ro + - ./scripts:/scripts:ro + ports: + - "7505:7505" # Management interface + command: ["/scripts/server-entrypoint.sh"] + healthcheck: + test: ["CMD", "sh", "-c", "ip link show tun0 >/dev/null 2>&1"] + interval: 2s + timeout: 2s + retries: 15 + start_period: 5s + + client1: + build: + context: ../.. + dockerfile: tests/reload_push_options/Dockerfile + cap_add: + - NET_ADMIN + devices: + - /dev/net/tun:/dev/net/tun + networks: + vpn_net: + ipv4_address: 10.100.0.10 + volumes: + - ./configs:/etc/openvpn:ro + - ./keys:/etc/openvpn/keys:ro + - ./scripts:/scripts:ro + - ./results:/results + depends_on: + server: + condition: service_healthy + command: ["/scripts/client-entrypoint.sh", "client1"] + + client2: + build: + context: ../.. + dockerfile: tests/reload_push_options/Dockerfile + cap_add: + - NET_ADMIN + devices: + - /dev/net/tun:/dev/net/tun + networks: + vpn_net: + ipv4_address: 10.100.0.11 + volumes: + - ./configs:/etc/openvpn:ro + - ./keys:/etc/openvpn/keys:ro + - ./scripts:/scripts:ro + - ./results:/results + depends_on: + server: + condition: service_healthy + command: ["/scripts/client-entrypoint.sh", "client2"] + +networks: + vpn_net: + driver: bridge + ipam: + config: + - subnet: 10.100.0.0/24 diff --git a/tests/reload_push_options/run.sh b/tests/reload_push_options/run.sh new file mode 100755 index 00000000000..bd43ead37c1 --- /dev/null +++ b/tests/reload_push_options/run.sh @@ -0,0 +1,277 @@ +#!/bin/bash +# Test suite for reload-push-options management command +set -e + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +cd "$SCRIPT_DIR" + +# Colors +RED='\033[0;31m' +GREEN='\033[0;32m' +YELLOW='\033[1;33m' +NC='\033[0m' + +pass() { echo -e "${GREEN}✓ PASS${NC}: $1"; } +fail() { + echo -e "${RED}✗ FAIL${NC}: $1" + info "Debug: Server logs (last 30 lines):" + docker compose exec -T server tail -30 /var/log/openvpn.log 2>/dev/null || true + info "Debug: Client1 logs (last 30 lines):" + docker compose exec -T client1 tail -30 /results/client1.log 2>/dev/null || true + docker compose down -v 2>/dev/null || true + exit 1 +} +info() { echo -e "${YELLOW}→${NC} $1"; } + +mgmt() { + echo "$1" | nc -q1 localhost 7505 2>/dev/null || echo "$1" | nc -w1 localhost 7505 2>/dev/null +} + +get_client_log_lines() { + local client="$1" + docker compose exec -T "$client" wc -l /results/${client}.log 2>/dev/null | awk '{print $1}' || echo "0" +} + +wait_for_client_ready() { + local client="$1" + local lines_before="$2" + local max_wait=20 + + for i in $(seq 1 $max_wait); do + sleep 1 + if docker compose exec -T "$client" tail -n +$((lines_before + 1)) /results/${client}.log 2>/dev/null | grep -q "Initialization Sequence Completed"; then + return 0 + fi + done + return 1 +} + +get_client_routes() { + local client="$1" + if ! docker compose exec -T "$client" ip link show tun0 &>/dev/null; then + echo "(tun0 not found)" + return + fi + docker compose exec -T "$client" ip route show 2>/dev/null | grep -E "^(192\.168\.|10\.|172\.)" || echo "(no matching routes)" +} + +update_server_config() { + local config_content="$1" + # Generate config from template inside the container using envsubst + docker compose exec -T -e PUSH_OPTIONS="$config_content" server \ + sh -c 'envsubst '"'"'${PUSH_OPTIONS}'"'"' < /etc/openvpn/server.conf.default > /etc/openvpn/server.conf' +} + +wait_for_clients() { + info "Waiting for clients to connect..." + for i in {1..30}; do + local count=$(mgmt "status" | grep -c "^client" || true) + if [ "$count" -ge 2 ]; then + return 0 + fi + sleep 1 + done + fail "Clients did not connect in time" +} + +cleanup() { + info "Cleaning up..." + docker compose down -v 2>/dev/null || true + rm -rf results/* +} + +# Run test: update config, reload (with or without update-clients), verify routes +# Args: test_name, client, update_clients (0|1|skip), config_content, must_have_pattern, must_not_have_pattern +# update_clients=skip: don't send mgmt command, just verify routes (for reconnect tests) +run_test() { + local test_name="$1" + local client="$2" + local update_clients="$3" + local config_content="$4" + local must_have="$5" + local must_not_have="$6" + + echo "" + echo "--- $test_name ---" + + local routes_before=$(get_client_routes "$client") + local log_lines=$(get_client_log_lines "$client") + + update_server_config "$config_content" + + if [ "$update_clients" != "skip" ]; then + local cmd="reload-push-options" + [ "$update_clients" = "1" ] && cmd="reload-push-options update-clients" + + local result=$(mgmt "$cmd") + echo "Management response: $result" + + if ! echo "$result" | grep -q "SUCCESS"; then + fail "$test_name: command failed" + fi + pass "$test_name: command succeeded" + + if [ "$update_clients" = "1" ]; then + wait_for_client_ready "$client" "$log_lines" + else + sleep 2 + fi + fi + + local routes=$(get_client_routes "$client") + info "Routes: $routes" + + if [ -n "$must_have" ]; then + if echo "$routes" | grep -qE "$must_have"; then + pass "$test_name: expected routes present" + else + fail "$test_name: expected routes ($must_have) not found" + fi + fi + + if [ -n "$must_not_have" ]; then + if echo "$routes" | grep -qE "$must_not_have"; then + fail "$test_name: removed routes ($must_not_have) still present" + else + pass "$test_name: routes correctly removed" + fi + fi +} + +trap cleanup EXIT + +# Generate keys if needed +if [ ! -f keys/ca.crt ]; then + info "Generating test PKI..." + chmod +x scripts/gen-keys.sh + ./scripts/gen-keys.sh +fi + +chmod +x scripts/*.sh +rm -rf results/* +mkdir -p results + +echo "" +echo "==========================================" +echo " reload-push-options Test Suite" +echo "==========================================" +echo "" + +docker compose down -v 2>/dev/null || true + +# Initial config is now baked into the image (server.conf.default) +# and restored on container start by server-entrypoint.sh + +info "Building and starting containers..." +docker compose build +docker compose up -d --wait +wait_for_clients + +# Test 1: No update-clients - routes must NOT change (still have initial routes, not the new 192.168.30.0) +run_test "Test 1: No update-clients" client1 0 \ + 'push "route 192.168.10.0 255.255.255.0" +push "route 192.168.20.0 255.255.255.0" +push "route 192.168.30.0 255.255.255.0" +push "dhcp-option DNS 8.8.8.8"' \ + "192\.168\.10\.0|192\.168\.20\.0" "192\.168\.30\.0" + +# Test 2: Add route +run_test "Test 2: Add route" client1 1 \ + 'push "route 192.168.10.0 255.255.255.0" +push "route 192.168.20.0 255.255.255.0" +push "route 192.168.30.0 255.255.255.0" +push "route 192.168.40.0 255.255.255.0" +push "dhcp-option DNS 8.8.8.8"' \ + "192\.168\.40\.0" "" + +# Test 3: Remove route +run_test "Test 3: Remove route" client1 1 \ + 'push "route 192.168.10.0 255.255.255.0" +push "route 192.168.30.0 255.255.255.0" +push "route 192.168.40.0 255.255.255.0" +push "dhcp-option DNS 8.8.8.8"' \ + "" "192\.168\.20\.0" + +# Test 4: Remove all routes +run_test "Test 4: Remove all routes" client1 1 \ + 'push "dhcp-option DNS 8.8.8.8"' \ + "" "192\.168\." + +# Test 5: Add new routes +run_test "Test 5: New routes" client1 1 \ + 'push "route 172.16.0.0 255.255.0.0" +push "route 172.17.0.0 255.255.0.0" +push "dhcp-option DNS 1.1.1.1"' \ + "172\.(16|17)\.0\.0" "" + +# Test 6: Mixed - remove 172.16, keep 172.17, add 10.10 +run_test "Test 6: Mixed changes" client1 1 \ + 'push "route 172.17.0.0 255.255.0.0" +push "route 10.10.0.0 255.255.0.0" +push "dhcp-option DNS 1.1.1.1"' \ + "172\.17\.0\.0|10\.10\.0\.0" "172\.16\.0\.0" + +# Test 7: Reconnected client gets current config +echo "" +echo "--- Test 7: Reconnect ---" +info "Updating config and restarting client2" + +update_server_config 'push "route 172.17.0.0 255.255.0.0" +push "route 10.10.0.0 255.255.0.0" +push "route 192.168.100.0 255.255.255.0" +push "route 192.168.200.0 255.255.255.0" +push "dhcp-option DNS 1.1.1.1"' + +docker compose restart client2 +sleep 5 + +run_test "Test 7: Reconnect" client2 skip \ + 'push "route 172.17.0.0 255.255.0.0" +push "route 10.10.0.0 255.255.0.0" +push "route 192.168.100.0 255.255.255.0" +push "route 192.168.200.0 255.255.255.0" +push "dhcp-option DNS 1.1.1.1"' \ + "172\.17\.0\.0|10\.10\.0\.0|192\.168\.100\.0|192\.168\.200\.0" "" + +# Test 8: Stress test with 500 routes +echo "" +echo "--- Test 8: 500 routes stress test ---" +info "Generating config with 500 routes..." + +# Generate 500 routes: 10.{1-250}.{0,128}.0/25 +routes_config="" +for i in $(seq 1 250); do + routes_config+="push \"route 10.$i.0.0 255.255.128.0\" +" + routes_config+="push \"route 10.$i.128.0 255.255.128.0\" +" +done +routes_config+='push "dhcp-option DNS 8.8.8.8"' + +update_server_config "$routes_config" + +log_lines=$(get_client_log_lines client1) +result=$(mgmt "reload-push-options update-clients") +echo "Management response: $result" + +if ! echo "$result" | grep -q "SUCCESS"; then + fail "Test 8: 500 routes - command failed" +fi +pass "Test 8: 500 routes - command succeeded" + +wait_for_client_ready client1 "$log_lines" + +routes=$(get_client_routes client1) +route_count=$(echo "$routes" | grep -c "^10\." || true) +info "Route count: $route_count" + +if [ "$route_count" -ge 450 ]; then + pass "Test 8: 500 routes - received $route_count routes" +else + fail "Test 8: 500 routes - expected ~500 routes, got $route_count" +fi + +echo "" +echo "==========================================" +echo -e "${GREEN}All tests completed!${NC}" +echo "==========================================" diff --git a/tests/reload_push_options/scripts/client-entrypoint.sh b/tests/reload_push_options/scripts/client-entrypoint.sh new file mode 100755 index 00000000000..9b65aeaa1ee --- /dev/null +++ b/tests/reload_push_options/scripts/client-entrypoint.sh @@ -0,0 +1,18 @@ +#!/bin/bash +set -e + +CLIENT_NAME="${1:-client1}" +echo "Starting OpenVPN client: $CLIENT_NAME" + +# Wait for server to be ready +sleep 3 + +# Start OpenVPN with client-specific cert/key +exec /usr/local/sbin/openvpn \ + --config /etc/openvpn/client.conf \ + --cert /etc/openvpn/keys/${CLIENT_NAME}.crt \ + --key /etc/openvpn/keys/${CLIENT_NAME}.key \ + --log /results/${CLIENT_NAME}.log + + + diff --git a/tests/reload_push_options/scripts/gen-keys.sh b/tests/reload_push_options/scripts/gen-keys.sh new file mode 100755 index 00000000000..a0b8dee4deb --- /dev/null +++ b/tests/reload_push_options/scripts/gen-keys.sh @@ -0,0 +1,48 @@ +#!/bin/bash +# Generate test PKI for OpenVPN testing +set -e + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +KEYS_DIR="$SCRIPT_DIR/../keys" +mkdir -p "$KEYS_DIR" +cd "$KEYS_DIR" + +# Generate CA +openssl genrsa -out ca.key 2048 +openssl req -new -x509 -days 365 -key ca.key -out ca.crt \ + -subj "/CN=Test CA" + +# Generate server cert +openssl genrsa -out server.key 2048 +openssl req -new -key server.key -out server.csr \ + -subj "/CN=server" +openssl x509 -req -days 365 -in server.csr -CA ca.crt -CAkey ca.key \ + -CAcreateserial -out server.crt + +# Generate client1 cert +openssl genrsa -out client1.key 2048 +openssl req -new -key client1.key -out client1.csr \ + -subj "/CN=client1" +openssl x509 -req -days 365 -in client1.csr -CA ca.crt -CAkey ca.key \ + -CAcreateserial -out client1.crt + +# Generate client2 cert +openssl genrsa -out client2.key 2048 +openssl req -new -key client2.key -out client2.csr \ + -subj "/CN=client2" +openssl x509 -req -days 365 -in client2.csr -CA ca.crt -CAkey ca.key \ + -CAcreateserial -out client2.crt + +# Generate DH params (2048 required by modern OpenSSL) +openssl dhparam -out dh.pem 2048 + +# Generate TLS auth key +openvpn --genkey secret ta.key 2>/dev/null || \ + dd if=/dev/urandom of=ta.key bs=256 count=1 2>/dev/null + +# Cleanup CSRs +rm -f *.csr + +echo "Keys generated in $KEYS_DIR" +ls -la "$KEYS_DIR" + diff --git a/tests/reload_push_options/scripts/log-routes.sh b/tests/reload_push_options/scripts/log-routes.sh new file mode 100755 index 00000000000..88d9a2292b9 --- /dev/null +++ b/tests/reload_push_options/scripts/log-routes.sh @@ -0,0 +1,13 @@ +#!/bin/bash +# Log current routes to results file +TIMESTAMP=$(date +%Y%m%d_%H%M%S) +ROUTES_FILE="/results/routes_${common_name:-unknown}_${TIMESTAMP}.txt" + +echo "=== Route event at $TIMESTAMP ===" >> "$ROUTES_FILE" +echo "Script: $script_type" >> "$ROUTES_FILE" +echo "Routes:" >> "$ROUTES_FILE" +ip route show >> "$ROUTES_FILE" +echo "" >> "$ROUTES_FILE" + + + diff --git a/tests/reload_push_options/scripts/server-entrypoint.sh b/tests/reload_push_options/scripts/server-entrypoint.sh new file mode 100755 index 00000000000..9f476f9997b --- /dev/null +++ b/tests/reload_push_options/scripts/server-entrypoint.sh @@ -0,0 +1,20 @@ +#!/bin/bash +set -e + +echo "Starting OpenVPN server..." + +# Default push options (used on initial start) +export PUSH_OPTIONS='push "route 192.168.10.0 255.255.255.0" +push "route 192.168.20.0 255.255.255.0" +push "dhcp-option DNS 8.8.8.8"' + +# Generate config from template +envsubst '${PUSH_OPTIONS}' < /etc/openvpn/server.conf.default > /etc/openvpn/server.conf +echo "Generated server config with default push options" + +# Enable IP forwarding (ignore error in container) +echo 1 > /proc/sys/net/ipv4/ip_forward 2>/dev/null || true + +# Start OpenVPN +exec /usr/local/sbin/openvpn --config /etc/openvpn/server.conf + From 2dd10d6cf9547df505cb47f78d91daf51a7dc8c5 Mon Sep 17 00:00:00 2001 From: Moritz Fain Date: Wed, 26 Nov 2025 16:23:05 +0100 Subject: [PATCH 2/2] PUSH_UPDATE: fix option reset logic in continuation messages Previously, the logic for resetting push options (like 'route') was based on `update_options_found` which was local to `apply_push_options`. This meant that if a PUSH_UPDATE was split across multiple continuation messages, the state was lost, causing routes to be reset multiple times (once per message chunk) rather than once per update sequence. This patch moves the state tracking to `struct options` as `push_update_options_found`, allowing it to persist across the entire PUSH_UPDATE sequence. This fixes an issue where large route lists sent via PUSH_UPDATE would result in only the last chunk's routes being applied, or previous routes being continuously deleted and re-added. Added unit tests to verify the fix. --- src/openvpn/options.c | 27 ++--- src/openvpn/options.h | 6 +- src/openvpn/options_parse.c | 8 +- src/openvpn/push.c | 8 +- tests/unit_tests/openvpn/test_options_parse.c | 2 +- .../unit_tests/openvpn/test_push_update_msg.c | 106 +++++++++++++++++- 6 files changed, 131 insertions(+), 26 deletions(-) diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 8a97374e0bb..7b52239e4b2 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -3238,6 +3238,7 @@ pre_connect_restore(struct options *o, struct gc_arena *gc) o->push_continuation = 0; o->push_option_types_found = 0; + o->push_update_options_found = 0; o->imported_protocol_flags = 0; } @@ -5413,14 +5414,14 @@ void update_option(struct context *c, struct options *options, char *p[], bool is_inline, const char *file, int line, const int level, const msglvl_t msglevel, const unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es, unsigned int *update_options_found) + struct env_set *es) { const bool pull_mode = BOOL_CAST(permission_mask & OPT_P_PULL_MODE); ASSERT(MAX_PARMS >= 7); if (streq(p[0], "route") && p[1] && !p[5]) { - if (!(*update_options_found & OPT_P_U_ROUTE)) + if (!(options->push_update_options_found & OPT_P_U_ROUTE)) { VERIFY_PERMISSION(OPT_P_ROUTE); if (!check_route_option(options, p, msglevel, pull_mode)) @@ -5433,12 +5434,12 @@ update_option(struct context *c, struct options *options, char *p[], bool is_inl es, &c->net_ctx); RESET_OPTION_ROUTES(options->routes, routes); } - *update_options_found |= OPT_P_U_ROUTE; + options->push_update_options_found |= OPT_P_U_ROUTE; } } else if (streq(p[0], "route-ipv6") && p[1] && !p[4]) { - if (!(*update_options_found & OPT_P_U_ROUTE6)) + if (!(options->push_update_options_found & OPT_P_U_ROUTE6)) { VERIFY_PERMISSION(OPT_P_ROUTE); if (!check_route6_option(options, p, msglevel, pull_mode)) @@ -5451,12 +5452,12 @@ update_option(struct context *c, struct options *options, char *p[], bool is_inl ROUTE_OPTION_FLAGS(&c->options), es, &c->net_ctx); RESET_OPTION_ROUTES(options->routes_ipv6, routes_ipv6); } - *update_options_found |= OPT_P_U_ROUTE6; + options->push_update_options_found |= OPT_P_U_ROUTE6; } } else if (streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) { - if (!(*update_options_found & OPT_P_U_REDIR_GATEWAY)) + if (!(options->push_update_options_found & OPT_P_U_REDIR_GATEWAY)) { VERIFY_PERMISSION(OPT_P_ROUTE); if (options->routes) @@ -5469,12 +5470,12 @@ update_option(struct context *c, struct options *options, char *p[], bool is_inl } env_set_del(es, "route_redirect_gateway_ipv4"); env_set_del(es, "route_redirect_gateway_ipv6"); - *update_options_found |= OPT_P_U_REDIR_GATEWAY; + options->push_update_options_found |= OPT_P_U_REDIR_GATEWAY; } } else if (streq(p[0], "dns") && p[1]) { - if (!(*update_options_found & OPT_P_U_DNS)) + if (!(options->push_update_options_found & OPT_P_U_DNS)) { VERIFY_PERMISSION(OPT_P_DHCPDNS); if (!check_dns_option(options, p, msglevel, pull_mode)) @@ -5483,13 +5484,13 @@ update_option(struct context *c, struct options *options, char *p[], bool is_inl } gc_free(&options->dns_options.gc); CLEAR(options->dns_options); - *update_options_found |= OPT_P_U_DNS; + options->push_update_options_found |= OPT_P_U_DNS; } } #if defined(_WIN32) || defined(TARGET_ANDROID) else if (streq(p[0], "dhcp-option") && p[1] && !p[3]) { - if (!(*update_options_found & OPT_P_U_DHCP)) + if (!(options->push_update_options_found & OPT_P_U_DHCP)) { struct tuntap_options *o = &options->tuntap_options; VERIFY_PERMISSION(OPT_P_DHCPDNS); @@ -5519,17 +5520,17 @@ update_option(struct context *c, struct options *options, char *p[], bool is_inl o->http_proxy_port = 0; o->http_proxy = NULL; #endif - *update_options_found |= OPT_P_U_DHCP; + options->push_update_options_found |= OPT_P_U_DHCP; } } #else /* if defined(_WIN32) || defined(TARGET_ANDROID) */ else if (streq(p[0], "dhcp-option") && p[1] && !p[3]) { - if (!(*update_options_found & OPT_P_U_DHCP)) + if (!(options->push_update_options_found & OPT_P_U_DHCP)) { VERIFY_PERMISSION(OPT_P_DHCPDNS); delete_all_dhcp_fo(options, &es->list); - *update_options_found |= OPT_P_U_DHCP; + options->push_update_options_found |= OPT_P_U_DHCP; } } #endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ diff --git a/src/openvpn/options.h b/src/openvpn/options.h index bd013dbae53..b98a1d8ec81 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -559,6 +559,7 @@ struct options bool pull; /* client pull of config options from server */ int push_continuation; unsigned int push_option_types_found; + unsigned int push_update_options_found; /* tracks which option types have been reset in current PUSH_UPDATE sequence */ const char *auth_user_pass_file; bool auth_user_pass_file_inline; struct options_pre_connect *pre_connect; @@ -864,14 +865,11 @@ void remove_option(struct context *c, struct options *options, char *p[], bool i * @param option_types_found A pointer to the variable where the flags corresponding to the options * found are stored. * @param es The environment set structure. - * @param update_options_found A pointer to the variable where the flags corresponding to the update - * options found are stored, used to check if an option of the same type has already been processed - * by update_option() within the same push-update message. */ void update_option(struct context *c, struct options *options, char *p[], bool is_inline, const char *file, int line, const int level, const msglvl_t msglevel, const unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es, unsigned int *update_options_found); + struct env_set *es); void parse_argv(struct options *options, const int argc, char *argv[], const msglvl_t msglevel, const unsigned int permission_mask, unsigned int *option_types_found, diff --git a/src/openvpn/options_parse.c b/src/openvpn/options_parse.c index bb5b4049061..4cbd903bfd0 100644 --- a/src/openvpn/options_parse.c +++ b/src/openvpn/options_parse.c @@ -517,7 +517,6 @@ apply_push_options(struct context *c, struct options *options, struct buffer *bu int line_num = 0; const char *file = "[PUSH-OPTIONS]"; const msglvl_t msglevel = D_PUSH_ERRORS | M_OPTERR; - unsigned int update_options_found = 0; while (buf_parse(buf, ',', line, sizeof(line))) { @@ -564,8 +563,13 @@ apply_push_options(struct context *c, struct options *options, struct buffer *bu } else { + /* + * Use persistent push_update_options_found from options struct to track + * which option types have been reset across continuation messages. + * This prevents routes from being reset on each continuation message. + */ update_option(c, options, p, false, file, line_num, 0, msglevel, permission_mask, - option_types_found, es, &update_options_found); + option_types_found, es); } } } diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 7852d360059..898d15851c2 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -540,11 +540,15 @@ incoming_push_message(struct context *c, const struct buffer *buffer) } else { - if (!option_types_found) + /* Clear push_update_options_found for next PUSH_UPDATE sequence */ + c->options.push_update_options_found = 0; + + /* Use accumulated option_types_found for the entire PUSH_UPDATE sequence */ + if (!c->options.push_option_types_found) { msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); } - else if (!do_update(c, option_types_found)) + else if (!do_update(c, c->options.push_option_types_found)) { msg(D_PUSH_ERRORS, "Failed to update options"); goto error; diff --git a/tests/unit_tests/openvpn/test_options_parse.c b/tests/unit_tests/openvpn/test_options_parse.c index 59a3f6df60a..adf3e5ba8b4 100644 --- a/tests/unit_tests/openvpn/test_options_parse.c +++ b/tests/unit_tests/openvpn/test_options_parse.c @@ -60,7 +60,7 @@ void update_option(struct context *c, struct options *options, char *p[], bool is_inline, const char *file, int line, const int level, const msglvl_t msglevel, const unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es, unsigned int *update_options_found) + struct env_set *es) { } diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c index 6ef12668657..5c16001b6ef 100644 --- a/tests/unit_tests/openvpn/test_push_update_msg.c +++ b/tests/unit_tests/openvpn/test_push_update_msg.c @@ -54,6 +54,20 @@ options_postprocess_pull(struct options *options, struct env_set *es) return true; } +/* + * Counters to track route accumulation across continuation messages. + * Used to verify the bug where update_options_found resets per message. + */ +static int route_reset_count = 0; +static int route_add_count = 0; + +static void +reset_route_counters(void) +{ + route_reset_count = 0; + route_add_count = 0; +} + bool apply_push_options(struct context *c, struct options *options, struct buffer *buf, unsigned int permission_mask, unsigned int *option_types_found, @@ -61,6 +75,12 @@ apply_push_options(struct context *c, struct options *options, struct buffer *bu { char line[OPTION_PARM_SIZE]; + /* + * Use persistent push_update_options_found from options struct to track + * which option types have been reset across continuation messages. + * This is the FIXED behavior - routes are only reset once per PUSH_UPDATE sequence. + */ + while (buf_parse(buf, ',', line, sizeof(line))) { unsigned int push_update_option_flags = 0; @@ -84,10 +104,27 @@ apply_push_options(struct context *c, struct options *options, struct buffer *bu return false; /* Cause push/pull error and stop push processing */ } } - /* - * No need to test also the application part here - * (add_option/remove_option/update_option) - */ + + /* Simulate route handling from update_option() in options.c */ + if (strncmp(&line[i], "route ", 6) == 0) + { + if (!(options->push_update_options_found & OPT_P_U_ROUTE)) + { + /* First route in entire PUSH_UPDATE sequence - reset routes once */ + route_reset_count++; + options->push_update_options_found |= OPT_P_U_ROUTE; + } + route_add_count++; + } + /* Simulate add_option() push-continuation logic */ + else if (!strcmp(&line[i], "push-continuation 2")) + { + options->push_continuation = 2; + } + else if (!strcmp(&line[i], "push-continuation 1")) + { + options->push_continuation = 1; + } } return true; } @@ -292,6 +329,65 @@ test_incoming_push_message_mix2(void **state) free_buf(&buf); } +/** + * Test that routes accumulate correctly across multiple continuation messages. + * This test exposes a bug where update_options_found is reset to 0 for each + * continuation message, causing routes to be reset on each message instead + * of accumulating. + * + * Expected behavior: routes should only be reset ONCE (when the first route is received), + * then all subsequent routes should accumulate. + * + * Current bug: routes are reset on the first route of EACH continuation message. + */ +static void +test_incoming_push_continuation_route_accumulation(void **state) +{ + struct context *c = *state; + unsigned int option_types_found = 0; + + reset_route_counters(); + + /* Message 1: first batch of routes, continuation 2 (more coming) */ + struct buffer buf1 = alloc_buf(512); + const char *msg1 = "PUSH_UPDATE, route 10.1.0.0 255.255.0.0, route 10.2.0.0 255.255.0.0, route 10.3.0.0 255.255.0.0,push-continuation 2"; + buf_write(&buf1, msg1, strlen(msg1)); + + assert_int_equal(process_incoming_push_msg(c, &buf1, c->options.pull, pull_permission_mask(c), + &option_types_found), + PUSH_MSG_CONTINUATION); + free_buf(&buf1); + + /* Message 2: more routes, continuation 2 (more coming) */ + struct buffer buf2 = alloc_buf(512); + const char *msg2 = "PUSH_UPDATE, route 10.4.0.0 255.255.0.0, route 10.5.0.0 255.255.0.0, route 10.6.0.0 255.255.0.0,push-continuation 2"; + buf_write(&buf2, msg2, strlen(msg2)); + + assert_int_equal(process_incoming_push_msg(c, &buf2, c->options.pull, pull_permission_mask(c), + &option_types_found), + PUSH_MSG_CONTINUATION); + free_buf(&buf2); + + /* Message 3: final batch of routes, continuation 1 (last message) */ + struct buffer buf3 = alloc_buf(512); + const char *msg3 = "PUSH_UPDATE, route 10.7.0.0 255.255.0.0, route 10.8.0.0 255.255.0.0, route 10.9.0.0 255.255.0.0,push-continuation 1"; + buf_write(&buf3, msg3, strlen(msg3)); + + assert_int_equal(process_incoming_push_msg(c, &buf3, c->options.pull, pull_permission_mask(c), + &option_types_found), + PUSH_MSG_UPDATE); + free_buf(&buf3); + + /* Verify: all 9 routes should have been added */ + assert_int_equal(route_add_count, 9); + + /* + * Verify: route option is reset only one time in the first message + * if a push-continuation is present. + */ + assert_int_equal(route_reset_count, 1); +} + #ifdef ENABLE_MANAGEMENT char *r0[] = { "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0", @@ -603,6 +699,8 @@ main(void) cmocka_unit_test_setup_teardown(test_incoming_push_message_bad_format, setup, teardown), cmocka_unit_test_setup_teardown(test_incoming_push_message_mix, setup, teardown), cmocka_unit_test_setup_teardown(test_incoming_push_message_mix2, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_continuation_route_accumulation, setup, + teardown), #ifdef ENABLE_MANAGEMENT cmocka_unit_test_setup_teardown(test_send_push_msg0, setup2, teardown2),