[SSSD] [PATCHES] IFP: add FindByCertificate method for User objects

Pavel Březina pbrezina at redhat.com
Tue Jun 16 08:44:35 UTC 2015


On 06/11/2015 01:08 PM, Sumit Bose wrote:
> On Thu, Jun 04, 2015 at 12:31:34PM +0200, Pavel Březina wrote:
>> On 06/04/2015 11:06 AM, Sumit Bose wrote:
>>> Hi,
>>>
>>> this patch-set aims to solve https://fedorahosted.org/sssd/ticket/2596.
>>> The first patch is unrelated but needed to make the changes in the
>>> second patch properly aligned.
>>>
>>> Patches 3,4 and 6 add some certificate related utilities while patch 5
>>> adds the backend changes and patch 7 the changes for InfoPipe.
>>>
>>> bye,
>>> Sumit
>
> Hi Pavel,
>
> thank you for the review.
>
>>
>>> +errno_t sysdb_search_object_by_cert(TALLOC_CTX *mem_ctx,
>>> +                                    struct sss_domain_info *domain,
>>> +                                    const char *cert,
>>> +                                    const char **attrs,
>>> +                                    struct ldb_result **res)
>>> +{
>>> +    int ret;
>>> +    char *user_filter;
>>> +
>>> +    ret = sss_cert_derb64_to_ldap_filter(mem_ctx, cert, SYSDB_USER_CERT,
>>> +                                         &user_filter);
>>> +    if (ret != EOK) {
>>> +        DEBUG(SSSDBG_OP_FAILURE, "sss_cert_derb64_to_ldap_filter failed.\n");
>>> +        return ret;
>>> +    }
>>> +
>>              vv two spaces here
>
> fixed
>
>>> +    ret =  sysdb_search_object_by_str_attr(mem_ctx, domain,
>>> +                                           SYSDB_USER_CERT_FILTER,
>>> +                                           user_filter, attrs, res);
>>> +    talloc_free(user_filter);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +errno_t sysdb_search_user_by_cert(TALLOC_CTX *mem_ctx,
>>> +                                  struct sss_domain_info *domain,
>>> +                                  const char *cert,
>>> +                                  struct ldb_result **res)
>>> +{
>>> +    const char *user_attrs[] = SYSDB_PW_ATTRS;
>>> +
>>> +    return  sysdb_search_object_by_cert(mem_ctx, domain, cert, user_attrs, res);
>>               ^^ two spaces here
>
> fixed
>
>>> +}
>>
>> cache_req:
>>> search_str = state->input->type == CACHE_REQ_USER_BY_CERT ?
>>>                             state->input->orig_name : state->input->dom_objname;
>>
>> I think it will be better to create a new field in cache_req_input, say
>> 'cert', to not abuse fields that resemble object names. The code will be
>> cleaner and you can get rid of few parts like:
>
> I added cert.
>
>>
>>>     if (state->input->orig_name != NULL && domain == NULL
>>>             && (input->type == CACHE_REQ_USER_BY_NAME
>>>                 || input->type == CACHE_REQ_GROUP_BY_NAME
>>>                 || input->type == CACHE_REQ_INITGROUPS))
>
> new version attached.
>
> bye,
> Sumit

Hi,
I rebased it on top of current master (just trivial Makefile.am 
conflicts) and fixed some indentation. Diff:

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 3aa7099..6d0aede 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -3720,8 +3720,8 @@ errno_t sysdb_search_object_by_cert(TALLOC_CTX 
*mem_ctx,
      }

      ret = sysdb_search_object_by_str_attr(mem_ctx, domain,
-                                           SYSDB_USER_CERT_FILTER,
-                                           user_filter, attrs, res);
+                                          SYSDB_USER_CERT_FILTER,
+                                          user_filter, attrs, res);
      talloc_free(user_filter);

      return ret;

Ack to all patches but the last.

> @@ -82,6 +83,17 @@ cache_req_input_create(TALLOC_CTX *mem_ctx,
>              goto fail;
>          }
>          break;
> +    case CACHE_REQ_USER_BY_CERT:
> +        if (name == NULL) {
> +            DEBUG(SSSDBG_CRIT_FAILURE, "Bug: certificate cannot be NULL!\n");
> +            goto fail;
> +        }

Can you create a parameter 'cert' instead of abusing 'name' here?

> +
> +        input->cert = talloc_strdup(input, name);
> +        if (input->cert == NULL) {
> +            goto fail;
> +        }
> +        break;

> +    case CACHE_REQ_USER_BY_CERT:
> +        fqn = talloc_asprintf(tmp_ctx, "CERT:%s@%s", input->cert, domain->name);
> +        if (fqn == NULL) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +        break;

The 'fqn' is used for debugging purpose. How large can string 
'input->cert' get? I'm a little worried that it will just spam log files 
with long string noone will ever read. Wouldn't it suffice to just print 
few first/middle/last characters (or combined)?

> +    search_str = (state->input->type == CACHE_REQ_USER_BY_CERT) ?
> +                                 state->input->cert : state->input->dom_objname;

Since you have to break the line anyway, the following may read better, 
what do you think?

search_str = state->input->dom_objname;
if (state->input->type == CERT) {
     search_str = ...
}

> --- a/src/sbus/sssd_dbus_errors.h
> +++ b/src/sbus/sssd_dbus_errors.h
> @@ -25,5 +25,6 @@
>
>  #define SBUS_ERROR_INTERNAL "org.freedesktop.sssd.Error.Internal"
>  #define SBUS_ERROR_NOT_FOUND "org.freedesktop.sssd.Error.NotFound"
> +#define SBUS_ERROR_EINVAL "org.freedesktop.sssd.Error.InvalidArgument"

D-Bus error for EINVAL already exist: DBUS_ERROR_INVALID_ARGS. I think 
we should stick to standard ones when applicable.



-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-adding-ldap_user_auth_type-where-missing.patch
Type: text/x-patch
Size: 3265 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150616/6d513138/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-LDAP-add-ldap_user_certificate-option.patch
Type: text/x-patch
Size: 8175 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150616/6d513138/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-certs-add-PEM-DER-conversion-utilities.patch
Type: text/x-patch
Size: 30772 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150616/6d513138/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-sysdb-add-sysdb_search_user_by_cert-and-sysdb_search.patch
Type: text/x-patch
Size: 8003 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150616/6d513138/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-LDAP-IPA-add-user-lookup-by-certificate.patch
Type: text/x-patch
Size: 8645 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150616/6d513138/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-ncache-add-calls-for-certificate-based-searches.patch
Type: text/x-patch
Size: 7297 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150616/6d513138/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-IFP-add-FindByCertificate-method-for-User-objects.patch
Type: text/x-patch
Size: 20423 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150616/6d513138/attachment-0006.bin>


More information about the sssd-devel mailing list