Refactor git ssh url parsing

[jedevc]

This is an alternative to the solution proposed in #4069, see #4069 (comment) (cc @aaronlehmann)

Wherever possible, we should parse SSH urls using url.Parse. See the updated tests for the exact changes in behavior:

• Don't strip the username during parsing (I'm not 100% sure why we did this in the first place - maybe I'm missing something obvious?)
• Normalize SCP-style git urls (username@host:org/repo) into ssh:// URLs.

[alexcb]

Don't strip the username during parsing (I'm not 100% sure why we did this in the first place - maybe I'm missing something obvious?)

This was done in order to construct a consistent ID (that's passed to NewState(...) that didn't depend on the protocol or username, e.g.

"https://github.com/moby/buildkit", "git@github.com/moby/buildkit", and "https://username:token@github.com/moby/buildkit" all refer to the same repo, and in order to re-use the cache they should all have the same ID.

This reason was ever-so-briefly described in the old code's comment:

			// keep remote consistent with http(s) version
			remote = parts[0] + "/" + parts[1]

There might additionally be a security concern now that remote contains a username/password (or in the case of github, they actually allow a token to be passed as the username) -- credentials shouldn't be stored in the cache ID.

[jedevc]

This was done in order to construct a consistent ID (that's passed to NewState(...) that didn't depend on the protocol or username

There might additionally be a security concern now that remote contains a username/password (or in the case of github, they actually allow a token to be passed as the username) -- credentials shouldn't be stored in the cache ID.

This should be ok I think - the cache key for git sources is computed from the git sha (see #2091)

buildkit/source/git/gitsource.go

Line 299 in 67e1e6d

func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index int) (string, string, solver.CacheOpts, bool, error) {

The identifier itself isn't used to compute the cache, so it should never end up in the cache id, it's only ever recorded in the op. Even then, we wouldn't be able to execute two different urls concurrently, since the vertex digests used for this are a checksum of the entire source definition (which will include the full URL).

I think we can keep the consistent ID, but I'm fairly sure that it won't actually change the caching/concurrent execution behavior.