On Tue, 2012-01-31 at 22:15 +0100, Jan Zeleny wrote: > Jan Zelený <jzel...@redhat.com> wrote: > > Hi, > > I'm sending all patches implementing support for SELinux user maps. Some > > support patches are included as well. > > > > #0001: > > Implemented support for multiple search bases in HBAC rules and services. > > As discussed before, this is not strictly needed, but I did it anyway to > > unify the approach to multiple search bases. Just a reminder: the plan is > > to use these structures and then limit maximal number of search bases to 1 > > since there is no support in IPA server for more bases anyway. > >
Ack. This is good for maintaining code consistency, as well as being
able to support filtering if necessary.
> > #0002:
> > This fixes minor regression brought by my previous patch which is already
> > pushed (multiple search bases in IPA hosts).
> >
Nack. I fail to see the point of the misdirection through "target" here.
Isn't it simpler to do:
state->hostgroups[state->hostgroup_count] =
talloc_steal(state->hostgroups, hostgroups[i]);
> > #0003:
> > Add generic routines to retrieve IPA configuration object. These routines
> > will be used in other parts of the code.
> >
Ack.
> > #0004:
> > Rewrite retrieval of password migration flag from IPA config to user
> > previously implemented generic IPA config interface.
> >
Ack.
> > #0005:
> > Some sysdb netgroup attributes will be used in SELinux user maps. They will
> > also have the same semantics, therefore they should be renamed and then re-
> > used.
> >
Ack.
> > #0006:
> > Some sysdb routines for SELinux support. Please note that some routines are
> > written in very generic way - I'd like to use them also elsewhere in the
> > current code, perhaps as a part of some sysdb refactoring.
> >
Nack.
(This patch and the next one should be applied in the reverse order.
This patch depends on Patch 0007 but the reverse is not true).
The in_transaction construct in sysdb_store_selinux_entity() is wrong.
You should not be doing sysdb_transaction_commit() after the done:
label. The point of saving the in_transaction value is so that if you
'goto done' after the transaction has started, you will know to cancel
the transaction. In the current code, if you failed get_entity(),
sysdb_add_selinux_entity(), sysdb_set_selinux_entity_attr() or
get_rm_msg(), you're going to be calling sysdb_transaction_commit() on
incomplete data. OUCH.
sysdb_search_selinux_config_int() is improperly named. That name implies
that it is the internal implementation of sysdb_search_selinux_config(),
when it is in fact a wrapper function. How about just calling it
get_selinux_config_entity()?
Thinking more about this, I don't think there's really any reason to
create and use entity_by_name_fn at all. It would be easier and more
readable (I think) to just use an enum for whether it's a user or config
lookup. Then you can just call sysdb_search_selinux_usermap_by_name() or
sysdb_search_selinux_config() directly in sysdb_store_selinux_entity().
If there were a lot of possible entities to store, it would be one
thing. But for just two of them, let's not make the code needlessly
complicated.
Don't call the DN argument of sysdb_store_selinux_entity()
"dn_template". It's not a template, it has already been constructed.
Further, I think it would be better to carry around an ldb_dn rather
than just the string representation of the DN. Right now it's being
separately constructed in both sysdb_add_selinux_entity() and
sysdb_set_selinux_entity_attr(). Beyond that, if you construct the DN in
sysdb_store_selinux_entity() and sysdb_store_selinux_usermap(), you
don't need to have sysdb_set_selinux_entity_attr() at all, since all it
is doing is calling sysdb_set_entry_attr().
Don't "goto done" directly after sysdb_add_selinux_entity(). As
mentioned previously, you need to call sysdb_transaction_complete() and
(if it succeeds) set in_transaction = false.
sysdb_add_selinux_entity() and sysdb_set_selinux_entity_attr() should
set and update the SYSDB_LAST_UPDATE. sysdb_add_selinux_entity() should
also set SYSDB_CREATE_TIME. For debugging purposes, it's important to
know when the entries were created and updated.
There's no explanation for what the difference is between
sysdb_search_selinux_usermap_by_name() and
sysdb_search_selinux_usermap_by_user(). Perhaps
sysdb_search_selinux_usermap_by_mapname() and
sysdb_search_selinux_usermap_by_username() would be more clear?
Please don't call the return value "_msg" in
sysdb_search_selinux_usermap_by_name(). It's not descriptive. Name it
after the data it should contain (which I think is the contents of the
map, expected to be a single entry). If the map name is intended to be
unique (which I gather it must be, since it's a base search and you're
only returning the first result), you should probably check that
sysdb_search_entry returns zero or one for msgs_count. Please make the
same change for sysdb_search_selinux_config().
The number of usermaps in the cache is theoretically unbounded. It's
probably not safe to allocate an array to contain all of them if we're
going to be matching and holding onto only a subset of them. I'd rather
that you just realloc to fit each one that we filter (or if you believe
that this is going to occur computationally-often, write a simple
doubling algorithm). As it is, I'm wary of doing ALL of the processing
as a post-operation. Is there any way to improve the sysdb search filter
to reduce what we have to filter through?
Also, please free each message that doesn't match as we go through the
loop. There's no good reason to hang on to memory any longer than we
have to. I don't want to tempt the OOM killer.
I know we do the "assume the largest" array allocation in a lot of
places, but that's generally where we can reasonably assume array sizes
of < 100 entries. With the user maps, there's no limitation on how large
it can grow.
sysdb_search_selinux_usermap_by_user() should return ENOENT and free
"usermaps" if after the filtering usermaps[0] is NULL. This will make
the check in get_selinux_string() in Patch 0009 more clear.
> > #0007:
> > Utility functions for SELinux map matching against information about
> > current user and host.
> >
Nack.
In match_entity() and sss_selinux_match(), do we have a guarantee that
the entities being matched are UTF-8 strings? If not, you should
probably use the sss_utf8_case_eq() routine.
sss_selinux_match() should probably check for usermap == NULL.
sss_selinux_match() is in desperate need of comments. It's not clear
what exactly you're comparing.
sss_selinux_extract_user() doesn't have a matching entry in
sss_selinux.h. Looks like you changed plans?
> > #0008:
> > SELinux user maps support in IPA provider. Also generig data provider
> > related code is here. I'm considering splitting this patch in two or
> > three. Let me know your opinion.
> >
Too much code to look over at 10pm. Will get to it tomorrow.
> > #0009:
> > Responder support of SELinux user maps - retrieve all applicable maps from
> > sysbd and create content of the user mapping file
> > /etc/selinux/<policy>/logins/<usernale>
> >
Nack.
Please add comments to get_selinux_string().
You need to initialize "order" to NULL in case you get through all of
the config elements without finding it. Right now, the
if (default_user == NULL || order == NULL) {
could be doing a comparison against uninitialized "order".
Don't call strlen(order). You already have this information from the
ldb_val.length.
As mentioned in the review for patch 0006,
sysdb_search_selinux_usermap_by_user() should be returning ENOENT, which
you should check for here instead of NULL usermaps[0].
Did I mention the need for comments in get_selinux_string()? The loop
through the usermaps is ugly and hard to read. Please identify why
you're writing each piece. (A description comment above describing the
file format of the SELinux user map would be great).
Among other things, it might be nicer to write a comparison routine
that, if it passes, you append to file_content. This convoluted
triply-nested loop is hard to grok.
> > #0010:
> > Get the file content from PAM responder and write it to the file. I'm not
> > completely sure whether or not to implement some kind of locking to prevent
> > possible race conditions when reading/writing to this file.
> >
Nack.
As you suggest, you need to handle the file writing safely. Use
mkstemp() (properly with umask) to create the contents in a
randomly-generated temp file, then move it atop the old file with
rename() when it's complete (which is an atomic action).
> > Thanks in advance for the review. Any advices how to improve the code will
> > be appreciated.
Be careful what you wish for ;-)
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
