[SSSD] Re: KCM notifications
On Mon, 2020-02-24 at 16:08 +0100, Debarshi Ray wrote: > Hey, > > On Fri, Feb 21, 2020 at 7:26 PM Robbie Harwood wrote: > > CCing some GOA folks. I believe their preference was for the inotify > > approach. > > > > Moreover, I'd like to provide a consistent interface to all of the > > ccaches from krb5. I'm not going to be able to do that if dbus becomes > > involved (for a variety of reasons). > > The GOA code monitoring Kerberos credentials caches is already custom > made for each different cache type. > > So far it has only been inotify for FILE and DIR caches, but we are > currently working on adding notification support for KEYRING caches > based on David Howell's ongoing kernel work. See: > https://gitlab.gnome.org/GNOME/gnome-online-accounts/-/merge_requests/47 I am worried it will take longer still: https://lkml.org/lkml/2020/1/15/1030 BNu twho knows, Linus can change mind quickly. > I don't particularly expect D-Bus signals for KCM caches. If it's > D-Bus, then we will add another code path for KCM, if it's something > else, we will try to fit it in as best as we can. > > Of course, a unified libkrb5 API would be really nice. My personal comments were going in this direction, however I do not have a strong preference and if there is consistent investment that we want to preserve I won't complain. Simo. -- Simo Sorce RHEL Crypto Team Red Hat, Inc ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] Re: KCM notifications
On Fri, 2020-02-21 at 11:28 +0100, Sumit Bose wrote: > On Thu, Feb 20, 2020 at 12:27:23PM -0500, Simo Sorce wrote: > > On Thu, 2020-02-20 at 11:18 +0100, Pavel Březina wrote: > > > Hi devs, > > > > > > I'm thinking about ways to implement SSSD KCM notification that > > > something has changed (i.e. user called kinit/kdestroy) [1]. The main > > > use case is to notify Gnome Online Accounts (which is a daemon running > > > under logged-in user) when something has changed and it is already a > > > D-Bus service. > > > > > > The basic idea is that we would use D-Bus signals that would be emitted > > > by SSSD KCM responder (sssd_kcm process). Signals are broadcasted > > > messages that are delivered to client that chose to listen to them. > > > > > > The problem is that we > > > 1) can't connect to specific user's session bus because KCM runs as > > > root/sssd and connecting to other user's bus is not allowed > > > 2) can't specify which user is allowed to get the signal > > > > Can you explain what this means? > > > > > 3) therefore we can't send the signal only to specific user > > > > Why not? > > We definitely should be able to know what user is listening on the > > other side of a bus, so we could simply only send back info related to > > that same user? Or are we blinded to which user is interacting with us? > > > > > So the solution is that KCM connects to system bus and sends > > > org.sssd.kcm.Changed(uid) signal where uid is uid of the user which > > > ccache has changed so the receiver can know which user is affected. This > > > signal is broadcasted to everyone who listens to it. > > > > This means users can monitor each other, sounds bad. > > > > > It is perfectly usable, however the question is whether we can broadcast > > > this information (that user A run kinit/kdestroy/other modification of > > > ccache) or it is a security leak that we must avoid and we should seek > > > other solution. > > > > Looks quite bad, why do we need to broadcast to everyone? > > Isn't there an interface where a specific user can initiate a listening > > channel and we send back only info related to that user? > > > > > I asked this secalert and they reply that there is no security concern > > > "but": > > > > > > > After looking at this issue, honestly I dont see an attack vector here, > > > > but i am afraid something like this could be used with some other > > > > security flaw to maybe gain privesc? For example a flaw which is some > > > > component which can be triggered only when kinit is run? where precise > > > > timing is required. > > > > > > > > So in conclusion if there is another "better" solution than it should > > > > be preferred. Or in worse case atleast have an option to disable this > > > > somehow via some config, so that such situations can be avoided. > > > > > > I don't think a precise timing is necessary and we can send the signal > > > few miliseconds or a second later. That should eliminate this concern (I > > > asked this, waiting for an answer). > > > > It's more of a privacy reason to me, if we can find other ways that do > > not broadcast this to everyone I think it would be better. > > > > > I can think of two more solutions currently: > > > > > > 1) Have client connect to KCM socket and await signal there. This > > > however would mean that connection between client and KCM need to be > > > always established and KCM responder would be running all the time > > > (currently it is only short-lived socket activated process). > > > > Is this (shortlivedness) why we can't use a listener ? > > > > > 2) Create a temporary file in a directory owned by user. User can then > > > setup inotify watch to the file and sssd would write to the file on > > > changes. Since it is in directory owned by user, other users would not > > > be able to setup the watch. > > > > But how will the daemon be able to do this? > > At the very least the file would need to be in /run/user because home > > directories could be on NFS where root can't write, and besides this is > > really temporary state and has no business in home directories. > > > > > Note that it is possile to setup inotify watch on FILE ccache (keeping > > > distro settings out of the question the default
[SSSD] Re: KCM notifications
On Fri, 2020-02-21 at 11:22 +0100, Pavel Březina wrote: > On 2/20/20 6:27 PM, Simo Sorce wrote: > > On Thu, 2020-02-20 at 11:18 +0100, Pavel Březina wrote: > > > Hi devs, > > > > > > I'm thinking about ways to implement SSSD KCM notification that > > > something has changed (i.e. user called kinit/kdestroy) [1]. The main > > > use case is to notify Gnome Online Accounts (which is a daemon running > > > under logged-in user) when something has changed and it is already a > > > D-Bus service. > > > > > > The basic idea is that we would use D-Bus signals that would be emitted > > > by SSSD KCM responder (sssd_kcm process). Signals are broadcasted > > > messages that are delivered to client that chose to listen to them. > > > > > > The problem is that we > > > 1) can't connect to specific user's session bus because KCM runs as > > > root/sssd and connecting to other user's bus is not allowed > > > 2) can't specify which user is allowed to get the signal > > > > Can you explain what this means? > > See below. > > > > 3) therefore we can't send the signal only to specific user > > > > Why not? > > We definitely should be able to know what user is listening on the > > other side of a bus, so we could simply only send back info related to > > that same user? Or are we blinded to which user is interacting with us? > > We need to distinguish method calls and signals. Method calls are > (usually) unicast - client send method call to message bus and it is > routed to the destination. In this case the destination knows who the > sender was. > > Signals are broadcasted messages and do not have destination. They are > emitted by the sender and message bus resend it to anyone who is > listening. The sender does not know who received it. > > See: > https://dbus.freedesktop.org/doc/dbus-tutorial.html#signalprocedure > > In theory, it is possible to send signal to a particular destination: > https://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-routing > but we would have to know the destination name. But I am not aware of > any way to obtain list of listeners. > > But perhaps, we could create a dedicated message bus with our sbus that > would be publicly available. It could be able to emit the signal only > for connections with particular uid. Clients could still use standard > dbus api. Do we actually have to send a signal? I was thinking if it is possible for client instead to create a service with the username in it. Then KCM could be their "client" and call a method to pass data (which ccache was updated ?) to the service. No signal involved. The only problem is that I guess there is no way to restrict paths wo we would still need to double check that the service name whatever- whatever- is really owned by before sending data. Can a user create a service on the system bus? Or does it require privileges ? > > > So the solution is that KCM connects to system bus and sends > > > org.sssd.kcm.Changed(uid) signal where uid is uid of the user which > > > ccache has changed so the receiver can know which user is affected. This > > > signal is broadcasted to everyone who listens to it. > > > > This means users can monitor each other, sounds bad. > > > > > It is perfectly usable, however the question is whether we can broadcast > > > this information (that user A run kinit/kdestroy/other modification of > > > ccache) or it is a security leak that we must avoid and we should seek > > > other solution. > > > > Looks quite bad, why do we need to broadcast to everyone? > > Isn't there an interface where a specific user can initiate a listening > > channel and we send back only info related to that user? > > I am not aware of such interface. Se above what I mean, it is about reversing the client-server paradigm, where KCM is the system bus client, and interested parties are actual "services" ? I guess one issue is that you can only have on service doing this, but that one service could be a gnome component that then can rebroadcast the information on the session bus via a signal? > > > I asked this secalert and they reply that there is no security concern > > > "but": > > > > > > > After looking at this issue, honestly I dont see an attack vector here, > > > > but i am afraid something like this could be used with some other > > > > security flaw to maybe gain privesc? For example a flaw which is some > > > > component which can be triggered only when kinit is run? where precise
[SSSD] Re: KCM notifications
On Thu, 2020-02-20 at 11:18 +0100, Pavel Březina wrote:
> Hi devs,
>
> I'm thinking about ways to implement SSSD KCM notification that
> something has changed (i.e. user called kinit/kdestroy) [1]. The main
> use case is to notify Gnome Online Accounts (which is a daemon running
> under logged-in user) when something has changed and it is already a
> D-Bus service.
>
> The basic idea is that we would use D-Bus signals that would be emitted
> by SSSD KCM responder (sssd_kcm process). Signals are broadcasted
> messages that are delivered to client that chose to listen to them.
>
> The problem is that we
> 1) can't connect to specific user's session bus because KCM runs as
> root/sssd and connecting to other user's bus is not allowed
> 2) can't specify which user is allowed to get the signal
Can you explain what this means?
> 3) therefore we can't send the signal only to specific user
Why not?
We definitely should be able to know what user is listening on the
other side of a bus, so we could simply only send back info related to
that same user? Or are we blinded to which user is interacting with us?
> So the solution is that KCM connects to system bus and sends
> org.sssd.kcm.Changed(uid) signal where uid is uid of the user which
> ccache has changed so the receiver can know which user is affected. This
> signal is broadcasted to everyone who listens to it.
This means users can monitor each other, sounds bad.
> It is perfectly usable, however the question is whether we can broadcast
> this information (that user A run kinit/kdestroy/other modification of
> ccache) or it is a security leak that we must avoid and we should seek
> other solution.
Looks quite bad, why do we need to broadcast to everyone?
Isn't there an interface where a specific user can initiate a listening
channel and we send back only info related to that user?
> I asked this secalert and they reply that there is no security concern
> "but":
>
> > After looking at this issue, honestly I dont see an attack vector here, but
> > i am afraid something like this could be used with some other security flaw
> > to maybe gain privesc? For example a flaw which is some component which can
> > be triggered only when kinit is run? where precise timing is required.
> >
> > So in conclusion if there is another "better" solution than it should be
> > preferred. Or in worse case atleast have an option to disable this somehow
> > via some config, so that such situations can be avoided.
>
> I don't think a precise timing is necessary and we can send the signal
> few miliseconds or a second later. That should eliminate this concern (I
> asked this, waiting for an answer).
It's more of a privacy reason to me, if we can find other ways that do
not broadcast this to everyone I think it would be better.
> I can think of two more solutions currently:
>
> 1) Have client connect to KCM socket and await signal there. This
> however would mean that connection between client and KCM need to be
> always established and KCM responder would be running all the time
> (currently it is only short-lived socket activated process).
Is this (shortlivedness) why we can't use a listener ?
> 2) Create a temporary file in a directory owned by user. User can then
> setup inotify watch to the file and sssd would write to the file on
> changes. Since it is in directory owned by user, other users would not
> be able to setup the watch.
But how will the daemon be able to do this?
At the very least the file would need to be in /run/user because home
directories could be on NFS where root can't write, and besides this is
really temporary state and has no business in home directories.
> Note that it is possile to setup inotify watch on FILE ccache (keeping
> distro settings out of the question the default ccache is
> FILE:/tmp/krb5cc_%{uid} as per krb5.conf manpage) so perhaps we really
> don't have to care about broadcasting this information.
That is a good point, I still dislike actively broadcasting around I
wonder if it is possible to just reconnect to the system bus and learn
who is listening and just target those listeners as appropriate.
Is the system bus stateful? Do clients have to reconnect whenever a
daemon restarts?
> Thanks,
> Pavel.
>
> [1] https://pagure.io/SSSD/sssd/issue/3568
--
Simo Sorce
RHEL Crypto Team
Red Hat, Inc
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
[SSSD] Re: RFC: 2.0 release notes
LGTM, great work guys. Simo. On Mon, 2018-08-13 at 15:20 +0200, Jakub Hrozek wrote: > Hi, > > we’re about to release 2.0. Here are my draft release notes: > > SSSD 2.0.0 > === > > > Highlights > -- > This release removes or deprecates functionality from SSSD, therefore the SSSD > team decided it was time to bump the major version number. The sssd-1-16 > branch will be still supported (most probably even as a LTM branch) so that > users who rely on any of the removed features can either migrate or ask for > the features to be readded. > > Except for the removed features, this release contains a reworked internal IPC > and a new default storage back end for the KCM responder. > > Removed features > > * The Python API for managing users and groups in local domains > (`id_provider=local`) was removed completely. The interface > had been packaged as module called `pysss.local` > * The LDAP provider had a special-case branch for evaluating group > memberships with the RFC2307bis schema when group nesting was > explicitly disabled. This codepath was adding needless additional > complexity for little performance gain and was rarely used. > * The `ldap_groups_use_matching_rule_in_chain` and > `ldap_initgroups_use_matching_rule_in_chain` options and the code that > evaluated them was removed. Neither of these options provided > a significant performance benefit and the code implementing > these options was complex and rarely used. > > Deprecated features > ^^^ > * The local provider (`id_provider=local`) and the command line > tools to manage users and groups in the local domains, such as > `sss_useradd` is not built by default anymore. There is a configure-time > switch `--enable-local-domain` you can use to re-enable the local > domain support. However, upstream would like to remove the local > domain completely in a future release. > * The `sssd_secrets`` responder is not packaged by default. The responder > was meant to provide a REST API to access user secrets as well as > a proxy to Custodia servers, but as Custodia development all but > stopped and the local secrets handling so far didn't gain traction, > we decided to not enable this code by default. This also means that the > default SSSD configuration no longer requires libcurl and http-parser. > > Changed default settings > > * The `ldap_sudo_include_regexp` option changed its default value > from `true` to `false`. This means that wild cards in the `sudoHost` > LDAP attribute are no longer supported by default. The reason we > changed the default was that the wildcard was costly to evaluate > on the LDAP server side and at the same time rarely used. > > New features > > * The KCM responder has a new back end to store credential caches > in a local database. This new back end is enabled by default and > actually uses the same storage as the `sssd-secrets` responder had used, > so the switch from sssd-secrets to this new back end should be > completely seamless. The `sssd-secrets` socket is no longer required for > KCM to operate. > > Packaging Changes > - > * The `sss_useradd`, `sss_userdel`, `sss_usermod`, `sss_groupadd`, > `sss_groupdel`, `sss_groupshow` and `sss_groupmod` binaries and their > manual pages are no longer packaged by default unless > `--enable-local-provider` is selected. > * The sssd_secrets responder is no longer packaged by default unless > `--enable-secrets-responder` is selected. > * The new internal IPC mechanism uses several private libraries that > need to be packaged - `libsss_sbus.so`, `libsss_sbus_sync.so`, > `libsss_iface.so`, > `libsss_iface_sync.so`, `libifp_iface.so` and `libifp_iface_sync.so` > * The new KCM ccache back end relies on a private library > `libsss_secrets.so` that must be packaged in case either the KCM > responder > or the secrets responder are enabled. > > Documentation Changes > - > * The `ldap_groups_use_matching_rule_in_chain` and > `ldap_initgroups_use_matching_rule_in_chain` options were removed. > * The `ldap_sudo_include_regexp` option changed its default value > from `true` to `false`. > > Tickets Fixed > - > To be generated > > Detailed Changelog > -- > To be generated > ___ > sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org > To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org > Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines > List Archives: >
[SSSD] Re: [RFC] sbus2 integration
On Mon, 2018-05-21 at 11:52 +0200, Pavel Březina wrote: > On 05/18/2018 09:50 PM, Simo Sorce wrote: > > On Fri, 2018-05-18 at 16:11 +0200, Sumit Bose wrote: > > > On Fri, May 18, 2018 at 02:33:32PM +0200, Pavel Březina wrote: > > > > Hi folks, > > > > I sent a mail about new sbus implementation (I'll refer to it as sbus2) > > > > [1]. > > > > Sorry Pavel, > > but I need to ask, why a new bus instead of somthing like varlink ? > > This is an old work, we did not know about varlink until this work was > already finished. But since we still provide public D-Bus API, we need a > way to work with it anyway. Ack, thanks, wasn't sure how old the approach was, so I just asked :-) > > > > Now, I'm integrating it into SSSD. The work is quite difficult since it > > > > touches all parts of SSSD and the changes are usually interconnected > > > > but I'm > > > > slowly moving towards the goal [2]. > > > > > > > > At this moment, I'm trying to take "miminum changes" paths so the code > > > > can > > > > be built and function with sbus2, however to take full advantage of it, > > > > it > > > > will take further improvements (that will not be very difficult). > > > > > > > > There is one big change that I would like to take though, that needs to > > > > be > > > > discussed. It is about how we currently handle sbus connections. > > > > > > > > In current state, monitor and each backend creates a private sbus > > > > server. > > > > The current implementation of a private sbus server is not a message > > > > bus, it > > > > only serves as an address to create point to point nameless connection. > > > > Thus > > > > each client must maintain several connections: > > > > - each responder is connected to monitor and to all backends > > > > - each backend is connected to monitor > > > > - we have monitor + number of backends private servers > > > > - each private server maintains about 10 active connections > > > > > > > > This has several disadvantages - there are many connections, we cannot > > > > broadcast signals, if a process wants to talk to other process it needs > > > > to > > > > connect to its server and maintain the connection. Since responders do > > > > not > > > > currently provider a server, they cannot talk between each other. > > > > This design has a key advantage, a single process going down does not > > affect all other processes communication. How do you recover if the > > "switch-board" goes down during message processing with sbus ? > > The "switch-board" will be restarted and other processes will reconnect. > The same way as it is today when one backend dies. Yes, but what about in-flight operations ? Both client and server will abort and retry ? Will the server just keep around data forever ? It'd be nice to understand the mechanics of recovery to make sure the actual clients do not end up being impacted, by lack of service. > > > > sbus2 implements proper private message bus. So it can work in the same > > > > way > > > > as session or system bus. It is a server that maintains the connections, > > > > keep tracks of their names and then routes messages from one connection > > > > to > > > > another. > > > > > > > > My idea is to have only one sbus server managed by monitor. > > > > This conflict wth the idea of getting rid of the monitor process, do > > not know if this is currently still pursued but it was brought up over > > and over many times that we might want to use systemd as the "monitor" > > and let socket activation deal with the rest. > > I chose monitor process for the message bus, since 1) it is stable, 2) > it is idle most of the time. However, it can be a process on its own. Not sure that moving it to another process makes a difference, the concern would be the same I think. > That being said, it does not conflict with removing the monitoring > functionality. We only leave a single message bus. Right but at that point might as well retain monitoring ... > > > > Other processes > > > > will connect to this server with a named connection (e.g. sssd.nss, > > > > sssd.backend.dom1, sssd.backend.dom2). We can then send message to this > > > > message bus (only one connection) and set destination to nam
[SSSD] Re: [RFC] sbus2 integration
On Mon, 2018-05-21 at 10:38 +0200, Jakub Hrozek wrote: > > On 18 May 2018, at 21:50, Simo Sorce <s...@redhat.com> wrote: > > > > Sorry Pavel, > > but I need to ask, why a new bus instead of somthing like varlink ? > > Do you think there is an advantage with varlink over D-Bus as long as > we use a private style of communication and use either varlink or D- > Bus more or less as a marshalling mechanism? Only if we have to start from scratch or make significant changes, I wouldn't embark on replacing the tool just for the sake of replacing. > > > > > > Now, I'm integrating it into SSSD. The work is quite difficult since it > > > > touches all parts of SSSD and the changes are usually interconnected > > > > but I'm > > > > slowly moving towards the goal [2]. > > > > > > > > At this moment, I'm trying to take "miminum changes" paths so the code > > > > can > > > > be built and function with sbus2, however to take full advantage of it, > > > > it > > > > will take further improvements (that will not be very difficult). > > > > > > > > There is one big change that I would like to take though, that needs to > > > > be > > > > discussed. It is about how we currently handle sbus connections. > > > > > > > > In current state, monitor and each backend creates a private sbus > > > > server. > > > > The current implementation of a private sbus server is not a message > > > > bus, it > > > > only serves as an address to create point to point nameless connection. > > > > Thus > > > > each client must maintain several connections: > > > > - each responder is connected to monitor and to all backends > > > > - each backend is connected to monitor > > > > - we have monitor + number of backends private servers > > > > - each private server maintains about 10 active connections > > > > > > > > This has several disadvantages - there are many connections, we cannot > > > > broadcast signals, if a process wants to talk to other process it needs > > > > to > > > > connect to its server and maintain the connection. Since responders do > > > > not > > > > currently provider a server, they cannot talk between each other. > > > > This design has a key advantage, a single process going down does not > > affect all other processes communication. How do you recover if the > > "switch-board" goes down during message processing with sbus ? > > FWIW, this is a worry I also expressed to Pavel on the phone the other day. > > > > > > > sbus2 implements proper private message bus. So it can work in the same > > > > way > > > > as session or system bus. It is a server that maintains the connections, > > > > keep tracks of their names and then routes messages from one connection > > > > to > > > > another. > > > > > > > > My idea is to have only one sbus server managed by monitor. > > > > This conflict wth the idea of getting rid of the monitor process, do > > not know if this is currently still pursued but it was brought up over > > and over many times that we might want to use systemd as the "monitor" > > and let socket activation deal with the rest. > > It’s something we’ve been talking about but never got around to > implementing. Additionally, there are users who are running sssd in > a container for better or worse and I’m not sure we systemd in a > container is something used or stable outside some test builds.. So .. we do not know :-) Simo. -- Simo Sorce Sr. Principal Software Engineer Red Hat, Inc ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/sssd-devel@lists.fedorahosted.org/message/L3V7ZZR2B47GLNPIPWPVZWMXOXSKNXTI/
[SSSD] Re: [RFC] sbus2 integration
bus, and given we already had ideas about offering some plugic interface over the message bus we went that way so we could later reuse the integration. Today we'd probably go with something a lot more lightweight like varlink. > If I understood you correctly we not only have 'a' private server but 4 > for a typically minimal setup (monitor, pam, nss, backend). > > Given your arguments above I think using a private message bus would > have benefits. Currently two questions came to my mind. First, what > happens to ongoing requests if the monitor dies and is restarted. E.g. > If the backend is processing a user lookup request and the monitor is > restarted can the backend just send the reply to the freshly stared > instance and the nss responder will finally get it? Or is there some > state lost which would force the nss responder to resend the request? How would the responder even know the other side died, is there a way for clients to know that services died and all requests in flight need to be resent ? > The second is about the overhead. Do you have any numbers how much > longer e.g. the nss responder has to wait e.g. for a backend if offline > reply? I would expect that we loose more time at other places, > nevertheless it would be good to have some basic understanding about the > overhead. Latency is what e should be worried ab out, one other reason to go with direct connections is that you did not have to wait for 3 processes to be awake and be scheduled (client/monitor/server) but only 2 (client/server). On busy machines the latency can be (relatively) quite high if an additional process need to be scheduled just to pass a long a message. Simo. > Thank you for your hard work on sbus2. > > bye, > Sumit > > > > [1] https://github.com/pbrezina/sbus > > [2] https://github.com/pbrezina/sssd/tree/sbus > > ___ > > sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org > > To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org > > Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html > > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines > > List Archives: > > https://lists.fedoraproject.org/archives/list/sssd-devel@lists.fedorahosted.org/message/Z7ZSIEX7QAAZAUGCVNLTYDAYEUHOQHY6/ > > ___ > sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org > To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org > Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines > List Archives: > https://lists.fedoraproject.org/archives/list/sssd-devel@lists.fedorahosted.org/message/FMN2C5GXHDIQ6SNPRWWGPSF4EJEFX7PC/ -- Simo Sorce Sr. Principal Software Engineer Red Hat, Inc ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/sssd-devel@lists.fedorahosted.org/message/H4J4OUHYLVCBJNTXLFZJJWRHUTIENVV6/
[SSSD] Re: [RFC] sbus2 integration
On Fri, 2018-05-18 at 21:02 +0200, Jakub Hrozek wrote: > > On 18 May 2018, at 14:33, Pavel Březina <pbrez...@redhat.com> wrote: > > > > Also here is a bonus question - do any of you remember why we use private > > server at all? Why don't we connect to system message bus? I do not see any > > benefit in having a private server. > > To expand on what Sumit said, at one point we were betting on kdbus > to become a thing and then it didn’t. (I don’t know enough about > bus1, but IIRC was "just" a userspace reimplementation of the D-Bus > protocol, so the same trust limitations apply) bus1 was also a kernel implementation, but that one also did not pan out ... Simo. -- Simo Sorce Sr. Principal Software Engineer Red Hat, Inc ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/sssd-devel@lists.fedorahosted.org/message/RWGIJVWWJEUPZAWE3BGHG7SNQKWTBA46/
[SSSD] Re: KCM talking to secrets over REST API (or not)
On Tue, 2018-03-20 at 20:36 +0100, Jakub Hrozek wrote: > > On 20 Mar 2018, at 13:51, Simo Sorce <s...@redhat.com> wrote: > > > > On Tue, 2018-03-20 at 12:54 +0100, Jakub Hrozek wrote: > > > Let me bump this thread..see some ideas inline. > > Thank you for the prompt response. YW, more inline. > > > > > > > On 13 Mar 2018, at 14:07, Jakub Hrozek <jhro...@redhat.com> wrote: > > > > > > > > > > > > > > > > > On 13 Mar 2018, at 13:42, Simo Sorce <s...@redhat.com> wrote: > > > > > > > > > > On Tue, 2018-03-13 at 12:05 +0100, Jakub Hrozek wrote: > > > > > > Hi, > > > > > > > > > > > > last week, me, some other SSSD developers and Robbie looked at how > > > > > > the KCM server in its current state can be debugged and what the > > > > > > current issues are. One thing that stood out was that because every > > > > > > Kerberos operation now requires a round-trip between libkrb5 to > > > > > > sssd-kcm and then to sssd-secrets via the REST API, there are > > > > > > several components involved which makes the setup brittle. In > > > > > > addition, the marshalling of each request into JSON and sending it > > > > > > over the sssd-secrets REST API adds a bit of an overhead. > > > > > > > > > > > > In a follow-up e-mail thread, Simo suggested that instead of using > > > > > > the REST API, sssd-kcm might access the sssd-secrets database > > > > > > directly. I wanted to discuss this potential change with the other > > > > > > developers. > > > > > > > > > > > > As I’ve been thinking about Simo’s proposal, it actually makes > > > > > > sense to me. The pros and cons as I see them are: > > > > > > [+] less complexity. We would get rid of the whole JSON > > > > > > marshalling code and wouldn’t have to allow on a second > > > > > > socket-activated deamon. > > > > > > [+] better performance. Again, no JSON marshalling, but in the > > > > > > past we were working around the REST API not providing a PATCH > > > > > > verb, so KCM had to serialize requests to make sure two concurrent > > > > > > PUT requests don’t step over one another. > > > > > > [+] the “proprietary” API between KCM and secrets could be > > > > > > augmented more easily to allow e.g. introspection and quota > > > > > > management better. > > > > > > We wanted to allow some form of introspection and if we > > > > > > confine ourselves to the REST API, I can’t think of another way of > > > > > > answering “how much of their quota is UID X using" except adding a > > > > > > KCM-specific REST API endpoint anyway. > > > > > > > > > > > > [-] we would have to define a private API between secrets and > > > > > > KCM. The REST API already has a nice router (or multiplexer if you > > > > > > will) to route the different HTTP verbs into internal sssd-secrets > > > > > > actions, e.g HTTP GET into retrieving a secret. > > > > > > [-] overall allowing to write to the secrets database via the > > > > > > REST API seems somewhat cleaner > > > > > > > > > > > > Are there other opinions on the matter? Should we go ahead and > > > > > > change the design to write to the database directly? > > > > > > > > > > So one of the reasons to use secrets this way was that we could use > > > > > the > > > > > REST interface (with HTTPS) to route requests from a container to the > > > > > host or a different node, and keep shared ccaches if needed. > > > > > > > > > > > > > To Custodia? > > > > Yes > > > > > So, if this is a use-case you would like to keep upstream, how about we > > > expose the kcm “back end” option? We could keep the current REST API code > > > around, it’s tested with upstream integration tests and if we developed > > > ‘local secrets’ back end, we could default to that and keep the full REST > > > API as an option. > > > > Sure, why not. > > OK, I filed https://pagure.io/SS
[SSSD] Re: KCM talking to secrets over REST API (or not)
On Tue, 2018-03-20 at 12:54 +0100, Jakub Hrozek wrote: > Let me bump this thread..see some ideas inline. > > > On 13 Mar 2018, at 14:07, Jakub Hrozek <jhro...@redhat.com> wrote: > > > > > > > > > On 13 Mar 2018, at 13:42, Simo Sorce <s...@redhat.com> wrote: > > > > > > On Tue, 2018-03-13 at 12:05 +0100, Jakub Hrozek wrote: > > > > Hi, > > > > > > > > last week, me, some other SSSD developers and Robbie looked at how the > > > > KCM server in its current state can be debugged and what the current > > > > issues are. One thing that stood out was that because every Kerberos > > > > operation now requires a round-trip between libkrb5 to sssd-kcm and > > > > then to sssd-secrets via the REST API, there are several components > > > > involved which makes the setup brittle. In addition, the marshalling of > > > > each request into JSON and sending it over the sssd-secrets REST API > > > > adds a bit of an overhead. > > > > > > > > In a follow-up e-mail thread, Simo suggested that instead of using the > > > > REST API, sssd-kcm might access the sssd-secrets database directly. I > > > > wanted to discuss this potential change with the other developers. > > > > > > > > As I’ve been thinking about Simo’s proposal, it actually makes sense to > > > > me. The pros and cons as I see them are: > > > > [+] less complexity. We would get rid of the whole JSON > > > > marshalling code and wouldn’t have to allow on a second > > > > socket-activated deamon. > > > > [+] better performance. Again, no JSON marshalling, but in the > > > > past we were working around the REST API not providing a PATCH verb, so > > > > KCM had to serialize requests to make sure two concurrent PUT requests > > > > don’t step over one another. > > > > [+] the “proprietary” API between KCM and secrets could be > > > > augmented more easily to allow e.g. introspection and quota management > > > > better. > > > >We wanted to allow some form of introspection and if we > > > > confine ourselves to the REST API, I can’t think of another way of > > > > answering “how much of their quota is UID X using" except adding a > > > > KCM-specific REST API endpoint anyway. > > > > > > > > [-] we would have to define a private API between secrets and > > > > KCM. The REST API already has a nice router (or multiplexer if you > > > > will) to route the different HTTP verbs into internal sssd-secrets > > > > actions, e.g HTTP GET into retrieving a secret. > > > > [-] overall allowing to write to the secrets database via the > > > > REST API seems somewhat cleaner > > > > > > > > Are there other opinions on the matter? Should we go ahead and change > > > > the design to write to the database directly? > > > > > > So one of the reasons to use secrets this way was that we could use the > > > REST interface (with HTTPS) to route requests from a container to the > > > host or a different node, and keep shared ccaches if needed. > > > > > > > To Custodia? Yes > So, if this is a use-case you would like to keep upstream, how about we > expose the kcm “back end” option? We could keep the current REST API code > around, it’s tested with upstream integration tests and if we developed > ‘local secrets’ back end, we could default to that and keep the full REST API > as an option. Sure, why not. > > > > > I do not know if that's an important use case. > > > > My guess is “not very much” or at least I think our attempts to convince > > people to use Kerberos with containers fell flat. I do not think it did, but some of these things are still getting developed. I know kerberos is not mainstream in containers, but we did have people asking for it in the past. > > > > > > One other reason I thought was nice was that we could easily debug > > > stuff by looking at simple text based HTTP Requests and Replies. > > > > > > It requires to dump or sniff the communication between kcm and secrets, > > > but I did not think it was too hard to do ? > > > > The communication is already logged, the problem as far as debuggability is > > concerned is that you need to enable debugging for two services and then > > correlate the two log fi
[SSSD] Re: KCM talking to secrets over REST API (or not)
On Tue, 2018-03-13 at 12:05 +0100, Jakub Hrozek wrote: > Hi, > > last week, me, some other SSSD developers and Robbie looked at how the KCM > server in its current state can be debugged and what the current issues are. > One thing that stood out was that because every Kerberos operation now > requires a round-trip between libkrb5 to sssd-kcm and then to sssd-secrets > via the REST API, there are several components involved which makes the setup > brittle. In addition, the marshalling of each request into JSON and sending > it over the sssd-secrets REST API adds a bit of an overhead. > > In a follow-up e-mail thread, Simo suggested that instead of using the REST > API, sssd-kcm might access the sssd-secrets database directly. I wanted to > discuss this potential change with the other developers. > > As I’ve been thinking about Simo’s proposal, it actually makes sense to me. > The pros and cons as I see them are: > [+] less complexity. We would get rid of the whole JSON marshalling > code and wouldn’t have to allow on a second socket-activated deamon. > [+] better performance. Again, no JSON marshalling, but in the past we > were working around the REST API not providing a PATCH verb, so KCM had to > serialize requests to make sure two concurrent PUT requests don’t step over > one another. > [+] the “proprietary” API between KCM and secrets could be augmented > more easily to allow e.g. introspection and quota management better. > We wanted to allow some form of introspection and if we confine > ourselves to the REST API, I can’t think of another way of answering “how > much of their quota is UID X using" except adding a KCM-specific REST API > endpoint anyway. > > [-] we would have to define a private API between secrets and KCM. The > REST API already has a nice router (or multiplexer if you will) to route the > different HTTP verbs into internal sssd-secrets actions, e.g HTTP GET into > retrieving a secret. > [-] overall allowing to write to the secrets database via the REST API > seems somewhat cleaner > > Are there other opinions on the matter? Should we go ahead and change the > design to write to the database directly? So one of the reasons to use secrets this way was that we could use the REST interface (with HTTPS) to route requests from a container to the host or a different node, and keep shared ccaches if needed. I do not know if that's an important use case. One other reason I thought was nice was that we could easily debug stuff by looking at simple text based HTTP Requests and Replies. It requires to dump or sniff the communication between kcm and secrets, but I did not think it was too hard to do ? If debuggability is the only issue have you thought about adding an option to dump all requests and replies from both the kcm frontend socket and the secrets backend sockets into a secure debug log file ? Would that help ? Simo. -- Simo Sorce Sr. Principal Software Engineer Red Hat, Inc ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Fleet Commander: design changes due to the drop of DAC_OVERRIDE capability
On Mon, 2018-01-22 at 15:10 +0100, Fabiano Fidêncio wrote: > People, > > Let's start with the context of this email: > https://bugzilla.redhat.com/show_bug.cgi?id=1536854 > So, seems that even without knowing that, I've relied on CAP_DAC_OVERRIDE > in order to have the Fleet Commander integration working as expected and in > the implementation details of this feature. > > The desktop profiles are stored in a dir like: > /var/lib/sss/deskprofile/$domain/$user/$profile. > > Currently, the way I've been creating those are: > $domain = 755 (root:root) > $user = 600 ($user:$user_group) > $profile = 600 ($user:$user_group) > > Now, as mentioned in the bugzilla linked in this email, the current code > fails with an EACCES. > > With all this background, I'd like to discuss what's the best approach to > take. I've opened a PR (https://github.com/SSSD/sssd/pull/498) which makes > everything work again, but does the following changes: > > $domain = 755 (root:root) -- NO changes here > $user = 770 ($user:root) --> changed from 600 ($user:$user_group) > $profile = 660 ($user:root) --> changed from 600 ($user:$user_group) > > This is one way to solve the issue suggested at > https://bugzilla.redhat.com/show_bug.cgi?id=1536854#c5. > > Another suggestion, also mentioned in the bugzilla, would be to only > fchown()/fchmod() the files/dirs *after* all the operations we do are over. > > Is there any other suggestion? Whatever comes out of this discussion will > be used to update the feature's design page accordingly. Change euid to that of the user during operations, leave the permissions strict ? Simo. -- Simo Sorce Sr. Principal Software Engineer Red Hat, Inc ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Design document: Enhanced NSS API
On Thu, 2017-11-02 at 14:53 +0200, Alexander Bokovoy wrote: > On to, 02 marras 2017, Simo Sorce wrote: > > On Thu, 2017-11-02 at 13:14 +0100, Sumit Bose wrote: > > > On Fri, Oct 27, 2017 at 08:43:28AM -0400, Simo Sorce wrote: > > > > On Thu, 2017-10-26 at 22:14 +0200, Sumit Bose wrote: > > > > > On Thu, Oct 26, 2017 at 02:43:29PM -0400, Simo Sorce wrote: > > > > > > On Thu, 2017-10-26 at 12:16 +0200, Jakub Hrozek wrote: > > > > > > > On Wed, Oct 25, 2017 at 05:39:21PM +0200, Sumit Bose > > > > > > > wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > please find below the design document for the enhanced > > > > > > > > NSS > > > > > > > > API > > > > > > > > which > > > > > > > > makes e.g. the client side timeouts which where > > > > > > > > recently > > > > > > > > refactored > > > > > > > > available to callers. > > > > > > > > > > > > > > > > A more visual friendly version can be found at: > > > > > > > > https://pagure.io/fork/sbose/SSSD/docs/blob/07514ce5284 > > > > > > > > 5d47 > > > > > > > > fe6b > > > > > > > > b327 > > > > > > > > f782a865bfa75628a/f/design_pages/enhanced_nss_api.rst > > > > > > > > > > > > > > > > bye, > > > > > > > > Sumit > > > > > > > > > > > > > > LGTM! > > > > > > > > > > > > > > > > > > > Looking at this I have some questions, if you are going to > > > > > > create a > > > > > > new > > > > > > library and just need to set a timeout it seem it would be > > > > > > a > > > > > > much > > > > > > better interface to use a context handle you allocate and > > > > > > pass > > > > > > into > > > > > > each call, and then have getters setters to set timeouts or > > > > > > any > > > > > > other > > > > > > flags that should influence the whole behavior. This will > > > > > > allow > > > > > > you > > > > > > also to control how many concurrent connections you want to > > > > > > have > > > > > > against sssd, as each new context will create a new socket > > > > > > connection > > > > > > and all. > > > > > > > > > > > > In the original libnss_sss.so we could not do that because > > > > > > the > > > > > > glibc > > > > > > interface does not offer any way to hold a context, but > > > > > > there > > > > > > is no > > > > > > reason to continue along that line in a *new* API. And not > > > > > > using > > > > > > contexts in threaded applications is generally a bad idea, > > > > > > as > > > > > > you > > > > > > end > > > > > > up *requiring* te use of mutexes when that is really not > > > > > > always > > > > > > a > > > > > > need > > > > > > (separated threads can open separate connections and not > > > > > > share > > > > > > any > > > > > > data > > > > > > that require mutexes). > > > > > > > > > > This sounds like a good 2.0 feature, are you interested in > > > > > creating a > > > > > more detailed design page for this? > > > > > > > > Sure. > > > > > > > > > Currently my goal was to reuse as > > > > > much or the current trusted code we already have. > > > > > > > > > > > > > > > > > On the responder side I also do not see why new calls are > > > > > > being > > > > > > created. You clearly want a client-wide behavior, introduce > > > > > > ONE > > > > > > new > > > > > > call that sets flags for the rest of the connection, and > > > > > > just > > >
[SSSD] Re: Design document: Enhanced NSS API
On Thu, 2017-11-02 at 13:14 +0100, Sumit Bose wrote: > On Fri, Oct 27, 2017 at 08:43:28AM -0400, Simo Sorce wrote: > > On Thu, 2017-10-26 at 22:14 +0200, Sumit Bose wrote: > > > On Thu, Oct 26, 2017 at 02:43:29PM -0400, Simo Sorce wrote: > > > > On Thu, 2017-10-26 at 12:16 +0200, Jakub Hrozek wrote: > > > > > On Wed, Oct 25, 2017 at 05:39:21PM +0200, Sumit Bose wrote: > > > > > > Hi, > > > > > > > > > > > > please find below the design document for the enhanced NSS > > > > > > API > > > > > > which > > > > > > makes e.g. the client side timeouts which where recently > > > > > > refactored > > > > > > available to callers. > > > > > > > > > > > > A more visual friendly version can be found at: > > > > > > https://pagure.io/fork/sbose/SSSD/docs/blob/07514ce52845d47 > > > > > > fe6b > > > > > > b327 > > > > > > f782a865bfa75628a/f/design_pages/enhanced_nss_api.rst > > > > > > > > > > > > bye, > > > > > > Sumit > > > > > > > > > > LGTM! > > > > > > > > > > > > > Looking at this I have some questions, if you are going to > > > > create a > > > > new > > > > library and just need to set a timeout it seem it would be a > > > > much > > > > better interface to use a context handle you allocate and pass > > > > into > > > > each call, and then have getters setters to set timeouts or any > > > > other > > > > flags that should influence the whole behavior. This will allow > > > > you > > > > also to control how many concurrent connections you want to > > > > have > > > > against sssd, as each new context will create a new socket > > > > connection > > > > and all. > > > > > > > > In the original libnss_sss.so we could not do that because the > > > > glibc > > > > interface does not offer any way to hold a context, but there > > > > is no > > > > reason to continue along that line in a *new* API. And not > > > > using > > > > contexts in threaded applications is generally a bad idea, as > > > > you > > > > end > > > > up *requiring* te use of mutexes when that is really not always > > > > a > > > > need > > > > (separated threads can open separate connections and not share > > > > any > > > > data > > > > that require mutexes). > > > > > > This sounds like a good 2.0 feature, are you interested in > > > creating a > > > more detailed design page for this? > > > > Sure. > > > > > Currently my goal was to reuse as > > > much or the current trusted code we already have. > > > > > > > > > > > On the responder side I also do not see why new calls are being > > > > created. You clearly want a client-wide behavior, introduce ONE > > > > new > > > > call that sets flags for the rest of the connection, and just > > > > reuse > > > > the > > > > usual commands otherwise. > > > > > > The current flags, like invalidating a cached entry are per- > > > request, > > > only the single object address by the current request should be > > > invalidate not all object which are requested on the same > > > connection. > > > > I would probably add a command to explicitly invalidate individual > > caches, this would avoid having special paths on every other call, > > resulting in cleaner code, at the cost of one more roundtrip > > though, so > > I guess it is a matter of figuring out what is the right balance > > here. > > > > > > > > > > I do not understand what is the point of nss_truste_users why a > > > > force > > > > reload is a privileged operation ? > > > > > > Since it can force expensive operations on the backends which > > > will > > > hit servers I think not everybody should be allowed to do this. > > > > You can already force this by requesting unexisting users/groups, I > > am > > not convinced this necessarily needs to be a privileged operation > > as > > there are already ways to cause work for SSSD. > > I would r
[SSSD] Re: Design document: Enhanced NSS API
On Thu, 2017-10-26 at 22:14 +0200, Sumit Bose wrote: > On Thu, Oct 26, 2017 at 02:43:29PM -0400, Simo Sorce wrote: > > On Thu, 2017-10-26 at 12:16 +0200, Jakub Hrozek wrote: > > > On Wed, Oct 25, 2017 at 05:39:21PM +0200, Sumit Bose wrote: > > > > Hi, > > > > > > > > please find below the design document for the enhanced NSS API > > > > which > > > > makes e.g. the client side timeouts which where recently > > > > refactored > > > > available to callers. > > > > > > > > A more visual friendly version can be found at: > > > > https://pagure.io/fork/sbose/SSSD/docs/blob/07514ce52845d47fe6b > > > > b327 > > > > f782a865bfa75628a/f/design_pages/enhanced_nss_api.rst > > > > > > > > bye, > > > > Sumit > > > > > > LGTM! > > > > > > > Looking at this I have some questions, if you are going to create a > > new > > library and just need to set a timeout it seem it would be a much > > better interface to use a context handle you allocate and pass into > > each call, and then have getters setters to set timeouts or any > > other > > flags that should influence the whole behavior. This will allow you > > also to control how many concurrent connections you want to have > > against sssd, as each new context will create a new socket > > connection > > and all. > > > > In the original libnss_sss.so we could not do that because the > > glibc > > interface does not offer any way to hold a context, but there is no > > reason to continue along that line in a *new* API. And not using > > contexts in threaded applications is generally a bad idea, as you > > end > > up *requiring* te use of mutexes when that is really not always a > > need > > (separated threads can open separate connections and not share any > > data > > that require mutexes). > > This sounds like a good 2.0 feature, are you interested in creating a > more detailed design page for this? Sure. > Currently my goal was to reuse as > much or the current trusted code we already have. > > > > > On the responder side I also do not see why new calls are being > > created. You clearly want a client-wide behavior, introduce ONE new > > call that sets flags for the rest of the connection, and just reuse > > the > > usual commands otherwise. > > The current flags, like invalidating a cached entry are per-request, > only the single object address by the current request should be > invalidate not all object which are requested on the same connection. I would probably add a command to explicitly invalidate individual caches, this would avoid having special paths on every other call, resulting in cleaner code, at the cost of one more roundtrip though, so I guess it is a matter of figuring out what is the right balance here. > > > > I do not understand what is the point of nss_truste_users why a > > force > > reload is a privileged operation ? > > Since it can force expensive operations on the backends which will > hit servers I think not everybody should be allowed to do this. You can already force this by requesting unexisting users/groups, I am not convinced this necessarily needs to be a privileged operation as there are already ways to cause work for SSSD. I would rather drop it. If we really want to deal with potential abuse we should introduce rate-limiting per uid, and basically slow down to a halt abusing clients by giving weights and possibly quotas, doling out privileges is cumbersome anyway and does not really prevent a malicious client to cause hard ATM. IMHO nss_trusted_users gets a NACK as a concept and should be dropped. > > > > I guess DNLSGTM ? > > A test-builds with a simplified version of the related patches solved > bottleneck issues with 389ds IPA modules. So at least it would help > without introducing other issues. I did not know we already had code, and I am not against the goal of improving the bottlenecks at all, but we should definitely improve the design here as this design is way too conservative and introduces APIs that we probably want to toss with a better design. So please mark those functions "private/tech preview/whatever" by putting them behind a guarding #ifdef in the header so that people really need to know what they are doing to use them and we can more easily deprecate and remove them quickly. Simo. > bye, > Sumit > > > > > Simo. > > > > -- > > Simo Sorce > > Sr. Principal Software Engineer > > Red Hat, Inc > > ___
[SSSD] Re: Design document: Enhanced NSS API
On Thu, 2017-10-26 at 12:16 +0200, Jakub Hrozek wrote: > On Wed, Oct 25, 2017 at 05:39:21PM +0200, Sumit Bose wrote: > > Hi, > > > > please find below the design document for the enhanced NSS API > > which > > makes e.g. the client side timeouts which where recently refactored > > available to callers. > > > > A more visual friendly version can be found at: > > https://pagure.io/fork/sbose/SSSD/docs/blob/07514ce52845d47fe6bb327 > > f782a865bfa75628a/f/design_pages/enhanced_nss_api.rst > > > > bye, > > Sumit > > LGTM! > Looking at this I have some questions, if you are going to create a new library and just need to set a timeout it seem it would be a much better interface to use a context handle you allocate and pass into each call, and then have getters setters to set timeouts or any other flags that should influence the whole behavior. This will allow you also to control how many concurrent connections you want to have against sssd, as each new context will create a new socket connection and all. In the original libnss_sss.so we could not do that because the glibc interface does not offer any way to hold a context, but there is no reason to continue along that line in a *new* API. And not using contexts in threaded applications is generally a bad idea, as you end up *requiring* te use of mutexes when that is really not always a need (separated threads can open separate connections and not share any data that require mutexes). On the responder side I also do not see why new calls are being created. You clearly want a client-wide behavior, introduce ONE new call that sets flags for the rest of the connection, and just reuse the usual commands otherwise. I do not understand what is the point of nss_truste_users why a force reload is a privileged operation ? I guess DNLSGTM ? Simo. -- Simo Sorce Sr. Principal Software Engineer Red Hat, Inc ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: kinit on IPA server does not exclusively talk to local KDC
On Thu, 2017-09-21 at 17:56 +0200, Sumit Bose wrote: > On Thu, Sep 21, 2017 at 11:23:20AM -0400, Simo Sorce wrote: > > On Thu, 2017-09-21 at 16:52 +0200, Lukas Slebodnik wrote: > > > Here you are. > > > local master: kvm-02-guest11.testrelm.test > > > replica: bkr-hv01-guest19.testrelm.test > > > > > > [root@kvm-02-guest11 ~]# cat /etc/krb5.conf > > > includedir /etc/krb5.conf.d/ > > > includedir /var/lib/sss/pubconf/krb5.include.d/ > > > > > > [logging] > > > default = FILE:/var/log/krb5libs.log > > > kdc = FILE:/var/log/krb5kdc.log > > > admin_server = FILE:/var/log/kadmind.log > > > > > > [libdefaults] > > > default_realm = TESTRELM.TEST > > > dns_lookup_realm = false > > > dns_lookup_kdc = true > > > > This sounds wrong on a master > > no, you need this to find any AD DC in a trusted forest. Shouldn't SSSD do that for us via proper site discovery ? Simo. > bye, > Sumit > > > > > Simo. > > > > -- > > Simo Sorce > > Sr. Principal Software Engineer > > Red Hat, Inc > > -- Simo Sorce Sr. Principal Software Engineer Red Hat, Inc ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: kinit on IPA server does not exclusively talk to local KDC
On Thu, 2017-09-21 at 16:52 +0200, Lukas Slebodnik wrote: > Here you are. > local master: kvm-02-guest11.testrelm.test > replica: bkr-hv01-guest19.testrelm.test > > [root@kvm-02-guest11 ~]# cat /etc/krb5.conf > includedir /etc/krb5.conf.d/ > includedir /var/lib/sss/pubconf/krb5.include.d/ > > [logging] > default = FILE:/var/log/krb5libs.log > kdc = FILE:/var/log/krb5kdc.log > admin_server = FILE:/var/log/kadmind.log > > [libdefaults] > default_realm = TESTRELM.TEST > dns_lookup_realm = false > dns_lookup_kdc = true This ^^^^ sounds wrong on a master Simo. -- Simo Sorce Sr. Principal Software Engineer Red Hat, Inc ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Changes to default ccache in krb5.conf
On Wed, 2017-05-31 at 10:59 +0200, Jakub Hrozek wrote: > On Wed, May 31, 2017 at 10:31:38AM +0200, Lukas Slebodnik wrote: > > ehlo, > > > > I had a discussion with QEs and realized that sssd need to be > > restarted > > if default_ccache_name is changed in krb5 configuration files. > > > > The reason is that we cache the value but do not refresh it. > > https://pagure.io/SSSD/sssd/blob/master/f/src/providers/krb5/krb5_c > > ommon.c#_264 > > > > We might changed that using inotify. But we would need to change. > > I am not sure whether it will be trivail to change because we would > > need to > > change cached value in "struct dp_option *opts" for all domains > > (including > > subdomains) > > > > ATM the safest way is to restart sssd. But do we want to be more > > flexible here? > > We could do one thing that Simo proposed some time ago which is to > not > cache the KRB5CCNAME at all if it only contains 'predictable' > components. > > For example, KEYRING:$uid or KCM: don't need to be cached at all. > FILE:krb5ccname_X does. +1 Simo. ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Design document - SSSD KCM server
On Wed, 2017-04-05 at 21:02 +0200, Jakub Hrozek wrote: > On Tue, Nov 22, 2016 at 08:51:10AM +0100, Jakub Hrozek wrote: > > Hi, > > > > I was working on a KCM server for SSSD for some time already in parallel > > with the files provider and had some discussions with Simo as well. Of > > course my intent wasn't to implement a feature secretly without a design > > review, but to have a prototype to base a proper design on :) > > > > However it makes sense to have a peer-reviewed design page now, also > > because of Fedora's move towards Kerberos and KDC proxy, which leads to > > questions on the Fedora lists about ccache renewals and so on -- so I > > think it makes sense to pitch the design to Fedora at least already.. > > > > Here is the design page: > > https://fedorahosted.org/sssd/wiki/DesignDocs/KCM > > and here is the code of the KCM responder so far: > > https://github.com/jhrozek/sssd/tree/kcm > > Hi, > > I moved the design doc to pagure: > https://docs.pagure.org/SSSD.sssd/design_pages/kcm.html > > It's been cleaned up and reconciled with the implementation. It looks really nice with the docs formatting/font/style :-) .. and the content LGTM too. Simo. -- Simo Sorce Sr. Principal Software Engineer Red Hat, Inc ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Design discussion: Support for non-POSIX users and groups
On Mon, 2017-03-06 at 14:49 +0100, Jakub Hrozek wrote: > [sssd] > > domains = appdomain.test, posixdomain.test > > services = ifp, pam, nss > > > > [pam] > > application_services = sss_test > > > > [ifp] > > user_attributes = +phone > > > > [domain/appdomain.test] > > id_provider = ldap > > ldap_uri = ldap://server.test > > domain_type = application > > ldap_user_extra_attrs = phone:telephoneNumber > > > > [domain/posixdomain.test] > > id_provider = ldap > > ldap_uri = ldap://server.test Jakub, I am thinking we may want to make some things here slightly different. Instead of calling them all domains, and having to sort out the differences later, I am thinking we may want to define a completely new section type called "application", this will allow us to "inherit" configuration from domain for compactness when both posix and non-posix are needed from the same domain. Example: [sssd] domains = domain.test services = ifp, pam, nss [pam] application_services = sss_test [ifp] user_attributes = +phone [domain/domain.test] id_provider = ldap ldap_uri = ldap://server.test [application/domain.test] inherit_from = domain/domain.test ldap_user_extra_attrs = phone:telephoneNumber The idea is that the application domain inherits most of its parameters from the posix domain references (if any is referenced). If no difference between application domain and posix domain is needed then the application domain section could be almost completely empty (win). I am not sure if we need to explicitly list application domains in the sssd or PAM sections, or if we should just automatically have them available by the simple fact they have been defined. One thing we may want though is to have an option that explicitly maps application domains to specific "services", for both PAM and IFP at the same time (given it makes no sense to do that independently and both need to be on the same page for this). So move to a new section and modify the application_services parameter to take tuples: [applications] application_services = apache:domain.test myapp:other.dom \ otherapp:domain.test,other.dom,third.dom \ fooapp:ALL The other option may be to list what services each application domain will be made available to in the application domain definition itself: [application/domain.test] available_for = ALL [application/other.dom] available_for = apache, otherapp, fooapp [application/third.dom] available_for = otherapp I think the problem will be to find the right balance to make it simple to configure which apps can get to which domains, the main trick being that we should not make application domains available to things like ssh, gdm, login by default because then NSS would fail later on and we'd waste time. Also we need to be careful in never attempting double authentication (both to the posix domain and then again via application domain for any single app, as we may cause premature account locks that way). So perhaps explicitly listing services is always needed after all, and when a service is listed then only the specific list of application domains is considered for that service and nothing else. Just a few ideas to be able to better control what app does what. I am not sure we need different query types in the protocol either, because the above configuration will make it pretty clear what apps should get from the IFP, and backends will be of the appropriate type already so when you contact one they already know what they should return in their cache. Hope this makes sense and complements the design rather than disrupt it, it does complement it in my mind :-) Simo. ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Design discussion: Support for non-POSIX users and groups
On Wed, 2017-03-08 at 11:39 +0100, Jakub Hrozek wrote: > On Wed, Mar 08, 2017 at 10:45:32AM +0100, Pavel Březina wrote: > > On 03/07/2017 03:11 PM, Jakub Hrozek wrote: > > > On Tue, Mar 07, 2017 at 02:31:27PM +0100, Pavel Březina wrote: > > > > On 03/07/2017 01:33 PM, Jakub Hrozek wrote: > > > > > On Tue, Mar 07, 2017 at 01:18:36PM +0100, Pavel Březina > > > > > wrote: > > > > > > On 03/07/2017 01:16 PM, Pavel Březina wrote: > > > > > > > On 03/06/2017 02:49 PM, Jakub Hrozek wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > I prepared a design page for a new feature about > > > > > > > > fetching and > > > > > > > > authenticating non-POSIX users: > > > > > > > > https://docs.pagure.org/SSSD.sssd/design_pages/non_ > > > > > > > > posix_support.html > > > > > > > > > > > > > > > > For your convenience, I'm also copying the .rst text > > > > > > > > below: > > > > > > > > > > > > > > > > Support for non-POSIX users and groups > > > > > > > > == > > > > > > > > > > > > > > > > Related ticket(s): > > > > > > > > -- > > > > > > > > https://pagure.io/SSSD/sssd/issue/3310 > > > > > > > > > > > > > > I find this document quite hard to understand, so I want > > > > > > > to ensure I get > > > > > > > it right: > > > > > > > > > > > > > > 1) You can't have one domain that return both posix and > > > > > > > non-posix users. > > > > > > > 2) PAM is allowed to login a non-posix users for given > > > > > > > services. > > > > > > > 3) If CACHE_REQ_APP is used, non-posix domains are > > > > > > > searched first then > > > > > > > posix domains. > > > > > > > 4) If CACHE_REQ_POSIX is used, non-posix domains are > > > > > > > skipped. > > > > > > > 5) Non-posix domains require fully qualified name. > > > > > > > 6) Posix users return only posix groups membership. > > > > > > > 7) Non-posix users return both posix and non-posix > > > > > > > membership. > > > > > > > > > > > > And > > > > > > 8) You can have two users, one posix, one non-posix with > > > > > > the same name. > > > > > > > > > > In theory yes, but I don't think this would be too common. In > > > > > general > > > > > you could have two entries, one with objectclass user, the > > > > > other with > > > > > objectclass posixUser where each domain would use a different > > > > > attribute > > > > > for the username. But even so, I think the current scheme > > > > > would protect > > > > > us against these strange setups. > > > > > > > > If find this rather complicated at least from what we talked > > > > about on irc. > > > > If I recall correctly, we leaned in a way that we always > > > > download the user > > > > whether it is posix or not and then let the caller decide if it > > > > should be > > > > returned by sssd. I.e. > > > > > > > > if !non_posix_users_enabled(domain) then > > > > download only posix users > > > > else > > > > download user even if it is non posix > > > > > > > > In NSS (and other posix responders) we would return ENOENT for > > > > non-posix > > > > users. In IFP we would not care (or care if we change API to > > > > select). > > > > > > > > This wouldn't require the domain separation. What were the > > > > reasons to not > > > > use this approach? > > > > > > The conflicts between different 'views'. Consider the case where > > > an IFP > > > user would request the groups of a user who is a member of both > > > POSIX and > > > non-POSIX groups. Then, a second later, the NSS responder calls > > > initgroups. > > > > > > What does the initgroups POSIX call on the back end level do with > > > the > > > non-POSIX groups, especially the leaf ones? Does it remove them? > > > Do we > > > add logic to ignore the non-POSIX groups? How do we tell if the > > > groups > > > were removed from the server if the searches match only the POSIX > > > data > > > in the second request? > > > > In this approach, backend is supposed to always download both posix > > and > > non-posix users. Backend should not be aware about posix attributes > > at all. > > Responder should decide whether non-posix users should be removed > > from the > > result (not from the cache) or not. > > This would be hugely inefficient and e.g. logins would take forever. Inefficiency is secondary, the problem is that you would very quickly get requests to have different filters for posix users and non-posix users, and you end up adding a ton of options for the two distinct cases. It is better if the users (and groups) for an application have the filters required for that application and exactly only those filters, or you'll pretty quickly have to add filtering both in the backend and in the NSS frontend and the code becomes super-complicated == tons of bugs and regressions. If applications want their own view they get their special domain and then everything can be tweaked as wanted w/o impacting the underlying system users and their domain settings. Simo.
[SSSD] Re: RFC: Socket-activation, some changes in the architecture.
On Mon, 2017-01-09 at 13:35 +0100, Jakub Hrozek wrote: > On Mon, Jan 09, 2017 at 01:25:48PM +0100, Pavel Březina wrote: > > On 01/08/2017 09:44 PM, Fabiano Fidêncio wrote: > > > People, > > > > > > Recently I've faced some issues when testing the socket-activation > > > working running as sssd-user, which will force me to take a different > > > path for a few things and I really would like to know your opinion on > > > those things. > > > > > > So, currently, this is what the nss.service looks like: > > > > > > [Unit] > > > Description=SSSD NSS Service responder > > > Documentation=man:sssd.conf(5) > > > After=sssd.service > > > BindsTo=sssd.service > > > > > > [Install] > > > Also=sssd-nss.socket > > > > > > [Service] > > > ExecStartPre=-/bin/chown @SSSD_USER@:@SSSD_USER@ @logpath@/sssd_nss.log > > > ExecStart=@libexecdir@/sssd/sssd_nss --debug-to-files --unprivileged-start > > > Restart=on-failure > > > User=@SSSD_USER@ > > > Group=@SSSD_USER@ > > > PermissionsStartOnly=true > > > > > > As you probably noticed, I've been using systemd's machinery to change > > > the debug files' owner and to start the responder by the proper user > > > (sssd or root). Well, it doesn't work that well as expected as systemd > > > ends up calling initgroups(sssd, ...) in order to start any service > > > using "sssd" user and this call is done _before_ starting the NSS > > > responder, which will hang for the "default client timeout" (300s). > > > > > > Okay, we have to change it and here is where I need your help! > > > > The simplest solution would be to disable socket activation for NSS > > responder. Socket activation is supposed to be used for responders that are > > seldom used. > > I also wonder if this was the easiest. Just enable the service as well > in the RPM.. Once we start handling local users (and we want to do that by default) we'll have to always run as root anyway, so what's the point of dealing with changing the user ? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Design document - Socket-activatable responders
On Thu, 2016-12-01 at 15:22 +0100, Pavel Březina wrote: > On 12/01/2016 02:56 PM, Simo Sorce wrote: > > On Thu, 2016-12-01 at 14:44 +0100, Pavel Březina wrote: > >> On 11/24/2016 02:33 PM, Fabiano Fidêncio wrote: > >>> The design page is done [0] and it's based on this discussion [1] we > >>> had on this very same mailing list. A pull-request with the > >>> implementation is already opened [2]. > >>> > >>> [0]: > >>> https://fedorahosted.org/sssd/wiki/DesignDocs/SocketActivatableResponders > >>> [1]: > >>> https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org/message/H6JOF5SGGSIJUIWYNANDA73ODHWBS7J2/ > >>> [2]: https://github.com/SSSD/sssd/pull/84 > >> > >> I think we should also provide 'disabled_services' option, to give > >> admins a way to explicitly disable some responders if the don't want to > >> used them. > > > > How would this work ? > > If responder is listed in disabled_services, it won't be allowed to > start via socket activation. If disabling the socket as Fabiano > mentioned in the other mail is enough, I'm fine with it, plese test. I am not sure this is a good behavior as clients will see a connection being accept and then dropped, and may misbehave or report strange errors. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Design document - Socket-activatable responders
On Thu, 2016-12-01 at 15:19 +0100, Fabiano Fidêncio wrote: > On Thu, Dec 1, 2016 at 2:44 PM, Pavel Březina <pbrez...@redhat.com> wrote: > > On 11/24/2016 02:33 PM, Fabiano Fidêncio wrote: > >> > >> The design page is done [0] and it's based on this discussion [1] we > >> had on this very same mailing list. A pull-request with the > >> implementation is already opened [2]. > >> > >> [0]: > >> https://fedorahosted.org/sssd/wiki/DesignDocs/SocketActivatableResponders > >> [1]: > >> https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org/message/H6JOF5SGGSIJUIWYNANDA73ODHWBS7J2/ > >> [2]: htatps://github.com/SSSD/sssd/pull/84 > > > > > > I think we should also provide 'disabled_services' option, to give admins a > > way to explicitly disable some responders if the don't want to used them. > > I have to double check a few things here but, AFAIU, just having the > socket disabled (systemctl disable sssd-@responder@.socket) should be > enough. I guess I misunderstood what ou mean by "provide 'disabled_services' option", I though you wanted to add that option to sssd.conf Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Design document - Socket-activatable responders
On Thu, 2016-12-01 at 14:44 +0100, Pavel Březina wrote: > On 11/24/2016 02:33 PM, Fabiano Fidêncio wrote: > > The design page is done [0] and it's based on this discussion [1] we > > had on this very same mailing list. A pull-request with the > > implementation is already opened [2]. > > > > [0]: > > https://fedorahosted.org/sssd/wiki/DesignDocs/SocketActivatableResponders > > [1]: > > https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org/message/H6JOF5SGGSIJUIWYNANDA73ODHWBS7J2/ > > [2]: https://github.com/SSSD/sssd/pull/84 > > I think we should also provide 'disabled_services' option, to give > admins a way to explicitly disable some responders if the don't want to > used them. How would this work ? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Design document - SSSD KCM server
gt; === How To Test ===
> > In order for the admin to start using the KCM service, the sssd-kcm
> > responder's systemd service must be enabled. Then, libkrb5 must also be
> > configured to use KCM as its default ccache type in `/etc/krb5.conf`
> > {{{
> > [libdefaults]
> > default_ccache_name = KCM
> > }}}
> >
> > After that, all common operations like kinit, kdestroy or login through
> > pam_sss should just work and store their credentials in the KCM server.
> >
> > The KCM server must implement access control correctly, so even
> > trying to access other user's KCM credentials by setting KRB5CCNAME to
> > `KCM:1234:RESIDUAL` would not work (except for root).
> >
> > Restarting the KCM server or rebooting the machine must persist the tickets.
> >
> > As far as automatic unit and integration testing is required, we need to
> > make
> > sure that MIT's testsuite passes with Kerberos ccache defaulting to KCM
> > and SSSD KCM deamon running. In the SSSD upstream, we should write
> > integration tests that run a MIT KDC under socket_wrapper to exercise the
> > KCM server.
> >
>
> How do containers access the KCM? Would they have to run their own copy
> internal
> to the container? Would we bind-mount the /var/run/.heim_org.h5l.kcm-socket
> and
> then work some namespacing magic in the host?
Deployment specific, I can see either way as an option depending on what
you are doing.
> You call out in the introduction that this will help address container
> use-cases, but then don't describe that implementation. This seems like an
> important piece of the puzzle that should be added to the page (or made more
> clear, since if it's in there, I can't spot it).
What would you want to call out exactly ?
Simo.
--
Simo Sorce * Red Hat, Inc * New York
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: about letting the responder choose the sysdb optimization level
On Wed, 2016-11-09 at 13:08 +0100, Sumit Bose wrote:
> On Tue, Nov 08, 2016 at 10:28:20AM +0100, Jakub Hrozek wrote:
> > Hi,
> >
> > I would like to ask for opinions about:
> > https://fedorahosted.org/sssd/ticket/3126
> >
> > The basic idea is that the responder would choose what kind of optimization
> > would the back end perform when saving the sysdb entries. Requests that
> > just return information might choose to optimize very aggressively (using
> > modifyTimestamp) and requests that actually authenticate or authorize the
> > user might choose to not optimize at all to avoid issues like the ones we
> > saw
> > with virtual attributes that don't bump the modifyTimestamp attribute at
> > all.
> >
> > On the responder side, this is quite easy, just send an additional flag
> > during the responder request. It's the provider part I'm not so sure
> > about, because there the optimizations are performed at the sysdb level.
> >
> > So far I can only think about extending sysdb_transaction_start() (or
> > providing sysdb_opt_transaction_start and letting the old
> > sysdb_transaction_start default to no optimization) which would
> > internally keep track of the active transaction and the optimization we
> > want to perform. Since only sssd_be is the cache writer and there is
> > only one cache per domain.
> >
> > Additionally, we would have to keep the transaction optimization level
> > around
> > in some context until the request bubbles from the data provider handler to
> > actually saving the transaction. I don't I hope this won't be too messy, but
> > since the requests are asynchronous, so far I don't see any way around
> > it. The only thing that might be less messy in the long term is to provide
> > a bit more generic structure ("request status") that would so far only
> > include the optimization level and later might be extended to include
> > e.g. intermediate data. But on the other hand, I'm not sure I have
> > thought about passing the data between requests hard enough to design
> > this properly. Should I?
> >
> > Any other opinions? Thoughts?
>
> I'm not sure if this wouldn't confuse users. If I understood the
> proposal correctly the typical nss calls like getpwnam(), getgrnam() etc
> will use the optimized requests with modifyTimestamp checks. If there is
> a server which does not change modifyTimestamp on updates changes in
> e.g. the user's shell or in the list of group-members will not be
> visible even after calling 'sss_cache -E' because modifyTimestamp didn't
> change on the server. But typically a user does not log in again to
> check for such changes but just calls e.g. 'getent group changed_group'
> again and again waiting for changes.
>
> I think if there are really such servers out there which do not update
> modifyTimestamp it should be just possible to switch of the time-stamp
> cache generally in this case (I wonder if this is already possible by
> setting ldap_{user|group|netgroup}_modify_timestamp to a non-existing
> attribute?). In this case the number of cache updates can be reduced by
> increasing the cache timeout because during authenticating and
> authorization fresh data is requested from the backend anyways.
There are cases where things like CoS are used that can change an
attribute value (even dynamically) but do not alter the modifyTimestamp.
In general operational attributes can behave that way.
> I think the time would be better spend e.g. on
> https://fedorahosted.org/sssd/ticket/3211 "Refactor the
> sdap_async_groups.c module" and make sure all needed data is read from
> LDAP (multiple LDAP connections might be involved here) first and
> written to the cache in a single transaction.
The problem is deciding when to forcibly reload data we already have in
the cache, regardless of cache status.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: [Freeipa-devel] [RFC] Matching and Mapping Certificates
On Thu, 2016-10-13 at 18:52 +0200, Sumit Bose wrote: > Compatibility with Active Directory > Active Directory uses a per-user LDAP attribute > [https://msdn.microsoft.com/en-us/library/cc220106.aspx > altSecurityIdentities] to allow arbitrary user-certificate mappings is there > is no suitable user-principal-name entry in the SAN of the certificate. > > Unfortunately it is more or less undocumented how AD use the values of > this attribute. The best overview I found is in > https://blogs.msdn.microsoft.com/spatdsg/2010/06/18/howto-map-a-user-to-a-certificate-via-all-the-methods-available-in-the-altsecurityidentities-attribute/. A few more pointers Sumit: - This describes what is allowed for users: https://msdn.microsoft.com/en-us/library/ms677943%28v=vs.85%29.aspx - This describes a use for devices: https://msdn.microsoft.com/en-us/library/dn408946.aspx - additional description specific for PKINIT: https://msdn.microsoft.com/en-us/library/hh536384.aspx - This is a good detailed overview of the Smart Card logon workflow in windows, it describes Vista but I do not think it changed in fundamental ways in following releases: https://msdn.microsoft.com/en-us/library/bb905527.aspx NOTE: Please look at the small paragraph named "Smart card logon across forests", we definitely want to think about this problem as well from the get-go and not try to retrofit something later on. HTH, Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Monotonic clock for timed events
On Wed, 2016-10-12 at 10:52 +0200, Pavel Březina wrote: > On 10/11/2016 03:26 PM, Simo Sorce wrote: > > On Mon, 2016-10-10 at 14:04 +0200, Pavel Březina wrote: > >> On 10/10/2016 10:09 AM, Fabiano Fidêncio wrote: > >>> Victor, > >>> > >>> On Mon, Oct 10, 2016 at 10:04 AM, Victor Tapia > >>> <victor.ta...@canonical.com> wrote: > >>>> Hi list, > >>>> > >>>> I've faced a race condition when SSSD boots in a machine with a big > >>>> clock drift. This is what I see: > >>>> > >>>> 1. SSSD starts before the network is up, queries the LDAP server without > >>>> success and sets a retry timer (~60 secs) > >>>> 2. NTP starts and corrects the clock, 1 hour back for example. > >>>> 3. SSSD takes ~60 secs + the drift correction (1 hour) to retry the > >>>> connection. > >>>> > >>>> In this particular scenario the credentials cache is disabled, so the > >>>> wait time to login is noticeable. How feasible would it be to use a > >>>> monotonic clock for this kind of timed events? > >>> > >>> Are you running git master? This issue is supposed to be already > >>> solved by > >>> https://github.com/SSSD/sssd/commit/b8ceaeb80cffb00c26390913ea959b77f7e848b9 > >> > >> This patch fix the issue only in watchdog which would result in > >> terminating sssd otherwise. Fixing it across whole sssd would be > >> difficult. The fix should go to tevent. > > > > It also seem to fix the issue only if the time jumps backwards, not if > > it jumps forward, in that case if I read the code right, we'd still end > > up killing sssd. > > Yes, we don't need0 to care about forward jump since that means all > tevent timers that are within this time shift are fired anyway. Well I do care if sssd kills all children just because I suspended the laptop for a while :-) Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Monotonic clock for timed events
On Mon, 2016-10-10 at 11:19 +0200, Jakub Hrozek wrote: > On Mon, Oct 10, 2016 at 11:09:35AM +0200, Victor Tapia wrote: > > El 10/10/16 a las 10:56, Ian Kent escribió: > > > On Mon, 2016-10-10 at 10:42 +0200, Jakub Hrozek wrote: > > >> On Mon, Oct 10, 2016 at 10:04:30AM +0200, Victor Tapia wrote: > > >>> Hi list, > > >>> > > >>> I've faced a race condition when SSSD boots in a machine with a big > > >>> clock drift. This is what I see: > > >>> > > >>> 1. SSSD starts before the network is up, queries the LDAP server without > > >>> success and sets a retry timer (~60 secs) > > >>> 2. NTP starts and corrects the clock, 1 hour back for example. > > >>> 3. SSSD takes ~60 secs + the drift correction (1 hour) to retry the > > >>> connection. > > >>> > > >>> In this particular scenario the credentials cache is disabled, so the > > >>> wait time to login is noticeable. How feasible would it be to use a > > >>> monotonic clock for this kind of timed events? > > >> > > >> I really have not tried this and I guess I don't know tevent internals > > >> well enough if this works, but I wonder if just using: > > >> clock_getime() > > > > > > With a CLOCK_MONOTONIC I presume? > > > I think that's what has been suggested. > > > > > > > I was thinking about using CLOCK_MONOTONIC_RAW: > > > > CLOCK_MONOTONIC_RAW (since Linux 2.6.28; Linux-specific) > > Similar to CLOCK_MONOTONIC, but provides access to a raw hardware-based > > time that is not subject to NTP adjustments or the incremental > > adjustments performed by adjtime(3). > > > > >> and constructing struct timeval > > >> in place of: > > >> tevent_timeval_current_ofs() > > >> could solve this particular issue. > > >> > > >> On the other hand, this is a pattern we use in SSSD all through the code > > >> for timed events and we're just not well equipped to handle time drifts. > > >> Did you investigate why doesn't sssd detect the networking change from > > >> libnl messages or from resolv.conf being touched? > > > > I didn't dig much into it yet (I just checked tevent to confirm it uses > > gettimeofday()), so I'll take this as my next step. > > btw the samba-technical mailing list is the best source of info about > libtevent and the best place to ask questions about libtevent.. We should suggest that libtevent starts using timerfd with epoll to do time tracking instead of the internal computations, and then a monotonic clock can be used with some calculation at set time to convert the time of day to the current monotonic clock. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Monotonic clock for timed events
On Mon, 2016-10-10 at 14:04 +0200, Pavel Březina wrote: > On 10/10/2016 10:09 AM, Fabiano Fidêncio wrote: > > Victor, > > > > On Mon, Oct 10, 2016 at 10:04 AM, Victor Tapia > > <victor.ta...@canonical.com> wrote: > >> Hi list, > >> > >> I've faced a race condition when SSSD boots in a machine with a big > >> clock drift. This is what I see: > >> > >> 1. SSSD starts before the network is up, queries the LDAP server without > >> success and sets a retry timer (~60 secs) > >> 2. NTP starts and corrects the clock, 1 hour back for example. > >> 3. SSSD takes ~60 secs + the drift correction (1 hour) to retry the > >> connection. > >> > >> In this particular scenario the credentials cache is disabled, so the > >> wait time to login is noticeable. How feasible would it be to use a > >> monotonic clock for this kind of timed events? > > > > Are you running git master? This issue is supposed to be already > > solved by > > https://github.com/SSSD/sssd/commit/b8ceaeb80cffb00c26390913ea959b77f7e848b9 > > This patch fix the issue only in watchdog which would result in > terminating sssd otherwise. Fixing it across whole sssd would be > difficult. The fix should go to tevent. It also seem to fix the issue only if the time jumps backwards, not if it jumps forward, in that case if I read the code right, we'd still end up killing sssd. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: Monotonic clock for timed events
On Mon, 2016-10-10 at 10:04 +0200, Victor Tapia wrote: > Hi list, > > I've faced a race condition when SSSD boots in a machine with a big > clock drift. This is what I see: > > 1. SSSD starts before the network is up, queries the LDAP server without > success and sets a retry timer (~60 secs) > 2. NTP starts and corrects the clock, 1 hour back for example. > 3. SSSD takes ~60 secs + the drift correction (1 hour) to retry the > connection. > > In this particular scenario the credentials cache is disabled, so the > wait time to login is noticeable. How feasible would it be to use a > monotonic clock for this kind of timed events? We should use a monotonic clock for most internal events. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] Re: [PATCH] GPO: Cat vals with same key from different GPOs
On Wed, 2016-08-31 at 17:41 +0200, Michal Židek wrote: > Hi, > > here is patch for ticket #3161. > > See more in the ticket description. > > I was thinking why we originally replaced > the lists and I think it comes from confusion > on how we handle the same keys in single > GPO ini file, however that is handled by > libini not by SSSD. Sorry to come to this late, but do you have a documentation reference that says that merging is the correct behavior ? I forgot a lot about how multiple GPOs are supposed to be merged but I seem to recall there may be a policy that actually controls how merging is done. CCing Günther who has worked around GPO processing a few years ago. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [RFCv3] NSS tlog integration
On Mon, 2016-08-29 at 22:47 +0200, Sumit Bose wrote: > About Simo's concern. If there are groups defined in the > session_recording configuration you look up those groups with > cache_req_group_by_name_send(). As long as the groups are in the cache > and not expired they are returned directly from the cache. I think > this lookup cannot be avoided because in theory the groups name, be it > the original name or an override name, might change. And you want to > be sure that the name listed in the configuration is still a valid > group name. My concern is that an initgroups call can be quite expensive in certain domains where a user is a member of many groups. If we really want to check memberships based on session recording configuration *at query time* rather than at storing time, I would think it would be more efficient to resolve only the group memberships of the groups listed in the session recording configuration, and then check if the user is member of any of those groups. Given in general this kind of access is geared toward auditing the work of small groups of people (admins with root access or similar), it would probably be more efficient than calling initgroups, as it will end up pulling a less data, less often. If session recording is invoked often those groups will be cached so very few lookups would be made even if many different users log into the system. If you call initgroups instead you may end up updating several dozen groups for each user login. What do you think ? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [RFCv3] NSS tlog integration
On Thu, 2016-08-25 at 14:24 +0300, Nikolai Kondrashov wrote: > Hi Simo, > > Thanks for looking at the patches and for the feedback! > I'm replying below. > > On 08/24/2016 11:24 PM, Simo Sorce wrote: > > On Tue, 2016-08-23 at 17:24 +0300, Nikolai Kondrashov wrote: > >> Hi everyone, > >> > >> Attached is the third version of work-in-progress SSSD/tlog integration > >> patches. I'm sending them in the hope that somebody takes a look and > >> perhaps points out some wrong bits I can fix before I'm too dependent on > >> them. > >> > >> The changes from the last version is some refactoring of the NSS and the > >> common parts, plus start of the PAM part of the implementation. > >> > >> Also, at this point, I think I could contribute some general fixes and > >> prerequisite refactoring patches separately. > > > > So I have been going through the patchset and I have concerns about how > > you are determining if the shell needs to be substituted with the > > session recording shell. > > It seem you do this work every single time a getpwname/uis/etc request > > is run. this is very expensive as you do a full group search on each of > > those requests, to find data that arguably rarely changes. > > > > I think in general this should be done at "write" time not at "read" > > time. > > Ie whenever the the session recording configuration changes or when a > > new user is written in the cache, then you should check if session > > recording apply to this user and write an attribute in the user entry. > > > > On getpwnam/uid/ent calls you would look for those calls and replace the > > shell entry accordingly. > > > > Unless there is some very good reason to do it always at query time this > > is, I am afraid, a nack on the approach. > > I'm not sure I fully understand your suggestion, but I like it so far. > I wanted to do something like this from the start, but Sumit had some concerns > which I can't quite remember now. > > Sumit, maybe you can take a look at this? > > I'll try to describe what I think you mean. So, we can modify the > user cache-filling machinery to look at the session recording configuration > upon adding a new user, and if session recording is enabled for the user > (in whatever way), add an attribute signifying that. Then NSS could just check > that attribute and substitute the shell when needed. I assume PAM would also > be able to do that. Although it would still need the shell override logic, > currently residing in NSS (I started another thread on this). Is that what you > mean? Yes > Still, I have a bunch of concerns about this, which perhaps you can help me > resolve. > > Would it be OK for the cache machinery to also pull groups into the cache, for > which session recording is enabled, so that it can check if the user belongs > to them? It would need to fetch them by names that the administrator would put > into the configuration. I.e. with all the overrides, whitespace, and other > changes. I am a bit concerned that this list is based on group names in general, how do you deal with local groups that list a remote user as member ? > Similarly, would the cache machinery be in a good position to understand the > names of the users listed in the session recording configuration? I.e. I > expect the administrators would like to put there names as they see them. Also > with overrides, whitespace and other tweaks NSS does. You have access to the same configuration rules that the frontend has access to if you need, so you can do whatever mapping is needed at store time. > Then, I expect we need to check whether a user should be recorded also on > cache refresh, not only on initial addition and configuration change. Because > groups can go away, or their names might change due to overrides. The check needs to be performed any time that: - a user is added or refreshed - a group is added or refreshed (memberships changed) - the session recording configuration is changed > Lastly, user names might change due to overrides. Can this approach handle it? afaik overrides are also store in cache so it can be taken in account. I am curious about what are Sumit concerns though. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [RFCv3] NSS tlog integration
On Tue, 2016-08-23 at 17:24 +0300, Nikolai Kondrashov wrote: > Hi everyone, > > Attached is the third version of work-in-progress SSSD/tlog > integration > patches. I'm sending them in the hope that somebody takes a look and > perhaps > points out some wrong bits I can fix before I'm too dependent on them. > > The changes from the last version is some refactoring of the NSS and > the > common parts, plus start of the PAM part of the implementation. > > Also, at this point, I think I could contribute some general fixes and > prerequisite refactoring patches separately. So I have been going through the patchset and I have concerns about how you are determining if the shell needs to be substituted with the session recording shell. It seem you do this work every single time a getpwname/uis/etc request is run. this is very expensive as you do a full group search on each of those requests, to find data that arguably rarely changes. I think in general this should be done at "write" time not at "read" time. Ie whenever the the session recording configuration changes or when a new user is written in the cache, then you should check if session recording apply to this user and write an attribute in the user entry. On getpwnam/uid/ent calls you would look for those calls and replace the shell entry accordingly. Unless there is some very good reason to do it always at query time this is, I am afraid, a nack on the approach. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [SSSD/sssd #5] Miscellanous patches for the sssd-secrets responder (opened)
On Fri, 2016-08-19 at 17:23 +0200, Lukas Slebodnik wrote: > On (19/08/16 09:38), Simo Sorce wrote: > >On Fri, 2016-08-19 at 11:20 +0200, Lukas Slebodnik wrote: > >> On (19/08/16 10:41), Jakub Hrozek wrote: > >> >On Fri, Aug 19, 2016 at 10:39:27AM +0200, Lukas Slebodnik wrote: > >> >> On (19/08/16 10:25), sssd-github-notificat...@fedorahosted.org wrote: > >> >> >jhrozek's pull request #5: "Miscellanous patches for the sssd-secrets > >> >> >responder" was opened > >> >> > > >> >> >PR body: > >> >> >The first patch just makes an internal function static. The other just > >> >> >makes the use of a config section more uniform. We could go one way or > >> >> >the other, I don't really mind which, but we shouldn't use two ways to > >> >> >access the same data. > >> >> > > >> >> >See the full pull-request at https://github.com/SSSD/sssd/pull/5 > >> >> > >> >> Would it be possible to sent patches in mail? > >> >> samba does it. It would be a hint for me wheter it worth > >> >> to review a patch in web interface. > >> > > >> >It's possible, but not implemented. I would like to send a separate mail > >> >about the proposed workflow anyway. > >> Then please attach mails to the thread :-) > > > >+1 > > > Simo, I thought it would be for you to see patches from github. > So you needn't watch every PR in GH but I didn't want to be your > spokesman :-) > I am actually happy to use both, but in different ways. Mail is more of a batch/notification thing. Github pages are more of a "let's see what need to be acted upon". If it were only on github I would have to chase closed PRs, as I am usually "late to the party", so for "catching up" email is much better. But for the case where you just want to see what is open and may need review, going to the open PR page is easier as you can have a quick glance at what is pending (just like with patchwork, which fulfilled this role). Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [SSSD/sssd #5] Miscellanous patches for the sssd-secrets responder (opened)
On Fri, 2016-08-19 at 11:20 +0200, Lukas Slebodnik wrote: > On (19/08/16 10:41), Jakub Hrozek wrote: > >On Fri, Aug 19, 2016 at 10:39:27AM +0200, Lukas Slebodnik wrote: > >> On (19/08/16 10:25), sssd-github-notificat...@fedorahosted.org wrote: > >> >jhrozek's pull request #5: "Miscellanous patches for the sssd-secrets > >> >responder" was opened > >> > > >> >PR body: > >> >The first patch just makes an internal function static. The other just > >> >makes the use of a config section more uniform. We could go one way or > >> >the other, I don't really mind which, but we shouldn't use two ways to > >> >access the same data. > >> > > >> >See the full pull-request at https://github.com/SSSD/sssd/pull/5 > >> > >> Would it be possible to sent patches in mail? > >> samba does it. It would be a hint for me wheter it worth > >> to review a patch in web interface. > > > >It's possible, but not implemented. I would like to send a separate mail > >about the proposed workflow anyway. > Then please attach mails to the thread :-) +1 Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] sssd-secrets systemd service and small fixes
On Mon, 2016-08-15 at 16:05 +0200, Jakub Hrozek wrote: > On Mon, Aug 15, 2016 at 04:01:13PM +0200, Jakub Hrozek wrote: > > Hi, > > > > attached are three small but important patches related to sssd-secrets. > > The context is that I started to write tests and manpage for > > sssd-secrets and noticed some issues. I hope the patches themselves > > offer a nice commit message. > > > > To test the socket activation, you can just install the RPMs built with > > these patches and call the socket responder, for example: > > curl -H "Content-Type: application/json" \ > > --unix-socket /var/run/secrets.socket \ > > -XGET http://localhost/secrets/ > > > > (You can see more examples in the WIP manpage at: > > https://github.com/jhrozek/sssd/commit/508c9eec040cada06bc6a61fe200f2db20a0735b) > > > > If these patches are accepted, I'll also amend the manpage to list the > > service and the socket. > > > > btw it occured to me we might want to support service self-termination, > > but I would prefer to do that in another patchset and tracked with a > > ticket. > > Let's try it with the patches actually.. LGTM all three. Simo. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH SET} A new Secrets service
On Thu, 2016-06-23 at 21:36 +0200, Jakub Hrozek wrote: > On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote: > > Given that Lukas says "http_parser can provide pkgconfig in future", I > > read his mail as a preference to keep the pkg-check test. And actually I > > agree, it doesn't hurt, let's keep it in. > > I wanted to push these patches: > https://github.com/jhrozek/sssd/tree/secrets-review > but..I can't find http_parser_strict on Debian, only http-parser and the > patches seem to require the _strict version. I really don't know the > difference between the two, can we fallback to the non-strict? If it not too hard to detect if strict is present I would try to use it and fallback to not strict only of not available. Strict *seems* a safer option. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH 2/2] UTIL: Revent connection handling in sssd_async_connect_send
On Mon, 2016-06-20 at 11:04 +0200, Lukas Slebodnik wrote: > On (19/06/16 15:27), Simo Sorce wrote: > >As the commit message says, nothing more. > >Isn't it right to wait for 6 seconds as the timeout says ? > >Can you add debug to see what errno is returned (if any) ? > >Or does the code never trigger and only the timeout kick in ? > > > Yes, only the timeout kick in > It might be caused by using DROP instead of REJECT in firewall. > But users use DROP very often as well. But I'm not sure. If you had drop then I would expect you always to have to wait 6 seconds, the only case I can see happening here is that on a RST the epoll actually returns a readable event instead of a writable one. > >We can revert that change in tevent flags if they cause a regression, > >but I want a comment in the code that the connect() man page is > >misleading if that's the case. > > > I added comment; similar as in commit message. Both commits look good to me. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH 2/2] UTIL: Revent connection handling in sssd_async_connect_send
:28 2016) [sssd[be[LDAP]]] [sssd_async_socket_init_send] > (0x0400): Setting 6 seconds timeout for connecting > (Sat Jun 18 12:04:34 2016) [sssd[be[LDAP]]] [sssd_async_connect_timeout] > (0x0100): The connection timed out > (Sat Jun 18 12:04:34 2016) [sssd[be[LDAP]]] [sssd_async_socket_init_done] > (0x0020): sdap_async_sys_connect request failed: [110]: Connection timed out. > (Sat Jun 18 12:04:34 2016) [sssd[be[LDAP]]] [sss_ldap_init_sys_connect_done] > (0x0020): sssd_async_socket_init request failed: [110]: Connection timed out. > (Sat Jun 18 12:04:34 2016) [sssd[be[LDAP]]] [sdap_sys_connect_done] (0x0020): > sdap_async_connect_call request failed: [110]: Connection timed out. > (Sat Jun 18 12:04:34 2016) [sssd[be[LDAP]]] [sdap_handle_release] (0x2000): > Trace: sh[0xfe49dc0], connected[0], ops[(nil)], ldap[(nil)], > destructor_lock[0], release_memory[0] > (Sat Jun 18 12:04:34 2016) [sssd[be[LDAP]]] [get_port_status] (0x1000): Port > status of port 636 for server 'ibm-x3650m4-01-vm-07.example.com' is 'not > working' > (Sat Jun 18 12:04:34 2016) [sssd[be[LDAP]]] [fo_resolve_service_send] > (0x0020): No available servers for service 'LDAP' > (Sat Jun 18 12:04:34 2016) [sssd[be[LDAP]]] [be_resolve_server_done] > (0x1000): Server resolution failed: [5]: Input/output error > (Sat Jun 18 12:04:34 2016) [sssd[be[LDAP]]] [sdap_id_op_connect_done] > (0x0020): Failed to connect, going offline (5 [Input/output error]) > > If you did not have a special reason for this change then > I would appreciate if we could change it back. > > Two patches attached. > > LS -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH SET} A new Secrets service
On Wed, 2016-04-20 at 09:59 +0200, Jakub Hrozek wrote: > On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote: > > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote: > > > Thanks, IIRC the int-instead of enum use is intentional, I will look > > > at the others. > > > > The last coverity/clang thing is a false positive, but I initialized > > reply to NULL anyway, I expect now it will start complaining of possible > > NULL dereference :-) > > > > Attached find patches that fixes all other issues (hopefully), one of > > them simply dropped an entire function as it turned out I wasn't using > > it. > > > > Simo. > > > > -- > > Simo Sorce * Red Hat, Inc * New York > > Ugh, it took me too long to get to this review properly. Sorry about > this.. > > Since this patchset is large, I will send replies in batches. > > > From e215e5534bb56f3887521443ce6c77d13ea3518d Mon Sep 17 00:00:00 2001 > > From: Simo Sorce <s...@redhat.com> > > Date: Tue, 12 Jan 2016 20:07:59 -0500 > > Subject: [PATCH 01/15] Util: Add watchdog helper > > ACK > > I know Pavel Brezina also reviewed this patch earlier in a separate thread, > so note to self: Add his RB :-) > > > From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001 > > From: Simo Sorce <s...@redhat.com> > > Date: Tue, 12 Jan 2016 20:13:28 -0500 > > Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons > > > > This allows the services to self monitor. > > > > Related: > > https://fedorahosted.org/sssd/ticket/2921 > > Is it intentional that we also enable the watchdog in monitor? I wasn?t intentional in the sense that i really wanted at all cost to have this behavior, but I see nothing wrong on sssd watchdogging itself. > I haven't > seen the sssd process being stuck and if it does, we probably have > bigger issues, so it's probably fine, I just need to remember to not > SIGSTOP sssd when testing anymore :) New debugging trick, as soon as you attach or execute a process run: p teardown_watchdog() I have a gdb cmd file where I execute this as a matter of fact when attached to a sssd process. > Otherwise ack. Thanks. > > From c576e37c188ded932996c9714b18a71251ef1d63 Mon Sep 17 00:00:00 2001 > > From: Simo Sorce <s...@redhat.com> > > Date: Wed, 13 Jan 2016 11:51:09 -0500 > > Subject: [PATCH 03/15] Monitor: Remove ping infrastructure > > Can we also remove the force_timeout option and the functions that use > it? Looking at monitor.c, they are only called from a single failure > situation which is ENOMEM, so I think it's actually a dead code, the > probability we ever call it is very low. > > If you agree with removing the force_timeout but you're busy, let me know > and I can prepare a patch atop yours. Please add patch on top. > The only code that proved to be useful in the field of all this we are > about to remove is the diag_cmd(). We could use it to run pstack on the > 'stuck' child to learn where it was actually stuck. But I'm not sure > it's possible to implement it since the watchdog handler is a "plain" > signal handler and the diag_cmd() forks and execs... signal(7) sasys fork()/execve() are async-signal-safe functions, so it could be run from the handler directly, the other option is to kill(SIGXXX) the monitor, and then wait a few seconds before aborting, and hope the monitor has time to run pstack on the child for us. Either way I'd consider writing a new patch for this later, and would consider internally dumping a stack trace to a debug file instead of executing an external process. > That said, I still think it's worth removing the pings in favor of the > watchdog, the pings proved to be too fragile.. Not only fragile but also caused churn and wakeups in the system, as every watchdog beat forced at least a context switch and 2 processes to run. > Maybe it would be better to file a ticket to check again if systemd > watchdog can be used in the future? We can look at it, I didn't want to get too far down the systemd road with this patchset as the goal was another. > To be continued.. Ty, Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH SET} A new Secrets service
On Wed, 2016-04-20 at 19:58 +0200, Lukas Slebodnik wrote:
> On (20/04/16 17:21), Jakub Hrozek wrote:
> >On Wed, Apr 20, 2016 at 09:59:19AM -0400, Simo Sorce wrote:
> >> On Wed, 2016-04-20 at 14:16 +0200, Jakub Hrozek wrote:
> >> > On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
> >> > > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
> >> > > > Thanks, IIRC the int-instead of enum use is intentional, I will look
> >> > > > at the others.
> >> > >
> >> > > The last coverity/clang thing is a false positive, but I initialized
> >> > > reply to NULL anyway, I expect now it will start complaining of
> >> > > possible
> >> > > NULL dereference :-)
> >> > >
> >> > > Attached find patches that fixes all other issues (hopefully), one of
> >> > > them simply dropped an entire function as it turned out I wasn't using
> >> > > it.
> >> > >
> >> > > Simo.
> >> > >
> >> > > --
> >> > > Simo Sorce * Red Hat, Inc * New York
> >> >
> >> > > From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001
> >> > > From: Simo Sorce <s...@redhat.com>
> >> > > Date: Thu, 7 Jan 2016 10:17:38 -0500
> >> > > Subject: [PATCH 05/15] Responders: Add support for socket activation
> >> >
> >> > ACK (visual at this point) with a question - do we want to check
> >> > that the fd we received is a UNIX socket using sd_is_socket_unix()?
> >> >
> >> > The sd_listen_fds() manpage recommends that.
> >>
> >> If they recommend it we should, yes.
> >
> >OK, same as with the responder issue, I will prepare a fixup patch and
> >ask you to check it before squashing into your patches..
> >
> >>
> >> > > From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001
> >> > > From: Simo Sorce <s...@redhat.com>
> >> > > Date: Wed, 20 Jan 2016 10:33:39 -0500
> >> > > Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
> >> > >
> >> > > The secrets database will have "subsections", ie sections that are in
> >> > > the
> >> > > "secrets" namespace and look like this: [secrets/]
> >> > >
> >> > > This function allows to source any section under secrets/ or under any
> >> > > arbitrary sub-path.
> >> > >
> >> > > Related:
> >> > > https://fedorahosted.org/sssd/ticket/2913
> >
> >[...]
> >
> >> > > +int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
> >> > > +struct confdb_ctx *cdb,
> >> > > +const char *section,
> >> > > +char ***sections,
> >> > > +int *num_sections)
> >> > > +{
> >> > > +TALLOC_CTX *tmp_ctx = NULL;
> >> > > +char *secdn;
> >> > > +struct ldb_dn *base = NULL;
> >> > > +struct ldb_result *res = NULL;
> >> > > +static const char *attrs[] = {"cn", NULL};
> >> > > +char **names;
> >> > > +int base_comp_num;
> >> > > +int num;
> >> > > +int i;
> >> >
> >> > Can you use size_t here so that clang doesn't complain about "comparison
> >> > of integers of different signs: 'int' and 'unsigned int'" in the for
> >> > loop below?
> >>
> >> meh, ok :-)
> >
> >Trivial, I can also fix this locally and ask you if it's OK to squash.
> >
> >>
> >> > > +int ret;
> >> > > +
> >> > > +tmp_ctx = talloc_new(mem_ctx);
> >> > > +if (tmp_ctx == NULL) {
> >> > > +return ENOMEM;
> >> > > +}
> >> > > +
> >> > > +ret = parse_section(tmp_ctx, section, , NULL);
> >> > > +if (ret != EOK) {
> >> > > +goto done;
> >> > > +}
> >> > > +
> >> > > +base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
> >> > > +if (base == NULL) {
> >> > > +ret = ENOMEM;
> >> > > +goto done;
> >> > > +}
[SSSD] Re: [PATCH SET} A new Secrets service
On Wed, 2016-04-20 at 17:18 +0200, Jakub Hrozek wrote: > On Wed, Apr 20, 2016 at 09:43:05AM -0400, Simo Sorce wrote: > > On Wed, 2016-04-20 at 11:12 +0200, Jakub Hrozek wrote: > > > On Wed, Apr 20, 2016 at 10:32:59AM +0200, Jakub Hrozek wrote: > > > > > > From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 > > > > > > 2001 > > > > > > From: Simo Sorce <s...@redhat.com> > > > > > > Date: Tue, 12 Jan 2016 20:13:28 -0500 > > > > > > Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons > > > > > > > > > > > > This allows the services to self monitor. > > > > > > > > > > > > Related: > > > > > > https://fedorahosted.org/sssd/ticket/2921 > > > > > > > > > > Is it intentional that we also enable the watchdog in monitor? I > > > > > haven't > > > > > seen the sssd process being stuck and if it does, we probably have > > > > > bigger issues, so it's probably fine, I just need to remember to not > > > > > SIGSTOP sssd when testing anymore :) > > > > > > > > > > Otherwise ack. > > > > > > > > Actually, more questions... > > > > > > > > Can you help me test this patch? I tried to inject sleep() into sssd_be > > > > code and the sleep was just interrupted by the SIGRT delivery. With > > > > SSSD, > > > > most of the time the process was stuck was because it was writing a lot > > > > of > > > > data with fsync()/fdatasync(). I can't find any information in the Linux > > > > fsync manpage on how fsync behaves wrt signals. openpub manpages > > > > indicate > > > > that fsync would return EINTR, which worries me a bit.. > > > > > > Hmm, sorry, I was not being careful enough. man 7 signal also says: > > > """ > > > The sleep(3) function is also never restarted if interrupted by a > > > handler, but gives a success return: the number of seconds remaining to > > > sleep. > > > """ > > > > > > so the sleep testcase was wrong even though CatchSignal uses SA_RESTART. > > > But do you know how would write() or fsync() behave here? The signal > > > manpage is a bit unclar to me as it talks about "slow" devices.. > > > > > > Or can you think of some easy way to test this? > > > > The fsync manpage here says: > > "The call blocks until the device reports that the transfer has > > completed." > > > > And does not report EINTR as a possible error. > > > > That said I am a bit unclear what you want to test actually ? > > I want to actually test that the service can be restarted if stuck and > reconnects fine. So far I haven't been lucky - SIGSTOP-ing the service > stopped delivery of the signals, so did attaching gdb and waiting. gdb it, do *not* cont, but tell gdb to not block signals, iirc you do that with: handle SIGRTMIN nostop noprint pass if you then wait in gdb the timers will fire and eventually the process will be killed because you are blocking the main loop and not resetting the watchdog. > But most importantly I want to make sure that if tdb is writing a > transaction and the signal is delivered, then we don't fsync() in tdb > doesn't get interrupted and doesn't corrupt the database. From all the > cases where users complained about a service being restarted, it was > always about tdb writing stuff to disk... I'm inquiring about this. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH SET} A new Secrets service
On Wed, 2016-04-20 at 14:16 +0200, Jakub Hrozek wrote:
> On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
> > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
> > > Thanks, IIRC the int-instead of enum use is intentional, I will look
> > > at the others.
> >
> > The last coverity/clang thing is a false positive, but I initialized
> > reply to NULL anyway, I expect now it will start complaining of possible
> > NULL dereference :-)
> >
> > Attached find patches that fixes all other issues (hopefully), one of
> > them simply dropped an entire function as it turned out I wasn't using
> > it.
> >
> > Simo.
> >
> > --
> > Simo Sorce * Red Hat, Inc * New York
>
> > From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001
> > From: Simo Sorce <s...@redhat.com>
> > Date: Thu, 7 Jan 2016 10:17:38 -0500
> > Subject: [PATCH 05/15] Responders: Add support for socket activation
>
> ACK (visual at this point) with a question - do we want to check
> that the fd we received is a UNIX socket using sd_is_socket_unix()?
>
> The sd_listen_fds() manpage recommends that.
If they recommend it we should, yes.
> > From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001
> > From: Simo Sorce <s...@redhat.com>
> > Date: Wed, 20 Jan 2016 10:33:39 -0500
> > Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
> >
> > The secrets database will have "subsections", ie sections that are in the
> > "secrets" namespace and look like this: [secrets/]
> >
> > This function allows to source any section under secrets/ or under any
> > arbitrary sub-path.
> >
> > Related:
> > https://fedorahosted.org/sssd/ticket/2913
> > ---
> > src/confdb/confdb.c | 92
> > +
> > src/confdb/confdb.h | 26 +++
> > 2 files changed, 118 insertions(+)
> >
> > diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
> > index
> > d409344890c869aa3e7b2dbb49c0f51cd3a20adc..03adbe593b42f556b560df77d410f80460200a67
> > 100644
> > --- a/src/confdb/confdb.c
> > +++ b/src/confdb/confdb.c
> > @@ -1531,3 +1531,95 @@ done:
> > talloc_free(tmp_ctx);
> > return ret;
> > }
> > +
> > +int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
> > +struct confdb_ctx *cdb,
> > +const char *section,
> > +char ***sections,
> > +int *num_sections)
> > +{
> > +TALLOC_CTX *tmp_ctx = NULL;
> > +char *secdn;
> > +struct ldb_dn *base = NULL;
> > +struct ldb_result *res = NULL;
> > +static const char *attrs[] = {"cn", NULL};
> > +char **names;
> > +int base_comp_num;
> > +int num;
> > +int i;
>
> Can you use size_t here so that clang doesn't complain about "comparison
> of integers of different signs: 'int' and 'unsigned int'" in the for
> loop below?
meh, ok :-)
> > +int ret;
> > +
> > +tmp_ctx = talloc_new(mem_ctx);
> > +if (tmp_ctx == NULL) {
> > +return ENOMEM;
> > +}
> > +
> > +ret = parse_section(tmp_ctx, section, , NULL);
> > +if (ret != EOK) {
> > +goto done;
> > +}
> > +
> > +base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
> > +if (base == NULL) {
> > +ret = ENOMEM;
> > +goto done;
> > +}
> > +
> > +base_comp_num = ldb_dn_get_comp_num(base);
> > +
> > +ret = ldb_search(cdb->ldb, tmp_ctx, , base, LDB_SCOPE_SUBTREE,
> > + attrs, NULL);
> > +if (ret != LDB_SUCCESS) {
> > +ret = EIO;
> > +goto done;
> > +}
> > +
> > +names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
> > +if (names == NULL) {
> > +ret = ENOMEM;
> > +goto done;
> > +}
> > +
> > +for (num = 0, i = 0; i < res->count; i++) {
> > +const struct ldb_val *val;
> > +char *name;
> > +int n;
> > +int j;
>
> Every time I see variables declared in a scope in C except loop control
> variables I think "This should be a static function of its own" :-)
Should it be in this case ?
> > +
> > +n = ldb_dn_get_comp_num(res->msgs[i]->dn);
> > +if (n == base_comp_num) continue;
&g
[SSSD] Re: [PATCH SET} A new Secrets service
On Wed, 2016-04-20 at 11:55 +0200, Jakub Hrozek wrote: > On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote: > > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote: > > > Thanks, IIRC the int-instead of enum use is intentional, I will look > > > at the others. > > > > The last coverity/clang thing is a false positive, but I initialized > > reply to NULL anyway, I expect now it will start complaining of possible > > NULL dereference :-) > > > > Attached find patches that fixes all other issues (hopefully), one of > > them simply dropped an entire function as it turned out I wasn't using > > it. > > > > Simo. > > > > -- > > Simo Sorce * Red Hat, Inc * New York > > > From 02e259e44631d228e0ec8311392e4a1a1a661b89 Mon Sep 17 00:00:00 2001 > > From: Simo Sorce <s...@redhat.com> > > Date: Fri, 8 Jan 2016 17:51:06 -0500 > > Subject: [PATCH 04/15] Responders: Make the client context more generic > > This patch breaks the PAM responder: > Program received signal SIGSEGV, Segmentation fault. > 0x in ?? () > (gdb) bt > #0 0x in ?? () > #1 0x00413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, > flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478 > #2 0x7f8fe25d5bf3 in epoll_event_loop (tvalp=0x7ffd4e5d41b0, > epoll_ev=0x653c80) at ../tevent_epoll.c:728 > #3 epoll_event_loop_once (ev=, location=) at > ../tevent_epoll.c:926 > #4 0x7f8fe25d4137 in std_event_loop_once (ev=0x653a40, > location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at > ../tevent_standard.c:114 > #5 0x7f8fe25d038d in _tevent_loop_once (ev=ev@entry=0x653a40, > location=location@entry=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at > ../tevent.c:533 > #6 0x7f8fe25d052b in tevent_common_loop_wait (ev=0x653a40, > location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent.c:637 > #7 0x7f8fe25d40d7 in std_event_loop_wait (ev=0x653a40, > location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at > ../tevent_standard.c:140 > #8 0x7f8fe5f67b55 in server_loop (main_ctx=0x654e90) at > /sssd/src/util/server.c:689 > #9 0x00406f57 in main (argc=6, argv=0x7ffd4e5d4598) at > /sssd/src/responder/pam/pamsrv.c:426 > (gdb) frame 1 > #1 0x00413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, > flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478 > 478 ret = accept_ctx->connection_setup(cctx); > (gdb) p accept_ctx > $1 = (struct accept_fd_ctx *) 0x6604b0 > (gdb) p accept_ctx->connection_setup > $2 = (connection_setup_t) 0x0 > > Do you want me to fix this and proceed or will you? If you already know what's wrong, feel free to fix it, if you have to spend time analyzing I can do it, should be an easy fix. > The NSS, autofs and IFP responders seem to work fine. I haven't tested > the others yet. Ok, sorry for that, I did install SSSD at various times, but was more concentrated on testing the secrets stuff, and the tests went smoothly, but I now realize they fake the connection handling. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH SET} A new Secrets service
On Wed, 2016-04-20 at 11:12 +0200, Jakub Hrozek wrote: > On Wed, Apr 20, 2016 at 10:32:59AM +0200, Jakub Hrozek wrote: > > > > From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001 > > > > From: Simo Sorce <s...@redhat.com> > > > > Date: Tue, 12 Jan 2016 20:13:28 -0500 > > > > Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons > > > > > > > > This allows the services to self monitor. > > > > > > > > Related: > > > > https://fedorahosted.org/sssd/ticket/2921 > > > > > > Is it intentional that we also enable the watchdog in monitor? I haven't > > > seen the sssd process being stuck and if it does, we probably have > > > bigger issues, so it's probably fine, I just need to remember to not > > > SIGSTOP sssd when testing anymore :) > > > > > > Otherwise ack. > > > > Actually, more questions... > > > > Can you help me test this patch? I tried to inject sleep() into sssd_be > > code and the sleep was just interrupted by the SIGRT delivery. With SSSD, > > most of the time the process was stuck was because it was writing a lot of > > data with fsync()/fdatasync(). I can't find any information in the Linux > > fsync manpage on how fsync behaves wrt signals. openpub manpages indicate > > that fsync would return EINTR, which worries me a bit.. > > Hmm, sorry, I was not being careful enough. man 7 signal also says: > """ > The sleep(3) function is also never restarted if interrupted by a > handler, but gives a success return: the number of seconds remaining to > sleep. > """ > > so the sleep testcase was wrong even though CatchSignal uses SA_RESTART. > But do you know how would write() or fsync() behave here? The signal > manpage is a bit unclar to me as it talks about "slow" devices.. > > Or can you think of some easy way to test this? The fsync manpage here says: "The call blocks until the device reports that the transfer has completed." And does not report EINTR as a possible error. That said I am a bit unclear what you want to test actually ? Yes interruptible calls can be interrupted by a signal, that's always the case, if we have code that misbehave when a syscall is interrupted we need to fix that code. Afaik when we write() we always check the return and retry on EINTR. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH SET} A new Secrets service
On Tue, 2016-04-05 at 14:54 -0400, Simo Sorce wrote: > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote: > > Thanks, IIRC the int-instead of enum use is intentional, I will look > > at the others. > > The last coverity/clang thing is a false positive, but I initialized > reply to NULL anyway, I expect now it will start complaining of possible > NULL dereference :-) > > Attached find patches that fixes all other issues (hopefully), one of > them simply dropped an entire function as it turned out I wasn't using > it. BUMP ? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH SET} A new Secrets service
On Fri, 2016-04-01 at 13:05 +0200, Lukas Slebodnik wrote:
> On (30/03/16 12:31), Simo Sorce wrote:
> >This patchset implements a new responder like service in SSSD called
> >secrets. It uses the Custodia project API to offer a service where
> >applications/users can store secrets in a way that makes requests
> >remotizable and routable with a high degree of configurability (esp, in
> >conjunction with a Custodia proxy).
> >
> >Included are also accessory patches to change the monitor and other
> >aspects of service startup and monitoring necessary to have this new
> >kind of service which is more independent than the pam/nss based
> >services.
> >
> >There is no testsuite for the service yet.
> >
> >The work is also not complete in that the monitor does not start the
> >service yet, I have an experimental unit file I am working on but it is
> >not fully functional and not included yet..
> >
> >I do not expect all patches to be accepted right away, but they all work
> >individually (manually tested), but I think it is a good time to start
> >review and bring in what works, as we are going to spread some of the
> >remaining work across multiple people.
> >
> >HTH,
> >Simo.
> >
>
> I do not plan to do a full review. Jakub voluntered IIRC :-)
> But here are few comments.
>
> >From cd731590f1830ab9686949af1fa66d2b7463eafc Mon Sep 17 00:00:00 2001
> >From: Simo Sorce <s...@redhat.com>
> >Date: Tue, 12 Jan 2016 20:07:59 -0500
> >Subject: [PATCH 01/15] Util: Add watchdog helper
> >
> >The watchdog uses a kernel timer to issue a signal to the process.
> >It checks if the ticker is not being reset by the main event loop, which
> >would indicate that the process got stuck.
> >At the same time it sets a tevent timer to clear the watchdog ticker, so
> >that the watchdog handler is kept happy.
> >
> >If the watchdog detects that the timer event failed to reset the watchdog for
> >three times in a row then the process is killed.
> >Normally the monitor will detect the child terminated and will rescheduled
> >it.
> >
> >Related:
> >https://fedorahosted.org/sssd/ticket/2921
> >---
> >diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c
> >new file mode 100644
> >index
> >..64faaaf03213319d3127fde3946124ee26c7794a
> >--- /dev/null
> >+++ b/src/util/util_watchdog.c
> //snip
>
> >+int setup_watchdog(struct tevent_context *ev, int interval)
> >+{
> >+struct sigevent sev;
> >+struct itimerspec its;
> >+int signum = SIGRTMIN;
> >+int ret;
> >+
> >+CatchSignal(signum, watchdog_handler);
> >+
> >+sev.sigev_notify = SIGEV_SIGNAL;
> >+sev.sigev_signo = signum;
> >+sev.sigev_value.sival_ptr = _ctx.timerid;
> >+errno = 0;
> >+ret = timer_create(CLOCK_MONOTONIC, , _ctx.timerid);
> I got a valgrind error here on rhel6
> ==2376== Syscall param timer_create(evp) points to uninitialised byte(s)
> ==2376==at 0x5B0B08D: timer_create@@GLIBC_2.3.3 (in
> /lib64/librt-2.12.so)
> ==2376==by 0x54942CD: setup_watchdog (util_watchdog.c:88)
> ==2376==by 0x40414F: server_setup (server.c:651)
> ==2376==by 0x403059: test_run_as_root_fg (test_server.c:104)
> ==2376==by 0x5243A94: ??? (in /usr/lib64/libcmocka.so.0.3.1)
> ==2376==by 0x5243DA9: _cmocka_run_group_tests (in
> /usr/lib64/libcmocka.so.0.3.1)
> ==2376==by 0x402CA1: main (test_server.c:204)
> ==2376== Address 0x7fefff920 is on thread 1's stack
>
> Following diff fixed it.
Thanks for finding this!
> diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c
> index 64faaaf..d6f71ad 100644
> --- a/src/util/util_watchdog.c
> +++ b/src/util/util_watchdog.c
> @@ -74,7 +74,7 @@ static void watchdog_event_handler(struct tevent_context
> *ev,
>
> int setup_watchdog(struct tevent_context *ev, int interval)
> {
> -struct sigevent sev;
> +struct sigevent sev = { 0, };
> struct itimerspec its;
> int signum = SIGRTMIN;
> int ret;
> >diff --git a/configure.ac b/configure.ac
> >index
> >783372dd6b5d86793c218ac513a93b65e97d4c06..f7430ca71f07b1085f49a7635070f71894f1a4a9
> > 100644
> >--- a/configure.ac
> >+++ b/configure.ac
> >@@ -185,6 +185,8 @@ m4_include([src/external/libnfsidmap.m4])
> > m4_include([src/external/cwrap.m4])
> > m4_include([src/external/libresolv.m4])
> > m4_include([src/external/intgcheck.m4])
> >+m4_include([src/external/l
[SSSD] Multiple PID file macros ?
While looking at the monitor code I realize we define SSSD_PIDFILE_PATH in monitor.c in a different way than we define SSSD_PIDFILE in tools_util.h Although the definitions differ, they end up being effectively the same string for now. It seem to me those definition should be merged and harmonized into one. If not a comment should be put in the code explaining why we have 2 (potentially) different pid file names. Hints, on which way is right ? Should we open a ticket on this ? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Config file merging in SSSD
On Tue, 2016-03-22 at 16:19 +0100, Michal Židek wrote: > On 03/22/2016 03:29 PM, Sumit Bose wrote: > > On Tue, Mar 22, 2016 at 12:29:39PM +0100, Michal Židek wrote: > >> Hi, > >> > >> I would like to write a patch that will > >> allow SSSD to use the config file merging > >> feature from libini. But first I would like > >> to ask developers for their opinions on how > >> this should be implemented. > >> > >> My idea was that it could work like this. > >> > >> The current /etc/sssd/sssd.conf would work > >> as usual. > >> > >> We would add new directory /etc/sssd/conf.d/ > >> and its content would be following: > >> - README file that informs what the direcotory > >> is for. > >> - any number of files ending with .conf extension > >>that would contain additional configuration for > >>SSSD. These files would have higher prioriry > >>than the /etc/sssd/sssd.conf , meaning if > >>the same option is present in these files > >>it will override the /etc/sssd/sssd.conf > >>value. > >> > >> SSSD would automatically pick up files ending > >> in .conf from that direcory and use them. In > >> order to disable the config file, the admin will > >> have to rename the file ending (for example > >> .conf.disabled). This way, we do not need to > >> inspect the snippets for any special options > >> like 'enable_this_snippet = true' which would > >> just complicate the processing. > >> > >> In order for SSSD to load a configuration, all > >> the config snippets in /etc/sssd/conf.d/ and > >> /etc/sssd/sssd.conf must in combination > >> result in valid configuration. If there is an > >> error in processing one of the config files, > >> the whole configuration loading will be > >> unsuccessful and there will be no way to > >> skip problematic snippets (later we may add > >> a fallback config, but that is different issue). > >> > >> Of course sssd will have an cli option to > >> use alternative directory for snippets (similar > >> to what we use for config file now). > >> > >> Could it be implemented this way? > >> Any comments are welcome. > > > > How will this interact with the python configuration API? If some > > application will use the API to change the configuration the result will > > be written to sssd.conf. But if there is a config snippets which sets > > the same value the change via the config API is overridden which I guess > > is unexpected by the application using the config API. > > > > bye, > > Sumit > > > > Good question. I was not thinking about this. We > could change the config API to actually write to its > own snippet that will be always applied last. > > OTOH some admins may want to really override whatever > other applications may set up using python config API. > > If we decide to apply snippets in alphabetical > order as Lukas suggested, we can give the snippet to which > python API writes a name for example 990_pyapi.conf > and if the admin decides to create snippet with starting > number lower than 990 it will have lower pririty than > python config API. > > We should document such behavior in the README file > that will be placed in the /etc/sssd/conf.d directory. If you make sssd.conf always win over snippets then you do not need to change anything, though. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Config file merging in SSSD
On Tue, 2016-03-22 at 15:15 +0100, Michal Židek wrote: > On 03/22/2016 02:46 PM, Lukas Slebodnik wrote: > > On (22/03/16 14:30), Michal Židek wrote: > >> On 03/22/2016 12:29 PM, Michal Židek wrote: > >>> Hi, > >>> > >>> I would like to write a patch that will > >>> allow SSSD to use the config file merging > >>> feature from libini. But first I would like > >>> to ask developers for their opinions on how > >>> this should be implemented. > >>> > >>> My idea was that it could work like this. > >>> > >>> The current /etc/sssd/sssd.conf would work > >>> as usual. > >>> > >>> We would add new directory /etc/sssd/conf.d/ > >>> and its content would be following: > >>> - README file that informs what the direcotory > >>> is for. > >>> - any number of files ending with .conf extension > >>>that would contain additional configuration for > >>>SSSD. These files would have higher prioriry > >>>than the /etc/sssd/sssd.conf , meaning if > >>>the same option is present in these files > >>>it will override the /etc/sssd/sssd.conf > >>>value. What's the rational behind 'snippets vs main conf' makes snippet a winner ? > >>> SSSD would automatically pick up files ending > >>> in .conf from that direcory and use them. In > >>> order to disable the config file, the admin will > >>> have to rename the file ending (for example > >>> .conf.disabled). This way, we do not need to > >>> inspect the snippets for any special options > >>> like 'enable_this_snippet = true' which would > >>> just complicate the processing. > >>> > > Another, way how to ignore snippet is to ignore > > any file which start with dot ".". > > "hiddent files". It would avoid adding suffix to every file. > > > > BTW logrotate and crond do the same > > /etc/logrotate.d/ > > /etc/cron.d/ logrotate also adds .old in var/log, in general you should not have .conf files in conf.d if you do not want them used. Ignoring .something files should always be true as it makes it hard to understand what is going on if you just do ls and can't see a snippet. > >>> In order for SSSD to load a configuration, all > >>> the config snippets in /etc/sssd/conf.d/ and > >>> /etc/sssd/sssd.conf must in combination > >>> result in valid configuration. If there is an > >>> error in processing one of the config files, > >>> the whole configuration loading will be > >>> unsuccessful and there will be no way to > >>> skip problematic snippets (later we may add > >>> a fallback config, but that is different issue). > >>> > > I do not remember the lib_iniconfig API. > > But it might be already solved there. It is, question though: do you want to allow to override anything in a snippet ? At some point I had the idea the snippet would only allow to define/override a single domain and the snippet name would have to be domain_NAME.conf, but that may be overly restrictive given today we are adding stuff like secrets which has a different way to deal with section names. > >>> Of course sssd will have an cli option to > >>> use alternative directory for snippets (similar > >>> to what we use for config file now). > >>> > >>> Could it be implemented this way? > >>> Any comments are welcome. > >>> > >>> Michal > >> > >> I forgot to notice one more thing. > >> > >> There can be conflicts in configuration > >> among the snippets in /etc/sssd/conf.d directory. > >> IMO the best thing to do in this situation is > >> to end with an error. Defining priorities among > >> the snippets could cause situations where people > >> will write difficult to debug configurations. > >> > > In most, project you use numbers in the begining of files. > > It guarantee the order and last value win. It's current behaviour > > in sssd.conf. > > So should we rely on alphabetical order? I personally > think it will add a little chaos to the configuration > but maybe not. > > If we decide to rely on alphabetical order it may > be nice to have a tool that will print the actual > configuration in stdout skipping all the overriden options. > This may be good for troubleshooting SSSD. We need an order so that admins can understand why a conflict happened, alphabetical is as good as any. > > > > We can also provide something like "systemctl cat name.service" > > or we can even save parsed/merged.. configuration with > > ini_config_save_as. > > > > What do you think? > > It sounds good to me. The condition for a conf file could be: > (ends with '.conf') and (does not start with '.') > everything else will be ignored. > > > > > BTW you might also look into gssproxy how they implemented it. > > They already uses this feature from lib_iniconfig IIRC. We do, indeed. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: NSS responder should negatively cache local users for a longer time
On Sun, 2016-03-20 at 21:28 +0100, Jakub Hrozek wrote: > > On 16 Mar 2016, at 13:45, Petr Cech <pc...@redhat.com> wrote: > > > > Hi, > > > > I will work on $subject [1] and I have discussed this topic with > Jakub a week ago. There are some open questions, so I will be glad if > you say your opinion. > > > > There could be heavy traffic between SSSD client and server coused > by local users. We need longer timeout in negative cache for local > users. > > > > Questions are: > > > > a) Is better hack negative_cache or responder? > > > > I would say that this solution should be reusable by other responders > like ifp as well. Therefore I would say either negcache (but there I > would say a new function, not extend the generic one) or a reusable > function in responder/common. > > > b) Is better set timeout = 0 (it means permanently in negative > cache) or set something really big like 12 hours? > > * We couldn't remove local users from permanent negative cache (only > by restart). > > * Is timeout = 12 hours means some kind of network peak? > > > > I guess some long timeout is slightly more flexible for cases where > the admin would add the local user to LDAP groups. A couple of hours > should be enough, as long as the negative entries are cached across > all clients, then if a single client queries the server once a couple > of hours, that should not bring the server down.. 12 hours is a lot, if you made a mistake and want to correct it (eg some software install created a local user by mistake that the admin removes because they want it in LDAP) you do not want to wait for hours. The main issue with the initgroups calls is not the load o the server, but the slowdown of the call which ends up contacting a network (slower) store for a local user. I would use a smaller timeout here, like 10 minutes, but potentially add a midway cache check, like we do for positive results. so after 5 min, if a request comes in we do an asynchronous positive check to see if we still need to extend the negative cache for another 10 minutes. If the local user disappeared we drop the negative cache. > btw do you think this feature should be enabled or disabled by > default? Good q. how often does this problem happen ? > > c) Is it enough to do it only for initgroups? > > Hmm, not sure, by convention initgroups is the most frequent example > (maybe there will be some users of the new libc merge feature), but at > the same time special-casing initgroups doesn't gain much.. > > I guess I would personally do this for all lookups that the NSS > interface can do (by name, by id) but I'm not 100% for or against > either.. I agree, keep it generic. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] Fix responders socket cleanup code
On Thu, 2016-03-10 at 21:04 -0500, Simo Sorce wrote:
> The attached patch fixes #2973,
> it's pretty straightforward.
Same patch but fixed the typos in the commit message.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 93a636ca1283ca9b2bfbda55684eec43afff5c06 Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Thu, 10 Mar 2016 20:52:43 -0500
Subject: [PATCH] Responders: Fix client destructor
To close a socket associated to an fd event we must set the close
function of the event and not associate a destructor to a parent context.
Otherwise the destructor will close() the socket before the fd event is
freed, and this may cause invalid calls on a closed file descriptor to
poll/epoll/etc.
Discovered by looking at strace output.
Resolves:
https://fedorahosted.org/sssd/ticket/2973
---
src/responder/common/responder_common.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 6ac1ea984442792a21b53869a2911b431255..982318647ee9ee7d6795621c03ad8cf53fb78f43 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -66,9 +66,12 @@ static errno_t set_close_on_exec(int fd)
return EOK;
}
-static int client_destructor(struct cli_ctx *ctx)
+static void client_close_fn(struct tevent_context *ev,
+struct tevent_fd *fde, int fd,
+void *ptr)
{
errno_t ret;
+struct cli_ctx *ctx = talloc_get_type(ptr, struct cli_ctx);
if ((ctx->cfd > 0) && close(ctx->cfd) < 0) {
ret = errno;
@@ -80,7 +83,8 @@ static int client_destructor(struct cli_ctx *ctx)
DEBUG(SSSDBG_TRACE_INTERNAL,
"Terminated client [%p][%d]\n",
ctx, ctx->cfd);
-return 0;
+
+ctx->cfd = -1;
}
static errno_t get_client_cred(struct cli_ctx *cctx)
@@ -474,12 +478,11 @@ static void accept_fd_handler(struct tevent_context *ev,
accept_ctx->is_private ? " on privileged pipe" : "");
return;
}
+tevent_fd_set_close_fn(cctx->cfde, client_close_fn);
cctx->ev = ev;
cctx->rctx = rctx;
-talloc_set_destructor(cctx, client_destructor);
-
/* Set up the idle timer */
ret = reset_idle_timer(cctx);
if (ret != EOK) {
--
2.5.0
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] Fix responders socket cleanup code
The attached patch fixes #2973,
it's pretty straightforward.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 8cfba566ecddfc59e9c07236d28c5cdc62a316cd Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Thu, 10 Mar 2016 20:52:43 -0500
Subject: [PATCH] Responders: Fix client destrcutor
To close a socket associated to an fd event we must set the close
function of the event and not associate a destructor to aparent context.
Otherwise the destructor will close() the socket before the fd event is
freed, and this may cause invalid calls on a closed file descriptor to
poll/epoll/etc.
Discovered by looking at startce output.
Resolves:
https://fedorahosted.org/sssd/ticket/2973
---
src/responder/common/responder_common.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 3eec66aba6411f20997d868fa71037b68a4722df..deeb4e4c88d2a3bf6ff6be42fa496b11e4b20762 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -70,9 +70,12 @@ static errno_t set_close_on_exec(int fd)
return EOK;
}
-static int client_destructor(struct cli_ctx *ctx)
+static void client_close_fn(struct tevent_context *ev,
+struct tevent_fd *fde, int fd,
+void *ptr)
{
errno_t ret;
+struct cli_ctx *ctx = talloc_get_type(ptr, struct cli_ctx);
if ((ctx->cfd > 0) && close(ctx->cfd) < 0) {
ret = errno;
@@ -84,7 +87,8 @@ static int client_destructor(struct cli_ctx *ctx)
DEBUG(SSSDBG_TRACE_INTERNAL,
"Terminated client [%p][%d]\n",
ctx, ctx->cfd);
-return 0;
+
+ctx->cfd = -1;
}
static errno_t get_client_cred(struct cli_ctx *cctx)
@@ -489,12 +493,11 @@ static void accept_fd_handler(struct tevent_context *ev,
accept_ctx->is_private ? " on privileged pipe" : "");
return;
}
+tevent_fd_set_close_fn(cctx->cfde, client_close_fn);
cctx->ev = ev;
cctx->rctx = rctx;
-talloc_set_destructor(cctx, client_destructor);
-
/* Set up the idle timer */
ret = reset_idle_timer(cctx);
if (ret != EOK) {
--
2.5.0
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] make async connect generic
On Tue, 2016-03-08 at 17:48 +0100, Jakub Hrozek wrote: > On Tue, Mar 08, 2016 at 10:18:46AM -0500, Simo Sorce wrote: > > Fixing everything else commented before. > > > > On Sat, 2016-03-05 at 15:31 +0100, Jakub Hrozek wrote: > > > And this is the question. The new code doesn't restore the flags, is > > > this an intentional change? Do you know why we restored the flags > > > previously? > > > > Yes, it is an intentional change as restoring the flags was not needed. > > What happens if the function fails is that we are going to close the > > socket anyway, so what's the point of restoring flags (which means > > removing O_NONBLOCK in the end, somethign we never want to do as all > > sockets must be non-blocking in SSSD to avoig hangs. > > > > > > Fixed patches attacched. > > > > Simo. > > > > -- > > Simo Sorce * Red Hat, Inc * New York > > > From 5551dc918890cf445cadb1b39c42d9a6dffa8bb8 Mon Sep 17 00:00:00 2001 > > From: Simo Sorce <s...@redhat.com> > > Date: Wed, 2 Mar 2016 15:49:27 -0500 > > Subject: [PATCH 2/3] Util: Set socket options and flags separately > > > > Reorganize functions to set options and flags, all flags can be set at once, > > and there is no need to keep old falgs around as nothing ever used that for > > anything useful. > > > > Related: > > https://fedorahosted.org/sssd/ticket/2968 > > This patch breaks failover for me. I can't really figure out why, the > code looks OK, though. > > What I'm seeing is that the connect() call blocks. If I just add another > SETFD with O_NONBLOCK, everything works fine. > > I really don't see the error though, can you? I can reliably reproduce > the error with: > - search for a user to establish connection > - pause the VM with the server > - search again My bad, I see the error, proper patch coming soon. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] make async connect generic
Fixing everything else commented before.
On Sat, 2016-03-05 at 15:31 +0100, Jakub Hrozek wrote:
> And this is the question. The new code doesn't restore the flags, is
> this an intentional change? Do you know why we restored the flags
> previously?
Yes, it is an intentional change as restoring the flags was not needed.
What happens if the function fails is that we are going to close the
socket anyway, so what's the point of restoring flags (which means
removing O_NONBLOCK in the end, somethign we never want to do as all
sockets must be non-blocking in SSSD to avoig hangs.
Fixed patches attacched.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 9b8fd65b6eb242936a5d0734eb05e3c09d3268a5 Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Wed, 2 Mar 2016 14:33:38 -0500
Subject: [PATCH 1/3] Util: Move socket setup in a common utility file
Other components may need to connect sockets, the code here is generic enough
that with minimal modifications can be used for non-ldap connections too.
So create a sss_sockets.c/h utility file with all the non-ldap specific socket
setup functions and make them available for other uses.
Resolves:
https://fedorahosted.org/sssd/ticket/2968
---
Makefile.am| 5 +
src/util/sss_ldap.c| 258 ++-
src/util/sss_sockets.c | 356 +
src/util/sss_sockets.h | 39 ++
4 files changed, 413 insertions(+), 245 deletions(-)
create mode 100644 src/util/sss_sockets.c
create mode 100644 src/util/sss_sockets.h
diff --git a/Makefile.am b/Makefile.am
index 4e4f38a5eaf5bfa2cfafa88b9b32848e6c27131b..d6eb0fc732a3b566dba91952bd6598a31f8d8d3e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -541,6 +541,7 @@ dist_noinst_HEADERS = \
src/util/sss_python.h \
src/util/sss_krb5.h \
src/util/sss_selinux.h \
+src/util/sss_sockets.h \
src/util/sss_utf8.h \
src/util/sss_ssh.h \
src/util/sss_ini.h \
@@ -1663,6 +1664,7 @@ ipa_ldap_opt_tests_SOURCES = \
src/providers/ad/ad_opts.c \
src/providers/ipa/ipa_opts.c \
src/providers/krb5/krb5_opts.c \
+src/util/sss_sockets.c \
src/util/sss_ldap.c \
src/tests/ipa_ldap_opt-tests.c
ipa_ldap_opt_tests_CFLAGS = \
@@ -1877,6 +1879,7 @@ TEST_MOCK_RESP_OBJ = \
src/responder/common/responder_cache_req.c
TEST_MOCK_PROVIDER_OBJ = \
+ src/util/sss_sockets.c \
src/util/sss_ldap.c \
src/providers/data_provider_opts.c \
src/providers/ldap/ldap_opts.c \
@@ -2264,6 +2267,7 @@ sdap_tests_SOURCES = \
src/providers/ldap/sdap_range.c \
src/providers/ldap/ldap_opts.c \
src/providers/ipa/ipa_opts.c \
+src/util/sss_sockets.c \
src/util/sss_ldap.c \
src/tests/cmocka/test_sdap.c \
$(NULL)
@@ -2879,6 +2883,7 @@ libsss_ldap_common_la_SOURCES = \
src/providers/ldap/sdap.c \
src/providers/ipa/ipa_dn.c \
src/util/user_info_msg.c \
+src/util/sss_sockets.c \
src/util/sss_ldap.c \
$(NULL)
libsss_ldap_common_la_CFLAGS = \
diff --git a/src/util/sss_ldap.c b/src/util/sss_ldap.c
index c440d5445133c8b31f662fc69b35e2296c3f1702..7fdaadb5cebf5d3e7fe7f8fb1780f0db3dbcae4a 100644
--- a/src/util/sss_ldap.c
+++ b/src/util/sss_ldap.c
@@ -17,18 +17,13 @@
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#include
-#include
-#include
-#include
-#include
-#include
#include "config.h"
-
-#include "providers/ldap/sdap.h"
-#include "util/sss_ldap.h"
#include "util/util.h"
+#include "util/sss_sockets.h"
+#include "util/sss_ldap.h"
+
+#include "providers/ldap/sdap.h"
const char* sss_ldap_err2string(int err)
{
@@ -103,183 +98,6 @@ int sss_ldap_control_create(const char *oid, int iscritical,
}
#ifdef HAVE_LDAP_INIT_FD
-struct sdap_async_sys_connect_state {
-long old_flags;
-struct tevent_fd *fde;
-int fd;
-socklen_t addr_len;
-struct sockaddr_storage addr;
-};
-
-static void sdap_async_sys_connect_done(struct tevent_context *ev,
-struct tevent_fd *fde, uint16_t flags,
-void *priv);
-
-static struct tevent_req *sdap_async_sys_connect_send(TALLOC_CTX *mem_ctx,
-struct tevent_context *ev,
-int fd,
-const struct sockaddr *addr,
-socklen_t addr_len)
-{
-struct tevent_req *req;
-struct sdap_async_sys_connect_state *state;
-long flags;
-int ret;
-int fret;
-
-flags = fcntl(fd, F_GETFL, 0);
-if (flags == -1) {
-DEBUG(SSSDBG_CRIT_FAILURE, "fcntl F_GETFL failed.\n");
-return NULL;
-}
-
-
[SSSD] Re: [PATCH] KRB5: Discern between expired & disabled AD user
On Mon, 2016-03-07 at 18:40 +0100, Lukas Slebodnik wrote: > On (07/03/16 11:29), Simo Sorce wrote: > >On Mon, 2016-03-07 at 16:58 +0100, Lukas Slebodnik wrote: > >> On (04/03/16 16:42), Simo Sorce wrote: > >> >On Fri, 2016-03-04 at 21:27 +0100, Lukas Slebodnik wrote: > >> >> On (02/03/16 10:02), Simo Sorce wrote: > >> >> >On Wed, 2016-03-02 at 15:34 +0100, Lukas Slebodnik wrote: > >> >> >> On (01/03/16 18:28), Simo Sorce wrote: > >> >> >> >On Tue, 2016-03-01 at 18:22 -0500, Simo Sorce wrote: > >> >> >> >> On Tue, 2016-03-01 at 22:34 +0100, Lukas Slebodnik wrote: > >> >> >> >> > On (01/03/16 12:05), Simo Sorce wrote: > >> >> >> >> > >On Tue, 2016-03-01 at 17:51 +0100, Lukas Slebodnik wrote: > >> >> >> >> > >> On (01/03/16 17:45), Lukas Slebodnik wrote: > >> >> >> >> > >> >On (31/01/16 11:53), Simo Sorce wrote: > >> >> >> >> > >> >>Expired != Disabled > >> >> >> >> > >> >>this change is intentional. > >> >> >> >> > >> >> > >> >> >> >> > >> >Yes, but explain it to Active directory :-) > >> >> >> >> > >> > > >> >> >> >> > >> >Attached is patch with workaround/hack > >> >> >> >> > >> >regression with expired AD users. > >> >> >> >> > >> > > >> >> >> >> > >> ENOPATCH > >> >> >> >> > >> > >> >> >> >> > >> LS > >> >> >> >> > > > >> >> >> >> > >I think a better approach is to return the KRBKDC error from > >> >> >> >> > >the child > >> >> >> >> > >without mapping (or with an intermediate mapping) and have the > >> >> >> >> > >IPA and > >> >> >> >> > >AD providers map it on their own. > >> >> >> >> > > > >> >> >> >> > It's not related to mapping KRBKDC error codes to internal > >> >> >> >> > error code. > >> >> >> >> > The main problem is that AD return the same error code for > >> >> >> >> > expired > >> >> >> >> > and disabled user. And ad provider used generic krb5 functions. > >> >> >> >> > > >> >> >> >> > BTW the same issue would be with id_provider ldap + > >> >> >> >> > auth_provider = krb5 with AD :-( > >> >> >> >> > I'm not sure how your proposal would help. > >> >> >> >> > >> >> >> >> I think AD returns additional information in edata, maybe we can > >> >> >> >> use > >> >> >> >> that to do the proper mapping in the generic krb5 code. > >> >> >> >> > >> >> >> >> Absence of AD specific edata would indicate MIT mapping, presence > >> >> >> >> would > >> >> >> >> allow us to use that additional data to figure out the correct > >> >> >> >> mapping. > >> >> >> >> > >> >> >> >> Simo. > >> >> >> >> > >> >> >> > > >> >> >> >See MS-KILE[1] 2.2.1, I bet the two conditions returns two different > >> >> >> >windows Style errors in etext (not edata, sorry). > >> >> >> > > >> >> >> >[1] https://msdn.microsoft.com/en-us/library/cc233855.aspx > >> >> >> > > >> >> >> Interesting idea and it seems to work. > >> >> >> The main difference was in time and last octet string. > >> >> >> * response for expired user [2] > >> >> >>the last octet string in ASN: > >> >> >> 930100C00100 > >> >> > > >> >> >This is error C193 aka NT_STATUS_ACCOUNT_EXPIRED NT_STATUS > >> >> >https://git.samba.org/?p=samba.git;a=blob;f=libcli/util/ntstatus.h;h=572093b431d80c9e12461255d01063412bbfa5b5;hb=HEAD#l494 >
[SSSD] Re: [PATCH] KRB5: Discern between expired & disabled AD user
On Mon, 2016-03-07 at 16:58 +0100, Lukas Slebodnik wrote: > On (04/03/16 16:42), Simo Sorce wrote: > >On Fri, 2016-03-04 at 21:27 +0100, Lukas Slebodnik wrote: > >> On (02/03/16 10:02), Simo Sorce wrote: > >> >On Wed, 2016-03-02 at 15:34 +0100, Lukas Slebodnik wrote: > >> >> On (01/03/16 18:28), Simo Sorce wrote: > >> >> >On Tue, 2016-03-01 at 18:22 -0500, Simo Sorce wrote: > >> >> >> On Tue, 2016-03-01 at 22:34 +0100, Lukas Slebodnik wrote: > >> >> >> > On (01/03/16 12:05), Simo Sorce wrote: > >> >> >> > >On Tue, 2016-03-01 at 17:51 +0100, Lukas Slebodnik wrote: > >> >> >> > >> On (01/03/16 17:45), Lukas Slebodnik wrote: > >> >> >> > >> >On (31/01/16 11:53), Simo Sorce wrote: > >> >> >> > >> >>Expired != Disabled > >> >> >> > >> >>this change is intentional. > >> >> >> > >> >> > >> >> >> > >> >Yes, but explain it to Active directory :-) > >> >> >> > >> > > >> >> >> > >> >Attached is patch with workaround/hack > >> >> >> > >> >regression with expired AD users. > >> >> >> > >> > > >> >> >> > >> ENOPATCH > >> >> >> > >> > >> >> >> > >> LS > >> >> >> > > > >> >> >> > >I think a better approach is to return the KRBKDC error from the > >> >> >> > >child > >> >> >> > >without mapping (or with an intermediate mapping) and have the > >> >> >> > >IPA and > >> >> >> > >AD providers map it on their own. > >> >> >> > > > >> >> >> > It's not related to mapping KRBKDC error codes to internal error > >> >> >> > code. > >> >> >> > The main problem is that AD return the same error code for expired > >> >> >> > and disabled user. And ad provider used generic krb5 functions. > >> >> >> > > >> >> >> > BTW the same issue would be with id_provider ldap + > >> >> >> > auth_provider = krb5 with AD :-( > >> >> >> > I'm not sure how your proposal would help. > >> >> >> > >> >> >> I think AD returns additional information in edata, maybe we can use > >> >> >> that to do the proper mapping in the generic krb5 code. > >> >> >> > >> >> >> Absence of AD specific edata would indicate MIT mapping, presence > >> >> >> would > >> >> >> allow us to use that additional data to figure out the correct > >> >> >> mapping. > >> >> >> > >> >> >> Simo. > >> >> >> > >> >> > > >> >> >See MS-KILE[1] 2.2.1, I bet the two conditions returns two different > >> >> >windows Style errors in etext (not edata, sorry). > >> >> > > >> >> >[1] https://msdn.microsoft.com/en-us/library/cc233855.aspx > >> >> > > >> >> Interesting idea and it seems to work. > >> >> The main difference was in time and last octet string. > >> >> * response for expired user [2] > >> >>the last octet string in ASN: > >> >> 930100C00100 > >> > > >> >This is error C193 aka NT_STATUS_ACCOUNT_EXPIRED NT_STATUS > >> >https://git.samba.org/?p=samba.git;a=blob;f=libcli/util/ntstatus.h;h=572093b431d80c9e12461255d01063412bbfa5b5;hb=HEAD#l494 > >> > > >> >> * response for disabled user [3] > >> >>the last octet string in ASN: > >> >> 72C00100T > >> > > >> >This is error C072, aka NT_STATUS_ACCOUNT_DISABLED NT_STATUS > >> >https://git.samba.org/?p=samba.git;a=blob;f=libcli/util/ntstatus.h;h=572093b431d80c9e12461255d01063412bbfa5b5;hb=HEAD#l211 > >> >> > >> >> The only question is how to get etext from krb5 response. > >> >> I do not want to implement ASN.1 parser. > >> > > >> >We use asn1c in FreeIPA if we need to generate a parser, but I do not > >> >think we need to get that far. > >> >T
[SSSD] Re: [PATCH] KRB5: Discern between expired & disabled AD user
On Fri, 2016-03-04 at 21:27 +0100, Lukas Slebodnik wrote: > On (02/03/16 10:02), Simo Sorce wrote: > >On Wed, 2016-03-02 at 15:34 +0100, Lukas Slebodnik wrote: > >> On (01/03/16 18:28), Simo Sorce wrote: > >> >On Tue, 2016-03-01 at 18:22 -0500, Simo Sorce wrote: > >> >> On Tue, 2016-03-01 at 22:34 +0100, Lukas Slebodnik wrote: > >> >> > On (01/03/16 12:05), Simo Sorce wrote: > >> >> > >On Tue, 2016-03-01 at 17:51 +0100, Lukas Slebodnik wrote: > >> >> > >> On (01/03/16 17:45), Lukas Slebodnik wrote: > >> >> > >> >On (31/01/16 11:53), Simo Sorce wrote: > >> >> > >> >>Expired != Disabled > >> >> > >> >>this change is intentional. > >> >> > >> >> > >> >> > >> >Yes, but explain it to Active directory :-) > >> >> > >> > > >> >> > >> >Attached is patch with workaround/hack > >> >> > >> >regression with expired AD users. > >> >> > >> > > >> >> > >> ENOPATCH > >> >> > >> > >> >> > >> LS > >> >> > > > >> >> > >I think a better approach is to return the KRBKDC error from the > >> >> > >child > >> >> > >without mapping (or with an intermediate mapping) and have the IPA > >> >> > >and > >> >> > >AD providers map it on their own. > >> >> > > > >> >> > It's not related to mapping KRBKDC error codes to internal error code. > >> >> > The main problem is that AD return the same error code for expired > >> >> > and disabled user. And ad provider used generic krb5 functions. > >> >> > > >> >> > BTW the same issue would be with id_provider ldap + > >> >> > auth_provider = krb5 with AD :-( > >> >> > I'm not sure how your proposal would help. > >> >> > >> >> I think AD returns additional information in edata, maybe we can use > >> >> that to do the proper mapping in the generic krb5 code. > >> >> > >> >> Absence of AD specific edata would indicate MIT mapping, presence would > >> >> allow us to use that additional data to figure out the correct mapping. > >> >> > >> >> Simo. > >> >> > >> > > >> >See MS-KILE[1] 2.2.1, I bet the two conditions returns two different > >> >windows Style errors in etext (not edata, sorry). > >> > > >> >[1] https://msdn.microsoft.com/en-us/library/cc233855.aspx > >> > > >> Interesting idea and it seems to work. > >> The main difference was in time and last octet string. > >> * response for expired user [2] > >>the last octet string in ASN: > >> 930100C00100 > > > >This is error C193 aka NT_STATUS_ACCOUNT_EXPIRED NT_STATUS > >https://git.samba.org/?p=samba.git;a=blob;f=libcli/util/ntstatus.h;h=572093b431d80c9e12461255d01063412bbfa5b5;hb=HEAD#l494 > > > >> * response for disabled user [3] > >>the last octet string in ASN: > >> 72C00100T > > > >This is error C072, aka NT_STATUS_ACCOUNT_DISABLED NT_STATUS > >https://git.samba.org/?p=samba.git;a=blob;f=libcli/util/ntstatus.h;h=572093b431d80c9e12461255d01063412bbfa5b5;hb=HEAD#l211 > >> > >> The only question is how to get etext from krb5 response. > >> I do not want to implement ASN.1 parser. > > > >We use asn1c in FreeIPA if we need to generate a parser, but I do not > >think we need to get that far. > >The main issue is whether the krb5 fucntions let us get access to the > >etext data at all, it seem buried pretty low in the internal of krb5. > > > It took me a while to find out why my patch broke password change > then I realized I have unused argument in_tkt_service > in my version of krb5_get_init_creds_password. > We might enable gcc warning Wunused-argument :-) > > Please review attached patch. There is a difference between using krb5_get_init_creds_password() and the function you created in that the krb5 native API actually retries by locating a KDC master server if the initial attempt fails and the KDC that was contacted is not a master KDC. Is that functionality we want to preserve ? Otherwise LGTM. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] make async connect generic
See ticket #2968.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From dcaae5431617312b69d175274c8b29c430ec6b04 Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Wed, 2 Mar 2016 14:33:38 -0500
Subject: [PATCH 1/3] Util: Move socket setup in a common utility file
Other components may need to connect sockets, the code here is generic enough
that with minimal modifications can be used for non-ldap connections too.
So create a sss_sockets.c/h utility file with all the non-ldap specific socket
setup functions and make them available for other uses.
Resolves:
https://fedorahosted.org/sssd/ticket/2968
---
Makefile.am| 4 +
src/util/sss_ldap.c| 258 ++-
src/util/sss_sockets.c | 356 +
src/util/sss_sockets.h | 39 ++
4 files changed, 412 insertions(+), 245 deletions(-)
create mode 100644 src/util/sss_sockets.c
create mode 100644 src/util/sss_sockets.h
diff --git a/Makefile.am b/Makefile.am
index 4e4f38a5eaf5bfa2cfafa88b9b32848e6c27131b..587595f1ba341403e6a1699fe314d036bd49e234 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1663,6 +1663,7 @@ ipa_ldap_opt_tests_SOURCES = \
src/providers/ad/ad_opts.c \
src/providers/ipa/ipa_opts.c \
src/providers/krb5/krb5_opts.c \
+src/util/sss_sockets.c \
src/util/sss_ldap.c \
src/tests/ipa_ldap_opt-tests.c
ipa_ldap_opt_tests_CFLAGS = \
@@ -1877,6 +1878,7 @@ TEST_MOCK_RESP_OBJ = \
src/responder/common/responder_cache_req.c
TEST_MOCK_PROVIDER_OBJ = \
+ src/util/sss_sockets.c \
src/util/sss_ldap.c \
src/providers/data_provider_opts.c \
src/providers/ldap/ldap_opts.c \
@@ -2264,6 +2266,7 @@ sdap_tests_SOURCES = \
src/providers/ldap/sdap_range.c \
src/providers/ldap/ldap_opts.c \
src/providers/ipa/ipa_opts.c \
+src/util/sss_sockets.c \
src/util/sss_ldap.c \
src/tests/cmocka/test_sdap.c \
$(NULL)
@@ -2879,6 +2882,7 @@ libsss_ldap_common_la_SOURCES = \
src/providers/ldap/sdap.c \
src/providers/ipa/ipa_dn.c \
src/util/user_info_msg.c \
+src/util/sss_sockets.c \
src/util/sss_ldap.c \
$(NULL)
libsss_ldap_common_la_CFLAGS = \
diff --git a/src/util/sss_ldap.c b/src/util/sss_ldap.c
index c440d5445133c8b31f662fc69b35e2296c3f1702..7fdaadb5cebf5d3e7fe7f8fb1780f0db3dbcae4a 100644
--- a/src/util/sss_ldap.c
+++ b/src/util/sss_ldap.c
@@ -17,18 +17,13 @@
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#include
-#include
-#include
-#include
-#include
-#include
#include "config.h"
-
-#include "providers/ldap/sdap.h"
-#include "util/sss_ldap.h"
#include "util/util.h"
+#include "util/sss_sockets.h"
+#include "util/sss_ldap.h"
+
+#include "providers/ldap/sdap.h"
const char* sss_ldap_err2string(int err)
{
@@ -103,183 +98,6 @@ int sss_ldap_control_create(const char *oid, int iscritical,
}
#ifdef HAVE_LDAP_INIT_FD
-struct sdap_async_sys_connect_state {
-long old_flags;
-struct tevent_fd *fde;
-int fd;
-socklen_t addr_len;
-struct sockaddr_storage addr;
-};
-
-static void sdap_async_sys_connect_done(struct tevent_context *ev,
-struct tevent_fd *fde, uint16_t flags,
-void *priv);
-
-static struct tevent_req *sdap_async_sys_connect_send(TALLOC_CTX *mem_ctx,
-struct tevent_context *ev,
-int fd,
-const struct sockaddr *addr,
-socklen_t addr_len)
-{
-struct tevent_req *req;
-struct sdap_async_sys_connect_state *state;
-long flags;
-int ret;
-int fret;
-
-flags = fcntl(fd, F_GETFL, 0);
-if (flags == -1) {
-DEBUG(SSSDBG_CRIT_FAILURE, "fcntl F_GETFL failed.\n");
-return NULL;
-}
-
-req = tevent_req_create(mem_ctx, ,
-struct sdap_async_sys_connect_state);
-if (req == NULL) {
-DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create failed.\n");
-return NULL;
-}
-
-state->old_flags = flags;
-state->fd = fd;
-state->addr_len = addr_len;
-memcpy(>addr, addr, addr_len);
-
-ret = fcntl(fd, F_SETFL, flags | O_NONBLOCK);
-if (ret != EOK) {
-DEBUG(SSSDBG_CRIT_FAILURE, "fcntl F_SETFL failed.\n");
-goto done;
-}
-
-ret = connect(fd, addr, addr_len);
-if (ret == EOK) {
-goto done;
-}
-
-ret = errno;
-switch(ret) {
-case EINPROGRESS:
-case EINTR:
-state->fde = tevent_add_fd(ev, state, fd,
- TEVENT_FD_READ |
[SSSD] Re: [PATCH] KRB5: Discern between expired & disabled AD user
On Wed, 2016-03-02 at 15:34 +0100, Lukas Slebodnik wrote: > On (01/03/16 18:28), Simo Sorce wrote: > >On Tue, 2016-03-01 at 18:22 -0500, Simo Sorce wrote: > >> On Tue, 2016-03-01 at 22:34 +0100, Lukas Slebodnik wrote: > >> > On (01/03/16 12:05), Simo Sorce wrote: > >> > >On Tue, 2016-03-01 at 17:51 +0100, Lukas Slebodnik wrote: > >> > >> On (01/03/16 17:45), Lukas Slebodnik wrote: > >> > >> >On (31/01/16 11:53), Simo Sorce wrote: > >> > >> >>Expired != Disabled > >> > >> >>this change is intentional. > >> > >> >> > >> > >> >Yes, but explain it to Active directory :-) > >> > >> > > >> > >> >Attached is patch with workaround/hack > >> > >> >regression with expired AD users. > >> > >> > > >> > >> ENOPATCH > >> > >> > >> > >> LS > >> > > > >> > >I think a better approach is to return the KRBKDC error from the child > >> > >without mapping (or with an intermediate mapping) and have the IPA and > >> > >AD providers map it on their own. > >> > > > >> > It's not related to mapping KRBKDC error codes to internal error code. > >> > The main problem is that AD return the same error code for expired > >> > and disabled user. And ad provider used generic krb5 functions. > >> > > >> > BTW the same issue would be with id_provider ldap + > >> > auth_provider = krb5 with AD :-( > >> > I'm not sure how your proposal would help. > >> > >> I think AD returns additional information in edata, maybe we can use > >> that to do the proper mapping in the generic krb5 code. > >> > >> Absence of AD specific edata would indicate MIT mapping, presence would > >> allow us to use that additional data to figure out the correct mapping. > >> > >> Simo. > >> > > > >See MS-KILE[1] 2.2.1, I bet the two conditions returns two different > >windows Style errors in etext (not edata, sorry). > > > >[1] https://msdn.microsoft.com/en-us/library/cc233855.aspx > > > Interesting idea and it seems to work. > The main difference was in time and last octet string. > * response for expired user [2] >the last octet string in ASN: > 930100C00100 This is error C193 aka NT_STATUS_ACCOUNT_EXPIRED NT_STATUS https://git.samba.org/?p=samba.git;a=blob;f=libcli/util/ntstatus.h;h=572093b431d80c9e12461255d01063412bbfa5b5;hb=HEAD#l494 > * response for disabled user [3] >the last octet string in ASN: > 72C00100T This is error C072, aka NT_STATUS_ACCOUNT_DISABLED NT_STATUS https://git.samba.org/?p=samba.git;a=blob;f=libcli/util/ntstatus.h;h=572093b431d80c9e12461255d01063412bbfa5b5;hb=HEAD#l211 > > The only question is how to get etext from krb5 response. > I do not want to implement ASN.1 parser. We use asn1c in FreeIPA if we need to generate a parser, but I do not think we need to get that far. The main issue is whether the krb5 fucntions let us get access to the etext data at all, it seem buried pretty low in the internal of krb5. Simo. > LS > > [1] https://msdn.microsoft.com/en-us/library/cc233855.aspx > [2] > http://www.lapo.it/asn1js/#7E753073A003020105A10302011EA411180F32303136303330323133323533355AA50502030D327DA603020112A90C1B0A5353534441442E434F4DAA1F301DA003020102A11630141B066B72627467741B0A5353534441442E434F4DAC1904173015A103020103A20E040C930100C00100 > [3] > http://www.lapo.it/asn1js/#7E753073A003020105A10302011EA411180F32303136303330323132313734365AA50502030E0586A603020112A90C1B0A5353534441442E434F4DAA1F301DA003020102A11630141B066B72627467741B0A5353534441442E434F4DAC1904173015A103020103A20E040C72C00100 -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] KRB5: Discern between expired & disabled AD user
On Tue, 2016-03-01 at 18:22 -0500, Simo Sorce wrote: > On Tue, 2016-03-01 at 22:34 +0100, Lukas Slebodnik wrote: > > On (01/03/16 12:05), Simo Sorce wrote: > > >On Tue, 2016-03-01 at 17:51 +0100, Lukas Slebodnik wrote: > > >> On (01/03/16 17:45), Lukas Slebodnik wrote: > > >> >On (31/01/16 11:53), Simo Sorce wrote: > > >> >>Expired != Disabled > > >> >>this change is intentional. > > >> >> > > >> >Yes, but explain it to Active directory :-) > > >> > > > >> >Attached is patch with workaround/hack > > >> >regression with expired AD users. > > >> > > > >> ENOPATCH > > >> > > >> LS > > > > > >I think a better approach is to return the KRBKDC error from the child > > >without mapping (or with an intermediate mapping) and have the IPA and > > >AD providers map it on their own. > > > > > It's not related to mapping KRBKDC error codes to internal error code. > > The main problem is that AD return the same error code for expired > > and disabled user. And ad provider used generic krb5 functions. > > > > BTW the same issue would be with id_provider ldap + > > auth_provider = krb5 with AD :-( > > I'm not sure how your proposal would help. > > I think AD returns additional information in edata, maybe we can use > that to do the proper mapping in the generic krb5 code. > > Absence of AD specific edata would indicate MIT mapping, presence would > allow us to use that additional data to figure out the correct mapping. > > Simo. > See MS-KILE[1] 2.2.1, I bet the two conditions returns two different windows Style errors in etext (not edata, sorry). [1] https://msdn.microsoft.com/en-us/library/cc233855.aspx Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] KRB5: Discern between expired & disabled AD user
On Tue, 2016-03-01 at 22:34 +0100, Lukas Slebodnik wrote: > On (01/03/16 12:05), Simo Sorce wrote: > >On Tue, 2016-03-01 at 17:51 +0100, Lukas Slebodnik wrote: > >> On (01/03/16 17:45), Lukas Slebodnik wrote: > >> >On (31/01/16 11:53), Simo Sorce wrote: > >> >>Expired != Disabled > >> >>this change is intentional. > >> >> > >> >Yes, but explain it to Active directory :-) > >> > > >> >Attached is patch with workaround/hack > >> >regression with expired AD users. > >> > > >> ENOPATCH > >> > >> LS > > > >I think a better approach is to return the KRBKDC error from the child > >without mapping (or with an intermediate mapping) and have the IPA and > >AD providers map it on their own. > > > It's not related to mapping KRBKDC error codes to internal error code. > The main problem is that AD return the same error code for expired > and disabled user. And ad provider used generic krb5 functions. > > BTW the same issue would be with id_provider ldap + > auth_provider = krb5 with AD :-( > I'm not sure how your proposal would help. I think AD returns additional information in edata, maybe we can use that to do the proper mapping in the generic krb5 code. Absence of AD specific edata would indicate MIT mapping, presence would allow us to use that additional data to figure out the correct mapping. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] KRB5: Discern between expired & disabled AD user
On Tue, 2016-03-01 at 17:51 +0100, Lukas Slebodnik wrote: > On (01/03/16 17:45), Lukas Slebodnik wrote: > >On (31/01/16 11:53), Simo Sorce wrote: > >>Expired != Disabled > >>this change is intentional. > >> > >Yes, but explain it to Active directory :-) > > > >Attached is patch with workaround/hack > >regression with expired AD users. > > > ENOPATCH > > LS I think a better approach is to return the KRBKDC error from the child without mapping (or with an intermediate mapping) and have the IPA and AD providers map it on their own. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Configuring tlog from SSSD
On Thu, 2016-01-28 at 11:24 +0100, Lukas Slebodnik wrote:
> On (27/01/16 16:30), Nikolai Kondrashov wrote:
> > On 01/27/2016 04:17 PM, Lukas Slebodnik wrote:
> > > You mention many options which could be possibly passed to tlog.
> > > e.g.
> > > TLOG_REC_CONF='{
> > > "shell":"/bin/bash",
> > > "warning": "WARNING! Your session is being recorded!\n",
> > > "latency": 10,
> > > "writer": "syslog",
> > > "syslog": {
> > > "facility": "authpriv",
> > > "level":"info"
> > > }
> > > }'
> > >
> > > Where will be these option stored? In LDAP?
> >
> > No idea yet. Some of them definitely will, but likely not all.
> >
> In this case I would prefer to have the simplest change in sssd
> as possible. https://fedorahosted.org/sssd/ticket/2893
>
> SSSD should just enforce using tlog as a shell and provide
> name of profile. This profile will be used by tlog to download
> configuration (json) from webservice.
> The similar approach was discussed with IPA integration with GNOME.
> IIRC there is already POC; Alexander might know more.
Having tlog load data over a network would make it a lot more complex
and expose an attack surface.
It would also fail for offline cases.
For IPA integration we will also probably want to store this data in
LDAP, and not have to invent a new webservice, new authorization engine
and so on and so forth.
Simo.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] fix account lockout reporting with the krb5 provider
Expired != Disabled this change is intentional. Simo. - Original Message - > From: "Lukas Slebodnik" <lsleb...@redhat.com> > To: "Development of the System Security Services Daemon" > <sssd-devel@lists.fedorahosted.org> > Cc: "Simo Sorce" <s...@redhat.com> > Sent: Friday, January 29, 2016 9:22:23 AM > Subject: Re: [SSSD] Re: [PATCH] fix account lockout reporting with the krb5 > provider > > On (14/01/16 18:38), Jakub Hrozek wrote: > >On Thu, Jan 14, 2016 at 12:09:12PM -0500, Simo Sorce wrote: > >> > OK to push now? > >> > >> Yes please :-) > >> > >> Simo > > > >* master: 19e44537c28f6d5f011cd7ac885c74c1e892605f > I have a question about this patch. > > I can see some inconsistencies for expired/disabled user. > > Here is a LDIF for expiration of user > dn: cn=$username,$ou,$basedn > changetype: modify > replace: accountExpires > accountExpires: 1294650180 > > and for disabling user > dn: cn=$username,$ou,$basedn > changetype: modify > replace: userAccountControl > userAccountControl: 514 > > > There are test with ssh + password (pam auth) > and ssh + key (pam pam account) > > and here is current state with master. > -- > disabled AD user > pam_sss(sshd:auth): received for user testuser01-17923: 6 (Permission > denied) > > pam_sss(sshd:account): system info: [The user account is disabled on the AD > server] > pam_sss(sshd:account): Access denied for user testuser01-17923: 6 > (Permission denied) > > expired AD user > pam_sss(sshd:auth): received for user testuser01-17923: 6 (Permission > denied) > > pam_sss(sshd:account): system info: [The user account is expired on the AD > server] > pam_sss(sshd:account): Access denied for user testuser01-17923: 13 (User > account has expired) > > > Previously, we could see info "User account has expired" > even in auth phase. And it's unusual that auth and account returned different > error codes. > > I think that this patch fixed "auth" PAM error code for disabled user > but it broke for expired user or did I miss something? > > LS > ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On Wed, 2016-01-20 at 16:38 +0100, Lukas Slebodnik wrote:
> On (19/01/16 15:38), Simo Sorce wrote:
> >On Tue, 2016-01-19 at 10:34 -0500, Simo Sorce wrote:
> >> On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote:
> >> [...]
> >> > >+#endif /* __SSSD_UTIL_SELINUX_H__ */
> >> > BTW will we need this header file if we make
> >> > struct cli_creds opaque?
> >>
> >> Replying to both your mails here:
> >> Making cli_creds opaque requires using a pointer and dealing with
> >> allocating it, I guess I can do that, the headers file would still be
> >> needed in order to avoid huge ifdefs around the functions that implement
> >> handling SELinux stuff. It makes the code a lot more readable and
> >> searchable.
> >>
> >> Simo.
> >>
> >
> >Attached find patch that changes the code so that cli_creds is now
> >opaque.
> >This *should* work w/o the patch that changes the headers but I haven't
> >tested it w/o that patch yet.
> >
> I had an issue to build it correctly with ifp responder.
> src/responder/ifp/ifpsrv_util.c: In function ‘ifp_req_create’:
> src/responder/ifp/ifpsrv_util.c:64:30: error: passing argument 1 of
> ‘check_allowed_uids’ makes pointer from integer without a cast
> [-Werror=int-conversion]
> ret = check_allowed_uids(dbus_req->client,
> ^
> In file included from ../sssd/src/responder/ifp/ifp_private.h:27:0,
> from ../sssd/src/responder/ifp/ifpsrv_util.c:27:
> src/responder/common/responder.h:334:9: note: expected ‘struct cli_creds *’
> but argument is of type ‘int64_t {aka long int}’
> errno_t check_allowed_uids(struct cli_creds *creds,
> ^
>
> ifp responder uses different way to obtain uid. I changed back the prototype
> of
> check_allowed_uids.
> Here is a diff on to of your patch.
>
> diff --git a/src/responder/common/responder.h
> b/src/responder/common/responder.h
> index 419a863..3b70b69 100644
> --- a/src/responder/common/responder.h
> +++ b/src/responder/common/responder.h
> @@ -331,8 +331,7 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx,
> const char *csv_string,
> size_t *_uid_count, uid_t **_uids);
>
> uid_t client_euid(struct cli_creds *creds);
> -errno_t check_allowed_uids(struct cli_creds *creds,
> - size_t allowed_uids_count,
> +errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
> uid_t *allowed_uids);
>
> struct tevent_req *
> diff --git a/src/responder/common/responder_common.c
> b/src/responder/common/responder_common.c
> index 27193ce..6ac1ea2 100644
> --- a/src/responder/common/responder_common.c
> +++ b/src/responder/common/responder_common.c
> @@ -138,8 +138,7 @@ uid_t client_euid(struct cli_creds *creds)
> return cli_creds_get_uid(creds);
> }
>
> -errno_t check_allowed_uids(struct cli_creds *creds,
> - size_t allowed_uids_count,
> +errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
> uid_t *allowed_uids)
> {
> size_t c;
> @@ -149,7 +148,7 @@ errno_t check_allowed_uids(struct cli_creds *creds,
> }
>
> for (c = 0; c < allowed_uids_count; c++) {
> -if (client_euid(creds) == allowed_uids[c]) {
> +if (uid == allowed_uids[c]) {
> return EOK;
> }
> }
> @@ -449,12 +448,13 @@ static void accept_fd_handler(struct tevent_context *ev,
> return;
> }
>
> -ret = check_allowed_uids(cctx->creds, rctx->allowed_uids_count,
> +ret = check_allowed_uids(client_euid(cctx->creds),
> rctx->allowed_uids_count,
> rctx->allowed_uids);
> if (ret != EOK) {
> if (ret == EACCES) {
> -DEBUG(SSSDBG_CRIT_FAILURE, "Access denied for uid [%d].\n",
> -(int)client_euid(cctx->creds));
> +DEBUG(SSSDBG_CRIT_FAILURE,
> + "Access denied for uid [%"SPRIuid"].\n",
> + client_euid(cctx->creds));
> } else {
> DEBUG(SSSDBG_OP_FAILURE, "check_allowed_uids failed.\n");
> }
> diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
> index 5fe3e6b..bfc534f 100644
> --- a/src/responder/pam/pamsrv_cmd.c
> +++ b/src/responder/pam/pamsrv_cmd.c
> @@ -1001,7 +1001,7 @@ static bool is_uid_trusted(struct cli_cred
[SSSD] Re: [PATCHES] UTIL: Provide varargs version of debug_fn
On Sat, 2016-01-16 at 12:33 +0100, Lukas Slebodnik wrote: > On (15/01/16 16:09), Simo Sorce wrote: > >On Fri, 2016-01-15 at 12:44 +0100, Lukas Slebodnik wrote: > >> On (15/01/16 12:03), Pavel Březina wrote: > >> >On 01/12/2016 10:15 AM, Lukas Slebodnik wrote: > >> >>ehlo, > >> >> > >> >>The main reason for these patch was to improve > >> >>recently added logging to hbac. > >> >> > >> >>Side effect of these change is improvement for libldb > >> >>and libsemanage (6th patch) > >> >> > >> >>4th patch is not API/ABI change because > >> >>such version has not beeen released yet. > >> >>If you do not like change in hbac callback > >> >>hbac_debug_fn_t then we should also remove > >> >>because it is too internal then we should > >> >>remove also the first two arguments. > >> >>"file", "line" also leaks internal data from libhbac. > >> >>Removing the first two arguments would be almost > >> >>consistent callbacks in libldb and libsemanage. > >> >> > >> >>LS > >> > > >> >Hi, > >> >I'm getting following errors: > >> > > >> >In file included from > >> >/home/pbrezina/workspace/sssd/src/python/pyhbac.c:26:0: > >> >/home/pbrezina/workspace/sssd/src/providers/ipa/ipa_hbac.h:54:0: error: > >> >"SSS_ATTRIBUTE_PRINTF" redefined [-Werror] > >> > #define SSS_ATTRIBUTE_PRINTF(a1, a2) __attribute__((format(printf, a1, > >> > a2))) > >> > ^ > >> >In file included from > >> >/home/pbrezina/workspace/sssd/src/python/pyhbac.c:24:0: > >> >/home/pbrezina/workspace/sssd/src/util/util.h:62:0: note: this is the > >> >location of the previous definition > >> > #define SSS_ATTRIBUTE_PRINTF(a1, a2) __attribute__ ((format (printf, a1, > >> >a2))) > >> > > >> I might add another #ifdef guard but I decided to rename macro > >> SSS_ATTRIBUTE_PRINTF -> HBAC_ATTRIBUTE_PRINTF > > > > > >Why is debug_fn being called directly in hbac/semanage/etc.. insted of > >being called through a common macro ? > > > libldb, libsemanage ... provide function which allows you to register > your own debug function > > e.g. > libldb has ldb_set_debug and we register ldb_debug_messages wih prototype >void ldb_debug_messages(void *context, enum ldb_debug_level level, >const char *fmt, va_list ap) > > However you cannot call DEBUG macro with agruments:"const char *fmt, va_list > ap" > So you need to call "vasprintf(, fmt, ap)" and then log message > and then log message + release message; it is inefficient. > > The same situation is with libhbac and libsemanage. > > Does this answer your question? Yes. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On Tue, 2016-01-19 at 10:34 -0500, Simo Sorce wrote:
> On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote:
> [...]
> > >+#endif /* __SSSD_UTIL_SELINUX_H__ */
> > BTW will we need this header file if we make
> > struct cli_creds opaque?
>
> Replying to both your mails here:
> Making cli_creds opaque requires using a pointer and dealing with
> allocating it, I guess I can do that, the headers file would still be
> needed in order to avoid huge ifdefs around the functions that implement
> handling SELinux stuff. It makes the code a lot more readable and
> searchable.
>
> Simo.
>
Attached find patch that changes the code so that cli_creds is now
opaque.
This *should* work w/o the patch that changes the headers but I haven't
tested it w/o that patch yet.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 47d2ab3bb559ed0b0ea112c2eb8b3c4ceaa86929 Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Mon, 18 Jan 2016 15:21:57 -0500
Subject: [PATCH] Util: Improve code to get connection credentials
Adds support to get SELINUX context and make code more abstract so
that struct ucred (if availale) can be used w/o redefining uid,gid,pid to
int32. Also givces a layer of indirection that may come handy if we want
to imrpove the code further in the future.
Resolves:
https://fedorahosted.org/sssd/ticket/
---
Makefile.am | 2 +
src/responder/common/responder.h| 11 +++--
src/responder/common/responder_common.c | 59
src/responder/pam/pamsrv_cmd.c | 23 -
src/util/util_creds.h | 82 +
5 files changed, 141 insertions(+), 36 deletions(-)
create mode 100644 src/util/util_creds.h
diff --git a/Makefile.am b/Makefile.am
index 407053b1a6dcd0255be76ae7f9252a671b965ea2..98f5df9ff4fbb122930c6109620a58cfd1ca 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -496,6 +496,7 @@ SSSD_LIBS = \
$(COLLECTION_LIBS) \
$(DHASH_LIBS) \
$(OPENLDAP_LIBS) \
+$(SELINUX_LIBS) \
$(TDB_LIBS)
PYTHON_BINDINGS_LIBS = \
@@ -556,6 +557,7 @@ dist_noinst_HEADERS = \
src/util/authtok-utils.h \
src/util/util_safealign.h \
src/util/util_sss_idmap.h \
+src/util/util_creds.h \
src/monitor/monitor.h \
src/monitor/monitor_interfaces.h \
src/responder/common/responder.h \
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 6b511368c9b08d1cfc828d16f57a2cde902dc82b..69eb6a1888a0c3632fc61768e12ce60c0114d22b 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -123,6 +123,8 @@ struct resp_ctx {
bool shutting_down;
};
+struct cli_creds;
+
struct cli_ctx {
struct tevent_context *ev;
struct resp_ctx *rctx;
@@ -131,9 +133,8 @@ struct cli_ctx {
tevent_fd_handler_t cfd_handler;
struct sockaddr_un addr;
int priv;
-int32_t client_euid;
-int32_t client_egid;
-int32_t client_pid;
+
+struct cli_creds *creds;
void *protocol_ctx;
void *state_ctx;
@@ -331,7 +332,9 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const char *csv_string,
bool allow_sss_loop,
size_t *_uid_count, uid_t **_uids);
-errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
+uid_t client_euid(struct cli_creds *creds);
+errno_t check_allowed_uids(struct cli_creds *creds,
+ size_t allowed_uids_count,
uid_t *allowed_uids);
struct tevent_req *
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index fecb72384786ea2840ed349a6afcb36efc60d492..fc074fee25f96a931649d39cccaa4fd8312ee4e7 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -42,6 +42,7 @@
#include "providers/data_provider.h"
#include "monitor/monitor_interfaces.h"
#include "sbus/sbus_client.h"
+#include "util/util_creds.h"
#ifdef HAVE_SYSTEMD
#include
@@ -88,16 +89,20 @@ static int client_destructor(struct cli_ctx *ctx)
static errno_t get_client_cred(struct cli_ctx *cctx)
{
-cctx->client_euid = -1;
-cctx->client_egid = -1;
-cctx->client_pid = -1;
+SEC_CTX secctx;
+int ret;
+
+cctx->creds = talloc(cctx, struct cli_creds);
+if (!cctx->creds) return ENOMEM;
#ifdef HAVE_UCRED
-int ret;
-struct ucred client_cred;
-socklen_t client_cred_len = sizeof(client_cred);
+socklen_t client_cred_len = sizeof(struct ucred);
-ret = getsockopt(cctx->cfd, SOL_SOCKET, SO_PEERCRED, _cred,
+cctx->creds->ucred.uid = -1;
+cctx->creds->ucred.gid = -1;
+cctx->creds->ucred.pid = -1;
+
+ret = getsockopt(cctx->cfd, SOL_SOCKET, SO_PEERCRED, >creds->ucred,
_cred_
[SSSD] Re: [PATCH] Fix headers order
On Tue, 2016-01-19 at 20:20 +0100, Lukas Slebodnik wrote: > On (19/01/16 11:30), Simo Sorce wrote: > >On Tue, 2016-01-19 at 17:06 +0100, Lukas Slebodnik wrote: > >> On (19/01/16 16:47), Michal Židek wrote: > >> >On 01/19/2016 04:28 PM, Simo Sorce wrote: > >> >>On Tue, 2016-01-19 at 02:54 +0100, Michal Židek wrote: > >> >>>On 01/19/2016 12:03 AM, Simo Sorce wrote: > >> >>>>Found this while working on another patch. > >> >>>> > >> >>>>It is not evident by this patch alone but ... "trust me" :-) > >> >>>>(I'll send the other patch next, try to apply just that one and see > >> >>>>what > >> >>>>I mean if you want) > >> >>>> > >> >>>>Simo. > >> >>>> > >> >>>> > >> >>> > >> >>>Hi Simo! > >> >>> > >> >>>I wonder if including config.h indirectly through > >> >>>util.h is a good thing. It may be better > >> >>>to simply include config.h at the beginning of every > >> >>>.c file (after license) as a rule of thumb. This way > >> >>>even if we do not need util.h, we will have the same > >> >>>beginning of file and it will be more difficult to forget > >> >>>config.h. What do you think? > >> >> > >> >>I a ok with such a rule, I can change the patch to explicitly add > >> >>config.h as first in all files I touched, are the other cleanups I did > >> >>also ok ? > >> >>Or do we want a patch that only adds config.h as the first header and > >> >>touches nothing else ? > >> > > >> >Yes, adding just config.h (or moving it to the top) > >> >is probably the best thing to do. It is minimal change > >> >and it will solve the immediate problem. And Lukas > >> >will also agree with this IMO (see the explanation > >> >below). > >> > > >> >> > >> >>>That being said, I know you made this patch in order > >> >>>to move work on another patch, so I definitely do not > >> >>>want to block the review. > >> >>> > >> >>>CI passed: > >> >>>http://sssd-ci.duckdns.org/logs/job/35/72/summary.html > >> >>> > >> >>>But I will wait with the ack until you respond to the > >> >>>question above. > >> >>> > >> >>>Btw. we have quite a lot of files that do not use > >> >>>util.h that probably already use this rule (I did > >> >>>not check all of them, just did the grep). I think > >> >>>the rule would add consistency to the code. > >> >> > >> >>Yes a lot of files do the "right" thing (between quotes because Lukas > >> >>apparently has a different opinion), just not all of them. > >> > > >> >I think there is a misunderstanding. Lukas is right that > >> >it is a common practice to include system headers first > >> >and after that your own headers. However config.h is > >> >an exception. It is a special header that set's up the > >> >environment. Btw. the reason why system headers should be > >> >added before our own headers is similar to why config.h should > >> >be the first header. They sometimes also change environment > >> >(by environment I mean, they define special macros that > >> >change behavior of some functions etc.) > >> > > >> >If I undestood it correctly Lukas did not protest against > >> >including config.h as the first header, but he did not like > >> >the util.h as first (because it is our own header). > >> > > >> > >> I read/replied to this patch before seeing a problematic patch > >> "[PATCH] Abstract and improve connection credential handling" > >> > >> There would not be a problem to include "config.h" in every > >> implentation module which includes "src/responder/common/responder.h" > >> But we would not fix a problem. The same issue would be in > >> any new implementation module for new responder. > >> > >> I think I proposed better solution in reply to the problematic patch. > >> If we move conditional definition of struct cli_creds or > >> (UCRED_CTX, SELINUX_CTX)
[SSSD] Re: [PATCH] Fix headers order
On Tue, 2016-01-19 at 09:46 +0100, Lukas Slebodnik wrote: > On (18/01/16 18:03), Simo Sorce wrote: > >Found this while working on another patch. > > > >It is not evident by this patch alone but ... "trust me" :-) > >(I'll send the other patch next, try to apply just that one and see what > >I mean if you want) > > > Then probably you have something bad in your patches. > > It's best practice to include system beader file before > local header file and you changed that. Sorry Lukas, but I have to call BS on this. > I will wait for your another patch and I will try to propose > different solution :-) The other patch is already on the list, and I can also describe exactly what is wrong with current headers. [NOTE: I am going from memory so I may get a system config file name wrong here and there, follow the essencce not the letter please] They includes system files before config.h, system headers (as any other are included only once after which they #define themselves off, and most system headers end up calling each other. Specifically what happens here is that because a system header is include before config.h it does *not* see the _GNU_SOURCE macro defined there, and therefore sys/types.h (which is included by pretty much anything) ends up *not* defining __USE_GNU internally. This in turn ends up causing things like struct ucred to not be defined at all as they are guarded by __USE_GNU. If you want to include system headers *before "config.h" that is a valid choice but then you'll have to move all declarations like _GNU_SOURCE into makefile -D variables and not incllude them in config.h because then it is too late. If you want to go that approach, feel free to do it as well, but unless you do that within a short time, for now we should fix the headers. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] Abstract and improve connection credential handling
On Tue, 2016-01-19 at 11:23 +0100, Lukas Slebodnik wrote: [...] > >+#endif /* __SSSD_UTIL_SELINUX_H__ */ > BTW will we need this header file if we make > struct cli_creds opaque? Replying to both your mails here: Making cli_creds opaque requires using a pointer and dealing with allocating it, I guess I can do that, the headers file would still be needed in order to avoid huge ifdefs around the functions that implement handling SELinux stuff. It makes the code a lot more readable and searchable. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] Fix headers order
Found this while working on another patch.
It is not evident by this patch alone but ... "trust me" :-)
(I'll send the other patch next, try to apply just that one and see what
I mean if you want)
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 64c71d6fdd57527af607a61f32c7e1eb7f632386 Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Mon, 18 Jan 2016 17:25:37 -0500
Subject: [PATCH] Headers: Fix subtle errors in header oredering
The config.h header MUST be incliuded first as it defines variables that change
the behavior of system libraries (for example _GNU_SOURCE). If config.h is not
included first then some files will have incorrect definitions.
While there try to fix as many redundant inclusions and ordering issues in the
responders. Uncovered by work on a patch that uses struct ucred
Resolves:
https://fedorahosted.org/sssd/ticket/2932
---
src/providers/data_provider.h | 12 +---
src/responder/autofs/autofssrv.c | 3 ---
src/responder/autofs/autofssrv_cmd.c | 3 ---
src/responder/autofs/autofssrv_dp.c| 5 -
src/responder/common/responder_cache_req.c | 7 ++-
src/responder/common/responder_cmd.c | 3 +--
src/responder/common/responder_dp.c| 2 --
src/responder/ifp/ifp_cache.c | 5 +
src/responder/ifp/ifp_components.c | 8 +---
src/responder/ifp/ifp_domains.c| 7 +--
src/responder/ifp/ifp_groups.c | 3 ---
src/responder/ifp/ifp_iface.c | 2 +-
src/responder/ifp/ifp_iface_nodes.c| 1 +
src/responder/ifp/ifp_users.c | 6 +-
src/responder/ifp/ifpsrv.c | 13 +
src/responder/ifp/ifpsrv_util.c| 2 +-
src/responder/nss/nsssrv.c | 14 +-
src/responder/nss/nsssrv_services.c| 2 --
src/responder/pam/pam_LOCAL_domain.c | 5 ++---
src/responder/pam/pamsrv.c | 15 +--
src/responder/pam/pamsrv_cmd.c | 2 --
src/responder/pam/pamsrv_dp.c | 7 +--
src/responder/pam/pamsrv_p11.c | 3 ---
src/responder/ssh/sshsrv.c | 3 ---
src/responder/ssh/sshsrv_dp.c | 5 -
src/responder/sudo/sudosrv.c | 3 ---
src/responder/sudo/sudosrv_cmd.c | 4
src/responder/sudo/sudosrv_dp.c| 5 -
src/responder/sudo/sudosrv_query.c | 6 --
src/util/usertools.c | 7 +--
src/util/util.h| 2 ++
31 files changed, 20 insertions(+), 145 deletions(-)
diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h
index 39051b90c3aad96f62dcbb86a20bcfd8c954879b..671ec198e38c9cacc6d96d14ad9c8b94312b4e79 100644
--- a/src/providers/data_provider.h
+++ b/src/providers/data_provider.h
@@ -22,23 +22,13 @@
#ifndef __DATA_PROVIDER_H__
#define __DATA_PROVIDER_H__
-#include "config.h"
-
-#include
+#include "util/util.h"
#include
-#include
#include
#ifdef USE_KEYRING
-#include
#include
#endif
-#include
-#include
-#include
#include
-
-#include "util/util.h"
-#include "confdb/confdb.h"
#include "sbus/sssd_dbus.h"
#include "sbus/sbus_client.h"
#include "sss_client/sss_cli.h"
diff --git a/src/responder/autofs/autofssrv.c b/src/responder/autofs/autofssrv.c
index ff30167298e0028da3f14f4d63a4d129c9c70bf0..65ab99ad8817a62fb3bdd9b77d5574289b615903 100644
--- a/src/responder/autofs/autofssrv.c
+++ b/src/responder/autofs/autofssrv.c
@@ -20,10 +20,7 @@
along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#include
-
#include "util/util.h"
-#include "confdb/confdb.h"
#include "monitor/monitor_interfaces.h"
#include "responder/common/responder.h"
#include "providers/data_provider.h"
diff --git a/src/responder/autofs/autofssrv_cmd.c b/src/responder/autofs/autofssrv_cmd.c
index 42754aceb0d695b7d4dd17b196a785a2550c..cd688d651b56f076a36bb9bb684e503ebfe46553 100644
--- a/src/responder/autofs/autofssrv_cmd.c
+++ b/src/responder/autofs/autofssrv_cmd.c
@@ -20,15 +20,12 @@
along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#include
-
#include "util/util.h"
#include "responder/common/responder.h"
#include "responder/common/responder_packet.h"
#include "responder/autofs/autofs_private.h"
#include "db/sysdb.h"
#include "db/sysdb_autofs.h"
-#include "confdb/confdb.h"
static int autofs_cmd_send_error(struct autofs_cmd_ctx *cmdctx, int err)
{
diff --git a/src/responder/autofs/autofssrv_dp.c b/src/responder/autofs/autofssrv_dp.c
index 041f0629eafadb92fc6b8ec7190830a2662b40a0..6eb23a02285c86804b4842de49f2eea99c594fa4 100644
--- a/src/responder/autofs/autofssrv_dp.c
+++
[SSSD] [PATCH] Abstract and improve connection credential handling
I will needed selinux support later on in the secrets work, so I looked
into how we were getting peercreds and found it could be improved by
making it a little more abstract.
This patch also uncovered issues with header inclusion (patch sent
earlier).
(I did not open a bug for this one)
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 7cc82eff48dabc4b15e119146f36597f4cd75827 Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Mon, 18 Jan 2016 15:21:57 -0500
Subject: [PATCH] Util: Improve code to get connection credentials
Adds support to get SELINUX context and make code more abstract so
that struct ucred (if availale) can be used w/o redefining uid,gid,pid to
int32. Also givces a layer of indirection that may come handy if we want
to imrpove the code further in the future.
Resolves:
https://fedorahosted.org/sssd/ticket/
---
Makefile.am | 2 +
src/responder/common/responder.h| 25 ++--
src/responder/common/responder_common.c | 50 +++-
src/responder/pam/pamsrv_cmd.c | 23 +--
src/util/util_selinux.h | 68 +
5 files changed, 133 insertions(+), 35 deletions(-)
create mode 100644 src/util/util_selinux.h
diff --git a/Makefile.am b/Makefile.am
index 407053b1a6dcd0255be76ae7f9252a671b965ea2..2a02add0dc1942c57dec03f4762444c48a710a10 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -496,6 +496,7 @@ SSSD_LIBS = \
$(COLLECTION_LIBS) \
$(DHASH_LIBS) \
$(OPENLDAP_LIBS) \
+$(SELINUX_LIBS) \
$(TDB_LIBS)
PYTHON_BINDINGS_LIBS = \
@@ -556,6 +557,7 @@ dist_noinst_HEADERS = \
src/util/authtok-utils.h \
src/util/util_safealign.h \
src/util/util_sss_idmap.h \
+src/util/util_selinux.h \
src/monitor/monitor.h \
src/monitor/monitor_interfaces.h \
src/responder/common/responder.h \
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 6b511368c9b08d1cfc828d16f57a2cde902dc82b..4735eb7af7d65c1e822d662e7200a8a7418e191e 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -36,6 +36,7 @@
#include "sbus/sssd_dbus.h"
#include "responder/common/negcache.h"
#include "sss_client/sss_cli.h"
+#include "util/util_selinux.h"
extern hash_table_t *dp_requests;
@@ -123,6 +124,21 @@ struct resp_ctx {
bool shutting_down;
};
+#ifdef HAVE_UCRED
+typedef struct ucred UCRED_CTX;
+#define UCRED_get_uid(x) x.uid
+
+#else /* not HAVE_UCRED */
+struct ucred_ctx { int none; };
+typedef struct ucred_ctx UCRED_CTX;
+#define UCRED_get_uid(x) -1
+#endif /* done HAVE_UCRED */
+
+struct cli_creds {
+UCRED_CTX ucred;
+SELINUX_CTX selinux_ctx;
+};
+
struct cli_ctx {
struct tevent_context *ev;
struct resp_ctx *rctx;
@@ -131,9 +147,8 @@ struct cli_ctx {
tevent_fd_handler_t cfd_handler;
struct sockaddr_un addr;
int priv;
-int32_t client_euid;
-int32_t client_egid;
-int32_t client_pid;
+
+struct cli_creds creds;
void *protocol_ctx;
void *state_ctx;
@@ -331,7 +346,9 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const char *csv_string,
bool allow_sss_loop,
size_t *_uid_count, uid_t **_uids);
-errno_t check_allowed_uids(uid_t uid, size_t allowed_uids_count,
+uid_t client_euid(struct cli_creds *creds);
+errno_t check_allowed_uids(struct cli_creds *creds,
+ size_t allowed_uids_count,
uid_t *allowed_uids);
struct tevent_req *
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index fecb72384786ea2840ed349a6afcb36efc60d492..1417c600e35afab6298109d8be6b6dc14b4b51eb 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -88,16 +88,17 @@ static int client_destructor(struct cli_ctx *ctx)
static errno_t get_client_cred(struct cli_ctx *cctx)
{
-cctx->client_euid = -1;
-cctx->client_egid = -1;
-cctx->client_pid = -1;
+SEC_CTX secctx;
+int ret;
#ifdef HAVE_UCRED
-int ret;
-struct ucred client_cred;
-socklen_t client_cred_len = sizeof(client_cred);
+socklen_t client_cred_len = sizeof(struct ucred);
-ret = getsockopt(cctx->cfd, SOL_SOCKET, SO_PEERCRED, _cred,
+cctx->creds.ucred.uid = -1;
+cctx->creds.ucred.gid = -1;
+cctx->creds.ucred.pid = -1;
+
+ret = getsockopt(cctx->cfd, SOL_SOCKET, SO_PEERCRED, >creds.ucred,
_cred_len);
if (ret != EOK) {
ret = errno;
@@ -111,18 +112,31 @@ static errno_t get_client_cred(struct cli_ctx *cctx)
return ENOMSG;
}
-cctx->client_euid = client_cred.uid;
-cctx->client_egid = client_cred.gid;
-cctx->client_pid = client_cred.pid;
-
DEBUG(SSSDBG_TRACE_ALL, &q
[SSSD] Re: [PATCHES] UTIL: Provide varargs version of debug_fn
On Fri, 2016-01-15 at 12:44 +0100, Lukas Slebodnik wrote: > On (15/01/16 12:03), Pavel Březina wrote: > >On 01/12/2016 10:15 AM, Lukas Slebodnik wrote: > >>ehlo, > >> > >>The main reason for these patch was to improve > >>recently added logging to hbac. > >> > >>Side effect of these change is improvement for libldb > >>and libsemanage (6th patch) > >> > >>4th patch is not API/ABI change because > >>such version has not beeen released yet. > >>If you do not like change in hbac callback > >>hbac_debug_fn_t then we should also remove > >>because it is too internal then we should > >>remove also the first two arguments. > >>"file", "line" also leaks internal data from libhbac. > >>Removing the first two arguments would be almost > >>consistent callbacks in libldb and libsemanage. > >> > >>LS > > > >Hi, > >I'm getting following errors: > > > >In file included from /home/pbrezina/workspace/sssd/src/python/pyhbac.c:26:0: > >/home/pbrezina/workspace/sssd/src/providers/ipa/ipa_hbac.h:54:0: error: > >"SSS_ATTRIBUTE_PRINTF" redefined [-Werror] > > #define SSS_ATTRIBUTE_PRINTF(a1, a2) __attribute__((format(printf, a1, a2))) > > ^ > >In file included from /home/pbrezina/workspace/sssd/src/python/pyhbac.c:24:0: > >/home/pbrezina/workspace/sssd/src/util/util.h:62:0: note: this is the > >location of the previous definition > > #define SSS_ATTRIBUTE_PRINTF(a1, a2) __attribute__ ((format (printf, a1, > >a2))) > > > I might add another #ifdef guard but I decided to rename macro > SSS_ATTRIBUTE_PRINTF -> HBAC_ATTRIBUTE_PRINTF Why is debug_fn being called directly in hbac/semanage/etc.. insted of being called through a common macro ? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] fix account lockout reporting with the krb5 provider
On Thu, 2016-01-14 at 12:41 +0100, Jakub Hrozek wrote: > On Wed, Jan 13, 2016 at 02:56:25PM -0500, Simo Sorce wrote: > > subj says it all, > > bug: https://fedorahosted.org/sssd/ticket/2924 > > > > I have compiled and run make check|intgcheck but "not" actively tested > > this patch. > > I did test the patch by crating an account in AD and then ticking the > "Account is disabled" box. PAM returned error code 6, which is what I'd > expect. > > ACK Did the old code return the wrong error too ? I haven't looked at the AD case only IPA when I coded it but AD and IPA share the krb5 provider so I guess testing either would be fine ... Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] fix account lockout reporting with the krb5 provider
On Thu, 2016-01-14 at 17:30 +0100, Jakub Hrozek wrote: > On Thu, Jan 14, 2016 at 11:03:51AM -0500, Simo Sorce wrote: > > On Thu, 2016-01-14 at 12:41 +0100, Jakub Hrozek wrote: > > > On Wed, Jan 13, 2016 at 02:56:25PM -0500, Simo Sorce wrote: > > > > subj says it all, > > > > bug: https://fedorahosted.org/sssd/ticket/2924 > > > > > > > > I have compiled and run make check|intgcheck but "not" actively tested > > > > this patch. > > > > > > I did test the patch by crating an account in AD and then ticking the > > > "Account is disabled" box. PAM returned error code 6, which is what I'd > > > expect. > > > > > > ACK > > > > Did the old code return the wrong error too ? > > Yes, before the patch I see: > Jan 14 16:23:51 adclient.win.trust.test su[3745]: pam_sss(su-l:auth): > received for user lockuser: 13 (User account has expired) > > With the patched code I see: > Jan 14 16:26:13 adclient.win.trust.test su[27524]: pam_sss(su-l:auth): > received for user lockuser: 6 (Permission denied) > > > I haven't looked at the AD case only IPA when I coded it but AD and IPA > > share the krb5 provider so I guess testing either would be fine ... > > Yes, the PAM error codes are exactly the same after "ipa user-disable". > > OK to push now? Yes please :-) Simo -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCH] fix account lockout reporting with the krb5 provider
subj says it all,
bug: https://fedorahosted.org/sssd/ticket/2924
I have compiled and run make check|intgcheck but "not" actively tested
this patch.
HTH,
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 73bc4d73e84c298de94dd269039310a87305fe5c Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Wed, 13 Jan 2016 14:34:33 -0500
Subject: [PATCH] Krb5/PAM: Fix account lockout error handling
The krb5 provider was mapping KRB5KDC_ERR_CLIENT_REVOKED as
ERR_ACCOUNT_EXPIRED. This is incorrect as KRB5KDC_ERR_CLIENT_REVOKED is
returned by the KDC when an account lockout is in effect. When an account is
expired the kdc returns KRB5KDC_ERR_NAME_EXP.
Fix the mapping by adding a new ERR_ACCOUNT_LOCKOUT sssd_error code.
Resolves:
https://fedorahosted.org/sssd/ticket/2924
---
src/providers/krb5/krb5_auth.c | 6 ++
src/providers/krb5/krb5_child.c | 3 +++
src/util/util_errors.c | 1 +
src/util/util_errors.h | 1 +
4 files changed, 11 insertions(+)
diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
index 7657b4ded69351e2caa4c2554d8a62e504b94f34..f69245efdbac2e771e09c35706cd96552de8375d 100644
--- a/src/providers/krb5/krb5_auth.c
+++ b/src/providers/krb5/krb5_auth.c
@@ -1006,6 +1006,12 @@ static void krb5_auth_done(struct tevent_req *subreq)
ret = EOK;
goto done;
+case ERR_ACCOUNT_LOCKED:
+state->pam_status = PAM_PERM_DENIED;
+state->dp_err = DP_ERR_OK;
+ret = EOK;
+goto done;
+
case ERR_NO_CREDS:
state->pam_status = PAM_CRED_UNAVAIL;
state->dp_err = DP_ERR_OK;
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index c8b8a00533c3d4f1ea9fa4c90122708cf61a1820..12eb9e2093d2bdd7d67e8d029fec1455488aa67c 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -1316,6 +1316,9 @@ static errno_t map_krb5_error(krb5_error_code kerr)
return ERR_NETWORK_IO;
case KRB5KDC_ERR_CLIENT_REVOKED:
+return ERR_ACCOUNT_LOCKED;
+
+case KRB5KDC_ERR_NAME_EXP:
return ERR_ACCOUNT_EXPIRED;
case KRB5KDC_ERR_KEY_EXP:
diff --git a/src/util/util_errors.c b/src/util/util_errors.c
index ed19346d9b588a711367af4c891b1298cd4f067e..e7f30ab3c982ebca32844ec3755a69bc00f85f4f 100644
--- a/src/util/util_errors.c
+++ b/src/util/util_errors.c
@@ -82,6 +82,7 @@ struct err_string error_to_str[] = {
{ "Address family not supported" }, /* ERR_ADDR_FAMILY_NOT_SUPPORTED */
{ "Message sender is the bus" }, /* ERR_SBUS_SENDER_BUS */
{ "Subdomain is inactive" }, /* ERR_SUBDOM_INACTIVE */
+{ "Account is locked" }, /* ERR_ACCOUNT_LOCKED */
{ "ERR_LAST" } /* ERR_LAST */
};
diff --git a/src/util/util_errors.h b/src/util/util_errors.h
index c1d081912a382d645c27809a3ac336ff90047cdf..a1c822c4b03817cb8ebf32a9adeb657de57c9de6 100644
--- a/src/util/util_errors.h
+++ b/src/util/util_errors.h
@@ -104,6 +104,7 @@ enum sssd_errors {
ERR_ADDR_FAMILY_NOT_SUPPORTED,
ERR_SBUS_SENDER_BUS,
ERR_SUBDOM_INACTIVE,
+ERR_ACCOUNT_LOCKED,
ERR_LAST/* ALWAYS LAST */
};
--
2.5.0
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [PATCHES] Replace monitor pings with in process watchdog
The attached patches implement a service watchdog based on timers and a
custom SIGRT signal (of which there are 30/32 available to use) and
removes the ping based solution.
In case a child gets stuck in a tevent loop the timer will eventually
kill it (in 30 sec. by default) and the monitor will catch the child has
terminated (via SGICHLD) and restart it. This makes the ping based
infrastructure obsolet so the monitor now stops setting it up.
In order to avoid changes to the dbus interface the ping method is still
in places for responders/providers, but simply never invoked.
Resolves:
https://fedorahosted.org/sssd/ticket/2921
--
Simo Sorce * Red Hat, Inc * New York
From 8820926905b9bfb188b6be6766e932be49aa3e0b Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Wed, 13 Jan 2016 11:51:09 -0500
Subject: [PATCH 3/3] Monitor: Remove ping infrastructure
Now thast services use an internal watchdog we do not need pings anymore,
this will cut down the chatter and allow more flexible process management,
for example socket activation and exit-on-idle.
Resolves:
https://fedorahosted.org/sssd/ticket/2921
---
src/config/SSSDConfig/__init__.py.in | 2 +-
src/monitor/monitor.c| 231 +--
src/monitor/monitor_iface.xml| 2 +-
3 files changed, 8 insertions(+), 227 deletions(-)
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
index fe2971d993ccf2975bbddbe6c74572ab9f5d4e51..05b6f529479a0787f9fb685ac3897deabecb9e88 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -44,7 +44,7 @@ option_strings = {
'debug_timestamps' : _('Include timestamps in debug logs'),
'debug_microseconds' : _('Include microseconds in timestamps in debug logs'),
'debug_to_files' : _('Write debug messages to logfiles'),
-'timeout' : _('Ping timeout before restarting service'),
+'timeout' : _('Watchdog timeout before restarting service'),
'force_timeout' : _('Timeout between three failed ping checks and forcibly killing the service'),
'command' : _('Command to start service'),
'reconnection_retries' : _('Number of times to attempt connection to Data Providers'),
diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index ac3af282d82d79a046fe0a9227a3cd14946ac595..6a0707f0b8f5886d50080074f4faccd10d89af80 100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -55,9 +55,6 @@
#include
#endif
-/* ping time cannot be less then once every few seconds or the
- * monitor will get crazy hammering children with messages */
-#define MONITOR_DEF_PING_TIME 10
/* terminate the child after this interval by default if it
* doesn't shutdown on receiving SIGTERM */
#define MONITOR_DEF_FORCE_TIME 60
@@ -117,7 +114,6 @@ struct mt_svc {
pid_t pid;
char *diag_cmd;
-int ping_time;
int kill_time;
struct tevent_timer *kill_timer;
@@ -126,13 +122,10 @@ struct mt_svc {
int restarts;
time_t last_restart;
-int failed_pongs;
DBusPendingCall *pending;
int debug_level;
-struct tevent_timer *ping_ev;
-
struct sss_child_ctx *child_ctx;
};
@@ -183,11 +176,8 @@ static int start_service(struct mt_svc *mt_svc);
static int monitor_service_init(struct sbus_connection *conn, void *data);
-static int service_send_ping(struct mt_svc *svc);
static int service_signal_reset_offline(struct mt_svc *svc);
-static void ping_check(DBusPendingCall *pending, void *data);
-static void set_tasks_checker(struct mt_svc *srv);
static int monitor_kill_service (struct mt_svc *svc);
static int get_service_config(struct mt_ctx *ctx, const char *name,
@@ -337,7 +327,7 @@ static int svc_destructor(void *mem)
DLIST_REMOVE(svc->mt_ctx->svc_list, svc);
}
-/* Cancel any pending pings */
+/* Cancel any pending calls */
if (svc->pending) {
dbus_pending_call_cancel(svc->pending);
}
@@ -700,67 +690,6 @@ static int monitor_dbus_init(struct mt_ctx *ctx)
return ret;
}
-static void tasks_check_handler(struct tevent_context *ev,
-struct tevent_timer *te,
-struct timeval t, void *ptr)
-{
-struct mt_svc *svc = talloc_get_type(ptr, struct mt_svc);
-int ret;
-
-ret = service_send_ping(svc);
-switch (ret) {
-case EOK:
-/* all fine */
-break;
-
-case ENXIO:
-DEBUG(SSSDBG_CRIT_FAILURE,
- "Child (%s) not responding! (yet)\n", svc->name);
-break;
-
-default:
-/* TODO: should we tear it down ? */
-DEBUG(SSSDBG_CRIT_FAILURE,
- "Sending a message to service (%s) failed!!\n", svc->name);
-break;
-}
-
-if (svc->failed_pongs >= 3) {
-/* too long since we last heard of this process */
-DEBUG(SSSDBG_CRIT_FAILURE,
- "Killing service
[SSSD] Re: [PATCH] Make responder connectin code more generic
On Tue, 2016-01-12 at 14:04 +0100, Jakub Hrozek wrote:
> On Mon, Jan 11, 2016 at 01:39:33PM -0500, Simo Sorce wrote:
> > The following 2 patches change the connection setup code to be more
> > flexible.
> >
> > They are the groundwork to add a new secrets[1] responder that uses a
> > REST API over a unix socket and therefore requires a different protocol
> > handler.
> >
> > The first patch move some protocol and state data into opaque pointers
> > so that responders can arbitrarily set them.
> > Ticket: https://fedorahosted.org/sssd/ticket/2918
> >
> > The second patch adds the ability to use socket activation for
> > responders. Although this is not currently wired in for use in
> > currently.
> > It is related to: https://fedorahosted.org/sssd/ticket/2243
> >
> > These patches are being worked on on my secsrv branch here:
> > https://fedorapeople.org/cgit/simo/public_git/sssd.git/log/?h=secsrv
> >
> >
> > [1] https://fedorahosted.org/sssd/wiki/DesignDocs/SecretsService
> >
> > --
> > Simo Sorce * Red Hat, Inc * New York
>
> > From e82a6c4fc5bd0c6dc264961c5b0fa4f97372e984 Mon Sep 17 00:00:00 2001
> > From: Simo Sorce <s...@redhat.com>
> > Date: Fri, 8 Jan 2016 17:51:06 -0500
> > Subject: [PATCH] Responders: Make the client context more generic
> >
> > This is useufl to allow reusing the responder code with other protocols.
> > Store protocol data and responder state data behind opaque pointers and
> > use tallog_get_type to check they are of the right type.
> >
> > This also allows to store per responder state_ctx so that, for example,
> > the autofs responder does not have to carry useless variables used only
> > by the nss responder.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2918
>
> There are some new Coverity warnings with this code:
> Error: NEGATIVE_RETURNS (CWE-394):
> sssd-1.13.90/src/responder/common/responder_common.c:756: var_tested_neg:
> Assigning: "rctx->lfd" = a negative value.
> sssd-1.13.90/src/responder/common/responder_common.c:790: negative_returns:
> "rctx->lfd" is passed to a parameter that cannot be negative.
> sssd-1.13.90/src/responder/common/responder_common.c:745:5:
> neg_sink_parm_call: Passing "rctx->lfd" to "close", which cannot accept a
> negative number.
> # 788| #endif
> # 789|
> # 790|-> ret = set_unix_socket(rctx, conn_setup);
> # 791| if (ret != EOK) {
> # 792| DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> sockets\n");
>
> Error: NEGATIVE_RETURNS (CWE-394):
> sssd-1.13.90/src/responder/common/responder_common.c:757: var_tested_neg:
> Assigning: "rctx->priv_lfd" = a negative value.
> sssd-1.13.90/src/responder/common/responder_common.c:790: negative_returns:
> "rctx->priv_lfd" is passed to a parameter that cannot be negative.
> sssd-1.13.90/src/responder/common/responder_common.c:746:5:
> neg_sink_parm_call: Passing "rctx->priv_lfd" to "close", which cannot accept
> a negative number.
> # 788| #endif
> # 789|
> # 790|-> ret = set_unix_socket(rctx, conn_setup);
> # 791| if (ret != EOK) {
> # 792| DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> sockets\n");
>
> But in general it looks good to me. I need to run more tests, though..
I'll check these out.
> > From 796ad20628994e671b9af903ed396eb8103d18c4 Mon Sep 17 00:00:00 2001
> > From: Simo Sorce <s...@redhat.com>
> > Date: Thu, 7 Jan 2016 10:17:38 -0500
> > Subject: [PATCH 2/2] Responders: Add support for socket activation
> >
> > Add helper that uses systemd socket activation if available to accept a
> > pre-listining socket at startup.
> >
> > Related:
> > https://fedorahosted.org/sssd/ticket/2913
>
> [...]
>
> > +ret = sd_listen_fds(1);
> > +if (ret < 0) {
> > +DEBUG(SSSDBG_MINOR_FAILURE,
> > + "Unexpected error probing for active sockets. "
> > + "Will proceed with no sockets. [Error %d (%s)]\n",
> > + -ret, sss_strerror(-ret));
> > +} else if (ret > numfds) {
> > +DEBUG(SSSDBG_FATAL_FAILURE,
> > + "Too many activated sockets have been found, "
> > + "expected %d, found %d\n", numfds, ret);
> > +ret = E2BIG;
> > +goto done;
> > +}
> > +
> > +if (ret == numfds) {
>
> Is this exact comparison correct? If w
[SSSD]Re: about fedorahosted-to-github mirror
On Thu, 2015-12-03 at 21:00 +0100, Jakub Hrozek wrote: > Hi, > > I was looking at options we have for setting up an automated way to > mirror our fedorahosted.org repo to github.com. Unfortunately, the > github mirror functionality seems to be discontinued[*], so the next > best thing to do is to set up a github deploy key: > https://developer.github.com/guides/managing-deploy-keys/#deploy-keys > > The private key would be on the machine we'd mirror from, the public key > would be uploaded to github. My question is -- do we want to set up the > push job on fedorahosted.org or one of our machines? > > 1) fedorahosted.org > [+] We don't have to manage the machine, dedicated admins do > [-] We'd have to give read ACL to an identity that pushes /all/ > fedorahosted.org projects. I do not see why the above is a minus, isn't the repo already readable by anyone ? > 2) Our own (CI?) machines > [+] We manage the machine with the private key. We keep control of the > key. > [-] We manage the machine with the private key. We're developers, not > admins. > > I would personally prefer 1) because if the git user on fedorahosted is > compromised, all bets are off anyway and the concern about a push key to > our /mirror/ repo would not be the primary one. But at the same time, I > don't feel comfortable doing the decision without asking the > list. > > So -- is anyone opposed to me asking fedorahosted.org to generate a keypair > and giving us the public key that I would upload to github? Once you have a mirror there have you made any determination about how to deal with PRs ? I assume you disable the issue tracker ? Simo. > Thanks! > > > > [*] github has gained enough traction already, so they don't care about > this functionality anymore.. They start to become hostile to "competition" I guess... not a good sign, oh well. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
Re: [SSSD] [PATCH] confdb: Remove unused function confdb_get_long
On 29/09/15 08:21, Lukas Slebodnik wrote: [root@unused-4-233 ~]# objdump -T /usr/lib64/sssd/libsss_util.so | grep confdb This library is considered (or has been till I was more involved with the code) private to SSSD, with no stability or ABI guarantees, and with updates that go in lockstep with the main binaries. I do not think it would serve any useful purpose to make internal interfaces public, or commit to ABI stability for them, they are supposed to be easy to change to adapt to new needs and other internal changes. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] Code style -- for loop iterative variables initial declaration
On Fri, 2015-08-28 at 09:03 +0200, Petr Cech wrote: Hi everyone, I would like to ask you what you think about the initialization of iterative variables in forloops. I know that present code style does not allow it. But how I recognized, we use C99, and this feature is here now. (example) Instead of:| |||# inti; # for(i =0;...)||| we could write: ||# for(inti =0;...)| I see an advantage in limiting the validity of such variables. That means higher code readability. Disadvantages I searched but did not find. I think it is ok to accept this style when there is *only one* variable involved and the iterator variable is useful exclusively within the for loop. I used this style elsewhere and it was ok. Much care need to be taken when *converting* code to this style, because in many case the index is used after the for loop to check for failure/success when the loop breaks in the middle. Simo. P.S: please do not use HTML emails, see how butchered your email comes out in the txt version. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] Embedding Lua into SSSD
On Wed, 2015-08-26 at 16:10 +0300, Nikolai Kondrashov wrote: On 08/25/2015 10:19 PM, Simo Sorce wrote: On Tue, 2015-08-25 at 21:26 +0300, Nikolai Kondrashov wrote: On 08/25/2015 08:48 PM, Simo Sorce wrote: On Tue, 2015-08-25 at 16:14 +0300, Nikolai Kondrashov wrote: * automatic memory management (incremental mark-and-sweep with good controls, customizable allocator) This is not really interesting as we already have talloc that does a great job at keeping track of memory allocations and cleanups (esp if we stop using the anti-pattern of allocating on NULL as discussed a few times already). You still need to do it explicitly and think about it. Not that much, of course, and it's not that you don't have to think about it with a mark-and-sweep GC at all. I think a GC would be a regression to be honest,for the usage pattern of SSSD a hierarchical allocator makes better use of memory. I agree, memory can be used more efficiently with a hierarchical allocator. However, is saving memory a priority over lowering complexity for providers? It is important, yes, because this is a system service that always runs on the machine. We cannot explode the memory requirements or it will negatively affect the whole system, we are already borderline for some use cases. Still, in the majority of cases you don't have to think about it, and you certainly don't have to assign memory to contexts and keep them, nor pass them around. True, but then you do not ever think of memory allocation with usually comical situations where you leak tremendous amounts of memory because you keep some reference mistakenly. Once that happen finding out what is happening and reviewing every single variable allocation to find out what is going on is generally extremely hard. It's a trade off really. Agree. Doesn't happen often, though. In fact, I can't remember it happening to me. I had some difficulties bridging Lua and a C library (ClearSilver) memory management, but it wasn't Lua's fault and it worked in the end. * brevity (implicit dynamic typing, syntax sugar) brevity is good for long-winded functions indeed. Well, it kind of works all through the code. Sometimes the brevity of scripting languages is obtained through extremely inefficient code, with potentially unwanted side-effect (esp wrt memory management). So it is not better in general, it is better for the specific goal of making long functions more readable. Sure. We can only consider it better or worse in isolation from other factors. * OOP support (there are several libraries to choose from) This may be good or bad, it all depends on what you need to do. Sure. Although having something is better than not having, theoretically. Except diseases, of course :) As you see, sometimes having something is worse. For software that works at lower levels OOP usually makes for software that is harder to debug, so it may be an anti-feature. Sure. OOP might only be useful for higher-level logic, but I don't suggest we use it otherwise. It was just a point that might help us. How does Lua work with asynchronous operations ? We have a ton of that and unless Lua has native support it would be a huge mismatch. Now this is a good question. It depends on what you mean. If you mean central loop with state machines underneath, then coroutines is an awesome solution. It is not clear to me that Lua's coroutines (http://www.lua.org/pil/9.1.html) would do what asynchronous tevents do. I'll try marrying them when I have time and we'll see. If you mean OS threads, then there are several libraries that support it, AFAIK. I haven't looked into them though. No, I do not mean threads, in fact sssd is not thread safe. I mean I/O based events where a socket file becoming readable makes the chain of nested events keep going on until the innnermost completes it's work and data is returned (potentially massaged all the way back) to the outermost event. With control returning to the main loop every time an I/O operation would block. Ah, good, I was worried we might need threads. We'll have to try it to really know. If you can give me an example, I can try thinking about it. Look at how tevent_req stuff works / see above. I will, thank you. I don't know enough JavaScript to really compare, but to me knowing that it has two pairs of equality operators [1] is quite off-putting. I hope it doesn't compare much because javascript is awful, especially for anything security oriented. Now, this is interesting. Care to elaborate? Is there anything really in the language itself and not the browser parts? Not that I like JavaScript myself, just curious. The browsers just implement the language, which is quite awful. Well the fact that a browser would implement
Re: [SSSD] Embedding Lua into SSSD
On Tue, 2015-08-25 at 16:14 +0300, Nikolai Kondrashov wrote: Hi Pavel, On 08/24/2015 11:54 AM, Pavel Březina wrote: On 08/21/2015 07:01 PM, Nikolai Kondrashov wrote: I might be in a strange and careless mood today, but here is something I wanted to suggest since the time I saw the amount of logic that goes into SSSD and is implemented in C. What if we implement some of the complicated logic inside SSSD in Lua [1]? can you tell us what features of Lua do you like and might help simplify SSSD? I don't know the language, just fast scrolled over the manual and seen few examples. It seems to be similar to javascript. Most of all, I like its simplicity, then flexibility. And then how easy it is to embed. The latter comes from simplicity, first of all. I listed the features I think will help us in the other message in this thread, but here they are again and then some: * exception handling (somewhat rudimentary, but workable) This may be interesting, but we are pretty diligent I think, thanks to the tevent_req design pattern, at surfacing error * automatic memory management (incremental mark-and-sweep with good controls, customizable allocator) This is not really interesting as we already have talloc that does a great job at keeping track of memory allocations and cleanups (esp if we stop using the anti-pattern of allocating on NULL as discussed a few times already). * easier data massaging (string operations, built-in (limited) regexes, etc.) We do some limited string manipulation but not really much, what other kind of data massaging does Lua provide that makes it worth ? * brevity (implicit dynamic typing, syntax sugar) brevity is good for long-winded functions indeed. * OOP support (there are several libraries to choose from) This may be good or bad, it all depends on what you need to do. How does Lua work with asynchronous operations ? We have a ton of that and unless Lua has native support it would be a huge mismatch. I don't know enough JavaScript to really compare, but to me knowing that it has two pairs of equality operators [1] is quite off-putting. I hope it doesn't compare much because javascript is awful, especially for anything security oriented. There is, however, a seemingly good comparison, which should give the general idea: http://stackoverflow.com/a/1022683/1161045 To be honest a Javascript to Lua comparison doesn't really help much understanding if Lua could be an asset for SSSD or not. See above questions. Simo. Nick [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference#Equality_operators ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Switch ldap_user_certificate default to userCertificate; binary
On Fri, 2015-08-21 at 09:50 +0200, Jan Cholasta wrote: Hi, On 10.8.2015 12:59, Jakub Hrozek wrote: Hi, the attached patches fix #2742. The first one makes sure we can print the certificate (or any binary attribute, really) safely. We only need to make sure to escape the attribute values before saving them to sysdb, because then ldb guarantees terminating them. The second just switches the attribute value. I tested using this howto: http://www.freeipa.org/page/V4/User_Certificates#How_to_Test You'll also want to use a recent enough IPA version, one that fixes: https://fedorahosted.org/freeipa/ticket/5173 Then, on the client, call: dbus-send --print-reply \ --system \ --dest=org.freedesktop.sssd.infopipe \ /org/freedesktop/sssd/infopipe/Users \ org.freedesktop.sssd.infopipe.Users.FindByCertificate \ string:$( openssl x509 cert.pem ) The result will be an object path. LGTM, but I would think userCertificate;binary should be the default everywhere, i.e. generic LDAP, as that is the correct attribute name according to RFC 4523. IMHO when someone uses the standard name in generic LDAP, they should not be forced to change SSSD configuration because of it. +1 Simo -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] Embedding Lua into SSSD
On Fri, 2015-08-21 at 20:01 +0300, Nikolai Kondrashov wrote: Hi everyone, I might be in a strange and careless mood today, but here is something I wanted to suggest since the time I saw the amount of logic that goes into SSSD and is implemented in C. What if we implement some of the complicated logic inside SSSD in Lua [1]? Lua is a language specifically designed to be embedded. It grew into popularity after it was embedded into World Of Warcraft. It was also embedded and is still being embedded into a bunch of other games. However, games is far from the only application for Lua. Lua is a simple and lightweight language, yet with lots of flexibility - grasping the essence of the whole reference manual [2] takes one evening and it's an entertaining read. The VM is small and fast. In fact some people use it on microcontrollers [3]. I personally embedded it into two C-based projects, one of which was already quite big, and another was a rewrite of a smallish, but complicated C project. In both cases, where before things were tedious and hard, they have become easy and fun. The first project was on an embedded platform. I also had some other, but smaller stuff done with it. Generally, Lua gets put into the middle with outside interfaces and performance-critical parts implemented in C. With a very simple interface to and from C one can put boundaries wherever necessary. Yes, the interface is simpler than Python's. The process is this: you implement some C libraries (shared/static) which are loadable from Lua (Lua-C interface) and then create a VM or several, load some Lua code and call into it (C-Lua interface). Some of the benefits are: Clearer logic (higher level language) Much less memory leaks and very little memory management (mostly in Lua-C interface). Faster development cycle (no compilation, edit and retry on a live daemon) Potentially easy plugin interface (yay for admin scripts!) Some of the drawbacks are: More difficult to do low level stuff (so we just don't do that) Lower performance than C (still good enough for games, and we can have the critical parts in C easily) Needs learning and development investment. Regarding the development investment, due to low cost of C/Lua interface implementation, it is practical to replace the relevant C code piece-by-piece, provided pieces are not too small (say, 2KLOC and up). Lastly, don't take this too seriously. I realise that this is disruptive and hard. However if you ever feel like SSSD has grown too big and complex, ping me, or just try Lua yourself :) Nick [1] http://www.lua.org/ [2] http://www.lua.org/manual/5.3/manual.html [3] http://www.eluaproject.net/ Hi Nick, can you provide an example of a piece of SSSD youd replace with Lua ? I am not asking for an implementation but a high level view of what a function looks like to day and what it would look like if we were going to use Lua (ideally with a list of primitives we'd still need to provide, to understand how much code Lua replaces for real. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Mon, 2015-07-06 at 11:46 +0200, Sumit Bose wrote: On Fri, Jul 03, 2015 at 05:01:13PM -0400, Simo Sorce wrote: On Fri, 2015-07-03 at 21:34 +0200, Sumit Bose wrote: On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: On Fri, 2015-07-03 at 11:59 +0200, Jakub Hrozek wrote: On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote: Hi, the attached patches fix https://fedorahosted.org/sssd/ticket/2701 The first patch just adds a common function instead of copying the same pattern again to the new test. The second adds a new request krb5_auth_queue_send() that wraps krb5_auth_send() and also uses the Kerberos authentication queue. I hope the unit tests cover a lot of use-cases, if not, please suggest more! btw I was thinking that the chaining might not always be necessary if the ccache is of type MEMORY and I hope that the serializaton wouldn't be perceived as performance regression for users. Shall we say that Pavel's cached auth patches are a more systematic solution that doesn't rely on properties of the ccache type in that case? I'm sorry, but CI fails on Debian because of wrong linking with libraries. I'm already testing a fix. Review of the rest is appreciated :-) If we are serializing all authentications then a busy server would be swamped. I do not see a per-user/per-cache queue so I would tentatively NACK the approach sorry. The current cache queue is per user, see add_to_wait_queue() for details. I actually did look to check that and failed, I blame the late hour :-) Per-user is fine by me, though I would seriously consider de-duplication while we are here. I think this might be a useful feature, although I think it should not be on by default. We have discussed this already a bit 5 years ago together with the initial implementation of the auth queues in the krb5 responder. Here are some links for reference: https://fedorahosted.org/sssd/ticket/533 https://lists.fedorahosted.org/pipermail/sssd-devel/2010-October/004821.html https://lists.fedorahosted.org/pipermail/sssd-devel/2010-December/005307.html But I think this feature would need some more discussion. E.g. where should it be implemented, in the PAM responder on in the auth backends? auth backend, the pam responder may not have enough context to properly decide. How does is related to the cached authentication feature? It doesn't relate in any way IMO. How should failed authentication be handled, de-dup as well or send them to the backend to allow the server to update the failed logon counter properly? The backend de-dups them and you won't know if an authentication is successful or failed until the answer comes back, so de-duping has already happened. Shall de-duplication be done by user or by user and by service? This is a good question, I think it makes sense to have it per-user, however you must always check that everything is the same, user, password, any other option. Reading #533 I think the original concern about counting failures is a little bit misguided. The reason why we have unsuccessful counts is to avoid password guessing where an attacker tries a different password at every attempt. It is not a measure to somehow punish the user for sending the wrong password. So if an attacker where to send 100 auth requests all with the same password it makes no sense to lock up the user account, they SHOULD, ideally, count as a single failure. And actually de-duplication may end up saving a user with scripts. Think of a situation where the user has a script that makes 10 actions that require authentication when he inputs his password. If he enters the wrong password he may lock himself out for X time with just one attempt. With de-dup instead if chewed only one attempt and he can try again (assuming all auths happen at the same time). Given that I would recommend to commit the patches in the current state and add a RFE ticket to discussion de-duplication for one of the next releases. Sure, we can open a separate ticket. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, 2015-07-03 at 11:59 +0200, Jakub Hrozek wrote: On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote: Hi, the attached patches fix https://fedorahosted.org/sssd/ticket/2701 The first patch just adds a common function instead of copying the same pattern again to the new test. The second adds a new request krb5_auth_queue_send() that wraps krb5_auth_send() and also uses the Kerberos authentication queue. I hope the unit tests cover a lot of use-cases, if not, please suggest more! btw I was thinking that the chaining might not always be necessary if the ccache is of type MEMORY and I hope that the serializaton wouldn't be perceived as performance regression for users. Shall we say that Pavel's cached auth patches are a more systematic solution that doesn't rely on properties of the ccache type in that case? I'm sorry, but CI fails on Debian because of wrong linking with libraries. I'm already testing a fix. Review of the rest is appreciated :-) If we are serializing all authentications then a busy server would be swamped. I do not see a per-user/per-cache queue so I would tentatively NACK the approach sorry. If clobbering is the only issues we have then what we can do is to always use a custom memory ccache for each auth attempt and then use a locking mechanism to copy from the memory ccache to the actual ccache. But we should really do either per-ccache queuing (maybe not per user as in pathological cases we may have the same ccache for different users ?) or use memory ccaches and copy them with locking, but fully serializing all authentications is not really a solution, when a full auth may require multiple network roundtrips. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, 2015-07-03 at 21:34 +0200, Sumit Bose wrote: On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: On Fri, 2015-07-03 at 11:59 +0200, Jakub Hrozek wrote: On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote: Hi, the attached patches fix https://fedorahosted.org/sssd/ticket/2701 The first patch just adds a common function instead of copying the same pattern again to the new test. The second adds a new request krb5_auth_queue_send() that wraps krb5_auth_send() and also uses the Kerberos authentication queue. I hope the unit tests cover a lot of use-cases, if not, please suggest more! btw I was thinking that the chaining might not always be necessary if the ccache is of type MEMORY and I hope that the serializaton wouldn't be perceived as performance regression for users. Shall we say that Pavel's cached auth patches are a more systematic solution that doesn't rely on properties of the ccache type in that case? I'm sorry, but CI fails on Debian because of wrong linking with libraries. I'm already testing a fix. Review of the rest is appreciated :-) If we are serializing all authentications then a busy server would be swamped. I do not see a per-user/per-cache queue so I would tentatively NACK the approach sorry. The current cache queue is per user, see add_to_wait_queue() for details. I actually did look to check that and failed, I blame the late hour :-) Per-user is fine by me, though I would seriously consider de-duplication while we are here. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, 2015-07-03 at 20:33 +0200, Jakub Hrozek wrote: On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: On Fri, 2015-07-03 at 11:59 +0200, Jakub Hrozek wrote: On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote: Hi, the attached patches fix https://fedorahosted.org/sssd/ticket/2701 The first patch just adds a common function instead of copying the same pattern again to the new test. The second adds a new request krb5_auth_queue_send() that wraps krb5_auth_send() and also uses the Kerberos authentication queue. I hope the unit tests cover a lot of use-cases, if not, please suggest more! btw I was thinking that the chaining might not always be necessary if the ccache is of type MEMORY and I hope that the serializaton wouldn't be perceived as performance regression for users. Shall we say that Pavel's cached auth patches are a more systematic solution that doesn't rely on properties of the ccache type in that case? I'm sorry, but CI fails on Debian because of wrong linking with libraries. I'm already testing a fix. Review of the rest is appreciated :-) If we are serializing all authentications then a busy server would be swamped. I do not see a per-user/per-cache queue so I would tentatively NACK the approach sorry. Thanks for the feedback! I had my doubts about serialization by default as well, but I would like to note that serialization had been the default in the krb5 responder, just not IPA or AD responder since 2010... Please note I'm not bashing the existing code, I think the serialization is the only good approach for old platforms that use FILE: ccache by default..which I guess is why we used it. I'd have no problems bashing the existing code, it is not perfect, but we should use this chance to improve it :-) If clobbering is the only issues we have then what we can do is to always use a custom memory ccache for each auth attempt and then use a locking mechanism to copy from the memory ccache to the actual ccache. This is an issue on old platforms like RHEL-6 where FILE is the only (supported) ccache. So memory cache might not be available.. For better or worse, these platforms are used in critical environments and high-load servers and are hitting the bug. Afaik the Memory ccache is available in all OSs we support including very old RHEL versions. But we should really do either per-ccache queuing Can you elaborate a bit? Are you suggesting to use ccache as the key in the hash table? I would have to think about a bit, but so far it seems like a bit of an overkill, because even now if the same ccache was used for different users we would clobber it anyway I think. Sure, but it is as easy to index it by user name or by ccache, so we can just go by ccache. (maybe not per user as in pathological cases we may have the same ccache for different users ?) or use memory ccaches and copy them with locking Yes, but memory ccache is not available on all platforms.. Which platforms miss it ? I am not aware we support any platform that lack the MEMORY ccache type, but I may be wrong. , but fully serializing all authentications is not really a solution, when a full auth may require multiple network roundtrips. That's why I was suggesting using the cached auth feature as a remedy. It is not a remedy, if you have a few hundred users logging in, in the morning, all at once, there is no reason to serialize all their operations, and serializing them would make some user wait way too much. Caching wouldn't help as there'd be no cache to check. But I do see your point. Would serializing only for FILE: ccache be an accebtable option for you? Something like: No, it would be wrong, DIR or KEYRING ccahes have the same issues IMO. def krb5_queue_auth_send() if ccache_type == FILE: queue() else: send_auth() Then we could document for admins that on busy servers that are unfortunate to only support FILE ccache, use cached auth. If you're running on a recent platform (RHEL-7+), though, serialization is not used. Nope, I think per-user serialization (and actually maybe even auth de-duplication, why not, if user/pw are the same why run the whole stack twice ?) is what we need in all cases. Actually in most cases de-duplication would successfully handle 99.9% of the issues/races. The only case left would be people firing in rapid succession auth attempts with different parameters (which could still happen if we think of Optional 2FA logins, but would be much rarer). Simo. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] Design Discussion: D-Bus Signal: Notify Property Changed
when the monitor starts IMO. You do not want a race where the IFP starts after one of the sssd_be processes and these will never notify. 2. If modified object supports the signal store it in the TDB === Configuration changes === In IFP section: * '''ifp_notification_interval''': period of ''IFP: notify properties changed'', disabled if 0, default 300 (5 minutes) This sounds a bit too long, the changelog should be mostly empty and checking it is a simple r/o operation, any reason why we shouldn't check much more frequently? As indicated before, there may be a cycle but in general I think we should rather use signals to kick the process or notifying the clients. The process ID to signal should be obtained somehow, probably by having the IFP write it down in the TDB file as soon as it is started (or it may be maintained by the monitor in the TDB or other file or even communicated via internal dbus commands, so that the other processes do not have to constantly read the pid file/record every time and rely on the monitor to tell them when the IFP has a new PID). === How To Test === 1. Hook onto ''!PropertiesChanged'' signal, e. g. with ''dbus-monitor'̈́' 2. Trigger change of user/group It should be made clear that the change can be done using /any/ update, ie even getent group updating a user counts. (This is mostly us bragging about how awesome the feature is :-)) 3. Signal should be recieved === Questions === 1. Do we want to use ''changed_properties'' or ''invalidated_properties'' Hmm, both are part of the D-Bus interface itself, right? Do we have a choice? HTH, Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] RFC: Improving the debug messages
On Mon, 2015-06-29 at 18:13 +0200, Jakub Hrozek wrote: Hi, I spent many hours debugging SSSD in different scenarios last week and I admit it wasn't always easy -- and I have the source code knowledge I can use. I imagine it's considerably harder for users and admins.. So this is a brainstorm request on how can we make debugging with SSSD easier. Maybe there are some low-hanging fruits that can be fixed easily. Off the top of my head: - it should be easier to see start and end of a request in the back end. Instead of: [be_get_account_info] (0x0200): Got request for [0x1001][1][name=admin] [acctinfo_callback] (0x0100): Request processed. Returned 0,0,Success (Success) We could make the debug messages more explicit: [be_get_account_info] (0x0200): Received request for [object=user][key=name][value=admin] [acctinfo_callback] (0x0200): Finished request for [object=user][key=name][value=admin]. Returned 0,0,Success Then we could document the messages in our troubleshooting document. Please note I'm not proposing to turn debug messages into any kind of API and keep them the same forever, but decorate the usual flow with messages that make sense without source level knowledge. - same for authentication - same for responder cache requests. We seem to have gotten better with the new cache_req code there, so this is mostly about using the new code in all responders. But also the commands we receive from sockets should be printed in human-readable form. - Running sssd in environment where all actions complete successfully should emit no debug messages. Default log level should be moved to SSSDBG_OP_FAILURE or CRIT_FAILURE. (This basically amounts to checking all OP, FATAL and CRIT failure messages..) The reason is that sometimes sssd fails, but because logging is totally silent, we don't know what happened at all. Currently we have a couple of small bugs where we might print a loud DEBUG message just because we search for an entry which is not there etc. - anything that causes SSSD to fail to start should also emit a syslog message. Admins don't really know about sssd debug logs. - our man pages are not structured well, especially the LDAP man page is too big and contains too many options. One reason I'm bringing this up now is that we'll have a new SSSD developer starting soon and these might be nice tasks to start with AND they're also needed. +1 Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Store the one-way keytabs in /var/lib/sss/keytabs
On Tue, 2015-06-16 at 15:10 +0200, Jakub Hrozek wrote: Proactively store the keytabs in /var/lib/sss/keytabs instead of /var/lib/sss/db/keytabs because users (including developers who rote tests) are used to removing everything under /var/lib/sss/db which removes the sssd-owned directory. Unlike the other directories under /var/lib/sss this one doesn't have a matching configure option...I don't this we need one. Make sure the directory is only accessible to the sssd user. CI (rigorous by default now): http://sssd-ci.duckdns.org/logs/commit/27/df243b8f6182a6093af432f1d23a21e4fb1456/1743/summary.html LGTM Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] Should we have a github r/o mirror for SSSD?
On Mon, 2015-06-15 at 09:32 +0200, Jakub Hrozek wrote: See this message on sssd-users: https://lists.fedorahosted.org/pipermail/sssd-users/2015-June/003078.html For better or worse, many users expect a project (any project..) to be on github or else it doesn't exist. Should we have some kind of automated read-only repository on github.com with readme.md pointing to fedorapeople.org to avoid the confusion? +1 But then you need someone to monitor pull requests. Also can issues be disabled ? They are a terrible medium to report any issue (completely unstructured with replies hard to follow) and we should direct people to trac for real bug reporting. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCHES] krb5: new option krb5_map_user
On Thu, 2015-05-28 at 11:29 +0200, Jakub Hrozek wrote: On Thu, May 28, 2015 at 11:09:50AM +0200, Pavel Reichl wrote: On 05/28/2015 11:04 AM, Jakub Hrozek wrote: On Wed, May 27, 2015 at 09:40:13AM +0200, Jakub Hrozek wrote: On Tue, May 26, 2015 at 04:36:18PM +0200, Pavel Reichl wrote: From 923e68ba56f276db473a38fffe339a0dc9770a4f Mon Sep 17 00:00:00 2001 From: Pavel Reichl prei...@redhat.com Date: Thu, 30 Apr 2015 06:43:05 -0400 Subject: [PATCH] krb5: new option krb5_map_user New option `krb5_map_user` providing mapping of ID provider names to Kerberos principals. Resolves: https://fedorahosted.org/sssd/ticket/2509 [...] + quotejoe/quote and quotedick/quote are + UNIX user names and quotejuser/quote and + quoterichard/quote are primaries of kerberos + principals. For user quotejoe/quote resp. + quotedick/quote SSSD will try to kinit as + quotedick@REALM/quote resp. kinit as juser@REALM right? + quoterichard@REALM/quote. +/para + +para +Default: not set +/para +/listitem +/varlistentry + /variablelist /para /refsect1 But since this is the last nitpick I found (for real this time :-)) I can fix this up locally and push.. Attached is a patch I'm about to push. Man page looks good to me. Sorry for so many mistakes in such a short text. CI link: http://sssd-ci.duckdns.org/logs/job/16/06/summary.html * master: aa8a8318aaa3270e9d9957d0c22dec6342360a37 * sssd-1-12: c494e100f9b2422e2890507f63019afcaff9b7c6 I still think it makes sense to push the patch to sssd-1-12 as well -- it's not too risky and there's quite a few users who'd like to see this feature. I know this has been pushed already, but I am not very happy about this feature. Why is krb5_primary missing the realm part ? We already support trust relationships with kerberos why can't I have: joe:j...@sub1.realm.com, jane:j...@sub2.realm.com ? I think allowing shortcuts may be fine but we should also allow admin to explicitly map to a specific realm. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
