Browse Source

enhance "hit paywall" error message to mention the probable cause

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.
tags/debian/1.1.exp9
David Kalnischkies 6 years ago
parent
commit
dd676dc71e
6 changed files with 83 additions and 60 deletions
  1. +44
    -46
      apt-pkg/acquire-item.cc
  2. +23
    -0
      apt-pkg/acquire-item.h
  3. +5
    -4
      apt-pkg/acquire-worker.cc
  4. +2
    -3
      test/integration/test-apt-update-nofallback
  5. +6
    -5
      test/integration/test-ubuntu-bug-346386-apt-get-update-paywall
  6. +3
    -2
      test/integration/test-ubuntu-bug-784473-InRelease-one-message-only

+ 44
- 46
apt-pkg/acquire-item.cc View File

@@ -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


+ 23
- 0
apt-pkg/acquire-item.h View File

@@ -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;



+ 5
- 4
apt-pkg/acquire-worker.cc View File

@@ -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());
}


+ 2
- 3
test/integration/test-apt-update-nofallback View File

@@ -151,9 +151,8 @@ test_subvert_inrelease()
# replace InRelease with something else
mv $APTARCHIVE/dists/unstable/Release $APTARCHIVE/dists/unstable/InRelease

testfailureequal "W: Failed to fetch file:${APTARCHIVE}/dists/unstable/InRelease Does not start with a cleartext signature

E: Some index files failed to download. They have been ignored, or old ones used instead." aptget update -qq
testfailuremsg "W: Failed to fetch file:${APTARCHIVE}/dists/unstable/InRelease Clearsigned file isn't valid, got 'NOSPLIT' (does the network require authentication?)
E: Some index files failed to download. They have been ignored, or old ones used instead." aptget update

# ensure we keep the repo
testfileequal lists.before "$(listcurrentlistsdirectory)"


+ 6
- 5
test/integration/test-ubuntu-bug-346386-apt-get-update-paywall View File

@@ -36,8 +36,8 @@ ensure_n_canary_strings_in_dir() {

LISTS='rootdir/var/lib/apt/lists'
rm -rf rootdir/var/lib/apt/lists
msgtest 'Got expected failure message' 'apt-get update'
aptget update -qq 2>&1 | grep -q 'W:.*Does not start with a cleartext signature' && msgpass || msgfail
testfailure aptget update
testsuccess grep '^W:.*Clearsigned file .*NOSPLIT.*' rootdir/tmp/testfailure.output

ensure_n_canary_strings_in_dir $LISTS 'ni ni ni' 0
testequal 'lock
@@ -48,8 +48,8 @@ for f in Release Release.gpg main_binary-amd64_Packages main_source_Sources; do
echo 'peng neee-wom' > $LISTS/localhost:8080_dists_stable_${f}
done

msgtest 'Got expected failure message in' 'apt-get update'
aptget update -qq 2>&1 | grep -q 'W:.*Does not start with a cleartext signature' && msgpass || msgfail
testfailure aptget update
testsuccess grep '^W:.*Clearsigned file .*NOSPLIT.*' rootdir/tmp/testfailure.output

ensure_n_canary_strings_in_dir $LISTS 'peng neee-wom' 4
ensure_n_canary_strings_in_dir $LISTS 'ni ni ni' 0
@@ -58,7 +58,8 @@ ensure_n_canary_strings_in_dir $LISTS 'ni ni ni' 0
echo 'peng neee-wom' > $LISTS/localhost:8080_dists_stable_InRelease
rm -f $LISTS/localhost:8080_dists_stable_Release $LISTS/localhost:8080_dists_stable_Release.gpg
msgtest 'excpected failure of' 'apt-get update'
aptget update -qq 2>&1 | grep -q 'W:.*Does not start with a cleartext signature' && msgpass || msgfail
testfailure aptget update
testsuccess grep '^W:.*Clearsigned file .*NOSPLIT.*' rootdir/tmp/testfailure.output

ensure_n_canary_strings_in_dir $LISTS 'peng neee-wom' 3
ensure_n_canary_strings_in_dir $LISTS 'ni ni ni' 0

+ 3
- 2
test/integration/test-ubuntu-bug-784473-InRelease-one-message-only View File

@@ -27,8 +27,9 @@ MD5Sum:
1b895931853981ad8204d2439821b999 4144 Packages.gz'; echo; cat ${RELEASE}.old;) > ${RELEASE}
done

msgtest 'The unsigned garbage before signed block is' 'ignored'
aptget update -qq 2>&1 | grep -q 'W:.*Does not start with a cleartext signature' && msgpass || msgfail
testfailure aptget update
testsuccess grep '^W:.*Clearsigned file .*NOSPLIT.*' rootdir/tmp/testfailure.output


ROOTDIR="$(readlink -f .)"
testsuccessequal "Package files:


Loading…
Cancel
Save