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.
>
>> +
>> + 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