git: allow scp-style urls without username

According to the git docs:

> Or you can use the shorter scp-like syntax for the SSH protocol:
>
> 	$ git clone [user@]server:project.git

Previously, we were doing this potentially wrong - we were requiring the
presence of a username for all scp-style URLs. This could result in
disparity from the git CLI which successfully manages to parse
`github.com:moby/buildkit.git` (though cloning will generally fail, with
an ambiguous username).

However, we aim keep the behavior of git ref as before - this is because
making changes to this can effect the parsing of dockerfiles: where
before "foo:bar" would be detected as a local source, after, it would be
detected as a git repo (despite the fact it correctly parses as one). To
avoid this, we simply keep this behavior as before.

Signed-off-by: Justin Chadwell <[email protected]>

util/gitutil/git_ref.go

@@ -5,7 +5,6 @@ import (
 	"strings"
 
 	cerrdefs "github.com/containerd/errdefs"
-	"github.com/pkg/errors"
 )
 
 // GitRef represents a git ref.
@@ -67,10 +66,7 @@ func ParseGitRef(ref string) (*GitRef, error) {
 			Path:   strings.TrimPrefix(ref, "github.com/"),
 		})
 	} else {
-		remote, err = ParseURL(ref)
-		if errors.Is(err, ErrUnknownProtocol) {
-			return nil, err
-		}
+		remote, err = ParseURL(ref, NoImplicitSCPUsername())
 		if err != nil {
 			return nil, err
 		}

util/gitutil/git_ref_test.go

@@ -1,6 +1,7 @@
 package gitutil
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -88,6 +89,10 @@ func TestParseGitRef(t *testing.T) {
 				ShortName: "buildkit",
 			},
 		},
