From fb7b11ebb852fa255053ecab605bc9cfe9de0603 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 29 Apr 2016 00:31:49 +0200 Subject: don't show NO_PUBKEY warning if repo is signed by another key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Daniel Kahn Gillmor highlights in the bugreport that security isn't improving by having the user import additional keys – especially as importing keys securely is hard. The bugreport was initially about dropping the warning to a notice, but in given the previously mentioned observation and the fact that we weren't printing a warning (or a notice) for expired or revoked keys providing a signature we drop it completely as the code to display a message if this was the only key is in another path – and is considered critical. Closes: 618445 --- apt-pkg/acquire-item.cc | 21 +------- methods/gpgv.cc | 8 +-- test/integration/framework | 67 +++++++++++++++++--------- test/integration/test-apt-key | 54 ++++++++++++++++++++- test/integration/test-apt-update-ims | 2 +- test/integration/test-releasefile-verification | 31 +++++++++++- 6 files changed, 134 insertions(+), 49 deletions(-) diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 317db65b8..fde45e1fe 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -1365,25 +1365,8 @@ void pkgAcqMetaBase::QueueIndexes(bool const verify) /*{{{*/ } } /*}}}*/ -bool pkgAcqMetaBase::VerifyVendor(string const &Message) /*{{{*/ +bool pkgAcqMetaBase::VerifyVendor(string const &) /*{{{*/ { - string::size_type pos; - - // check for missing sigs (that where not fatal because otherwise we had - // bombed earlier) - string missingkeys; - string msg = _("There is no public key available for the " - "following key IDs:\n"); - pos = Message.find("NO_PUBKEY "); - if (pos != std::string::npos) - { - string::size_type start = pos+strlen("NO_PUBKEY "); - string Fingerprint = Message.substr(start, Message.find("\n")-start); - missingkeys += (Fingerprint); - } - if(!missingkeys.empty()) - _error->Warning("%s", (msg + missingkeys).c_str()); - string Transformed = TransactionManager->MetaIndexParser->GetExpectedDist(); if (Transformed == "../project/experimental") @@ -1391,7 +1374,7 @@ bool pkgAcqMetaBase::VerifyVendor(string const &Message) /*{{{*/ Transformed = "experimental"; } - pos = Transformed.rfind('/'); + auto pos = Transformed.rfind('/'); if (pos != string::npos) { Transformed = Transformed.substr(0, pos); diff --git a/methods/gpgv.cc b/methods/gpgv.cc index 53c3ff80e..9099521bd 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -203,14 +203,14 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, } else if (strncmp(buffer, GNUPGGOODSIG, sizeof(GNUPGGOODSIG)-1) == 0) { - char *sig = buffer + sizeof(GNUPGPREFIX); - char *p = sig + sizeof("GOODSIG"); + char *sig = buffer + sizeof(GNUPGGOODSIG); + char *p = sig; while (*p && isxdigit(*p)) p++; *p = 0; if (Debug == true) - std::clog << "Got GOODSIG, key ID:" << sig << std::endl; - GoodSigners.push_back(string(sig)); + std::clog << "Got GOODSIG, key ID: " << sig << std::endl; + GoodSigners.push_back(string(buffer+sizeof(GNUPGPREFIX))); } else if (strncmp(buffer, GNUPGVALIDSIG, sizeof(GNUPGVALIDSIG)-1) == 0) { diff --git a/test/integration/framework b/test/integration/framework index a5cc842ba..7eaa36415 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -1082,40 +1082,61 @@ setupaptarchive() { } signreleasefiles() { - local SIGNER="${1:-Joe Sixpack}" + local SIGNERS="${1:-Joe Sixpack}" local REPODIR="${2:-aptarchive}" if [ -n "$1" ]; then shift; fi if [ -n "$1" ]; then shift; fi - local KEY="keys/$(echo "$SIGNER" | tr 'A-Z' 'a-z' | sed 's# ##g')" - local GPG="aptkey --quiet --keyring ${KEY}.pub --secret-keyring ${KEY}.sec --readonly adv --batch --yes --digest-algo ${APT_TESTS_DIGEST_ALGO:-SHA512}" - msgninfo "\tSign archive with $SIGNER key $KEY… " + local KEY="keys/$(echo "$SIGNERS" | tr 'A-Z' 'a-z' | tr -d ' ,')" + msgninfo "\tSign archive with $SIGNERS key $KEY… " local REXKEY='keys/rexexpired' local SECEXPIREBAK="${REXKEY}.sec.bak" local PUBEXPIREBAK="${REXKEY}.pub.bak" - if [ "${SIGNER}" = 'Rex Expired' ]; then - # the key is expired, so gpg doesn't allow to sign with and the --faked-system-time - # option doesn't exist anymore (and using faketime would add a new obscure dependency) - # therefore we 'temporary' make the key not expired and restore a backup after signing - cp "${REXKEY}.sec" "$SECEXPIREBAK" - cp "${REXKEY}.pub" "$PUBEXPIREBAK" - local SECUNEXPIRED="${REXKEY}.sec.unexpired" - local PUBUNEXPIRED="${REXKEY}.pub.unexpired" - if [ -f "$SECUNEXPIRED" ] && [ -f "$PUBUNEXPIRED" ]; then - cp "$SECUNEXPIRED" "${REXKEY}.sec" - cp "$PUBUNEXPIRED" "${REXKEY}.pub" - else - if ! printf "expire\n1w\nsave\n" | $GPG --default-key "$SIGNER" --command-fd 0 --edit-key "${SIGNER}" >setexpire.gpg 2>&1; then - cat setexpire.gpg - exit 1 + local SIGUSERS="" + while [ -n "${SIGNERS%%,*}" ]; do + local SIGNER="${SIGNERS%%,*}" + if [ "${SIGNERS}" = "${SIGNER}" ]; then + SIGNERS="" + fi + SIGNERS="${SIGNERS#*,}" + # FIXME: This should be the full name, but we can't encode the space properly currently + SIGUSERS="${SIGUSERS} -u ${SIGNER#* }" + if [ "${SIGNER}" = 'Rex Expired' ]; then + # the key is expired, so gpg doesn't allow to sign with and the --faked-system-time + # option doesn't exist anymore (and using faketime would add a new obscure dependency) + # therefore we 'temporary' make the key not expired and restore a backup after signing + cp "${REXKEY}.sec" "$SECEXPIREBAK" + cp "${REXKEY}.pub" "$PUBEXPIREBAK" + local SECUNEXPIRED="${REXKEY}.sec.unexpired" + local PUBUNEXPIRED="${REXKEY}.pub.unexpired" + if [ -f "$SECUNEXPIRED" ] && [ -f "$PUBUNEXPIRED" ]; then + cp "$SECUNEXPIRED" "${REXKEY}.sec" + cp "$PUBUNEXPIRED" "${REXKEY}.pub" + else + if ! printf "expire\n1w\nsave\n" | aptkey --quiet --keyring "${REXKEY}.pub" --secret-keyring "${REXKEY}.sec" \ + --readonly adv --batch --yes --digest-algo "${APT_TESTS_DIGEST_ALGO:-SHA512}" \ + --default-key "$SIGNER" --command-fd 0 --edit-key "${SIGNER}" >setexpire.gpg 2>&1; then + cat setexpire.gpg + exit 1 + fi + cp "${REXKEY}.sec" "$SECUNEXPIRED" + cp "${REXKEY}.pub" "$PUBUNEXPIRED" fi - cp "${REXKEY}.sec" "$SECUNEXPIRED" - cp "${REXKEY}.pub" "$PUBUNEXPIRED" fi + if [ ! -e "${KEY}.pub" ]; then + local K="keys/$(echo "$SIGNER" | tr 'A-Z' 'a-z' | tr -d ' ,')" + cat "${K}.pub" >> "${KEY}.new.pub" + cat "${K}.sec" >> "${KEY}.new.sec" + fi + done + if [ ! -e "${KEY}.pub" ]; then + mv "${KEY}.new.pub" "${KEY}.pub" + mv "${KEY}.new.sec" "${KEY}.sec" fi + local GPG="aptkey --quiet --keyring ${KEY}.pub --secret-keyring ${KEY}.sec --readonly adv --batch --yes --digest-algo ${APT_TESTS_DIGEST_ALGO:-SHA512}" for RELEASE in $(find "${REPODIR}/" -name Release); do - testsuccess $GPG "$@" --default-key "$SIGNER" --armor --detach-sign --sign --output "${RELEASE}.gpg" "${RELEASE}" + testsuccess $GPG "$@" $SIGUSERS --armor --detach-sign --sign --output "${RELEASE}.gpg" "${RELEASE}" local INRELEASE="$(echo "${RELEASE}" | sed 's#/Release$#/InRelease#')" - testsuccess $GPG "$@" --default-key "$SIGNER" --clearsign --output "$INRELEASE" "$RELEASE" + testsuccess $GPG "$@" $SIGUSERS --clearsign --output "$INRELEASE" "$RELEASE" # we might have set a specific date for the Release file, so copy it touch -d "$(stat --format "%y" ${RELEASE})" "${RELEASE}.gpg" "${INRELEASE}" done diff --git a/test/integration/test-apt-key b/test/integration/test-apt-key index 82b64963c..ddb9bf9d2 100755 --- a/test/integration/test-apt-key +++ b/test/integration/test-apt-key @@ -19,6 +19,11 @@ cleanplate() { rm -rf rootdir/etc/apt/trusted.gpg.d/ rootdir/etc/apt/trusted.gpg mkdir rootdir/etc/apt/trusted.gpg.d/ } +testmultigpg() { + testfailure --nomsg aptkey --quiet --readonly "$@" + testsuccess grep "^gpgv: Can't check signature" rootdir/tmp/testfailure.output + testsuccess grep '^gpgv: Good signature from' rootdir/tmp/testfailure.output +} echo 'APT::Key::ArchiveKeyring "./keys/joesixpack.pub"; APT::Key::RemovedKeys "./keys/rexexpired.pub";' > rootdir/etc/apt/apt.conf.d/aptkey.conf @@ -178,7 +183,6 @@ gpg: unchanged: 1' aptkey --fakeroot update adv --batch --yes --default-key 'Marvin' --armor --detach-sign --sign --output signature.gpg signature testsuccess test -s signature.gpg -a -s signature - for GPGV in '' 'gpgv' 'gpgv2'; do echo "APT::Key::GPGVCommand \"$GPGV\";" > rootdir/etc/apt/apt.conf.d/00gpgvcmd @@ -209,6 +213,54 @@ gpg: unchanged: 1' aptkey --fakeroot update echo 'lalalalala' > signature2 testfailure --nomsg aptkey --quiet --readonly verify signature.gpg signature2 done + rm -f rootdir/etc/apt/apt.conf.d/00gpgvcmd + + msgtest 'Test verify a file' 'with good keyring' + testsuccess --nomsg aptkey --quiet --readonly --keyring keys/testcase-multikey.pub verify signature.gpg signature + + cleanplate + cat keys/joesixpack.pub keys/marvinparanoid.pub > keys/double.pub + cat keys/joesixpack.sec keys/marvinparanoid.sec > keys/double.sec + cp -a keys/double.pub rootdir/etc/apt/trusted.gpg.d/double.gpg + cp -a keys/testcase-multikey.pub rootdir/etc/apt/trusted.gpg.d/multikey.gpg + testsuccess aptkey --quiet --keyring keys/double.pub --secret-keyring keys/double.sec --readonly \ + adv --batch --yes -u 'Marvin' -u 'Joe' --armor --detach-sign --sign --output signature.gpg signature + testsuccess test -s signature.gpg -a -s signature + + for GPGV in '' 'gpgv' 'gpgv2'; do + echo "APT::Key::GPGVCommand \"$GPGV\";" > rootdir/etc/apt/apt.conf.d/00gpgvcmd + + msgtest 'Test verify a doublesigned file' 'with all keys' + testsuccess --nomsg aptkey --quiet --readonly verify signature.gpg signature + + msgtest 'Test verify a doublesigned file' 'with good keyring joe' + testmultigpg --keyring keys/joesixpack.pub verify signature.gpg signature + + msgtest 'Test verify a doublesigned file' 'with good keyring marvin' + testmultigpg --keyring keys/marvinparanoid.pub verify signature.gpg signature + + msgtest 'Test fail verify a doublesigned file' 'with bad keyring' + testfailure --nomsg aptkey --quiet --readonly --keyring keys/rexexpired.pub verify signature.gpg signature + + msgtest 'Test fail verify a doublesigned file' 'with non-existing keyring' + testfailure --nomsg aptkey --quiet --readonly --keyring keys/does-not-exist.pub verify signature.gpg signature + testfailure test -e keys/does-not-exist.pub + + # note: this isn't how apts gpgv method implements keyid for verify + msgtest 'Test verify a doublesigned file' 'with good keyid' + testmultigpg --keyid 'Paranoid' verify signature.gpg signature + + msgtest 'Test fail verify a doublesigned file' 'with bad keyid' + testfailure --nomsg aptkey --quiet --readonly --keyid 'Rex' verify signature.gpg signature + + msgtest 'Test fail verify a doublesigned file' 'with non-existing keyid' + testfailure --nomsg aptkey --quiet --readonly --keyid 'Kalnischkies' verify signature.gpg signature + + msgtest 'Test verify fails on' 'bad doublesigned file' + echo 'lalalalala' > signature2 + testfailure --nomsg aptkey --quiet --readonly verify signature.gpg signature2 + done + rm -f rootdir/etc/apt/apt.conf.d/00gpgvcmd } setupgpgcommand() { diff --git a/test/integration/test-apt-update-ims b/test/integration/test-apt-update-ims index d433baeb5..241bf383b 100755 --- a/test/integration/test-apt-update-ims +++ b/test/integration/test-apt-update-ims @@ -49,7 +49,7 @@ runtest() { $TEST aptget update -o Debug::Acquire::gpgv=1 $APTOPT cp rootdir/tmp/${TEST}.output goodsign.output testfileequal 'listsdir.lst' "$(listcurrentlistsdirectory)" - testsuccess grep '^Got GOODSIG, key ID:GOODSIG' goodsign.output + testsuccess grep '^Got GOODSIG, key ID:' goodsign.output fi # ensure no leftovers in partial diff --git a/test/integration/test-releasefile-verification b/test/integration/test-releasefile-verification index a061832b6..5da0a8292 100755 --- a/test/integration/test-releasefile-verification +++ b/test/integration/test-releasefile-verification @@ -127,7 +127,7 @@ runtest() { testsuccessequal "$(cat "${PKGFILE}") " aptcache show apt failaptold - rm rootdir/etc/apt/trusted.gpg.d/rexexpired.gpg + rm -f rootdir/etc/apt/trusted.gpg.d/rexexpired.gpg msgmsg 'Cold archive expired signed by' 'Joe Sixpack' if dpkg --compare-versions "$(aptkey adv --version | head -n 2 | tail -n 1 | cut -d' ' -f 3)" '>=' '2.1' >/dev/null 2>&1; then @@ -152,6 +152,28 @@ runtest() { msgskip 'Not a new enough gpg available providing --fake-system-time' fi + msgmsg 'Cold archive signed by' 'Joe Sixpack,Marvin Paranoid' + prepare "${PKGFILE}" + rm -rf rootdir/var/lib/apt/lists + signreleasefiles 'Joe Sixpack,Marvin Paranoid' + find aptarchive/ -name "$DELETEFILE" -delete + successfulaptgetupdate 'NO_PUBKEY' + testsuccessequal "$(cat "${PKGFILE}") +" aptcache show apt + installaptold + + msgmsg 'Cold archive signed by' 'Joe Sixpack,Rex Expired' + prepare "${PKGFILE}" + rm -rf rootdir/var/lib/apt/lists + signreleasefiles 'Joe Sixpack,Rex Expired' + find aptarchive/ -name "$DELETEFILE" -delete + cp keys/rexexpired.pub rootdir/etc/apt/trusted.gpg.d/rexexpired.gpg + successfulaptgetupdate 'EXPKEYSIG' + rm -f rootdir/etc/apt/trusted.gpg.d/rexexpired.gpg + testsuccessequal "$(cat "${PKGFILE}") +" aptcache show apt + installaptold + msgmsg 'Cold archive signed by' 'Marvin Paranoid' prepare "${PKGFILE}" rm -rf rootdir/var/lib/apt/lists @@ -302,11 +324,18 @@ export APT_TESTS_DIGEST_ALGO='SHA224' successfulaptgetupdate() { testsuccess aptget update -o Debug::pkgAcquire::Worker=1 -o Debug::Acquire::gpgv=1 + if [ -n "$1" ]; then + cp rootdir/tmp/testsuccess.output aptupdate.output + testsuccess grep "$1" aptupdate.output + fi } runtest3 'Trusted' successfulaptgetupdate() { testwarning aptget update -o Debug::pkgAcquire::Worker=1 -o Debug::Acquire::gpgv=1 + if [ -n "$1" ]; then + testsuccess grep "$1" rootdir/tmp/testwarning.output + fi testsuccess grep 'uses weak digest algorithm' rootdir/tmp/testwarning.output } runtest3 'Weak' -- cgit v1.2.3