8000 ssh: Omit port option from ssh command unless specified in remote url by jayong93 · Pull Request #6845 · libgit2/libgit2 · GitHub
[go: up one dir, main page]
Skip to content

ssh: Omit port option from ssh command unless specified in remote url #6845

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 22, 2024
Merged

ssh: Omit port option from ssh command unless specified in remote url #6845

merged 1 commit into from
Sep 22, 2024

Conversation

jayong93
Copy link
Contributor
@jayong93 jayong93 commented Jul 6, 2024 •
edited
Loading

Currently, when using GIT_SSH_EXEC feature, libgit2 doen't respect a Port config in ssh config because it always insert -p 22 option to a ssh command if there is no port value in a git remote url (e.g. git@my-custom-domain.org:user/path). I've experienced this for my company's GHE.

One caveat of this implementation is that if you specify port 22 in a remote url despite a different port in ssh config, it will be ignored.
For example, in ~/.ssh/config:

Host my-custom-domain.org
  Port 1234

and if you try to fetch or push with a remote url ssh://git@my-custom-domain.org:22/user/path, the port 1234 will be used.
It would be unlikely someone tries to do this though, I guess.

EDITED:
Now -p <port> option is added only when the port is specified in a remote url.

@ethomson
Copy link
Member

One caveat of this implementation is that if you specify port 22 in a remote url despite a different port in ssh config, it will be ignored.

Yeah, you're right. Looking at git, they specify -p <num> if and only if the port number is specified in the URL. IOW, ssh://host/path will honor Port in the config, while ssh://host:22/port will add -p 22 to the command-line to override that.

I added #6851 so that we could discern that as well. Does that make sense in this regard and can you check whether the port was specifically provided using that?

@jayong93
Copy link
Contributor Author

Yeah, you're right. Looking at git, they specify -p <num> if and only if the port number is specified in the URL. IOW, ssh://host/path will honor Port in the config, while ssh://host:22/port will add -p 22 to the command-line to override that.

I added #6851 so that we could discern that as well. Does that make sense in this regard and can you check whether the port was specifically provided using that?

That is what I wanted. I'll rebase this PR on #6851 and using that to check whether we should add -p option or not. Thank you!

@jayong93 jayong93 force-pushed the no-default-ssh-port branch 3 times, most recently from 35dccc3 to ce1d2c0 Compare July 15, 2024 05:47
ethomson
ethomson reviewed Aug 27, 2024
View reviewed changes
Copy link
Member
@ethomson ethomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry; I didn't notice that you'd pushed an update. One thing I notice is that I think that we can avoid an allocation of the port_str

src/libgit2/transports/ssh_exec.c Outdated
@@ -131,6 +131,7 @@ static int get_ssh_cmdline(
git_str ssh_cmd = GIT_STR_INIT;
const char *default_ssh_cmd = "ssh";
int error;
git_str port_str = GIT_STR_INIT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
git_str port_str = GIT_STR_INIT;

src/libgit2/transports/ssh_exec.c Outdated
url->username ? url->username : "",
url->username ? "@" : "",
url->host,
command,
url->path);

done:
git_str_dispose(&port_str);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
git_str_dispose(&port_str);

src/libgit2/transports/ssh_exec.c Outdated
Comment on lines 162 to 164
if (url->port_specified && (error = git_str_printf(&port_str, "-p %s", url->port)) < 0)
goto done;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (url->port_specified && (error = git_str_printf(&port_str, "-p %s", url->port)) < 0)
goto done;

src/libgit2/transports/ssh_exec.c Outdated
ssh_cmd.size > 0 ? ssh_cmd.ptr : default_ssh_cmd,
url->port,
port_str.ptr,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
port_str.ptr,
url->port_specified ? "-p " : "",
url->port_specified ? url->port : "",

@jayong93
ssh: Omit port option from ssh command unless specified in remote url
d2389b2 
Omit `-p 22` option from ssh command by default.
Adding `-p 22` option when a port is not in a remote url causes that
`Port` option in a ssh config is ignored.
@jayong93
Copy link
Contributor Author

@ethomson

Sorry; I didn't notice that you'd pushed an update. One thing I notice is that I think that we can avoid an allocation of the port_str

Right. I've removed the useless allocation.

@ethomson ethomson merged commit cb0bf67 into libgit2:main Sep 22, 2024
19 checks passed
@ethomson
Copy link
Member

Thanks for the fix! Great improvement.

@ethomson ethomson added the bug label Sep 29, 2024
@BrewTestBot BrewTestBot mentioned this pull request Dec 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethomson ethomson ethomson approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jayong93 @ethomson
0