Reviewed-by: Konstantin Kostiuk <kkost...@redhat.com>
On Fri, Jul 12, 2024 at 4:26 PM Daniel P. Berrangé <berra...@redhat.com>
wrote:
> It is confusing having many different pieces of code enabling and
> disabling commands, and it is not clear that they all have the same
> semantics, especially wrt prioritization of the block/allow lists.
> The code attempted to prevent the user from setting both the block
> and allow lists concurrently, however, the logic was flawed as it
> checked settings in the configuration file separately from the
> command line arguments. Thus it was possible to set a block list
> in the config file and an allow list via a command line argument.
> The --dump-conf option also creates a configuration file with both
> keys present, even if unset, which means it is creating a config
> that cannot actually be loaded again.
>
> Centralizing the code in a single method "ga_apply_command_filters"
> will provide a strong guarantee of consistency and clarify the
> intended behaviour. With this there is no compelling technical
> reason to prevent concurrent setting of both the allow and block
> lists, so this flawed restriction is removed.
>
> Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
> ---
> docs/interop/qemu-ga.rst | 14 +++++
> qga/commands-posix.c | 6 --
> qga/commands-win32.c | 6 --
> qga/main.c | 128 +++++++++++++++++----------------------
> 4 files changed, 70 insertions(+), 84 deletions(-)
>
> diff --git a/docs/interop/qemu-ga.rst b/docs/interop/qemu-ga.rst
> index e42b370319..fb75cfd8d4 100644
> --- a/docs/interop/qemu-ga.rst
> +++ b/docs/interop/qemu-ga.rst
> @@ -28,6 +28,20 @@ configuration options on the command line. For the same
> key, the last
> option wins, but the lists accumulate (see below for configuration
> file format).
>
> +If an allowed RPCs list is defined in the configuration, then all
> +RPCs will be blocked by default, except for the allowed list.
> +
> +If a blocked RPCs list is defined in the configuration, then all
> +RPCs will be allowed by default, except for the blocked list.
> +
> +If both allowed and blocked RPCs lists are defined in the configuration,
> +then all RPCs will be blocked by default, then the allowed list will
> +be applied, followed by the blocked list.
> +
> +While filesystems are frozen, all except for a designated safe set
> +of RPCs will blocked, regardless of what the general configuration
> +declares.
> +
> Options
> -------
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index f4104f2760..578d29f228 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1136,12 +1136,6 @@ error:
>
> #endif /* HAVE_GETIFADDRS */
>
> -/* add unsupported commands to the list of blocked RPCs */
> -GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
> -{
> - return blockedrpcs;
> -}
> -
> /* register init/cleanup routines for stateful command groups */
> void ga_command_state_init(GAState *s, GACommandState *cs)
> {
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 5866cc2e3c..61b36da469 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -1958,12 +1958,6 @@ done:
> g_free(rawpasswddata);
> }
>
> -/* add unsupported commands to the list of blocked RPCs */
> -GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
> -{
> - return blockedrpcs;
> -}
> -
> /* register init/cleanup routines for stateful command groups */
> void ga_command_state_init(GAState *s, GACommandState *cs)
> {
> diff --git a/qga/main.c b/qga/main.c
> index 6ae911eb15..b8f7b1e4a3 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -423,60 +423,79 @@ static gint ga_strcmp(gconstpointer str1,
> gconstpointer str2)
> return strcmp(str1, str2);
> }
>
> -/* disable commands that aren't safe for fsfreeze */
> -static void ga_disable_not_allowed_freeze(const QmpCommand *cmd, void
> *opaque)
> +static bool ga_command_is_allowed(const QmpCommand *cmd, GAState *state)
> {
> - bool allowed = false;
> int i = 0;
> + GAConfig *config = state->config;
> const char *name = qmp_command_name(cmd);
> + /* Fallback policy is allow everything */
> + bool allowed = true;
>
> - while (ga_freeze_allowlist[i] != NULL) {
> - if (strcmp(name, ga_freeze_allowlist[i]) == 0) {
> + if (config->allowedrpcs) {
> + /*
> + * If an allow-list is given, this changes the fallback
> + * policy to deny everything
> + */
> + allowed = false;
> +
> + if (g_list_find_custom(config->allowedrpcs, name, ga_strcmp) !=
> NULL) {
> allowed = true;
> }
> - i++;
> }
> - if (!allowed) {
> - g_debug("disabling command: %s", name);
> - qmp_disable_command(&ga_commands, name, "the agent is in frozen
> state");
> - }
> -}
>
> -/* [re-]enable all commands, except those explicitly blocked by user */
> -static void ga_enable_non_blocked(const QmpCommand *cmd, void *opaque)
> -{
> - GAState *s = opaque;
> - GList *blockedrpcs = s->blockedrpcs;
> - GList *allowedrpcs = s->allowedrpcs;
> - const char *name = qmp_command_name(cmd);
> -
> - if (g_list_find_custom(blockedrpcs, name, ga_strcmp) == NULL) {
> - if (qmp_command_is_enabled(cmd)) {
> - return;
> + /*
> + * If both allowedrpcs and blockedrpcs are set, the blocked
> + * list will take priority
> + */
> + if (config->blockedrpcs) {
> + if (g_list_find_custom(config->blockedrpcs, name, ga_strcmp) !=
> NULL) {
> + allowed = false;
> }
> + }
>
> - if (allowedrpcs &&
> - g_list_find_custom(allowedrpcs, name, ga_strcmp) == NULL) {
> - return;
> - }
> + /*
> + * If frozen, this filtering must take priority over
> + * absolutely everything
> + */
> + if (state->frozen) {
> + allowed = false;
>
> - g_debug("enabling command: %s", name);
> - qmp_enable_command(&ga_commands, name);
> + while (ga_freeze_allowlist[i] != NULL) {
> + if (strcmp(name, ga_freeze_allowlist[i]) == 0) {
> + allowed = true;
> + }
> + i++;
> + }
> }
> +
> + return allowed;
> }
>
> -/* disable commands that aren't allowed */
> -static void ga_disable_not_allowed(const QmpCommand *cmd, void *opaque)
> +static void ga_apply_command_filters_iter(const QmpCommand *cmd, void
> *opaque)
> {
> - GList *allowedrpcs = opaque;
> + GAState *state = opaque;
> + bool want = ga_command_is_allowed(cmd, state);
> + bool have = qmp_command_is_enabled(cmd);
> const char *name = qmp_command_name(cmd);
>
> - if (g_list_find_custom(allowedrpcs, name, ga_strcmp) == NULL) {
> + if (want == have) {
> + return;
> + }
> +
> + if (have) {
> g_debug("disabling command: %s", name);
> qmp_disable_command(&ga_commands, name, "the command is not
> allowed");
> + } else {
> + g_debug("enabling command: %s", name);
> + qmp_enable_command(&ga_commands, name);
> }
> }
>
> +static void ga_apply_command_filters(GAState *state)
> +{
> + qmp_for_each_command(&ga_commands, ga_apply_command_filters_iter,
> state);
> +}
> +
> static bool ga_create_file(const char *path)
> {
> int fd = open(path, O_CREAT | O_WRONLY, S_IWUSR | S_IRUSR);
> @@ -509,15 +528,14 @@ void ga_set_frozen(GAState *s)
> if (ga_is_frozen(s)) {
> return;
> }
> - /* disable all forbidden (for frozen state) commands */
> - qmp_for_each_command(&ga_commands, ga_disable_not_allowed_freeze,
> NULL);
> g_warning("disabling logging due to filesystem freeze");
> - ga_disable_logging(s);
> s->frozen = true;
> if (!ga_create_file(s->state_filepath_isfrozen)) {
> g_warning("unable to create %s, fsfreeze may not function
> properly",
> s->state_filepath_isfrozen);
> }
> + ga_apply_command_filters(s);
> + ga_disable_logging(s);
> }
>
> void ga_unset_frozen(GAState *s)
> @@ -549,12 +567,12 @@ void ga_unset_frozen(GAState *s)
> }
>
> /* enable all disabled, non-blocked and allowed commands */
> - qmp_for_each_command(&ga_commands, ga_enable_non_blocked, s);
> s->frozen = false;
> if (!ga_delete_file(s->state_filepath_isfrozen)) {
> g_warning("unable to delete %s, fsfreeze may not function
> properly",
> s->state_filepath_isfrozen);
> }
> + ga_apply_command_filters(s);
> }
>
> #ifdef CONFIG_FSFREEZE
> @@ -1086,13 +1104,6 @@ static void config_load(GAConfig *config, const
> char *confpath, bool required)
> split_list(config->aliststr,
> ","));
> }
>
> - if (g_key_file_has_key(keyfile, "general", "block-rpcs", NULL) &&
> - g_key_file_has_key(keyfile, "general", "allow-rpcs", NULL)) {
> - g_critical("wrong config, using 'block-rpcs' and 'allow-rpcs'
> keys at"
> - " the same time is not allowed");
> - exit(EXIT_FAILURE);
> - }
> -
> end:
> g_key_file_free(keyfile);
> if (gerr && (required ||
> @@ -1172,7 +1183,6 @@ static void config_parse(GAConfig *config, int argc,
> char **argv)
> {
> const char *sopt = "hVvdc:m:p:l:f:F::b:a:s:t:Dr";
> int opt_ind = 0, ch;
> - bool block_rpcs = false, allow_rpcs = false;
> const struct option lopt[] = {
> { "help", 0, NULL, 'h' },
> { "version", 0, NULL, 'V' },
> @@ -1268,7 +1278,6 @@ static void config_parse(GAConfig *config, int argc,
> char **argv)
> }
> config->blockedrpcs = g_list_concat(config->blockedrpcs,
> split_list(optarg, ","));
> - block_rpcs = true;
> break;
> }
> case 'a': {
> @@ -1278,7 +1287,6 @@ static void config_parse(GAConfig *config, int argc,
> char **argv)
> }
> config->allowedrpcs = g_list_concat(config->allowedrpcs,
> split_list(optarg, ","));
> - allow_rpcs = true;
> break;
> }
> #ifdef _WIN32
> @@ -1319,12 +1327,6 @@ static void config_parse(GAConfig *config, int
> argc, char **argv)
> exit(EXIT_FAILURE);
> }
> }
> -
> - if (block_rpcs && allow_rpcs) {
> - g_critical("wrong commandline, using --block-rpcs and
> --allow-rpcs at the"
> - " same time is not allowed");
> - exit(EXIT_FAILURE);
> - }
> }
>
> static void config_free(GAConfig *config)
> @@ -1435,7 +1437,6 @@ static GAState *initialize_agent(GAConfig *config,
> int socket_activation)
> s->deferred_options.log_filepath = config->log_filepath;
> }
> ga_disable_logging(s);
> - qmp_for_each_command(&ga_commands, ga_disable_not_allowed_freeze,
> NULL);
> } else {
> if (config->daemonize) {
> become_daemon(config->pid_filepath);
> @@ -1459,25 +1460,6 @@ static GAState *initialize_agent(GAConfig *config,
> int socket_activation)
> return NULL;
> }
>
> - if (config->allowedrpcs) {
> - qmp_for_each_command(&ga_commands, ga_disable_not_allowed,
> config->allowedrpcs);
> - s->allowedrpcs = config->allowedrpcs;
> - }
> -
> - /*
> - * Some commands can be blocked due to system limitation.
> - * Initialize blockedrpcs list even if allowedrpcs specified.
> - */
> - config->blockedrpcs =
> ga_command_init_blockedrpcs(config->blockedrpcs);
> - if (config->blockedrpcs) {
> - GList *l = config->blockedrpcs;
> - s->blockedrpcs = config->blockedrpcs;
> - do {
> - g_debug("disabling command: %s", (char *)l->data);
> - qmp_disable_command(&ga_commands, l->data, NULL);
> - l = g_list_next(l);
> - } while (l);
> - }
> s->command_state = ga_command_state_new();
> ga_command_state_init(s, s->command_state);
> ga_command_state_init_all(s->command_state);
> @@ -1503,6 +1485,8 @@ static GAState *initialize_agent(GAConfig *config,
> int socket_activation)
> }
> #endif
>
> + ga_apply_command_filters(s);
> +
> ga_state = s;
> return s;
> }
> --
> 2.45.1
>
>
