[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