db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2025-03-21 21:17:22 +02:00
Code Issues Releases Activity

Allow option validity to be determined by command role.

Browse Source 
Validity by command was not granular enough so numerous options needed be marked internal so users would not stumble across them. Options were also needlessly being passed to roles that had no use for them.

Introduce per-role validity lists that depend on what roles are valid per command. Also add a check to ensure that only valid roles are used with a command.

This commit adds the functionality but does not introduce any new behavior, i.e. all options are valid for all roles that the command is valid for. A subsequent commit will introduce the new role restrictions to make the changes easier to audit.
This commit is contained in:
David Steele 2020-12-28 09:43:23 -05:00 committed by GitHub
parent 715fa7a2f1
commit 23f5712d02
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 3314 additions and 159 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

61
build/lib/pgBackRestBuild/Config/BuildParse.pm
View File

@ -98,6 +98,11 @@ sub buildConfigDefineOptionTypeEnum
push @EXPORT, qw(buildConfigDefineOptionTypeEnum);
sub buildConfigCommandRoleEnum
{
return bldEnum('cfgCmdRole', shift);
}
####################################################################################################################################
# Helper functions for building optional option data
####################################################################################################################################
@ -258,6 +263,20 @@ sub buildConfigParse
" PARSE_RULE_COMMAND_PARAMETER_ALLOWED(true),\n";
}
$strBuildSource .=
"\n" .
" PARSE_RULE_COMMAND_ROLE_VALID_LIST\n" .
" (\n";
foreach my $strCommandRole (sort(keys(%{$rhCommand->{&CFGDEF_COMMAND_ROLE}})))
{
$strBuildSource .=
" PARSE_RULE_COMMAND_ROLE(" . buildConfigCommandRoleEnum($strCommandRole) . ")\n";
}
$strBuildSource .=
" ),\n";
$strBuildSource .=
" ),\n";
};
@ -336,27 +355,35 @@ sub buildConfigParse
" PARSE_RULE_OPTION_GROUP_ID(" . buildConfigOptionGroupEnum($rhOption->{&CFGDEF_GROUP}) . "),\n";
}
# Build command role default list
# --------------------------------------------------------------------------------------------------------------------------
# Build command role valid lists
#---------------------------------------------------------------------------------------------------------------------------
my $strBuildSourceSub = "";
foreach my $strCommand (cfgDefineCommandList())
foreach my $strCommandRole (CFGCMD_ROLE_DEFAULT, CFGCMD_ROLE_ASYNC, CFGCMD_ROLE_LOCAL, CFGCMD_ROLE_REMOTE)
{
if (defined($rhOption->{&CFGDEF_COMMAND}{$strCommand}))
{
$strBuildSourceSub .=
" PARSE_RULE_OPTION_COMMAND(" . buildConfigCommandEnum($strCommand) . ")\n";
}
}
$strBuildSourceSub = "";
if ($strBuildSourceSub ne "")
{
$strBuildSource .=
"\n" .
" PARSE_RULE_OPTION_COMMAND_LIST\n" .
" (\n" .
$strBuildSourceSub .
" ),\n";
foreach my $strCommand (cfgDefineCommandList())
{
if (defined($rhOption->{&CFGDEF_COMMAND}{$strCommand}))
{
if (defined($rhCommandDefine->{$strCommand}{&CFGDEF_COMMAND_ROLE}{$strCommandRole}))
{
$strBuildSourceSub .=
" PARSE_RULE_OPTION_COMMAND(" . buildConfigCommandEnum($strCommand) . ")\n";
}
}
}
if ($strBuildSourceSub ne "")
{
$strBuildSource .=
"\n" .
" PARSE_RULE_OPTION_COMMAND_ROLE_" . uc($strCommandRole) . "_VALID_LIST\n" .
" (\n" .
$strBuildSourceSub .
" ),\n";
}
}
# Render optional data and command overrides

92
build/lib/pgBackRestBuild/Config/Data.pm
View File

