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