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

Pavel Březina pbrezina at redhat.com
Thu Jun 18 07:04:48 UTC 2015


On 06/17/2015 11:52 AM, Pavel Březina wrote:
> On 06/16/2015 05:20 PM, Sumit Bose wrote:
>> On Tue, Jun 16, 2015 at 10:44:35AM +0200, Pavel Březina wrote:
>>> 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:
>>
>> yes, I aldready had this in my tree but forgot to send it.
>>
>>>
>>> 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;
>>
>> thanks, added
>>
>>>
>>> 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?
>>
>> ok, I added it, but maybe in the long when we add searches by SIDs or
>> email addresses something more flexible, maybe a union would be useful
>> here.
>
> Yes, this is a good idea.
>
>>
>>>
>>>> +
>>>> +        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)?
>>
>> I added a small utility which returns the last x characters of a string
>> because I think the last characters are more unique here then the first
>> ones.
>>
>>>
>>>> +    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 = ...
>>> }
>>
>> replaced
>>
>>>
>>>> --- 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.
>>
>> replaced
>>
>> Thank you for the review, new version attached.
>>
>> bye,
>> Sumit
>
>
> ACK

Those patches needs to be pushed *after* cached object patches land in 
since they depend on them -- even though they can be (wrongly) applied 
to master.




More information about the sssd-devel mailing list