@ -15,6 +15,11 @@
# NOTE: If the option (A) has a dependency on another option (B) then the CFGCMD_ must also be specified in the other option
# (B), else it will still error on the option (A).
#
# CFGDEF_COMMAND_ROLE:
#
# Define the command roles that a command is valid for. CFGCMD_ROLE_DEFAULT is valid for all commands and is therefore added
# programmatically.
#
# CFGDEF_REQUIRED:
# In global section:
# true - if the option does not have a default, then setting CFGDEF_REQUIRED in the global section means all commands
@ -99,6 +104,18 @@ use constant CFGCMD_STOP => 'stop';
use constant CFGCMD_VERIFY => 'verify';
use constant CFGCMD_VERSION => 'version';
####################################################################################################################################
# Command role constants - roles allowed for each command
####################################################################################################################################
use constant CFGCMD_ROLE_DEFAULT => 'default';
push @EXPORT, qw(CFGCMD_ROLE_DEFAULT);
use constant CFGCMD_ROLE_ASYNC => 'async';
push @EXPORT, qw(CFGCMD_ROLE_ASYNC);
use constant CFGCMD_ROLE_LOCAL => 'local';
push @EXPORT, qw(CFGCMD_ROLE_LOCAL);
use constant CFGCMD_ROLE_REMOTE => 'remote';
push @EXPORT, qw(CFGCMD_ROLE_REMOTE);
####################################################################################################################################
# Option constants - options that are allowed for commands
####################################################################################################################################
@ -459,6 +476,8 @@ use constant CFGDEF_PREFIX => 'prefix';
push @EXPORT, qw(CFGDEF_PREFIX);
use constant CFGDEF_COMMAND => 'command';
push @EXPORT, qw(CFGDEF_COMMAND);
use constant CFGDEF_COMMAND_ROLE => 'command-role';
push @EXPORT, qw(CFGDEF_COMMAND_ROLE);
use constant CFGDEF_REQUIRED => 'required';
push @EXPORT, qw(CFGDEF_REQUIRED);
use constant CFGDEF_RESET => 'reset';
@ -506,6 +525,12 @@ my $rhCommandDefine =
&CFGDEF_LOG_FILE => false,
&CFGDEF_LOCK_TYPE => CFGDEF_LOCK_TYPE_ARCHIVE,
&CFGDEF_PARAMETER_ALLOWED => true,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_ASYNC => {},
&CFGCMD_ROLE_LOCAL => {},
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_ARCHIVE_PUSH =>
@ -514,6 +539,12 @@ my $rhCommandDefine =
&CFGDEF_LOCK_REMOTE_REQUIRED => true,
&CFGDEF_LOCK_TYPE => CFGDEF_LOCK_TYPE_ARCHIVE,
&CFGDEF_PARAMETER_ALLOWED => true,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_ASYNC => {},
&CFGCMD_ROLE_LOCAL => {},
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_BACKUP =>
@ -521,11 +552,20 @@ my $rhCommandDefine =
&CFGDEF_LOCK_REQUIRED => true,
&CFGDEF_LOCK_REMOTE_REQUIRED => true,
&CFGDEF_LOCK_TYPE => CFGDEF_LOCK_TYPE_BACKUP,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_LOCAL => {},
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_CHECK =>
{
&CFGDEF_LOG_FILE => false,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_EXPIRE =>
@ -545,12 +585,20 @@ my $rhCommandDefine =
{
&CFGDEF_LOG_FILE => false,
&CFGDEF_LOG_LEVEL_DEFAULT => DEBUG,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_REPO_CREATE =>
{
&CFGDEF_INTERNAL => true,
&CFGDEF_LOG_FILE => false,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_REPO_GET =>
@ -559,6 +607,10 @@ my $rhCommandDefine =
&CFGDEF_LOG_FILE => false,
&CFGDEF_LOG_LEVEL_DEFAULT => DEBUG,
&CFGDEF_PARAMETER_ALLOWED => true,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_REPO_LS =>
@ -567,6 +619,10 @@ my $rhCommandDefine =
&CFGDEF_LOG_FILE => false,
&CFGDEF_LOG_LEVEL_DEFAULT => DEBUG,
&CFGDEF_PARAMETER_ALLOWED => true,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_REPO_PUT =>
@ -575,6 +631,10 @@ my $rhCommandDefine =
&CFGDEF_LOG_FILE => false,
&CFGDEF_LOG_LEVEL_DEFAULT => DEBUG,
&CFGDEF_PARAMETER_ALLOWED => true,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_REPO_RM =>
@ -583,28 +643,49 @@ my $rhCommandDefine =
&CFGDEF_LOG_FILE => false,
&CFGDEF_LOG_LEVEL_DEFAULT => DEBUG,
&CFGDEF_PARAMETER_ALLOWED => true,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_RESTORE =>
{
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_LOCAL => {},
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_STANZA_CREATE =>
{
&CFGDEF_LOCK_REQUIRED => true,
&CFGDEF_LOCK_TYPE => CFGDEF_LOCK_TYPE_ALL,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_STANZA_DELETE =>
{
&CFGDEF_LOCK_REQUIRED => true,
&CFGDEF_LOCK_TYPE => CFGDEF_LOCK_TYPE_ALL,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_STANZA_UPGRADE =>
{
&CFGDEF_LOCK_REQUIRED => true,
&CFGDEF_LOCK_TYPE => CFGDEF_LOCK_TYPE_ALL,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_START =>
@ -618,6 +699,11 @@ my $rhCommandDefine =
&CFGCMD_VERIFY =>
{
&CFGDEF_INTERNAL => true,
&CFGDEF_COMMAND_ROLE =>
{
&CFGCMD_ROLE_LOCAL => {},
&CFGCMD_ROLE_REMOTE => {},
},
},
&CFGCMD_VERSION =>
@ -2894,6 +2980,12 @@ foreach my $strCommand (sort(keys(%{$rhCommandDefine})))
{
$rhCommandDefine->{$strCommand}{&CFGDEF_PARAMETER_ALLOWED} = false;
}
# All commands have the default role
if (!defined($rhCommandDefine->{$strCommand}{&CFGDEF_COMMAND_ROLE}{&CFGCMD_ROLE_DEFAULT}))
{
$rhCommandDefine->{$strCommand}{&CFGDEF_COMMAND_ROLE}{&CFGCMD_ROLE_DEFAULT} = {};
}
}
####################################################################################################################################

2
src/command/command.c
View File

@ -76,7 +76,7 @@ cmdOption(void)
// Skip the option if not valid for this command. Generally only one command runs at a time, but sometimes
// commands are chained together (e.g. backup and expire) and the second command may not use all the options of
// the first command. Displaying them is harmless but might cause confusion.
if (!cfgOptionValid(optionId) || !cfgParseOptionValid(cfgCommand(), optionId))
if (!cfgOptionValid(optionId) || !cfgParseOptionValid(cfgCommand(), cfgCommandRole(), optionId))
continue;
// Loop through option indexes

2
src/command/help/help.c
View File

@ -325,7 +325,7 @@ helpRender(void)
for (unsigned int optionId = 0; optionId < CFG_OPTION_TOTAL; optionId++)
{
if (cfgParseOptionValid(commandId, optionId) && !optionData[optionId].internal)
if (cfgParseOptionValid(commandId, cfgCmdRoleDefault, optionId) && !optionData[optionId].internal)
{
const String *section = optionData[optionId].section;

2
src/config/config.h
View File

@ -39,6 +39,8 @@ typedef enum
#define CONFIG_COMMAND_ROLE_LOCAL "local"
#define CONFIG_COMMAND_ROLE_REMOTE "remote"
#define CFG_COMMAND_ROLE_TOTAL 4
/***********************************************************************************************************************************
Constants

3
src/config/exec.c
View File

@ -35,7 +35,8 @@ cfgExecParam(ConfigCommand commandId, ConfigCommandRole commandRoleId, const Key
// Skip the option if it is not valid for the original/specified command or if is secure. Also skip repo1-cipher-type
// because there's no point of passing it if the other process doesn't have access to repo1-cipher-pass. There is
// probably a better way to do this last part...
if (!cfgParseOptionValid(commandId, optionId) || cfgParseOptionSecure(optionId) || optionId == cfgOptRepoCipherType)
if (!cfgParseOptionValid(commandId, commandRoleId, optionId) || cfgParseOptionSecure(optionId) ||
optionId == cfgOptRepoCipherType)
{
continue;
}

3251
src/config/parse.auto.c
View File

File diff suppressed because it is too large Load Diff

41
src/config/parse.c
View File

@ -89,6 +89,7 @@ Define how a command is parsed
typedef struct ParseRuleCommand
{
const char *name; // Name
unsigned int commandRoleValid:CFG_COMMAND_ROLE_TOTAL; // Valid for the command role?
bool parameterAllowed:1; // Command-line parameters are allowed
} ParseRuleCommand;
@ -99,6 +100,12 @@ typedef struct ParseRuleCommand
#define PARSE_RULE_COMMAND_NAME(nameParam) \
.name = nameParam
#define PARSE_RULE_COMMAND_ROLE_VALID_LIST(...) \
.commandRoleValid = 0 __VA_ARGS__
#define PARSE_RULE_COMMAND_ROLE(commandRoleParam) \
| (1 << commandRoleParam)
#define PARSE_RULE_COMMAND_PARAMETER_ALLOWED(parameterAllowedParam) \
.parameterAllowed = parameterAllowedParam
@ -130,12 +137,12 @@ typedef struct ParseRuleOption
bool multi:1; // Can be specified multiple times?
bool group:1; // In a group?
unsigned int groupId:1; // Id if in a group
uint64_t commandValid:CFG_COMMAND_TOTAL; // Valid for the command?
uint32_t commandRoleValid[CFG_COMMAND_ROLE_TOTAL]; // Valid for the command role?
const void **data; // Optional data and command overrides
} ParseRuleOption;
// Define additional types of data that can be associated with an option. Because these types are rare they are not give dedicated
// Define additional types of data that can be associated with an option. Because these types are rare they are not given dedicated
// fields and are instead packed into an array which is read at runtime. This may seem inefficient but they are only accessed a
// single time during parse so space efficiency is more important than performance.
typedef enum
@ -177,8 +184,17 @@ typedef enum
#define PARSE_RULE_OPTION_GROUP_ID(groupIdParam) \
.groupId = groupIdParam
#define PARSE_RULE_OPTION_COMMAND_LIST(...) \
.commandValid = 0 __VA_ARGS__
#define PARSE_RULE_OPTION_COMMAND_ROLE_DEFAULT_VALID_LIST(...) \
.commandRoleValid[cfgCmdRoleDefault] = 0 __VA_ARGS__
#define PARSE_RULE_OPTION_COMMAND_ROLE_ASYNC_VALID_LIST(...) \
.commandRoleValid[cfgCmdRoleAsync] = 0 __VA_ARGS__
#define PARSE_RULE_OPTION_COMMAND_ROLE_LOCAL_VALID_LIST(...) \
.commandRoleValid[cfgCmdRoleLocal] = 0 __VA_ARGS__
#define PARSE_RULE_OPTION_COMMAND_ROLE_REMOTE_VALID_LIST(...) \
.commandRoleValid[cfgCmdRoleRemote] = 0 __VA_ARGS__
#define PARSE_RULE_OPTION_COMMAND(commandParam) \
| (1 << commandParam)
@ -552,17 +568,18 @@ cfgParseOptionType(ConfigOption optionId)
/**********************************************************************************************************************************/
bool
cfgParseOptionValid(ConfigCommand commandId, ConfigOption optionId)
cfgParseOptionValid(ConfigCommand commandId, ConfigCommandRole commandRoleId, ConfigOption optionId)
{
FUNCTION_TEST_BEGIN();
FUNCTION_TEST_PARAM(ENUM, commandId);
FUNCTION_TEST_PARAM(ENUM, commandRoleId);
FUNCTION_TEST_PARAM(ENUM, optionId);
FUNCTION_TEST_END();
ASSERT(commandId < CFG_COMMAND_TOTAL);
ASSERT(optionId < CFG_OPTION_TOTAL);
FUNCTION_TEST_RETURN(parseRuleOption[optionId].commandValid & (1 << commandId));
FUNCTION_TEST_RETURN(parseRuleOption[optionId].commandRoleValid[commandRoleId] & ((uint32_t)1 << commandId));
}
/***********************************************************************************************************************************
@ -951,6 +968,10 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
if (config->command == cfgCmdNone)
THROW_FMT(CommandInvalidError, "invalid command '%s'", command);
// Error when role is not valid for the command
if (!(parseRuleCommand[config->command].commandRoleValid & ((unsigned int)1 << config->commandRole)))
THROW_FMT(CommandInvalidError, "invalid command/role combination '%s'", command);
if (config->command == cfgCmdHelp)
config->help = true;
else
@ -1155,7 +1176,7 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
}
// Continue if the option is not valid for this command
if (!cfgParseOptionValid(config->command, option.id))
if (!cfgParseOptionValid(config->command, config->commandRole, option.id))
continue;
if (strSize(value) == 0)
@ -1276,7 +1297,7 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
kvPut(optionFound, optionFoundKey, VARSTR(key));
// Continue if the option is not valid for this command
if (!cfgParseOptionValid(config->command, option.id))
if (!cfgParseOptionValid(config->command, config->commandRole, option.id))
{
// Warn if it is in a command section
if (sectionIdx % 2 == 0)
@ -1363,7 +1384,7 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
config->option[optionId].name = parseRuleOption[optionId].name;
// Is the option valid for this command?
if (cfgParseOptionValid(config->command, optionId))
if (cfgParseOptionValid(config->command, config->commandRole, optionId))
{
config->option[optionId].valid = true;
config->option[optionId].group = parseRuleOption[optionId].group;
@ -1502,6 +1523,8 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
ConfigOption dependOptionId = (ConfigOption)depend.data;
ConfigOptionType dependOptionType = cfgParseOptionType(dependOptionId);
ASSERT(config->option[dependOptionId].index != NULL);
// Get the depend option value
const Variant *dependValue = config->option[dependOptionId].index[optionListIdx].value;

2
src/config/parse.h
View File

@ -48,6 +48,6 @@ ConfigOptionType cfgParseOptionType(ConfigOption optionId);
bool cfgParseOptionRequired(ConfigCommand commandId, ConfigOption optionId);
// Is the option valid for the command?
bool cfgParseOptionValid(ConfigCommand commandId, ConfigOption optionId);
bool cfgParseOptionValid(ConfigCommand commandId, ConfigCommandRole commandRoleId, ConfigOption optionId);
#endif

4
test/src/common/harnessConfig.c
View File

@ -58,11 +58,11 @@ harnessCfgLoadRole(ConfigCommand commandId, ConfigCommandRole commandRoleId, con
StringList *argList = strLstDup(argListParam);
// Set log path if valid
if (cfgParseOptionValid(commandId, cfgOptLogPath))
if (cfgParseOptionValid(commandId, commandRoleId, cfgOptLogPath))
strLstInsert(argList, 0, strNewFmt("--" CFGOPT_LOG_PATH "=%s", testDataPath()));
// Set lock path if valid
if (cfgParseOptionValid(commandId, cfgOptLockPath))
if (cfgParseOptionValid(commandId, commandRoleId, cfgOptLockPath))
strLstInsert(argList, 0, strNewFmt("--" CFGOPT_LOCK_PATH "=%s/lock", testDataPath()));
// Insert the command so it does not interfere with parameters

13
test/src/module/config/parseTest.c
View File

@ -43,7 +43,7 @@ testRun(void)
{
TEST_TITLE("check size of parse structures");
TEST_RESULT_UINT(sizeof(ParseRuleOption), TEST_64BIT() ? 24 : 12, "ParseRuleOption size");
TEST_RESULT_UINT(sizeof(ParseRuleOption), TEST_64BIT() ? 40 : 28, "ParseRuleOption size");
}
// Config functions that are not tested with parse
@ -575,6 +575,17 @@ testRun(void)
strLstAdd(argList, strNew(BOGUS_STR));
TEST_ERROR(configParse(strLstSize(argList), strLstPtr(argList), false), CommandInvalidError, "invalid command 'BOGUS'");
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("invalid command/role combination");
argList = strLstNew();
strLstAddZ(argList, TEST_BACKREST_EXE);
strLstAddZ(argList, CFGCMD_BACKUP ":" CONFIG_COMMAND_ROLE_ASYNC);
TEST_ERROR(
configParse(strLstSize(argList), strLstPtr(argList), false), CommandInvalidError,
"invalid command/role combination 'backup:async'");
// -------------------------------------------------------------------------------------------------------------------------
argList = strLstNew();
strLstAdd(argList, strNew(TEST_BACKREST_EXE));