From dd676dc71e31c20f66d5b9d9ac1c5a4c8883cd79 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 9 Aug 2015 17:40:57 +0200 Subject: enhance "hit paywall" error message to mention the probable cause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reporting errors from Done() is bad for progress reporting and such, so factoring this out is a good idea and we start with moving the supposed- to-be clearsigned file isn't clearsigned out first – improving the error message in the process as we use the same message for a similar case (NODATA) as this is what I have to look at with the venue wifi at DebCamp and the old errormessage doesn't really say anything. --- apt-pkg/acquire-item.cc | 90 +++++++++++++++++++++++------------------------ apt-pkg/acquire-item.h | 23 ++++++++++++ apt-pkg/acquire-worker.cc | 9 ++--- 3 files changed, 72 insertions(+), 50 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 8a566fea0..0a7e0e11f 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -514,6 +514,23 @@ void pkgAcquire::Item::Start(string const &/*Message*/, unsigned long long const FileSize = Size; } /*}}}*/ +// Acquire::Item::VerifyDone - check if Item was downloaded OK /*{{{*/ +/* Note that hash-verification is 'hardcoded' in acquire-worker and has + * already passed if this method is called. */ +bool pkgAcquire::Item::VerifyDone(std::string const &Message, + pkgAcquire::MethodConfig const * const /*Cnf*/) +{ + std::string const FileName = LookupTag(Message,"Filename"); + if (FileName.empty() == true) + { + Status = StatError; + ErrorText = "Method gave a blank filename"; + return false; + } + + return true; +} + /*}}}*/ // Acquire::Item::Done - Item downloaded OK /*{{{*/ void pkgAcquire::Item::Done(string const &/*Message*/, HashStringList const &Hashes, pkgAcquire::MethodConfig const * const /*Cnf*/) @@ -585,8 +602,8 @@ bool pkgAcquire::Item::RenameOnError(pkgAcquire::Item::RenameOnErrorState const Status = StatError; break; case NotClearsigned: - errtext = _("Does not start with a cleartext signature"); - Status = StatError; + strprintf(errtext, _("Clearsigned file isn't valid, got '%s' (does the network require authentication?)"), "NOSPLIT"); + Status = StatAuthError; break; case MaximumSizeExceeded: // the method is expected to report a good error for this @@ -783,7 +800,7 @@ bool pkgAcqMetaBase::CheckStopAuthentication(pkgAcquire::Item * const I, const s _error->Error(_("GPG error: %s: %s"), Desc.Description.c_str(), LookupTag(Message,"Message").c_str()); - I->Status = StatError; + I->Status = StatAuthError; return true; } else { _error->Warning(_("GPG error: %s: %s"), @@ -829,14 +846,7 @@ bool pkgAcqMetaBase::CheckDownloadDone(pkgAcqTransactionItem * const I, const st // We have just finished downloading a Release file (it is not // verified yet) - string const FileName = LookupTag(Message,"Filename"); - if (FileName.empty() == true) - { - I->Status = StatError; - I->ErrorText = "Method gave a blank filename"; - return false; - } - + std::string const FileName = LookupTag(Message,"Filename"); if (FileName != I->DestFile && RealFileExists(I->DestFile) == false) { I->Local = true; @@ -1142,6 +1152,16 @@ string pkgAcqMetaClearSig::Custom600Headers() const return Header; } /*}}}*/ +bool pkgAcqMetaClearSig::VerifyDone(std::string const &Message, + pkgAcquire::MethodConfig const * const Cnf) +{ + Item::VerifyDone(Message, Cnf); + + if (FileExists(DestFile) && !StartsWithGPGClearTextSignature(DestFile)) + return RenameOnError(NotClearsigned); + + return true; +} // pkgAcqMetaClearSig::Done - We got a file /*{{{*/ void pkgAcqMetaClearSig::Done(std::string const &Message, HashStringList const &Hashes, @@ -1149,17 +1169,6 @@ void pkgAcqMetaClearSig::Done(std::string const &Message, { Item::Done(Message, Hashes, Cnf); - // if we expect a ClearTextSignature (InRelease), ensure that - // this is what we get and if not fail to queue a - // Release/Release.gpg, see #346386 - if (FileExists(DestFile) && !StartsWithGPGClearTextSignature(DestFile)) - { - pkgAcquire::Item::Failed(Message, Cnf); - RenameOnError(NotClearsigned); - TransactionManager->AbortTransaction(); - return; - } - if(AuthPass == false) { if(CheckDownloadDone(this, Message, Hashes) == true) @@ -1190,6 +1199,16 @@ void pkgAcqMetaClearSig::Failed(string const &Message,pkgAcquire::MethodConfig c if (AuthPass == false) { + if (Status == StatAuthError) + { + // if we expected a ClearTextSignature (InRelease) and got a file, + // but it wasn't valid we end up here (see VerifyDone). + // As these is usually called by web-portals we do not try Release/Release.gpg + // as this is gonna fail anyway and instead abort our try (LP#346386) + TransactionManager->AbortTransaction(); + return; + } + // Queue the 'old' InRelease file for removal if we try Release.gpg // as otherwise the file will stay around and gives a false-auth // impression (CVE-2012-0214) @@ -2500,7 +2519,7 @@ void pkgAcqIndex::StageDownloadDone(string const &Message, HashStringList const Complete = true; // Handle the unzipd case - string FileName = LookupTag(Message,"Alt-Filename"); + std::string FileName = LookupTag(Message,"Alt-Filename"); if (FileName.empty() == false) { Stage = STAGE_DECOMPRESS_AND_VERIFY; @@ -2511,13 +2530,7 @@ void pkgAcqIndex::StageDownloadDone(string const &Message, HashStringList const SetActiveSubprocess("copy"); return; } - FileName = LookupTag(Message,"Filename"); - if (FileName.empty() == true) - { - Status = StatError; - ErrorText = "Method gave a blank filename"; - } // Methods like e.g. "file:" will give us a (compressed) FileName that is // not the "DestFile" we set, in this case we uncompress from the local file @@ -2791,15 +2804,7 @@ void pkgAcqArchive::Done(string const &Message, HashStringList const &Hashes, Item::Done(Message, Hashes, Cfg); // Grab the output filename - string FileName = LookupTag(Message,"Filename"); - if (FileName.empty() == true) - { - Status = StatError; - ErrorText = "Method gave a blank filename"; - return; - } - - // Reference filename + std::string const FileName = LookupTag(Message,"Filename"); if (DestFile != FileName && RealFileExists(DestFile) == false) { StoreFilename = DestFile = FileName; @@ -3121,14 +3126,7 @@ void pkgAcqFile::Done(string const &Message,HashStringList const &CalcHashes, { Item::Done(Message,CalcHashes,Cnf); - string FileName = LookupTag(Message,"Filename"); - if (FileName.empty() == true) - { - Status = StatError; - ErrorText = "Method gave a blank filename"; - return; - } - + std::string const FileName = LookupTag(Message,"Filename"); Complete = true; // The files timestamp matches diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index 2349d386c..d6bcdafcb 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -176,6 +176,28 @@ class pkgAcquire::Item : public WeakPointable /*{{{*/ */ virtual void Failed(std::string const &Message,pkgAcquire::MethodConfig const * const Cnf); + /** \brief Invoked by the acquire worker to check if the successfully + * fetched object is also the objected we wanted to have. + * + * Note that the object might \e not have been written to + * DestFile; check for the presence of an Alt-Filename entry in + * Message to find the file to which it was really written. + * + * This is called before Done is called and can prevent it by returning + * \b false which will result in Failed being called instead. + * + * You should prefer to use this method over calling Failed() from Done() + * as this has e.g. the wrong progress reporting. + * + * \param Message Data from the acquire method. Use LookupTag() + * to parse it. + * \param Cnf The method via which the object was fetched. + * + * \sa pkgAcqMethod + */ + virtual bool VerifyDone(std::string const &Message, + pkgAcquire::MethodConfig const * const Cnf); + /** \brief Invoked by the acquire worker when the object was * fetched successfully. * @@ -563,6 +585,7 @@ class APT_HIDDEN pkgAcqMetaClearSig : public pkgAcqMetaIndex virtual void Failed(std::string const &Message,pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE; virtual std::string Custom600Headers() const APT_OVERRIDE; + virtual bool VerifyDone(std::string const &Message, pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE; virtual void Done(std::string const &Message, HashStringList const &Hashes, pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE; diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc index dc03a8870..2c84020fe 100644 --- a/apt-pkg/acquire-worker.cc +++ b/apt-pkg/acquire-worker.cc @@ -411,11 +411,14 @@ bool pkgAcquire::Worker::RunMessages() else consideredOkay = true; + if (consideredOkay == true) + consideredOkay = Owner->VerifyDone(Message, Config); + else // hashsum mismatch + Owner->Status = pkgAcquire::Item::StatAuthError; + if (consideredOkay == true) { Owner->Done(Message, ReceivedHashes, Config); - - // Log that we are done if (Log != 0) { if (isIMSHit) @@ -426,9 +429,7 @@ bool pkgAcquire::Worker::RunMessages() } else { - Owner->Status = pkgAcquire::Item::StatAuthError; Owner->Failed(Message,Config); - if (Log != 0) Log->Fail(Owner->GetItemDesc()); } -- cgit v1.2.3