Skip to content
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

Add ssh_config equivalents of -N, -s, -n and -f #231

Closed
wants to merge 3 commits into from

Conversation

vog
Copy link

@vog vog commented Feb 26, 2021

Dear OpenSSH developers,

I kindly ask you to review the attached set of patches which introduce ssh_config equivalents of the flags -N, -s, -n and -f in a straight forward way:

  • NoShell for -N
  • SessionType for -N and -s
  • StdinNull for -n
  • ForkAfterAuthentication for -f

The ssh_config names are corresponding directly to the internal flag names. The man pages ssh(1) and ssh_config(5) are adjusted accordingly.

As a final remark, I noticed that the variable ono_shell_flag (not to be confused with no_shell_flag) is only assigned once and never used. So I assume this is dead code and I'm proposing a patch to remove it.
(Update: ono_shell_flag was actually meant to be used, see 5953c14)

Related mailing list threads:

Best regards,
Volker

johnsonjh added a commit to johnsonjh/j-hpn-ssh that referenced this pull request Feb 26, 2021
openssh#231
openssh@6c6813f
openssh@de28eef
openssh@aac32c9
openssh@0f7dd4d
https://marc.info/?l=openssh-unix-dev&m=147248154926769
https://marc.info/?l=openssh-unix-dev&m=161301825023655
https://marc.info/?l=openssh-unix-dev&m=161387765919633

Description (from @vog):

Dear OpenSSH developers,

I kindly ask you to review the attached set of
patches which introduce ssh_config equivalents
of the flags -N, -n and -f in a
straight-forward way:

    NoShell for -N
    StdinNull for -n
    ForkAfterAuthentication for -f

The ssh_config names were derived directly from
the internal flag names, e.g. no_shell_flag was
moved to option.no_shell and hence the
ssh_config name became NoShell.

The man pages ssh(1) and ssh_config(5) are
adjusted accordingly.

As a final remark, I noticed that the variable
ono_shell_flag (not to be confused with
no_shell_flag) is only assigned once and never
used. So I assume this is dead code and I'm
proposing a patch to remove it.
@vog vog force-pushed the ssh-config-improvements branch from 6c6813f to 792101d Compare March 9, 2021 00:03
@vog
Copy link
Author

vog commented Mar 9, 2021

Rebased to latest master.

@vog
Copy link
Author

vog commented Mar 17, 2021

Rebased again to latest master.

@vog
Copy link
Author

vog commented Mar 25, 2021

Rebased again to latest master.

@vog
Copy link
Author

vog commented May 4, 2021

Rebased yet another time to latest master.

@vog vog force-pushed the ssh-config-improvements branch from a20217b to b50aeb2 Compare June 10, 2021 15:30
@vog
Copy link
Author

vog commented Jun 10, 2021

Rebased to latest master, removing the commit that removed ono_shell_flag, as this variable is no longer unused (see 5953c14)

@vog
Copy link
Author

vog commented Jun 10, 2021

Feedback from Damien Miller via mailing list:

I think SessionType=none/subsystem/default is the way to go. I'll
have some time to look at it next week, but if you want to implement
it then please don't let me stop you :)

Thanks again for your patience.

-d

@vog vog force-pushed the ssh-config-improvements branch from b50aeb2 to b9e0e25 Compare June 10, 2021 16:34
@vog
Copy link
Author

vog commented Jun 10, 2021

Implemented SessionType instead of NoShell. That way, we also have an ssh_option that is equivalent to -s.

@vog vog changed the title Add ssh_config equivalents of -N, -n and -f Add ssh_config equivalents of -N, -s, -n and -f Jun 10, 2021
mux.c Outdated
@@ -71,7 +71,6 @@
/* from ssh.c */
extern int tty_flag;
extern Options options;
extern int stdin_null_flag;
extern char *host;
extern int subsystem_flag;
Copy link

Choose a reason for hiding this comment

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

Should extern int subsystem_flag be removed too?

Copy link
Author

Choose a reason for hiding this comment

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

Of course! Fixed.

ssh_config.5 Outdated
is run in the background.
A common trick is to use this to run X11 programs on a remote machine.
For example,
.Ic ssh shadows.cs.hut.fi emacs &
Copy link

Choose a reason for hiding this comment

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

Should this say ssh -n ...? Or ssh -o StdinNull=yes ...?

Otherwise, you've silently assumed the reader has put this option in ~/.ssh/config for Host shadows.cs.hut.fi (but they may not wish to do so for every session to that host)

Copy link
Author

Choose a reason for hiding this comment

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

I took that example from the ssh.1 manpage where "ssh -n" is described.

I agree that it doesn't fit well here.

I'm just leaving this out of ssh_config.5, the description is perfectly fine without it.

@@ -683,6 +683,45 @@ Valid options are:
and
.Cm sha256
(the default).
.It Cm ForkAfterAuthentication
Copy link

Choose a reason for hiding this comment

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

Should these new options also be listed in ssh.1 under -o option?

Copy link
Author

Choose a reason for hiding this comment

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

Indeed! I missed that place in the manpages. Fixed.

@vog
Copy link
Author

vog commented Jul 6, 2021

Thanks for the feedback! I'll have a look at it this evening.

@vog vog force-pushed the ssh-config-improvements branch from b9e0e25 to 8f31723 Compare July 6, 2021 20:04
@vog
Copy link
Author

vog commented Jul 6, 2021

I just improved the pull request by rebasing it to the latest master and applying all feedback received via mailing list and in this GitHub issue so far.

@djmdjm
Copy link
Contributor

djmdjm commented Jul 9, 2021

FYI these patches are all out for review upstream

@vog
Copy link
Author

vog commented Jul 9, 2021

Thanks!

@djmdjm
Copy link
Contributor

djmdjm commented Jul 13, 2021

SessionType merged as eda8909

@vog
Copy link
Author

vog commented Jul 21, 2021

@djmdjm Thanks for the notification! What is the status of the other 2 commits? Should I rebase them, or anything?

@djmdjm
Copy link
Contributor

djmdjm commented Jul 23, 2021

All committed now (as a917e97 and e0c5088) - thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants