oamg / leapp-repository

Leapp repositories containing actors for the Leapp framework (https://github.com/oamg/leapp). Currently provides leapp repositories for in-place upgrades of RHEL systems.
Apache License 2.0
52 stars 146 forks source link

OpenSSHConfigScanner: Include directive is supported since RHEL 8.6 #1212

Closed Jakuje closed 3 months ago

Jakuje commented 6 months ago

This issue could cause false positive reports when the user has the configuration options such as "Subsystem sftp" defined in included file only.

Resolves: RHEL-33902

github-actions[bot] commented 6 months ago

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable. If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build. If you need a different version of leapp, e.g. from PR#42, use /packit test oamg/leapp#42 Note that first time contributors cannot run tests automatically - they will be started by a reviewer.

It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported, beaker-minimal and kernel-rt, both can be used to be run on all upgrade paths or just a couple of specific ones. To launch on-demand tests with packit:

See other labels for particular jobs defined in the .packit.yaml file.

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra.

pirat89 commented 3 months ago

/packit copr-build

MichalHe commented 3 months ago

@Jakuje Could the glob expression in the Include directive contain spaces? For example

Include "/home/my configs/sshd_config"

In such a case the logic would be faulty as it would try to use the pattern /home/my

A solution in such a case would be to use (this ensures that only the leftmost whitespace will be used for splitting)

el = el.split(None, 1)
Jakuje commented 3 months ago

@Jakuje Could the glob expression in the Include directive contain spaces? For example

Include "/home/my configs/sshd_config"

In such a case the logic would be faulty as it would try to use the pattern /home/my

A solution in such a case would be to use (this ensures that only the leftmost whitespace will be used for splitting)

el = el.split(None, 1)

Good catch! You are right. I will change it.

From the manual page of sshd_config, it can also accept several filenames one one line so that would be also good to test if this needs some special handling.

Jakuje commented 3 months ago

el = el.split(None, 1)

reading through the code, this wont work as in some other cases we already depend on the rest being split by spaces (such as match, subsystem). The way how the splitting is implemented in OpenSSH

https://github.com/openssh/openssh-portable/blob/master/misc.c#L2044

is something like the shell expansions so using shlex should do.

Pushed the updated change.

MichalHe commented 3 months ago

/packit build

MichalHe commented 3 months ago

I have tested the patch using the setup described in RHEL-33902 and it works like a charm. Just waiting for the tests to finish and this can be merged.

MichalHe commented 3 months ago

/packit build