Hi,
the attached patches upstream functionality that mod_lookup_identity has been using for a while, but which we couldn't push to master due to the pending sbus changes. Now that these are accepted, it's time to merge the DBus methods themselves. Previously, mod_lookup_identity relied on a private branch with quite a bit of ad-hoc code.
Please note that when the work in progress to expose users, groups and domains is merged as well, this interface will become obsolete in favor of the objects. But we shouldn't let a released version of mod_lookup_identity rely on unreleased sssd code any longer. Moreover, all patches except the last one will be used even later on and there's no fixed timeline on converting mod_lookup_identity.
[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.
[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.
[PATCH 4/6] IFP: use a list of allowed_uids for authentication Similar to the PAC responder, the InfoPipe uses a list of UIDs that are allowed to communicate with the IFP responder.
[PATCH 5/6] IFP: Initialize negative cache timeout In order to avoid hitting the back end with repetitive requests, the InfoPipe responder needs a negative cache, too. This patch follows the convention set by other responders, where the negative cache timeouts are read from the [nss] section. This is not ideal, however, and ticket #2318 tracks moving the configuration to the [ifp] section primarily.
The timeout is also a separate parameter in the NSS context. We should consider moving it to the negcache context instead (#2317).
[PATCH 6/6] IFP: Add GetUserAttrs call Adds a DBus method that allows the caller to retrieve attributes of a user. The synopsis of the call is as follows: <method name="GetUserAttr"> <arg type="s" name="user" direction="in"/> <arg type="as" name="attr" direction="in"/> <arg type="a{sv}" name="values" direction="out"/> </method>
The return value is an array (one attribute per array member) of dictionaries. The key of the dictionary is the attribute name, the value is a variant containing the attribute values as strings.
If an attribute does not exist or is not permitted to be read, no error is returned. If the users does not exist, the method returns an error.
In future patches this function will be marked as obsolete in favor of object-oriented approach.
ifp_user_get_attr_unpack_msg is a separate function to allow extending it in a later patch.
The function to check the cache validity duplicates quite a bit of code with the NSS responder. The refactoring would be nice to get done along with #843. I have a patch in my private branch, but it's not complete, sorry.
On 04/23/2014 10:43 PM, Jakub Hrozek wrote:
Hi,
the attached patches upstream functionality that mod_lookup_identity has been using for a while, but which we couldn't push to master due to the pending sbus changes. Now that these are accepted, it's time to merge the DBus methods themselves. Previously, mod_lookup_identity relied on a private branch with quite a bit of ad-hoc code.
Please note that when the work in progress to expose users, groups and domains is merged as well, this interface will become obsolete in favor of the objects. But we shouldn't let a released version of mod_lookup_identity rely on unreleased sssd code any longer. Moreover, all patches except the last one will be used even later on and there's no fixed timeline on converting mod_lookup_identity.
[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.
[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.
[PATCH 4/6] IFP: use a list of allowed_uids for authentication Similar to the PAC responder, the InfoPipe uses a list of UIDs that are allowed to communicate with the IFP responder.
[PATCH 5/6] IFP: Initialize negative cache timeout In order to avoid hitting the back end with repetitive requests, the InfoPipe responder needs a negative cache, too. This patch follows the convention set by other responders, where the negative cache timeouts are read from the [nss] section. This is not ideal, however, and ticket #2318 tracks moving the configuration to the [ifp] section primarily.
The timeout is also a separate parameter in the NSS context. We should consider moving it to the negcache context instead (#2317).
[PATCH 6/6] IFP: Add GetUserAttrs call Adds a DBus method that allows the caller to retrieve attributes of a user. The synopsis of the call is as follows: <method name="GetUserAttr"> <arg type="s" name="user" direction="in"/> <arg type="as" name="attr" direction="in"/> <arg type="a{sv}" name="values" direction="out"/> </method>
The return value is an array (one attribute per array member) of dictionaries. The key of the dictionary is the attribute name, the value is a variant containing the attribute values as strings.
If an attribute does not exist or is not permitted to be read, no error is returned. If the users does not exist, the method returns an error.
In future patches this function will be marked as obsolete in favor of object-oriented approach.
ifp_user_get_attr_unpack_msg is a separate function to allow extending it in a later patch.
The function to check the cache validity duplicates quite a bit of code with the NSS responder. The refactoring would be nice to get done along with #843. I have a patch in my private branch, but it's not complete, sorry.
Patch 1: Ack.
Patch 2: Ack.
Patch 3: Ack.
Patch 4: Ack.
Patch 5: Ack.
Patch 6: Ack. Only the commit subject needs to be fixed. You use Attrs there, however, the method uses singular.
User attributes are correctly returned. Allowed_uids option works.
I would just like to propose two more patches: * IFP: indentation fixes Just spaces.
* IFP: report error from ifp_req_create() This patch makes d-bus error reporting more meaningful.
Instead of generic: Error org.freedesktop.DBus.Error.Failed: Could not create ifp_req
You get: Error org.freedesktop.DBus.Error.Failed: Permission denied
when user is not found in ACL.
On Thu, Apr 24, 2014 at 11:48:47AM +0200, Pavel Březina wrote:
On 04/23/2014 10:43 PM, Jakub Hrozek wrote:
Hi,
the attached patches upstream functionality that mod_lookup_identity has been using for a while, but which we couldn't push to master due to the pending sbus changes. Now that these are accepted, it's time to merge the DBus methods themselves. Previously, mod_lookup_identity relied on a private branch with quite a bit of ad-hoc code.
Please note that when the work in progress to expose users, groups and domains is merged as well, this interface will become obsolete in favor of the objects. But we shouldn't let a released version of mod_lookup_identity rely on unreleased sssd code any longer. Moreover, all patches except the last one will be used even later on and there's no fixed timeline on converting mod_lookup_identity.
[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.
[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.
[PATCH 4/6] IFP: use a list of allowed_uids for authentication Similar to the PAC responder, the InfoPipe uses a list of UIDs that are allowed to communicate with the IFP responder.
[PATCH 5/6] IFP: Initialize negative cache timeout In order to avoid hitting the back end with repetitive requests, the InfoPipe responder needs a negative cache, too. This patch follows the convention set by other responders, where the negative cache timeouts are read from the [nss] section. This is not ideal, however, and ticket #2318 tracks moving the configuration to the [ifp] section primarily.
The timeout is also a separate parameter in the NSS context. We should consider moving it to the negcache context instead (#2317).
[PATCH 6/6] IFP: Add GetUserAttrs call Adds a DBus method that allows the caller to retrieve attributes of a user. The synopsis of the call is as follows: <method name="GetUserAttr"> <arg type="s" name="user" direction="in"/> <arg type="as" name="attr" direction="in"/> <arg type="a{sv}" name="values" direction="out"/> </method>
The return value is an array (one attribute per array member) of dictionaries. The key of the dictionary is the attribute name, the value is a variant containing the attribute values as strings.
If an attribute does not exist or is not permitted to be read, no error is returned. If the users does not exist, the method returns an error.
In future patches this function will be marked as obsolete in favor of object-oriented approach.
ifp_user_get_attr_unpack_msg is a separate function to allow extending it in a later patch.
The function to check the cache validity duplicates quite a bit of code with the NSS responder. The refactoring would be nice to get done along with #843. I have a patch in my private branch, but it's not complete, sorry.
Patch 1: Ack.
Patch 2: Ack.
Patch 3: Ack.
Patch 4: Ack.
Patch 5: Ack.
Patch 6: Ack. Only the commit subject needs to be fixed. You use Attrs there, however, the method uses singular.
User attributes are correctly returned. Allowed_uids option works.
I would just like to propose two more patches:
- IFP: indentation fixes
Just spaces.
- IFP: report error from ifp_req_create()
This patch makes d-bus error reporting more meaningful.
Instead of generic: Error org.freedesktop.DBus.Error.Failed: Could not create ifp_req
You get: Error org.freedesktop.DBus.Error.Failed: Permission denied
when user is not found in ACL.
Thanks, I merged your patches in, the first one pretty much verbatim, the second one with modifications -- I named the variable 'ireq' not 'req', sine 'req' should be mostly used for tevent requests and fixed some bugs in your version of ifp_req_create, in paricular the ireq was freed unconditionally in the done handler :-)
I also fixed some bugs Jan P. found when testing the request with his modules. So far I've pushed the whole patchset into my private branch, feel free to check them out. I'm still working on Stef's suggestions.
On 23.04.2014 22:43, Jakub Hrozek wrote:
Hi,
the attached patches upstream functionality that mod_lookup_identity has been using for a while, but which we couldn't push to master due to the pending sbus changes. Now that these are accepted, it's time to merge the DBus methods themselves. Previously, mod_lookup_identity relied on a private branch with quite a bit of ad-hoc code.
Please note that when the work in progress to expose users, groups and domains is merged as well, this interface will become obsolete in favor of the objects. But we shouldn't let a released version of mod_lookup_identity rely on unreleased sssd code any longer. Moreover, all patches except the last one will be used even later on and there's no fixed timeline on converting mod_lookup_identity.
Did some high level review of the DBus related stuff...
[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()
[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?
What we really want is to be able to get this information asyncronously and fill it in the sbus_request before dispatching a given method call to the handler. We would also cache this information on sbus_connection.
+{ + const char *conn_name;
Suggest using 'sender' instead of 'conn_name' for this variable name.
+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#ga2fea5f...
[PATCH 6/6] IFP: Add GetUserAttrs call Adds a DBus method that allows the caller to retrieve attributes of a user. The synopsis of the call is as follows: <method name="GetUserAttr"> <arg type="s" name="user" direction="in"/> <arg type="as" name="attr" direction="in"/> <arg type="a{sv}" name="values" direction="out"/> </method>
- parsed = sbus_request_parse_or_finish(attr_req->ireq->dbus_req,
DBUS_TYPE_STRING, &attr_req->name,DBUS_TYPE_ARRAY, DBUS_TYPE_STRING,&attr_req->attrs,&attr_req->nattrs,DBUS_TYPE_INVALID);
attr_req->nattrs is of the wrong type. It needs to be an int. This shows why using RawHandler is a bit tricky. I wish we had support for unwrapping and wraping a{sv}. Is there any good sssd data structure that it would make sense to represent such a dict as? Probably pretty tough.
Cheers,
Stef
On Thu, Apr 24, 2014 at 12:59:45PM +0200, Stef Walter wrote:
[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?
What we really want is to be able to get this information asyncronously and fill it in the sbus_request before dispatching a given method call to the handler. We would also cache this information on sbus_connection.
Hi Stef,
thanks a lot for the review!
Do you know if dbus_bus_get_unix_user does any Name Service Switch calls that could potentially go over network? Blocking on local IO would be OK I think, but not if we risk blocking on network IO.
Can you suggest how should I get the UID info asynchronously? I agree with the rest of your comments and will fix the issues..
On 24.04.2014 13:24, Jakub Hrozek wrote:
On Thu, Apr 24, 2014 at 12:59:45PM +0200, Stef Walter wrote:
[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?
What we really want is to be able to get this information asyncronously and fill it in the sbus_request before dispatching a given method call to the handler. We would also cache this information on sbus_connection.
Hi Stef,
thanks a lot for the review!
Do you know if dbus_bus_get_unix_user does any Name Service Switch calls that could potentially go over network? Blocking on local IO would be OK I think, but not if we risk blocking on network IO.
Can you suggest how should I get the UID info asynchronously? I agree with the rest of your comments and will fix the issues..
If you look at dbus_bus_get_unix_user, it's just running a DBus method calling to the system bus.
So essentially you would do this in the sssd_dbus_connection dispatch code. If you don't have unix credentials for the sender cached, then you perform that DBus method call back to the bus asking for it, get it, cache it, and only then dispatch the method.
kdbus fixes this. But until then this is the robust way to accomplish this.
I imagine sssd_dbus_connection would have a flag saying whether it's running on the system bus or not. And would enable this behavior.
Stef
On Thu, Apr 24, 2014 at 12:59:45PM +0200, Stef Walter wrote:
On 23.04.2014 22:43, Jakub Hrozek wrote:
Hi,
the attached patches upstream functionality that mod_lookup_identity has been using for a while, but which we couldn't push to master due to the pending sbus changes. Now that these are accepted, it's time to merge the DBus methods themselves. Previously, mod_lookup_identity relied on a private branch with quite a bit of ad-hoc code.
Please note that when the work in progress to expose users, groups and domains is merged as well, this interface will become obsolete in favor of the objects. But we shouldn't let a released version of mod_lookup_identity rely on unreleased sssd code any longer. Moreover, all patches except the last one will be used even later on and there's no fixed timeline on converting mod_lookup_identity.
Did some high level review of the DBus related stuff...
Thanks, much appreciated!
[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.
[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 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.
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..
What we really want is to be able to get this information asyncronously and fill it in the sbus_request before dispatching a given method call to the handler. We would also cache this information on sbus_connection.
+{ + const char *conn_name;
Suggest using 'sender' instead of 'conn_name' for this variable name.
This variable went away in the current patchset.
+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#ga2fea5f...
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?
In the current patchset, I removed ifp_enomem() completely and I'd like to add the preallocated handler in a future patch (tracked by a ticket, otherwise we'd forget), since ENOMEM is very unlikely on Linux anyway. The current patchset just calls "return sbus_request_finish(dbus_req, NULL);" in an OOM situation.
[PATCH 6/6] IFP: Add GetUserAttrs call Adds a DBus method that allows the caller to retrieve attributes of a user. The synopsis of the call is as follows: <method name="GetUserAttr"> <arg type="s" name="user" direction="in"/> <arg type="as" name="attr" direction="in"/> <arg type="a{sv}" name="values" direction="out"/> </method>
- parsed = sbus_request_parse_or_finish(attr_req->ireq->dbus_req,
DBUS_TYPE_STRING, &attr_req->name,DBUS_TYPE_ARRAY, DBUS_TYPE_STRING,&attr_req->attrs,&attr_req->nattrs,DBUS_TYPE_INVALID);attr_req->nattrs is of the wrong type. It needs to be an int. This shows why using RawHandler is a bit tricky.
This was actually fixed in the next patch in my tree where I don't assign to nattrs directly, which is why I never saw the error probably.
A new patchset is attached. Thanks again for the review.
I wish we had support for unwrapping and wraping a{sv}. Is there any good sssd data structure that it would make sense to represent such a dict as? Probably pretty tough.
I wrote some helpers when working on the property getters, maybe when we review the property access, we could take a look at making them more generic..but in general I agree, the repetitive tasks should be abstracted.
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#ga2fea5f...
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
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#ga2fea5f...
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.
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#ga2fea5f...
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.
On (13/05/14 21:58), Jakub Hrozek wrote:
On Tue, May 13, 2014 at 04:32:26PM +0200, Pavel Březina wrote:
Thanks for the review!
New patchset is attached.
Ack.
Pushed to master in 997d4bc..2fbe9b9
Thanks for the review Pavel and Stef!
You can be interested in SELinux BZ. https://bugzilla.redhat.com/show_bug.cgi?id=1094873
I found SELinux problem with this patches, therefore I am replying to this mail.
LS
On Tue, May 13, 2014 at 09:58:42PM +0200, Jakub Hrozek wrote:
On Tue, May 13, 2014 at 04:32:26PM +0200, Pavel Březina wrote:
Thanks for the review!
New patchset is attached.
Ack.
Pushed to master in 997d4bc..2fbe9b9
Thanks for the review Pavel and Stef!
Also pushed to sssd-1-11: 6ad198aad0406bf6649540ff99c699cdc3f5f3b7 78512c471e3fd70811ad9def888778aec0f492dc 2c3fa3a3264c957957db48c6c488049b6cf8b7a1 f0875d13c3bd4766eea72b054365abfb9fd610a4 1cd6f169add862ef24653d2b99fbd6302c27239b c5651f39ef1660150ffdaed6f35ea14708327adc 40074db97ed4552cf09b1b6d71c19f53d010f80b 18cea3a5324d4cd56e62e3df7422fb6d14ad293b dfaad002f3b49e06dd121ceda83b28c8cdfc2c78 0ca2e6881c6dfa99c42e34d0afd038c7a946b41c 7daf068c4bbf581a898891e130c244eb9707b32a
On 09.05.2014 21:33, 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#ga2fea5f...
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.
This looks good.
Stef
On (23/04/14 22:43), Jakub Hrozek wrote:
Hi,
the attached patches upstream functionality that mod_lookup_identity has been using for a while, but which we couldn't push to master due to the pending sbus changes. Now that these are accepted, it's time to merge the DBus methods themselves. Previously, mod_lookup_identity relied on a private branch with quite a bit of ad-hoc code.
Please note that when the work in progress to expose users, groups and domains is merged as well, this interface will become obsolete in favor of the objects. But we shouldn't let a released version of mod_lookup_identity rely on unreleased sssd code any longer. Moreover, all patches except the last one will be used even later on and there's no fixed timeline on converting mod_lookup_identity.
[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.
[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.
[PATCH 4/6] IFP: use a list of allowed_uids for authentication Similar to the PAC responder, the InfoPipe uses a list of UIDs that are allowed to communicate with the IFP responder.
[PATCH 5/6] IFP: Initialize negative cache timeout In order to avoid hitting the back end with repetitive requests, the InfoPipe responder needs a negative cache, too. This patch follows the convention set by other responders, where the negative cache timeouts are read from the [nss] section. This is not ideal, however, and ticket #2318 tracks moving the configuration to the [ifp] section primarily.
The timeout is also a separate parameter in the NSS context. We should consider moving it to the negcache context instead (#2317).
[PATCH 6/6] IFP: Add GetUserAttrs call Adds a DBus method that allows the caller to retrieve attributes of a user. The synopsis of the call is as follows:
<method name="GetUserAttr"> <arg type="s" name="user" direction="in"/> <arg type="as" name="attr" direction="in"/> <arg type="a{sv}" name="values" direction="out"/> </method>
The return value is an array (one attribute per array member) of dictionaries. The key of the dictionary is the attribute name, the value is a variant containing the attribute values as strings.
If an attribute does not exist or is not permitted to be read, no error is returned. If the users does not exist, the method returns an error.
In future patches this function will be marked as obsolete in favor of object-oriented approach.
ifp_user_get_attr_unpack_msg is a separate function to allow extending it in a later patch.
The function to check the cache validity duplicates quite a bit of code with the NSS responder. The refactoring would be nice to get done along with #843. I have a patch in my private branch, but it's not complete, sorry.
From 53a9af5e3659293e2845b9a7aafea4e8bfbe818e Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 6 Jan 2014 15:27:44 +0100 Subject: [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.
Makefile.am | 29 ++++- src/responder/ifp/ifp_private.h | 16 +++ src/responder/ifp/ifpsrv_util.c | 197 ++++++++++++++++++++++++++++++++++ src/tests/cmocka/test_ifp.c | 231 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 472 insertions(+), 1 deletion(-) create mode 100644 src/responder/ifp/ifpsrv_util.c create mode 100644 src/tests/cmocka/test_ifp.c
diff --git a/Makefile.am b/Makefile.am index 56d8e1df648822a1fe8b3ca5fc8995f3c78a8566..e05461a3ab400e34bf685de89f2ec038822648fe 100644 --- a/Makefile.am +++ b/Makefile.am @@ -176,7 +176,12 @@ if HAVE_CMOCKA dp_opt_tests \ responder-get-domains-tests \ test_search_bases -endif
+if BUILD_IFP +non_interactive_cmocka_based_tests += ifp_tests +endif # BUILD_IFP
+endif # HAVE_CMOCKA
check_PROGRAMS = \ stress-tests \ @@ -818,6 +823,7 @@ sssd_ifp_SOURCES = \ src/responder/ifp/ifpsrv_cmd.c \ src/responder/ifp/ifp_iface_generated.c \ src/responder/ifp/ifp_iface_generated.h \
- src/responder/ifp/ifpsrv_util.c \ $(SSSD_UTIL_OBJ) \ $(SSSD_RESPONDER_OBJ)
sssd_ifp_CFLAGS = \ @@ -1676,8 +1682,29 @@ dp_opt_tests_LDADD = \ $(SSSD_INTERNAL_LTLIBS) \ libsss_test_common.la
+if BUILD_IFP +ifp_tests_DEPENDENCIES = \
^^^^ Use prefix EXTRA_ if you need memberof pluging for this test. Otherwise, it can break parallel build. We have this prefix also on other places.
$(ldblib_LTLIBRARIES)+ifp_tests_SOURCES = \
$(TEST_MOCK_RESP_OBJ) \- src/tests/cmocka/test_ifp.c \
- src/responder/ifp/ifpsrv_cmd.c \
- src/responder/ifp/ifpsrv_util.c
+ifp_tests_CFLAGS = \
- $(AM_CFLAGS)
+ifp_tests_LDFLAGS = \
- -Wl,-wrap,sbus_conn_send_reply \
- -Wl,-wrap,dbus_message_get_sender \
- -Wl,-wrap,dbus_bus_get_unix_user
+ifp_tests_LDADD = \
- $(CMOCKA_LIBS) \
- $(SSSD_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la
endif
+endif # HAVE_CMOCKA
noinst_PROGRAMS = pam_test_client if BUILD_SUDO noinst_PROGRAMS += sss_sudo_cli diff --git a/src/responder/ifp/ifp_private.h b/src/responder/ifp/ifp_private.h index b97cb8a7d9e55f550a3eda0e9acc034329d4ba17..d33009a5dbc95f8c0ca8905d9c8b9e42eaa24988 100644 --- a/src/responder/ifp/ifp_private.h +++ b/src/responder/ifp/ifp_private.h @@ -46,4 +46,20 @@ struct ifp_ctx {
- It will be removed later */
int ifp_ping(struct sbus_request *dbus_req, void *data);
+/* == Utility functions == */ +struct ifp_req {
- struct sbus_request *dbus_req;
- struct ifp_ctx *ifp_ctx;
- uid_t caller;
+};
+struct ifp_req *ifp_req_create(struct sbus_request *dbus_req,
struct ifp_ctx *ifp_ctx);+int ifp_enomem(struct ifp_req *ireq); +int ifp_return_failure(struct ifp_req *ireq, const char *err_msg); +const char *ifp_path_strip_prefix(const char *path, const char *prefix); +errno_t ifp_add_ldb_el_to_dict(DBusMessageIter *iter_dict,
struct ldb_message_element *el);#endif /* _IFPSRV_PRIVATE_H_ */
//snip
From 533aa8adef72fb5fa5cece55059ecfe1d1f030b9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 15 Apr 2014 15:42:15 +0200 Subject: [PATCH 6/6] IFP: Add GetUserAttrs call
Adds a DBus method that allows the caller to retrieve attributes of a user. The synopsis of the call is as follows:
<method name="GetUserAttr"> <arg type="s" name="user" direction="in"/> <arg type="as" name="attr" direction="in"/> <arg type="a{sv}" name="values" direction="out"/> </method>
The return value is an array (one attribute per array member) of dictionaries. The key of the dictionary is the attribute name, the value is a variant containing the attribute values as strings.
If an attribute does not exist or is not permitted to be read, no error is returned. If the users does not exist, the method returns an error.
In future patches this function will be marked as obsolete in favor of object-oriented approach.
ifp_user_get_attr_unpack_msg is a separate function to allow extending it in a later patch.
The function to check the cache validity duplicates quite a bit of code with the NSS responder. The refactoring would be nice to get done along with #843.
There are warnings introduced in this last patch.
src/responder/ifp/ifpsrv_cmd.c: In function ‘ifp_user_get_attr_process’: src/responder/ifp/ifpsrv_cmd.c:174:38: warning: ‘res’ may be used uninitialized in this function [-Wmaybe-uninitialized] el = ldb_msg_find_element(res->msgs[0], attrs[ai]); ^ src/responder/ifp/ifpsrv_cmd.c:119:24: note: ‘res’ was declared here struct ldb_result *res; ^
And some in tests.
src/tests/cmocka/test_ifp.c: In function ‘test_el_to_dict’: src/tests/cmocka/test_ifp.c:186:26: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual] el->values[0].data = (uint8_t *) "one"; ^ src/tests/cmocka/test_ifp.c:188:26: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual] el->values[1].data = (uint8_t *) "two";
LS
On Thu, Apr 24, 2014 at 01:35:09PM +0200, Lukas Slebodnik wrote:
+if BUILD_IFP +ifp_tests_DEPENDENCIES = \
^^^^ Use prefix EXTRA_ if you need memberof pluging for this test. Otherwise, it can break parallel build. We have this prefix also on other places.
This was a copy-n-paste error, I think. This test doesn't need the memberof plugin, so I simply removed the dependencies.
$(ldblib_LTLIBRARIES)+ifp_tests_SOURCES = \
$(TEST_MOCK_RESP_OBJ) \- src/tests/cmocka/test_ifp.c \
- src/responder/ifp/ifpsrv_cmd.c \
- src/responder/ifp/ifpsrv_util.c
+ifp_tests_CFLAGS = \
- $(AM_CFLAGS)
+ifp_tests_LDFLAGS = \
- -Wl,-wrap,sbus_conn_send_reply \
- -Wl,-wrap,dbus_message_get_sender \
- -Wl,-wrap,dbus_bus_get_unix_user
+ifp_tests_LDADD = \
- $(CMOCKA_LIBS) \
- $(SSSD_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la
endif
[...]
There are warnings introduced in this last patch.
Thanks, I haven't seen the warnings with clang. I can see the one in tests with gcc, but not the first one though..did you compile with optimizations?
src/responder/ifp/ifpsrv_cmd.c: In function ‘ifp_user_get_attr_process’: src/responder/ifp/ifpsrv_cmd.c:174:38: warning: ‘res’ may be used uninitialized in this function [-Wmaybe-uninitialized] el = ldb_msg_find_element(res->msgs[0], attrs[ai]); ^ src/responder/ifp/ifpsrv_cmd.c:119:24: note: ‘res’ was declared here struct ldb_result *res; ^
I think the warning is mostly spurious, I don't think we can reach the response with undefined 'res'. But I added an extra check anyway, doesn't hurt to be too defensive.
And some in tests.
src/tests/cmocka/test_ifp.c: In function ‘test_el_to_dict’: src/tests/cmocka/test_ifp.c:186:26: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual] el->values[0].data = (uint8_t *) "one"; ^ src/tests/cmocka/test_ifp.c:188:26: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual] el->values[1].data = (uint8_t *) "two";
Fixed, thanks.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Stef Walter -
Stef Walter