[pacman-dev] Implement SigLevel config parsing, v2.0
These revised patches make some fixes based on Dave's suggestions and also fix some problems I noticed after submitting the original patches. I don't think Optional is actually the default if no SigLevel is specified, but it's probably a good default going forward. -Kerrick Staley
The ALPM_SIG_PACKAGE_HASH_OK field indicates that secure hashes are to
be acceptable as signatures.
Also, having ALPM_SIG_USE_DEFAULT = (1 << 31) will cause alpm_siglevel_t
to be treated as an unsigned value on machines where int is 32 bits,
meaning negative numbers cannot be returned on error conditions. So,
make it (1 << 30). Still, there's no guarantee that signed
semantics will be used unless one of the enum values is negative. So,
supply ALPM_SIG_ERROR = -1 as a sort of dummy value (it can be used in
code, but "-1" is probably just as meaningful and is more consistent
with conventions used throughout the rest of the library).
Signed-off-by: Kerrick Staley
And...
I didn't actually hit save, so this is missing the ALPM_SIG_ERROR
part. Here's the fixed version.
Revise siglevel_t, adding PACKAGE_HASH_OK field
The ALPM_SIG_PACKAGE_HASH_OK field indicates that secure hashes are to
be acceptable as signatures.
Also, having ALPM_SIG_USE_DEFAULT = (1 << 31) will cause alpm_siglevel_t
to be treated as an unsigned value on machines where int is 32 bits,
meaning negative numbers cannot be returned on error conditions. So,
make it (1 << 30). Still, there's no guarantee that signed
semantics will be used unless one of the enum values is negative. So,
supply ALPM_SIG_ERROR = -1 as a sort of dummy value (it can be used in
code, but "-1" is probably just as meaningful and is more consistent
with conventions used throughout the rest of the library).
Signed-off-by: Kerrick Staley
On 18/07/11 16:59, Kerrick Staley wrote:
And... I didn't actually hit save, so this is missing the ALPM_SIG_ERROR part. Here's the fixed version.
Revise siglevel_t, adding PACKAGE_HASH_OK field
The ALPM_SIG_PACKAGE_HASH_OK field indicates that secure hashes are to be acceptable as signatures.
I do not understand how is this a useful option. There is always a hash in the repo database assuming it is created using repo-add (md5sum gets used as a download check, and sha256sums are there but do nothing). So this is the same as setting signature checking as "Optional" or "None". Also, is md5sum is a secure hash? Allan
The SigLevel config option replaces the VerifySig option, and has
similar semantics, but adds a set of advanced configuration options that
correspond to the recently introduced alpm_siglevel_t fields.
Signed-off-by: Kerrick Staley
On 18/07/11 16:35, Kerrick Staley wrote:
The SigLevel config option replaces the VerifySig option, and has similar semantics, but adds a set of advanced configuration options that correspond to the recently introduced alpm_siglevel_t fields.
Signed-off-by: Kerrick Staley
--- doc/pacman.conf.5.txt | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-) diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt index a28e00f..19cd6e3 100644 --- a/doc/pacman.conf.5.txt +++ b/doc/pacman.conf.5.txt @@ -156,6 +156,26 @@ Options packages are only cleaned if not installed locally and not present in any known sync database.
+*SigLevel =* ...::
I was fairly sure previous the discussion indicated we would go with CheckLevel for the name of this. Anyway, I think it would be good to get a complete list of options sorted before documenting and implementing... Currently it seems incomplete. Global options: Required (or Always?), Optional, Never - what about global controls for allowing marginal and unknown signatures. Also, we need to determine exactly how the suboptions will work. Do the PackageFoo options override global options? E.g. are these all equivalent (require database signatures but not package signatures)? CheckLevel = Required PackageOptional CheckLevel = Optional DatabaseRequired CheckLevel = DatabaseRequired PackageOptional Allan
Required, Optional, and Never aren't to be mixed with the other settings.
The original version said this, but I thought the whole thing was getting
long winded, so I reworded it in a way that I thought would make this
implicit (guess not :).
Required, Optional, and Never will be sufficient for like 90% of users
(which is probably why they were the only options until recently), so I
separated them out from the more "advanced" settings (many people won't [and
shouldn't have to] know what e.g. marginally trusted signatures are).
-Kerrick Staley
On Jul 18, 2011 2:55 AM, "Allan McRae"
On 18/07/11 16:35, Kerrick Staley wrote:
The SigLevel config option replaces the VerifySig option, and has similar semantics, but adds a set of advanced configuration options that correspond to the recently introduced alpm_siglevel_t fields.
Signed-off-by: Kerrick Staley
--- doc/pacman.conf.5.txt | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-) diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt index a28e00f..19cd6e3 100644 --- a/doc/pacman.conf.5.txt +++ b/doc/pacman.conf.5.txt @@ -156,6 +156,26 @@ Options packages are only cleaned if not installed locally and not present in any known sync database.
+*SigLevel =* ...::
I was fairly sure previous the discussion indicated we would go with CheckLevel for the name of this.
Anyway, I think it would be good to get a complete list of options sorted before documenting and implementing... Currently it seems incomplete.
Global options: Required (or Always?), Optional, Never - what about global controls for allowing marginal and unknown signatures.
Also, we need to determine exactly how the suboptions will work. Do the PackageFoo options override global options? E.g. are these all equivalent (require database signatures but not package signatures)?
CheckLevel = Required PackageOptional CheckLevel = Optional DatabaseRequired CheckLevel = DatabaseRequired PackageOptional
Allan
Add code to conf.c that parses the new SigLevel directive.
Signed-off-by: Kerrick Staley
On Mon, Jul 18, 2011 at 1:35 AM, Kerrick Staley
Add code to conf.c that parses the new SigLevel directive.
Signed-off-by: Kerrick Staley
--- src/pacman/conf.c | 97 +++++++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 80 insertions(+), 17 deletions(-) diff --git a/src/pacman/conf.c b/src/pacman/conf.c index fac6da3..de5b463 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -222,20 +222,83 @@ int config_set_arch(const char *arch) return 0; }
-static alpm_siglevel_t option_verifysig(const char *value) +/** + * Parse a signature verification level. Destroys the passed string. + * @param str the string listing the siglevel options + * @return -1 if value was empty, -2 if value contained unknown Shouldn't 0 be empty? Considering this method wouldn't even be called if you never had a LHS option key of SigLevel.
+ * options, -3 if some of the options were contradictory, the siglevel + * value otherwise. + */ +static alpm_siglevel_t option_siglevel(char *str) { - alpm_siglevel_t level; - if(strcmp(value, "Always") == 0) { - level = ALPM_SIG_PACKAGE | ALPM_SIG_DATABASE; - } else if(strcmp(value, "Optional") == 0) { - level = ALPM_SIG_PACKAGE | ALPM_SIG_PACKAGE_OPTIONAL | - ALPM_SIG_DATABASE | ALPM_SIG_DATABASE_OPTIONAL; - } else if(strcmp(value, "Never") == 0) { - level = 0; - } else { + alpm_siglevel_t level = 0; + + char *strtok_state; + char *tok = strtok_r(str, " \t", &strtok_state); If we're taking tabs here, then we should take them everywhere, and it should be done in a separate patch. See the strchr() call in setrepeatingoption().
I'd rather not reinvent the tokenization and parsing wheel here. Please use setrepeatingoption() and refactor this method to be more like process_cleanmethods().
+ + if (!tok) { return -1; } - pm_printf(ALPM_LOG_DEBUG, "config: VerifySig = %s (%d)\n", value, level); + + do { + if(strcmp(tok, "Never") == 0) { + level = level ? -3 : 0; + break; Why in the heck are you breaking out of the loop? This means SigLevel = Never asdlfs;aiophgewioyh432i89 fkhg qiew5 o089a wl; is now valid, no?
+ } else if(strcmp(tok, "Optional") == 0) { + level = level ? -3 : ALPM_SIG_PACKAGE | ALPM_SIG_PACKAGE_OPTIONAL | + ALPM_SIG_DATABASE | ALPM_SIG_DATABASE_OPTIONAL; + break; + } else if(strcmp(tok, "Required") == 0) { + level = level ? -3 : ALPM_SIG_PACKAGE | ALPM_SIG_DATABASE; + break; I disagree with every one of these breaks. Always finish parsing what you start.
+ } else if(strcmp(tok, "PackageHash") == 0) { + if (level & ALPM_SIG_DATABASE_OPTIONAL) { + level = -3; + break; + } + level |= ALPM_SIG_PACKAGE_HASH_OK; Kill this block, out of scope here.
+ } else if(strcmp(tok, "PackageRequired") == 0) { + if (level & ALPM_SIG_PACKAGE_OPTIONAL) { + level = -3; + break; Also disagreeing with these breaks. If we are going to have the non-package and non-database options, allowing one to override makes perfect sense to me and should not be rejected. SigLevel = Always PackageOptional should work fine. I see it is getting rejected below. You realize PACKAGE and PACKAGE_OPTIONAL are not mutually exclusive, right?
+ } + level |= ALPM_SIG_PACKAGE; + } else if(strcmp(tok, "DatabaseRequired") == 0) { + if (level & ALPM_SIG_DATABASE_OPTIONAL) { + level = -3; + break; + } + level |= ALPM_SIG_DATABASE; + } else if(strcmp(tok, "PackageOptional") == 0) { + if (level & ALPM_SIG_PACKAGE) { + level = -3; + break; + } + level |= ALPM_SIG_PACKAGE_OPTIONAL; + } else if(strcmp(tok, "DatabaseOptional") == 0) { + if (level & ALPM_SIG_PACKAGE) { + level = -3; + break; + } + level |= ALPM_SIG_DATABASE_OPTIONAL; + } else if(strcmp(tok, "PackageAllowMarginal") == 0) { + level |= ALPM_SIG_PACKAGE_MARGINAL_OK; + } else if(strcmp(tok, "DatabaseAllowMarginal") == 0) { + level |= ALPM_SIG_DATABASE_MARGINAL_OK; + } else if(strcmp(tok, "PackageAllowUnknown") == 0) { + level |= ALPM_SIG_PACKAGE_UNKNOWN_OK; + } else if(strcmp(tok, "DatabaseAllowUnknown") == 0) { + level |= ALPM_SIG_DATABASE_UNKNOWN_OK; + } else { + level = -2; + break; + } + } while ((tok = strtok_r(NULL, " \t", &strtok_state))); + + if (level >= 0) { + pm_printf(ALPM_LOG_DEBUG, "config: SigLevel = %s (%d)\n", str, level); + } No need to restrict this printout as it is a debug message.
+ return level; }
@@ -359,9 +422,9 @@ static int _parse_options(const char *key, char *value, return 1; } FREELIST(methods); - } else if(strcmp(key, "VerifySig") == 0) { - alpm_siglevel_t level = option_verifysig(value); - if(level != -1) { + } else if(strcmp(key, "SigLevel") == 0) { + alpm_siglevel_t level = option_siglevel(value); + if(level >= 0) { config->siglevel = level; } else { pm_printf(ALPM_LOG_ERROR, This error message won't work at all as you just finished trashing the contents of value.
@@ -726,9 +789,9 @@ static int _parseconfig(const char *file, struct section_t *section, goto cleanup; } section->servers = alpm_list_add(section->servers, strdup(value)); - } else if(strcmp(key, "VerifySig") == 0) { - alpm_siglevel_t level = option_verifysig(value); - if(level != -1) { + } else if(strcmp(key, "SigLevel") == 0) { + alpm_siglevel_t level = option_siglevel(value); + if(level >= 0) { This now rejects a 'VerifySig = ' line, which perhaps is odd, but I'm not sure is correct (and I don't think we did it before?). One would assume this would use the default value or something.
section->siglevel = level; } else { pm_printf(ALPM_LOG_ERROR, -- 1.7.6
Add code to conf.c that parses the new SigLevel directive. An
overwhelming number of options are presented, but most users will still
be fine with the Never/Optional/Required trio. More advanced users can
combine these or any of the other options on a 'SigLevel = ' line, which
is parsed in a left-to-right fashion and flags turned on and off
accordingly. For example, all three of these will net the same config:
SigLevel = Required PackageOptional
SigLevel = Optional DatabaseRequired
SigLevel = DatabaseRequired PackageOptional
Additionally, database-specific lines assume you wish to start with any
global default that has been set. For example, if any of the above lines
were in the [options] section, something such as:
SigLevel = PackageRequired PackageAllowMarginal
Would continue to enforce required database signatures.
Inspiration-by: Kerrick Staley
On 19/07/11 11:06, Dan McGee wrote:
Add code to conf.c that parses the new SigLevel directive. An overwhelming number of options are presented, but most users will still be fine with the Never/Optional/Required trio. More advanced users can combine these or any of the other options on a 'SigLevel = ' line, which is parsed in a left-to-right fashion and flags turned on and off accordingly. For example, all three of these will net the same config:
SigLevel = Required PackageOptional SigLevel = Optional DatabaseRequired SigLevel = DatabaseRequired PackageOptional
Additionally, database-specific lines assume you wish to start with any global default that has been set. For example, if any of the above lines were in the [options] section, something such as:
SigLevel = PackageRequired PackageAllowMarginal
Would continue to enforce required database signatures.
Inspiration-by: Kerrick Staley
Signed-off-by: Dan McGee ---
Signed-off-by: Allan Tested with the following in my pacman.conf [options] SigLevel = Optional (also with this commented out) [allanbrokeit] SigLevel = Required [kernel64] SigLevel = DatabaseRequired (Arch repos without SigLevel specified) Everything seems to work as expected. The [allanbrokeit] repo gives all sorts of failures without signatures and the [kernel64] repo failed when there was no repo signature but is happy about the lack of package signatures within the repo. Allan
Dan, Thanks for the feedback, especially regarding the code layout (your version is a lot cleaner) and the attempt to print the mutilated string. I do think that there are too many directives in your version: it'd be better to have a few directives that address the most likely cases, and then just provide one directive for each internal flag to allow advanced configuration. This also makes for a conciser description in the manpage. For the signed vs. unsigned issue, one solution that would work just as well (I think) would be to typedef int alpm_siglevel_t; and then use #defines for ALPM_SIG_* and have process_siglevel() return negative values on error conditions like before (ALPM_SIG_USE_DEFAULT would have to become 1 << 30). It kills the type safety though. Just an idea: what do you think? -Kerrick Staley
participants (4)
-
Allan McRae
-
Dan McGee
-
Dan McGee
-
Kerrick Staley