From: Alan T. DeKok Date: Sun, 15 Dec 2024 16:43:03 +0000 (+0100) Subject: use "mode" instead of replicate / synchronous / originate X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fdb2989ec9f956a2828e9c96c80dbd71a7176a96;p=thirdparty%2Ffreeradius-server.git use "mode" instead of replicate / synchronous / originate as it is more descriptive. The other configuration flags also can be configured in contradictory or conflicting ways. Perhaps we could add a non-synchronous proxy, but that is likely a good idea only if there's no Proxy-State in the packet. i.e. we received the packet from a NAS, which has it's own retransmission timers, and those timers are almost always garbage. --- diff --git a/src/modules/rlm_radius2/bio.c b/src/modules/rlm_radius2/bio.c index 30687520b29..16598592e5d 100644 --- a/src/modules/rlm_radius2/bio.c +++ b/src/modules/rlm_radius2/bio.c @@ -112,7 +112,6 @@ struct bio_request_s { uint32_t num_replies; //!< number of reply packets, sent is in retry.count - bool synchronous; //!< cached from inst->synchronous bool require_message_authenticator; //!< saved from the original packet. bool status_check; //!< is this packet a status check? @@ -901,7 +900,7 @@ static void thread_conn_notify(trunk_connection_t *tconn, connection_t *conn, /* * Over-ride read for replication. */ - if (h->inst->replicate) { + if (h->inst->mode == RLM_RADIUS_MODE_REPLICATE) { read_fn = conn_discard; if (fr_bio_fd_write_only(h->bio) < 0) { @@ -1071,7 +1070,7 @@ static int encode(rlm_radius_t const *inst, request_t *request, bio_request_t *u }, .code = u->code, .id = id, - .add_proxy_state = !inst->originate, + .add_proxy_state = (inst->mode == RLM_RADIUS_MODE_PROXY), }; /* @@ -1236,7 +1235,7 @@ static bool check_for_zombie(fr_event_list_t *el, trunk_connection_t *tconn, fr_ * We're replicating, and don't care about the health of * the home server, and this function should not be called. */ - fr_assert(!h->inst->replicate); + fr_assert(h->inst->mode != RLM_RADIUS_MODE_REPLICATE); /* * If we're status checking OR already zombie, don't go to zombie @@ -1253,7 +1252,7 @@ static bool check_for_zombie(fr_event_list_t *el, trunk_connection_t *tconn, fr_ /* * If we've seen ANY response in the allowed window, then the connection is still alive. */ - if (h->inst->synchronous && fr_time_gt(last_sent, fr_time_wrap(0)) && + if ((h->inst->mode == RLM_RADIUS_MODE_PROXY) && fr_time_gt(last_sent, fr_time_wrap(0)) && (fr_time_lt(fr_time_add(last_sent, h->inst->response_window), now))) return false; /* @@ -1372,7 +1371,7 @@ static void mod_retry(module_ctx_t const *mctx, request_t *request, fr_retry_t c /* * We don't do zombie stuff! */ - if (!tconn || inst->replicate) return; + if (!tconn || (inst->mode == RLM_RADIUS_MODE_REPLICATE)) return; check_for_zombie(unlang_interpret_event_list(request), tconn, now, retry->start); } @@ -1553,7 +1552,7 @@ do_write: /* * Don't print anything extra for replication. */ - if (inst->replicate) { + if (inst->mode == RLM_RADIUS_MODE_REPLICATE) { bio_result_t *r = talloc_get_type_abort(treq->rctx, bio_result_t); r->rcode = RLM_MODULE_OK; @@ -1573,7 +1572,7 @@ do_write: trunk_request_signal_sent(treq); - action = inst->originate ? "Originated" : "Proxied"; + action = inst->mode == (RLM_RADIUS_MODE_CLIENT) ? "Originated" : "Proxied"; } else { /* @@ -1584,7 +1583,7 @@ do_write: fr_assert(!u->status_check); - if (!inst->synchronous) { + if (inst->mode != RLM_RADIUS_MODE_PROXY) { RDEBUG("%s request. Expecting response within %pVs", action, fr_box_time_delta(u->retry.rt)); @@ -1796,10 +1795,12 @@ static void request_demux(UNUSED fr_event_list_t *el, trunk_connection_t *tconn, decode_fail_t reason; uint8_t code = 0; fr_pair_list_t reply; + fr_pair_t *vp; fr_time_t now; fr_pair_list_init(&reply); + /* * Drain the socket of all packets. If we're busy, this * saves a round through the event loop. If we're not @@ -1901,8 +1902,6 @@ static void request_demux(UNUSED fr_event_list_t *el, trunk_connection_t *tconn, * returning an ok/fail packet code. */ if ((u->code == FR_RADIUS_CODE_ACCESS_REQUEST) && (code == FR_RADIUS_CODE_ACCESS_CHALLENGE)) { - fr_pair_t *vp; - vp = fr_pair_find_by_da(&request->reply_pairs, NULL, attr_packet_type); if (!vp) { MEM(vp = fr_pair_afrom_da(request->reply_ctx, attr_packet_type)); @@ -1917,20 +1916,11 @@ static void request_demux(UNUSED fr_event_list_t *el, trunk_connection_t *tconn, fr_pair_delete_by_da(&reply, attr_proxy_state); /* - * If the reply has Message-Authenticator, delete - * it from the proxy reply so that it isn't - * copied over to our reply. But also create a - * reply.Message-Authenticator attribute, so that - * it ends up in our reply. + * If the reply has Message-Authenticator, then over-ride its value with all zeros, so + * that we don't confuse anyone reading the debug output. */ - if (fr_pair_find_by_da(&reply, NULL, attr_message_authenticator)) { - fr_pair_t *vp; - - fr_pair_delete_by_da(&reply, attr_message_authenticator); - - MEM(vp = fr_pair_afrom_da(request->reply_ctx, attr_message_authenticator)); + if ((vp = fr_pair_find_by_da(&reply, NULL, attr_message_authenticator)) != NULL) { (void) fr_pair_value_memdup(vp, (uint8_t const *) "", 1, false); - fr_pair_append(&request->reply_pairs, vp); } treq->request->reply->code = code; @@ -1984,7 +1974,7 @@ static void request_conn_release(connection_t *conn, void *preq_to_reset, UNUSED if (u->ev) (void)fr_event_timer_delete(&u->ev); if (u->packet) bio_request_reset(u); - if (h->inst->replicate) return; + if (h->inst->mode == RLM_RADIUS_MODE_REPLICATE) return; u->num_replies = 0; @@ -2067,7 +2057,6 @@ static void mod_signal(module_ctx_t const *mctx, UNUSED request_t *request, fr_s { rlm_radius_t const *inst = talloc_get_type_abort_const(mctx->mi->data, rlm_radius_t); - bio_thread_t *t = talloc_get_type_abort(module_thread(mctx->mi)->data, bio_thread_t); bio_result_t *r = talloc_get_type_abort(mctx->rctx, bio_result_t); bio_handle_t *h; @@ -2076,7 +2065,7 @@ static void mod_signal(module_ctx_t const *mctx, UNUSED request_t *request, fr_s * synchronous proxying. Ignore the dup, and rely on the * IO submodule to time it's own retransmissions. */ - if ((action == FR_SIGNAL_DUP) && !inst->synchronous) return; + if ((action == FR_SIGNAL_DUP) && (inst->mode != RLM_RADIUS_MODE_PROXY)) return; /* * If we don't have a treq associated with the @@ -2110,12 +2099,6 @@ static void mod_signal(module_ctx_t const *mctx, UNUSED request_t *request, fr_s * has already been sent out. */ case FR_SIGNAL_DUP: - /* - * If we're not synchronous, then rely on - * request_retry() to do the retransmissions. - */ - if (!t->inst->synchronous) return; - h = r->treq->tconn->conn->h; if (h->fd_info->write_blocked) { @@ -2125,8 +2108,8 @@ static void mod_signal(module_ctx_t const *mctx, UNUSED request_t *request, fr_s r->is_retry = true; /* - * We are synchronous, retransmit the current - * request on the same connection. + * We are doing synchronous proxying, retransmit + * the current request on the same connection. * * If it's zombie, we still resend it. If the * connection is dead, then a callback will move @@ -2201,7 +2184,6 @@ static unlang_action_t mod_enqueue(rlm_rcode_t *p_result, rlm_radius_t const *in */ MEM(u = talloc_zero(treq, bio_request_t)); u->code = request->packet->code; - u->synchronous = inst->synchronous; u->priority = request->async->priority; u->recv_time = request->async->recv_time; fr_pair_list_init(&u->extra); @@ -2253,13 +2235,13 @@ static unlang_action_t mod_enqueue(rlm_rcode_t *p_result, rlm_radius_t const *in talloc_set_destructor(u, _bio_request_free); /* - * For many cases, we just time out instead of retrying. + * We do our own retries if we're a client, and a datagram socket. */ - if (inst->synchronous || inst->replicate || (inst->fd_config.socket_type == SOCK_STREAM)) { - retry_config = &inst->timeout_retry; + if ((inst->mode == RLM_RADIUS_MODE_CLIENT) && (inst->fd_config.socket_type == SOCK_DGRAM)) { + retry_config = &inst->retry[u->code]; } else { - retry_config = &inst->retry[u->code]; + retry_config = &inst->timeout_retry; } /* diff --git a/src/modules/rlm_radius2/rlm_radius.c b/src/modules/rlm_radius2/rlm_radius.c index 0e6b5da5885..1c6d433730a 100644 --- a/src/modules/rlm_radius2/rlm_radius.c +++ b/src/modules/rlm_radius2/rlm_radius.c @@ -31,6 +31,7 @@ RCSID("$Id$") #include "rlm_radius.h" +static int mode_parse(TALLOC_CTX *ctx, void *out, UNUSED void *parent, CONF_ITEM *ci, conf_parser_t const *rule); static int type_parse(TALLOC_CTX *ctx, void *out, UNUSED void *parent, CONF_ITEM *ci, conf_parser_t const *rule); static int status_check_type_parse(TALLOC_CTX *ctx, void *out, UNUSED void *parent, CONF_ITEM *ci, conf_parser_t const *rule); static int status_check_update_parse(TALLOC_CTX *ctx, void *out, UNUSED void *parent, CONF_ITEM *ci, conf_parser_t const *rule); @@ -105,24 +106,21 @@ static conf_parser_t const transport_config[] = { * A mapping of configuration file names to internal variables. */ static conf_parser_t const module_config[] = { - /* - * This ref needs to be first, so it can load the - * transport, and push the transport-specific rules to - * the submodule CONF_SECTION. - */ + { FR_CONF_OFFSET("mode", rlm_radius_t, mode), .func = mode_parse }, + { FR_CONF_OFFSET_REF(rlm_radius_t, fd_config, fr_bio_fd_client_config) }, { FR_CONF_OFFSET_FLAGS("type", CONF_FLAG_NOT_EMPTY | CONF_FLAG_MULTI | CONF_FLAG_REQUIRED, rlm_radius_t, types), .func = type_parse }, - { FR_CONF_OFFSET("max_packet_size", rlm_radius_t, max_packet_size), .dflt = "4096" }, - { FR_CONF_OFFSET("max_send_coalesce", rlm_radius_t, max_send_coalesce), .dflt = "1024" }, + { FR_CONF_OFFSET_FLAGS("replicate", CONF_FLAG_DEPRECATED, rlm_radius_t, replicate) }, - { FR_CONF_OFFSET("replicate", rlm_radius_t, replicate) }, + { FR_CONF_OFFSET_FLAGS("synchronous", CONF_FLAG_DEPRECATED, rlm_radius_t, synchronous) }, - { FR_CONF_OFFSET("synchronous", rlm_radius_t, synchronous) }, + { FR_CONF_OFFSET_FLAGS("originate", CONF_FLAG_DEPRECATED, rlm_radius_t, originate) }, - { FR_CONF_OFFSET("originate", rlm_radius_t, originate) }, + { FR_CONF_OFFSET("max_packet_size", rlm_radius_t, max_packet_size), .dflt = "4096" }, + { FR_CONF_OFFSET("max_send_coalesce", rlm_radius_t, max_send_coalesce), .dflt = "1024" }, { FR_CONF_POINTER("status_check", 0, CONF_FLAG_SUBSECTION, NULL), .subcs = (void const *) status_check_config }, @@ -202,6 +200,55 @@ fr_dict_attr_autoload_t rlm_radius_dict_attr[] = { #include "bio.c" +static fr_table_num_sorted_t mode_names[] = { + { L("client"), RLM_RADIUS_MODE_CLIENT }, + { L("proxy"), RLM_RADIUS_MODE_PROXY }, + { L("replicate"), RLM_RADIUS_MODE_REPLICATE }, +}; +static size_t mode_names_len = NUM_ELEMENTS(mode_names); + + +/** Set the mode of operation + * + * @param[in] ctx to allocate data in (instance of rlm_radius). + * @param[out] out Where to write the parsed data. + * @param[in] parent Base structure address. + * @param[in] ci #CONF_PAIR specifying the name of the type module. + * @param[in] rule unused. + * @return + * - 0 on success. + * - -1 on failure. + */ +static int mode_parse(UNUSED TALLOC_CTX *ctx, void *out, void *parent, + CONF_ITEM *ci, UNUSED conf_parser_t const *rule) +{ + char const *name = cf_pair_value(cf_item_to_pair(ci)); + rlm_radius_mode_t mode; + rlm_radius_t *inst = talloc_get_type_abort(parent, rlm_radius_t); + + mode = fr_table_value_by_str(mode_names, name, RLM_RADIUS_MODE_INVALID); + +#if 0 + /* + * Commented out until we upgrade the old configurations. + */ + if (mode == RLM_RADIUS_MODE_INVALID) { + cf_log_err(ci, "Invalid mode name \"%s\"", name); + return -1; + } +#endif + + *(rlm_radius_mode_t *) out = mode; + + /* + * Normally we want connected sockets. + */ + inst->fd_config.type = FR_BIO_FD_CONNECTED; + + return 0; +} + + /** Set which types of packets we can parse * * @param[in] ctx to allocate data in (instance of rlm_radius). @@ -385,7 +432,7 @@ static void radius_fixups(rlm_radius_t const *inst, request_t *request) /* * Check for proxy loops. */ - if (!inst->originate && RDEBUG_ENABLED) { + if ((inst->mode == RLM_RADIUS_MODE_PROXY) && RDEBUG_ENABLED) { fr_dcursor_t cursor; for (vp = fr_pair_dcursor_by_da_init(&cursor, &request->request_pairs, attr_proxy_state); @@ -393,13 +440,19 @@ static void radius_fixups(rlm_radius_t const *inst, request_t *request) vp = fr_dcursor_next(&cursor)) { if (vp->vp_length != 4) continue; - if (memcmp(&inst->proxy_state, vp->vp_octets, 4) == 0) { + if (memcmp(&inst->common_ctx.proxy_state, vp->vp_octets, 4) == 0) { RWARN("Possible proxy loop - please check server configuration."); break; } } } + /* + * @todo - check for proxy loops for client && replicate, too. + * + * This only catches "self loops", but it may be worth doing. + */ + if (request->packet->code != FR_RADIUS_CODE_ACCESS_REQUEST) return; if (fr_pair_find_by_da(&request->request_pairs, NULL, attr_chap_password) && @@ -469,6 +522,7 @@ static unlang_action_t CC_HINT(nonnull) mod_process(rlm_rcode_t *p_result, modul module_thread(mctx->mi)->data, request); } + static int mod_instantiate(module_inst_ctx_t const *mctx) { size_t i, num_types; @@ -479,9 +533,48 @@ static int mod_instantiate(module_inst_ctx_t const *mctx) inst->received_message_authenticator = talloc_zero(NULL, bool); /* Allocated outside of inst to default protection */ /* - * Replication is write-only, and append by default. + * Allow explicit setting of mode. + */ + if (inst->mode != RLM_RADIUS_MODE_INVALID) goto check_others; + + /* + * If not set, try to insinuate it from context. */ if (inst->replicate) { + if (inst->originate) { + cf_log_err(conf, "Cannot set 'replicate=true' and 'originate=true' at the same time."); + return -1; + } + + if (inst->synchronous) { + cf_log_warn(conf, "Ignoring 'synchronous=true' due to 'replicate=true'"); + } + + inst->mode = RLM_RADIUS_MODE_REPLICATE; + goto check_others; + } + + /* + * Argubly we should be allowed to do synchronous proxying _and_ originating client packets. + * + * However, the previous code didn't really do that consistently. + */ + if (inst->synchronous && inst->originate) { + cf_log_err(conf, "Cannot set 'synchronous=true' and 'originate=true'"); + return -1; + } + + if (inst->synchronous) { + inst->mode = RLM_RADIUS_MODE_PROXY; + } else { + inst->mode = RLM_RADIUS_MODE_CLIENT; + } + +check_others: + /* + * Replication is write-only, and append by default. + */ + if (inst->mode == RLM_RADIUS_MODE_REPLICATE) { if (inst->fd_config.filename && (inst->fd_config.flags != O_WRONLY)) { cf_log_info(conf, "Setting 'flags = write-only' for writing to a file"); } @@ -538,7 +631,7 @@ static int mod_instantiate(module_inst_ctx_t const *mctx) inst->common_ctx = (fr_radius_ctx_t) { .secret = inst->secret, .secret_length = talloc_array_length(inst->secret) - 1, - .proxy_state = inst->proxy_state, + .proxy_state = fr_rand(), }; /* @@ -560,8 +653,8 @@ static int mod_instantiate(module_inst_ctx_t const *mctx) * If we're replicating, we don't care if the other end * is alive. */ - if (inst->replicate && inst->status_check) { - cf_log_warn(conf, "Ignoring 'status_check = %s' due to 'replicate = true'", + if (inst->status_check && (inst->mode == RLM_RADIUS_MODE_REPLICATE)) { + cf_log_warn(conf, "Ignoring 'status_check = %s' due to 'mode = replicate'", fr_radius_packet_name[inst->status_check]); inst->status_check = false; } @@ -605,10 +698,9 @@ static int mod_instantiate(module_inst_ctx_t const *mctx) inst->trunk_conf.req_pool_size = 1024 + sizeof(fr_pair_t) + 20; /* - * Don't sanity check the async timers if we're doing - * synchronous proxying. + * Only check the async timers when we're acting as a client. */ - if (inst->synchronous) goto setup_io_submodule; + if (inst->mode != RLM_RADIUS_MODE_CLIENT) return 0; /* * Set limits on retransmission timers @@ -691,12 +783,6 @@ static int mod_instantiate(module_inst_ctx_t const *mctx) FR_TIME_DELTA_BOUND_CHECK("Disconnect-Request.max_rtx_duration", inst->retry[FR_RADIUS_CODE_DISCONNECT_REQUEST].mrd, <=, fr_time_delta_from_sec(30)); } -setup_io_submodule: - /* - * Get random Proxy-State identifier for this module. - */ - inst->proxy_state = fr_rand(); - return 0; } diff --git a/src/modules/rlm_radius2/rlm_radius.h b/src/modules/rlm_radius2/rlm_radius.h index 0113e961bc4..309af469675 100644 --- a/src/modules/rlm_radius2/rlm_radius.h +++ b/src/modules/rlm_radius2/rlm_radius.h @@ -37,6 +37,13 @@ typedef struct rlm_radius_s rlm_radius_t; +typedef enum { + RLM_RADIUS_MODE_INVALID = 0, + RLM_RADIUS_MODE_PROXY, + RLM_RADIUS_MODE_CLIENT, + RLM_RADIUS_MODE_REPLICATE, +} rlm_radius_mode_t; + /* * Define a structure for our module configuration. */ @@ -58,16 +65,16 @@ struct rlm_radius_s { bool replicate; //!< Ignore responses. bool synchronous; //!< Retransmit when receiving a duplicate request. - bool originate; //!< Originating packets, instead of proxying existing ones. + bool originate; //!< Originating packets, instead of proxying existing ones. ///< Controls whether Proxy-State is added to the outbound - ///< request. + ///< request + rlm_radius_mode_t mode; //!< proxy, client, etc. uint32_t max_attributes; //!< Maximum number of attributes to decode in response. fr_radius_require_ma_t require_message_authenticator; //!< Require Message-Authenticator in responses. bool *received_message_authenticator; //!< Received Message-Authenticator in responses. - uint32_t proxy_state; //!< Unique ID (mostly) of this module. uint32_t *types; //!< array of allowed packet types uint32_t status_check; //!< code of status-check type map_list_t status_check_map; //!< attributes for the status-server checks