Skip to content
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

GPO: Add "thinlinc" to ad_gpo_map_remote_interactive #530

Closed
wants to merge 1 commit into from

Conversation

CendioOssman
Copy link

ThinLinc is a remote desktop server, very similar to the Remote Desktop
Services referenced in the Active Directory description for this policy.

ThinLinc is a remote desktop server, very similar to the Remote Desktop
Services referenced in the Active Directory description for this policy.
@centos-ci
Copy link

Can one of the admins verify this patch?

1 similar comment
@centos-ci
Copy link

Can one of the admins verify this patch?

@CendioOssman
Copy link
Author

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).

@fidencio
Copy link
Contributor

fidencio commented Mar 9, 2018

ok to test

@mzidek-gh
Copy link
Contributor

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

@CendioOssman
Copy link
Author

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.

@mzidek-gh
Copy link
Contributor

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

@CendioOssman
Copy link
Author

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.

@mzidek-gh
Copy link
Contributor

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

@simo5
Copy link
Contributor

simo5 commented Mar 9, 2018

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.

@CendioOssman
Copy link
Author

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?

@fidencio
Copy link
Contributor

fidencio commented Mar 9, 2018

@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?

@simo5
Copy link
Contributor

simo5 commented Mar 9, 2018

The default should be a global option, not per domain.

@fidencio
Copy link
Contributor

fidencio commented Mar 9, 2018

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?

@jhrozek
Copy link
Contributor

jhrozek commented Mar 11, 2018

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?

@fidencio
Copy link
Contributor

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).

@mzidek-gh
Copy link
Contributor

@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?

@simo5
Copy link
Contributor

simo5 commented Mar 12, 2018

No strong opinion beside bikeshedding on the name: domain/_defaults_ :-)

@mzidek-gh
Copy link
Contributor

@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.

@mzidek-gh
Copy link
Contributor

Actually, I do not have permissions to close this PR :D

So, someone who does have it, please close this PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants