[SSSD] [PATCHES] Add support for multiple servers in the Kerberos locator plugin

Simo Sorce simo at redhat.com
Wed Jul 18 14:11:09 UTC 2012


On Wed, 2012-07-18 at 08:05 -0400, Stephen Gallagher wrote:
> On Tue, 2012-07-17 at 17:46 -0400, Simo Sorce wrote:
> > On Tue, 2012-07-17 at 13:36 -0400, Stephen Gallagher wrote:
> > > This is the first half of the work necessary to resolve
> > > https://fedorahosted.org/sssd/ticket/941
> > > 
> > > This patch largely rewrites the Kerberos locator plugin so that it can
> > > parse kdcinfo files containing multiple servers (one per line, with
> > > optional port). These patches are testable by manually
> > > editing /var/lib/sss/pubconf/kdcinfo.REALM
> > > (or /var/lib/sss/pubconf/kpasswdinfo.REALM, as appropriate). They are
> > > also fully backwards-compatible.
> > > 
> > > I will be working on and sending the KRB5 provider side of things after
> > > this, but since these are ready, manually testable and
> > > backwards-compatible, it seemed prudent to send them for review in
> > > parallel.
> > 
> > Is it forward compatible ?
> > Meaning, if I have an application that already has the old plugin loaded
> > and you upgrade sssd to start generating multiple lines in the file, how
> > will the old app behave ?
> > 
> 
> No, it's not forward-compatible. The old code was VERY finicky. It just
> reads in the entire file, reverse-searches for a colon and assumes that
> the address is everything before that last colon. So it's not going to
> be able to read the new file.
> 
> FWIW, the old version couldn't even handle a trailing newline in the
> file.
> 
> I suppose we could switch to using a new filename (kdcinfo2.REALM for
> instance) and have the Kerberos provider produce both files for a while.
> 
> I don't love that answer, though.
> 
> > > Patch 0001: Convert memory allocation functions to use talloc. There's
> > > no reason to avoid this dependency, and it will make subsequent patches
> > > much simpler.
> > 
> > Nack,
> > this code is loaded by libkrb5 and imported into other applications.
> > Talloc is not thread safe and we do not want to drag this dependency in
> > random applications unless it is fully thread safe.
> > 
> 
> I don't understand. Where do you see a threading issue (that wouldn't be
> present with malloc alone)? We allocate one top-level sssd_ctx as the
> private data of the plugin and everything else as a child of that.
> 
> > > Patch 0002: Create a structure for holding address and port information
> > > (with linked-list semantics)
> > 
> > Do we really need a complex linked list ?
> > How many entries do we expect to have ?
> > Wouldn't a simple array be easier to manage ?
> > 
> 
> Eh, I wanted to be able to handle an arbitrary-length kdcinfo file, but
> I suppose we could get away with a fixed limit of addresses. (I was
> planning to make the number of addresses configurable in the Kerberos
> provider). But picking a reasonable limit (10) and making it an array is
> probably fine.
> 
> > > Patch 0003: Rewrite the kdcinfo parser so that it supports multiple
> > > entries, one per line. It ignores blank lines.
> > > Patch 0004: Modify the sssd_krb5_locator_lookup() function to send
> > > repeated calls to the cbfunc() callback, providing all of the
> > > kdcinfo-specified servers as values. Clients using libkrb5 will
> > > transparently try all of the provided servers until finding one that
> > > works.
> > 
> > Haven't really looked at the rest as the first nack unfortunately means
> > a lot needs to be changed now.
> 
> 
> Please help me understand where you see a thread-safety issue. I'm also
> more inclined to fix the synchronization in the locator than I am to
> back out of using talloc if I can help it.

This is sadly a final NACK.

We discussed this on IRC, and we agreed this approach is a dead end.
There are various reasons.

The concurrency issue comes in in threaded applications where multiple
threads use kerberos at the same time (gss-proxy is one such example).

Another issue is the fact that talloc is still use statically compiled
in some samba binaries and dragging in talloc as a dependency to the
kerberos library (also used by samba) could cause very bad issues if the
2 version do not match precisely.

In general we should try to do as little as possible in the locator
plugin itself as it runs within applications that link libkrb5.

A preliminary consensus seem to be that we should adopt a sss-client
like interface so we can just reuse our client code already used with
nss and pam interfaces there and the memory cache with it, the shared
cache and all the client code is already thread safe and most
importantly it is already well tested as safe and reduces the amount of
memory management and need to parsing to nothing not already
implemented.

Stephen will post later a new design document for this feature.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list