On Wed, 2012-02-01 at 18:29 +0100, Jan Zelený wrote:
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.
>
Still ack
> > > #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.
Ack.
> > > #0003:
> > > Add generic routines to retrieve IPA configuration object. These
> > > routines will be used in other parts of the code.
>
> Ack.
Still ack.
>
> > > #0004:
> > > Rewrite retrieval of password migration flag from IPA config to user
> > > previously implemented generic IPA config interface.
>
> Ack.
>
Still 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.
Still 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.
Ok, on a further read, this is safe (though I find it a bit ugly). Sorry
for the confusion.
> 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.
When using an enum, prefer a switch statement around 'type' (and skip
the 'default' case). This way the compiler would catch any potential
mistakes here such as adding a new type and not checking for it.
> 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.
Looks good.
> 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.
Acknowledged.
> 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? ;-)
Well, there are other things left to fix, so we might as well get this
in too.
> 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.
Thanks.
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.
Yes, you're right.
> 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).
That's a good point. The array of pointers is going to be negligible in
comparison to the array of sysdb_attrs.
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.
Hmm, it's more work, but it WOULD keep the memory down. This area of the
code really could become a pretty serious memory bottleneck in large
deployments. That said, I'm going to suggest that we go with this for
now and open a ticket to switch to a search per group later on. At the
very least I want to have a ticket to identify a possible pain point in
the future.
And obviously such an approach would have a much better effect of
keeping the memory down than just limiting the pointer array.
> 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
Thanks
> 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.
I'd like to hear this idea too, please :)
> 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
Thanks
> > > #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.
Sorry, I meant ASCII above, but you answered the question I meant to ask
anyway :)
That said, I think our better bet would be to actually convert them to
ldb_dn objects with ldb_dn_new() and use ldb_dn_compare() to determine
if they're the same. This will handle any case-sensitivity issues in an
LDB-appropriate way.
> sss_selinux_match() should probably check for usermap == NULL.
Done.
Thanks
> sss_selinux_match() is in desperate need of comments. It's not clear
> what exactly you're comparing.
Done.
Thanks
> 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.
Sorry, my eyes must have been glazing over by this point.
That said, you have a prototype for sss_selinux_extract_usermaps() but
it's not defined anywhere. I think this is where I got confused.
> > > #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.
See my other reply for changes needed here.
>
> > > #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.
While I recognize that the way you're splitting the order_array is
fairly clever, it's also somewhat difficult to read (and its memory
hierarchy is somewhat wrong, since the array and the contents are both
allocated at the same level on the tmp_ctx).
I'd prefer if you just walked through the order string until you hit a
'$', set that '$' to a '\0' and then did
order_array[i] = talloc_strdup(order_array, p);
(Where p is a pointer to the start of the 'word'.
You can then free 'order' after that and the order_array will have a
clear memory hierarchy (and the code will be more easily read).
> 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.
Thanks
> 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.
Thanks
> 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.
Thanks, that's much more clear now.
However, I'm pretty sure usernames in PAM can be unicode strings, so you
need to use the utf-8 case-insensitive string compare for matching the
users to the order list.
> 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
Thanks, but see above comment about unicode comparisons.
> > > #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.
Please issue a syslog message if the mkstemp() or write() fails. This is
probably going to end up being an SELinux or permission issue.
Also, please test whether this change is going to necessitate a change
to the SELinux policy shipped in Fedora. I don't know offhand if all
applications have privileges to write to mkstemp().
Otherwise, this looks good to me.
>
> > > 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
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel