-
Notifications
You must be signed in to change notification settings - Fork 331
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
ipa-client-install: add support for sss_ssh_knownhosts #7254
Conversation
@f-trivino the failure of test_commands is related to feb 29, please see 9548. Re-run the test tomorrow and it should succeed. |
@flo-renaud thank you for the pointer! Yeah, I noticed that the error wasn't related to the PR. I'll rerun it tomorrow so I can test this code "Without" sss_ssh_knownhosts feature (sssd-2-10), then will wait till sss_ssh_knownhosts is in place and re-run again so I test the "With". |
Looks like there is a usage error in test_integration/test_commands.py::TestIPACommand::test_sss_ssh_authorizedkeys and est_integration/test_sssd.py::TestNestedMembers::test_nested_group_members Usage: sss_ssh_knownhosts [-?] [-?|--help] [--usage] [-d|--domain=STRING] I wonder if the credentials revoked failure in test_integration/test_commands.py::TestIPACommand::test_reset_password_unlock is related to too many failed authentications. |
8e789a9
to
05591ca
Compare
05591ca
to
10e375d
Compare
thanks @rcritten , thanks to @aplopez I found out that the new arg for KnownHostsCommand is: "%H" instead of "-p %p %h". Fixed now. |
LGTM from the SSSD side. |
10e375d
to
654b987
Compare
Tests from temp_commit are passing. I'm adding links here just for reference: sssd-fedora/build — (^_^)/ |
654b987
to
7f75fc9
Compare
I have a worry about upgrades. What if ipa-client gets upgraded before the sssd-common package. In that case, the old ssh_ipa.conf will persist in the system even after sssd is updated, and the new sss_ssh_knownhosts file is present. |
The spec file can use |
468484a
to
e9136e6
Compare
For this particular case, I think it could be better to user file triggers ( https://rpm-software-management.github.io/rpm/manual/file_triggers.html
This example script is executed every time freeipa-client is installed and there is a file in /usr/bin (always), but it is also executed when another RPM installs a file in /usr/bin. It is not possible to know which file was installed and it can be invoked more than once. Documentation always mentions file prefixes as condition. I tried with the full file path and it worked for RPM removal ( |
e9136e6
to
d207f52
Compare
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.
Hi @f-trivino,
Please find inline comments.
d207f52
to
fa0b636
Compare
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.
Hi @f-trivino
thanks for the PR. I have 2 minor nitpicks (line length) but otherwise the fix looks good to me. I tested upgrade of ipa-client then sssd-client or sssd-client then ipa-client, as well as downgrade of sssd-client, we have the expected output.
New installation also properly handles the sssd version.
ipaclient/install/client.py
Outdated
) | ||
|
||
enableproxy = bool( | ||
options.sssd and os.path.isfile(paths.SSS_SSH_KNOWNHOSTSPROXY) and not enableknownhosts # noqa: E501 |
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.
Please add a line break instead of ignoring the linter error
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.
fixed
ipaclient/install/client.py
Outdated
enableproxy = bool( | ||
options.sssd and os.path.isfile(paths.SSS_SSH_KNOWNHOSTSPROXY) | ||
options.sssd and os.path.isfile(paths.SSS_SSH_KNOWNHOSTSPROXY) and not enableknownhosts # noqa: E501 |
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.
Please add a line break instead of ignoring the linter error
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.
fixed
fa0b636
to
c8e9cf7
Compare
thanks for testing all use cases. I do agree that lines can be broken while maintaining readability, fixed. |
thanks @aplopez , I finally opted for triggerin client -- sssd-common < 2.10 as filetriggerin would react for all file changes in /usr/bin |
c8e9cf7
to
c5ae58d
Compare
sss_ssh_knownhostsproxy will be deprecated in favor of sss_ssh_knownhosts. With this update, if the file /usr/bin/sss_ssh_knownhosts is present, KnownHostsCommand will be used instead of ProxyCommand. Also, GlobalKnownHostsFile is disabled as it is no longer needed. Fixes: https://pagure.io/freeipa/issue/9536 Signed-off-by: Francisco Trivino <[email protected]>
sss_ssh_knownhostsproxy will be deprecated in favor of sss_ssh_knownhosts. This commit implements a mechanism to apply the change when upgrading from older versions. Fixes: https://pagure.io/freeipa/issue/9536 Signed-off-by: Francisco Trivino <[email protected]>
c5ae58d
to
829dd45
Compare
sss_ssh_knownhostsproxy will be deprecated in favor of sss_ssh_knownhosts. With this update, if the file /usr/bin/sss_ssh_knownhosts is present, it will be used instead of /usr/bin/sss_ssh_knownhostsproxy. This commit also implements a mechanism to apply the change when upgrading from older versions.
Fixes: https://pagure.io/freeipa/issue/9536