[Rpm-maint] [PATCH 4/4] Sign package files during installation
Panu Matilainen
pmatilai at laiskiainen.org
Fri Oct 31 09:30:47 UTC 2014
On 10/07/2014 11:19 PM, fin at linux.vnet.ibm.com wrote:
> From: Fionnuala Gunter <fin at linux.vnet.ibm.com>
>
> It will take some time for distros to adopt the file signing process and
> distribute packages with file signatures, so this patch extends the rpm
> installer to support inline file signing. This patch adds a new option,
> signfiles, to the rpm installer.
>
> rpm -ivh [--signfiles [--fskpath <file signing key>]] PACKAGE_FILE ...
>
> Signed-off-by: Fionnuala Gunter <fin at linux.vnet.ibm.com>
> ---
> doc/rpm.8 | 28 +++++++++++++++++++---------
> lib/fsm.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
> lib/poptI.c | 7 +++++++
> lib/rpmcli.h | 2 ++
> lib/rpminstall.c | 10 +++++++++-
> lib/rpmts.c | 15 +++++++++++++++
> lib/rpmts.h | 15 +++++++++++++++
> lib/rpmts_internal.h | 2 ++
> 8 files changed, 114 insertions(+), 15 deletions(-)
>
> diff --git a/doc/rpm.8 b/doc/rpm.8
> index e583009..4079f71 100644
> --- a/doc/rpm.8
> +++ b/doc/rpm.8
> @@ -84,15 +84,14 @@ rpm \- RPM Package Manager
>
>
> [\fB--allfiles\fR] [\fB--badreloc\fR] [\fB--excludepath \fIOLDPATH\fB\fR]
> - [\fB--excludedocs\fR] [\fB--force\fR] [\fB-h,--hash\fR]
> - [\fB--ignoresize\fR] [\fB--ignorearch\fR] [\fB--ignoreos\fR]
> - [\fB--includedocs\fR] [\fB--justdb\fR] [\fB--nocollections\fR]
> - [\fB--nodeps\fR] [\fB--nodigest\fR] [\fB--nosignature\fR]
> - [\fB--noorder\fR] [\fB--noscripts\fR] [\fB--notriggers\fR]
> - [\fB--oldpackage\fR] [\fB--percent\fR] [\fB--prefix \fINEWPATH\fB\fR]
> - [\fB--relocate \fIOLDPATH\fB=\fINEWPATH\fB\fR]
> - [\fB--replacefiles\fR] [\fB--replacepkgs\fR]
> - [\fB--test\fR]
> + [\fB--excludedocs\fR] [\fB--force\fR] [\fB--fskpath \fIKEY\fB\fR]
> + [\fB-h,--hash\fR] [\fB--ignoresize\fR] [\fB--ignorearch\fR]
> + [\fB--ignoreos\fR] [\fB--includedocs\fR] [\fB--justdb\fR]
> + [\fB--nocollections\fR] [\fB--nodeps\fR] [\fB--nodigest\fR]
> + [\fB--nosignature\fR] [\fB--noorder\fR] [\fB--noscripts\fR]
> + [\fB--notriggers\fR] [\fB--oldpackage\fR] [\fB--percent\fR]
> + [\fB--prefix \fINEWPATH\fB\fR] [\fB--relocate \fIOLDPATH\fB=\fINEWPATH\fB\fR]
> + [\fB--replacefiles\fR] [\fB--replacepkgs\fR] [\fB--signfiles] [\fB--test\fR]
Uh, its pretty hard to see what changed in that. If you need to split a
line or two to make room then by all means do, rearranging every single
line to add two new options is way excessive, simply because the actual
change is completely lost in the noise.
The best way to do such "white space" changes is to rearrange in a
separate patch without doing any real changes (and say so in the commit
message), and then add the new stuff in a second patch, so its then
obvious what gets added.
>
> .SH "DESCRIPTION"
> .PP
> @@ -232,6 +231,9 @@ Don't install files whose name begins with
> Don't install any files which are marked as documentation
> (which includes man pages and texinfo documents).
> .TP
> +\fB--fskpath \fIKEY\fB\fR
> +Used with \fB--signfiles\fR, use file signing key \fIKEY\fR.
> +.TP
> \fB--force\fR
> Same as using
> \fB--replacepkgs\fR,
> @@ -362,6 +364,13 @@ already installed, packages.
> Install the packages even if some of them are already installed
> on this system.
> .TP
> +\fB--signfiles\fR
> +Sign package files. The macro \fB%_binary_filedigest_algorithm\fR must be set
> +before building the package, and the macro must be set to a supported algorithm:
> +2, 8, 9, or 10, which represent SHA1, SHA256, SHA384, and SHA512, respectively.
> +The file signing key (RSA private key) can be configured on the command line
> +with \fB--fskpath\fR or the macro \fB%_file_signing_key\fR.
> +.TP
> \fB--test\fR
> Do not install the package, simply check for and report
> potential conflicts.
> @@ -875,4 +884,5 @@ what's available.
> Marc Ewing <marc at redhat.com>
> Jeff Johnson <jbj at redhat.com>
> Erik Troan <ewt at redhat.com>
> +Fionnuala Gunter <fin at linux.vnet.ibm.com>
> .fi
> diff --git a/lib/fsm.c b/lib/fsm.c
> index dbeeaab..05ea230 100644
> --- a/lib/fsm.c
> +++ b/lib/fsm.c
> @@ -21,6 +21,7 @@
> #include "lib/rpmplugins.h" /* rpm plugins hooks */
> #include "lib/rpmug.h"
> #include "lib/rpmlib.h"
> +#include "lib/rpmsignfiles.h" /* getDigestAlgo, getDigestLength, signFile */
>
> #include "debug.h"
>
> @@ -825,8 +826,13 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
> const char *suffix;
> char *fpath = NULL;
> Header h = rpmteHeader(te);
> - struct rpmtd_s sigs;
> - char *sig = NULL;
> + struct rpmtd_s digests, sigs;
> + int signFiles = rpmtsSignFiles(ts);
> + const char *key;
> + const char *algo;
> + const char *digest;
> + const char *sig;
> + int diglen = 0;
>
> if (fi == NULL) {
> rc = RPMERR_BAD_MAGIC;
> @@ -838,7 +844,30 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
> goto exit;
> }
>
> - headerGet(h, RPMTAG_FILESIGNATURES, &sigs, HEADERGET_MINMEM);
> + if (signFiles) {
> + algo = getDigestAlgo(h);
> + if (!algo) {
> + rc = RPMRC_FAIL;
> + goto exit;
> + }
> +
> + diglen = getDigestLength(h);
> + if (diglen < 0) {
> + rc = RPMRC_FAIL;
> + goto exit;
> + }
> +
> + key = rpmExpand("%{_file_signing_key}", NULL);
> + if (rstreq(key, "")) {
> + rc = RPMRC_FAIL;
> + fprintf(stderr, _("You must set \"$$_file_signing_key\" in your macro file or on the command line with --fskpath\n"));
> + rpmlog(RPMLOG_ERR, _("no file signing key provided\n"));
> + }
You cannot assume rpm command line is being used for installing. These
days its much more likely that is NOT the case on average package install.
Also this is WAY too late for such a sanity check, such a check should
be done while setting up the transaction already.
> +
> + headerGet(h, RPMTAG_FILEDIGESTS, &digests, HEADERGET_MINMEM);
> + } else {
RPMTAG_FILEDIGESTS is available through the rpmfi, dont bother with the
header.
> + headerGet(h, RPMTAG_FILESIGNATURES, &sigs, HEADERGET_MINMEM);
> + }
...and this should be as well (see my comments on the first patch),
unless it can be handled internally to the plugin.
>
> /* transaction id used for temporary path suffix while installing */
> rasprintf(&tid, ";%08x", (unsigned)rpmtsGetTid(ts));
> @@ -964,12 +993,22 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
> if (rc)
> *failedFile = xstrdup(fpath);
>
> - /* get file signatures from header */
> - if (sb.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH)) {
> + /* sign executable files */
> + if (sb.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH) && signFiles) {
> + digest = rpmtdNextString(&digests);
> + sig = signFile(algo, digest, diglen, key);
> + if (!sig) {
> + rpmlog(RPMLOG_ERR, _("signFile failed\n"));
> + goto exit;
> + }
Again this is a problematic place to fail, it'd be better to do this
early if at all possible. If this signing only needs file digests then
that is available in early transaction setup already.
The other problem with adding stuff to the headers in mid-transaction is
that its not hard to exceed the maximum allowed header size in doing so,
which will then cause the install to fail. This issue is not specific to
file signatures and needs fixing (by increasing the header limits)
anyway though.
The bigger problem yet is AFAICS this adds the signatures one by one in
the order they happen to be encountered in the payload. The order of
files in payload and the header does differ, so the paths vs signatures
arrays are now out of order. Not to mention cases where files are
getting skipped for whatever reason so the arrays end up being of
different size too. Or perhaps I missed something.
> + }
> + /* or get file signatures from header */
> + else if (sb.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH)) {
> sig = rpmtdNextString(&sigs);
> } else {
> sig = NULL;
> rpmtdNextString(&sigs);
> + rpmtdNextString(&digests);
> }
>
> /* Run fsm file post hook for all plugins */
> @@ -984,6 +1023,7 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
> exit:
>
> /* No need to bother with close errors on read */
> + rpmtdFreeData(&digests);
> rpmtdFreeData(&sigs);
> headerFree(h);
> rpmfiArchiveClose(fi);
...and finally, why does this need to be in librpm when there is a
plugin for this stuff being added?
> diff --git a/lib/poptI.c b/lib/poptI.c
> index e21cde1..699c8cd 100644
> --- a/lib/poptI.c
> +++ b/lib/poptI.c
> @@ -16,8 +16,10 @@ struct rpmInstallArguments_s rpmIArgs = {
> 0, /* numRelocations */
> 0, /* noDeps */
> 0, /* incldocs */
> + 0, /* signFiles */
> NULL, /* relocations */
> NULL, /* prefix */
> + NULL, /* fileSigningKey */
> };
>
> #define POPT_RELOCATE -1021
> @@ -146,6 +148,9 @@ struct poptOption rpmInstallPoptTable[] = {
> (INSTALL_UPGRADE|INSTALL_FRESHEN|INSTALL_INSTALL),
> N_("upgrade package(s) if already installed"),
> N_("<packagefile>+") },
> + { "fskpath", '\0', POPT_ARG_STRING, &rpmIArgs.fileSigningKey, 0,
> + N_("use file signing key <key>"),
> + N_("<key>") },
> { "hash", 'h', POPT_BIT_SET, &rpmIArgs.installInterfaceFlags, INSTALL_HASH,
> N_("print hash marks as package installs (good with -v)"), NULL},
> { "ignorearch", '\0', POPT_BIT_SET,
> @@ -243,6 +248,8 @@ struct poptOption rpmInstallPoptTable[] = {
> { "replacepkgs", '\0', POPT_BIT_SET,
> &rpmIArgs.probFilter, RPMPROB_FILTER_REPLACEPKG,
> N_("reinstall if the package is already present"), NULL},
> + { "signfiles", '\0', POPT_ARG_NONE, &rpmIArgs.signFiles, 0,
> + N_("sign package files"), NULL},
> { "test", '\0', POPT_BIT_SET, &rpmIArgs.transFlags, RPMTRANS_FLAG_TEST,
> N_("don't install, but tell if it would work or not"), NULL},
> { "upgrade", 'U', POPT_BIT_SET,
> diff --git a/lib/rpmcli.h b/lib/rpmcli.h
> index 48e8250..ff89171 100644
> --- a/lib/rpmcli.h
> +++ b/lib/rpmcli.h
> @@ -339,8 +339,10 @@ struct rpmInstallArguments_s {
> int numRelocations;
> int noDeps;
> int incldocs;
> + int signFiles;
> rpmRelocation * relocations;
> char * prefix;
> + char * fileSigningKey;
> };
>
> /** \ingroup rpmcli
> diff --git a/lib/rpminstall.c b/lib/rpminstall.c
> index 2e7da7d..d98d506 100644
> --- a/lib/rpminstall.c
> +++ b/lib/rpminstall.c
> @@ -11,6 +11,7 @@
> #include <rpm/rpmds.h>
> #include <rpm/rpmts.h>
> #include <rpm/rpmlog.h>
> +#include <rpm/rpmmacro.h>
> #include <rpm/rpmfileutil.h>
>
> #include "lib/rpmgi.h"
> @@ -417,7 +418,14 @@ int rpmInstall(rpmts ts, struct rpmInstallArguments_s * ia, ARGV_t fileArgv)
>
> relocations = ia->relocations;
>
> - setNotifyFlag(ia, ts);
> + setNotifyFlag(ia, ts);
> +
> + rpmtsSetSignFiles(ts, ia->signFiles);
> +
> + if (ia->fileSigningKey) {
> + addMacro(NULL, "_file_signing_key", NULL, ia->fileSigningKey,
> + RMIL_GLOBAL);
> + }
>
> if ((eiu->relocations = relocations) != NULL) {
> while (eiu->relocations->oldPath)
> diff --git a/lib/rpmts.c b/lib/rpmts.c
> index a3b4ed2..6d9eb30 100644
> --- a/lib/rpmts.c
> +++ b/lib/rpmts.c
> @@ -897,6 +897,21 @@ int rpmtsSetNotifyCallback(rpmts ts,
> return 0;
> }
>
> +int rpmtsSignFiles(rpmts ts)
> +{
> + return ts ? ts->signFiles : NULL;
> +}
The return type is int, returning NULL is not correct.
> +
> +int rpmtsSetSignFiles(rpmts ts, int signFiles)
> +{
> + if (ts == NULL) {
> + return -1;
> + }
> +
> + ts->signFiles = signFiles;
> + return 0;
> +}
> +
> tsMembers rpmtsMembers(rpmts ts)
> {
> return (ts != NULL) ? ts->members : NULL;
> diff --git a/lib/rpmts.h b/lib/rpmts.h
> index 5231c80..5f45972 100644
> --- a/lib/rpmts.h
> +++ b/lib/rpmts.h
> @@ -393,6 +393,21 @@ const char * rpmtsRootDir(rpmts ts);
> */
> int rpmtsSetRootDir(rpmts ts, const char * rootDir);
>
> +/**
> + * Get transaction sign files flag
> + * @param ts transaction set
> + * @return non-zero if package files need to be signed
> + */
> +int rpmtsSignFiles(rpmts ts);
> +
> +/**
> + * Set transaction sign files flag
> + * @param ts transaction set
> + * @param signFiles new sign files flag
> + * @return 0 on success, -1 on error
> + */
> +int rpmtsSetSignFiles(rpmts ts, int signFiles);
> +
> /** \ingroup rpmts
> * Get transaction script file handle, i.e. stdout/stderr on scriptlet execution
> * @param ts transaction set
> diff --git a/lib/rpmts_internal.h b/lib/rpmts_internal.h
> index 0caa7cb..a196932 100644
> --- a/lib/rpmts_internal.h
> +++ b/lib/rpmts_internal.h
> @@ -68,6 +68,8 @@ struct rpmts_s {
> rpmPlugins plugins; /*!< Transaction plugins */
>
> int nrefs; /*!< Reference count. */
> +
> + int signFiles; /*!< Sign package files. */
> };
>
> #ifdef __cplusplus
>
Here too, all sorts of API additions for something that is supposed to
be mostly a plugin, I guess. I think this whole thing calls for a bit of
rethinking: whether it should be a plugin at all, and if so, how much of
it is possible to put into a plugin without changing librpm all over the
place.
- Panu -
More information about the Rpm-maint
mailing list