[Rpm-maint] [RFC PATCH v3 2/4] Sign package files and include signatures in package header
Fionnuala Gunter
fin at linux.vnet.ibm.com
Sat Nov 8 21:13:56 UTC 2014
On 10/31/2014 03:45 AM, Panu Matilainen wrote:
> On 10/07/2014 11:19 PM, fin at linux.vnet.ibm.com wrote:
>> From: Fionnuala Gunter <fin at linux.vnet.ibm.com>
>>
>> This patch extends the existing rpm signing tool to also sign package
>> files
>> and include them in the package header. It defines a tag,
>> RPMTAG_FILESIGNATURES, an RPM macro %_file_signing_key, and new options
>> fskpath, and signfiles.
>>
>> rpm --addsign [--signfiles [--fskpath <file signing key>]]
>> PACKAGE_FILE ...
>>
>> The new option to rpmsign signs all the file digests included in the
>> package. When a package is signed with the new option, the file
>> digests are
>> signed with libimaevm and the key %file_signing_key. The resulting
>> signatures are included in the package header as an RPMTAG_FILESIGNATURES
>> tag. Since the header is modified, the SHA1 and MD5 header digests are
>> recalculated and inserted in the signature header.
>>
>> After including file signatures, the package is signed normally.
>>
>> Signed-off-by: Fionnuala Gunter <fin at linux.vnet.ibm.com>
>> ---
>> configure.ac | 8 ++
>> doc/rpmsign.8 | 22 +++-
>> lib/Makefile.am | 3 +-
>> lib/rpmsignfiles.c | 130 ++++++++++++++++++++++
>> lib/rpmsignfiles.h | 45 ++++++++
>> lib/rpmtag.h | 1 +
>> rpmpopt.in | 1 +
>> rpmsign.c | 14 ++-
>> sign/rpmgensig.c | 319
>> ++++++++++++++++++++++++++++++++++++++++++++++++-----
>> sign/rpmsign.h | 7 +-
>> 10 files changed, 520 insertions(+), 30 deletions(-)
>> create mode 100644 lib/rpmsignfiles.c
>> create mode 100644 lib/rpmsignfiles.h
>>
>> diff --git a/configure.ac b/configure.ac
>> index cbb869f..21a6a95 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -658,6 +658,14 @@ AC_SUBST(WITH_SELINUX_LIB)
>> AC_SUBST(WITH_SEMANAGE_LIB)
>> AM_CONDITIONAL(SELINUX,[test "$with_selinux" = yes])
>>
>> +# libimaevm
>> +with_iamevm=no
>> +AC_ARG_WITH(imaevm, [AS_HELP_STRING([--with-imaevm],[build with
>> imaevm support])])
>> +if test "$with_imaevm" = yes ; then
>> + AC_DEFINE(IMAEVM, 1, [Build with imaevm support?])
>> + LIBS="$LIBS -limaevm"
>> +fi
>> +
>> # libcap
>> WITH_CAP_LIB=
>> AC_ARG_WITH(cap, [AS_HELP_STRING([--with-cap],[build with capability
>> support])],
>> diff --git a/doc/rpmsign.8 b/doc/rpmsign.8
>> index 53f2d70..b19f172 100644
>> --- a/doc/rpmsign.8
>> +++ b/doc/rpmsign.8
>> @@ -2,11 +2,17 @@
>> .SH NAME
>> rpmsign \- RPM Package Signing
>> .SH SYNOPSIS
>> +.SS "SIGNING PACKAGES:"
>> +.PP
>>
>> -\fBrpm\fR \fB--addsign|--resign\fR \fB\fIPACKAGE_FILE\fB\fR\fI ...\fR
>> +\fBrpm\fR \fB--addsign|--resign\fR [\fBrpmsign-options\fR]
>> \fB\fIPACKAGE_FILE\fB\fR\fI ...\fR
>>
>> \fBrpm\fR \fB--delsign\fR \fB\fIPACKAGE_FILE\fB\fR\fI ...\fR
>>
>> +.SS "rpmsign-options"
>> +.PP
>> + \fB--fskpath \fIKEY\fB\fR] [\fB--signfiles\fR]
>> +
>> .SH DESCRIPTION
>> .PP
>> Both of the \fB--addsign\fR and \fB--resign\fR
>> @@ -20,6 +26,19 @@ there is no difference in behavior currently.
>> .PP
>> Delete all signatures from each package \fIPACKAGE_FILE\fR given.
>>
>> +.SS "SIGN OPTIONS"
>> +.PP
>> +.TP
>> +\fB--fskpath \fIKEY\fB\fR
>> +Used with \fB--signfiles\fR, use file signing key \fIKEY\fR.
>> +.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.
>> +
>> .SS "USING GPG TO SIGN PACKAGES"
>> .PP
>> In order to sign packages using GPG, \fBrpm\fR
>> @@ -78,4 +97,5 @@ Marc Ewing <marc at redhat.com>
>> Jeff Johnson <jbj at redhat.com>
>> Erik Troan <ewt at redhat.com>
>> Panu Matilainen <pmatilai at redhat.com>
>> +Fionnuala Gunter <fin at linux.vnet.ibm.com>
>> .fi
>> diff --git a/lib/Makefile.am b/lib/Makefile.am
>> index a65eb80..f80a47a 100644
>> --- a/lib/Makefile.am
>> +++ b/lib/Makefile.am
>> @@ -38,7 +38,8 @@ librpm_la_SOURCES = \
>> verify.c rpmlock.c rpmlock.h misc.h relocation.c \
>> rpmscript.h rpmscript.c legacy.c \
>> rpmchroot.c rpmchroot.h \
>> - rpmplugins.c rpmplugins.h rpmplugin.h rpmug.c rpmug.h
>> + rpmplugins.c rpmplugins.h rpmplugin.h rpmug.c rpmug.h \
>> + rpmsignfiles.c rpmsignfiles.h
>>
>> librpm_la_LDFLAGS = -version-info $(rpm_version_info)
>>
>> diff --git a/lib/rpmsignfiles.c b/lib/rpmsignfiles.c
>> new file mode 100644
>> index 0000000..9c33103
>> --- /dev/null
>> +++ b/lib/rpmsignfiles.c
>> @@ -0,0 +1,130 @@
>> +/**
>> + * Copyright (C) 2014 IBM Corporation
>> + *
>> + * Author: Fionnuala Gunter <fin at linux.vnet.ibm.com>
>> + */
>> +
>> +#include "system.h"
>> +#include "imaevm.h"
>> +
>> +#include <rpm/rpmlog.h> /* rpmlog */
>> +#include <rpm/rpmstring.h> /* rnibble */
>> +#include <rpm/rpmpgp.h> /* rpmDigestLength */
>> +#include "lib/header.h" /* HEADERGET_MINMEM */
>> +#include "lib/rpmtypes.h" /* rpmRC */
>> +
>> +#include "lib/rpmsignfiles.h"
>> +
>> +static char *rpmDigestAlgo(uint32_t dalgo)
>> +{
>> + switch (dalgo) {
>> + case 0:
>> + case 1: return "md5";
>> + case 2: return "sha1";
>> + case 8: return "sha256";
>> + case 9: return "sha384";
>> + case 10: return "sha512";
>> + default: return NULL;
>> + }
>> +}
>
> This is returning pointers to string literals, the return type must be
> "const char *"
>
Yes, the return type is wrong.
>> +
>> +char *getDigestAlgo(Header h)
>> +{
>> + struct rpmtd_s digalgo;
>> + uint32_t *dalgo;
>> + char *algo;
>> +
>> + headerGet(h, RPMTAG_FILEDIGESTALGO, &digalgo, HEADERGET_MINMEM);
>> + dalgo = rpmtdGetUint32(&digalgo);
>> + if (!dalgo) {
>> + rpmlog(RPMLOG_ERR, _("rpmtdGetUint32 failed\n"));
>> + return NULL;
>> + }
>> + algo = rpmDigestAlgo(*dalgo);
>> + return algo;
>> +}
>> +
>> +int getDigestLength(Header h)
>> +{
>> + struct rpmtd_s digalgo;
>> + uint32_t *dalgo;
>> + int diglen;
>> +
>> + headerGet(h, RPMTAG_FILEDIGESTALGO, &digalgo, HEADERGET_MINMEM);
>> + dalgo = rpmtdGetUint32(&digalgo);
>> + if (!dalgo) {
>> + rpmlog(RPMLOG_ERR, _("rpmtdGetUint32 failed\n"));
>> + return -1;
>> + }
>> +
>> + diglen = rpmDigestLength(*dalgo);
>> + return diglen;
>> +}
>
> This is a really complicated way of getting a integer or two out of a
> header. headerGetNumber(h, RPMTAG_FILEDIGESTALGO) will give you that it
> one line, and I dont quite see a need for such helpers exported within rpm.
>
I agree, I will use headerGetNumber(h, RPMTAG_FILEDIGESTALGO) instead.
> Also these assume RPMTAG_FILEDIGESTALGO always exists, it does not
> (which means md5 is used)
>
Does RPMTAG_FILEDIGESTALGO exist whenever %_binary_filedigest_algorithm
macro is set?
>
>> +
>> +char *signFile(const char *algo, const char *fdigest, int diglen,
>> const char *key)
>> +{
>> + char *fsignature;
>> + unsigned char digest[BUFSIZ];
>
> BUFSIZ? The digest length is known, calculate the right size from that.
>
Right.
>> + unsigned char signature[BUFSIZ];
>> + int siglen;
>> +
>> +#ifndef IMAEVM
>> + rpmlog(RPMLOG_ERR, _("missing libimaevm\n"));
>> + return NULL;
>> +#endif
>> +
>> + /* convert file digest hex to binary */
>> + memset(digest, 0, BUFSIZ);
>> + for (int i = 0; i < diglen; ++i, fdigest += 2)
>> + digest[i] = (rnibble(fdigest[0]) << 4) | rnibble(fdigest[1]);
>> +
>> + /* prepare file signature */
>> + memset(signature, 0, BUFSIZ);
>> + signature[0] = '\x03';
>> +
>> + /* calculate file signature */
>> + siglen = sign_hash(algo, digest, diglen, key, signature+1);
>
> This looks gets() -class unsafe to me. How do you guarantee the buffer
> passed in is large enough? Certainly BUFSIZ seems wrong as its a
> platform dependent thing, it might be "large enough" on Linux but then
> its by no means obvious. How large are these signatures anyway?
>
>
>> + if (siglen < 0) {
>> + rpmlog(RPMLOG_ERR, _("sign_hash failed\n"));
>> + return NULL;
>> + }
>> +
>> + /* convert file signature binary to hex */
>> + fsignature = pgpHexStr(signature, siglen+1);
>> + return fsignature;
>> +}
>
> I guess sign_hash() is from the IMA library, and another library
> requiring translating between hash algorithm values is understandable,
> but there's no need to expose that to rpm at all. Everywhere in rpm,
> digest algorithms are integer values, and that's how this should take
> the argument as well. The rest of rpm does not need to care.
>
>> +
>> +rpmRC signFiles(Header h, const char *key)
>> +{
>> + struct rpmtd_s digests;
>> + const char *algo;
>> + const char *digest;
>> + char *signature;
>> + int diglen;
>> +
>> + algo = getDigestAlgo(h);
>> + if (!algo) {
>> + rpmlog(RPMLOG_ERR, _("getDigestAlgo failed\n"));
>> + return RPMRC_FAIL;
>> + }
>> +
>> + diglen = getDigestLength(h);
>> + if (diglen < 0) {
>> + rpmlog(RPMLOG_ERR, _("getDigestLength failed\n"));
>> + return RPMRC_FAIL;
>> + }
>
> Almost certainly any error here would get double-logged, first from the
> getDigestFoo() function and then here. The whole thing is as simple as
>
> unsigned int algval = headerGetNumber(h, RPMTAG_FILEDIGESTALGO);
> int diglen = rpmDigestLength(algval);
>
> ...and then pass the algval integer to your the conversion function to
> get something suitable for sign_hash() and *then* check for the error.
> There's little point i
>
>> + headerGet(h, RPMTAG_FILEDIGESTS, &digests, HEADERGET_MINMEM);
>> + while ((digest = rpmtdNextString(&digests))) {
>> + signature = signFile(algo, digest, diglen, key);
>> + if (!signature) {
>> + rpmlog(RPMLOG_ERR, _("signFile failed\n"));
>> + return RPMRC_FAIL;
>> + }
>> + if (!headerPutString(h, RPMTAG_FILESIGNATURES, signature)) {
>> + rpmlog(RPMLOG_ERR, _("headerPutString failed\n"));
>> + return RPMRC_FAIL;
>> + }
>> + }
>> + return RPMRC_OK;
>> +}
>> diff --git a/lib/rpmsignfiles.h b/lib/rpmsignfiles.h
>> new file mode 100644
>> index 0000000..05459f0
>> --- /dev/null
>> +++ b/lib/rpmsignfiles.h
>> @@ -0,0 +1,45 @@
>> +#ifndef H_RPMSIGNFILES
>> +#define H_RPMSIGNFILES
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +/**
>> + * Get file digest algorithm from header
>> + * @param header package header
>> + * @return file digest algorithm
>> + */
>> +char *getDigestAlgo(Header h);
>> +
>> +/**
>> + * Get file digest length from header
>> + * @param header package header
>> + * @return file digest length
>> + */
>> +int getDigestLength(Header h);
>> +
>> +/**
>> + * Sign a file digest with libimaevm
>> + * @param algo file digest algorithm
>> + * @param fdigest file digest hex
>> + * @param diglen digest length
>> + * @param key file signing key
>> + * @return file signature hex
>> + */
>> +char *signFile(const char *algo, const char *fdigest, int diglen,
>> + const char *key);
>> +
>> +/**
>> + * Sign file digests in header and store the signatures in header
>> + * @param h package header
>> + * @param key signing key
>> + * @return RPMRC_OK on success
>> + */
>> +rpmRC signFiles(Header h, const char *key);
>
> Any new non-static functions added to the libraries should be rpm-prefixed.
>
>> +#ifdef _cplusplus
>> +}
>> +#endif
>> +
>> +#endif /* H_RPMSIGNFILES */
>> diff --git a/lib/rpmtag.h b/lib/rpmtag.h
>> index 58b2479..bd6a36a 100644
>> --- a/lib/rpmtag.h
>> +++ b/lib/rpmtag.h
>> @@ -329,6 +329,7 @@ typedef enum rpmTag_e {
>> RPMTAG_SUPPLEMENTNEVRS = 5060, /* s[] extension */
>> RPMTAG_ENHANCENEVRS = 5061, /* s[] extension */
>> RPMTAG_ENCODING = 5062, /* s */
>> + RPMTAG_FILESIGNATURES = 5063, /* s[] */
>>
>> RPMTAG_FIRSTFREE_TAG /*!< internal */
>> } rpmTag;
>> diff --git a/rpmpopt.in b/rpmpopt.in
>> index 036ab4e..df5e2ec 100644
>> --- a/rpmpopt.in
>> +++ b/rpmpopt.in
>> @@ -162,6 +162,7 @@ rpm alias --httpproxy --define '_httpproxy
>> !#:+'
>> rpm exec --addsign rpmsign --addsign
>> rpm exec --delsign rpmsign --delsign
>> rpm exec --resign rpmsign --resign
>> +#rpm exec --signfiles rpmsign --signfiles
>> rpm exec --checksig rpmkeys --checksig
>> rpm exec -K rpmkeys --checksig
>> rpm exec --import rpmkeys --import
>> diff --git a/rpmsign.c b/rpmsign.c
>> index b8e5598..a2692b9 100644
>> --- a/rpmsign.c
>> +++ b/rpmsign.c
>> @@ -20,6 +20,9 @@ enum modes {
>>
>> static int mode = 0;
>>
>> +static int signfiles = 0;
>> +static char * fileSigningKey = NULL;
>> +
>> static struct poptOption signOptsTable[] = {
>> { "addsign", '\0', (POPT_ARG_VAL|POPT_ARGFLAG_OR), &mode,
>> MODE_ADDSIGN,
>> N_("sign package(s)"), NULL },
>> @@ -27,6 +30,11 @@ static struct poptOption signOptsTable[] = {
>> N_("sign package(s) (identical to --addsign)"), NULL },
>> { "delsign", '\0', (POPT_ARG_VAL|POPT_ARGFLAG_OR), &mode,
>> MODE_DELSIGN,
>> N_("delete package signatures"), NULL },
>> + { "signfiles", '\0', POPT_ARG_NONE, &signfiles, 0,
>> + N_("sign package(s) files"), NULL},
>> + { "fskpath", '\0', POPT_ARG_STRING, &fileSigningKey, 0,
>> + N_("use file signing key <key>"),
>> + N_("<key>") },
>> POPT_TABLEEND
>> };
>>
>> @@ -119,6 +127,10 @@ static int doSign(poptContext optCon)
>> goto exit;
>> }
>>
>> + if (fileSigningKey && signfiles) {
>> + addMacro(NULL, "_file_signing_key", NULL, fileSigningKey,
>> RMIL_GLOBAL);
>> + }
>> +
>> /* XXX FIXME: eliminate obsolete getpass() usage */
>> passPhrase = getpass(_("Enter pass phrase: "));
>> passPhrase = (passPhrase != NULL) ? rstrdup(passPhrase) : NULL;
>> @@ -127,7 +139,7 @@ static int doSign(poptContext optCon)
>> fprintf(stderr, _("Pass phrase is good.\n"));
>> rc = 0;
>> while ((arg = poptGetArg(optCon)) != NULL) {
>> - rc += rpmPkgSign(arg, NULL, passPhrase);
>> + rc += rpmPkgSign(arg, NULL, passPhrase, signfiles);
>> }
>> } else {
>> fprintf(stderr, _("Pass phrase check failed or gpg key
>> expired\n"));
>> diff --git a/sign/rpmgensig.c b/sign/rpmgensig.c
>> index 0bd14e3..f07c87e 100644
>> --- a/sign/rpmgensig.c
>> +++ b/sign/rpmgensig.c
>> @@ -1,4 +1,10 @@
>> -/** \ingroup rpmcli
>> +/**
>> + * Copyright (C) 2014 IBM Corporation
>> + *
>> + * Author: Fionnuala Gunter <fin at linux.vnet.ibm.com>
>> + * added support for file signatures
>> + *
>> + * \ingroup rpmcli
>> * \file lib/rpmchecksig.c
>> * Verify the signature of a package.
>> */
>> @@ -8,17 +14,23 @@
>> #include <errno.h>
>> #include <sys/wait.h>
>> #include <popt.h>
>> +#include <imaevm.h>
>> +#include <ctype.h>
>>
>> #include <rpm/rpmlib.h> /* RPMSIGTAG & related */
>> #include <rpm/rpmmacro.h>
>> #include <rpm/rpmpgp.h>
>> #include <rpm/rpmsign.h>
>> -#include <rpm/rpmfileutil.h> /* rpmMkTemp() */
>> +#include <rpm/rpmfileutil.h> /* rpmMkTemp() */
>> #include <rpm/rpmlog.h>
>> #include <rpm/rpmstring.h>
>>
>> #include "lib/rpmlead.h"
>> #include "lib/signature.h"
>> +#include "lib/header.h"
>> +#include "lib/cpio.h" /* rpmcpioOpen, rpmcpioTell */
>> +#include "lib/rpmsignfiles.h" /* signFiles */
>> +#include "rpmio/rpmio_internal.h" /* fdInitDigest, fdFiniDigest */
>>
>> #include "debug.h"
>>
>> @@ -88,6 +100,10 @@ static int manageFile(FD_t *fdp, const char *fn,
>> int flags)
>>
>> /**
>> * Copy header+payload, calculating digest(s) on the fly.
>> + * @param sfdp source file
>> + * @param sfnp source path
>> + * @param tfdp destination file
>> + * @param tfnp destination path
>> */
>> static int copyFile(FD_t *sfdp, const char *sfnp,
>> FD_t *tfdp, const char *tfnp)
>> @@ -121,8 +137,6 @@ static int copyFile(FD_t *sfdp, const char *sfnp,
>> rc = 0;
>>
>> exit:
>> - if (*sfdp) (void) closeFile(sfdp);
>> - if (*tfdp) (void) closeFile(tfdp);
>> return rc;
>> }
>>
>> @@ -458,14 +472,277 @@ static int replaceSignature(Header sigh,
>> sigTarget sigt1, sigTarget sigt2,
>> return rc;
>> }
>>
>> +static rpmRC generateSignature(char *SHA1, uint8_t *MD5, rpm_loff_t
>> size,
>> + rpm_loff_t payloadSize, FD_t fd)
>> +{
>> + Header sig = NULL;
>> + struct rpmtd_s td;
>> + rpmTagVal sizetag;
>> + rpmTagVal payloadtag;
>> + rpm_tagtype_t typetag;
>> + rpmRC rc = RPMRC_OK;
>> + char *reservedSpace;
>> + int spaceSize = 0;
>> +
>> + /* Prepare signature */
>> + sig = rpmNewSignature();
>> +
>> + rpmtdReset(&td);
>> + td.tag = RPMSIGTAG_SHA1;
>> + td.count = 1;
>> + td.type = RPM_STRING_TYPE;
>> + td.data = SHA1;
>> + headerPut(sig, &td, HEADERPUT_DEFAULT);
>> +
>> + rpmtdReset(&td);
>> + td.tag = RPMSIGTAG_MD5;
>> + td.count = 16;
>> + td.type = RPM_BIN_TYPE;
>> + td.data = MD5;
>> + headerPut(sig, &td, HEADERPUT_DEFAULT);
>> +
>> + if (payloadSize < UINT32_MAX) {
>> + sizetag = RPMSIGTAG_SIZE;
>> + payloadtag = RPMSIGTAG_PAYLOADSIZE;
>> + typetag = RPM_INT32_TYPE;
>> + } else {
>> + sizetag = RPMSIGTAG_LONGSIZE;
>> + payloadtag = RPMSIGTAG_LONGARCHIVESIZE;
>> + typetag = RPM_INT64_TYPE;
>> + }
>> +
>> + rpmtdReset(&td);
>> + td.tag = payloadtag;
>> + td.count = 1;
>> + td.type = typetag;
>> + td.data = &payloadSize;
>> + headerPut(sig, &td, HEADERPUT_DEFAULT);
>> +
>> + rpmtdReset(&td);
>> + td.tag = sizetag;
>> + td.count = 1;
>> + td.type = typetag;
>> + td.data = &size;
>> + headerPut(sig, &td, HEADERPUT_DEFAULT);
>> +
>> + spaceSize = rpmExpandNumeric("%{__gpg_reserved_space}");
>> + if(spaceSize > 0) {
>> + reservedSpace = xcalloc(spaceSize, sizeof(char));
>> + rpmtdReset(&td);
>> + td.tag = RPMSIGTAG_RESERVEDSPACE;
>> + td.count = spaceSize;
>> + td.type = RPM_BIN_TYPE;
>> + td.data = reservedSpace;
>> + headerPut(sig, &td, HEADERPUT_DEFAULT);
>> + free(reservedSpace);
>> + }
>> +
>> + /* Reallocate the signature into one contiguous region. */
>> + sig = headerReload(sig, RPMTAG_HEADERSIGNATURES);
>> + if (sig == NULL) { /* XXX can't happen */
>> + rpmlog(RPMLOG_ERR, _("Unable to reload signature header.\n"));
>> + rc = RPMRC_FAIL;
>> + goto exit;
>> + }
>> +
>> + /* Write the signature section into the package. */
>> + if (rpmWriteSignature(fd, sig)) {
>> + rc = RPMRC_FAIL;
>> + goto exit;
>> + }
>> +
>> +exit:
>> + rpmFreeSignature(sig);
>> + return rc;
>> +}
>> +
>> +static void unloadImmutableRegion(Header *hdrp, rpmTagVal tag, rpmtd
>> utd)
>> +{
>> + struct rpmtd_s copytd;
>> + Header nh;
>> + Header oh;
>> + HeaderIterator hi;
>> +
>> + if (headerGet(*hdrp, tag, utd, HEADERGET_DEFAULT)) {
>> + nh = headerNew();
>> + oh = headerCopyLoad(utd->data);
>> + hi = headerInitIterator(oh);
>> +
>> + while (headerNext(hi, ©td)) {
>> + if (copytd.data)
>> + headerPut(nh, ©td, HEADERPUT_DEFAULT);
>> + rpmtdFreeData(©td);
>> + }
>> +
>> + headerFreeIterator(hi);
>> + headerFree(oh);
>> + rpmtdFreeData(utd);
>> +
>> + *hdrp = headerLink(nh);
>> + headerFree(nh);
>> + }
>> +}
>
> Refactoring to smaller/saner functions etc is welcome, but it needs to
> be done as a separate patch. Ie first refactor to your liking in as many
> patches as needed, then add the new functionality as a last separate patch.
>
>> +/**
>> + * Include file signatures in header
>> + * @param fd
>> + * @param rpm path to package
>> + * @param sigp pointer to signature header
>> + * @param hdrp pointer to header
>> + * @param sigStart signature offset in rpm
>> + * @param headerStart header offset in rpm
>> + */
>> +static rpmRC includeFileSignatures(FD_t fd, const char *rpm,
>> + Header *sigp, Header *hdrp,
>> + off_t sigStart, off_t headerStart)
>> +{
>> + FD_t ofd = NULL;
>> + struct rpmtd_s td;
>> + char *trpm = NULL;
>> + const char *key;
>> + char *SHA1 = NULL;
>> + uint8_t *MD5 = NULL;
>> + unsigned char buf[32*BUFSIZ];
>> + size_t sha1len;
>> + size_t md5len;
>> + size_t headerSize;
>> + off_t archiveSize;
>> + rpmRC rc = RPMRC_OK;
>> +
>> + unloadImmutableRegion(hdrp, RPMTAG_HEADERIMMUTABLE, &td);
>> +
>> + 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"));
>> + goto exit;
>> + }
>> +
>> + rc = signFiles(*hdrp, key);
>> + if (rc != RPMRC_OK) {
>> + rpmlog(RPMLOG_ERR, _("signFiles failed\n"));
>> + goto exit;
>> + }
>> +
>> + *hdrp = headerReload(*hdrp, RPMTAG_HEADERIMMUTABLE);
>> + if (*hdrp == NULL) {
>> + rc = RPMRC_FAIL;
>> + rpmlog(RPMLOG_ERR, _("headerReload failed\n"));
>> + goto exit;
>> + }
>> +
>> + ofd = rpmMkTempFile(NULL, &trpm);
>> + if (ofd == NULL || Ferror(ofd)) {
>> + rc = RPMRC_FAIL;
>> + rpmlog(RPMLOG_ERR, _("rpmMkTemp failed\n"));
>> + goto exit;
>> + }
>> +
>> + /* Copy archive to temp file */
>> + if (copyFile(&fd, rpm, &ofd, trpm)) {
>> + rc = RPMRC_FAIL;
>> + rpmlog(RPMLOG_ERR, _("copyFile failed\n"));
>> + goto exit;
>> + }
>> +
>> + if (Fseek(fd, headerStart, SEEK_SET) < 0) {
>> + rc = RPMRC_FAIL;
>> + rpmlog(RPMLOG_ERR, _("Could not seek in file %s: %s\n"),
>> + rpm, Fstrerror(fd));
>> + goto exit;
>> + }
>> +
>> + /* Write header to rpm and recalculate SHA1 */
>> + fdInitDigest(fd, PGPHASHALGO_SHA1, 0);
>> + rc = headerWrite(fd, *hdrp, HEADER_MAGIC_YES);
>> + if (rc != RPMRC_OK) {
>> + rpmlog(RPMLOG_ERR, _("headerWrite failed\n"));
>> + goto exit;
>> + }
>> + fdFiniDigest(fd, PGPHASHALGO_SHA1, (void **)&SHA1, &sha1len, 1);
>> + headerSize = Ftell(fd) - headerStart;
>> +
>> + /* Copy archive from temp file */
>> + if (Fseek(ofd, 0, SEEK_SET) < 0) {
>> + rc = RPMRC_FAIL;
>> + rpmlog(RPMLOG_ERR, _("Could not seek in file %s: %s\n"),
>> + rpm, Fstrerror(fd));
>> + goto exit;
>> + }
>> + if (copyFile(&ofd, trpm, &fd, rpm)) {
>> + rc = RPMRC_FAIL;
>> + rpmlog(RPMLOG_ERR, _("copyFile failed\n"));
>> + goto exit;
>> + }
>> + unlink(trpm);
>> +
>> + /* Recalculate MD5 digest of header+archive */
>> + if (Fseek(fd, headerStart, SEEK_SET) < 0) {
>> + rc = RPMRC_FAIL;
>> + rpmlog(RPMLOG_ERR, _("Could not seek in file %s: %s\n"),
>> + rpm, Fstrerror(fd));
>> + goto exit;
>> + }
>> + fdInitDigest(fd, PGPHASHALGO_MD5, 0);
>> +
>> + while (Fread(buf, sizeof(buf[0]), sizeof(buf), fd) > 0)
>> + ;
>> + if (Ferror(fd)) {
>> + rc = RPMRC_FAIL;
>> + rpmlog(RPMLOG_ERR, _("Fread failed in file %s: %s\n"),
>> + rpm, Fstrerror(fd));
>> + goto exit;
>> + }
>> + fdFiniDigest(fd, PGPHASHALGO_MD5, (void **)&MD5, &md5len, 0);
>> +
>> + if (Fseek(fd, sigStart, SEEK_SET) < 0) {
>> + rc = RPMRC_FAIL;
>> + rpmlog(RPMLOG_ERR, _("Could not seek in file %s: %s\n"),
>> + rpm, Fstrerror(fd));
>> + goto exit;
>> + }
>> +
>> + /* Get payload size from signature tag */
>> + archiveSize = headerGetNumber(*sigp, RPMSIGTAG_PAYLOADSIZE);
>> + if (!archiveSize) {
>> + archiveSize = headerGetNumber(*sigp, RPMSIGTAG_LONGARCHIVESIZE);
>> + }
>> +
>> + /* Replace old digests in signature */
>> + rc = generateSignature(SHA1, MD5, headerSize, archiveSize, fd);
>> + if (rc != RPMRC_OK) {
>> + rpmlog(RPMLOG_ERR, _("insertDigests failed\n"));
>> + goto exit;
>> + }
>> +
>> + if (Fseek(fd, sigStart, SEEK_SET) < 0) {
>> + rc = RPMRC_FAIL;
>> + rpmlog(RPMLOG_ERR, _("Could not seek in file %s: %s\n"),
>> + rpm, Fstrerror(fd));
>> + goto exit;
>> + }
>> +
>> + rc = rpmReadSignature(fd, sigp, RPMSIGTYPE_HEADERSIG, NULL);
>> + if (rc != RPMRC_OK) {
>> + rpmlog(RPMLOG_ERR, _("rpmReadSignature failed\n"));
>> + goto exit;
>> + }
>> +
>> +exit:
>> + return rc;
>> +}
>> +
>> /** \ingroup rpmcli
>> * Create/modify elements in signature header.
>> * @param rpm path to package
>> * @param deleting adding or deleting signature?
>> * @param passPhrase passPhrase (ignored when deleting)
>> + * @param signfiles sign files if non-zero
>> * @return 0 on success, -1 on error
>> */
>> -static int rpmSign(const char *rpm, int deleting, const char
>> *passPhrase)
>> +static int rpmSign(const char *rpm, int deleting, const char
>> *passPhrase,
>> + int signfiles)
>> {
>> FD_t fd = NULL;
>> FD_t ofd = NULL;
>> @@ -516,25 +793,15 @@ static int rpmSign(const char *rpm, int
>> deleting, const char *passPhrase)
>> goto exit;
>> }
>>
>> - /* Dump the immutable region (if present). */
>> - if (headerGet(sigh, RPMTAG_HEADERSIGNATURES, &utd,
>> HEADERGET_DEFAULT)) {
>> - struct rpmtd_s copytd;
>> - Header nh = headerNew();
>> - Header oh = headerCopyLoad(utd.data);
>> - HeaderIterator hi = headerInitIterator(oh);
>> - while (headerNext(hi, ©td)) {
>> - if (copytd.data)
>> - headerPut(nh, ©td, HEADERPUT_DEFAULT);
>> - rpmtdFreeData(©td);
>> + if (signfiles) {
>> + rc = includeFileSignatures(fd, rpm, &sigh, &h, sigStart,
>> headerStart);
>> + if (rc != RPMRC_OK) {
>> + rpmlog(RPMLOG_ERR, _("includeFileSignatures failed\n"));
>> + goto exit;
>> }
>> - headerFreeIterator(hi);
>> - headerFree(oh);
>> - rpmtdFreeData(&utd);
>> -
>> - headerFree(sigh);
>> - sigh = headerLink(nh);
>> - headerFree(nh);
>> }
>> +
>> + unloadImmutableRegion(&sigh, RPMTAG_HEADERSIGNATURES, &utd);
>> origSigSize = headerSizeof(sigh, HEADER_MAGIC_YES);
>
> Maybe I'm missing something but this certainly looks like its doing a
> whole lot of work twice when files are being signed.
>
>>
>> if (deleting) { /* Nuke all the signature tags. */
>> @@ -566,6 +833,7 @@ static int rpmSign(const char *rpm, int deleting,
>> const char *passPhrase)
>>
>> /* Try to make new signature smaller to have size of original
>> signature */
>> rpmtdReset(&utd);
>> +
>> if (headerGet(sigh, RPMSIGTAG_RESERVEDSPACE, &utd,
>> HEADERGET_MINMEM)) {
>> int diff;
>> int count;
>> @@ -668,7 +936,8 @@ exit:
>> }
>>
>> int rpmPkgSign(const char *path,
>> - const struct rpmSignArgs * args, const char *passPhrase)
>> + const struct rpmSignArgs * args, const char *passPhrase,
>> + int signfiles)
>> {
>> int rc;
>>
>> @@ -684,7 +953,7 @@ int rpmPkgSign(const char *path,
>> }
>> }
>>
>> - rc = rpmSign(path, 0, passPhrase);
>> + rc = rpmSign(path, 0, passPhrase, signfiles);
>>
>> if (args) {
>> if (args->hashalgo) {
>> @@ -700,5 +969,5 @@ int rpmPkgSign(const char *path,
>>
>> int rpmPkgDelSign(const char *path)
>> {
>> - return rpmSign(path, 1, NULL);
>> + return rpmSign(path, 1, NULL, 0);
>> }
>> diff --git a/sign/rpmsign.h b/sign/rpmsign.h
>> index 15b3e0f..0ec37c1 100644
>> --- a/sign/rpmsign.h
>> +++ b/sign/rpmsign.h
>> @@ -11,6 +11,8 @@ extern "C" {
>> struct rpmSignArgs {
>> char *keyid;
>> pgpHashAlgo hashalgo;
>> + int signFiles;
>> + const char *fileSigningKey;
>> /* ... what else? */
>> };
>>
>> @@ -19,10 +21,11 @@ struct rpmSignArgs {
>> * @param path path to package
>> * @param args signing parameters (or NULL for defaults)
>> * @param passPhrase passphrase for the signing key
>> + * @param signfiles sign files if non-zero
>> * @return 0 on success
>> */
>> -int rpmPkgSign(const char *path,
>> - const struct rpmSignArgs * args, const char *passPhrase);
>> +int rpmPkgSign(const char *path, const struct rpmSignArgs * args,
>> + const char *passPhrase, int signfiles);
>
> You're already adding a signFiles integer to struct rpmSignArgs. Why
> should it ALSO be passed as a separate argument, breaking not only ABI
> but the API as well? Changing internal interfaces is fine (ie the
> rpmSign() case) but public API shouldn't be broken without a good
> reason, and never when there's just no good reason to do so.
>
> Oh and btw, this patch is still too large to swallow in one gulp. This
> is only a cursory review and I haven't really looked at the actual
> functionality here at all yet.
Thanks for taking a look, I will make these changes and break up the
patches more.
-Fin
>
> - Panu -
> _______________________________________________
> Rpm-maint mailing list
> Rpm-maint at lists.rpm.org
> http://lists.rpm.org/mailman/listinfo/rpm-maint
>
More information about the Rpm-maint
mailing list