WIP: Rewrite git://github.com to https://github.com in package URLs. #11312

LachlanGunn · 2015-05-17T14:14:01Z

Introduction

As git:// URLs cannot be loaded behind proxies, the current package manager is broken for many institutional users without manual configuration of Git using the "git://".insteadOf workaround. This is a major usability problem, and leaves a bad first impression for new users who are used to the polished installer of Matlab.

As discussed in #7005, the git:// protocol also provides no security against man-in-the-middle attacks, to which the current system is vulnerable due to the lack of digital signatures in the process.

The Pkg package has therefore been made to automatically change all git://github.com URLs to https://github.com. URLs pointing to other domains will be unaffected. If a complete change from git:// to https:// is made, then it is anticipated that existing URLs in METADATA.jl will be changed from git:// to https://. This may render the URL normalisation part of these changes unnecessary, however it would still be necessary to change the METADATA.jl URL to use HTTPS.

Changes

The JuliaLang/METADATA.jl repository URL has been changed from git:// to https:// .
Generated URLS have all been changed from git:// to https://
The existing URL normalisation code in base/pkg/git.jl has been modified so that it normalises to https:// rather than git://
All URLs are now normalised before being passed to "git clone" or Git.set_remote_url.

Together, these changes allow packages to be installed without access to the git:// port.

Test procedure

Compile Julia in Vagrant-setup virtual machine
Run 'sudo iptables -I OUTPUT -p tcp --dport 9418 -j REJECT'
Ensure that port is blocked by running 'git clone git://github.com/JuliaLang/Example.jl'.
Run 'jlmake test-pkg'

mlubin · 2015-05-17T16:06:47Z

base/pkg/entry.jl

@@ -148,11 +148,12 @@ function status(io::IO, pkg::AbstractString, ver::VersionNumber, fix::Bool)
 end

 function clone(url::AbstractString, pkg::AbstractString)
-    info("Cloning $pkg from $url")
+    normalized_url = Git.normalize_url(url)


Does this also rewrite urls when users explicitly use ssh, for example? If the user explicitly provides a url, it shouldn't be rewritten.

colne is done by user. Do not mess with user's url.

