Commit 780f2d93 authored by David Goulet's avatar David Goulet 🎭
Browse files

config: Catch missing Bridge for ClientTransportPlugin



First of all, tor doesn't support multiple transport name on a single
*TransportPlugin line so get rid of that. The manpage has never documented
that feature and it complexifies the line for no reason. A user can just use
multiple ClientTransportPlugin line instead.

Second, if no Bridge line is found, error immediately and exit. Don't leave
the Bridge line alone there leading to bugs like #25528

Third, when making sure we have a Bridge line with a ClientTransportPlugin, we
now check at the managed proxy list. In order to pull this off, the transport
name had to be added to the managed_proxy_t object so we can match for which
transport it is used for. And thus justifying the removal of multiple
transport support which adds no benefit except complexity.

Fixes #40106
Signed-off-by: David Goulet's avatarDavid Goulet <dgoulet@torproject.org>
parent 73fb44dc
Pipeline #1763 passed with stage
in 71 minutes and 5 seconds
o Minor bugfixes (config, bridge):
- Really fix the case where torrc has a missing Bridge line while configured
with a ClientTransportPlugin. Previously, we failed to also look at the
managed proxy list and thus it would fail for the "exec" case. Fixes bug
40106; bugfix on 0.4.5.1-alpha.
- Our code was supporting a *TransportPlugin line to have multiple
transport name but our documentation never detailed it. Remove that
capability in favor of using multiple *TransportPlugin line instead.
......@@ -2198,7 +2198,8 @@ options_act,(const or_options_t *old_options))
* validation time. */
SMARTLIST_FOREACH_BEGIN(bridge_list_get(), const bridge_info_t *, bi) {
const char *bi_transport_name = bridget_get_transport_name(bi);
if (bi_transport_name && !transport_get_by_name(bi_transport_name)) {
if (bi_transport_name && (!transport_get_by_name(bi_transport_name) &&
!managed_proxy_has_transport(bi_transport_name))) {
log_warn(LD_CONFIG, "Bridge line with transport %s is missing a "
"ClientTransportPlugin line", bi_transport_name);
return -1;
......@@ -5266,8 +5267,7 @@ pt_parse_transport_line(const or_options_t *options,
smartlist_t *items = NULL;
int r;
const char *transports = NULL;
smartlist_t *transport_list = NULL;
const char *transport_name = NULL;
char *type = NULL;
char *addrport = NULL;
tor_addr_t addr;
......@@ -5279,7 +5279,6 @@ pt_parse_transport_line(const or_options_t *options,
char **proxy_argv = NULL;
char **tmp = NULL;
int proxy_argc, i;
int is_useless_proxy = 1;
int line_length;
......@@ -5296,25 +5295,20 @@ pt_parse_transport_line(const or_options_t *options,
goto err;
}
/* Get the first line element, split it to commas into
transport_list (in case it's multiple transports) and validate
the transport names. */
transports = smartlist_get(items, 0);
transport_list = smartlist_new();
smartlist_split_string(transport_list, transports, ",",
SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0);
SMARTLIST_FOREACH_BEGIN(transport_list, const char *, transport_name) {
/* validate transport names */
if (!string_is_C_identifier(transport_name)) {
log_warn(LD_CONFIG, "Transport name is not a C identifier (%s).",
transport_name);
goto err;
}
/* see if we actually need the transports provided by this proxy */
if (!validate_only && transport_is_needed(transport_name))
is_useless_proxy = 0;
} SMARTLIST_FOREACH_END(transport_name);
/* Get transport name. */
transport_name = smartlist_get(items, 0);
if (!string_is_C_identifier(transport_name)) {
log_warn(LD_CONFIG, "Transport name is not a C identifier (%s).",
transport_name);
goto err;
}
/* If we can't find a Bridge line for that same transport, it is an error
* and thus stop immediately. */
if (!validate_only && !server && !transport_is_needed(transport_name)) {
log_warn(LD_CONFIG, "ClientTransportPlugin line for %s is missing a "
"corresponding Bridge line.", transport_name);
goto err;
}
type = smartlist_get(items, 1);
if (!strcmp(type, "exec")) {
......@@ -5357,20 +5351,13 @@ pt_parse_transport_line(const or_options_t *options,
if (is_managed) {
/* managed */
if (!server && !validate_only && is_useless_proxy) {
log_info(LD_GENERAL,
"Pluggable transport proxy (%s) does not provide "
"any needed transports and will not be launched.",
line);
}
/*
* If we are not just validating, use the rest of the line as the
* argv of the proxy to be launched. Also, make sure that we are
* only launching proxies that contribute useful transports.
*/
if (!validate_only && (server || !is_useless_proxy)) {
if (!validate_only) {
proxy_argc = line_length - 2;
tor_assert(proxy_argc > 0);
proxy_argv = tor_calloc((proxy_argc + 1), sizeof(char *));
......@@ -5385,9 +5372,9 @@ pt_parse_transport_line(const or_options_t *options,
/* kickstart the thing */
if (server) {
pt_kickstart_server_proxy(transport_list, proxy_argv);
pt_kickstart_server_proxy(transport_name, proxy_argv);
} else {
pt_kickstart_client_proxy(transport_list, proxy_argv);
pt_kickstart_client_proxy(transport_name, proxy_argv);
}
}
} else {
......@@ -5402,13 +5389,6 @@ pt_parse_transport_line(const or_options_t *options,
goto err;
}
if (smartlist_len(transport_list) != 1) {
log_warn(LD_CONFIG,
"You can't have an external proxy with more than "
"one transport.");
goto err;
}
addrport = smartlist_get(items, 2);
if (tor_addr_port_lookup(addrport, &addr, &port) < 0) {
......@@ -5426,12 +5406,10 @@ pt_parse_transport_line(const or_options_t *options,
if (!validate_only) {
log_info(LD_DIR, "%s '%s' at %s.",
server ? "Server transport" : "Transport",
transports, fmt_addrport(&addr, port));
transport_name, fmt_addrport(&addr, port));
if (!server) {
transport_add_from_config(&addr, port,
smartlist_get(transport_list, 0),
socks_ver);
transport_add_from_config(&addr, port, transport_name, socks_ver);
}
}
}
......@@ -5445,10 +5423,6 @@ pt_parse_transport_line(const or_options_t *options,
done:
SMARTLIST_FOREACH(items, char*, s, tor_free(s));
smartlist_free(items);
if (transport_list) {
SMARTLIST_FOREACH(transport_list, char*, s, tor_free(s));
smartlist_free(transport_list);
}
return r;
}
......
......@@ -368,6 +368,26 @@ static int unconfigured_proxies_n = 0;
/** Boolean: True iff we might need to restart some proxies. */
static int check_if_restarts_needed = 0;
/** Return true iff we have a managed_proxy_t in the global list is for the
* given transport name. */
bool
managed_proxy_has_transport(const char *transport_name)
{
tor_assert(transport_name);
if (!managed_proxy_list) {
return false;
}
SMARTLIST_FOREACH_BEGIN(managed_proxy_list, const managed_proxy_t *, mp) {
if (!strcmp(mp->transport_name, transport_name)) {
return true;
}
} SMARTLIST_FOREACH_END(mp);
return false;
}
/** Return true if there are still unconfigured managed proxies, or proxies
* that need restarting. */
int
......@@ -724,6 +744,7 @@ managed_proxy_destroy(managed_proxy_t *mp,
process_terminate(mp->process);
}
tor_free(mp->transport_name);
tor_free(mp);
}
......@@ -1495,10 +1516,11 @@ create_managed_proxy_environment(const managed_proxy_t *mp)
*
* Requires that proxy_argv have at least one element. */
STATIC managed_proxy_t *
managed_proxy_create(const smartlist_t *with_transport_list,
char **proxy_argv, int is_server)
managed_proxy_create(const char *transport_name, char **proxy_argv,
int is_server)
{
managed_proxy_t *mp = tor_malloc_zero(sizeof(managed_proxy_t));
mp->transport_name = tor_strdup(transport_name);
mp->conf_state = PT_PROTO_INFANT;
mp->is_server = is_server;
mp->argv = proxy_argv;
......@@ -1507,8 +1529,7 @@ managed_proxy_create(const smartlist_t *with_transport_list,
mp->process = process_new(proxy_argv[0]);
mp->transports_to_launch = smartlist_new();
SMARTLIST_FOREACH(with_transport_list, const char *, transport,
add_transport_to_proxy(transport, mp));
add_transport_to_proxy(mp->transport_name, mp);
/* register the managed proxy */
if (!managed_proxy_list)
......@@ -1521,9 +1542,9 @@ managed_proxy_create(const smartlist_t *with_transport_list,
return mp;
}
/** Register proxy with <b>proxy_argv</b>, supporting transports in
* <b>transport_list</b>, to the managed proxy subsystem.
* If <b>is_server</b> is true, then the proxy is a server proxy.
/** Register proxy with <b>proxy_argv</b>, supporting the transport name to
* the managed proxy subsystem. If <b>is_server</b> is true, then the proxy
* is a server proxy.
*
* Takes ownership of proxy_argv.
*
......@@ -1531,12 +1552,14 @@ managed_proxy_create(const smartlist_t *with_transport_list,
* elements, containing at least one element.
**/
MOCK_IMPL(void,
pt_kickstart_proxy, (const smartlist_t *with_transport_list,
char **proxy_argv, int is_server))
pt_kickstart_proxy,(const char *transport_name, char **proxy_argv,
int is_server))
{
managed_proxy_t *mp=NULL;
transport_t *old_transport = NULL;
tor_assert(transport_name);
if (!proxy_argv || !proxy_argv[0]) {
return;
}
......@@ -1544,7 +1567,7 @@ pt_kickstart_proxy, (const smartlist_t *with_transport_list,
mp = get_managed_proxy_by_argv_and_type(proxy_argv, is_server);
if (!mp) { /* we haven't seen this proxy before */
managed_proxy_create(with_transport_list, proxy_argv, is_server);
managed_proxy_create(transport_name, proxy_argv, is_server);
} else { /* known proxy. add its transport to its transport list */
if (mp->was_around_before_config_read) {
......@@ -1558,18 +1581,15 @@ pt_kickstart_proxy, (const smartlist_t *with_transport_list,
check_if_restarts_needed = 1;
}
/* For each new transport, check if the managed proxy used to
/* For the transport, check if the managed proxy used to
support it before the SIGHUP. If that was the case, make sure
it doesn't get removed because we might reuse it. */
SMARTLIST_FOREACH_BEGIN(with_transport_list, const char *, transport) {
old_transport = transport_get_by_name(transport);
if (old_transport)
old_transport->marked_for_removal = 0;
} SMARTLIST_FOREACH_END(transport);
old_transport = transport_get_by_name(transport_name);
if (old_transport)
old_transport->marked_for_removal = 0;
}
SMARTLIST_FOREACH(with_transport_list, const char *, transport,
add_transport_to_proxy(transport, mp));
add_transport_to_proxy(transport_name, mp);
free_execve_args(proxy_argv);
}
}
......
......@@ -41,15 +41,15 @@ void transport_free_(transport_t *transport);
#define transport_free(tr) FREE_AND_NULL(transport_t, transport_free_, (tr))
MOCK_DECL(transport_t*, transport_get_by_name, (const char *name));
bool managed_proxy_has_transport(const char *transport_name);
MOCK_DECL(void, pt_kickstart_proxy,
(const smartlist_t *transport_list, char **proxy_argv,
int is_server));
(const char *transport_name, char **proxy_argv, int is_server));
#define pt_kickstart_client_proxy(tl, pa) \
pt_kickstart_proxy(tl, pa, 0)
#define pt_kickstart_server_proxy(tl, pa) \
pt_kickstart_proxy(tl, pa, 1)
#define pt_kickstart_client_proxy(name, pa) \
pt_kickstart_proxy(name, pa, 0)
#define pt_kickstart_server_proxy(name, pa) \
pt_kickstart_proxy(name, pa, 1)
void pt_configure_remaining_proxies(void);
......@@ -87,6 +87,7 @@ struct process_t;
/** Structure containing information of a managed proxy. */
typedef struct {
char *transport_name; /* Transport name */
enum pt_proto_state conf_state; /* the current configuration state */
char **argv; /* the cli arguments of this proxy */
int conf_protocol; /* the configuration protocol version used */
......@@ -135,8 +136,9 @@ STATIC char *get_transport_options_for_server_proxy(const managed_proxy_t *mp);
STATIC void managed_proxy_destroy(managed_proxy_t *mp,
int also_terminate_process);
STATIC managed_proxy_t *managed_proxy_create(const smartlist_t *transport_list,
char **proxy_argv, int is_server);
STATIC managed_proxy_t *managed_proxy_create(const char *name,
char **proxy_argv,
int is_server);
STATIC int configure_proxy(managed_proxy_t *mp);
......
......@@ -627,8 +627,8 @@ get_total_system_memory_mock(size_t *mem_out)
/* Mocks needed for the transport plugin line test */
static void pt_kickstart_proxy_mock(const smartlist_t *transport_list,
char **proxy_argv, int is_server);
static void pt_kickstart_proxy_mock(const char *name, char **proxy_argv,
int is_server);
static int transport_add_from_config_mock(const tor_addr_t *addr,
uint16_t port, const char *name,
int socks_ver);
......@@ -640,12 +640,11 @@ static int transport_is_needed_mock_call_count = 0;
static int transport_is_needed_mock_return = 0;
static void
pt_kickstart_proxy_mock(const smartlist_t *transport_list,
char **proxy_argv, int is_server)
pt_kickstart_proxy_mock(const char *name, char **proxy_argv, int is_server)
{
(void) transport_list;
(void) proxy_argv;
(void) is_server;
(void) name;
/* XXXX check that args are as expected. */
++pt_kickstart_proxy_mock_call_count;
......@@ -759,13 +758,7 @@ test_config_parse_transport_plugin_line(void *arg)
"transport_1 exec /usr/bin/fake-transport", 1, 0);
tt_int_op(r, OP_EQ, 0);
r = pt_parse_transport_line(options,
"transport_1 exec /usr/bin/fake-transport", 1, 1);
tt_int_op(r, OP_EQ, 0);
r = pt_parse_transport_line(options,
"transport_1,transport_2 exec /usr/bin/fake-transport", 1, 0);
tt_int_op(r, OP_EQ, 0);
r = pt_parse_transport_line(options,
"transport_1,transport_2 exec /usr/bin/fake-transport", 1, 1);
"transport_1 exec /usr/bin/fake-transport", 1, 1);
tt_int_op(r, OP_EQ, 0);
/* Bad transport identifiers */
r = pt_parse_transport_line(options,
......@@ -786,13 +779,6 @@ test_config_parse_transport_plugin_line(void *arg)
r = pt_parse_transport_line(options,
"transport_1 proxy 1.2.3.4:567", 1, 1);
tt_int_op(r, OP_EQ, 0);
/* Multiple-transport error exit */
r = pt_parse_transport_line(options,
"transport_1,transport_2 socks5 1.2.3.4:567", 1, 0);
tt_int_op(r, OP_LT, 0);
r = pt_parse_transport_line(options,
"transport_1,transport_2 proxy 1.2.3.4:567", 1, 1);
tt_int_op(r, OP_LT, 0);
/* No port error exit */
r = pt_parse_transport_line(options,
"transport_1 socks5 1.2.3.4", 1, 0);
......@@ -862,7 +848,7 @@ test_config_parse_transport_plugin_line(void *arg)
r = pt_parse_transport_line(options,
"transport_1 exec /usr/bin/fake-transport", 0, 0);
/* Should have succeeded */
tt_int_op(r, OP_EQ, 0);
tt_int_op(r, OP_EQ, -1);
/* transport_is_needed() should have been called */
tt_assert(transport_is_needed_mock_call_count ==
old_transport_is_needed_mock_call_count + 1);
......
......@@ -146,7 +146,6 @@ static void
test_pt_get_transport_options(void *arg)
{
char **execve_args;
smartlist_t *transport_list = smartlist_new();
managed_proxy_t *mp;
or_options_t *options = get_options_mutable();
char *opt_str = NULL;
......@@ -157,7 +156,7 @@ test_pt_get_transport_options(void *arg)
execve_args[0] = tor_strdup("cheeseshop");
execve_args[1] = NULL;
mp = managed_proxy_create(transport_list, execve_args, 1);
mp = managed_proxy_create("aname", execve_args, 1);
tt_ptr_op(mp, OP_NE, NULL);
opt_str = get_transport_options_for_server_proxy(mp);
tt_ptr_op(opt_str, OP_EQ, NULL);
......@@ -191,7 +190,6 @@ test_pt_get_transport_options(void *arg)
tor_free(opt_str);
config_free_lines(cl);
managed_proxy_destroy(mp, 0);
smartlist_free(transport_list);
}
static void
......@@ -250,7 +248,6 @@ test_pt_get_extrainfo_string(void *arg)
{
managed_proxy_t *mp1 = NULL, *mp2 = NULL;
char **argv1, **argv2;
smartlist_t *t1 = smartlist_new(), *t2 = smartlist_new();
int r;
char *s = NULL;
(void) arg;
......@@ -265,8 +262,8 @@ test_pt_get_extrainfo_string(void *arg)
argv2[2] = tor_strdup("Schlangenkraft");
argv2[3] = NULL;
mp1 = managed_proxy_create(t1, argv1, 1);
mp2 = managed_proxy_create(t2, argv2, 1);
mp1 = managed_proxy_create("aname", argv1, 1);
mp2 = managed_proxy_create("aname", argv2, 1);
r = parse_smethod_line("SMETHOD hagbard 127.0.0.1:5555", mp1);
tt_int_op(r, OP_EQ, 0);
......@@ -284,9 +281,6 @@ test_pt_get_extrainfo_string(void *arg)
"transport celine 127.0.0.1:1723 card=no-enemy\n");
done:
/* XXXX clean up better */
smartlist_free(t1);
smartlist_free(t2);
tor_free(s);
}
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment