Git SSH submodules that omit "ssh://" no longer work

[sburton84]

Problem

When using a Git submodule that uses the SSH protocol, Git allows the ssh:// to be omitted and the URL to be provided in a form such as git@github.com:.... Previously this worked fine with Cargo, but since some changes a couple of weeks ago related to handling submodules this seems to no longer work. I instead get an error such as the following:

Caused by:
  failed to load source for dependency `<redacted>`

Caused by:
  Unable to update ssh://git@github.com/<redacted>

Caused by:
  failed to update submodule `<redacted>`

Caused by:
  failed to parse url for submodule `<redacted>`: `git@github.com:<redacted>.git`

Caused by:
  relative URL without a base

As you can see it's failing to parse the git@github.com:... URL, it seems to think it's a relative URL, but it is not a relative URL, it is an absolute URL which doesn't specify the ssh:// protocol, which is a valid Git submodule URL, so should be supported.

I am already using git-fetch-with-cli = true in my .cargo/config.toml file to fix other issues with using SSH authentication to private repositories, but it doesn't seem to fix this issue.

Steps

1. Create a crate which contains a submodule which uses an SSH URL that omits the ssh:// prefix.
2. Depend on that crate from another crate.
3. Try to build this latter crate.

Possible Solution(s)

Any parsing of a Git submodule URL should allow the protocol to be omitted. It may be the call to Url::parse() added in the commit 2717a34 which has triggered this, so possibly revert that particular part of the change to use the string verbatim without parsing, as was done before.

Notes

No response

Version

Fails using nightly-2023-06-11 or onwards, nightly-2023-06-10 does not exhibit this issue. The version output on the earliest failing version is:

cargo 1.72.0-nightly (49b6d9e17 2023-06-09)
release: 1.72.0-nightly
commit-hash: 49b6d9e179a91cf7645142541c9563443f64bf2b
commit-date: 2023-06-09
host: x86_64-unknown-linux-gnu
libgit2: 1.6.4 (sys:0.17.1 vendored)
libcurl: 8.1.2-DEV (sys:0.4.63+curl-8.1.2 vendored ssl:OpenSSL/1.1.1t)
ssl: OpenSSL 1.1.1t  7 Feb 2023
os: Ubuntu 22.04 (jammy) [64-bit]

[weihanglo]

Thanks for the report. I believe it was an overlook on my side.
@krs98 are you williing to look into this and file a fix?

[sburton84]

One thing to add after some additional experimentation, I think the issue may be also because of the fact that this form of URL uses a : rather than a / to separate the host-name from the path, rather than just because of the ssh:// being omitted. After adding ssh:// to the beginning I then get an invalid port number error, I assume because it thinks the : indicates a port will follow, and I also need to change : to / before it seems to start working.

[chris-crespo]

Using : to separate the host from the path is not valid syntax according to the whatwg url spec. The git docs also show what syntaxes can be used:

The following syntaxes may be used with them:

• ssh://[user@]host.xz[:port]/path/to/repo.git/

And

An alternative scp-like syntax may also be used with the ssh protocol:

• [user@]host.xz:path/to/repo.git/

The issue here is that the url crate does not support scp-like syntax as it is not part of the whatwg spec (issue). This has affected cargo for some years (#1851). Removing the call to Url::parse() implies having to change the type of parent_remote_url from &Url to &str, and we would not be able to rely on url to handle appending relative paths anymore.

An alternative solution could be to convert a scp-like url to the equivalent starting with ssh://, which would probably work for most cases, but, as mentioned in the first linked issue, they are not exactly the same.

Thoughts @weihanglo?

[weihanglo]

Thanks for both of your investigations!

Oh no, the issue is way thornier than I thought 😞.

Using &str instead of &Url looks pretty reasonable but just for this issue. The only URL-related operation is adding trailing /, which seems trivial to manipulate with &str I feel like it.

In long-term, we may want to support scp-like URL, but maybe not now given Cargo doesn't support that for direct git dependencies either.

Adding a test for SSH might be a bit difficult. Thankfully ehuss already setup container tests infra for us. I guess you can take ssh::known_host_works test as a reference to create an SSH submodule on a scp-like URL.

cargo/tests/testsuite/ssh.rs

Lines 167 to 168 in b81b251

	#[cargo_test(container_test)]
	fn known_host_works() {

[chris-crespo]

The only URL-related operation is adding trailing /, which seems trivial to manipulate with &str I feel like it.

There is also the call to .join when the url is relative. Without this we cannot tell users when they write an invalid relative url.

[weihanglo]

Yep, though I expect it's less likely to have a broken relative URL for submodules, since we've checked ./ and ../ prefix already, so the remaining part is just be a relative path. I tested it a bit and it will error out with something like:

Caused by:
  failed to update submodule `submod`

Caused by:
  failed to fetch submodule `submod` from [bad-url...]

Caused by:
  failed to resolve path '[bad-url...]': No such file or directory; class=Os (2)

Yep it will try fetching it the fail. Not an optimal one though. Do you have any other example of “bad relative URL” that may lead to any confusing error message?

Also still open to have an SCP-like URL parser, if it is not too hard to have one.

[chris-crespo]

Alright, I will try to file a fix tomorrow.

@rustbot claim

[weihanglo]

@krs98 are you still around having time to work on it? I believe we need to backport to 1.72 beta branch if we manage to have a patch. Feel free to release your assignment if you have no time to file a patch. The command is @rustbot release-assignment.