[PATCHES] IFP: add FindByCertificate method for User objects

[Sumit Bose]

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

[Pavel Březina]

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

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:
> 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))

[Sumit Bose]

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
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

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:

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;

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?
>    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)?
> 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 = ...
}
> --- 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.

[Sumit Bose]

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

[Pavel Březina]

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

[Pavel Březina]

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.

[Jakub Hrozek]

On Thu, Jun 18, 2015 at 09:04:48AM +0200, Pavel Březina wrote:
> > > 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.

Sorry, but the tests don't compile for me:
/home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c: In function ‘test_sysdb_search_user_by_cert’:
/home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:16: error: implicit declaration of function ‘sss_base64_decode’ [-Werror=implicit-function-declaration]
     val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
                ^
/home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:14: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);

[Sumit Bose]

On Thu, Jun 18, 2015 at 05:09:22PM +0200, Jakub Hrozek wrote:
> On Thu, Jun 18, 2015 at 09:04:48AM +0200, Pavel Březina wrote:
> > > > 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.
>
> Sorry, but the tests don't compile for me:
> /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c: In function ‘test_sysdb_search_user_by_cert’:
> /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:16: error: implicit declaration of function ‘sss_base64_decode’ [-Werror=implicit-function-declaration]
>      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
>                 ^
> /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:14: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
>      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);

not sure why I (and CI) don't see this, but

diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index 6dd299b..522a44a 100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -27,6 +27,7 @@
 #include <sys/stat.h>
 #include <sys/types.h>
 #include "util/util.h"
+#include "util/crypto/sss_crypto.h"
 #include "confdb/confdb_setup.h"
 #include "db/sysdb_private.h"
 #include "db/sysdb_services.h"

should fix it. Can you try?

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Thu, Jun 18, 2015 at 05:35:54PM +0200, Sumit Bose wrote:
> On Thu, Jun 18, 2015 at 05:09:22PM +0200, Jakub Hrozek wrote:
> > On Thu, Jun 18, 2015 at 09:04:48AM +0200, Pavel Březina wrote:
> > > > > 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.
> >
> > Sorry, but the tests don't compile for me:
> > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c: In function ‘test_sysdb_search_user_by_cert’:
> > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:16: error: implicit declaration of function ‘sss_base64_decode’ [-Werror=implicit-function-declaration]
> >      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
> >                 ^
> > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:14: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
> >      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
>
> not sure why I (and CI) don't see this, but
>
> diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
> index 6dd299b..522a44a 100644
> --- a/src/tests/sysdb-tests.c
> +++ b/src/tests/sysdb-tests.c
> @@ -27,6 +27,7 @@
>  #include <sys/stat.h>
>  #include <sys/types.h>
>  #include "util/util.h"
> +#include "util/crypto/sss_crypto.h"
>  #include "confdb/confdb_setup.h"
>  #include "db/sysdb_private.h"
>  #include "db/sysdb_services.h"
>
> should fix it. Can you try?

Yes it does, thanks. FWIW, I'm using GCC on F-22.

I've squashed the one-liner into "sysdb: add sysdb_search_user_by_cert()
and sysdb_search_object_by_cert()" and submitted the patches to CI and
Coverity again, just to be sure.

[Jakub Hrozek]

On Thu, Jun 18, 2015 at 05:51:08PM +0200, Jakub Hrozek wrote:
> On Thu, Jun 18, 2015 at 05:35:54PM +0200, Sumit Bose wrote:
> > On Thu, Jun 18, 2015 at 05:09:22PM +0200, Jakub Hrozek wrote:
> > > On Thu, Jun 18, 2015 at 09:04:48AM +0200, Pavel Březina wrote:
> > > > > > 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.
> > >
> > > Sorry, but the tests don't compile for me:
> > > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c: In function ‘test_sysdb_search_user_by_cert’:
> > > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:16: error: implicit declaration of function ‘sss_base64_decode’ [-Werror=implicit-function-declaration]
> > >      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
> > >                 ^
> > > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:14: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
> > >      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
> >
> > not sure why I (and CI) don't see this, but
> >
> > diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
> > index 6dd299b..522a44a 100644
> > --- a/src/tests/sysdb-tests.c
> > +++ b/src/tests/sysdb-tests.c
> > @@ -27,6 +27,7 @@
> >  #include <sys/stat.h>
> >  #include <sys/types.h>
> >  #include "util/util.h"
> > +#include "util/crypto/sss_crypto.h"
> >  #include "confdb/confdb_setup.h"
> >  #include "db/sysdb_private.h"
> >  #include "db/sysdb_services.h"
> >
> > should fix it. Can you try?
>
> Yes it does, thanks. FWIW, I'm using GCC on F-22.
>
> I've squashed the one-liner into "sysdb: add sysdb_search_user_by_cert()
> and sysdb_search_object_by_cert()" and submitted the patches to CI and
> Coverity again, just to be sure.

