[SSSD] [PATCH] AD: LDAP component of GPO-based access control

Jakub Hrozek jhrozek at redhat.com
Mon Apr 28 19:24:57 UTC 2014


On Mon, Apr 28, 2014 at 03:13:36PM -0400, Yassir Elley wrote:
> > > + done:
> > > +
> > > +    if (ret == EOK) {
> > > +        tevent_req_done(req);
> > 
> > Can we even have EOK here? Looks like the functions either return EAGAIN
> > or outright failure..
> 
> ad_gpo_get_som_attrs_step() returns EOK after all SOMs have been processed. There is a comment to that effect in the code.

Ah, my bad, sorry. And thanks for explanation!

> 
> > 
> > > +    } else if (ret != EAGAIN) {
> > > +        tevent_req_error(req, ret);
> > > +    }
> > > +
> > > +}
> > > +static errno_t
> > > +ad_gpo_get_som_attrs_step(struct tevent_req *req)
> > > +{
> > > +    const char *attrs[] = {AD_AT_GPLINK, AD_AT_GPOPTIONS, NULL};
> > > +    struct tevent_req *subreq;
> > > +    struct ad_gpo_process_som_state *state;
> > > +
> > > +    state = tevent_req_data(req, struct ad_gpo_process_som_state);
> > > +
> > > +    state->som_index++;
> > > +    struct gp_som *gp_som = state->som_list[state->som_index];
> > > +
> > > +    /* gp_som is NULL only after all SOMs have been processed */
> > > +    if (gp_som == NULL) return EOK;
> > > +
> > > +    char *som_dn = gp_som->som_dn;
> > > +    subreq = sdap_get_generic_send(state, state->ev,  state->opts,
> > > +                                   sdap_id_op_handle(state->sdap_op),
> > > +                                   som_dn, LDAP_SCOPE_BASE,
> > > +                                   "(objectclass=*)", attrs, NULL, 0,
> > > +                                   state->timeout,
> > > +                                   false);
> > > +
> > > +    if (subreq == NULL) {
> > > +        DEBUG(SSSDBG_OP_FAILURE, "sdap_get_generic_send failed.\n");
> > > +        return ENOMEM;
> > > +    }
> > > +
> > > +    tevent_req_set_callback(subreq, ad_gpo_get_som_attrs_done, req);
> > > +    return EAGAIN;
> > > +}
> > 
> > [...]
> > 
> > > diff --git a/src/util/sss_ldap.h b/src/util/sss_ldap.h
> > > index
> > > f298b2fbb30cf1532f8e94504ffb83ef73880b81..083c89141987f38313906d7d7a9a003a74c10cef
> > > 100644
> > > --- a/src/util/sss_ldap.h
> > > +++ b/src/util/sss_ldap.h
> > > @@ -55,6 +55,15 @@ int sss_ldap_get_diagnostic_msg(TALLOC_CTX *mem_ctx,
> > >  #define LDAP_SERVER_ASQ_OID "1.2.840.113556.1.4.1504"
> > >  #endif /* LDAP_SERVER_ASQ_OID */
> > >  
> > > +#ifndef LDAP_SERVER_SD_OID
> > > +#define LDAP_SERVER_SD_OID "1.2.840.113556.1.4.801"
> > > +#endif /* LDAP_SERVER_SD_OID */
> > > +
> > > +#define SECINFO_OWNER ( 0x00000001 )
> > > +#define SECINFO_GROUP ( 0x00000002 )
> > > +#define SECINFO_DACL ( 0x00000004 )
> > > +#define SECINFO_SACL ( 0x00000008 )
> > > +
> > 
> > Of all these constants, only SECINFO_DACL is used, is the rest included
> > for posterity? Do these magic values come from some technet article or
> > MSDN documentation? If so, it might be nice to add a link or a comment.
> 
> These four flags indicate which security descriptor parts to retrieve during a search. They are defined at http://msdn.microsoft.com/en-us/library/aa366987(v=vs.85).aspx. While we only use a single flag (SECINFO_DACL), I thought I would include all four flags for completeness. Should I remove the other three flags that are not being used?

No, I agree it's better to include them all. If you're changing the
patch anyway, can you put a comment above the constants?

> 
> > 
> > >  int sss_ldap_control_create(const char *oid, int iscritical,
> > >                              struct berval *value, int dupval,
> > >                              LDAPControl **ctrlp);
> > > --
> > > 1.8.5.3
> > > 
> > 
> > In general, it's really nice work, thank you!
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > 
> 
> I will now work on a new patch to address the comments you have raised.

Thank you for the patience during review!



More information about the sssd-devel mailing list