redownload pkgs when source hash verification fails

[gbaz]

Resolves #7451

(possibly)

Adds a call, when checking hashes in project planning, to also verify the hashes of existing tarballs downloaded from secure repos to make sure things haven't "changed out" as the might in the case of a mutable head.hackage or the like.

• Any changes that could be relevant to users have been recorded in the changelog.

[Mikolaj]

That looks like it may improve things. How to test it in CI?

[Mikolaj]

When you get around to this TODO, could you also make this more readable? The do in parens is hard to parse. E.g.,

Sec.withIndex repoSecure $ \callbacks -> do
  let verification = do
        fileInfo <- Sec.indexLookupFileInfo callbacks pkgid
        sz <- Sec.FileLength . fromInteger <$> getFileSize file
        if sz /= Sec.fileInfoLength (Sec.trusted fileInfo)
        then return False
        else Sec.compareTrustedFileInfo (Sec.trusted fileInfo) <$> Sec.computeFileInfo (Sec.Path file :: Sec.Path Sec.Absolute)
  infoVerified <- 
    verification
    `Sec.catchChecked` (\(e :: Sec.InvalidPackageException) -> warn verbosity (show e) >> return False)
    `Sec.catchChecked` (\(e :: Sec.VerificationError) -> warn verbosity (show e) >> return False)
  if infoVerified then return True else warn verbosity "whoops" >>  return False) -- TODO

which also changes the semantics slightly, probably resulting in a less puzzling warning if an exception occurs.

[gbaz]

Thanks for noticing the TODO. I refactored and cleaned up, but could not factor out into a let block as suggested -- the checked exceptions mechanism in hackage-security is fiddly, and trying some obvious things still resulted in a situation where the typechecker yelled at me. I'm sure it could be made to work, but I think the code after the refactor is sufficiently more clear.

[gbaz]

I don't think this is very CI testable. The best way I can think to test it is to fetch a package, then go into the cached downloads, mess with the downloaded package, and then attempt another action on it. I don't necessarily think adding a new test here makes much sense.

[Mikolaj]

Indeed, just realigning the do made it much easier to parse. Too bad about no smart enough tests to merit including in CI. Thank you.

[Bodigrim]

Is this error message triggered if file simply does not exist?

[Mikolaj]

if it is, it's pretty urgent to fix, since it's causing a warning spam: #8571 (comment)

[Mikolaj]

Yes, this function verifyFetchedTarball is called only in one place in the code, for all elements of repoTarballPkgsWithMetadataUnvalidated, which seems to contain ids of any packages that come from a secure repo, regardless of whether they were ever downloaded:

https://github.com/haskell/cabal/pull/8500/files#diff-2adae381a2eb2d8c804d5396694a431e1e3babd816a5d2408a62213cf774a893R929-R935

[Mikolaj]

A solution would be to move the partitioning wrt if a package is already downloaded (the block containing checkRepoTarballFetched) earlier and apply it to allPkgLocations instead of repoTarballPkgsWithoutMetadata. Only packages that are already downloaded would then be checked for RepoSecure and then verified. Does that make sense?

[Mikolaj]

Or, better, avoiding some filesystem IO, partition repoTarballPkgsWithMetadataUnvalidated wrt if a package is already downloaded.