Created attachment 127592 [details] [review]
Add nss and pam sssd.conf services after join

Merged into git master.

(In reply to Stef Walter from comment #2)
> Merged into git master.

Stef, seems that this commit broke SSSD on F26. Calling realm join "ad.example" generates a sssd.conf without the "services = " line.

The ISO I've used to test is: https://kojipkgs.fedoraproject.org/compose/branched/Fedora-26-20170420.n.0/compose/Server/x86_64/iso/Fedora-Server-dvd-x86_64-26-20170420.n.0.iso

Created attachment 131010 [details] [review]
Revert 402cbab

Created attachment 131011 [details] [review]
service: Add "pam" and "nss" services uing realm_ini_config_set_list_diff()

(In reply to Fabiano Fidêncio from comment #3)
> (In reply to Stef Walter from comment #2)
> > Merged into git master.
> 
> Stef, seems that this commit broke SSSD on F26. Calling realm join
> "ad.example" generates a sssd.conf without the "services = " line.
> 
> The ISO I've used to test is:
> https://kojipkgs.fedoraproject.org/compose/branched/Fedora-26-20170420.n.0/
> compose/Server/x86_64/iso/Fedora-Server-dvd-x86_64-26-20170420.n.0.iso

Stef, I'm proposing reverting the pushed patch and providing a quite simple patch that also would handle well adding the "nss" and "pam" services to sssd.conf in case such line already exists and also doesn't change the old behavior.

It was tested on a f26 local build.

Thanks for the patches. But these two commits when taken together, break the original case that was fixed here. Why are you reverting the changes to the IPA provider?

$ sudo cat /etc/sssd/sssd.conf

[sssd]
domains = 
config_file_version = 2
services = sudo

$ sudo realm join -v cockpit.lan
... IPA domain join ...

$ getent passwd admin@cockpit.lan
< no output>

$ sudo cat /etc/sssd/sssd.conf
[domain/cockpit.lan]
...

[sssd]
domains = cockpit.lan
config_file_version = 2
services = sudo, ssh

(In reply to Stef Walter from comment #7)
> Thanks for the patches. But these two commits when taken together, break the
> original case that was fixed here. Why are you reverting the changes to the
> IPA provider?

The main reason was because there's no detailed explanation about it (neither in the bug or in the commit message).

> 
> $ sudo cat /etc/sssd/sssd.conf
> 
> [sssd]
> domains = 
> config_file_version = 2
> services = sudo
> 
> $ sudo realm join -v cockpit.lan
> ... IPA domain join ...
> 
> $ getent passwd admin@cockpit.lan
> < no output>
> 
> $ sudo cat /etc/sssd/sssd.conf
> [domain/cockpit.lan]
> ...
> 
> [sssd]
> domains = cockpit.lan
> config_file_version = 2
> services = sudo, ssh

I will test whether the last patch is enough for our cases (and then the tests part has to be reverted) and re-submit it. Okay?

(In reply to Fabiano Fidêncio from comment #8)
> I will test whether the last patch is enough for our cases (and then the
> tests part has to be reverted) and re-submit it. Okay?

Yup, that sounds good.

Created attachment 131013 [details] [review]
service: Add "pam" and "nss" services in realm_sssd_config_add_domain()

Hopefully this patch is enough for both of us.

I've checked yours and mine use cases and both seems to work. But, as usual, I'd prefer to have a confirmation from you that your part was not broken by this patch.

Created attachment 131015 [details] [review]
FIXUP: Actually save the changes to the services line

I had to make these further changes to get things to work. Can you double check if they work for you? If they do, please squash these changes into your patch.

Created attachment 131017 [details] [review]
service: Add "pam" and "nss" services in realm_sssd_config_add_domain()

Squashed your fix up patch into mine as my use case works properly here.

Thanks. Merged into git master.

Attachment 131017 [details] pushed as 9d5b6f5 - service: Add "pam" and "nss" services in realm_sssd_config_add_domain()