[SSSD] THE SUDO rule elimination is over.

Stephen Gallagher sgallagh at redhat.com
Wed Aug 17 01:18:46 UTC 2011


For those following along, this is a review of the patches in Arun's
public git repository located at:
https://github.com/arunscaria91/sssd.git

On Tue, 2011-08-16 at 22:05 +0530, arun scaria wrote:
> 
> Dear sgallagh,
> I'm happy to inform you that the sudo rule elimination is over, Now I
> need to evaluate left out rules as we planned. Check my repo and
> comment through mail. If you can, inform the next meeting time. :)


I have a few review comments to make. Sorry that some of these probably
should have happened earlier, but this is the first chance I've had to
go through this in-depth.

sudo_query_validation():

1) This function is getting far too large. Please separate the D-BUS
message parsing into a separate function that allocates and returns 
'struct sss_sudo_msg_contents'.

It should look like:
sudo_query_parse(TALLOC_CTX *mem_ctx,
                 struct DBusMessage *message,
                 struct sss_sudo_msg_contents **contents);
and would be invoked like: sudo_query_parse(sudo_cli, message, &msg);

Please also separate the reply message creation into its own function,
in such a way that it can be called for success or failure.

Another point: do not explicitly cast to (TALLOC_CTX *). Internally,
it's just an alias for 'void *' and is safe to implicitly cast
everywhere. Adding the explicit cast just makes the code harder to read.

Please don't use GOTO except to an exit condition. Your use of
'free_ctx' is completely unacceptable. We only ever use GOTO if we're
short-circuiting to exit on failure. As mentioned above, please create a
function to send the reply and call it explicitly for either success or
user not found.

Please always test the returned values from ldb_msg_find_attr_*(),
because if the message did not contain the value, we need to consider it
an error and return a failure reply.

You still have "tom" passed into search_sudo_rules(), where it should be
user_name.

Please don't use malloc() or strdup() directly (as with "PASS"). Use
talloc() and talloc_strdup() instead.





search_sudo_rules():

Do not allocate tmp_ctx on sudocli. In a synchronous function, tmp_ctx
contexts should be allocated on NULL and guaranteed to be maintained
internally. This means that you must always talloc_free(tmp_ctx) before
any return from the function. This provides an easy way to clean up any
internal-only memory, but it should NOT be attached to an external
context. If it's attached to an external context and we forget to free
it before returning in some situations, not only is it a leak, but it's
a leak we cannot detect with tools like valgrind.

Furthermore, functions should not attach to an external context (such as
sudocli) directly. If they are going to allocate memory that is to be
returned, the function MUST take a mem_ctx argument (then the caller of
the function will decide whether sudocli is the appropriate parent for
it).

Don't allocate part of 'filter' first and then call prepare_filter().
You should just pass in tmpctx and user_name to prepare_filter and
create it all in that function.

Don't allocate listctx on NULL. You have a tmpctx already; this is
exactly what it's supposed to be for (so you only have to worry about
calling talloc_free(tmpctx) before returning).

Please don't write your own linked list method. We have our own
implementation already in the SSSD sources. See src/util/dlinklist.h.
You probably want to be using DLIST_FOREACH() in place of your while()
loop.

Your functions initList(), appendNode() etc violate our naming
standards.

There are a bunch of things wrong with list_sss. 1) Do not use typedefs.
2) Don't try to create generic lists. Just create a 'struct
sss_sudo_list' and use the real 'struct ldb_message *" instead of
casting back and forth to (void *). 3) sss_sudo_list should have the
prev and next pointers so it can be used with the DLIST macros.

The comment /* see if this is a user */ is in the wrong place. You're
comparing the command here.

Please make each of the individual comparisons (command, user, host)
into their own functions. search_sudo_rules() is too large and hard to
follow. (Also, add comments)

We need to figure out an alternative to the _SSS_LOOPS issue here (but
this isn't your problem, Arun). We have no choice but to call into glibc
if we want innetgr() to resolve properly. This comment is added so that
if anyone besides Arun has read this far, they should feel free to
comment :)

For users, commands and hosts you need to check for negation "!" rules.

As you're walking through each, if you hit an explicit denial at any
time, you should short-circuit to returning PERMISSION DENIED.

On a related note, this function needs to return the results necessary
to reply to the client. You probably need to construct this as a struct
that you'll pass back to sudo_query_validation() which will in turn pass
it into the reply creation routine I mentioned above.



get_host_name():

Please use "mem_ctx" to make things clear. 'ctx' is too ambiguous and
we're trying to phase it out elsewhere in the code.

Do not call exit(). EVER.
It is much better to have the function return an errno_t and instead
return the hostname as a "char **hostname" parameter to the function.

This function needs comments *desperately*. I really don't have any idea
what you're trying to do, but I'm sure you aren't doing it. Right now,
you're returning a copy of a member of the "p" variable that has never
been initialized. Also, this should just be talloc_strdup(mem_ctx,
p->ai_canonname);

I don't really know why you're calling getaddrinfo() for the "http"
service either.



prepare_filter():

See comment above about passing in user_name.
You also need to handle looking up the explicit negations. So your
search filter needs to include "sudoUser=!username", "sudoUser=!#gid"
and "sudoUser=!+*" (same for hosts).

We need to make sure that if any rule specifically denies this user or
host, it is honored.

Change the name of the variable "res" once it's passed in. It would be
more clear if it was named "groups_res".

As I said above, you can't assume that ldb_msg_find_* will return
non-NULL. You need to check this before you can pass it into
talloc_asprintf_append(). If it's NULL, log an error with DEBUG() and
skip it.

You use a lot of repeated code when checking the filter pieces. Might be
a good idea to just create a macro like FILTER_APPEND_CHECK(). (This is
just a suggestion; there's nothing wrong with the repetition other than
it takes up more space).

However, you're checking whether filter is NULL and then attempting to
print it in the DEBUG statements. That's not going to work :)



sysdb_search_sudo_rules():

As described above, tmpctx should be allocated on NULL, not mem_ctx.
Only the values being returned should ever be allocated or stolen with
talloc_steal() onto the passed-in mem_ctx.




compare_sudo_order():

Correct me if I'm wrong, but won't this be sorting from lowest to
highest? Doesn't sudo require us to process the higher values first?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110816/128236f4/attachment.sig>


More information about the sssd-devel mailing list