I'm sending corrected patches (without the largest one - #0008).
On Tue, 2012-01-31 at 22:15 +0100, Jan Zeleny wrote:
> Jan Zelený <jzeleny(a)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]);
Done. The reason here was originally to make the code easier to read. I'm
afraid I achieved exactly the opposite in the end.
> > #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).
Done. Although the commend is not quite valid but that's not much important.
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.
In fact, I copy-pasted this very constuct from sysdb_store_user(). It can be
found in sysdb_remove_attrs() as well. I also think the original thought
behind this construct was a bit different: to determine if commit/cancel is
needed. If it will be commit or cancel is determined by the return code.
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()?
Done, it was removed entirely based on your recommendation below.
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.
I was hoping that this implementation could be later adapted in other parts of
sysdb to unify the approach to storing various entities. But looking more
closely at the code, it seems that it would require great deal of other
changes. Hence I agree with your opinion, the change is included in the new
patch.
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().
Done. I just left the dn_template since it really is a template at the time
the function is called.
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.
As commented above.
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.
TODO: I just noticed that it isn't fixed before sending the email and I don't
have the strength to do it right now. I'll send the fix later. How about a
conditional ack for the patch until this is done? ;-)
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?
Done
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().
Renaming the _msg: done.
Checking the count: I don't think it's necessary. I don't see how it would be
possible to get more than one result from base search. Also, I just checked
source code of LDB - it explicitly sets count = 1 every time you perform base
search.
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?
I thought about this for quite some time. About the memory allocation: I'm
against reallocation of the final array one by one. The allocation of the
entire array could be solved by two-pass in place filtering algorithm (with
subsequent realloc), but it won't solve the issue that potentially large
amount of memory has to be allocated to store all the ldb search results
(that's why I left the second array - it's quite small compared to the memory
we need to store all fetched results).
I don't believe there's a good solution for this, since the user can be member
of unlimited number of groups (and our experience says the real number is
sometimes pretty high). That's why the post-processing is necessary. If the
goal was to improve memory consumption no matter what, the solution would be
to do multiple ldb searches, one for each group the user is member of. But I
don't like that scenario.
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.
Done
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.
That's true but I'm really not sure how to optimize this further. It would
need some design changes on the server side. I will consult my idea with Rob.
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.
Done
> > #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.
They are DNs. I'm not quite sure they need to be treated as UTF-8 strings.
sss_selinux_match() should probably check for usermap == NULL.
Done.
sss_selinux_match() is in desperate need of comments. It's not
clear
what exactly you're comparing.
Done.
sss_selinux_extract_user() doesn't have a matching entry in
sss_selinux.h. Looks like you changed plans?
Actually, it does have a matching entry in that file.
> > #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().
Done.
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".
Done. Also the default_user needed the same fix.
Don't call strlen(order). You already have this information from
the
ldb_val.length.
Done.
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].
Done.
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).
Done.
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.
Done
> > #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).
Done.
> > Thanks in advance for the review. Any advices how to improve the code
> > will be appreciated.
Be careful what you wish for ;-)
Thanks for the review
Jan