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

Pavel Březina pbrezina at redhat.com
Wed Jun 17 09:52:55 UTC 2015


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




More information about the sssd-devel mailing list