[SSSD] [PATCH] IFP: Add GetUserAttrs call

Jakub Hrozek jakub.hrozek at gmail.com
Fri May 9 19:33:16 UTC 2014


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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SBUS-several-trivial-style-fixes.patch
Type: text/x-patch
Size: 3694 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140509/0939aa87/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-SBUS-Fix-error-handling-condition.patch
Type: text/x-patch
Size: 1067 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140509/0939aa87/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-SBUS-Add-a-convenience-function-sbus_error_new.patch
Type: text/x-patch
Size: 4740 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140509/0939aa87/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-SBUS-Split-out-dbus_conn_send.patch
Type: text/x-patch
Size: 4252 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140509/0939aa87/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-SBUS-Add-SBUS_CONN_TYPE_SYSBUS.patch
Type: text/x-patch
Size: 2499 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140509/0939aa87/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-SBUS-Add-an-async-request-to-retrieve-the-caller-ID.patch
Type: text/x-patch
Size: 20958 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140509/0939aa87/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-SBUS-Refactor-sbus_message_handler-to-retrieve-calle.patch
Type: text/x-patch
Size: 10027 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140509/0939aa87/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-IFP-Add-utility-functions.patch
Type: text/x-patch
Size: 14441 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140509/0939aa87/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-IFP-use-a-list-of-allowed_uids-for-authentication.patch
Type: text/x-patch
Size: 13648 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140509/0939aa87/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0010-IFP-Initialize-negative-cache-timeout.patch
Type: text/x-patch
Size: 2436 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140509/0939aa87/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0011-IFP-Add-GetUserAttrs-call.patch
Type: text/x-patch
Size: 24955 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140509/0939aa87/attachment-0010.bin>


More information about the sssd-devel mailing list