Coverity came clean, but CI was giving me some issues with rpmbuild and
Debian builds. The attached interdiff takes care of the problems at
least locally. I submitted the changes to CI and would like to push the
patches if the results are OK.

[Sumit Bose]

On Fri, Jun 19, 2015 at 06:56:49PM +0200, Jakub Hrozek wrote:
> On Thu, Jun 18, 2015 at 05:51:08PM +0200, Jakub Hrozek wrote:
> > On Thu, Jun 18, 2015 at 05:35:54PM +0200, Sumit Bose wrote:
> > > On Thu, Jun 18, 2015 at 05:09:22PM +0200, Jakub Hrozek wrote:
> > > > On Thu, Jun 18, 2015 at 09:04:48AM +0200, Pavel Březina wrote:
> > > > > > >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.
> > > >
> > > > Sorry, but the tests don't compile for me:
> > > > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c: In function ‘test_sysdb_search_user_by_cert’:
> > > > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:16: error: implicit declaration of function ‘sss_base64_decode’ [-Werror=implicit-function-declaration]
> > > >      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
> > > >                 ^
> > > > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:14: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
> > > >      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
> > >
> > > not sure why I (and CI) don't see this, but
> > >
> > > diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
> > > index 6dd299b..522a44a 100644
> > > --- a/src/tests/sysdb-tests.c
> > > +++ b/src/tests/sysdb-tests.c
> > > @@ -27,6 +27,7 @@
> > >  #include <sys/stat.h>
> > >  #include <sys/types.h>
> > >  #include "util/util.h"
> > > +#include "util/crypto/sss_crypto.h"
> > >  #include "confdb/confdb_setup.h"
> > >  #include "db/sysdb_private.h"
> > >  #include "db/sysdb_services.h"
> > >
> > > should fix it. Can you try?
> >
> > Yes it does, thanks. FWIW, I'm using GCC on F-22.
> >
> > I've squashed the one-liner into "sysdb: add sysdb_search_user_by_cert()
> > and sysdb_search_object_by_cert()" and submitted the patches to CI and
> > Coverity again, just to be sure.
>
> Coverity came clean, but CI was giving me some issues with rpmbuild and
> Debian builds. The attached interdiff takes care of the problems at
> least locally. I submitted the changes to CI and would like to push the
> patches if the results are OK.

The changes make sense, please squash them in if the tests are passing.

bye,
Sumit
> diff -u b/Makefile.am b/Makefile.am
> --- b/Makefile.am
> +++ b/Makefile.am
> @@ -737,7 +737,9 @@
>      $(NULL)
>  libsss_cert_la_LIBADD = \
>      $(CRYPTO_LIBS) \
> $(TALLOC_LIBS) \
> libsss_crypt.la \
> libsss_debug.la \
> $(NULL)
>
> libsss_cert_la_LDFLAGS = \
>      -avoid-version \
> @@ -2514,14 +2516,17 @@
>      $(NULL)
>  test_cert_utils_CFLAGS = \
>      $(AM_CFLAGS) \
> $(CRYPTO_CFLAGS) \
> $(NULL)
>
> test_cert_utils_LDADD = \
>      $(CMOCKA_LIBS) \
>      $(POPT_LIBS) \
>      $(TALLOC_LIBS) \
> $(CRYPTO_LIBS) \
> libsss_debug.la \
> libsss_test_common.la \
> libsss_cert.la \
> libsss_crypt.la \
> $(NULL)
>
> endif # HAVE_CMOCKA
>
> only in patch2:
> unchanged:
> --- a/contrib/ci/deps.sh
> +++ b/contrib/ci/deps.sh
> @@ -105,6 +105,7 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then
>          systemd
>          xml-core
>          xsltproc
>    libssl-dev
> )
>    DEPS_INTGCHECK_SATISFIED=false
>
> fi
> only in patch2:
> unchanged:
> --- a/contrib/sssd.spec.in
> +++ b/contrib/sssd.spec.in
> @@ -106,6 +106,7 @@ BuildRequires: dbus-libs
>  BuildRequires: openldap-devel
>  BuildRequires: pam-devel
>  BuildRequires: nss-devel
> +BuildRequires: openssl-devel
>  BuildRequires: nspr-devel
>  BuildRequires: pcre-devel
>  BuildRequires: libxslt
> @@ -691,6 +692,7 @@ rm -rf $RPM_BUILD_ROOT
>  #Internal shared libraries
>  %{_libdir}/%{name}/libsss_child.so
>  %{_libdir}/%{name}/libsss_crypt.so
> +%{_libdir}/%{name}/libsss_cert.so
>  %{_libdir}/%{name}/libsss_debug.so
>  %{_libdir}/%{name}/libsss_krb5_common.so
>  %{_libdir}/%{name}/libsss_ldap_common.so
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Fri, Jun 19, 2015 at 08:14:58PM +0200, Sumit Bose wrote:
> On Fri, Jun 19, 2015 at 06:56:49PM +0200, Jakub Hrozek wrote:
> > On Thu, Jun 18, 2015 at 05:51:08PM +0200, Jakub Hrozek wrote:
> > > On Thu, Jun 18, 2015 at 05:35:54PM +0200, Sumit Bose wrote:
> > > > On Thu, Jun 18, 2015 at 05:09:22PM +0200, Jakub Hrozek wrote:
> > > > > On Thu, Jun 18, 2015 at 09:04:48AM +0200, Pavel Březina wrote:
> > > > > > >>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.
> > > > >
> > > > > Sorry, but the tests don't compile for me:
> > > > > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c: In function ‘test_sysdb_search_user_by_cert’:
> > > > > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:16: error: implicit declaration of function ‘sss_base64_decode’ [-Werror=implicit-function-declaration]
> > > > >      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
> > > > >                 ^
> > > > > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:14: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
> > > > >      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
> > > >
> > > > not sure why I (and CI) don't see this, but
> > > >
> > > > diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
> > > > index 6dd299b..522a44a 100644
> > > > --- a/src/tests/sysdb-tests.c
> > > > +++ b/src/tests/sysdb-tests.c
> > > > @@ -27,6 +27,7 @@
> > > >  #include <sys/stat.h>
> > > >  #include <sys/types.h>
> > > >  #include "util/util.h"
> > > > +#include "util/crypto/sss_crypto.h"
> > > >  #include "confdb/confdb_setup.h"
> > > >  #include "db/sysdb_private.h"
> > > >  #include "db/sysdb_services.h"
> > > >
> > > > should fix it. Can you try?
> > >
> > > Yes it does, thanks. FWIW, I'm using GCC on F-22.
> > >
> > > I've squashed the one-liner into "sysdb: add sysdb_search_user_by_cert()
> > > and sysdb_search_object_by_cert()" and submitted the patches to CI and
> > > Coverity again, just to be sure.
> >
> > Coverity came clean, but CI was giving me some issues with rpmbuild and
> > Debian builds. The attached interdiff takes care of the problems at
> > least locally. I submitted the changes to CI and would like to push the
> > patches if the results are OK.
>
> The changes make sense, please squash them in if the tests are passing.
>
> bye,
> Sumit

The CI run with the interdiff I squashed in (and Lukas helped me with,
thanks!) succeeded:
    http://sssd-ci.duckdns.org/logs/job/17/95/summary.html

ACK again.

[Lukas Slebodnik]

On (18/06/15 17:35), Sumit Bose wrote:
> On Thu, Jun 18, 2015 at 05:09:22PM +0200, Jakub Hrozek wrote:
> > On Thu, Jun 18, 2015 at 09:04:48AM +0200, Pavel Březina wrote:
> > > > > 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.
> >
> > Sorry, but the tests don't compile for me:
> > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c: In function ‘test_sysdb_search_user_by_cert’:
> > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:16: error: implicit declaration of function ‘sss_base64_decode’ [-Werror=implicit-function-declaration]
> >      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
> >                 ^
> > /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:14: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
> >      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
>
> not sure why I (and CI) don't see this, but
>
> diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
> index 6dd299b..522a44a 100644
> --- a/src/tests/sysdb-tests.c
> +++ b/src/tests/sysdb-tests.c
> @@ -27,6 +27,7 @@
> #include <sys/stat.h>
> #include <sys/types.h>
> #include "util/util.h"
> +#include "util/crypto/sss_crypto.h"
> #include "confdb/confdb_setup.h"
> #include "db/sysdb_private.h"
> #include "db/sysdb_services.h"
>
> should fix it. Can you try?

Sumit,

try to build old version of patch on your machine with
additional compiler flag "-ftrack-macro-expansion=0"
I hope it helps.

LS

[Jakub Hrozek]

On Thu, Jun 18, 2015 at 05:09:22PM +0200, Jakub Hrozek wrote:
> On Thu, Jun 18, 2015 at 09:04:48AM +0200, Pavel Březina wrote:
> > > > 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.
>
> Sorry, but the tests don't compile for me:
> /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c: In function ‘test_sysdb_search_user_by_cert’:
> /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:16: error: implicit declaration of function ‘sss_base64_decode’ [-Werror=implicit-function-declaration]
>      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);
>                 ^
> /home/remote/jhrozek/devel/sssd/src/tests/sysdb-tests.c:5218:14: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
>      val.data = sss_base64_decode(test_ctx, TEST_USER_CERT_DERB64, &val.length);

* master:
    * 827a016a07d5f911cc4195be89896a376fd71f59
    * a99845006f96f9d1e7af871ec67c71cee8408a62
    * 8d4dedea12e2b71f83a1b0e5f0fc5cdb706dcf98
    * caacea0dbfdc92613ae992681053b1d2665b80ca
    * 7d8b7d82f0a91ed656320577fc781f24a66db9f8
    * bf01e8179cbb2be476805340636098deda7e1366
    * e22e04517b9f9d0c7759dc4768eedfd05908e9b6
    * 070bb515321a7de091b884d9e0ab357b7b5ae578