[PATCH 2/2] s4-openldap: Optionally compile libcli/security as a public library to be used by openldap

Nadezhda Ivanova nivanova at samba.org
Thu Dec 12 11:44:37 MST 2013


Hi Jelmer,
By changing I mean that more functions will be exposed through the public
header, rather than changing the infrastructure. However if you have
misgivings I will re-submit the patch again when I have tested it as the
default behavior for samba. I would also appreciate others' opinion on what
else needs to be done before the patch is acceptable.

Do you have any objections to the first half of the changes (the one that
just moves functions to libcli/security), or are your objections only to
this half? Even if this one is postponed, it would be nice to have the
first patch in the tree in case any changes to the descriptor code are
necessary before this one takes effect.

Regards,
Nadya


On Thu, Dec 12, 2013 at 8:22 PM, Jelmer Vernooij <jelmer at samba.org> wrote:

> On Thu, Dec 12, 2013 at 07:34:08PM +0200, Nadezhda Ivanova wrote:
> > It is not the final version at this point, I just think it will be more
> > convenient to have it in master so other people (mostly Howard:)) will
> not
> > have to work with wip branches and merge. I can't think of a reason why
> it
> > cannot be made the default way later, when it is better tested is not
> > expected to change. Currently as it is not the default, it will not
> really
> > affect samba without OpenLDAP.
> If it's not ready yet, then it should probably not go into master in a way
> that is
> exposed to the user. At the very least the library infrastructure and
> naming
> should be fixed, since that may save us some pain in the future and is not
> a
> lot of effort. Getting the name and headers right now will also mean that
> Howard won't have to change his code later on to use a new name.
>
> Cheers,
>
> Jelmer
>
> > On Thu, Dec 12, 2013 at 7:25 PM, Jelmer Vernooij <jelmer at samba.org>
> wrote:
> >
> > > Hi Nadezhda,
> > >
> > > On Thu, Dec 12, 2013 at 06:57:44PM +0200, Nadezhda Ivanova wrote:
> > > > A continuation of the previous patch, this one adds an option to the
> > > > configure script to compile libcli/security as a library that can be
> used
> > > > by OpenLdap. This is not the final version of the interface, more
> > > functions
> > > > will be added as the access checks are posted to OpenLDAP.
> > >
> > > > From 5f789901941f29a1ddfa444bfcdc5f824e21860f Mon Sep 17 00:00:00
> 2001
> > > > From: Nadezhda Ivanova <nivanova at symas.com>
> > > > Date: Thu, 12 Dec 2013 18:40:41 +0200
> > > > Subject: [PATCH 2/2] s4-openldap: Optionally compile libcli/security
> as a
> > > >  public library to be used by openldap
> > > Mostly looks good to me, some minor comments below:
> > >
> > > > diff --git a/libcli/security/samba_security.h
> > > b/libcli/security/samba_security.h
> > > > new file mode 100644
> > > > index 0000000..880cc32
> > > > --- /dev/null
> > > > +++ b/libcli/security/samba_security.h
> > >
> > > samba_security.h is a very generic name; it looks like this is
> > > create_descriptor.h?
> > >
> > > Perhaps add a separate header file that includes
> > > <security/create_descriptor.h> and possibly other future public header
> > > files from libsamba-security.
> > >
> > > > @@ -0,0 +1,32 @@
> > > > +#ifndef _LIBCLI_SAMBA_SECURITY_SECURITY_H_
> > > > +#define _LIBCLI_SAMBA_SECURITY_SECURITY_H_
> > > > +#include <stdbool.h>
> > > > +#include <stdint.h>
> > > > +#include <talloc.h>
> > > > +#include "lib/util/data_blob.h"
> > > > +
> > > > +typedef enum {
> > > > +     CR_DESCR_INVALID = 0,
> > > > +     CR_DESCR_SCHEMA,
> > > > +     CR_DESCR_CONFIG,
> > > > +     CR_DESCR_DEFAULT,
> > > > +     CR_DESCR_OTHER
> > > > +} CR_DESCR_PARTITION;
> > > > +
> > > > +DATA_BLOB *cr_descr_get_descriptor_to_show(TALLOC_CTX *mem_ctx,
> > > > +                                        const DATA_BLOB *sd,
> > > > +                                        uint32_t sd_flags);
> > > > +
> > > > +DATA_BLOB *cr_descr_get_new_descriptor_as_blob(TALLOC_CTX *mem_ctx,
> > > > +                                            const DATA_BLOB
> *blob_token,
> > > > +                                            const DATA_BLOB
> > > *blob_domain_sid,
> > > > +                                            const char
> > > *defaultSecurityDescriptor,
> > > > +                                            const DATA_BLOB
> > > *schemaIDGUID,
> > > > +                                            const DATA_BLOB *parent,
> > > > +                                            const DATA_BLOB *object,
> > > > +                                            const DATA_BLOB *old_sd,
> > > > +                                            CR_DESCR_PARTITION
> > > partition,
> > > > +                                            uint32_t sd_flags);
> > > > +
> > > > +
> > > > +#endif
> > >
> > > > diff --git a/libcli/security/security.pc.in b/libcli/security/
> > > security.pc.in
> > > > new file mode 100644
> > > > index 0000000..2c33d67
> > > > --- /dev/null
> > > > +++ b/libcli/security/security.pc.in
> > > ^ This should be samba-security.pc.in, as it's going to be installed
> in
> > > the
> > > system pkg-config path on distributions, where the name 'security' is
> > > fairly
> > > ambiguous.
> > >
> > > > @@ -0,0 +1,10 @@
> > > > +prefix=@prefix@
> > > > +exec_prefix=@exec_prefix@
> > > > +libdir=@libdir@
> > > > +includedir=@includedir@
> > > > +
> > > > +Name: samba-security
> > > > +Description: A collection of samba access check functions
> > > This description doesn't seem to cover everything that is in
> > > libsamba-security.so,
> > > only what is in the current header file.
> > >
> > > > +Version: @PACKAGE_VERSION@
> > > > +Libs: @LIB_RPATH@ -L${libdir} -lsamba-security
> > > > +Cflags: -I${includedir}
> > > > \ No newline at end of file
> > > > diff --git a/libcli/security/wscript_build
> > > b/libcli/security/wscript_build
> > > > index b529ec8..7f20902 100644
> > > > --- a/libcli/security/wscript_build
> > > > +++ b/libcli/security/wscript_build
> > > > @@ -1,11 +1,28 @@
> > > >  #!/usr/bin/env python
> > > >
> > > > +VERSION = '0.0.1'
> > > > +if bld.CONFIG_SET('AD_OPENLDAP_DEPS_ENABLED'):
> > > > +    vnum = VERSION
> > > > +    private_library=False
> > > style nit: please add spaces around = in assignments.
> > >
> > > > +    pc_files='security.pc'
> > > > +    includes='include'
> > > > +    public_headers='samba_security.h'
> > > > +else:
> > > > +    vnum = None
> > > > +    private_library=True
> > > > +    pc_files=None
> > > > +    includes=''
> > > > +    public_headers=None
> > > Building samba-security as a public library optionally is going to
> > > increase the number of
> > > configurations in which Samba can be found. Is there a particular
> reason
> > > for not wanting this
> > > as a public library in some cases?
> > >
> > > Cheers,
> > >
> > > Jelmer
> > >
>
> --
>


More information about the samba-technical mailing list