From 8f98c18e1d3c95b3ce95b5dec05a57d1c68b1721 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 17 Jan 2026 15:11:25 +0200 Subject: [PATCH 1/9] feat(dhcp-inform): add provider architecture for flexible route sources BREAKING: Remove hard PostgreSQL dependency - plugin now works without database. Route sources (in priority order): 1. Traffic Selectors (EXCLUSIVE mode) - for Windows 7 compatibility 2. Database (PostgreSQL/MySQL/SQLite) - if configured 3. Static configuration from strongswan.conf New files: - dhcp_inform_provider.h: Route provider interface - dhcp_inform_ts_provider: Extract routes from IKE SA traffic selectors - dhcp_inform_db_provider: Database routes (refactored from responder) - dhcp_inform_static_provider: Config-based routes with per-pool support Configuration changes: - use_ts_routes: Enable exclusive TS mode - routes{}: Global static routes for all clients - pools{name}{cidr,routes{}}: Per-pool route overrides RPM: strongswan-pgsql changed from Requires to Recommends --- packaging/rpm/strongswan-sw.spec | 13 +- src/libcharon/plugins/dhcp_inform/Makefile.am | 6 +- .../plugins/dhcp_inform/dhcp-inform.conf | 70 ++- .../dhcp_inform/dhcp_inform_db_provider.c | 249 ++++++++++ .../dhcp_inform/dhcp_inform_db_provider.h | 50 ++ .../plugins/dhcp_inform/dhcp_inform_plugin.c | 17 +- .../plugins/dhcp_inform/dhcp_inform_plugin.h | 19 +- .../dhcp_inform/dhcp_inform_provider.h | 62 +++ .../dhcp_inform/dhcp_inform_responder.c | 263 +++++++--- .../dhcp_inform/dhcp_inform_responder.h | 4 +- .../dhcp_inform/dhcp_inform_static_provider.c | 466 ++++++++++++++++++ .../dhcp_inform/dhcp_inform_static_provider.h | 49 ++ .../dhcp_inform/dhcp_inform_ts_provider.c | 242 +++++++++ .../dhcp_inform/dhcp_inform_ts_provider.h | 49 ++ 14 files changed, 1451 insertions(+), 108 deletions(-) create mode 100644 src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c create mode 100644 src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.h create mode 100644 src/libcharon/plugins/dhcp_inform/dhcp_inform_provider.h create mode 100644 src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c create mode 100644 src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.h create mode 100644 src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c create mode 100644 src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.h diff --git a/packaging/rpm/strongswan-sw.spec b/packaging/rpm/strongswan-sw.spec index e4dbc59077..aaf4697cb4 100644 --- a/packaging/rpm/strongswan-sw.spec +++ b/packaging/rpm/strongswan-sw.spec @@ -77,12 +77,17 @@ Enables storing VPN user credentials and configuration in PostgreSQL. %package -n strongswan-dhcp-inform Summary: DHCP INFORM responder plugin for strongSwan Requires: strongswan-sw = %{version}-%{release} -# dhcp-inform plugin stores/retrieves split-tunnel routes in PostgreSQL database -Requires: strongswan-pgsql = %{version}-%{release} +# Database is optional - plugin can work with static routes or TS routes +Recommends: strongswan-pgsql = %{version}-%{release} %description -n strongswan-dhcp-inform -Responds to Windows DHCPINFORM requests with split-tunnel routes -from PostgreSQL database. Delivers routes via DHCP option 121/249. +Responds to Windows DHCPINFORM requests with split-tunnel routes. +Delivers routes via DHCP option 121/249. + +Route sources (in priority order): +1. Traffic Selectors - EXCLUSIVE mode for Windows 7 compatibility +2. Database (PostgreSQL/MySQL/SQLite) - if configured +3. Static configuration from strongswan.conf %prep %autosetup -n strongswan-%{upstream_version}-sw.%{sw_rev} diff --git a/src/libcharon/plugins/dhcp_inform/Makefile.am b/src/libcharon/plugins/dhcp_inform/Makefile.am index 8f2707888d..e791c73604 100644 --- a/src/libcharon/plugins/dhcp_inform/Makefile.am +++ b/src/libcharon/plugins/dhcp_inform/Makefile.am @@ -13,6 +13,10 @@ endif libstrongswan_dhcp_inform_la_SOURCES = \ dhcp_inform_plugin.h dhcp_inform_plugin.c \ - dhcp_inform_responder.h dhcp_inform_responder.c + dhcp_inform_responder.h dhcp_inform_responder.c \ + dhcp_inform_provider.h \ + dhcp_inform_static_provider.h dhcp_inform_static_provider.c \ + dhcp_inform_ts_provider.h dhcp_inform_ts_provider.c \ + dhcp_inform_db_provider.h dhcp_inform_db_provider.c libstrongswan_dhcp_inform_la_LDFLAGS = -module -avoid-version diff --git a/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf b/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf index e7b1019dbd..10c09de4b8 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf +++ b/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf @@ -1,20 +1,72 @@ # dhcp-inform plugin configuration -# Responds to DHCPINFORM from Windows VPN clients with routes from database +# Responds to DHCPINFORM from Windows VPN clients with split-tunnel routes +# +# THREE MUTUALLY EXCLUSIVE MODES (priority order): +# 1. Traffic Selectors - when use_ts_routes=yes +# 2. Database - when database URI configured +# 3. Static configuration - when no database +# +# Modes are EXCLUSIVE - they never combine. Configure only ONE mode. dhcp-inform { # Enable the plugin load = yes - # PostgreSQL database connection - # Same as attr-sql database - database = postgresql://vpn_strongswan:@localhost/vpn_admin - - # VPN interface to listen on (optional, listens on all if not set) - interface = eth0 - # Server IP address (required - usually the VPN gateway IP) server = 172.16.69.1 + # VPN interface to listen on (optional, listens on all if not set) + # interface = eth0 + # DNS server to advertise (optional) - dns = 172.16.69.1 + # dns = 172.16.69.1 + + # ========================================================================= + # MODE 1: Traffic Selector Routes (EXCLUSIVE) + # ========================================================================= + # Routes are extracted from IKE SA traffic selectors. + # Designed for Windows 7 compatibility (doesn't handle TS properly). + # When enabled, database and static routes are IGNORED. + # + # use_ts_routes = yes + + # ========================================================================= + # MODE 2: Database Routes (EXCLUSIVE) + # ========================================================================= + # Routes are queried from database by matching client IP to pool CIDR. + # When database is configured, static routes are IGNORED. + # Requires v_pool_routes view: (pool_cidr, route) + # + # database = postgresql://vpn_user:password@localhost/vpn_db + + # ========================================================================= + # MODE 3: Static Routes (EXCLUSIVE - only when no database) + # ========================================================================= + # Used ONLY when no database is configured. + # + # Global routes - apply to all clients not matching a specific pool: + # + # routes { + # route1 = 10.0.0.0/8 + # route2 = 172.16.0.0/12 + # route3 = 192.168.0.0/16 + # } + # + # Per-pool route overrides - clients matching pool CIDR get these instead: + # + # pools { + # production { + # cidr = 10.100.0.0/16 + # routes { + # r1 = 192.168.1.0/24 + # r2 = 192.168.2.0/24 + # } + # } + # development { + # cidr = 10.200.0.0/16 + # routes { + # r1 = 10.50.0.0/16 + # } + # } + # } } diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c new file mode 100644 index 0000000000..3d4e11a875 --- /dev/null +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c @@ -0,0 +1,249 @@ +/* + * Copyright (C) 2025 Structured World Foundation + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#include "dhcp_inform_db_provider.h" + +#include +#include +#include +#include +#include + +typedef struct private_dhcp_inform_db_provider_t private_dhcp_inform_db_provider_t; + +/** + * Private data + */ +struct private_dhcp_inform_db_provider_t { + + /** + * Public interface + */ + dhcp_inform_db_provider_t public; + + /** + * Database connection + */ + database_t *db; +}; + +/** + * Parse CIDR notation to traffic_selector + */ +static traffic_selector_t *parse_cidr(const char *cidr) +{ + char *slash, *ip_str; + int prefix = 32; + host_t *host; + traffic_selector_t *ts = NULL; + + if (!cidr || !*cidr) + { + DBG1(DBG_CFG, "dhcp-inform-db: CORRUPTED DATA - empty CIDR"); + return NULL; + } + + if (strlen(cidr) > 43) + { + DBG1(DBG_CFG, "dhcp-inform-db: CORRUPTED DATA - CIDR too long: %.20s...", + cidr); + return NULL; + } + + ip_str = strdup(cidr); + if (!ip_str) + { + return NULL; + } + + slash = strchr(ip_str, '/'); + if (slash) + { + *slash = '\0'; + prefix = atoi(slash + 1); + if (prefix < 0 || prefix > 32) + { + DBG1(DBG_CFG, "dhcp-inform-db: invalid prefix %d in %s", + prefix, cidr); + free(ip_str); + return NULL; + } + } + + host = host_create_from_string(ip_str, 0); + if (!host) + { + DBG1(DBG_CFG, "dhcp-inform-db: invalid IP in CIDR: %s", ip_str); + free(ip_str); + return NULL; + } + + ts = traffic_selector_create_from_subnet(host, prefix, 0, 0, 65535); + host->destroy(host); + free(ip_str); + + return ts; +} + +METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, + private_dhcp_inform_db_provider_t *this, const char *client_ip) +{ + linked_list_t *routes; + enumerator_t *enumerator; + char *route_value; + int routes_parsed = 0; + int routes_failed = 0; + + routes = linked_list_create(); + + if (!this->db) + { + return routes; + } + + if (!client_ip || !*client_ip) + { + DBG1(DBG_CFG, "dhcp-inform-db: CORRUPTED DATA - empty client IP"); + return routes; + } + + DBG2(DBG_CFG, "dhcp-inform-db: looking up routes for IP %s", client_ip); + + /* Query routes for the pool that contains this IP + * Uses v_pool_routes VIEW: (pool_cidr, route) + */ + enumerator = this->db->query(this->db, + "SELECT route FROM v_pool_routes WHERE ?::inet << pool_cidr::inet", + DB_TEXT, client_ip, + DB_TEXT); + + if (!enumerator) + { + DBG2(DBG_CFG, "dhcp-inform-db: primary query failed, trying fallback"); + /* Fallback: get routes from auto_ip_pools directly */ + enumerator = this->db->query(this->db, + "SELECT unnest(aip.routes) as route " + "FROM auto_ip_pools aip " + "JOIN environments e ON e.auto_ip_pool_id = aip.id " + "WHERE e.is_active = true " + "AND ?::inet << aip.cidr::inet", + DB_TEXT, client_ip, + DB_TEXT); + } + + if (!enumerator) + { + DBG1(DBG_CFG, "dhcp-inform-db: all queries failed for %s", client_ip); + return routes; + } + + while (enumerator->enumerate(enumerator, &route_value)) + { + traffic_selector_t *ts; + + routes_parsed++; + + if (!route_value) + { + DBG1(DBG_CFG, "dhcp-inform-db: CORRUPTED DATA - NULL route at row %d", + routes_parsed); + routes_failed++; + continue; + } + + ts = parse_cidr(route_value); + if (ts) + { + routes->insert_last(routes, ts); + DBG2(DBG_CFG, "dhcp-inform-db: added route %s", route_value); + } + else + { + DBG1(DBG_CFG, "dhcp-inform-db: failed to parse route: %s", + route_value); + routes_failed++; + } + } + enumerator->destroy(enumerator); + + if (routes_failed > 0) + { + DBG1(DBG_CFG, "dhcp-inform-db: WARNING - %d/%d routes had corrupted data", + routes_failed, routes_parsed); + } + + DBG1(DBG_CFG, "dhcp-inform-db: found %d valid routes for %s", + routes->get_count(routes), client_ip); + + return routes; +} + +METHOD(dhcp_inform_provider_t, get_name, const char*, + private_dhcp_inform_db_provider_t *this) +{ + return "database"; +} + +METHOD(dhcp_inform_provider_t, is_available, bool, + private_dhcp_inform_db_provider_t *this) +{ + return this->db != NULL; +} + +METHOD(dhcp_inform_provider_t, destroy, void, + private_dhcp_inform_db_provider_t *this) +{ + DESTROY_IF(this->db); + free(this); +} + +/** + * See header + */ +dhcp_inform_db_provider_t *dhcp_inform_db_provider_create() +{ + private_dhcp_inform_db_provider_t *this; + char *db_uri; + + INIT(this, + .public = { + .provider = { + .get_routes = _get_routes, + .get_name = _get_name, + .is_available = _is_available, + .destroy = _destroy, + }, + }, + ); + + /* Get database URI from configuration */ + db_uri = lib->settings->get_str(lib->settings, + "%s.plugins.dhcp-inform.database", NULL, lib->ns); + + if (db_uri) + { + this->db = lib->db->create(lib->db, db_uri); + if (this->db) + { + DBG1(DBG_CFG, "dhcp-inform: database provider connected"); + } + else + { + DBG1(DBG_CFG, "dhcp-inform: failed to connect to database, " + "database provider disabled"); + } + } + else + { + DBG2(DBG_CFG, "dhcp-inform: no database configured, " + "database provider disabled"); + } + + return &this->public; +} diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.h b/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.h new file mode 100644 index 0000000000..e589a8f04f --- /dev/null +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.h @@ -0,0 +1,50 @@ +/* + * Copyright (C) 2025 Structured World Foundation + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +/** + * @defgroup dhcp_inform_db_provider dhcp_inform_db_provider + * @{ @ingroup dhcp_inform + */ + +#ifndef DHCP_INFORM_DB_PROVIDER_H_ +#define DHCP_INFORM_DB_PROVIDER_H_ + +#include "dhcp_inform_provider.h" + +typedef struct dhcp_inform_db_provider_t dhcp_inform_db_provider_t; + +/** + * Database route provider - reads routes from SQL database. + * + * Supports PostgreSQL, MySQL, SQLite via strongSwan database abstraction. + * Queries routes from v_pool_routes view based on client virtual IP. + * + * Configuration: + * charon.plugins.dhcp-inform.database = pgsql://user:pass@host/db + * + * Required database schema: + * VIEW v_pool_routes (pool_cidr, route) + * - Returns routes for pools, client IP matched against pool_cidr + */ +struct dhcp_inform_db_provider_t { + + /** + * Implements dhcp_inform_provider_t interface + */ + dhcp_inform_provider_t provider; +}; + +/** + * Create database route provider. + * + * @return provider instance, NULL on failure + */ +dhcp_inform_db_provider_t *dhcp_inform_db_provider_create(); + +#endif /** DHCP_INFORM_DB_PROVIDER_H_ @}*/ diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_plugin.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_plugin.c index a8f75e4495..eaa0e7588e 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_plugin.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_plugin.c @@ -7,6 +7,18 @@ * option) any later version. */ +/** + * DHCP INFORM Plugin - responds to Windows DHCPINFORM with routes. + * + * Route sources (in priority order): + * 1. Traffic Selectors (EXCLUSIVE mode - when enabled, only TS routes used) + * 2. Database (PostgreSQL/MySQL/SQLite - optional) + * 3. Static configuration (from strongswan.conf) + * + * The plugin works WITHOUT any database if static routes or TS routes are + * configured. Database plugins are soft dependencies. + */ + #include "dhcp_inform_plugin.h" #include "dhcp_inform_responder.h" @@ -65,8 +77,9 @@ METHOD(plugin_t, get_features, int, static plugin_feature_t f[] = { PLUGIN_CALLBACK((plugin_feature_callback_t)plugin_cb, NULL), PLUGIN_PROVIDE(CUSTOM, "dhcp-inform"), - PLUGIN_DEPENDS(DATABASE, DB_PGSQL), - PLUGIN_SDEPEND(CUSTOM, "attr-sql"), + /* Database plugins are optional - plugin works with static + * routes or TS routes without any database */ + PLUGIN_SDEPEND(DATABASE, DB_ANY), }; *features = f; return countof(f); diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_plugin.h b/src/libcharon/plugins/dhcp_inform/dhcp_inform_plugin.h index a9ed8ac0f9..ea09a71f5c 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_plugin.h +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_plugin.h @@ -8,11 +8,11 @@ */ /** - * @defgroup dhcp_inform_p dhcp_inform + * @defgroup dhcp_inform dhcp_inform * @ingroup cplugins * * @defgroup dhcp_inform_plugin dhcp_inform_plugin - * @{ @ingroup dhcp_inform_p + * @{ @ingroup dhcp_inform */ #ifndef DHCP_INFORM_PLUGIN_H_ @@ -23,16 +23,17 @@ typedef struct dhcp_inform_plugin_t dhcp_inform_plugin_t; /** - * Plugin responding to DHCPINFORM with routes from PostgreSQL database. + * Plugin responding to DHCPINFORM with split-tunnel routes. * * Windows VPN clients send DHCPINFORM after IKEv2 connection to get - * split-tunnel routes via DHCP option 249 (Microsoft Classless Static Routes). + * split-tunnel routes via DHCP option 121/249. * - * This plugin: - * - Listens for DHCPINFORM on the VPN interface - * - Looks up routes by matching client's virtual IP to network pool CIDR - * - Queries PostgreSQL for routes configured in the pool's environment - * - Responds with DHCPACK containing option 121/249 with routes + * Route sources (in priority order): + * 1. Traffic Selectors - EXCLUSIVE mode (for Windows 7 compatibility) + * 2. Database (PostgreSQL/MySQL/SQLite) - if configured + * 3. Static configuration from strongswan.conf + * + * The plugin works WITHOUT any database when using static or TS routes. */ struct dhcp_inform_plugin_t { diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_provider.h b/src/libcharon/plugins/dhcp_inform/dhcp_inform_provider.h new file mode 100644 index 0000000000..8264a4e403 --- /dev/null +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_provider.h @@ -0,0 +1,62 @@ +/* + * Copyright (C) 2025 Structured World Foundation + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +/** + * @defgroup dhcp_inform_provider dhcp_inform_provider + * @{ @ingroup dhcp_inform + */ + +#ifndef DHCP_INFORM_PROVIDER_H_ +#define DHCP_INFORM_PROVIDER_H_ + +#include + +typedef struct dhcp_inform_provider_t dhcp_inform_provider_t; + +/** + * Route provider interface for DHCP INFORM plugin. + * + * Implementations provide routes from different sources: + * - Database (pgsql/mysql/sqlite) + * - Static configuration + * - IKE Traffic Selectors + */ +struct dhcp_inform_provider_t { + + /** + * Get routes for a client by virtual IP. + * + * @param client_ip client's virtual IP address string + * @return linked_list_t of traffic_selector_t (caller destroys), + * or NULL on error + */ + linked_list_t* (*get_routes)(dhcp_inform_provider_t *this, + const char *client_ip); + + /** + * Get provider name for logging. + * + * @return provider name string (static, do not free) + */ + const char* (*get_name)(dhcp_inform_provider_t *this); + + /** + * Check if provider is available/configured. + * + * @return TRUE if provider can provide routes + */ + bool (*is_available)(dhcp_inform_provider_t *this); + + /** + * Destroy provider instance. + */ + void (*destroy)(dhcp_inform_provider_t *this); +}; + +#endif /** DHCP_INFORM_PROVIDER_H_ @}*/ diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c index 2ee268087e..166fde32a7 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c @@ -1,9 +1,14 @@ /* * Copyright (C) 2025 Structured World Foundation * - * DHCP Inform Responder - responds to Windows DHCPINFORM with routes from DB. + * DHCP Inform Responder - responds to Windows DHCPINFORM with routes. * Uses packet socket like forecast plugin to catch broadcast from VPN tunnels. * + * Route sources (in priority order): + * 1. Traffic Selectors (EXCLUSIVE - when enabled, only TS routes are used) + * 2. Database (if configured and available) + * 3. Static configuration (from strongswan.conf) + * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the * Free Software Foundation; either version 2 of the License, or (at your @@ -11,12 +16,15 @@ */ #include "dhcp_inform_responder.h" +#include "dhcp_inform_provider.h" +#include "dhcp_inform_ts_provider.h" +#include "dhcp_inform_db_provider.h" +#include "dhcp_inform_static_provider.h" #include #include #include #include -#include #include #include @@ -104,9 +112,19 @@ struct private_dhcp_inform_responder_t { dhcp_inform_responder_t public; /** - * Database connection + * Traffic Selector route provider (EXCLUSIVE mode) + */ + dhcp_inform_ts_provider_t *ts_provider; + + /** + * Database route provider (optional) + */ + dhcp_inform_db_provider_t *db_provider; + + /** + * Static route provider (from config) */ - database_t *db; + dhcp_inform_static_provider_t *static_provider; /** * Packet socket for receiving broadcasts (AF_PACKET) @@ -201,17 +219,93 @@ static traffic_selector_t *parse_cidr(const char *cidr) } /** - * Get routes from database by client virtual IP. - * Looks up routes for the pool that contains this IP. + * Check if traffic selector already exists in list (deduplication) */ -static linked_list_t *get_routes_by_ip(private_dhcp_inform_responder_t *this, - const char *client_ip) +static bool route_exists_in_list(linked_list_t *list, traffic_selector_t *ts) { - linked_list_t *routes; enumerator_t *enumerator; - char *route_value; - int routes_parsed = 0; - int routes_failed = 0; + traffic_selector_t *existing; + bool found = FALSE; + + enumerator = list->create_enumerator(list); + while (enumerator->enumerate(enumerator, &existing)) + { + if (ts->equals(ts, existing)) + { + found = TRUE; + break; + } + } + enumerator->destroy(enumerator); + + return found; +} + +/** + * Add routes from provider to list with deduplication + */ +static void add_routes_from_provider(dhcp_inform_provider_t *provider, + const char *client_ip, + linked_list_t *routes) +{ + linked_list_t *provider_routes; + enumerator_t *enumerator; + traffic_selector_t *ts; + int added = 0; + int duplicates = 0; + + if (!provider || !provider->is_available(provider)) + { + return; + } + + provider_routes = provider->get_routes(provider, client_ip); + if (!provider_routes) + { + return; + } + + enumerator = provider_routes->create_enumerator(provider_routes); + while (enumerator->enumerate(enumerator, &ts)) + { + if (route_exists_in_list(routes, ts)) + { + duplicates++; + ts->destroy(ts); + } + else + { + routes->insert_last(routes, ts); + added++; + } + } + enumerator->destroy(enumerator); + + /* Just destroy the list, not the routes (we moved ownership) */ + provider_routes->destroy(provider_routes); + + if (added > 0 || duplicates > 0) + { + DBG2(DBG_NET, "dhcp-inform: %s provider added %d routes (%d duplicates)", + provider->get_name(provider), added, duplicates); + } +} + +/** + * Get routes for client using available providers. + * + * THREE MUTUALLY EXCLUSIVE MODES (priority order): + * 1. TS routes - when use_ts_routes=yes, ONLY traffic selector routes + * 2. DB routes - when database configured, ONLY database routes + * 3. Static routes - when no DB, use config routes with per-pool overrides + * + * Modes are exclusive - they never combine. + */ +static linked_list_t *get_routes_for_client(private_dhcp_inform_responder_t *this, + const char *client_ip) +{ + linked_list_t *routes; + dhcp_inform_provider_t *ts_prov, *db_prov, *static_prov; routes = linked_list_create(); if (!routes) @@ -220,12 +314,6 @@ static linked_list_t *get_routes_by_ip(private_dhcp_inform_responder_t *this, return NULL; } - if (!this->db) - { - DBG1(DBG_NET, "dhcp-inform: no database connection"); - return routes; - } - if (!client_ip || !*client_ip) { DBG1(DBG_NET, "dhcp-inform: CORRUPTED DATA - empty client IP"); @@ -234,68 +322,45 @@ static linked_list_t *get_routes_by_ip(private_dhcp_inform_responder_t *this, DBG1(DBG_NET, "dhcp-inform: looking up routes for IP %s", client_ip); - /* Query routes for the environment/pool that contains this IP - * Uses v_pool_routes VIEW: (pool_cidr, route) - * We check if client_ip falls within pool_cidr - */ - enumerator = this->db->query(this->db, - "SELECT route FROM v_pool_routes WHERE ?::inet << pool_cidr::inet", - DB_TEXT, client_ip, - DB_TEXT); - - if (!enumerator) - { - DBG1(DBG_NET, "dhcp-inform: primary query failed, trying fallback"); - /* Fallback: get routes from auto_ip_pools directly */ - enumerator = this->db->query(this->db, - "SELECT unnest(aip.routes) as route " - "FROM auto_ip_pools aip " - "JOIN environments e ON e.auto_ip_pool_id = aip.id " - "WHERE e.is_active = true " - "AND ?::inet << aip.cidr::inet", - DB_TEXT, client_ip, - DB_TEXT); - } + /* Get provider interfaces */ + ts_prov = this->ts_provider ? + &this->ts_provider->provider : NULL; + db_prov = this->db_provider ? + &this->db_provider->provider : NULL; + static_prov = this->static_provider ? + &this->static_provider->provider : NULL; - if (!enumerator) + /* MODE 1: TS routes (exclusive) */ + if (ts_prov && ts_prov->is_available(ts_prov)) { - DBG1(DBG_NET, "dhcp-inform: all queries failed for %s", client_ip); + DBG1(DBG_NET, "dhcp-inform: using TS routes mode (exclusive)"); + add_routes_from_provider(ts_prov, client_ip, routes); + DBG1(DBG_NET, "dhcp-inform: found %d routes from TS for %s", + routes->get_count(routes), client_ip); return routes; } - while (enumerator->enumerate(enumerator, &route_value)) + /* MODE 2: Database routes (exclusive) */ + if (db_prov && db_prov->is_available(db_prov)) { - routes_parsed++; - - if (!route_value) - { - DBG1(DBG_NET, "dhcp-inform: CORRUPTED DATA - NULL route value at row %d", routes_parsed); - routes_failed++; - continue; - } - - traffic_selector_t *ts = parse_cidr(route_value); - if (ts) - { - routes->insert_last(routes, ts); - DBG1(DBG_NET, "dhcp-inform: added route %s", route_value); - } - else - { - DBG1(DBG_NET, "dhcp-inform: CORRUPTED DATA - failed to parse route: %s", route_value); - routes_failed++; - } + DBG1(DBG_NET, "dhcp-inform: using database routes mode (exclusive)"); + add_routes_from_provider(db_prov, client_ip, routes); + DBG1(DBG_NET, "dhcp-inform: found %d routes from DB for %s", + routes->get_count(routes), client_ip); + return routes; } - enumerator->destroy(enumerator); - if (routes_failed > 0) + /* MODE 3: Static routes with per-pool overrides (exclusive) */ + if (static_prov && static_prov->is_available(static_prov)) { - DBG1(DBG_NET, "dhcp-inform: WARNING - %d/%d routes had corrupted data", - routes_failed, routes_parsed); + DBG1(DBG_NET, "dhcp-inform: using static routes mode (exclusive)"); + add_routes_from_provider(static_prov, client_ip, routes); + DBG1(DBG_NET, "dhcp-inform: found %d routes from config for %s", + routes->get_count(routes), client_ip); + return routes; } - DBG1(DBG_NET, "dhcp-inform: found %d valid routes for %s", - routes->get_count(routes), client_ip); + DBG1(DBG_NET, "dhcp-inform: no route provider available for %s", client_ip); return routes; } @@ -715,8 +780,8 @@ static void process_dhcp_packet(private_dhcp_inform_responder_t *this, inet_ntop(AF_INET, &dhcp->ciaddr, client_ip_str, sizeof(client_ip_str)); DBG1(DBG_NET, "dhcp-inform: received DHCPINFORM from %s", client_ip_str); - /* Get routes from database by client IP */ - routes = get_routes_by_ip(this, client_ip_str); + /* Get routes from providers (TS exclusive, or DB + static combined) */ + routes = get_routes_for_client(this, client_ip_str); if (!routes) { @@ -846,7 +911,21 @@ METHOD(dhcp_inform_responder_t, destroy, void, { close(this->raw_fd); } - DESTROY_IF(this->db); + + /* Destroy providers */ + if (this->ts_provider) + { + this->ts_provider->provider.destroy(&this->ts_provider->provider); + } + if (this->db_provider) + { + this->db_provider->provider.destroy(&this->db_provider->provider); + } + if (this->static_provider) + { + this->static_provider->provider.destroy(&this->static_provider->provider); + } + free(this->iface); free(this); } @@ -857,8 +936,9 @@ METHOD(dhcp_inform_responder_t, destroy, void, dhcp_inform_responder_t *dhcp_inform_responder_create() { private_dhcp_inform_responder_t *this; - char *db_uri, *iface, *server_ip, *dns_server; + char *iface, *server_ip, *dns_server; int on = 1; + bool has_routes = FALSE; INIT(this, .public = { @@ -869,8 +949,6 @@ dhcp_inform_responder_t *dhcp_inform_responder_create() ); /* Get configuration */ - db_uri = lib->settings->get_str(lib->settings, - "%s.plugins.dhcp-inform.database", NULL, lib->ns); iface = lib->settings->get_str(lib->settings, "%s.plugins.dhcp-inform.interface", NULL, lib->ns); server_ip = lib->settings->get_str(lib->settings, @@ -878,18 +956,41 @@ dhcp_inform_responder_t *dhcp_inform_responder_create() dns_server = lib->settings->get_str(lib->settings, "%s.plugins.dhcp-inform.dns", NULL, lib->ns); - if (!db_uri || !server_ip) + if (!server_ip) { - DBG1(DBG_NET, "dhcp-inform: missing database or server config"); + DBG1(DBG_NET, "dhcp-inform: missing server config"); destroy(this); return NULL; } - /* Connect to database */ - this->db = lib->db->create(lib->db, db_uri); - if (!this->db) + /* Initialize route providers */ + this->ts_provider = dhcp_inform_ts_provider_create(); + this->db_provider = dhcp_inform_db_provider_create(); + this->static_provider = dhcp_inform_static_provider_create(); + + /* Check if any provider is available */ + if (this->ts_provider && + this->ts_provider->provider.is_available(&this->ts_provider->provider)) + { + DBG1(DBG_NET, "dhcp-inform: TS provider enabled (EXCLUSIVE mode)"); + has_routes = TRUE; + } + if (this->db_provider && + this->db_provider->provider.is_available(&this->db_provider->provider)) + { + DBG1(DBG_NET, "dhcp-inform: database provider enabled"); + has_routes = TRUE; + } + if (this->static_provider && + this->static_provider->provider.is_available(&this->static_provider->provider)) + { + DBG1(DBG_NET, "dhcp-inform: static provider enabled"); + has_routes = TRUE; + } + + if (!has_routes) { - DBG1(DBG_NET, "dhcp-inform: failed to connect to database"); + DBG1(DBG_NET, "dhcp-inform: no route sources configured, plugin disabled"); destroy(this); return NULL; } diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.h b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.h index 91a9577a18..9dc97c579e 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.h +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.h @@ -9,7 +9,7 @@ /** * @defgroup dhcp_inform_responder dhcp_inform_responder - * @{ @ingroup dhcp_inform_p + * @{ @ingroup dhcp_inform */ #ifndef DHCP_INFORM_RESPONDER_H_ @@ -18,7 +18,7 @@ typedef struct dhcp_inform_responder_t dhcp_inform_responder_t; /** - * DHCPINFORM responder that sends routes from database. + * DHCPINFORM responder that sends routes via DHCP option 121/249. */ struct dhcp_inform_responder_t { diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c new file mode 100644 index 0000000000..c129486827 --- /dev/null +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c @@ -0,0 +1,466 @@ +/* + * Copyright (C) 2025 Structured World Foundation + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#include "dhcp_inform_static_provider.h" + +#include +#include +#include +#include + +typedef struct private_dhcp_inform_static_provider_t private_dhcp_inform_static_provider_t; + +/** + * Pool configuration entry + */ +typedef struct { + /** Pool name (informational) */ + char *name; + /** Pool CIDR as host_t with prefix */ + host_t *network; + /** Prefix length */ + uint8_t prefix; + /** Routes for this pool */ + linked_list_t *routes; +} pool_entry_t; + +/** + * Private data + */ +struct private_dhcp_inform_static_provider_t { + + /** + * Public interface + */ + dhcp_inform_static_provider_t public; + + /** + * Global routes (apply to all clients not matching a pool) + */ + linked_list_t *global_routes; + + /** + * Per-pool route configurations + */ + linked_list_t *pools; + + /** + * Whether any routes are configured + */ + bool has_routes; +}; + +/** + * Destroy a pool entry + */ +static void pool_entry_destroy(pool_entry_t *entry) +{ + if (entry) + { + free(entry->name); + DESTROY_IF(entry->network); + if (entry->routes) + { + entry->routes->destroy_offset(entry->routes, + offsetof(traffic_selector_t, destroy)); + } + free(entry); + } +} + +/** + * Parse CIDR notation to traffic_selector + */ +static traffic_selector_t *parse_cidr(const char *cidr) +{ + char *slash, *ip_str; + int prefix = 32; + host_t *host; + traffic_selector_t *ts = NULL; + + if (!cidr || !*cidr) + { + return NULL; + } + + if (strlen(cidr) > 43) + { + DBG1(DBG_CFG, "dhcp-inform: CIDR too long: %.20s...", cidr); + return NULL; + } + + ip_str = strdup(cidr); + if (!ip_str) + { + return NULL; + } + + slash = strchr(ip_str, '/'); + if (slash) + { + *slash = '\0'; + prefix = atoi(slash + 1); + if (prefix < 0 || prefix > 32) + { + DBG1(DBG_CFG, "dhcp-inform: invalid prefix %d in %s", prefix, cidr); + free(ip_str); + return NULL; + } + } + + host = host_create_from_string(ip_str, 0); + if (!host) + { + DBG1(DBG_CFG, "dhcp-inform: invalid IP in CIDR: %s", ip_str); + free(ip_str); + return NULL; + } + + ts = traffic_selector_create_from_subnet(host, prefix, 0, 0, 65535); + host->destroy(host); + free(ip_str); + + return ts; +} + +/** + * Parse CIDR to host and prefix for pool matching + */ +static bool parse_cidr_to_host(const char *cidr, host_t **host, uint8_t *prefix) +{ + char *slash, *ip_str; + int pfx = 32; + + if (!cidr || !*cidr) + { + return FALSE; + } + + ip_str = strdup(cidr); + if (!ip_str) + { + return FALSE; + } + + slash = strchr(ip_str, '/'); + if (slash) + { + *slash = '\0'; + pfx = atoi(slash + 1); + if (pfx < 0 || pfx > 32) + { + free(ip_str); + return FALSE; + } + } + + *host = host_create_from_string(ip_str, 0); + free(ip_str); + + if (!*host) + { + return FALSE; + } + + *prefix = pfx; + return TRUE; +} + +/** + * Check if an IP address falls within a network/prefix + */ +static bool ip_in_subnet(host_t *ip, host_t *network, uint8_t prefix) +{ + chunk_t ip_addr, net_addr; + uint8_t *ip_ptr, *net_ptr; + int bytes, bits, i; + uint8_t mask; + + if (ip->get_family(ip) != network->get_family(network)) + { + return FALSE; + } + + ip_addr = ip->get_address(ip); + net_addr = network->get_address(network); + + if (ip_addr.len != net_addr.len) + { + return FALSE; + } + + bytes = prefix / 8; + bits = prefix % 8; + + ip_ptr = ip_addr.ptr; + net_ptr = net_addr.ptr; + + /* Compare full bytes */ + for (i = 0; i < bytes && i < (int)ip_addr.len; i++) + { + if (ip_ptr[i] != net_ptr[i]) + { + return FALSE; + } + } + + /* Compare remaining bits */ + if (bits > 0 && bytes < (int)ip_addr.len) + { + mask = 0xFF << (8 - bits); + if ((ip_ptr[bytes] & mask) != (net_ptr[bytes] & mask)) + { + return FALSE; + } + } + + return TRUE; +} + +/** + * Load routes from a config section + */ +static linked_list_t *load_routes_from_section(const char *section) +{ + linked_list_t *routes; + enumerator_t *enumerator; + char *key, *value; + int count = 0; + + routes = linked_list_create(); + + enumerator = lib->settings->create_key_value_enumerator(lib->settings, + "%s.plugins.dhcp-inform.%s", lib->ns, section); + + while (enumerator->enumerate(enumerator, &key, &value)) + { + traffic_selector_t *ts = parse_cidr(value); + if (ts) + { + routes->insert_last(routes, ts); + count++; + DBG2(DBG_CFG, "dhcp-inform: loaded static route %s from %s", + value, section); + } + else + { + DBG1(DBG_CFG, "dhcp-inform: failed to parse route '%s' in %s", + value, section); + } + } + enumerator->destroy(enumerator); + + return routes; +} + +/** + * Load pool configurations + */ +static void load_pools(private_dhcp_inform_static_provider_t *this) +{ + enumerator_t *pool_enum; + char *pool_name; + + pool_enum = lib->settings->create_section_enumerator(lib->settings, + "%s.plugins.dhcp-inform.pools", lib->ns); + + while (pool_enum->enumerate(pool_enum, &pool_name)) + { + char *cidr; + char routes_section[256]; + pool_entry_t *entry; + host_t *network; + uint8_t prefix; + + cidr = lib->settings->get_str(lib->settings, + "%s.plugins.dhcp-inform.pools.%s.cidr", NULL, lib->ns, pool_name); + + if (!cidr) + { + DBG1(DBG_CFG, "dhcp-inform: pool '%s' missing cidr, skipping", + pool_name); + continue; + } + + if (!parse_cidr_to_host(cidr, &network, &prefix)) + { + DBG1(DBG_CFG, "dhcp-inform: pool '%s' invalid cidr '%s', skipping", + pool_name, cidr); + continue; + } + + INIT(entry, + .name = strdup(pool_name), + .network = network, + .prefix = prefix, + ); + + snprintf(routes_section, sizeof(routes_section), + "pools.%s.routes", pool_name); + entry->routes = load_routes_from_section(routes_section); + + if (entry->routes->get_count(entry->routes) > 0) + { + this->pools->insert_last(this->pools, entry); + this->has_routes = TRUE; + DBG1(DBG_CFG, "dhcp-inform: loaded pool '%s' (%s) with %d routes", + pool_name, cidr, entry->routes->get_count(entry->routes)); + } + else + { + DBG1(DBG_CFG, "dhcp-inform: pool '%s' has no routes, skipping", + pool_name); + pool_entry_destroy(entry); + } + } + pool_enum->destroy(pool_enum); +} + +/** + * Clone routes from a list + */ +static linked_list_t *clone_routes(linked_list_t *source) +{ + linked_list_t *cloned; + enumerator_t *enumerator; + traffic_selector_t *ts; + + cloned = linked_list_create(); + enumerator = source->create_enumerator(source); + while (enumerator->enumerate(enumerator, &ts)) + { + cloned->insert_last(cloned, ts->clone(ts)); + } + enumerator->destroy(enumerator); + + return cloned; +} + +METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, + private_dhcp_inform_static_provider_t *this, const char *client_ip) +{ + enumerator_t *enumerator; + pool_entry_t *pool; + host_t *client; + + if (!client_ip) + { + /* No client IP - return global routes */ + if (this->global_routes->get_count(this->global_routes) > 0) + { + DBG2(DBG_CFG, "dhcp-inform: returning global routes (no client IP)"); + return clone_routes(this->global_routes); + } + return linked_list_create(); + } + + client = host_create_from_string(client_ip, 0); + if (!client) + { + DBG1(DBG_CFG, "dhcp-inform: invalid client IP: %s", client_ip); + return linked_list_create(); + } + + /* Check pool-specific routes first */ + enumerator = this->pools->create_enumerator(this->pools); + while (enumerator->enumerate(enumerator, &pool)) + { + if (ip_in_subnet(client, pool->network, pool->prefix)) + { + enumerator->destroy(enumerator); + client->destroy(client); + + DBG1(DBG_CFG, "dhcp-inform: client %s matched pool '%s', " + "returning %d pool-specific routes", + client_ip, pool->name, pool->routes->get_count(pool->routes)); + + return clone_routes(pool->routes); + } + } + enumerator->destroy(enumerator); + client->destroy(client); + + /* Fall back to global routes */ + if (this->global_routes->get_count(this->global_routes) > 0) + { + DBG1(DBG_CFG, "dhcp-inform: client %s using %d global routes", + client_ip, this->global_routes->get_count(this->global_routes)); + return clone_routes(this->global_routes); + } + + DBG1(DBG_CFG, "dhcp-inform: no routes configured for client %s", client_ip); + return linked_list_create(); +} + +METHOD(dhcp_inform_provider_t, get_name, const char*, + private_dhcp_inform_static_provider_t *this) +{ + return "static"; +} + +METHOD(dhcp_inform_provider_t, is_available, bool, + private_dhcp_inform_static_provider_t *this) +{ + return this->has_routes; +} + +METHOD(dhcp_inform_provider_t, destroy, void, + private_dhcp_inform_static_provider_t *this) +{ + if (this->global_routes) + { + this->global_routes->destroy_offset(this->global_routes, + offsetof(traffic_selector_t, destroy)); + } + if (this->pools) + { + this->pools->destroy_function(this->pools, (void*)pool_entry_destroy); + } + free(this); +} + +/** + * See header + */ +dhcp_inform_static_provider_t *dhcp_inform_static_provider_create() +{ + private_dhcp_inform_static_provider_t *this; + + INIT(this, + .public = { + .provider = { + .get_routes = _get_routes, + .get_name = _get_name, + .is_available = _is_available, + .destroy = _destroy, + }, + }, + .pools = linked_list_create(), + .has_routes = FALSE, + ); + + /* Load global routes */ + this->global_routes = load_routes_from_section("routes"); + if (this->global_routes->get_count(this->global_routes) > 0) + { + this->has_routes = TRUE; + DBG1(DBG_CFG, "dhcp-inform: loaded %d global static routes", + this->global_routes->get_count(this->global_routes)); + } + + /* Load per-pool routes */ + load_pools(this); + + if (!this->has_routes) + { + DBG2(DBG_CFG, "dhcp-inform: no static routes configured"); + } + + return &this->public; +} diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.h b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.h new file mode 100644 index 0000000000..f5984305a8 --- /dev/null +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.h @@ -0,0 +1,49 @@ +/* + * Copyright (C) 2025 Structured World Foundation + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +/** + * @defgroup dhcp_inform_static_provider dhcp_inform_static_provider + * @{ @ingroup dhcp_inform + */ + +#ifndef DHCP_INFORM_STATIC_PROVIDER_H_ +#define DHCP_INFORM_STATIC_PROVIDER_H_ + +#include "dhcp_inform_provider.h" + +typedef struct dhcp_inform_static_provider_t dhcp_inform_static_provider_t; + +/** + * Static route provider - reads routes from strongswan.conf. + * + * Supports: + * - Global routes (apply to all clients) + * - Per-pool routes (override global for clients in specific CIDR) + * + * Configuration example: + * charon.plugins.dhcp-inform.routes.route1 = 10.0.0.0/8 + * charon.plugins.dhcp-inform.pools.prod.cidr = 10.100.0.0/16 + * charon.plugins.dhcp-inform.pools.prod.routes.r1 = 192.168.1.0/24 + */ +struct dhcp_inform_static_provider_t { + + /** + * Implements dhcp_inform_provider_t interface + */ + dhcp_inform_provider_t provider; +}; + +/** + * Create static route provider. + * + * @return provider instance, NULL on failure + */ +dhcp_inform_static_provider_t *dhcp_inform_static_provider_create(); + +#endif /** DHCP_INFORM_STATIC_PROVIDER_H_ @}*/ diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c new file mode 100644 index 0000000000..5f5e125203 --- /dev/null +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c @@ -0,0 +1,242 @@ +/* + * Copyright (C) 2025 Structured World Foundation + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#include "dhcp_inform_ts_provider.h" + +#include +#include +#include +#include +#include +#include + +typedef struct private_dhcp_inform_ts_provider_t private_dhcp_inform_ts_provider_t; + +/** + * Private data + */ +struct private_dhcp_inform_ts_provider_t { + + /** + * Public interface + */ + dhcp_inform_ts_provider_t public; + + /** + * Whether TS routes are enabled + */ + bool enabled; +}; + +/** + * Check if a traffic selector is a valid route (not 0.0.0.0/0) + */ +static bool is_valid_route_ts(traffic_selector_t *ts) +{ + uint8_t mask; + + if (ts->get_type(ts) != TS_IPV4_ADDR_RANGE) + { + /* Only handle IPv4 for DHCP option 121/249 */ + return FALSE; + } + + mask = ts->get_netmask(ts); + + /* Skip default route (0.0.0.0/0) - we don't want to push that */ + if (mask == 0) + { + return FALSE; + } + + return TRUE; +} + +/** + * Check if traffic selector already exists in list (deduplication) + */ +static bool ts_exists_in_list(linked_list_t *list, traffic_selector_t *ts) +{ + enumerator_t *enumerator; + traffic_selector_t *existing; + bool found = FALSE; + + enumerator = list->create_enumerator(list); + while (enumerator->enumerate(enumerator, &existing)) + { + if (ts->equals(ts, existing)) + { + found = TRUE; + break; + } + } + enumerator->destroy(enumerator); + + return found; +} + +/** + * Find IKE SA by virtual IP and extract traffic selectors + */ +static linked_list_t *extract_ts_from_ike_sa(const char *client_ip) +{ + linked_list_t *routes; + enumerator_t *ike_enum, *child_enum, *ts_enum; + ike_sa_t *ike_sa; + child_sa_t *child_sa; + traffic_selector_t *ts; + host_t *client_vip; + bool found_sa = FALSE; + int route_count = 0; + + routes = linked_list_create(); + + if (!client_ip) + { + DBG1(DBG_CFG, "dhcp-inform-ts: no client IP provided"); + return routes; + } + + client_vip = host_create_from_string(client_ip, 0); + if (!client_vip) + { + DBG1(DBG_CFG, "dhcp-inform-ts: invalid client IP: %s", client_ip); + return routes; + } + + /* Enumerate all IKE SAs to find one matching this client */ + ike_enum = charon->ike_sa_manager->create_enumerator( + charon->ike_sa_manager, TRUE); + + while (ike_enum->enumerate(ike_enum, &ike_sa)) + { + enumerator_t *vip_enum; + host_t *vip; + bool match = FALSE; + + /* Check if any virtual IP of this IKE SA matches our client */ + vip_enum = ike_sa->create_virtual_ip_enumerator(ike_sa, FALSE); + while (vip_enum->enumerate(vip_enum, &vip)) + { + if (vip->ip_equals(vip, client_vip)) + { + match = TRUE; + break; + } + } + vip_enum->destroy(vip_enum); + + if (!match) + { + continue; + } + + found_sa = TRUE; + DBG1(DBG_CFG, "dhcp-inform-ts: found IKE SA for client %s", client_ip); + + /* Enumerate CHILD SAs and extract remote traffic selectors */ + child_enum = ike_sa->create_child_sa_enumerator(ike_sa); + while (child_enum->enumerate(child_enum, &child_sa)) + { + /* Get remote (server-side) traffic selectors - these are the + * networks the client should be able to reach */ + ts_enum = child_sa->create_ts_enumerator(child_sa, FALSE); + while (ts_enum->enumerate(ts_enum, &ts)) + { + if (is_valid_route_ts(ts) && !ts_exists_in_list(routes, ts)) + { + routes->insert_last(routes, ts->clone(ts)); + route_count++; + DBG2(DBG_CFG, "dhcp-inform-ts: extracted route %R", ts); + } + } + ts_enum->destroy(ts_enum); + } + child_enum->destroy(child_enum); + + /* Found the SA, no need to continue */ + break; + } + ike_enum->destroy(ike_enum); + client_vip->destroy(client_vip); + + if (!found_sa) + { + DBG1(DBG_CFG, "dhcp-inform-ts: no IKE SA found for client %s", + client_ip); + } + else + { + DBG1(DBG_CFG, "dhcp-inform-ts: extracted %d routes for client %s", + route_count, client_ip); + } + + return routes; +} + +METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, + private_dhcp_inform_ts_provider_t *this, const char *client_ip) +{ + if (!this->enabled) + { + return linked_list_create(); + } + + return extract_ts_from_ike_sa(client_ip); +} + +METHOD(dhcp_inform_provider_t, get_name, const char*, + private_dhcp_inform_ts_provider_t *this) +{ + return "traffic-selectors"; +} + +METHOD(dhcp_inform_provider_t, is_available, bool, + private_dhcp_inform_ts_provider_t *this) +{ + return this->enabled; +} + +METHOD(dhcp_inform_provider_t, destroy, void, + private_dhcp_inform_ts_provider_t *this) +{ + free(this); +} + +/** + * See header + */ +dhcp_inform_ts_provider_t *dhcp_inform_ts_provider_create() +{ + private_dhcp_inform_ts_provider_t *this; + + INIT(this, + .public = { + .provider = { + .get_routes = _get_routes, + .get_name = _get_name, + .is_available = _is_available, + .destroy = _destroy, + }, + }, + .enabled = lib->settings->get_bool(lib->settings, + "%s.plugins.dhcp-inform.use_ts_routes", FALSE, lib->ns), + ); + + if (this->enabled) + { + DBG1(DBG_CFG, "dhcp-inform: TS route provider enabled (EXCLUSIVE mode)"); + } + else + { + DBG2(DBG_CFG, "dhcp-inform: TS route provider disabled"); + } + + return &this->public; +} diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.h b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.h new file mode 100644 index 0000000000..bcc5cede43 --- /dev/null +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.h @@ -0,0 +1,49 @@ +/* + * Copyright (C) 2025 Structured World Foundation + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +/** + * @defgroup dhcp_inform_ts_provider dhcp_inform_ts_provider + * @{ @ingroup dhcp_inform + */ + +#ifndef DHCP_INFORM_TS_PROVIDER_H_ +#define DHCP_INFORM_TS_PROVIDER_H_ + +#include "dhcp_inform_provider.h" + +typedef struct dhcp_inform_ts_provider_t dhcp_inform_ts_provider_t; + +/** + * Traffic Selector route provider - extracts routes from IKE SA. + * + * This provider is EXCLUSIVE - when enabled, it's the only source of routes. + * Designed for Windows 7 compatibility where clients don't properly handle + * traffic selectors pushed via IKE. + * + * Behavior: + * - Finds IKE SA by matching client virtual IP + * - Extracts remote traffic selectors from all CHILD_SAs + * - Converts traffic selectors to routes for DHCP Option 121/249 + */ +struct dhcp_inform_ts_provider_t { + + /** + * Implements dhcp_inform_provider_t interface + */ + dhcp_inform_provider_t provider; +}; + +/** + * Create TS route provider. + * + * @return provider instance, NULL on failure + */ +dhcp_inform_ts_provider_t *dhcp_inform_ts_provider_create(); + +#endif /** DHCP_INFORM_TS_PROVIDER_H_ @}*/ From 1db351cafd0901ca67220a508afd93a232b58221 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 17 Jan 2026 15:30:13 +0200 Subject: [PATCH 2/9] fix(dhcp-inform): correct compilation errors in provider files - Use ts->to_subnet() instead of non-existent ts->get_netmask() - Cast const char* to char* for host_create_from_string() calls - Remove unused parse_cidr() function from responder --- .../dhcp_inform/dhcp_inform_responder.c | 61 ------------------- .../dhcp_inform/dhcp_inform_static_provider.c | 2 +- .../dhcp_inform/dhcp_inform_ts_provider.c | 11 +++- 3 files changed, 10 insertions(+), 64 deletions(-) diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c index 166fde32a7..2178373345 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c @@ -157,67 +157,6 @@ struct private_dhcp_inform_responder_t { uint32_t dns_server; }; -/** - * Parse CIDR notation with validation - */ -static traffic_selector_t *parse_cidr(const char *cidr) -{ - char *slash, *ip_str; - int prefix = 32; - host_t *host; - traffic_selector_t *ts = NULL; - - if (!cidr || !*cidr) - { - DBG1(DBG_NET, "dhcp-inform: CORRUPTED DATA - empty CIDR"); - return NULL; - } - - if (strlen(cidr) > 43) /* max: xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx/128 */ - { - DBG1(DBG_NET, "dhcp-inform: CORRUPTED DATA - CIDR too long: %.20s...", cidr); - return NULL; - } - - ip_str = strdup(cidr); - if (!ip_str) - { - DBG1(DBG_NET, "dhcp-inform: memory allocation failed"); - return NULL; - } - - slash = strchr(ip_str, '/'); - if (slash) - { - *slash = '\0'; - prefix = atoi(slash + 1); - if (prefix < 0 || prefix > 32) - { - DBG1(DBG_NET, "dhcp-inform: CORRUPTED DATA - invalid prefix %d in %s", prefix, cidr); - free(ip_str); - return NULL; - } - } - - host = host_create_from_string(ip_str, 0); - if (!host) - { - DBG1(DBG_NET, "dhcp-inform: CORRUPTED DATA - invalid IP in CIDR: %s", ip_str); - free(ip_str); - return NULL; - } - - ts = traffic_selector_create_from_subnet(host, prefix, 0, 0, 65535); - host->destroy(host); - if (!ts) - { - DBG1(DBG_NET, "dhcp-inform: failed to create traffic selector for %s", cidr); - } - - free(ip_str); - return ts; -} - /** * Check if traffic selector already exists in list (deduplication) */ diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c index c129486827..17c32cc912 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c @@ -360,7 +360,7 @@ METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, return linked_list_create(); } - client = host_create_from_string(client_ip, 0); + client = host_create_from_string((char*)client_ip, 0); if (!client) { DBG1(DBG_CFG, "dhcp-inform: invalid client IP: %s", client_ip); diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c index 5f5e125203..934b5426fd 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c @@ -39,6 +39,7 @@ struct private_dhcp_inform_ts_provider_t { */ static bool is_valid_route_ts(traffic_selector_t *ts) { + host_t *net; uint8_t mask; if (ts->get_type(ts) != TS_IPV4_ADDR_RANGE) @@ -47,7 +48,13 @@ static bool is_valid_route_ts(traffic_selector_t *ts) return FALSE; } - mask = ts->get_netmask(ts); + /* Convert to subnet to get the mask */ + if (!ts->to_subnet(ts, &net, &mask)) + { + /* Not a valid subnet */ + return FALSE; + } + net->destroy(net); /* Skip default route (0.0.0.0/0) - we don't want to push that */ if (mask == 0) @@ -103,7 +110,7 @@ static linked_list_t *extract_ts_from_ike_sa(const char *client_ip) return routes; } - client_vip = host_create_from_string(client_ip, 0); + client_vip = host_create_from_string((char*)client_ip, 0); if (!client_vip) { DBG1(DBG_CFG, "dhcp-inform-ts: invalid client IP: %s", client_ip); From 5be9fc0a786ff3d587ad44900b184fd0d4b93387 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 17 Jan 2026 15:43:12 +0200 Subject: [PATCH 3/9] fix(dhcp-inform): address Copilot review feedback - Add NULL checks after all linked_list_create() calls - Replace atoi() with strtol() for safer CIDR prefix parsing - Add MAX_CIDR_LEN constant (43) with documentation - Use portable SQL for database provider (works with pgsql/mysql/sqlite) - Do IP-in-CIDR filtering in C code for database portability - Add comments explaining intentional code patterns: - Code duplication across providers (self-contained compilation) - Memory ownership transfer in route deduplication - Mutual exclusivity handled by priority order - Add buffer truncation check for pool names - Update documentation to reflect multi-database support --- .../plugins/dhcp_inform/dhcp-inform.conf | 10 +- .../dhcp_inform/dhcp_inform_db_provider.c | 207 ++++++++++++------ .../dhcp_inform/dhcp_inform_db_provider.h | 11 +- .../dhcp_inform/dhcp_inform_responder.c | 11 +- .../dhcp_inform/dhcp_inform_static_provider.c | 49 +++-- .../dhcp_inform/dhcp_inform_ts_provider.c | 7 +- 6 files changed, 197 insertions(+), 98 deletions(-) diff --git a/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf b/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf index 10c09de4b8..7019a85c61 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf +++ b/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf @@ -35,9 +35,15 @@ dhcp-inform { # ========================================================================= # Routes are queried from database by matching client IP to pool CIDR. # When database is configured, static routes are IGNORED. - # Requires v_pool_routes view: (pool_cidr, route) + # Works with PostgreSQL, MySQL, SQLite via strongSwan database abstraction. # - # database = postgresql://vpn_user:password@localhost/vpn_db + # Requires VIEW v_pool_routes (pool_cidr TEXT, route TEXT): + # - pool_cidr: CIDR notation (e.g., "10.0.0.0/8") + # - route: route to push to clients in this pool + # + # database = pgsql://vpn_user:password@localhost/vpn_db + # database = mysql://vpn_user:password@localhost/vpn_db + # database = sqlite:///etc/strongswan/routes.db # ========================================================================= # MODE 3: Static Routes (EXCLUSIVE - only when no database) diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c index 3d4e11a875..e15fe7b66a 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c @@ -33,74 +33,141 @@ struct private_dhcp_inform_db_provider_t { database_t *db; }; +/* Maximum CIDR string length: "xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx/128" = 43 chars */ +#define MAX_CIDR_LEN 43 + /** - * Parse CIDR notation to traffic_selector + * Parse CIDR to host and prefix. */ -static traffic_selector_t *parse_cidr(const char *cidr) +static bool parse_cidr_to_host(const char *cidr, host_t **host, uint8_t *prefix) { - char *slash, *ip_str; - int prefix = 32; - host_t *host; - traffic_selector_t *ts = NULL; + char *slash, *ip_str, *endptr; + long pfx = 32; - if (!cidr || !*cidr) + if (!cidr || !*cidr || strlen(cidr) > MAX_CIDR_LEN) { - DBG1(DBG_CFG, "dhcp-inform-db: CORRUPTED DATA - empty CIDR"); - return NULL; - } - - if (strlen(cidr) > 43) - { - DBG1(DBG_CFG, "dhcp-inform-db: CORRUPTED DATA - CIDR too long: %.20s...", - cidr); - return NULL; + return FALSE; } ip_str = strdup(cidr); if (!ip_str) { - return NULL; + return FALSE; } slash = strchr(ip_str, '/'); if (slash) { *slash = '\0'; - prefix = atoi(slash + 1); - if (prefix < 0 || prefix > 32) + pfx = strtol(slash + 1, &endptr, 10); + if (*endptr != '\0' || pfx < 0 || pfx > 32) { - DBG1(DBG_CFG, "dhcp-inform-db: invalid prefix %d in %s", - prefix, cidr); free(ip_str); - return NULL; + return FALSE; } } - host = host_create_from_string(ip_str, 0); - if (!host) + *host = host_create_from_string(ip_str, 0); + free(ip_str); + + if (!*host) + { + return FALSE; + } + + *prefix = pfx; + return TRUE; +} + +/** + * Parse CIDR notation to traffic_selector. + * Note: This function is intentionally duplicated in each provider file to keep + * providers self-contained and independently compilable without shared utilities. + */ +static traffic_selector_t *parse_cidr(const char *cidr) +{ + host_t *host; + uint8_t prefix; + traffic_selector_t *ts; + + if (!parse_cidr_to_host(cidr, &host, &prefix)) { - DBG1(DBG_CFG, "dhcp-inform-db: invalid IP in CIDR: %s", ip_str); - free(ip_str); + DBG1(DBG_CFG, "dhcp-inform-db: failed to parse CIDR: %s", cidr); return NULL; } ts = traffic_selector_create_from_subnet(host, prefix, 0, 0, 65535); host->destroy(host); - free(ip_str); return ts; } +/** + * Check if an IP address falls within a network/prefix. + * Note: Duplicated from static_provider for self-contained compilation. + */ +static bool ip_in_subnet(host_t *ip, host_t *network, uint8_t prefix) +{ + chunk_t ip_addr, net_addr; + uint8_t *ip_ptr, *net_ptr; + int bytes, bits, i; + uint8_t mask; + + if (ip->get_family(ip) != network->get_family(network)) + { + return FALSE; + } + + ip_addr = ip->get_address(ip); + net_addr = network->get_address(network); + + if (ip_addr.len != net_addr.len) + { + return FALSE; + } + + bytes = prefix / 8; + bits = prefix % 8; + + ip_ptr = ip_addr.ptr; + net_ptr = net_addr.ptr; + + /* Compare full bytes. Cast is safe: ip_addr.len is always <= 16 (IPv6) */ + for (i = 0; i < bytes && i < (int)ip_addr.len; i++) + { + if (ip_ptr[i] != net_ptr[i]) + { + return FALSE; + } + } + + /* Compare remaining bits */ + if (bits > 0 && bytes < (int)ip_addr.len) + { + mask = 0xFF << (8 - bits); + if ((ip_ptr[bytes] & mask) != (net_ptr[bytes] & mask)) + { + return FALSE; + } + } + + return TRUE; +} + METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, private_dhcp_inform_db_provider_t *this, const char *client_ip) { linked_list_t *routes; enumerator_t *enumerator; - char *route_value; - int routes_parsed = 0; - int routes_failed = 0; + char *pool_cidr, *route_value; + host_t *client; + int routes_added = 0; routes = linked_list_create(); + if (!routes) + { + return NULL; + } if (!this->db) { @@ -109,77 +176,73 @@ METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, if (!client_ip || !*client_ip) { - DBG1(DBG_CFG, "dhcp-inform-db: CORRUPTED DATA - empty client IP"); + DBG1(DBG_CFG, "dhcp-inform-db: empty client IP"); + return routes; + } + + client = host_create_from_string((char*)client_ip, 0); + if (!client) + { + DBG1(DBG_CFG, "dhcp-inform-db: invalid client IP: %s", client_ip); return routes; } DBG2(DBG_CFG, "dhcp-inform-db: looking up routes for IP %s", client_ip); - /* Query routes for the pool that contains this IP - * Uses v_pool_routes VIEW: (pool_cidr, route) + /* Query all pool/route pairs, filter in C for database portability. + * Uses v_pool_routes VIEW: (pool_cidr, route). + * Works with PostgreSQL, MySQL, SQLite via strongSwan database abstraction. */ enumerator = this->db->query(this->db, - "SELECT route FROM v_pool_routes WHERE ?::inet << pool_cidr::inet", - DB_TEXT, client_ip, - DB_TEXT); - - if (!enumerator) - { - DBG2(DBG_CFG, "dhcp-inform-db: primary query failed, trying fallback"); - /* Fallback: get routes from auto_ip_pools directly */ - enumerator = this->db->query(this->db, - "SELECT unnest(aip.routes) as route " - "FROM auto_ip_pools aip " - "JOIN environments e ON e.auto_ip_pool_id = aip.id " - "WHERE e.is_active = true " - "AND ?::inet << aip.cidr::inet", - DB_TEXT, client_ip, - DB_TEXT); - } + "SELECT pool_cidr, route FROM v_pool_routes", + DB_TEXT, DB_TEXT); if (!enumerator) { - DBG1(DBG_CFG, "dhcp-inform-db: all queries failed for %s", client_ip); + DBG1(DBG_CFG, "dhcp-inform-db: query failed"); + client->destroy(client); return routes; } - while (enumerator->enumerate(enumerator, &route_value)) + while (enumerator->enumerate(enumerator, &pool_cidr, &route_value)) { + host_t *pool_net; + uint8_t pool_prefix; traffic_selector_t *ts; - routes_parsed++; + if (!pool_cidr || !route_value) + { + continue; + } + + /* Parse pool CIDR and check if client IP is in this pool */ + if (!parse_cidr_to_host(pool_cidr, &pool_net, &pool_prefix)) + { + DBG2(DBG_CFG, "dhcp-inform-db: invalid pool CIDR: %s", pool_cidr); + continue; + } - if (!route_value) + if (!ip_in_subnet(client, pool_net, pool_prefix)) { - DBG1(DBG_CFG, "dhcp-inform-db: CORRUPTED DATA - NULL route at row %d", - routes_parsed); - routes_failed++; + pool_net->destroy(pool_net); continue; } + pool_net->destroy(pool_net); + /* Client is in this pool - add the route */ ts = parse_cidr(route_value); if (ts) { routes->insert_last(routes, ts); - DBG2(DBG_CFG, "dhcp-inform-db: added route %s", route_value); - } - else - { - DBG1(DBG_CFG, "dhcp-inform-db: failed to parse route: %s", - route_value); - routes_failed++; + routes_added++; + DBG2(DBG_CFG, "dhcp-inform-db: added route %s from pool %s", + route_value, pool_cidr); } } enumerator->destroy(enumerator); + client->destroy(client); - if (routes_failed > 0) - { - DBG1(DBG_CFG, "dhcp-inform-db: WARNING - %d/%d routes had corrupted data", - routes_failed, routes_parsed); - } - - DBG1(DBG_CFG, "dhcp-inform-db: found %d valid routes for %s", - routes->get_count(routes), client_ip); + DBG1(DBG_CFG, "dhcp-inform-db: found %d routes for %s", routes_added, client_ip); return routes; } diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.h b/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.h index e589a8f04f..5ed3423403 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.h +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.h @@ -22,15 +22,18 @@ typedef struct dhcp_inform_db_provider_t dhcp_inform_db_provider_t; /** * Database route provider - reads routes from SQL database. * - * Supports PostgreSQL, MySQL, SQLite via strongSwan database abstraction. - * Queries routes from v_pool_routes view based on client virtual IP. + * Works with PostgreSQL, MySQL, SQLite via strongSwan database abstraction. + * Uses portable SQL; IP-in-CIDR filtering done in C for compatibility. * * Configuration: * charon.plugins.dhcp-inform.database = pgsql://user:pass@host/db + * charon.plugins.dhcp-inform.database = mysql://user:pass@host/db + * charon.plugins.dhcp-inform.database = sqlite:///path/to/db.sqlite * * Required database schema: - * VIEW v_pool_routes (pool_cidr, route) - * - Returns routes for pools, client IP matched against pool_cidr + * VIEW v_pool_routes (pool_cidr TEXT, route TEXT) + * - pool_cidr: CIDR notation (e.g., "10.0.0.0/8") + * - route: route to push to clients in this pool (CIDR notation) */ struct dhcp_inform_db_provider_t { diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c index 2178373345..48916a6b60 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c @@ -204,23 +204,25 @@ static void add_routes_from_provider(dhcp_inform_provider_t *provider, return; } + /* Transfer ownership: routes go to target list or get destroyed if duplicate. + * After this loop, provider_routes list is empty of responsibility. */ enumerator = provider_routes->create_enumerator(provider_routes); while (enumerator->enumerate(enumerator, &ts)) { if (route_exists_in_list(routes, ts)) { duplicates++; - ts->destroy(ts); + ts->destroy(ts); /* Duplicate - destroy immediately */ } else { - routes->insert_last(routes, ts); + routes->insert_last(routes, ts); /* Transfer ownership to target */ added++; } } enumerator->destroy(enumerator); - /* Just destroy the list, not the routes (we moved ownership) */ + /* Destroy list structure only - elements already handled above */ provider_routes->destroy(provider_routes); if (added > 0 || duplicates > 0) @@ -238,7 +240,8 @@ static void add_routes_from_provider(dhcp_inform_provider_t *provider, * 2. DB routes - when database configured, ONLY database routes * 3. Static routes - when no DB, use config routes with per-pool overrides * - * Modes are exclusive - they never combine. + * Modes are exclusive - if multiple are configured, highest priority wins. + * This is intentional: config errors don't crash, just use first available mode. */ static linked_list_t *get_routes_for_client(private_dhcp_inform_responder_t *this, const char *client_ip) diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c index 17c32cc912..bdca901df2 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c @@ -74,13 +74,18 @@ static void pool_entry_destroy(pool_entry_t *entry) } } +/* Maximum CIDR string length: "xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx/128" = 43 chars */ +#define MAX_CIDR_LEN 43 + /** - * Parse CIDR notation to traffic_selector + * Parse CIDR notation to traffic_selector. + * Note: This function is intentionally duplicated in each provider file to keep + * providers self-contained and independently compilable without shared utilities. */ static traffic_selector_t *parse_cidr(const char *cidr) { - char *slash, *ip_str; - int prefix = 32; + char *slash, *ip_str, *endptr; + long prefix = 32; host_t *host; traffic_selector_t *ts = NULL; @@ -89,7 +94,7 @@ static traffic_selector_t *parse_cidr(const char *cidr) return NULL; } - if (strlen(cidr) > 43) + if (strlen(cidr) > MAX_CIDR_LEN) { DBG1(DBG_CFG, "dhcp-inform: CIDR too long: %.20s...", cidr); return NULL; @@ -105,10 +110,10 @@ static traffic_selector_t *parse_cidr(const char *cidr) if (slash) { *slash = '\0'; - prefix = atoi(slash + 1); - if (prefix < 0 || prefix > 32) + prefix = strtol(slash + 1, &endptr, 10); + if (*endptr != '\0' || prefix < 0 || prefix > 32) { - DBG1(DBG_CFG, "dhcp-inform: invalid prefix %d in %s", prefix, cidr); + DBG1(DBG_CFG, "dhcp-inform: invalid prefix in %s", cidr); free(ip_str); return NULL; } @@ -134,8 +139,8 @@ static traffic_selector_t *parse_cidr(const char *cidr) */ static bool parse_cidr_to_host(const char *cidr, host_t **host, uint8_t *prefix) { - char *slash, *ip_str; - int pfx = 32; + char *slash, *ip_str, *endptr; + long pfx = 32; if (!cidr || !*cidr) { @@ -152,8 +157,8 @@ static bool parse_cidr_to_host(const char *cidr, host_t **host, uint8_t *prefix) if (slash) { *slash = '\0'; - pfx = atoi(slash + 1); - if (pfx < 0 || pfx > 32) + pfx = strtol(slash + 1, &endptr, 10); + if (*endptr != '\0' || pfx < 0 || pfx > 32) { free(ip_str); return FALSE; @@ -201,7 +206,7 @@ static bool ip_in_subnet(host_t *ip, host_t *network, uint8_t prefix) ip_ptr = ip_addr.ptr; net_ptr = net_addr.ptr; - /* Compare full bytes */ + /* Compare full bytes. Cast is safe: ip_addr.len is always <= 16 (IPv6) */ for (i = 0; i < bytes && i < (int)ip_addr.len; i++) { if (ip_ptr[i] != net_ptr[i]) @@ -210,7 +215,7 @@ static bool ip_in_subnet(host_t *ip, host_t *network, uint8_t prefix) } } - /* Compare remaining bits */ + /* Compare remaining bits. Cast is safe: ip_addr.len is always <= 16 (IPv6) */ if (bits > 0 && bytes < (int)ip_addr.len) { mask = 0xFF << (8 - bits); @@ -234,6 +239,10 @@ static linked_list_t *load_routes_from_section(const char *section) int count = 0; routes = linked_list_create(); + if (!routes) + { + return NULL; + } enumerator = lib->settings->create_key_value_enumerator(lib->settings, "%s.plugins.dhcp-inform.%s", lib->ns, section); @@ -301,8 +310,14 @@ static void load_pools(private_dhcp_inform_static_provider_t *this) .prefix = prefix, ); - snprintf(routes_section, sizeof(routes_section), - "pools.%s.routes", pool_name); + if (snprintf(routes_section, sizeof(routes_section), + "pools.%s.routes", pool_name) >= (int)sizeof(routes_section)) + { + DBG1(DBG_CFG, "dhcp-inform: pool name '%s' too long, skipping", + pool_name); + pool_entry_destroy(entry); + continue; + } entry->routes = load_routes_from_section(routes_section); if (entry->routes->get_count(entry->routes) > 0) @@ -332,6 +347,10 @@ static linked_list_t *clone_routes(linked_list_t *source) traffic_selector_t *ts; cloned = linked_list_create(); + if (!cloned) + { + return NULL; + } enumerator = source->create_enumerator(source); while (enumerator->enumerate(enumerator, &ts)) { diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c index 934b5426fd..6fb91dc14d 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c @@ -66,7 +66,8 @@ static bool is_valid_route_ts(traffic_selector_t *ts) } /** - * Check if traffic selector already exists in list (deduplication) + * Check if traffic selector already exists in list (deduplication). + * Note: Duplicated for self-contained provider compilation. */ static bool ts_exists_in_list(linked_list_t *list, traffic_selector_t *ts) { @@ -103,6 +104,10 @@ static linked_list_t *extract_ts_from_ike_sa(const char *client_ip) int route_count = 0; routes = linked_list_create(); + if (!routes) + { + return NULL; + } if (!client_ip) { From a411159563b2751a82e8affd74e9f1d103bce46f Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 17 Jan 2026 15:54:49 +0200 Subject: [PATCH 4/9] fix: address Copilot second review round feedback - Update RPM spec comment to clarify multi-database support (PostgreSQL/MySQL/SQLite via strongSwan SQL plugins) - Fix snprintf negative return check in static_provider (len < 0 indicates encoding error per POSIX) - Clarify IPv4-only limitation in MAX_CIDR_LEN comments (DHCP option 121/249 classless static routes is IPv4-only) - Fix misleading comment in responder: providers are mutually exclusive (TS OR DB OR static), not combined --- packaging/rpm/strongswan-sw.spec | 2 +- .../plugins/dhcp_inform/dhcp_inform_db_provider.c | 4 +++- .../plugins/dhcp_inform/dhcp_inform_responder.c | 2 +- .../plugins/dhcp_inform/dhcp_inform_static_provider.c | 9 ++++++--- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/packaging/rpm/strongswan-sw.spec b/packaging/rpm/strongswan-sw.spec index aaf4697cb4..2a68c800a5 100644 --- a/packaging/rpm/strongswan-sw.spec +++ b/packaging/rpm/strongswan-sw.spec @@ -77,7 +77,7 @@ Enables storing VPN user credentials and configuration in PostgreSQL. %package -n strongswan-dhcp-inform Summary: DHCP INFORM responder plugin for strongSwan Requires: strongswan-sw = %{version}-%{release} -# Database is optional - plugin can work with static routes or TS routes +# Database is optional - supports PostgreSQL/MySQL/SQLite via strongSwan SQL plugins Recommends: strongswan-pgsql = %{version}-%{release} %description -n strongswan-dhcp-inform diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c index e15fe7b66a..babb79a395 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c @@ -33,7 +33,9 @@ struct private_dhcp_inform_db_provider_t { database_t *db; }; -/* Maximum CIDR string length: "xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx/128" = 43 chars */ +/* Maximum CIDR string length for IPv4: "255.255.255.255/32" = 18 chars. + * Note: DHCP option 121/249 (classless static routes) is IPv4-only. + * Using 43 to be safe with any reasonable input. */ #define MAX_CIDR_LEN 43 /** diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c index 48916a6b60..2562fb42d3 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c @@ -722,7 +722,7 @@ static void process_dhcp_packet(private_dhcp_inform_responder_t *this, inet_ntop(AF_INET, &dhcp->ciaddr, client_ip_str, sizeof(client_ip_str)); DBG1(DBG_NET, "dhcp-inform: received DHCPINFORM from %s", client_ip_str); - /* Get routes from providers (TS exclusive, or DB + static combined) */ + /* Get routes from providers (mutually exclusive: TS OR DB OR static) */ routes = get_routes_for_client(this, client_ip_str); if (!routes) diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c index bdca901df2..e26bd00fb0 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c @@ -74,7 +74,9 @@ static void pool_entry_destroy(pool_entry_t *entry) } } -/* Maximum CIDR string length: "xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx/128" = 43 chars */ +/* Maximum CIDR string length for IPv4: "255.255.255.255/32" = 18 chars. + * Note: DHCP option 121/249 (classless static routes) is IPv4-only. + * Using 43 to be safe with any reasonable input. */ #define MAX_CIDR_LEN 43 /** @@ -310,8 +312,9 @@ static void load_pools(private_dhcp_inform_static_provider_t *this) .prefix = prefix, ); - if (snprintf(routes_section, sizeof(routes_section), - "pools.%s.routes", pool_name) >= (int)sizeof(routes_section)) + int len = snprintf(routes_section, sizeof(routes_section), + "pools.%s.routes", pool_name); + if (len < 0 || len >= (int)sizeof(routes_section)) { DBG1(DBG_CFG, "dhcp-inform: pool name '%s' too long, skipping", pool_name); From 9f86e9e0e94b0ae88c1fe71c9189913a7c7a1a08 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 17 Jan 2026 15:57:15 +0200 Subject: [PATCH 5/9] fix: add clarifying comments for Copilot review threads - ts_provider: document that empty list return is intentional - dhcp-inform.conf: clarify priority selection behavior - static_provider: document prefix 0 handling and code duplication - db_provider: add duplication note and prefix 0 documentation --- src/libcharon/plugins/dhcp_inform/dhcp-inform.conf | 3 ++- .../plugins/dhcp_inform/dhcp_inform_db_provider.c | 2 ++ .../plugins/dhcp_inform/dhcp_inform_static_provider.c | 8 +++++++- .../plugins/dhcp_inform/dhcp_inform_ts_provider.c | 1 + 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf b/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf index 7019a85c61..ffd1eb8a97 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf +++ b/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf @@ -6,7 +6,8 @@ # 2. Database - when database URI configured # 3. Static configuration - when no database # -# Modes are EXCLUSIVE - they never combine. Configure only ONE mode. +# Modes are EXCLUSIVE - only the highest-priority configured mode is used. +# If multiple modes are configured, lower-priority modes are silently ignored. dhcp-inform { # Enable the plugin diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c index babb79a395..c7f1859e53 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_db_provider.c @@ -40,6 +40,8 @@ struct private_dhcp_inform_db_provider_t { /** * Parse CIDR to host and prefix. + * Prefix 0 (match-all) is allowed for admin flexibility. + * Note: Duplicated for self-contained compilation (see static_provider). */ static bool parse_cidr_to_host(const char *cidr, host_t **host, uint8_t *prefix) { diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c index e26bd00fb0..e9af80c3fd 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c @@ -83,6 +83,10 @@ static void pool_entry_destroy(pool_entry_t *entry) * Parse CIDR notation to traffic_selector. * Note: This function is intentionally duplicated in each provider file to keep * providers self-contained and independently compilable without shared utilities. + * + * Prefix 0 (/0, default route) is allowed here for admin flexibility. + * The TS provider filters out /0 routes since we don't want to push + * default routes extracted from traffic selectors. */ static traffic_selector_t *parse_cidr(const char *cidr) { @@ -137,7 +141,9 @@ static traffic_selector_t *parse_cidr(const char *cidr) } /** - * Parse CIDR to host and prefix for pool matching + * Parse CIDR to host and prefix for pool matching. + * Prefix 0 (match-all) is allowed for admin flexibility. + * Note: Duplicated for self-contained compilation (see parse_cidr comment). */ static bool parse_cidr_to_host(const char *cidr, host_t **host, uint8_t *prefix) { diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c index 6fb91dc14d..867b603cc5 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c @@ -119,6 +119,7 @@ static linked_list_t *extract_ts_from_ike_sa(const char *client_ip) if (!client_vip) { DBG1(DBG_CFG, "dhcp-inform-ts: invalid client IP: %s", client_ip); + /* Return empty list - caller handles empty routes gracefully */ return routes; } From 954843046c6df624346839023f32ff4a5f314707 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 17 Jan 2026 16:11:59 +0200 Subject: [PATCH 6/9] fix: address third Copilot review round - Add @param this documentation to all provider.h function pointers - Add strdup NULL check in load_pools (memory allocation failure) - Add NULL check for entry->routes before calling get_count - Add NULL check for global_routes in get_routes and constructor --- .../dhcp_inform/dhcp_inform_provider.h | 5 ++++ .../dhcp_inform/dhcp_inform_static_provider.c | 25 ++++++++++++++++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_provider.h b/src/libcharon/plugins/dhcp_inform/dhcp_inform_provider.h index 8264a4e403..9d1915327a 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_provider.h +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_provider.h @@ -32,6 +32,7 @@ struct dhcp_inform_provider_t { /** * Get routes for a client by virtual IP. * + * @param this provider instance * @param client_ip client's virtual IP address string * @return linked_list_t of traffic_selector_t (caller destroys), * or NULL on error @@ -42,6 +43,7 @@ struct dhcp_inform_provider_t { /** * Get provider name for logging. * + * @param this provider instance * @return provider name string (static, do not free) */ const char* (*get_name)(dhcp_inform_provider_t *this); @@ -49,12 +51,15 @@ struct dhcp_inform_provider_t { /** * Check if provider is available/configured. * + * @param this provider instance * @return TRUE if provider can provide routes */ bool (*is_available)(dhcp_inform_provider_t *this); /** * Destroy provider instance. + * + * @param this provider instance */ void (*destroy)(dhcp_inform_provider_t *this); }; diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c index e9af80c3fd..26cf43714d 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c @@ -318,6 +318,14 @@ static void load_pools(private_dhcp_inform_static_provider_t *this) .prefix = prefix, ); + if (!entry->name) + { + DBG1(DBG_CFG, "dhcp-inform: failed to allocate name for pool '%s'", + pool_name); + pool_entry_destroy(entry); + continue; + } + int len = snprintf(routes_section, sizeof(routes_section), "pools.%s.routes", pool_name); if (len < 0 || len >= (int)sizeof(routes_section)) @@ -329,6 +337,14 @@ static void load_pools(private_dhcp_inform_static_provider_t *this) } entry->routes = load_routes_from_section(routes_section); + if (!entry->routes) + { + DBG1(DBG_CFG, "dhcp-inform: failed to load routes for pool '%s'", + pool_name); + pool_entry_destroy(entry); + continue; + } + if (entry->routes->get_count(entry->routes) > 0) { this->pools->insert_last(this->pools, entry); @@ -380,7 +396,8 @@ METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, if (!client_ip) { /* No client IP - return global routes */ - if (this->global_routes->get_count(this->global_routes) > 0) + if (this->global_routes && + this->global_routes->get_count(this->global_routes) > 0) { DBG2(DBG_CFG, "dhcp-inform: returning global routes (no client IP)"); return clone_routes(this->global_routes); @@ -415,7 +432,8 @@ METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, client->destroy(client); /* Fall back to global routes */ - if (this->global_routes->get_count(this->global_routes) > 0) + if (this->global_routes && + this->global_routes->get_count(this->global_routes) > 0) { DBG1(DBG_CFG, "dhcp-inform: client %s using %d global routes", client_ip, this->global_routes->get_count(this->global_routes)); @@ -475,7 +493,8 @@ dhcp_inform_static_provider_t *dhcp_inform_static_provider_create() /* Load global routes */ this->global_routes = load_routes_from_section("routes"); - if (this->global_routes->get_count(this->global_routes) > 0) + if (this->global_routes && + this->global_routes->get_count(this->global_routes) > 0) { this->has_routes = TRUE; DBG1(DBG_CFG, "dhcp-inform: loaded %d global static routes", From c16a5786ddaa8e3a78af040ebc70af25bd044a67 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 17 Jan 2026 16:30:31 +0200 Subject: [PATCH 7/9] fix: address fourth Copilot review round - Add NULL check for ts->clone() in ts_provider - Add NULL check for ts in responder enumerate loop - Add NULL check for pools linked_list_create in constructor - Fix clone_routes to handle ts->clone() failure gracefully --- .../plugins/dhcp_inform/dhcp_inform_responder.c | 5 +++++ .../dhcp_inform/dhcp_inform_static_provider.c | 17 +++++++++++++++-- .../dhcp_inform/dhcp_inform_ts_provider.c | 10 +++++++++- 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c index 2562fb42d3..2ad38df5bd 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c @@ -209,6 +209,11 @@ static void add_routes_from_provider(dhcp_inform_provider_t *provider, enumerator = provider_routes->create_enumerator(provider_routes); while (enumerator->enumerate(enumerator, &ts)) { + if (!ts) + { + /* Skip NULL entries that may result from failed clone operations */ + continue; + } if (route_exists_in_list(routes, ts)) { duplicates++; diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c index 26cf43714d..ad9dde9225 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c @@ -369,7 +369,7 @@ static linked_list_t *clone_routes(linked_list_t *source) { linked_list_t *cloned; enumerator_t *enumerator; - traffic_selector_t *ts; + traffic_selector_t *ts, *clone; cloned = linked_list_create(); if (!cloned) @@ -379,7 +379,13 @@ static linked_list_t *clone_routes(linked_list_t *source) enumerator = source->create_enumerator(source); while (enumerator->enumerate(enumerator, &ts)) { - cloned->insert_last(cloned, ts->clone(ts)); + clone = ts->clone(ts); + if (!clone) + { + DBG1(DBG_CFG, "dhcp-inform: failed to clone traffic selector"); + continue; + } + cloned->insert_last(cloned, clone); } enumerator->destroy(enumerator); @@ -491,6 +497,13 @@ dhcp_inform_static_provider_t *dhcp_inform_static_provider_create() .has_routes = FALSE, ); + if (!this->pools) + { + DBG1(DBG_CFG, "dhcp-inform: failed to create pools list"); + free(this); + return NULL; + } + /* Load global routes */ this->global_routes = load_routes_from_section("routes"); if (this->global_routes && diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c index 867b603cc5..cebe854f40 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c @@ -162,9 +162,17 @@ static linked_list_t *extract_ts_from_ike_sa(const char *client_ip) ts_enum = child_sa->create_ts_enumerator(child_sa, FALSE); while (ts_enum->enumerate(ts_enum, &ts)) { + traffic_selector_t *clone; + if (is_valid_route_ts(ts) && !ts_exists_in_list(routes, ts)) { - routes->insert_last(routes, ts->clone(ts)); + clone = ts->clone(ts); + if (!clone) + { + DBG1(DBG_CFG, "dhcp-inform-ts: failed to clone TS"); + continue; + } + routes->insert_last(routes, clone); route_count++; DBG2(DBG_CFG, "dhcp-inform-ts: extracted route %R", ts); } From 4396584fb93decd697f848bc90f7eb658500bf12 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 17 Jan 2026 17:02:07 +0200 Subject: [PATCH 8/9] fix: handle clone_routes NULL return in get_routes All three clone_routes call sites now check for NULL and return an empty list on allocation failure, preventing NULL pointer dereference in the responder. Note: Provider NULL checks already exist in responder (lines 919, 925, 931 use && short-circuit evaluation). --- .../dhcp_inform/dhcp_inform_static_provider.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c index ad9dde9225..9755b4da0f 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c @@ -405,8 +405,12 @@ METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, if (this->global_routes && this->global_routes->get_count(this->global_routes) > 0) { + linked_list_t *routes; + DBG2(DBG_CFG, "dhcp-inform: returning global routes (no client IP)"); - return clone_routes(this->global_routes); + routes = clone_routes(this->global_routes); + /* Return empty list on allocation failure */ + return routes ? routes : linked_list_create(); } return linked_list_create(); } @@ -424,6 +428,8 @@ METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, { if (ip_in_subnet(client, pool->network, pool->prefix)) { + linked_list_t *routes; + enumerator->destroy(enumerator); client->destroy(client); @@ -431,7 +437,9 @@ METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, "returning %d pool-specific routes", client_ip, pool->name, pool->routes->get_count(pool->routes)); - return clone_routes(pool->routes); + routes = clone_routes(pool->routes); + /* Return empty list on allocation failure */ + return routes ? routes : linked_list_create(); } } enumerator->destroy(enumerator); @@ -441,9 +449,13 @@ METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, if (this->global_routes && this->global_routes->get_count(this->global_routes) > 0) { + linked_list_t *routes; + DBG1(DBG_CFG, "dhcp-inform: client %s using %d global routes", client_ip, this->global_routes->get_count(this->global_routes)); - return clone_routes(this->global_routes); + routes = clone_routes(this->global_routes); + /* Return empty list on allocation failure */ + return routes ? routes : linked_list_create(); } DBG1(DBG_CFG, "dhcp-inform: no routes configured for client %s", client_ip); From 0c51015ca235f6b312511a3ccb999d958f4da209 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 17 Jan 2026 18:01:55 +0200 Subject: [PATCH 9/9] fix: address sixth Copilot review round (per coding rules 18, 20, 21) Rule 18 (Interface Contract): - Update provider.h get_routes docs to clarify NULL vs empty return Rule 20 (Cascading Failure): - ts_provider: handle NULL from extract_ts_from_ike_sa - Document that fallback linked_list_create may also fail (OOM) Rule 21 (Terminology): - Change "MUTUALLY EXCLUSIVE" to "PRIORITY-BASED ROUTE SELECTION" - Update dhcp-inform.conf and responder.c comments - Clarifies that multiple sources = graceful fallback, not error --- .../plugins/dhcp_inform/dhcp-inform.conf | 20 +++++++++---------- .../dhcp_inform/dhcp_inform_provider.h | 5 +++-- .../dhcp_inform/dhcp_inform_responder.c | 12 +++++------ .../dhcp_inform/dhcp_inform_ts_provider.c | 11 +++++++++- 4 files changed, 29 insertions(+), 19 deletions(-) diff --git a/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf b/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf index ffd1eb8a97..466fb70c6e 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf +++ b/src/libcharon/plugins/dhcp_inform/dhcp-inform.conf @@ -1,13 +1,13 @@ # dhcp-inform plugin configuration # Responds to DHCPINFORM from Windows VPN clients with split-tunnel routes # -# THREE MUTUALLY EXCLUSIVE MODES (priority order): +# PRIORITY-BASED ROUTE SELECTION (first available wins): # 1. Traffic Selectors - when use_ts_routes=yes # 2. Database - when database URI configured -# 3. Static configuration - when no database +# 3. Static configuration - fallback when above unavailable # -# Modes are EXCLUSIVE - only the highest-priority configured mode is used. -# If multiple modes are configured, lower-priority modes are silently ignored. +# Only ONE source is used per request. Multiple sources can be configured +# for graceful fallback - highest-priority available source is selected. dhcp-inform { # Enable the plugin @@ -23,19 +23,19 @@ dhcp-inform { # dns = 172.16.69.1 # ========================================================================= - # MODE 1: Traffic Selector Routes (EXCLUSIVE) + # MODE 1: Traffic Selector Routes (highest priority) # ========================================================================= # Routes are extracted from IKE SA traffic selectors. # Designed for Windows 7 compatibility (doesn't handle TS properly). - # When enabled, database and static routes are IGNORED. + # When enabled, this source takes priority over database and static. # # use_ts_routes = yes # ========================================================================= - # MODE 2: Database Routes (EXCLUSIVE) + # MODE 2: Database Routes (second priority) # ========================================================================= # Routes are queried from database by matching client IP to pool CIDR. - # When database is configured, static routes are IGNORED. + # Used when TS routes disabled. Takes priority over static routes. # Works with PostgreSQL, MySQL, SQLite via strongSwan database abstraction. # # Requires VIEW v_pool_routes (pool_cidr TEXT, route TEXT): @@ -47,9 +47,9 @@ dhcp-inform { # database = sqlite:///etc/strongswan/routes.db # ========================================================================= - # MODE 3: Static Routes (EXCLUSIVE - only when no database) + # MODE 3: Static Routes (fallback) # ========================================================================= - # Used ONLY when no database is configured. + # Used when TS routes disabled and no database configured. # # Global routes - apply to all clients not matching a specific pool: # diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_provider.h b/src/libcharon/plugins/dhcp_inform/dhcp_inform_provider.h index 9d1915327a..a7fbd52a4d 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_provider.h +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_provider.h @@ -34,8 +34,9 @@ struct dhcp_inform_provider_t { * * @param this provider instance * @param client_ip client's virtual IP address string - * @return linked_list_t of traffic_selector_t (caller destroys), - * or NULL on error + * @return linked_list_t of traffic_selector_t (caller destroys). + * Returns valid list (possibly empty) on success. + * NULL indicates allocation failure; callers must handle. */ linked_list_t* (*get_routes)(dhcp_inform_provider_t *this, const char *client_ip); diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c index 2ad38df5bd..bd972a27d7 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_responder.c @@ -240,13 +240,13 @@ static void add_routes_from_provider(dhcp_inform_provider_t *provider, /** * Get routes for client using available providers. * - * THREE MUTUALLY EXCLUSIVE MODES (priority order): - * 1. TS routes - when use_ts_routes=yes, ONLY traffic selector routes - * 2. DB routes - when database configured, ONLY database routes - * 3. Static routes - when no DB, use config routes with per-pool overrides + * PRIORITY-BASED ROUTE SELECTION (first available wins): + * 1. TS routes - when use_ts_routes=yes, traffic selectors from IKE SA + * 2. DB routes - when database configured, routes from SQL database + * 3. Static routes - fallback to config routes with per-pool overrides * - * Modes are exclusive - if multiple are configured, highest priority wins. - * This is intentional: config errors don't crash, just use first available mode. + * Only ONE source is used per request. Multiple sources can be configured + * for graceful fallback - highest-priority available source is selected. */ static linked_list_t *get_routes_for_client(private_dhcp_inform_responder_t *this, const char *client_ip) diff --git a/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c index cebe854f40..74095e8abc 100644 --- a/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c +++ b/src/libcharon/plugins/dhcp_inform/dhcp_inform_ts_provider.c @@ -204,12 +204,21 @@ static linked_list_t *extract_ts_from_ike_sa(const char *client_ip) METHOD(dhcp_inform_provider_t, get_routes, linked_list_t*, private_dhcp_inform_ts_provider_t *this, const char *client_ip) { + linked_list_t *routes; + if (!this->enabled) { + /* Provider disabled - return empty list (may be NULL on OOM) */ return linked_list_create(); } - return extract_ts_from_ike_sa(client_ip); + routes = extract_ts_from_ike_sa(client_ip); + if (!routes) + { + /* Allocation failed - return empty list as fallback (may be NULL on OOM) */ + return linked_list_create(); + } + return routes; } METHOD(dhcp_inform_provider_t, get_name, const char*,