Add hash of nuget package to lock file

[xavierzwirtz]

Rebase and polish of the work done in #1332 by @baronfel. From my testing, I am not seeing any stability issues with the hashing of the nupkg.

This will enable Nix derivations to be generated from a paket.lock.

[baronfel]

I love it! Thank you for picking up the torch. @forki what do you think of updating the default 'paket init' settings to include hashes?

[forki]

I'd love to have lock file hashes as default for v6. We are currently in alpha. So now is the time. But please make it backwards compatible Chet Husk <notifications@github.com> schrieb am Fr., 7. Feb. 2020, 05:08:

…

I love it! Thank you for picking up the torch. @forki <https://github.com/forki> what do you think of updating the default 'paket init' settings to include hashes? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3789?email_source=notifications&email_token=AAAOANHP4FNW42GX5YDGCHLRBTNEZA5CNFSM4KRICMXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELBU7YI#issuecomment-583225313>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOANA7CXKUYOAFKP5BJ5DRBTNEZANCNFSM4KRICMXA> .

[xavierzwirtz]

I believe as it stands this is fully backwards compatible.The way it currently works hashes are only written to the lock file and verified if hash: on is set in the packet.dependencies. However if we're able to slip this into the 6.0 release it would be great to have hashes just be on by default. If that were the case hashes would be added on the next install or update and while missing just ignored for verification.

Let me know which one you want to go with.

[matthid]

Depending on what problems you are assuming to solve with this you might very well be fooled by how the dotnet sdk integration works.
In the end nuget/msbuild decides what is used for building and what is downloaded.
Paket versions are merely a "suggestion" to nuget.

