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

Verify git-tree-sha1 of registry downloads #3408

Merged
merged 1 commit into from
Mar 15, 2023
Merged

Conversation

fredrikekre
Copy link
Member

This patch adds verification of registries downloaded from Pkg server when using the default method of keeping registries as compressed tarballs. This verification is done before the registry is moved in place to ensure that a Pkg server that serves bad content does not also poison the users local machine.

This patch adds verification of registries downloaded from Pkg server
when using the default method of keeping registries as compressed
tarballs. This verification is done before the registry is moved in
place to ensure that a Pkg server that serves bad content does not also
poison the users local machine.
@IanButterworth
Copy link
Member

How expensive is the uncompress? Is it worth passing that forward out of the verify function for later use?

@fredrikekre
Copy link
Member Author

I suppose if there is a way to tee the stream it migh improve a bit. Here are some timings (naively would have thought the hash computation would be faster):

julia> @time Downloads.download("https://pkg.julialang.org/registry/23338594-aafe-5451-b93e-139f81909106/9226550d425f68a52fb15981e5601a6fef34ffcd", "reg.tar.gz");
  0.554557 seconds (5.65 k allocations: 5.920 MiB)

julia> @time Pkg.PlatformEngines.verify_archive_tree_hash("reg.tar.gz", Base.SHA1("9226550d425f68a52fb15981e5601a6fef34ffcd"));
  0.479317 seconds (3.41 M allocations: 200.620 MiB, 17.95% gc time)

@IanButterworth
Copy link
Member

IanButterworth commented Mar 15, 2023

Timing on my i7 2018 MBP

julia> @time Downloads.download("https://pkg.julialang.org/registry/23338594-aafe-5451-b93e-139f81909106/9226550d425f68a52fb15981e5601a6fef34ffcd", "reg.tar.gz");
  2.590834 seconds (17.59 k allocations: 6.416 MiB, 2.76% compilation time)

julia> @time Pkg.PlatformEngines.verify_archive_tree_hash("reg.tar.gz", Base.SHA1("9226550d425f68a52fb15981e5601a6fef34ffcd"));
  1.070156 seconds (3.80 M allocations: 224.332 MiB, 9.52% gc time, 48.54% compilation time)

After warmup

julia> @time Downloads.download("https://pkg.julialang.org/registry/23338594-aafe-5451-b93e-139f81909106/9226550d425f68a52fb15981e5601a6fef34ffcd", "reg.tar.gz");
  0.988754 seconds (17.85 k allocations: 6.427 MiB)

julia> @time Pkg.PlatformEngines.verify_archive_tree_hash("reg.tar.gz", Base.SHA1("9226550d425f68a52fb15981e5601a6fef34ffcd"));
  0.570759 seconds (3.41 M allocations: 200.609 MiB, 13.33% gc time)

@fredrikekre
Copy link
Member Author

Thanks. I don't think the verification will be noticeable in general then. In practice this will only happen if you run Pkg.update and there is a new registry. That operation likely will result in running the resolver, downloading new packages and artifacts, decompress and unpack, precompilation, etc.

@fredrikekre fredrikekre merged commit 1c6ba55 into master Mar 15, 2023
@fredrikekre fredrikekre deleted the fe/verify-registry branch March 15, 2023 15:22
@KristofferC KristofferC mentioned this pull request Mar 24, 2023
7 tasks
KristofferC pushed a commit that referenced this pull request Mar 24, 2023
This patch adds verification of registries downloaded from Pkg server
when using the default method of keeping registries as compressed
tarballs. This verification is done before the registry is moved in
place to ensure that a Pkg server that serves bad content does not also
poison the users local machine.

(cherry picked from commit 1c6ba55)
@fredrikekre
Copy link
Member Author

fredrikekre commented Apr 4, 2023

I don't think the verification will be noticeable

Well, this came back to bite me (https://github.com/fredrikekre/jlpkg runs with --compile=min):

$ time JULIA_DEPOT_PATH=$(mktemp -d) julia --compile=min -e 'import Pkg; Pkg.Registry.add()'
real    5m56,043s
user    5m53,843s
sys     0m0,754s

@IanButterworth
Copy link
Member

What does a profile look like? Any low hanging fruit?

@IanButterworth
Copy link
Member

@fredrikekre should fixing this this hold up updating Pkg? JuliaLang/julia#49295

@KristofferC
Copy link
Member

KristofferC commented Apr 9, 2023

No, compile=min is not a configuration that should block anything. We could precompile this function presumably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants