Merge #348

348: Mitigate dependency confusion r=GunnarFarneback a=GunnarFarneback

This PR adds a dependency confusion mitigation mechanism for public registries beside General, which are particularly exposed to the vulnerability since their UUIDs are, well, public.

The idea, once this PR goes live, is that public registries who want to take part of this mitigation would submit a PR against https://github.com/JuliaRegistries/General/blob/master/.github/workflows/automerge.yml to add their repo URL to the `RegistryCI.AutoMerge.run` argument `public_registries`. When automerge runs, it will clone those repositories and check for a UUID conflict when examining a new-package request. If a conflict is found automerge is blocked, usually.

There is one exception to this rule. Assume you want to add a package to General that has previously lived in one of the checked public repositories. Obviously this will trigger a UUID conflict. However, in this case the package repository will match, which means that
1. An attacker has no way to add rouge versions.
2. Registrator wouldn't have accepted the registration request from someone unauthorized.

So the rule is that automerge is blocked for conflicting UUIDs, unless repository URL and package name match. (Same UUID with different names is no good, regardless of dependency confusion.)

There are some outstanding questions of how much time this takes and whether it gracefully handles connection problems for the public registries.

Cc: @timholy


Co-authored-by: Gunnar Farnebäck <gunnar.farneback@contextvision.se>

Project.toml

@@ -1,7 +1,7 @@
 name = "RegistryCI"
 uuid = "0c95cc5f-2f7e-43fe-82dd-79dbcba86b32"
 authors = ["Dilum Aluthge <dilum@aluthge.com>", "Fredrik Ekre <ekrefredrik@gmail.com>", "contributors"]
-version = "6.3.1"
+version = "6.4.0"
 
 [deps]
 Base64 = "2a0f44e3-6c83-55bd-87e4-b1978d98bd5f"

src/AutoMerge/AutoMerge.jl

@@ -35,5 +35,6 @@ include("not-automerge-applicable.jl")
 include("pull-requests.jl")
 include("semver.jl")
 include("util.jl")
+include("dependency_confusion.jl")
 
 end # module

src/AutoMerge/dependency_confusion.jl

@@ -0,0 +1,58 @@
+# TODO: Add a more thorough explanation of the dependency confusion
+# vulnerability and how this guideline mitigates it.
+
+const guideline_dependency_confusion =
+    Guideline("No UUID conflict with other registries.",
+              data -> has_no_dependency_confusion(data.pkg,
+                                                  data.registry_head,
+                                                  data.public_registries))
+
+# TODO: Needs a strategy to handle connection failures for the public
+# registries. Preferably they should also be cloned only once and then
+# just updated to mitigate the effect of them being temporarily
+# offline. This could be implemented with the help of the Scratch
+# package, but requires Julia >= 1.5.
+function has_no_dependency_confusion(pkg, registry_head, public_registries)
+    # We know the name of this package but not its uuid. Look it up in
+    # the registry that includes the current PR.
+    packages = TOML.parsefile(joinpath(registry_head, "Registry.toml"))["packages"]
+    filter!(packages) do (key, value)
+        value["name"] == pkg
+    end
+    # For Julia >= 1.4 this can be simplified with the `only` function.
+    always_assert(length(packages) == 1)
+    uuid = first(keys(packages))
+    # Also need to find out the package repository.
+    package_repo = TOML.parsefile(joinpath(registry_head, packages[uuid]["path"], "Package.toml"))["repo"]
+    for repo in public_registries
+        try
+            registry = clone_repo(repo)
+            registry_toml = TOML.parsefile(joinpath(registry, "Registry.toml"))
+            packages = registry_toml["packages"]
+            if haskey(packages, uuid)
+                message = string("UUID $uuid conflicts with the package ",
+                                 packages[uuid]["name"], " in registry ",
+                                 registry_toml["name"], " at $repo.\n",
+                                 "This could be a dependency confusion attack.")
+                # Conflict detected. This is benign if the package name
+                # *and* the package URL matches.
+                if packages[uuid]["name"] != pkg
+                    return false, message
+                end
+                package_path = packages[uuid]["path"]
+                other_package_repo = TOML.parsefile(joinpath(registry, package_path, "Package.toml"))["repo"]
+                if package_repo != other_package_repo
+                    return false, message
+                end
+            end
+        catch
+            message = string("Failed to clone public registry $(repo) for a check against dependency confusion.\n",
+                             "This is an internal issue with the AutoMerge process and has nothing to do with ".
+                             "the package being registered but requires manual intervention before AutoMerge ",
+                             "can be resumed.")
+            return false, message
+        end
+    end
+
+    return true, ""
+end