Can we discuss what we are trying to solve with this? (I'm quite in favor of this, but users might expect more from this than what we actually achieve)

[xavierzwirtz]

My goal with this is to get the sha512 into the lockfile, so that I can feed the restore process files from Nix.

Nix likes to completely own all external requests, so that it can manage builds being fully reproducible, and pulling files from caches when necessary. This PR is a piece of what I need to do that. My rough plan is:

1. Add the SHA512 hash to the lockfile.
2. Add a command to paket that will output the lockfile in json formate, roughly: paket cat lock --json.
3. Use that data to manually restore all packages to a local cache folder inside the Nix sandbox.
4. Run dotnet restore as usual, I think paket should pick up the files in the local cache.

I dont think I will run afoul of any default dotnet behavior, in my experiments so far with manually placing the nupkg files in a local cache this all worked as expected.

The hash verification for non Nix users is a happy side effect, it is not needed for my use case.

[matthid]

@xavierzwirtz I think this might generally work, but there are some edge cases which might not: Sometimes NuGet decides it needs additional or different versioned packages (which are then downloaded with dotnet restore or dotnet build).
Additionally, in some scenarios NuGet still wants to do some requests on its own, so I wouldn't wonder if there are some scenarios where you will still run into issues when you go completely offline/sandbox. I think this is especially true for runtime based packages, like when you build for a specific platform or self-contained exe.

If you want to test early how viable this approach is, I think you can use the paket cache feature and then try to go offline and clear the nuget cache and see how far you can get with the paket cache only.

In any case maybe it works just fine now, last time we tried to disable all NuGet sources for nuget.exe/dotnet.exe and let only paket download we couldn't quite do it.

[baronfel]

There's a confounding factor here for nix derivations in particular, in that framework references aren't versioned in the nuget spec files. I imagine your derivation generator would need to take into account the global.json file in that case? That's not directly applicable for this work here just something I was thinking of.

[xavierzwirtz]

If you want to test early how viable this approach is, I think you can use the paket cache feature and then try to go offline and clear the nuget cache and see how far you can get with the paket cache only.

That's exactly what I did, was able to get a simple hello world project building. The nice thing about Nix is I don't have to clear anything, each build is fully sandboxed.

@baronfel, I definitely haven't got that far with it yet, just identified the issues with packages so far.

Looks like the SHA512 hash is generating differently in CI, probably because its running on Windows. Ill chase that to ground. What is the verdict for making recording hashes default? By my reckoning we have 3 good options:

1. Add hash saving and verification under a setting, default to off. Have paket init generate with it on.
2. Add hash saving as always on, verification under a setting, default to off. Have paket init generate with verification on.
3. Add hash saving and verification under a setting, default to on.

My preferred option is 3, however I am not the one who has to answer issues after this gets merged. If 3 makes you nervous, 2 is safer.

[matthid]

If you really want to test it thoroughly and want to commit to this feature (ie send patches after initial release). I'd suggest:

• Make it on by default after writing the next lockfile (update/install)
• Make sure newer paket can still restore old lockfiles
• Add an option to turn it off (otherwise @forki can only revert if there is an issue)

If you just want to make it work for yourself / opt-in. Or test it beforehand with a real release you can make it opt-in as you suggest. My feeling says if this is opt-in it will only be used by very few people and chances are high the feature will be broken eventually.

[xavierzwirtz]

I have found a pretty major wrinkle for this change. For some reason my linux machine's nuget cache is full of subtly modified nupkg files. The CI checks are failing because the lock file I added only works on my machine. From my investigation so far, I have no idea why the nupkg is different. I attempted a fresh restore with paket after wiping the cache, and the downloaded file has the correct hash. I then attempted wiping the cache, and using dotnet add package to have NuGet restore the nupkg. The hash was still correct.

After discovering this, I looked into how NuGet handles locking the hash of a package. It looks like they hash the contents of the nupkg, instead of hashing the nupkg itself. This makes me think that they discovered the same issue. The "bad" nupkg I had on my linux machine had identical contents to the "good" package that I have been able to repeatedly retrieve from nuget.org. As it stands, I don't think the ergonomics can be made acceptable for verifying the hash of the nupkg itself by default, too many users will have caches with subtly tainted files.

As a path for moving forward I propose:

1. Save the hash of the nupkg in the lock file by default. This will enable Nix derivations to be generated, and whenever a bad hash gets commited it will only blow up for Nix users, who can deal with purging caches and validating their hashes.
2. Add verify_nupkg setting that enables the nupkg verification that is currently in this PR. For projects that want to enable Nix derivations being built, this will allow them to smoke out any bad nupkgs in the developers caches.
3. Save the hash of the contents of the nupkg in the lock file by default.
4. Verify the hash of the contents of the nupkg by default.

[xavierzwirtz]

Looks like this is the culprit for my magical changing hash.

let fixDatesInArchive fileName =
    try
        use zipToOpen = new FileStream(fileName, FileMode.Open)
        use archive = new ZipArchive(zipToOpen, ZipArchiveMode.Update)
        let maxTime = DateTimeOffset.Now

        for e in archive.Entries do
            try
                let d = min maxTime e.LastWriteTime
                e.LastWriteTime <- d
            with
            | _ -> e.LastWriteTime <- maxTime
    with
    | exn -> traceWarnfn "Could not fix timestamps in %s. Error: %s" fileName exn.Message

let fixArchive fileName =
    if isMonoRuntime then
        fixDatesInArchive fileName

@matthid looks like you added this waaay back in
a044cf6#diff-3916c7428f8de1bf642235cbe84f3464R156-R174

I'm scared to touch something that weird looking, must have been a good reason for it.

[matthid]

I briefly looked at it as I couldn't remember doing such hacky stuff and it goes way back to c5a0ce0
and #761 Not sure if it is still relevant to be honest

[matthid]

So in summary that was a workaround for a mono bug which was fixed in mono 4

[xavierzwirtz]

I whacked it out, we'll see if tests pass.

[xavierzwirtz]

Not seeing any failures caused by the removal of fixDatesInArchive. Hash verification tests are still failing from it tainting the cache though, I believe because the copy of paket that is currently released is populating the cache with tainted packages during the build.

[baronfel]

You might be able to get around that for the test runs by setting a different environment variable value for 'NuGetCachePath' during the test runs.

[xavierzwirtz]

Tried that, its only saving some json files in the specified cache folder, no nupkg files.

[xavierzwirtz]

I could use some guidance on how to move this forward. It looks like the logic to hash nupkg files is done, with the default of saving the hash but not verifying. Tests are failing due to tainted caches on the build machines though. I dont like the option of deleting the paket cache from the hash integration tests, feels like that would bite someone in the butt.

[matthid]

What are the options or what do you need to get this green?
Maybe @forki can do an alpha release of this to bootstrap, would this help?

[xavierzwirtz]

I think I might have found a new tactic to make this work sanely, and allow it to be the default behavior. The problem is, I need a way to detect the difference between cached:

• Tainted paket packages that have had their timestamps touched
• Good paket packages that were restored without touched timestamps
• NuGet packages (assumed all are good at this point, not strong evidence other than some simple restores)

After reviewing the cached package folder for a package restored by NuGet, NuGet writes a file called .nupkg.metadata. I could use this file to trust that package as having been cleanly written by NuGet. I could then check to see if a .paket.metadata file has been written, and if so, trust the package. Otherwise, evict the package from the cache as possibly tainted.

This will cause caches to be completely removed upon first use of this new version. Is that an acceptable change?

[xavierzwirtz]

This feature has proven much more painful than I originally thought it would be. Here is the state of my latest work:

• New setting is introduced, nupkg_hash: (verify | save | off). Defaults to verify if not specified.
• Upon install or update when nupkg_hash = (verify | save) if the lock file is missing the hash for a package, the packages hash is calculated and saved.
• Upon restore, if nupkg_hash = verify and the hash has been saved to the lock file, the hash of the restored nupkg is verified.

This logic feels rock solid from my testing, no issue with hashes of identical files not reproducing between machines. The problem arises whenever the nupkg files are not identical. So far there are two routes to that.

In versions of paket other than this PR, when running under mono paket modifies files inside the nupkg LastWriteTime. This causes the hash to not be reproduceable. The way I have resolved that, is writing .paket.metadata into the
package cache upon download of a nupkg, and if a nupkg does not have that file treating it as tainted and purging it.

A bigger issue, is the nugetfallbackfolder. It looks like the packages in the nugetfallbackfolder on the CI server do not match what gets downloaded from nuget, making them dangerous to use if hashing is enabled.

Looping back around to the package cache, I think that whenever NuGet itself downloads a package, it does not modify the nupkg in any way. However, it does use the nugetfallbackfolder, and will insert tainted nupkg files into the cache from there. Therefore we have to treat cached nupkg files downloaded with NuGet the same and evict them from the cache.

I am considering this change done enough for a PoC, if @matthid and @forki are okay with:

1. Requiring users to re-download and extract any packages that would currently be cached
2. Re-downloading packages that were downloaded by NuGet.
3. Removing use of nugetfallbackfolder when hashing is enabled (or entirely if we keep hash verification as the default)

then I will polish this up and get it merge ready.

[xavierzwirtz]

Related.

[ezemtsov]

@xavierzwirtz hi any updates on that? Interested in using Paket with Nix as well.

[xavierzwirtz]

Got ghosted, so I moved on without this PR. I have an implementation of dotnet2nix here that handles paket, at the subset of features that I use.

[baronfel]

I think the only outstanding question was how to handle the fallback cache, right? I think nuget is moving away from the fallback cache with the package source work they just released, and I think a part of that is a flag to disable the fallback cache. We should look at setting that all the time, IMO.

[ezemtsov]

@xavierzwirtz @baronfel do you think we could consider merging it with a flag that disables fallback cache?

[xavierzwirtz]

It has been a long time since I wrote this code and thought about the impact it would have to paket to merge. I am not comfortable vouching for this code, and can't commit any more time to trying to get it merged.

If your goal is to utilize paket with nix, the version of dotnet2nix that I linked above works around the need for this pr by creating a dotnet.lock.nix that contains the sha256 hashes of all nuget packages.

[baronfel]

Re: fallback cache, I think we should add <DisableImplicitNuGetFallbackFolder>true</DisableImplicitNuGetFallbackFolder> to the Paket.Restore.targets actually, to prevent use of the fallback cache in any project that uses paket. that could coincide with the removal of the fallback cache that @xavierzwirtz added in this PR, and more generally falls in line with how Nuget is trending with their lockfile support.

[ezemtsov]

@baronfel Is adding this tag something that is planned in near future?