Mitigate dependency confusion #348
Conversation
|
A slight generalization would be if we could hook this up either to a registry repo URL, |
|
I agree that some blacklisting solution also for private registries would be desirable but I'm not convinced that it should be plain text and for this PR the focus is to give the more exposed public registries a tool to work with. However, a private registry which really wants to take part in this scheme could make a public dummy registry, which wouldn't have to be much more than a list of UUIDs. |
|
For integration testing it should work to set up a "public registry" in a temporary directory and point to it with a |
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild failed: |
|
bors try |
GunnarFarneback
changed the title
| # 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. |
There was a problem hiding this comment.
I'd be in favor of bumping the minimum version to Julia 1.5 if it helps here; the syntax benefits would be nice too (no more x=x in keyword arguments). And since RegistryCI is usually run by itself in CI (and is a leaf of the dependency tree), one doesn't need to upgrade their whole stack to use the new version.
There was a problem hiding this comment.
a related concern is if an external registry gets corrupted and has an invalid TOML file, say. Then RegistryCI would throw everytime it gets to this check. I wonder if we should use TOML.tryparsefile and fail the check if we get an error, so at least RegistryCI doesn't crash in that event, but we just fail this check each time. That would still be pretty disruptive though.
There was a problem hiding this comment.
It can definitely be done with Scratch, but maybe that's a secondary concern.
Invalid TOML files or other registry irregularities is a real risk. The easiest protection would be a try/catch, but that's rather heavyhanded.
There was a problem hiding this comment.
I think a try/catch would be OK; it seems like that code should only fail if there are issues cloning and parsing the registry. And if we fail the check in the event of an error, we won't be sweeping any issues under the rug, since automerge will fail.
There was a problem hiding this comment.
It's slightly unfair to the affected PRs since it's in no way their fault (well, unless they have made a DOS attack against the registry to try to disable the check...), but I agree that getting attention to the issue is of higher value.
Does someone know how clone_repo behaves when there are network problems?
There was a problem hiding this comment.
yeah, agreed it's not great for the affected PRs... it seems too risky to pass the check if there's an error though, since then an attacker could even do something like add a new public registry they control, then invalidate the TOML or cause network issues in that registry, and then get their package in during the auto-pass window before the problem is fixed.
no idea about what happens during network issues, sorry
There was a problem hiding this comment.
I've added a try/catch with an explanatory message.
|
It seems like we should do this as well as some other mitigations — defense in depth and all that. I agree that if we have a mechanism for private repos to publish this that it should be salted and hashed. That can be done separately. We can develop that in the future. Good to let public non-General registries have some protection ASAP and being able to register themselves here is simple and effective. |
|
Eric and Gunnar, once you're satisfied with this, go ahead and merge it. |
|
And then register a new release of RegistryCI so that we can deploy this. |
|
|
|
bors try |
GunnarFarneback
force-pushed
the
mitigate_dependency_confusion
branch
from
31cb7d9
to
96f7e88
Compare
|
bors merge |
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.runargumentpublic_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
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