Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dotnet: strip signature files from NuGet #326785

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

Smaug123
Copy link
Contributor

@Smaug123 Smaug123 commented Jul 13, 2024

Description of changes

Strip signature files from downloaded NuGet packages.

The result of this PR is that fetchNuget Nupkgs are source-independent per my testing.

Ref #326345

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: dotnet Language: .NET label Jul 13, 2024
runCommand "${pname}.${version}.nupkg" {
nativeBuildInputs = [ zip ];
ORIGINAL_NUPKG = from-source;
# DOTNET_EXE = "${dotnet-sdk}/bin/dotnet";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet been able to verify signatures, so skipping that for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably just do that in nuget-to-nix. no point in doing it again after that.

it might even be done already during restore, but I'm not sure.

Copy link
Contributor Author

@Smaug123 Smaug123 Jul 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems reasonable. I'll delete that completely from this PR. (The thing we lose by relying on nuget-to-nix is any handling of certificate revocation, which has to happen at package download time. If this PR were merged as-is right now, then dotnet restore at Nix build time would never see packages that contained signatures at all, so any revocation checks would have to happen at fetchNuget time.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think revocation lists would be distributed in the sdk, or would it always need to be online in order to check for revocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This depends on NUGET_CERT_REVOCATION_MODE. I generally work with that variable set to offline so that it checks only against the SDK's bundled lists, but that's because I'm usually behind a firewall and don't have access to the Internet.

@Smaug123
Copy link
Contributor Author

Afraid I don't understand what ofborg is telling me here. I edited beatsabermodmanager to delete its platforms restriction, and it built fine (though on my Mac it exits with code -1 as soon as I launch it).

@Smaug123 Smaug123 marked this pull request as ready for review July 13, 2024 16:55
@Smaug123 Smaug123 requested a review from corngood as a code owner July 13, 2024 16:55
@Smaug123
Copy link
Contributor Author

nixpkgs-review is chugging away at the dotnet-vmr builds now, but on the commit before I refactored to mkDerivation, everything went through fine (except for one build, already silently broken on master, for which I raised #326931).

@Smaug123 Smaug123 force-pushed the nuget-deterministic branch 4 times, most recently from 75ebd16 to 8b826fe Compare July 14, 2024 10:10
@Smaug123
Copy link
Contributor Author

I've tested this end-to-end again; it does indeed remove the signatures. Currently running nixpkgs-review; I think this is ready again.

@Smaug123
Copy link
Contributor Author

nixpkgs-review came back clean except for the known-broken-on-master pablodraw and certdump, and the nonfree OpenUtau-0.1.327 which I didn't build.

@Smaug123
Copy link
Contributor Author

A member of the NuGet team points out:

For packages that have an existing .signature.p7s file, this would not work [to recover the original package]. This most often occurs when the package is author signed. In this case the .signature.p7s file is modified to contain a repository counter signature.

I'm pretty sure we don't care about that: we don't care about recovering the original package so much as recovering a deterministic version of the package.

@Smaug123
Copy link
Contributor Author

Smaug123 commented Jul 14, 2024

End-to-end test was again successful on 1b1b23e (a resulting dependency from NuGet.org correctly did not contain the signature); nixpkgs-review again in progress.

Copy link
Contributor

@corngood corngood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll give @NixOS/dotnet a chance to comment before merging.

Copy link
Contributor

@GGG-KILLER GGG-KILLER left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@tie tie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if runCommand is more appropriate here instead of stdenvNoCC.mkDerivation (we don’t really need phases for running zip command), but otherwise LGTM.

@Smaug123
Copy link
Contributor Author

Smaug123 commented Jul 14, 2024

Oh noooo that doesn't produce any output at all when zip fails 🤦 (ah I see I missed your comment)

@tie
Copy link
Member

tie commented Jul 14, 2024

@Smaug123, yes, sorry for the confusion. We can install or cp before running zip (#326785 (comment)) or use --output-file but run install on exit code 12.

zip "$src" --temp-path "$TMPDIR" --output-file "$out" --delete .signature.p7s || {
  (( $? == 12 ))
  install -Dm644 "$src" "$out"
}

@corngood
Copy link
Contributor

I'm running nixpkgs-review here too. Will post when done.

@tie
Copy link
Member

tie commented Jul 14, 2024

Result of nixpkgs-review pr --checkout commit 326785 run on aarch64-linux 1

1 package marked as broken and skipped:
  • naps2
90 packages built:
  • ArchiSteamFarm
  • am2rlauncher
  • avalonia-ilspy
  • beatsabermodmanager
  • bicep
  • boogie (dotnetPackages.Boogie)
  • btcpayserver
  • btcpayserver-altcoins
  • cavalier
  • celeste64
  • certdump
  • csharp-ls
  • csharpier
  • csharprepl
  • cyclonedx-cli
  • dafny
  • denaro
  • depotdownloader
  • dotnet-outdated
  • dotnetCorePackages.dotnet_8.aspnetcore
  • dotnetCorePackages.dotnet_8.runtime
  • dotnetCorePackages.dotnet_8.sdk
  • dotnetCorePackages.dotnet_8.sdk.artifacts
  • dotnetCorePackages.dotnet_8.sdk.packages
  • dotnetCorePackages.dotnet_8.vmr
  • dotnetCorePackages.dotnet_9.aspnetcore
  • dotnetCorePackages.dotnet_9.runtime
  • dotnetCorePackages.dotnet_9.sdk
  • dotnetCorePackages.dotnet_9.sdk.artifacts
  • dotnetCorePackages.dotnet_9.sdk.packages
  • dotnetCorePackages.dotnet_9.vmr
  • fable
  • famistudio
  • fantomas
  • formula
  • fsautocomplete
  • galaxy-buds-client
  • garnet
  • gh-gei
  • git-credential-manager
  • github-runner
  • gitversion
  • godot3-mono
  • godot3-mono-debug-server
  • godot3-mono-export-templates
  • godot3-mono-headless
  • godot3-mono-headless.dev
  • godot3-mono-headless.man
  • godot3-mono-server
  • godot3-mono.dev
  • godot3-mono.man
  • ilspycmd
  • jackett
  • jellyfin
  • kavita
  • knossosnet
  • libation
  • lubelogger
  • lumafly
  • marksman
  • mqttmultimeter
  • msbuild
  • nbxplorer
  • netcoredbg
  • networkminer
  • nexusmods-app
  • nexusmods-app-unfree
  • omnisharp-roslyn
  • opentabletdriver
  • openutau
  • pablodraw
  • parabolic
  • pbm
  • pinta
  • pupdate
  • python311Packages.clr-loader
  • python311Packages.clr-loader.dist
  • python312Packages.clr-loader
  • python312Packages.clr-loader.dist
  • retrospy
  • roslyn
  • roslyn-ls
  • ryujinx
  • scarab
  • slskd
  • sonarr
  • technitium-dns-server
  • tests.dotnet.structured-attrs.no-structured-attrs
  • tone
  • torrentstream

Note: this change essentially doubles disk usage for rebuilds, had to restart nixpkgs-review because I ended up running out of filesystem space.

Just a thought (not asking to implement this in the PR) — perhaps we can add an option to run zip in postFetch to avoid building multiple derivations for newly generated deps.nix (I don’t think we can use postFetch now without invalidating existing hashes)?
E.g.

fetchNuGet {
  pname = "protobuf-net";
  version = "3.2.16";
  hash = "…";
  deleteSignatureInPostFetch = true;
}

@corngood
Copy link
Contributor

That's more or less what I was thinking about here: #326345 (comment)

We just need to teach nuget-to-nix about it.

@Smaug123
Copy link
Contributor Author

I recently deleted Baldur's Gate III, which left me with plenty of room :P

My nixpkgs-review has failed for the first time today for what I weakly suspect is an unrelated reason (the logs are pretty inscrutable but during the build of Microsoft.AspNetCore.Watch.BrowserRefresh.csproj in dotnet-vmr-9 I got "Process terminated"). Past midnight now though so I won't dig any more today.

@corngood
Copy link
Contributor

I think there might be an intermittent failure in dotnet 9 VMR, unfortunately, so I wouldn't worry too much about that one.

@corngood
Copy link
Contributor

Result of nixpkgs-review pr 326785 run on x86_64-linux 1

1 package failed to build:
  • ryujinx
104 packages built:
  • ArchiSteamFarm
  • alttpr-opentracker
  • am2rlauncher
  • avalonia-ilspy
  • azure-functions-core-tools
  • beatsabermodmanager
  • bicep
  • boogie
  • btcpayserver
  • btcpayserver-altcoins
  • cavalier
  • celeste64
  • certdump
  • csharp-ls
  • csharpier
  • csharprepl
  • cyclonedx-cli
  • dafny
  • denaro
  • depotdownloader
  • discordchatexporter-cli
  • dotnet-outdated
  • dotnetCorePackages.dotnet_8.aspnetcore
  • dotnetCorePackages.dotnet_8.runtime
  • dotnetCorePackages.dotnet_8.sdk
  • dotnetCorePackages.dotnet_8.sdk.artifacts
  • dotnetCorePackages.dotnet_8.sdk.packages
  • dotnetCorePackages.dotnet_8.vmr
  • dotnetCorePackages.dotnet_9.aspnetcore
  • dotnetCorePackages.dotnet_9.runtime
  • dotnetCorePackages.dotnet_9.sdk
  • dotnetCorePackages.dotnet_9.sdk.artifacts
  • dotnetCorePackages.dotnet_9.sdk.packages
  • dotnetCorePackages.dotnet_9.vmr
  • eventstore
  • fable
  • famistudio
  • fantomas
  • formula
  • fsautocomplete
  • galaxy-buds-client
  • garnet
  • gh-gei
  • git-credential-manager
  • github-runner
  • gitversion
  • godot3-mono
  • godot3-mono-debug-server
  • godot3-mono-export-templates
  • godot3-mono-headless
  • godot3-mono-headless.dev
  • godot3-mono-headless.man
  • godot3-mono-server
  • godot3-mono.dev
  • godot3-mono.man
  • ilspycmd
  • inklecate
  • jackett
  • jellyfin
  • kavita
  • knossosnet
  • libation
  • lubelogger
  • lumafly
  • marksman
  • mqttmultimeter
  • msbuild
  • naps2
  • nbxplorer
  • netcoredbg
  • networkminer
  • nexusmods-app
  • nexusmods-app-unfree
  • omnisharp-roslyn
  • openra
  • opentabletdriver
  • openutau
  • osu-lazer
  • pablodraw
  • parabolic
  • pbm
  • pinta
  • ps3-disc-dumper
  • pupdate
  • python311Packages.clr-loader
  • python311Packages.clr-loader.dist
  • python311Packages.pythonnet
  • python311Packages.pythonnet.dist
  • python312Packages.clr-loader
  • python312Packages.clr-loader.dist
  • python312Packages.pythonnet
  • python312Packages.pythonnet.dist
  • retrospy
  • roslyn
  • roslyn-ls
  • scarab
  • slskd
  • sonarr
  • space-station-14-launcher
  • technitium-dns-server
  • tone
  • torrentstream
  • wasabibackend
  • xivlauncher

@corngood
Copy link
Contributor

The only failure was ryujinx hanging during checkPhase, so I'll consider that a pass.

@corngood corngood merged commit d50a509 into NixOS:master Jul 15, 2024
22 checks passed
@Smaug123 Smaug123 deleted the nuget-deterministic branch July 15, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants