[SSSD] [PATCH] IFP: Add GetUserAttrs call

Stef Walter swalter at redhat.com
Mon May 5 09:25:47 UTC 2014


On 02.05.2014 17:25, Jakub Hrozek wrote:
>>> [PATCH 1/6] SBUS: two trivial style fixes SSIA
>>>
>>> [PATCH 2/6] SBUS: Add a convenience function Adds a convenience
>>> function that constructs a DBusError internally and as such can be
>>> used to mark an sbus request as failed without having to create a
>>> DBusError instance by the caller. The error message must be either
>>> allocated on top of dbus_req or a constant, DBus itself doesn't copy
>>> the error message! This is reflected in the doxygen docstring.
>>
>> Hmmm, rather than that (and because of the dbus behavior), my preference
>> would be to have an sbus_error_new() method that created a DBusError on
>> a MEM_CTX, and could be used like this:
>>
>> sbus_request_fail_and_finish (dbus_req,
>>                               sbus_error_new (dbus_req,
>>                                               "dotted.Error",
>>                                               "format str: %s", arg));
>>
>> This is what I sorta imagined when implementing
>> sbus_request_fail_and_finish()
> 
> Done, this makes sense, thanks for the suggestion.

+DBusError *sbus_error_new(TALLOC_CTX *mem_ctx,
+                          const char *dbus_err_name,
+                          const char *fmt,
+                          ...);

This declaration should be decorated with the appropriate GCC attribute
for printf style functions. Otherwise folks will easily create printf
string vulnerabilities. ie:

#if __GNUC__ > 2
__attribute__((__format__(__printf__, 3, 4)));
#endif

Or you probably have this wrapped up in a nice macro in SSSD, like this:

http://cgit.freedesktop.org/p11-glue/p11-kit/tree/common/compat.h#n47

>>> [PATCH 3/6] IFP: Add utility functions Adds a number of utility
>>> functions, most importanly ifp_req_create(). The ifp_req is a
>>> structure that will be passed along with the ifp request and contains
>>> the sbus request and some ifp-specific data, like caller ID or the
>>> responder context.
>>
>> dbus_bus_get_unix_user() blocks. Is that fine in this case?
> 
> I implemented what you suggested in patch 5/9. It's not polished yet,
> because I wanted to check if that's what you had in mind before spending
> too much time.

In general it looks right on.

> In particular, I would like to chain concurrent requests for the same
> sender ID to not trash the sysbus if many requests came in after startup
> for instance. Also unit tests are needed.

That makes sense.

> I'm not too happy about sbus_message_handler now either, I'd like to
> refactor it to be more readable, maybe pass around a single struct
> instance instead of handler/sender/interface/etc..

I think that struct should be sbus_request. But more to the point, I
think that much of the code should run *after* you get the caller's
information, not before.

+struct sbus_unhandled_request {
+    struct sbus_request *dbus_req;
+    const struct sbus_method_meta *method;
+    sbus_msg_handler_fn handler_fn;

By the way you already have the above 2 fields inside of dbus_req.

+    key.type = HASH_KEY_STRING;
+    key.str = discard_const(state->sender);
+    value.type = HASH_VALUE_UINT;
+    value.ul = state->uid;
+    ret = hash_enter(state->conn->clients, &key, &value);
+    if (ret != HASH_SUCCESS) {
+        ret = EIO;
+        goto done;
+    }

What owns state->member? Is it going to be referenced after free?

>  * Should we carry on when we can't retrieve the ID for one reason or
>    another and let the callee kick out the request?

I think you should drop the request if you can't identify the caller
(may have exited for example). Maybe log something, but no use trying to
talk to a caller you cannot identify.

By the way, once you want the PID or SELinux context, you'll want to
change GetConnectionUnixUser to GetConnectionCredentials. For now though
it's fine to just get the UID.

>>> +int ifp_enomem(struct ifp_req *ireq)
>>
>> This won't work as you expect. When you're out of memory it's unlikely
>> that you'll be able to send a normal message. DBus has method calls
>> specifically for this case:
>>
>> http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga2fea5f972d1bfe7bcde8c0ec65ca9e90
> 
> Ah, OK, I must say this part was always a bit confusing to me. Why do we try
> to send messages crated with dbus_message_new_error(DBUS_ERROR_NO_MEMORY)
> in sbus_message_handler, should those be fixed too?

Yes probably. I'm not a big fan of trying to handle memory errors like
other errors. It just never works reliably, and it's nearly untestable.
I think avoiding a crash freeing up resources and moving on, maybe log
something if you can, is the best approach.

I think the only reason dbus even has this code is so things like
systemd (ie: pid 1) can make dbus calls.

> These options can be used to configure the InfoPipe responder.

PATCH007: This stuff should be using polkit. I understand if the current
patch set doesn't use polkit, but sssd will eventually need to ask
polkit if X principal is allowed to do Y via DBus. Perhaps allowed_uids
would bypass the polkit check (many DBus services do such a bypass with
uid = 0).

Cheers,

Stef



More information about the sssd-devel mailing list