[SSSD] [PATCH] IFP: Add GetUserAttrs call

Pavel Březina pbrezina at redhat.com
Tue May 13 14:32:26 UTC 2014


On 05/09/2014 09:33 PM, Jakub Hrozek wrote:
> On Mon, 2014-05-05 at 11:25 +0200, Stef Walter wrote:
>> 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
>>
>
> Thanks, good catch. We already have SSS_ATTRIBUTE_PRINTF() macro, which
> I forgot to use. The patch was updated.
>
>>>>> [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 haven't implemented the request chaining mostly because I think we
> have bigger issues to solve. But I don't think we should forget about
> the problem either, so I filed:
> https://fedorahosted.org/sssd/ticket/2329
>
> I split the async request to retrieve the caller ID into a separate
> patch, because with the sbus_message_handler changes, the combined patch
> was too big and too hard to read.
>
>>
>>> 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.
>>
>
> Yeah, that was one of the points I was pondering -- how much should the
> sbus message handler do. The attached patch "SBUS: Refactor
> sbus_message_handler to retrieve caller ID" only uses the message
> handler to verify the message should be handled by one of SSSD's
> interfaces and if so, starts the get-ID async request. The real work is
> done in the request callback, sbus_handler_got_caller_id().
>
>> +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.
>
> This structure is gone in the attached patches.
>
>>
>> +    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?
>>
>
> Ah, good catch! The current version of the async request strdups the
> state onto the request state, so the request would run to completion
> regardless of what happens to the 'sender' variable.
>
>>>   * 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.
>>
>
> Right, done.
>
>> 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.
>>
>
> I added a comment to the code.
>
>>>>> +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).
>>
>
> I agree, but I would prefer to finish the getsetters and implement the
> object caching etc first. So I filed:
> https://fedorahosted.org/sssd/ticket/2328
>
> Thanks for the review!
>
> New patchset is attached.

Ack.




More information about the sssd-devel mailing list