src/AutoMerge/new-package.jl

@@ -52,6 +52,10 @@ function pull_request_build(data::GitHubAutoMergeData, ::NewPackage)::Nothing
     # 13. Version can be loaded
     #     - once it's been installed (and built?), can we load the code?
     #     - i.e. can we run `import Foo`
+    # 14. Package UUID doesn't conflict with an UUID in the provided
+    #     list of package registries. The exception is if also the
+    #     package name *and* package URL matches those in the other
+    #     registry, in which case this is a valid co-registration.
     this_is_jll_package = is_jll_name(data.pkg)
     @info("This is a new package pull request",
           pkg = data.pkg,
@@ -89,7 +93,9 @@ function pull_request_build(data::GitHubAutoMergeData, ::NewPackage)::Nothing
          (guideline_name_ascii, true), # 11
          (:update_status, true),
          (guideline_version_can_be_pkg_added, true), # 12
-         (guideline_version_can_be_imported, true)] # 13
+         (guideline_version_can_be_imported, true), # 13
+         (:update_status, true),
+         (guideline_dependency_confusion, true)] # 14
 
     checked_guidelines = Guideline[]
 

src/AutoMerge/public.jl

@@ -23,7 +23,12 @@ function run(env = ENV,
              suggest_onepointzero::Bool = true,
              #
              registry_deps::Vector{<:AbstractString} = String[],
-             api_url::String="https://api.github.com")::Nothing
+             api_url::String="https://api.github.com",
+             # A list of public Julia registries (repository URLs)
+             # which will be checked for UUID collisions in order to
+             # mitigate the dependency confusion vulnerability. See
+             # the `dependency_confusion.jl` file for details.
+             public_registries::Vector{<:AbstractString} = String[])::Nothing
     all_statuses = deepcopy(additional_statuses)
     all_check_runs = deepcopy(additional_check_runs)
     push!(all_statuses, "automerge/decision")
@@ -72,7 +77,8 @@ function run(env = ENV,
                            master_branch_is_default_branch = master_branch_is_default_branch,
                            suggest_onepointzero = suggest_onepointzero,
                            whoami = whoami,
-                           registry_deps = registry_deps)
+                           registry_deps = registry_deps,
+                           public_registries = public_registries)
     else
         always_assert(run_merge_build)
         cron_or_api_build(api,

src/AutoMerge/pull-requests.jl

@@ -67,7 +67,8 @@ function pull_request_build(api::GitHub.GitHubAPI,
                             master_branch::String,
                             master_branch_is_default_branch::Bool,
                             suggest_onepointzero::Bool,
-                            registry_deps::Vector{<:AbstractString} = String[])::Nothing
+                            registry_deps::Vector{<:AbstractString} = String[],
+                            public_registries::Vector{<:AbstractString} = String[])::Nothing
     pr = my_retry(() -> GitHub.pull_request(api, registry, pr_number; auth=auth))
     _github_api_pr_head_commit_sha = pull_request_head_sha(pr)
     if current_pr_head_commit_sha != _github_api_pr_head_commit_sha
@@ -86,7 +87,8 @@ function pull_request_build(api::GitHub.GitHubAPI,
                                 master_branch_is_default_branch=master_branch_is_default_branch,
                                 suggest_onepointzero=suggest_onepointzero,
                                 whoami=whoami,
-                                registry_deps=registry_deps)
+                                registry_deps=registry_deps,
+                                public_registries=public_registries)
     return result
 end
 
@@ -103,7 +105,8 @@ function pull_request_build(api::GitHub.GitHubAPI,
                             master_branch_is_default_branch::Bool,
                             suggest_onepointzero::Bool,
                             whoami::String,
-                            registry_deps::Vector{<:AbstractString} = String[])::Nothing
+                            registry_deps::Vector{<:AbstractString} = String[],
+                            public_registries::Vector{<:AbstractString} = String[])::Nothing
     # 1. Check if the PR is open, if not quit.
     # 2. Determine if it is a new package or new version of an
     #    existing package, if neither quit.
@@ -159,7 +162,8 @@ function pull_request_build(api::GitHub.GitHubAPI,
                                registry_master = registry_master,
                                suggest_onepointzero = suggest_onepointzero,
                                whoami = whoami,
-                               registry_deps = registry_deps)
+                               registry_deps = registry_deps,
+                               public_registries = public_registries)
     pull_request_build(data, registration_type)
     rm(registry_master; force = true, recursive = true)
     return nothing

src/AutoMerge/types.jl

