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

Ignore comments in ssh_config Include parsing #1199

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

remil1000
Copy link

When parsing Include directive in ssh_config files, comments on the same line as the directive are considered as files to be recursively processed. Best case the recursive max_depth=16 terminates this early, but on old versions it may lead to random reads or an infinite recursion, hanging the shell process

Example before/after:

A minimal ssh client config file

$ cat config 
Include foo bar .ssh/baz # do not include

The current sed expression

$ sed -ne 's/^[[:blank:]]*[Ii][Nn][Cc][Ll][Uu][Dd][Ee][[:blank:]]\(.*\)$/\1/p' config 
foo bar .ssh/baz # do not include

Proposed change to sed expression (ignore anything coming after a #)

$ sed -ne 's/^[[:blank:]]*[Ii][Nn][Cc][Ll][Uu][Dd][Ee][[:blank:]]\([^#]*\).*$/\1/p' config
foo bar .ssh/baz 

Copy link
Owner

@scop scop left a comment

Choose a reason for hiding this comment

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

Thanks!

In addition to the requested test suite update, please search for "conventional commits" in CONTRIBUTING.md for info how to format the commit message and modify it accordingly (+ force-push with it).

Copy link
Owner

Choose a reason for hiding this comment

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

Let's amend our test suite to cover for this case, see test/t/unit/test_unit_known_hosts.py

Copy link
Author

@remil1000 remil1000 Jun 4, 2024

Choose a reason for hiding this comment

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

  • reworked the sed expression to remove trailing spaces
  • test suite amended (hopefully I understood it well)
  • commit message updated also

also I realized we may be in murky waters at this inline/trailing comment support has only been gradually added to sshd_config then ssh_config around SSH version 8.5 (sshd_config - https://bugzilla.mindrot.org/attachment.cgi?id=3460 and ssh_config https://bugzilla.mindrot.org/show_bug.cgi?id=2320)

So for example code

_comp_split hosts "$(command sed -ne 's/^[[:blank:]]*[Hh][Oo][Ss][Tt][[:blank:]=]\{1,\}\(.*\)$/\1/p' "${config[@]}")"; then

in block https://github.com/scop/bash-completion/blob/main/bash_completion#L2646-L2652 may need changes also to handle those inline comments

@remil1000 remil1000 force-pushed the main branch 2 times, most recently from 57561fb to 663255d Compare June 4, 2024 06:34
Copy link
Collaborator

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

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

Thanks for updating.

@@ -127,6 +126,8 @@ def test_included_configs(self, bash, hosts):
expected.extend("asterisk_1 asterisk_2".split())
# fixtures/_known_hosts/.ssh/config_question_mark
expected.append("question_mark")
# fixtures/_known_hosts/.ssh/config_comment
expected.append("with_comment")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The test seems to require the hostname found by the Include directive with inline comments, but the test doesn't seem to require that the words in the comment are excluded.

Copy link
Author

Choose a reason for hiding this comment

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

the original fix is to discard inline comments after an Include directive

if the included file would contain Host with_comment # to be ignored then this patch would also require to update the Host directive parsing sed expression (which I already tried) only to find out it breaks https://github.com/scop/bash-completion/blob/main/test/fixtures/_known_hosts/config#L9 Host = hus%%eth0 !negated #not-a-comment as until now fixtures considered inline comments as actual content to be parsed

This is precisely why I only patched the Include parsing expression and mentioned this future Host issue, because tests would fail one after the other

Copy link
Collaborator

@akinomyoga akinomyoga Jun 5, 2024

Choose a reason for hiding this comment

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

I think the fixture for Host should also be updated.

Edit: The original fix for sshd_config seems to be 20 years old: openbsd/src@5541d00, and then for ssh_config, openbsd/src@dd1a025. This is an explicit change in the upstream parsing, so we can update the fixture to match the upstream.


By the way, I started this "conversation" (in the GitHub term) to discuss what should be tested. You've modified the test, but it doesn't seem to test the expectation by the original fix for Include. Could you test that a word that matches a filename in the comment is excluded? You can add another file in the fixture and put a hostname that should not be collected there. Then, you can include the filename in the inline comment on a Include line.

test/t/unit/test_unit_known_hosts.py Outdated Show resolved Hide resolved
@@ -2447,7 +2447,8 @@ _comp__included_ssh_config_files()
# https://github.com/openssh/openssh-portable/blob/5ec5504f1d328d5bfa64280cd617c3efec4f78f3/readconf.c#L2240
local max_depth=16
while ((${#included[@]} > 0 && depth++ < max_depth)); do
_comp_split include_files "$(command sed -ne 's/^[[:blank:]]*[Ii][Nn][Cc][Ll][Uu][Dd][Ee][[:blank:]]\(.*\)$/\1/p' "${included[@]}")" || return
_comp_split include_files "$(command sed -n -e 's/[[:blank:]]*#.*//' -e 's/^[[:blank:]]*[Ii][Nn][Cc][Ll][Uu][Dd][Ee][[:blank:]]*\(..*\)/\1/p' "${included[@]}")" || return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remove the trailing/preceding whitespaces? Those are stripped by _comp_split anyway. Then, the sed expression can be simplified:

Suggested change
_comp_split include_files "$(command sed -n -e 's/[[:blank:]]*#.*//' -e 's/^[[:blank:]]*[Ii][Nn][Cc][Ll][Uu][Dd][Ee][[:blank:]]*\(..*\)/\1/p' "${included[@]}")" || return
_comp_split include_files "$(command sed -ne 's/^[[:blank:]]*[Ii][Nn][Cc][Ll][Uu][Dd][Ee][[:blank:]]*\([^#]*\).*$/\1/p' "${included[@]}")" || return

Copy link
Author

Choose a reason for hiding this comment

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

this is mainly because I discovered OpenSSH maintainers, in their parser, are actually proceeding like this, simply stopping processing as soon as they hit a # sign and trimming

	/* Remove comment and trailing whitespace */
	if ((cp = strchr(orig, '#')) != NULL)
		*cp = '\0';
	rtrim(orig);

https://github.com/openbsd/src/blob/master/usr.bin/ssh/readconf.c#L3229-L3232

I can revert to your proposition, which, incidentally is very similar to my first proposal in the PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the result change with this implementation detail?

@akinomyoga
Copy link
Collaborator

So for example code

_comp_split hosts "$(command sed -ne 's/^[[:blank:]]*[Hh][Oo][Ss][Tt][[:blank:]=]\{1,\}\(.*\)$/\1/p' "${config[@]}")"; then

in block https://github.com/scop/bash-completion/blob/main/bash_completion#L2646-L2652 may need changes also to handle those inline comments

I agree. We want to apply the changes consistently for similar cases.

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

Successfully merging this pull request may close these issues.

3 participants