-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix mishandling of SSH port numbers in llb.Git #4069
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I get it correctly that when the custom port is set then the only valid format is ssh://git@github.com:port/user/repo.git
?
Could we get a test that covers this case to avoid regressions. Unit test just for parsing should be fine.
client/llb/source.go
Outdated
// don't do this transformation if the text after the : looks | ||
// like a port number. | ||
if len(parts) == 2 && | ||
(len(parts[1]) == 0 || parts[1][0] < '0' || parts[1][0] > '9') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be safer to do strconv.ParseInt
in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it be safer? I don't think it's quite the same thing, for example it would parse +22
as an int.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iiuc this currently checks the range for first letter after :
. Why can't a regular repo name start with a number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could, but how would we know it's supposed to be a repo name and not a port number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how to know it precisely, but I thought it would at least be safer to check all the letters than just the first one. I didn't consider that ParseInt
also accepts the sign.
I'm not sure I understand your question precisely, but I don't think that's correct. We parse the protocol out before this with I could try to handle URIs like |
That's what I was wondering as well. |
Added a test. Also made this work for URIs like |
3d99e08
to
d7afafc
Compare
I think we should defer to the git cli compatibility here if we can. We might want to split out the protocol in |
Great, this makes things simpler. I've updated the PR to drop support for both a port number and a colon-delimited path at the same time. |
d7afafc
to
6624adb
Compare
client/llb/source.go
Outdated
// check for a possible port number after the host | ||
if !isNumeric(hostParts[1]) { | ||
sshHost = hostParts[0] | ||
pathPrefix := strings.Join(hostParts[1:], "/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be no-op as hostParts[1:]
always only has one item because of the SplitN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was left over from when it handled multiple :
s. Removed.
In creating a host argument to pass into sshutil.SSHKeyScan, the current code loses the port number and will hang if packets to port 22 are blocked. A port number separated by a : should be preserved. Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
6624adb
to
eeb6f87
Compare
@tonistiigi: Does this look good now? |
{ | ||
remote: "ssh://github.com:22:moby/buildkit.git", | ||
expectedProtocol: gitutil.SSHProtocol, | ||
expectedSSHHost: "github.com", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected host here should be github.com:22:moby
if we want to follow the same behavior as git:
$ git clone ssh://git@github.com:moby:22/buildkit.git
Cloning into 'buildkit'...
ssh: Could not resolve hostname github.com:moby:22: Name or service not known
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
@aaronlehmann @tonistiigi what do you think of the approach in master...jedevc:buildkit:wip-gitutil? I had a bit of a play around. Essentially a lot of this issue seems to be that (I think this solves the original issue?) |
@jedevc: I really like that approach. Using |
Closing in favor of #4142 |
In creating a host argument to pass into
sshutil.SSHKeyScan
, the current code loses the port number and will hang if packets to port 22 are blocked. A port number separated by a:
should be preserved.