@@ -80,6 +80,12 @@ struct GitHubAutoMergeData
     # List of dependent registries. Typically this would contain
     # "General" when running automerge for a private registry.
     registry_deps::Vector{String}
+
+    # A list of public Julia registries (repository URLs) which will
+    # be checked for UUID collisions in order to mitigate the
+    # dependency confusion vulnerability. See the
+    # `dependency_confusion.jl` file for details.
+    public_registries::Vector{String}
 end
 
 # Constructor that requires all fields as named arguments.

test/automerge-integration-utils.jl

@@ -284,3 +284,12 @@ function with_feature_branch(f::Function,
     result = f(b)
     return result
 end
+
+function generate_public_registry(public_dir::AbstractString, GIT)
+    public_git_repo = mktempdir()
+    cp(templates(public_dir), public_git_repo, force = true)
+    run(`$(GIT) -C $(public_git_repo) init`)
+    run(`$(GIT) -C $(public_git_repo) add .`)
+    run(`$(GIT) -C $(public_git_repo) commit -m "create"`)
+    return public_git_repo
+end

test/automerge-integration.jl

@@ -31,19 +31,28 @@ delete_old_pull_request_branches(
 )
 
 @testset "Integration tests" begin
-    for (test_number, master_dir, feature_dir, title, pass) in [
-            (1, "master_1", "feature_1", "New package: Requires v1.0.0", true),            # OK: new package
-            (2, "master_2", "feature_2", "New version: Requires v2.0.0", true),            # OK: new version
-            (3, "master_1", "feature_3", "New package: Req v1.0.0", false),                # FAIL: name too short
-            (4, "master_2", "feature_4", "New version: Requires v2.0.1", false),           # FAIL: skips v2.0.0
-            (5, "master_3", "feature_5", "New version: Requires v2.0.0", false),           # FAIL: modifies extra file
-            (6, "master_1", "feature_6", "New package: HelloWorldC_jll v1.0.6+0", true),   # OK: new JLL package
-            (7, "master_4", "feature_7", "New version: HelloWorldC_jll v1.0.8+0", true),   # OK: new JLL version
-            (8, "master_1", "feature_8", "New package: HelloWorldC_jll v1.0.6+0", false),  # FAIL: unallowed dependency
+    for (test_number, master_dir, feature_dir, public_dir, title, pass) in [
+            (1, "master_1", "feature_1", "", "New package: Requires v1.0.0", true),            # OK: new package
+            (2, "master_2", "feature_2", "", "New version: Requires v2.0.0", true),            # OK: new version
+            (3, "master_1", "feature_3", "", "New package: Req v1.0.0", false),                # FAIL: name too short
+            (4, "master_2", "feature_4", "", "New version: Requires v2.0.1", false),           # FAIL: skips v2.0.0
+            (5, "master_3", "feature_5", "", "New version: Requires v2.0.0", false),           # FAIL: modifies extra file
+            (6, "master_1", "feature_6", "", "New package: HelloWorldC_jll v1.0.6+0", true),   # OK: new JLL package
+            (7, "master_4", "feature_7", "", "New version: HelloWorldC_jll v1.0.8+0", true),   # OK: new JLL version
+            (8, "master_1", "feature_8", "", "New package: HelloWorldC_jll v1.0.6+0", false),  # FAIL: unallowed dependency
+            (9, "master_1", "feature_1", "public_1", "New package: Requires v1.0.0", true),    # OK: no UUID conflict
+            (10, "master_1", "feature_1", "public_2", "New package: Requires v1.0.0", false),  # FAIL: UUID conflict, name differs
+            (11, "master_1", "feature_1", "public_3", "New package: Requires v1.0.0", false),  # FAIL: UUID conflict, repo differs
+            (12, "master_1", "feature_1", "public_4", "New package: Requires v1.0.0", true),   # OK: UUID conflict but name and repo match
         ]
         @info "Performing integration tests with settings" test_number master_dir feature_dir title pass
         with_master_branch(templates(master_dir), "master"; GIT = GIT, repo_url = repo_url_with_auth) do master
             with_feature_branch(templates(feature_dir), master; GIT = GIT, repo_url = repo_url_with_auth) do feature
+                public_registries = String[]
+                if public_dir != ""
+                    public_git_repo = generate_public_registry(public_dir, GIT)
+                    push!(public_registries, "file://$(public_git_repo)/.git")
+                end
                 head = feature
                 base = master
                 params = Dict("title" => title,
@@ -75,8 +84,9 @@ delete_old_pull_request_branches(
                                       authorized_authors_special_jll_exceptions = String[whoami],
                                       error_exit_if_automerge_not_applicable = true,
                                       master_branch = master,
-                                      master_branch_is_default_branch = false)
-                        @info "Running integration test for " test_number master_dir feature_dir title pass
+                                      master_branch_is_default_branch = false,
+                                      public_registries = public_registries)
+                        @info "Running integration test for " test_number master_dir feature_dir public_dir title pass
                         if pass
                             run_thunk()
                         else

test/templates/public_1/E/Example/Compat.toml

@@ -0,0 +1,2 @@
+["0.5.3-0"]
+julia = "1"

test/templates/public_1/E/Example/Package.toml

@@ -0,0 +1,3 @@
+name = "Example"
+uuid = "7876af07-990d-54b4-ab0e-23690620f79a"
+repo = "https://github.com/JuliaLang/Example.jl.git"

test/templates/public_1/E/Example/Versions.toml

@@ -0,0 +1,2 @@
+["0.5.3"]
+git-tree-sha1 = "46e44e869b4d90b96bd8ed1fdcf32244fddfb6cc"

test/templates/public_1/Registry.toml

@@ -0,0 +1,8 @@
+name = "PublicRegistry"
+uuid = "1003c4b9-3c72-409d-ba60-5578a18ea1a7"
+repo = ""
+
+description = "This is a test registry for the AutoMerge integration tests."
+
+[packages]
+7876af07-990d-54b4-ab0e-23690620f79a = { name = "Example", path = "E/Example" }

test/templates/public_2/R/Req/Compat.toml

@@ -0,0 +1,2 @@
+[1-2]
+julia = "1"

test/templates/public_2/R/Req/Package.toml

@@ -0,0 +1,3 @@
+name = "Req"
+uuid = "ae029012-a4dd-5104-9daa-d747884805df"
+repo = "https://github.com/MikeInnes/Requires.jl.git"

test/templates/public_2/R/Req/Versions.toml

@@ -0,0 +1,2 @@
+["1.0.0"]
+git-tree-sha1 = "999513b7dea8ac17359ed50ae8ea089e4464e35e"

test/templates/public_2/Registry.toml

@@ -0,0 +1,8 @@
+name = "PublicRegistry"
+uuid = "1003c4b9-3c72-409d-ba60-5578a18ea1a7"
+repo = ""
+
+description = "This is a test registry for the AutoMerge integration tests."
+
+[packages]
+ae029012-a4dd-5104-9daa-d747884805df = { name = "Req", path = "R/Req" }

test/templates/public_3/R/Requires/Compat.toml

@@ -0,0 +1,2 @@
+[1-2]
+julia = "1"

test/templates/public_3/R/Requires/Package.toml

@@ -0,0 +1,3 @@
+name = "Requires"
+uuid = "ae029012-a4dd-5104-9daa-d747884805df"
+repo = "https://github.com/JuliaLang/Example.jl.git"

test/templates/public_3/R/Requires/Versions.toml

@@ -0,0 +1,5 @@
+["1.0.0"]
+git-tree-sha1 = "999513b7dea8ac17359ed50ae8ea089e4464e35e"
+
+["2.0.0"]
+git-tree-sha1 = "999513b7dea8ac17359ed50ae8ea089e4464e35e"

test/templates/public_3/Registry.toml

@@ -0,0 +1,8 @@
+name = "PublicRegistry"
+uuid = "1003c4b9-3c72-409d-ba60-5578a18ea1a7"
+repo = ""
+
+description = "This is a test registry for the AutoMerge integration tests."
+
+[packages]
+ae029012-a4dd-5104-9daa-d747884805df = { name = "Requires", path = "R/Requires" }

test/templates/public_4/R/Requires/Compat.toml

@@ -0,0 +1,2 @@
+[0-1]
+julia = "1"

test/templates/public_4/R/Requires/Package.toml

@@ -0,0 +1,3 @@
+name = "Requires"
+uuid = "ae029012-a4dd-5104-9daa-d747884805df"
+repo = "https://github.com/MikeInnes/Requires.jl.git"

test/templates/public_4/R/Requires/Versions.toml

@@ -0,0 +1,2 @@
+["0.5.2"]
+git-tree-sha1 = "f6fbf4ba64d295e146e49e021207993b6b48c7d1"

test/templates/public_4/Registry.toml

@@ -0,0 +1,8 @@
+name = "PublicRegistry"
+uuid = "1003c4b9-3c72-409d-ba60-5578a18ea1a7"
+repo = ""
+
+description = "This is a test registry for the AutoMerge integration tests."
+
+[packages]
+ae029012-a4dd-5104-9daa-d747884805df = { name = "Requires", path = "R/Requires" }