From 6b01cd087e6f92c5511fe6eea73699e075aa699a Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 11 Sep 2018 16:45:06 +0200 Subject: Refactor internal Signers information storage in gpgv Having a method take a bunch of string vectors is bad style, so we change this to a wrapping struct and adapt the rest of the code brushing it up slightly in the process, which results even in a slightly "better" debug output, no practical change otherwise. Gbp-Dch: Ignore --- methods/gpgv.cc | 140 +++++++++++++++++++++++++++----------------------------- 1 file changed, 67 insertions(+), 73 deletions(-) (limited to 'methods') diff --git a/methods/gpgv.cc b/methods/gpgv.cc index 9135b49c5..1b9f6dd64 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -102,17 +102,21 @@ static bool IsTheSameKey(std::string const &validsig, std::string const &goodsig return validsig.compare(24, 16, goodsig, strlen("GOODSIG "), 16) == 0; } +struct APT_HIDDEN SignersStorage { + std::vector Good; + std::vector Bad; + std::vector Worthless; + // a worthless signature is a expired or revoked one + std::vector SoonWorthless; + std::vector NoPubKey; +}; class GPGVMethod : public aptMethod { private: string VerifyGetSigners(const char *file, const char *outfile, vector const &keyFpts, vector const &keyFiles, - vector &GoodSigners, - vector &BadSigners, - vector &WorthlessSigners, - vector &SoonWorthlessSigners, - vector &NoPubKeySigners); + SignersStorage &Signers); protected: virtual bool URIAcquire(std::string const &Message, FetchItem *Itm) APT_OVERRIDE; public: @@ -146,14 +150,18 @@ static void PushEntryWithUID(std::vector &Signers, char * const buf std::clog << "Got " << msg << " !" << std::endl; Signers.push_back(msg); } +static void implodeVector(std::vector const &vec, std::ostream &out, char const * const sep) +{ + if (vec.empty()) + return; + std::copy(vec.begin(), std::prev(vec.end()), std::ostream_iterator(out, sep)); + out << *vec.rbegin(); + return; +} string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, vector const &keyFpts, vector const &keyFiles, - vector &GoodSigners, - vector &BadSigners, - vector &WorthlessSigners, - vector &SoonWorthlessSigners, - vector &NoPubKeySigners) + SignersStorage &Signers) { bool const Debug = DebugEnabled(); @@ -171,11 +179,7 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, else if (pid == 0) { std::ostringstream keys; - if (keyFiles.empty() == false) - { - std::copy(keyFiles.begin(), keyFiles.end()-1, std::ostream_iterator(keys, ",")); - keys << *keyFiles.rbegin(); - } + implodeVector(keyFiles, keys, ","); ExecGPGV(outfile, file, 3, fd, keys.str()); } close(fd[1]); @@ -201,25 +205,25 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, // if we improve the apt method communication stuff later // it will be better. if (strncmp(buffer, GNUPGBADSIG, sizeof(GNUPGBADSIG)-1) == 0) - PushEntryWithUID(BadSigners, buffer, Debug); + PushEntryWithUID(Signers.Bad, buffer, Debug); else if (strncmp(buffer, GNUPGERRSIG, sizeof(GNUPGERRSIG)-1) == 0) PushEntryWithKeyID(ErrSigners, buffer, Debug); else if (strncmp(buffer, GNUPGNOPUBKEY, sizeof(GNUPGNOPUBKEY)-1) == 0) { - PushEntryWithKeyID(NoPubKeySigners, buffer, Debug); + PushEntryWithKeyID(Signers.NoPubKey, buffer, Debug); ErrSigners.erase(std::remove_if(ErrSigners.begin(), ErrSigners.end(), [&](std::string const &errsig) { return errsig.compare(strlen("ERRSIG "), 16, buffer, sizeof(GNUPGNOPUBKEY), 16) == 0; }), ErrSigners.end()); } else if (strncmp(buffer, GNUPGNODATA, sizeof(GNUPGNODATA)-1) == 0) gotNODATA = true; else if (strncmp(buffer, GNUPGEXPKEYSIG, sizeof(GNUPGEXPKEYSIG)-1) == 0) - PushEntryWithUID(WorthlessSigners, buffer, Debug); + PushEntryWithUID(Signers.Worthless, buffer, Debug); else if (strncmp(buffer, GNUPGEXPSIG, sizeof(GNUPGEXPSIG)-1) == 0) - PushEntryWithUID(WorthlessSigners, buffer, Debug); + PushEntryWithUID(Signers.Worthless, buffer, Debug); else if (strncmp(buffer, GNUPGREVKEYSIG, sizeof(GNUPGREVKEYSIG)-1) == 0) - PushEntryWithUID(WorthlessSigners, buffer, Debug); + PushEntryWithUID(Signers.Worthless, buffer, Debug); else if (strncmp(buffer, GNUPGGOODSIG, sizeof(GNUPGGOODSIG)-1) == 0) - PushEntryWithKeyID(GoodSigners, buffer, Debug); + PushEntryWithKeyID(Signers.Good, buffer, Debug); else if (strncmp(buffer, GNUPGVALIDSIG, sizeof(GNUPGVALIDSIG)-1) == 0) { std::istringstream iss(buffer + sizeof(GNUPGVALIDSIG)); @@ -232,16 +236,16 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, case Digest::State::Weak: // Treat them like an expired key: For that a message about expiry // is emitted, a VALIDSIG, but no GOODSIG. - SoonWorthlessSigners.push_back({sig, digest.name}); + Signers.SoonWorthless.push_back({sig, digest.name}); if (Debug == true) std::clog << "Got weak VALIDSIG, key ID: " << sig << std::endl; break; case Digest::State::Untrusted: // Treat them like an expired key: For that a message about expiry // is emitted, a VALIDSIG, but no GOODSIG. - WorthlessSigners.push_back(sig); - GoodSigners.erase(std::remove_if(GoodSigners.begin(), GoodSigners.end(), [&](std::string const &goodsig) { - return IsTheSameKey(sig, goodsig); }), GoodSigners.end()); + Signers.Worthless.push_back(sig); + Signers.Good.erase(std::remove_if(Signers.Good.begin(), Signers.Good.end(), [&](std::string const &goodsig) { + return IsTheSameKey(sig, goodsig); }), Signers.Good.end()); if (Debug == true) std::clog << "Got untrusted VALIDSIG, key ID: " << sig << std::endl; break; @@ -264,7 +268,7 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, } fclose(pipein); free(buffer); - std::move(ErrSigners.begin(), ErrSigners.end(), std::back_inserter(WorthlessSigners)); + std::move(ErrSigners.begin(), ErrSigners.end(), std::back_inserter(Signers.Worthless)); // apt-key has a --keyid parameter, but this requires gpg, so we call it without it // and instead check after the fact which keyids where used for verification @@ -273,11 +277,11 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, if (Debug == true) { std::clog << "GoodSigs needs to be limited to keyid(s): "; - std::copy(keyFpts.begin(), keyFpts.end(), std::ostream_iterator(std::clog, ", ")); + implodeVector(keyFpts, std::clog, ", "); std::clog << "\n"; } std::vector filteredGood; - for (auto &&good: GoodSigners) + for (auto &&good: Signers.Good) { if (Debug == true) std::clog << "Key " << good << " is good sig, is it also a valid and allowed one? "; @@ -320,9 +324,9 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, if (found) filteredGood.emplace_back(std::move(good)); else - NoPubKeySigners.emplace_back(std::move(good)); + Signers.NoPubKey.emplace_back(std::move(good)); } - GoodSigners = std::move(filteredGood); + Signers.Good= std::move(filteredGood); } int status; @@ -334,16 +338,16 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, if (Debug) { - std::cerr << "Summary:" << std::endl << " Good: "; - std::copy(GoodSigners.begin(), GoodSigners.end(), std::ostream_iterator(std::cerr, ", ")); - std::cerr << std::endl << " Bad: "; - std::copy(BadSigners.begin(), BadSigners.end(), std::ostream_iterator(std::cerr, ", ")); - std::cerr << std::endl << " Worthless: "; - std::copy(WorthlessSigners.begin(), WorthlessSigners.end(), std::ostream_iterator(std::cerr, ", ")); - std::cerr << std::endl << " SoonWorthless: "; - std::for_each(SoonWorthlessSigners.begin(), SoonWorthlessSigners.end(), [](Signer const &sig) { std::cerr << sig.key << ", "; }); - std::cerr << std::endl << " NoPubKey: "; - std::copy(NoPubKeySigners.begin(), NoPubKeySigners.end(), std::ostream_iterator(std::cerr, ", ")); + std::cerr << "Summary:\n Good: "; + implodeVector(Signers.Good, std::cerr, ", "); + std::cerr << "\n Bad: "; + implodeVector(Signers.Bad, std::cerr, ", "); + std::cerr << "\n Worthless: "; + implodeVector(Signers.Worthless, std::cerr, ", "); + std::cerr << "\n SoonWorthless: "; + std::for_each(Signers.SoonWorthless.begin(), Signers.SoonWorthless.end(), [](Signer const &sig) { std::cerr << sig.key << ", "; }); + std::cerr << "\n NoPubKey: "; + implodeVector(Signers.NoPubKey, std::cerr, ", "); std::cerr << std::endl << " NODATA: " << (gotNODATA ? "yes" : "no") << std::endl; } @@ -369,12 +373,12 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, { // gpgv will report success, but we want to enforce a certain keyring // so if we haven't found the key the valid we found is in fact invalid - if (GoodSigners.empty()) + if (Signers.Good.empty()) return _("At least one invalid signature was encountered."); } else { - if (GoodSigners.empty()) + if (Signers.Good.empty()) return _("Internal error: Good signature, but could not determine key fingerprint?!"); } return ""; @@ -391,13 +395,8 @@ bool GPGVMethod::URIAcquire(std::string const &Message, FetchItem *Itm) { URI const Get = Itm->Uri; string const Path = Get.Host + Get.Path; // To account for relative paths - vector GoodSigners; - vector BadSigners; - // a worthless signature is a expired or revoked one - vector WorthlessSigners; - vector SoonWorthlessSigners; - vector NoPubKeySigners; - + SignersStorage Signers; + FetchResult Res; Res.Filename = Itm->DestFile; URIStart(Res); @@ -410,68 +409,63 @@ bool GPGVMethod::URIAcquire(std::string const &Message, FetchItem *Itm) keyFpts.emplace_back(std::move(key)); // Run apt-key on file, extract contents and get the key ID of the signer - string const msg = VerifyGetSigners(Path.c_str(), Itm->DestFile.c_str(), keyFpts, keyFiles, - GoodSigners, BadSigners, WorthlessSigners, - SoonWorthlessSigners, NoPubKeySigners); + string const msg = VerifyGetSigners(Path.c_str(), Itm->DestFile.c_str(), keyFpts, keyFiles, Signers); if (_error->PendingError()) return false; // Check if all good signers are soon worthless and warn in that case - if (std::all_of(GoodSigners.begin(), GoodSigners.end(), [&](std::string const &good) { - return std::any_of(SoonWorthlessSigners.begin(), SoonWorthlessSigners.end(), [&](Signer const &weak) { + if (std::all_of(Signers.Good.begin(), Signers.Good.end(), [&](std::string const &good) { + return std::any_of(Signers.SoonWorthless.begin(), Signers.SoonWorthless.end(), [&](Signer const &weak) { return IsTheSameKey(weak.key, good); }); })) { - for (auto const & Signer : SoonWorthlessSigners) + for (auto const & Signer : Signers.SoonWorthless) // TRANSLATORS: The second %s is the reason and is untranslated for repository owners. Warning(_("Signature by key %s uses weak digest algorithm (%s)"), Signer.key.c_str(), Signer.note.c_str()); } - if (GoodSigners.empty() || !BadSigners.empty() || !NoPubKeySigners.empty()) + if (Signers.Good.empty() || !Signers.Bad.empty() || !Signers.NoPubKey.empty()) { string errmsg; // In this case, something bad probably happened, so we just go // with what the other method gave us for an error message. - if (BadSigners.empty() && WorthlessSigners.empty() && NoPubKeySigners.empty()) + if (Signers.Bad.empty() && Signers.Worthless.empty() && Signers.NoPubKey.empty()) errmsg = msg; else { - if (!BadSigners.empty()) + if (!Signers.Bad.empty()) { errmsg += _("The following signatures were invalid:\n"); - for (vector::iterator I = BadSigners.begin(); - I != BadSigners.end(); ++I) - errmsg += (*I + "\n"); + for (auto const &I : Signers.Bad) + errmsg.append(I).append("\n"); } - if (!WorthlessSigners.empty()) + if (!Signers.Worthless.empty()) { errmsg += _("The following signatures were invalid:\n"); - for (vector::iterator I = WorthlessSigners.begin(); - I != WorthlessSigners.end(); ++I) - errmsg += (*I + "\n"); + for (auto const &I : Signers.Worthless) + errmsg.append(I).append("\n"); } - if (!NoPubKeySigners.empty()) + if (!Signers.NoPubKey.empty()) { errmsg += _("The following signatures couldn't be verified because the public key is not available:\n"); - for (vector::iterator I = NoPubKeySigners.begin(); - I != NoPubKeySigners.end(); ++I) - errmsg += (*I + "\n"); + for (auto const &I : Signers.NoPubKey) + errmsg.append(I).append("\n"); } } // this is only fatal if we have no good sigs or if we have at // least one bad signature. good signatures and NoPubKey signatures // happen easily when a file is signed with multiple signatures - if(GoodSigners.empty() or !BadSigners.empty()) + if(Signers.Good.empty() or !Signers.Bad.empty()) return _error->Error("%s", errmsg.c_str()); } // Just pass the raw output up, because passing it as a real data // structure is too difficult with the method stuff. We keep it // as three separate vectors for future extensibility. - Res.GPGVOutput = GoodSigners; - std::move(BadSigners.begin(), BadSigners.end(), std::back_inserter(Res.GPGVOutput)); - std::move(NoPubKeySigners.begin(), NoPubKeySigners.end(), std::back_inserter(Res.GPGVOutput)); + Res.GPGVOutput = Signers.Good; + std::move(Signers.Bad.begin(), Signers.Bad.end(), std::back_inserter(Res.GPGVOutput)); + std::move(Signers.NoPubKey.begin(), Signers.NoPubKey.end(), std::back_inserter(Res.GPGVOutput)); URIDone(Res); if (DebugEnabled()) -- cgit v1.2.3