[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