Please see the attached patches. I tried to split the patches logically
into manageable sets.
Unfortunately I made a minor mistake and I am afraid I will do something
wrong to fix it.
I merged two wrong patches. Fortunately it was three liner with 1 liner
so it is not a big of the deal but I am really scared that I will do
something wrong and loose the work I have done.
So I hope it is Ok to send it as is.
0001--INI-Making-Coverity-happy.patch <- this is the patch I submitted
earlier that I merged by mistake. I was supposed to merge it with patch
25 but picked the wrong one instead.
Patch 25 addresses the real issue found by Coverity as mentioned in
Stephen's review mail but it did not apply cleanly since it relies on
some code from the patches in the middle.
0002--INI-Adding-missing-function-declararion.patch <- this is the
patch that was rejected from the second set sent earlier. Fixed
according to review comment.
0003--BUILD-Allow-trace-per-component.patch <- This patch allows tracing
The following set of patches introduces the merging of sections during
the reading of the file:
Patches related porting of the meta data from old way of doing things to
the new way of doing things:
0021--INI-Avoid-double-free.patch <- patch related to 17 (missed check)
0024--INI-Rename-error-print-function.patch <- rename error printing
function for consistency with new interface
0025--INI-Initialize-variables-in-loops.patch <- Coverity issue
addressed. Related to patch 0001.
0026--INI-Exposing-functions.patch <- Make some internal functions reusable
There is also patch 27. It is a piece of new functionality. It is a
preview. Please see the comment before reviewing it.
Do I need to split it into multiple patches or it is Ok as is? It is
pretty big but all changes are in one file and logically related.
The UNIT test is missing so I am not claiming it actually works as
Sr. Engineering Manager IPA project,
Red Hat Inc.
Looking to carve out IT costs?
So here it is, a set of patches splitting file sdap_async_accounts and making
some subsequent changes. If anyone has other ideas what could be changed, let
I was in favor of splitting the file into 3, as Simo suggested:
This way three basic operations are separated, but they are not separated too
much (otherwise a _common.c file would have to be present, since some routines
are shared by code paths in different schemas).
While attempting to extract the SSSD's HBAC rule evaluation into a
common library, I discovered several shortcomings. Some were related to
extracting the library, others were related to the rule evaluation
Related to extracting the library, I needed for the code to be able to
identify and resolve without having direct access to LDAP or the LDB.
This meant I could not rely on the memberOf objects to be valid.
As for the rule evaluation, I discovered that we were only caching for
offline use those remote hosts that had previously attempted to log in.
This meant that if the IPA server was unreachable, valid users would be
unable to log in from some valid machines.
After careful consideration (and a few false starts), I decided that a
total rewrite was necessary. So here it is.
The major changes are these:
1) We have moved to always performing a full enumeration of known IPA
hosts and HBAC services. This actually results in a net reduction in
round-trips to the LDAP server, which was a primary bottleneck.
Additionally, this means that we also have a full understanding when
offline as to whether a remote host is permitted access or not.
2) I've added a configurable timeout for hbac rule refreshes to the LDAP
server. Previously, if we were online, EVERY pam_account() request would
result in at least one LDAP round-trip. By default, we'll only refresh
the HBAC rules every five seconds now.
3) I broke the acquisition and processing of the rules, hosts and
services into separate C files, as ipa_access.c was getting to be much
to large to manage.
The overall view of the code (when online) is this:
A request comes in, we call to LDAP to get, in order, the set of known
hosts, the set of known PAM services and then finally the set of
*enabled* rules with the current ipa_hostname as the targethost.
And now, the patches:
Patch 0001: The new standalone evaluator library. It's very simple at
the moment (but it will become much more complex in the near future when
timerule evaluation is added). Jakub, Rob and I agreed on the interface
and Jakub is creating Python bindings for it.
Patch 0002: This is the meat of the new IPA access backend. All of the
helper functions necessary for looking up, storing and converting the
hosts, rules and services are in this patch.
Patch 0003: This is not really a patch as much as it is a trash can. I
split this from Patch 0004 to make the latter more reviewable. Patch
0003 contains only the deletions of the old access provider
Patch 0004: Adds the new IPA access provider, taking advantage of the
helper functions from Patch 0002.
Patch 0005: Adds a refresh timeout to reduce round-trips to IPA while
the attached patch provides a new python module "pyhbac" that implements
python bindings for the HBAC evaluator library.
The patch depends on Stephen's last patches which are on review as of
now, but the test suite passed, so I think the bindings can be reviewed
"make check" loads the built python module from tree by doing some
sys.path magic. If you'd like to experiment with the module yourself,
you must either install it or set PYTHONPATH to $SSSD_BUILD_DIR/.libs
by chance I realized that an OpenLDAP server does not list all controls
it can handle in the rootDSE attribute supportedControl.
Especially LDAP_CONTROL_PASSWORDPOLICY is not listed. According to the
OpenLDAP developers this is because the related spec
still a draft and not finalized
Since sssd only uses controls which are in the supportedControl list we
will not be able to give the user expiration warnings or information
about grace logins for OpenLDAP servers with the password policy overlay
I'm not sure if we need to do anything about it but at least I think it
is good to be aware of.
this patch adds support for the NDS/eDirectory access control attributes
loginDisabled, loginExpirationTime and loginAllowedTimeMap. It is not
fully tested because currently I do not have access to an eDirectory
server. Since the impact on other parts of the code is minimal I didn't
add a seperate configure option for it but only mark it in the man page
I you have a chance to test this feature please let me know the result.
We will have some experimental features added in the near future and
this patch tries to help to identify them. One part is a new configure
option which enables all experimental features which check if the option
is set. The second is a small include file for man pages to indicate
that a certain feature is experimental.
This patch add the possibility to exclude attributes when building requested
attribute list from a map. It also utilizes this new functionality by not
loading memberuid of parent groups during initgroups in rfc2307 schema.
On Wed, 2011-06-29 at 23:35 +0100, Matthew Ife wrote:
> Hi Stephen,
> I am working on the SSSD policy for local db management and hit across
> an alert that I believed should be fixed in code.
> The alert is because nscd.c called system() to request a flush command
> to nscd if it exists.
> This is a problem policy wise because this calls /bin/sh to execute the
> command which in turn forces policy to allow a much greater access to
> sssd to execute stuff that exists in /usr/bin and friends.
> In the patch attached I replaced the system() request with a fork/execl
> pair. This is much nicer in policy as I can change policy to call a
> specific transition into the nscd_t domain directly without giving
> access to sssd to the bin_t types in /usr/bin.
> Please check the patch. I'm not familiar with talloc (I dont really
> consider myself a C programmer of any merit) and my workaround to create
> a char** array to talloc memory might be a bit daft to do. If you can
> alter the code to something more elegant please feel free!
> On Mon, 2011-06-27 at 07:13 -0400, Stephen Gallagher wrote:
> > I happened to notice this email on the selinux list. This discussion
> > would probably be best served cross-posted to
> > sssd-devel(a)lists.fedorahosted.org, so we on the SSSD team can be
> > involved if there are any code changes/improvements we need to make in
> > order to further advance this proposal.
I took a look at the patch, and unfortunately it would need a fair
amount of rework to get it to function properly. However, it brings up
an interesting topic that I'd like to discuss with the SSSD list at
The main reason for the nscd flush is because, for a time, we were
attempting to allow the SSSD to operate alongside NSCD. These days,
however, we've acknowledged that the two caches really don't interact
well and we strongly advise that users disable user and group support in
nscd while using SSSD.
So I'd like to propose that, rather than fixing this code to work in the
way Matthew is suggesting, we remove it entirely, acknowledging that
nscd/sssd interaction is unlikely to ever be safe.