SSSD on IPA should fail with short names#8261
Conversation
There was a problem hiding this comment.
Code Review
This pull request correctly implements a check to prevent SSSD from starting in IPA server mode when full_name_format is set to use only short names. The accompanying test case is also well-written and verifies the new behavior. My only suggestion is to add a system log message for the startup failure to improve visibility for administrators, as the current implementation only logs to the debug log.
|
Hi, thank you for the patch. I think Gemini's comment is valid and I wonder if you can combine the new check with the existing one in an While checking the code I found that A second, not related item, bye, |
00d9796 to
4909749
Compare
|
Hi @sumit-bose , I fixed the The other two items I did as separate commits. |
4909749 to
2e4db1c
Compare
src/tests/system/tests/test_ipa.py
Outdated
| 1. SSSD must refuse to start | ||
| :customerscenario: False | ||
| """ | ||
| cfg = "[domain/ipa.test]\nfull_name_format = %1$s\n" |
There was a problem hiding this comment.
Hi,
the IPA domain might not be called ipa.test in all test environment, I guess it would be better to use ipa.domain instead or the framework functions which can modify sssd.conf directly.
Is there a reason for using a config snippet?
bye,
Sumit
There was a problem hiding this comment.
Good point, updated.
Regarding snippets - yes it is needed here. The main config is somehow overwritten/managed by topology. I did not look into details tough. This was the easiest way to set it.
2e4db1c to
a641592
Compare
src/tests/system/tests/test_ipa.py
Outdated
|
|
||
| @pytest.mark.importance("low") | ||
| @pytest.mark.topology(KnownTopology.IPA) | ||
| def test_ipa__prohibited_short_names(client: Client, ipa: IPA): |
There was a problem hiding this comment.
The client: Client parameter is not used anywhere, please remove it.
src/tests/system/tests/test_ipa.py
Outdated
| """ | ||
| cfg = f"[domain/{ipa.domain}]\nfull_name_format = %1$s\n" | ||
| ipa.fs.write("/etc/sssd/conf.d/shortname.conf", cfg, mode="640") | ||
| result = ipa.host.conn.exec(["systemctl", "restart", "sssd"], raise_on_error=False) |
There was a problem hiding this comment.
Instead of raw command, we can use this, result = ipa.sssd.restart(raise_on_error=False, apply_config=False)
src/tests/system/tests/test_ipa.py
Outdated
| :customerscenario: False | ||
| """ | ||
| cfg = f"[domain/{ipa.domain}]\nfull_name_format = %1$s\n" | ||
| ipa.fs.write("/etc/sssd/conf.d/shortname.conf", cfg, mode="640") |
There was a problem hiding this comment.
Separate the code with a blank line, following the setup, step, and result.
a641592 to
cde7d34
Compare
|
Thanks to Madhuri's comments I also found a way how to do the test without snippet, lets wait for CI to see if it works. |
cde7d34 to
1b27317
Compare
|
@madhuriupadhye, take a look at the test updates. ci-failures are not related to this change. |
madhuriupadhye
left a comment
There was a problem hiding this comment.
LGTM, thank you!!
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
thank you for the update, test look much nicer now, ACK.
bye,
Sumit
|
Hi, I restarted the two failed test to see if it works better now and will set 'Accepted' when they are done. bye, |
SSSD in server mode on IPA server is not allowed to use short names. Reviewed-by: Madhuri Upadhye <mupadhye@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Reviewed-by: Madhuri Upadhye <mupadhye@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
This format is not used anywhere except in unnecessary check. This update removes it. Reviewed-by: Madhuri Upadhye <mupadhye@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Reviewed-by: Madhuri Upadhye <mupadhye@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
1b27317 to
42be2d4
Compare
SSSD should refuse to start when SSSD is in server mode and
full_name_formatis set to%1s$.