URL: https://github.com/SSSD/sssd/pull/530 Author: CendioOssman Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive Action: opened
PR body: """ ThinLinc is a remote desktop server, very similar to the Remote Desktop Services referenced in the Active Directory description for this policy. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/530/head:pr530 git checkout pr530
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371759075
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371759076
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
CendioOssman commented: """ Any backports of this to stable branches would also be nice as the failure mode is rather confusing (the default logging doesn't state why the account is denied login). """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371759313
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
fidencio commented: """ ok to test """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371763136
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
mzidek-rh commented: """ Hello,
it is not necessary to change the internal defaults to support ThinLinc. The mapping of PAM services to GPO rules is configurable in the domain section of sssd.conf. So users of ThinLinc should have this in sssd.conf: ``` ad_gpo_map_remote_interactive = +thinlinc ``` See man sssd-ad for more details.
Michal """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371765921
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
CendioOssman commented: """ Right, we found that. But it's hard to discover that you have to do that, and it would be better if things just worked by default. """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371766870
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
mzidek-rh commented: """ Hello,
I do not think this is a good way to approach this issue. There can be hundreds of different PAM services that could be mapped to different gpo rules. We do not want to list them all in man pages and put them as defaults. It would actually worsen the situation because many admins would not want to use most of the services (I think this is also the case for thinlinc pam service) and would end up removing them from the defaults using the '-pam_service_name' syntax in the gpo_map_* options. This is exactly why the mappings are configurable.
If it was difficult to find out the documentation for gpo_map_* settings then I think the real issue is that our documentation is not easy to use (and unfortunately I do think this is the case and we need to improve this). Could you suggest what we could do to improve the documentation? Was there something specific that was confusing or something that you would like to add? I really do not know how people use the documentation or how they look for it (man, google, blog posts?). Your feedback in this area would be valuable.
Thanks, Michal """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371785740
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
CendioOssman commented: """
I do not think this is a good way to approach this issue. There can be hundreds of different PAM services that could be mapped to different gpo rules. We do not want to list them all in man pages and put them as defaults. It would actually worsen the situation because many admins would not want to use most of the services (I think this is also the case for thinlinc pam service) and would end up removing them from the defaults using the '-pam_service_name' syntax in the gpo_map_* options. This is exactly why the mappings are configurable.
I don't fully agree with you here. This setting is about mapping what kind of service each PAM service is. That is a statement of fact, not policy. Yes, people might use that to also control which services are allowed but that is not its stated purpose. So from that point of view this list should ideally include every service out there, properly mapped to the correct category.
If it was difficult to find out the documentation for gpo_map_* settings then I think the real issue is that our documentation is not easy to use (and unfortunately I do think this is the case and we need to improve this). Could you suggest what we could do to improve the documentation? Was there something specific that was confusing or something that you would like to add? I really do not know how people use the documentation or how they look for it (man, google, blog posts?). Your feedback in this area would be valuable.
The issue is with the logging. The man page was fairly clear once I figured out what to look for.
The standard error message gives absolutely no hint as to why access is denied:
``` Mar 8 15:13:51 ubuntu1604 pamtester: pam_sss(thinlinc:account): Access denied for user ossman: 6 (Permission denied) ```
The debug message at least says that it has to do with GPOs and services, but no clue beyond that:
``` (Thu Mar 8 15:13:51 2018) [sssd[be[lab.lkpg.cendio.se]]] [ad_gpo_access_send] (0x0400): service thinlinc maps to Denied ```
I would have preferred if the standard log message informed me that access was denied because the service thinlinc is not mapped to any GPO rule. """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371791167
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
mzidek-rh commented: """ I opened https://pagure.io/SSSD/sssd/issue/3664 to track the logging issue. Thank you @CendioOssman for this feedback.
About putting the thinlinc PAM service to the default mapping, I still think it should not be there for the reasons I mentioned before (and I really do not like the idea of having all possible PAM services in the default maps, IMO there should be only PAM services available by default in common distributions / or really common PAM services), but I may be wrong and would like to ask other team members about their opinion on this. So, @jhrozek @pbrezina @sumit-bose @fidencio , what do you guys think about this?
Michal """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371808526
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
simo5 commented: """ We may think of a mechanism to add a comment or a file somewhere we can read, so that software can distribute their GPO mappings in their RPMs/DEBs/etc.. instead of us having to patch SSSD all the time or admins having to list mapping on their own. """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371836359
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
CendioOssman commented: """ I saw a `conf.d/` on RHEL 7, but not on Ubuntu 16.04. That could be a way. But it seems that `ad_gpo_map_remote_interactive` cannot be changed in the global section? """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371837986
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
fidencio commented: """ @simo5, @CendioOssman,
Dropping a config snippet in /etc/sssd/conf.d/ with the option would be the way to go ... and I'd like to see it being done by, for instance, the package that wants/needs this option (thinlinc in this case).
However, I can see at least one issue here that has to be solved by SSSD before actually relying on it (or please, someone correct me in case I'm mistaken): The domain section usually looks like: [domain/<whatever you want to put here>] ... and it may be problematic ... as the package wouldn't know what's the <whatever you want to put here> to create a functional snippet.
I'll wait for the opinion from other developers and then we can open a bug and have a discussion (rather there, in the issue, than here, in order to keep it easily tracked) about what would be the easiest/fastest way to have it done.
Does this make sense? """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371863363
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
fidencio commented: """ @simo5, @CendioOssman,
Dropping a config snippet in /etc/sssd/conf.d/ with the option would be the way to go ... and I'd like to see it being done by, for instance, the package that wants/needs this option (thinlinc in this case).
However, I can see at least one issue here that has to be solved by SSSD before actually relying on it (or please, someone correct me in case I'm mistaken): The domain section usually looks like: [domain/"<whatever you want to put here>"] ... and it may be problematic ... as the package wouldn't know what's the "<whatever you want to put here>" to create a functional snippet.
I'll wait for the opinion from other developers and then we can open a bug and have a discussion (rather there, in the issue, than here, in order to keep it easily tracked) about what would be the easiest/fastest way to have it done.
Does this make sense? """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371863363
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
fidencio commented: """ @simo5, @CendioOssman,
Dropping a config snippet in /etc/sssd/conf.d/ with the option would be the way to go ... and I'd like to see it being done by, for instance, the package that wants/needs this option (thinlinc in this case).
However, I can see at least one issue here that has to be solved by SSSD before actually relying on it (or please, someone correct me in case I'm mistaken): The domain section usually looks like: [domain/"whatever you want to put here"] ... and it may be problematic ... as the package wouldn't know what's the "whatever you want to put here" to create a functional snippet.
I'll wait for the opinion from other developers and then we can open a bug and have a discussion (rather there, in the issue, than here, in order to keep it easily tracked) about what would be the easiest/fastest way to have it done.
Does this make sense? """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371863363
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
simo5 commented: """ The default should be a global option, not per domain. """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371873957
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
fidencio commented: """ We have discussed this a little bit on #sssd and, AFAIU (@simo5, please correct me if I got something wrong) the ideal situation would be: - **also** add this option as a global one - let the domain specific option **override** the global one
@SSSD/developers, @simo5's suggestion sounds quite reasonable to me. Would you guys like to bring up as well? """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-371909244
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
jhrozek commented: """ I'm confused, what do you mean by a global option? One in the `[sssd]` section? Or one that is in the domain/joined.ad.domain section and affects all the subdomains?
Would it be easier to add the discussion or some most important parts of it to this PR or to a ticket so that it's easy to follow the logic? """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-372147017
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
fidencio commented: """ I'm adding the discussion here: ``` <fidencio> simo: not going to extend the discussion in the pagure with a question that sounds quite stupid to me ... but why the option should be global? <fidencio> simo: I would easily buy if you tell me it should go under [pam] section as it's a list of PAM service names <fidencio> s/pagure/github PR/ <simo> fidencio: because the association is not domain specific <simo> if a service drops a file it wants that association to be valid for any possible domain you may join with that machine <simo> it is a default mapping <simo> has nothing to do with which domain you specifically ended up joining <fidencio> simo: hmmm. I see your point. OTOH I can also think that as it's something AD specific ... it should go in the domain section (and I guess that's the reason it's there) (mind that I'm not advocating for the current behaviour ... just trying to see both sides) <simo> fidencio: a domain section is a specific instantiation <simo> we are talking about a global default behavior <simo> service X uses pam file Y <simo> that's true besides what specific instantiation you use <simo> although in some contrived cases you may want to override this behavior in a specific domain <simo> which is also why software should not drop a domain specific snippet as admins may have their own specific configs <fidencio> simo: okay, got your point <fidencio> simo: so, a good way to fix this is to move this option to a global section ... <simo> fidencio: we need this option *also* as a global, to set defaults <simo> then per domain for overrides/domain specific ```
And the discussion can be added to the ticket as soon as we specifically have a ticket for this and/or decide to use the documentation ticket for this (I'll leave this decision to @mzidek-rh). """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-372226106
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
mzidek-rh commented: """ @simo5 : I understand why you want to set defaults in the [sssd] section and it makes sense to me. But I also think we may end up finding similar situations in the future (need defaults in [sssd] with the ability to override it in [domain/..] section) and I do not like it much, we basically double the same option.
Not just for this use case with gpo rule to pam service mappings, but for making the config file snippets more powerful, we could implement something like ` [domain/_GLOBAL_] ` that could set global defaults for all domains (and that would apply to all options available for domain section). I already opened this ticket: https://pagure.io/SSSD/sssd/issue/3670
What do you think?
"""
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-372304194
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
simo5 commented: """ No strong opinion beside bikeshedding on the name: domain/_defaults_ :-) """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-372307376
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
simo5 commented: """ No strong opinion beside bikeshedding on the name: ```domain/_defaults_``` :-) """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-372307376
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
mzidek-rh commented: """ @simo5 `domain/_defaults_` sounds good to me as well :) , I will updated the issue and I am closing this PR.
Thanks everyone for your input. """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-372317234
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
mzidek-rh commented: """ Actually, I do not have permissions to close this PR :D
So, someone who does have it, please close this PR :) """
See the full comment at https://github.com/SSSD/sssd/pull/530#issuecomment-372318447
URL: https://github.com/SSSD/sssd/pull/530 Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive
Label: +Rejected
URL: https://github.com/SSSD/sssd/pull/530 Author: CendioOssman Title: #530: GPO: Add "thinlinc" to ad_gpo_map_remote_interactive Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/530/head:pr530 git checkout pr530
sssd-devel@lists.fedorahosted.org
-
CendioOssman -
centos-ci
-
fidencio
-
jhrozek
-
mzidek-rh
-
simo5