This won't rewrite SSH URLs, only git and https, but you're right, if the user manually provides a URL to clone(), it shouldn't be rewritten. I'll remove the normalisation from this function. For reference, the regex (which I've not touched) is:

r"^(?:git@|git://|https://(?:[\w\.\+\-]+@)?)github.com[:/](([^/].+)/(.+?))(?:\.git)?$"i

Keno · 2015-05-17T19:27:43Z

This used to be the default, but then got changed because it turned out more users had https blocked than git://. We really need to provide a way to have the user be able to configure it (or try to autodetect).

simonster · 2015-05-17T21:31:51Z

I would really like to see everything (but especially METADATA.jl) use https by default. The current setup is really not very secure.

tkelman · 2015-05-17T21:54:30Z

Not to mention SSH keys are a completely foreign beast on Windows, ref #10892

mikewl · 2015-05-19T07:50:17Z

To keep everyone happy, why not modify the function arguments with a kwarg for selecting the URL type?

LachlanGunn · 2015-05-19T11:54:56Z

That seems like the best idea---my thinking is to make HTTPS the default, but allowing the user to use git:// or to disable rewriting altogether with an option to Pkg.init. Any thoughts?

aviks · 2015-05-19T11:56:33Z

@LachlanGunn +1

ihnorton · 2015-05-19T11:56:50Z

+1

LachlanGunn · 2015-05-19T13:50:37Z

Ok, the first of these issues is dealt with---Pkg.clone() won't do any rewriting anymore. Making all rewriting optional/configurable will be a few days more.

tkelman · 2015-05-19T14:42:05Z

So is this still WIP then?

LachlanGunn · 2015-05-19T14:46:09Z

Yes. I've changed the title to reflect this.

ViralBShah · 2015-05-20T04:51:19Z

I too like the idea of https by default, but perhaps Pkg can automatically fall back to git if https fails, and print a warning to that effect, and finally printing an error message about proxies/firewalls, if git fails too.

simonster · 2015-05-20T13:08:51Z

I don't think Pkg should automatically fall back to git:// without user intervention. That has the same security issues as using git:// by default. If you don't cancel before the package build scripts run, your computer can be compromised.

ScottPJones · 2015-05-20T13:11:04Z

I think @simonster is correct, these days the security issues are extremely important...

felipenoris · 2015-10-08T14:54:13Z

I would suggest https:// to be the default behavior without fallback.
Maybe Pkg could provide a function to customize this. Something like Pkg.usegitprotocol(true) writing this configuration to the user configuration file ( ~/.juliarc.jl or something like that).

LachlanGunn · 2015-10-08T15:31:27Z

Yes, you are correct. This change was reasonably close to being finished before I was interrupted by other things, I must really knock off the final interface.

Since to my knowledge there is no cryptographic verification of the packages beyond the checksum, I agree that this really is important.

felipenoris · 2015-10-17T12:04:17Z

awesome

LachlanGunn · 2015-10-17T12:10:54Z

Ok, so I have merged the changes forward and finally gotten around to making the protocol configurable---"https", "git", etc., or "" to make it not change anything. Default is HTTPS.

Moving to an apartment whose internet connection has git:// blocked gave me a bit of encouragement :)

After this, I'm planning to look at changing the git:// URLs in the build system to https://.

kmsquire · 2015-10-17T16:40:41Z

Just in case this isn't obvious from previous conversations about git vs https, as strange as it may seem, there have been reports of the opposite problem--that https (to GitHub at least) was blocked, but that the git protocol worked. So while one might be good as a default, it should be possible to use the other.

LachlanGunn · 2015-10-17T16:52:00Z

Yes, that capability is there---I have added a new function to Pkg.git that allows the protocol to be changed.

kmsquire · 2015-10-17T16:56:29Z

Cool, thanks for the clarification.

felipenoris · 2015-10-17T20:35:56Z

LibGit2?

wildart · 2015-10-17T21:10:28Z

base/pkg/cache.jl

@@ -2,7 +2,7 @@

 module Cache

-import ...LibGit2, ..Dir, ...Pkg.PkgError
+import ...LibGit2, ..Dir, ...Pkg.PkgError, ..Git


Do not use Git module. It will be removed. LibGit2 has normalization function.

LachlanGunn · 2015-10-18T11:26:38Z

I'm not sure that LibGit2 is the right place though, since it's supposed to be a wrapper library as I understand it, and adding a bunch of github-specific functionality seems a bit out of place. But if that's the consensus or I've misunderstood the purpose of LibGit2, then I'll go with the majority and put it there.

tkelman · 2015-10-18T23:42:16Z

Look around the LibGit2 module or the Pkg module (maybe in Pkg.Read?), there are already likely a few places that deal with urls.

wildart · 2015-10-19T02:47:32Z

base/pkg/git.jl

@@ -112,13 +114,27 @@ function set_remote_url(url::AbstractString; remote::AbstractString="origin", di
    run(`config remote.$remote.url $url`, dir=dir)
    m = match(GITHUB_REGEX,url)
    m === nothing && return
-    push = "[email protected]:$(m.captures[1]).git"
+    push = "$rewrite_url_to://[email protected]:$(m.captures[1]).git"


You should put git@ prefix only for git protocol.

LachlanGunn · 2015-10-27T16:51:48Z

Righto; thanks for all of your comments, I've tried to address them all in this next iteration.

The URL normalisation is now in Pkg.Cache, and the configuration function has been referenced into the top level Pkg module. What do you guys think?

tkelman · 2015-10-27T20:12:55Z

base/pkg/git.jl

@@ -105,20 +105,18 @@ function is_ancestor_of(a::AbstractString, b::AbstractString; dir="")
    readchomp(`merge-base $A $b`, dir=dir) == A
 end

-const GITHUB_REGEX =
-    r"^(?:git@|git://|https://(?:[\w\.\+\-]+@)?)github.com[:/](([^/].+)/(.+?))(?:\.git)?$"i
-
 function set_remote_url(url::AbstractString; remote::AbstractString="origin", dir="")


this version of the function is obsolete and subject to being deleted - see the version in libgit2.jl

First of all, no changes to git.jl, all modifications should go to libgit2.jl

Ok, I'll leave it in its previous state then. I was a bit antsy about the potentially inconsistent behaviour, even if it's slated for removal and not being used at the moment.

tkelman · 2015-10-27T20:15:03Z

On naming, I think Pkg.setprotocol! would be preferable to Pkg.set_git_proto

LachlanGunn · 2015-10-27T20:38:23Z

@tkelman Yes, you're right, I'm retrospect it isn't very idiomatic. I'll definitely be changing that.

LachlanGunn · 2015-10-28T13:07:02Z

I've reverted Pkg.Git to its original state now as well. Any further thoughts?

tkelman · 2015-10-28T13:29:55Z

base/pkg.jl

 export dir, init, rm, add, available, installed, status, clone, checkout,
-       update, resolve, test, build, free, pin, PkgError
+       update, resolve, test, build, free, pin, PkgError, set_git_proto


fix name here

Ack, that is really embarrassingly stupid! Sorry to keep wasting your time on such things.

no big deal, this is why code review is a thing

LachlanGunn · 2015-10-28T14:36:34Z

Righto, the proper function name has now been exported.

LachlanGunn · 2015-11-02T09:14:49Z

With those changes made, are there any further comments? Apologies for the bump.

tkelman · 2015-11-02T09:24:20Z

This looks good, the bump was warranted. I think we should document the new protocol setting API.

LachlanGunn · 2015-11-02T09:36:27Z

Righto, thanks---now that it's settled I'll do documentation/tests/etc. The fact that the URLs are going to be fiddled with is itself probably something to make clear in the documentation of PkgDev/METADATA as well.

wildart · 2015-11-03T07:26:35Z

LGTM

LachlanGunn · 2015-11-03T22:38:35Z

If PkgDev is depending on this shall I get this squashed up and make a second pull request for the tests?

tkelman · 2015-11-04T02:55:26Z

works for me

WIP: Rewrite git://github.com to https://github.com in package URLs.

LachlanGunn mentioned this pull request May 17, 2015

Pkg.add: provide a way to download over https instead of git #7005

Closed

mlubin reviewed May 17, 2015
View reviewed changes

tkelman added the packages Package management and loading label May 19, 2015

LachlanGunn changed the title ~~RFC: Rewrite git://github.com to https://github.com in package URLs.~~ WIP: Rewrite git://github.com to https://github.com in package URLs. May 19, 2015

felipenoris mentioned this pull request Oct 8, 2015

Release 0.4: make https protocol as default one for a new packages #13493

Merged

LachlanGunn closed this Oct 16, 2015

LachlanGunn reopened this Oct 16, 2015

LachlanGunn closed this Oct 16, 2015

LachlanGunn reopened this Oct 17, 2015

LachlanGunn changed the title ~~WIP: Rewrite git://github.com to https://github.com in package URLs.~~ RFC: Rewrite git://github.com to https://github.com in package URLs. Oct 17, 2015

wildart reviewed Oct 17, 2015
View reviewed changes

wildart reviewed Oct 19, 2015
View reviewed changes

LachlanGunn closed this Oct 27, 2015

LachlanGunn reopened this Oct 27, 2015

tkelman reviewed Oct 27, 2015
View reviewed changes

tkelman reviewed Oct 28, 2015
View reviewed changes

tkelman mentioned this pull request Nov 2, 2015

PkgDev.register failed with UndefVarError JuliaLang/PkgDev.jl#18

Closed

LachlanGunn closed this Nov 3, 2015

Added URL rewriting to Pkg.

ce2500e

LachlanGunn reopened this Nov 3, 2015

tkelman added a commit that referenced this pull request Nov 4, 2015

Merge pull request #11312 from LachlanGunn/master

8798aab

WIP: Rewrite git://github.com to https://github.com in package URLs.

tkelman merged commit 8798aab into JuliaLang:master Nov 4, 2015

LachlanGunn mentioned this pull request Nov 4, 2015

RFC: Added documentation and tests for Pkg.setprotocol #13879

Merged

tkelman mentioned this pull request Nov 14, 2015

Remove command-line Git from Windows and Mac binaries #13958

Closed

axsk mentioned this pull request Apr 28, 2016

Julia won't pull from https behind a corporate proxy #14167

Closed

WIP: Rewrite git://github.com to https://github.com in package URLs. #11312

WIP: Rewrite git://github.com to https://github.com in package URLs. #11312

Conversation

LachlanGunn commented May 17, 2015

Introduction

Changes

Test procedure

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keno commented May 17, 2015

simonster commented May 17, 2015

tkelman commented May 17, 2015

mikewl commented May 19, 2015

LachlanGunn commented May 19, 2015

aviks commented May 19, 2015

ihnorton commented May 19, 2015

LachlanGunn commented May 19, 2015

tkelman commented May 19, 2015

LachlanGunn commented May 19, 2015

ViralBShah commented May 20, 2015

simonster commented May 20, 2015

ScottPJones commented May 20, 2015

felipenoris commented Oct 8, 2015

LachlanGunn commented Oct 8, 2015

felipenoris commented Oct 17, 2015

LachlanGunn commented Oct 17, 2015

kmsquire commented Oct 17, 2015

LachlanGunn commented Oct 17, 2015

kmsquire commented Oct 17, 2015

felipenoris commented Oct 17, 2015

Choose a reason for hiding this comment

LachlanGunn commented Oct 18, 2015

tkelman commented Oct 18, 2015

Choose a reason for hiding this comment

LachlanGunn commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Oct 27, 2015

LachlanGunn commented Oct 27, 2015

LachlanGunn commented Oct 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LachlanGunn commented Oct 28, 2015

LachlanGunn commented Nov 2, 2015

tkelman commented Nov 2, 2015

LachlanGunn commented Nov 2, 2015

wildart commented Nov 3, 2015

LachlanGunn commented Nov 3, 2015

tkelman commented Nov 4, 2015