+		{
+			ref:      "github.com:moby/buildkit",
+			expected: nil,
+		},
 		{
 			ref: "[email protected]:moby/buildkit",
 			expected: &GitRef{
@@ -141,7 +146,7 @@ func TestParseGitRef(t *testing.T) {
 	}
 	for _, tt := range cases {
 		tt := tt
-		t.Run(tt.ref, func(t *testing.T) {
+		t.Run(strings.ReplaceAll(tt.ref, "/", "_"), func(t *testing.T) {
 			got, err := ParseGitRef(tt.ref)
 			if tt.expected == nil {
 				require.Nil(t, got)

util/gitutil/git_url.go

@@ -17,8 +17,9 @@ const (
 )
 
 var (
-	ErrUnknownProtocol = errors.New("unknown protocol")
-	ErrInvalidProtocol = errors.New("invalid protocol")
+	ErrUnknownProtocol  = errors.New("unknown protocol")
+	ErrInvalidProtocol  = errors.New("invalid protocol")
+	ErrImplicitUsername = errors.New("implicit username not permitted")
 )
 
 var supportedProtos = map[string]struct{}{
@@ -74,9 +75,25 @@ func splitGitFragment(fragment string) *GitURLFragment {
 	return &GitURLFragment{Ref: ref, Subdir: subdir}
 }
 
+type parseUrlOpts struct {
+	noImplicitSCPUsername bool
+}
+type ParseURLOpt func(*parseUrlOpts)
+
+func NoImplicitSCPUsername() ParseURLOpt {
+	return func(opts *parseUrlOpts) {
+		opts.noImplicitSCPUsername = true
+	}
+}
+
 // ParseURL parses a BuildKit-style Git URL (that may contain additional
 // fragment metadata) and returns a parsed GitURL object.
-func ParseURL(remote string) (*GitURL, error) {
+func ParseURL(remote string, opts ...ParseURLOpt) (*GitURL, error) {
+	opt := parseUrlOpts{}
+	for _, f := range opts {
+		f(&opt)
+	}
+
 	if proto := protoRegexp.FindString(remote); proto != "" {
 		proto = strings.ToLower(strings.TrimSuffix(proto, "://"))
 		if _, ok := supportedProtos[proto]; !ok {
@@ -90,19 +107,18 @@ func ParseURL(remote string) (*GitURL, error) {
 	}
 
 	if url, err := sshutil.ParseSCPStyleURL(remote); err == nil {
+		if opt.noImplicitSCPUsername && url.User == nil {
+			return nil, ErrImplicitUsername
+		}
 		return fromSCPStyleURL(url), nil
 	}
 
 	return nil, ErrUnknownProtocol
 }
 
-func IsGitTransport(remote string) bool {
-	if proto := protoRegexp.FindString(remote); proto != "" {
-		proto = strings.ToLower(strings.TrimSuffix(proto, "://"))
-		_, ok := supportedProtos[proto]
-		return ok
-	}
-	return sshutil.IsImplicitSSHTransport(remote)
+func IsGitTransport(remote string, opts ...ParseURLOpt) bool {
+	_, err := ParseURL(remote, opts...)
+	return err == nil
 }
 
 func fromURL(url *url.URL) *GitURL {

util/gitutil/git_url_test.go

@@ -2,6 +2,7 @@ package gitutil
 
 import (
 	"net/url"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -75,6 +76,15 @@ func TestParseURL(t *testing.T) {
 				User:   url.User("git"),
 			},
 		},
+		{
+			url: "github.com:moby/buildkit.git",
+			result: GitURL{
+				Scheme: SSHProtocol,
+				Host:   "github.com",
+				Path:   "moby/buildkit.git",
+				User:   nil,
+			},
+		},
 		{
 			url: "[email protected]:moby/buildkit.git",
 			result: GitURL{
@@ -153,7 +163,7 @@ func TestParseURL(t *testing.T) {
 		},
 	}
 	for _, test := range tests {
-		t.Run(test.url, func(t *testing.T) {
+		t.Run(strings.ReplaceAll(test.url, "/", "_"), func(t *testing.T) {
 			remote, err := ParseURL(test.url)
 			if test.err {
 				require.Error(t, err)

util/sshutil/scpurl.go

@@ -2,14 +2,18 @@ package sshutil
 
 import (
 	"errors"
-	"fmt"
 	"net/url"
 	"regexp"
 )
 
-var gitSSHRegex = regexp.MustCompile("^([a-zA-Z0-9-_]+)@([a-zA-Z0-9-.]+):(.*?)(?:#(.*))?$")
+var gitSSHRegex = regexp.MustCompile("^(?:([a-zA-Z0-9-_]+)@)?([a-zA-Z0-9-.]+):(.*?)(?:#(.*))?$")
 
 func IsImplicitSSHTransport(s string) bool {
+	if u, _ := url.Parse(s); u != nil && u.Scheme != "" && u.Opaque == "" {
+		// valid scp-urls do have an explicit scheme
+		return false
+	}
+
 	return gitSSHRegex.MatchString(s)
 }
 
@@ -22,20 +26,32 @@ type SCPStyleURL struct {
 }
 
 func ParseSCPStyleURL(raw string) (*SCPStyleURL, error) {
+	if u, _ := url.Parse(raw); u != nil && u.Scheme != "" && u.Opaque == "" {
+		return nil, errors.New("invalid scp-style url: scheme found")
+	}
+
 	matches := gitSSHRegex.FindStringSubmatch(raw)
 	if matches == nil {
-		return nil, errors.New("invalid scp-style url")
+		return nil, errors.New("invalid scp-style url: no match")
+	}
+
+	var user *url.Userinfo
+	if matches[1] != "" {
+		user = url.User(matches[1])
 	}
 	return &SCPStyleURL{
-		User:     url.User(matches[1]),
+		User:     user,
 		Host:     matches[2],
 		Path:     matches[3],
 		Fragment: matches[4],
 	}, nil
 }
 
 func (url *SCPStyleURL) String() string {
-	base := fmt.Sprintf("%s@%s:%s", url.User.String(), url.Host, url.Path)
+	base := url.Host + ":" + url.Path
+	if url.User != nil {
+		base = url.User.String() + "@" + base
+	}
 	if url.Fragment == "" {
 		return base
 	}

util/sshutil/scpurl_test.go

@@ -1,28 +1,31 @@
 package sshutil
 
 import (
+	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
 func TestIsImplicitSSHTransport(t *testing.T) {
-	require.False(t, IsImplicitSSHTransport("http://github.com/moby/buildkit"))
-	require.False(t, IsImplicitSSHTransport("github.com/moby/buildkit"))
-	require.False(t, IsImplicitSSHTransport("github.com:moby/buildkit.git"))
-	require.False(t, IsImplicitSSHTransport("helloworld.net"))
-	require.False(t, IsImplicitSSHTransport("[email protected]"))
-	require.False(t, IsImplicitSSHTransport("[email protected]/foo/bar.git"))
-	require.False(t, IsImplicitSSHTransport("bad:[email protected]:foo/bar.git"))
-	require.False(t, IsImplicitSSHTransport(""))
-	require.True(t, IsImplicitSSHTransport("[email protected]:moby/buildkit.git"))
-	require.True(t, IsImplicitSSHTransport("[email protected]:/srv/repos/weird/project.git"))
-	require.True(t, IsImplicitSSHTransport("[email protected]:path/to/repo.git/"))
-	require.True(t, IsImplicitSSHTransport("[email protected]:/to/really:odd:repo.git/"))
-	require.True(t, IsImplicitSSHTransport("[email protected]:/~/catnip.git/"))
+	assert.True(t, IsImplicitSSHTransport("github.com:moby/buildkit.git"))
+	assert.True(t, IsImplicitSSHTransport("[email protected]:moby/buildkit.git"))
+	assert.True(t, IsImplicitSSHTransport("[email protected]:/srv/repos/weird/project.git"))
+	assert.True(t, IsImplicitSSHTransport("[email protected]:path/to/repo.git/"))
+	assert.True(t, IsImplicitSSHTransport("[email protected]:/to/really:odd:repo.git/"))
+	assert.True(t, IsImplicitSSHTransport("[email protected]:/~/catnip.git/"))
+	assert.True(t, IsImplicitSSHTransport("weird:[email protected]:foo/bar.git"))
+
+	assert.False(t, IsImplicitSSHTransport("http://github.com/moby/buildkit"))
+	assert.False(t, IsImplicitSSHTransport("github.com/moby/buildkit"))
+	assert.False(t, IsImplicitSSHTransport("helloworld.net"))
+	assert.False(t, IsImplicitSSHTransport("[email protected]"))
+	assert.False(t, IsImplicitSSHTransport("[email protected]/foo/bar.git"))
+	assert.False(t, IsImplicitSSHTransport(""))
 
 	// explicit definitions are checked via isGitTransport, and are not implicit therefore this should fail:
-	require.False(t, IsImplicitSSHTransport("ssh://[email protected]:2222/root/my/really/weird/path/foo.git"))
+	assert.False(t, IsImplicitSSHTransport("ssh://[email protected]:2222/root/my/really/weird/path/foo.git"))
 }
 
 func TestParseSCPStyleURL(t *testing.T) {
@@ -42,6 +45,11 @@ func TestParseSCPStyleURL(t *testing.T) {
 			url: "ssh://[email protected]:22/moby/buildkit.git",
 			err: true,
 		},
+		{
+			url:  "github.com:moby/buildkit.git",
+			host: "github.com",
+			path: "moby/buildkit.git",
+		},
 		{
 			url:  "[email protected]:moby/buildkit.git",
 			host: "github.com",
@@ -57,7 +65,7 @@ func TestParseSCPStyleURL(t *testing.T) {
 		},
 	}
 	for _, test := range tests {
-		t.Run(test.url, func(t *testing.T) {
+		t.Run(strings.ReplaceAll(test.url, "/", "_"), func(t *testing.T) {
 			remote, err := ParseSCPStyleURL(test.url)
 			if test.err {
 				require.Error(t, err)