[PATCH] some cleanups for smbldap.c

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
Hi!

The main focus is to make "struct smbldap_state" private to smbldap.c
for better encapsulation.

Review appreciated!

Thanks, Volker

patch.txt (78K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On Wed, Apr 19, 2017 at 04:20:36PM +0200, vl--- via samba-technical wrote:
> The main focus is to make "struct smbldap_state" private to smbldap.c
> for better encapsulation.

+1 for the encapsulation but can we possibly use a different name then
smbldap_get_ld() ? The ld makes it look like you could get a linker out of
smbldap. :)

Maybe smbldap_get_ldap() ?

If you don't agree and would like to stick with smbldap_get_ld() please go ahead
and push with my rb.

If you agree but wonder how to rename without revisiting every file, sed inplace
would do the trick, so

$ find . -name '*.c' | xargs sed -i s/smbldap_get_ld/smbldap_get_ldap/g
$ find . -name '*.h' | xargs sed -i s/smbldap_get_ld/smbldap_get_ldap/g

If you want you can offload the task, I can do it, just tell me I should go
ahead and push with this change.

-slow

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On ke, 19 huhti 2017, vl--- via samba-technical wrote:
> Hi!
>
> The main focus is to make "struct smbldap_state" private to smbldap.c
> for better encapsulation.
>
> Review appreciated!
Looks good to me. This will break ipasam in FreeIPA but I already
started moving it to own private struct as 'struct ldapsam_privates' is
not accessible anymore and I neeed anyway to implement some more of a
backend code that needs own proper private struct. Updating it to use
your wrappers is not a problem.

Please do not remove getter/setter for paged results, this is useful.

RB+.

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On Wed, Apr 19, 2017 at 05:41:48PM +0300, Alexander Bokovoy wrote:

> On ke, 19 huhti 2017, vl--- via samba-technical wrote:
> > Hi!
> >
> > The main focus is to make "struct smbldap_state" private to smbldap.c
> > for better encapsulation.
> >
> > Review appreciated!
> Looks good to me. This will break ipasam in FreeIPA but I already
> started moving it to own private struct as 'struct ldapsam_privates' is
> not accessible anymore and I neeed anyway to implement some more of a
> backend code that needs own proper private struct. Updating it to use
> your wrappers is not a problem.

Ok, thanks for the confirmation.

> Please do not remove getter/setter for paged results, this is useful.

I think this should be hidden behind an improved smbldap_search
interface in the future.

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On ke, 19 huhti 2017, [hidden email] wrote:

> On Wed, Apr 19, 2017 at 05:41:48PM +0300, Alexander Bokovoy wrote:
> > On ke, 19 huhti 2017, vl--- via samba-technical wrote:
> > > Hi!
> > >
> > > The main focus is to make "struct smbldap_state" private to smbldap.c
> > > for better encapsulation.
> > >
> > > Review appreciated!
> > Looks good to me. This will break ipasam in FreeIPA but I already
> > started moving it to own private struct as 'struct ldapsam_privates' is
> > not accessible anymore and I neeed anyway to implement some more of a
> > backend code that needs own proper private struct. Updating it to use
> > your wrappers is not a problem.
>
> Ok, thanks for the confirmation.
>
> > Please do not remove getter/setter for paged results, this is useful.
>
> I think this should be hidden behind an improved smbldap_search
> interface in the future.
Agreed. If we can move most of the code in
ldapsam_search_firstpage/nextpage/next_entry/end and ldap2displayentry
to a common utility in smbldap library, then pdb_ldap/ipasam will just
become users of that interface and would only need to pass an opaque
pointer to "struct smbldap_state".

That would effectively reduce a duplicated code.

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Apr 19, 2017 at 04:41:36PM +0200, Ralph Böhme via samba-technical wrote:

> On Wed, Apr 19, 2017 at 04:20:36PM +0200, vl--- via samba-technical wrote:
> > The main focus is to make "struct smbldap_state" private to smbldap.c
> > for better encapsulation.
>
> +1 for the encapsulation but can we possibly use a different name then
> smbldap_get_ld() ? The ld makes it look like you could get a linker out of
> smbldap. :)
>
> Maybe smbldap_get_ldap() ?
>
> If you don't agree and would like to stick with smbldap_get_ld() please go ahead
> and push with my rb.
>
> If you agree but wonder how to rename without revisiting every file, sed inplace
> would do the trick, so
>
> $ find . -name '*.c' | xargs sed -i s/smbldap_get_ld/smbldap_get_ldap/g
> $ find . -name '*.h' | xargs sed -i s/smbldap_get_ld/smbldap_get_ldap/g
>
> If you want you can offload the task, I can do it, just tell me I should go
> ahead and push with this change.

Pushed it with that change. It made a few lines bump off the 80 chars,
so this is a few more + in the patch :-)

Thanks!

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Apr 19, 2017 at 05:41:48PM +0300, Alexander Bokovoy via samba-technical wrote:

> On ke, 19 huhti 2017, vl--- via samba-technical wrote:
> > Hi!
> >
> > The main focus is to make "struct smbldap_state" private to smbldap.c
> > for better encapsulation.
> >
> > Review appreciated!
> Looks good to me. This will break ipasam in FreeIPA but I already
> started moving it to own private struct as 'struct ldapsam_privates' is
> not accessible anymore and I neeed anyway to implement some more of a
> backend code that needs own proper private struct. Updating it to use
> your wrappers is not a problem.

I've pushed the "privatize smbldap_struct" now. This renders the
bind_callback inaccessible too. What are you using it for?  Looking at
ipa_sam.c I don't get the full picture.

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On ke, 19 huhti 2017, [hidden email] wrote:

> On Wed, Apr 19, 2017 at 05:41:48PM +0300, Alexander Bokovoy via samba-technical wrote:
> > On ke, 19 huhti 2017, vl--- via samba-technical wrote:
> > > Hi!
> > >
> > > The main focus is to make "struct smbldap_state" private to smbldap.c
> > > for better encapsulation.
> > >
> > > Review appreciated!
> > Looks good to me. This will break ipasam in FreeIPA but I already
> > started moving it to own private struct as 'struct ldapsam_privates' is
> > not accessible anymore and I neeed anyway to implement some more of a
> > backend code that needs own proper private struct. Updating it to use
> > your wrappers is not a problem.
>
> I've pushed the "privatize smbldap_struct" now. This renders the
> bind_callback inaccessible too. What are you using it for?  Looking at
> ipa_sam.c I don't get the full picture.
We do SASL GSSAPI authentication against IPA LDAP server. The reason for
that is because cifs/... principal has special rights in LDAP to read
and write keys of TDO objects and ability to set up access to them for
SSSD on IPA master.

Thus, BIND callback is really important to have to FreeIPA.

Would a similar

 void smbldap_set_bind_callback(struct smbldap_state*, bindproc, void  *binddata);

where bindproc is what we have already

int (*bind_callback)(LDAP *ldap_struct, struct smbldap_state *ldap_state, void *data);

be acceptable?

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On Wed, Apr 19, 2017 at 06:39:18PM +0300, Alexander Bokovoy via samba-technical wrote:

> We do SASL GSSAPI authentication against IPA LDAP server. The reason for
> that is because cifs/... principal has special rights in LDAP to read
> and write keys of TDO objects and ability to set up access to them for
> SSSD on IPA master.
>
> Thus, BIND callback is really important to have to FreeIPA.
>
> Would a similar
>
>  void smbldap_set_bind_callback(struct smbldap_state*, bindproc, void  *binddata);
>
> where bindproc is what we have already
>
> int (*bind_callback)(LDAP *ldap_struct, struct smbldap_state *ldap_state, void *data);
>
> be acceptable?

As a quick workaround, sure. I would however highly appreciate to get
proper authentication (based on gensec?) into smbldap proper. That's
one of the reasons why I started working on that: I want to get rid of
the special code in source3/libads/ldap.c. That also does "proper"
authentication, and I want that to use smbldap, or vice-versa. But
because smbldap looks more basic to me, the initial idea is to layer
ads_struct on top of smbldap. So smbldap needs to learn sasl.

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On ke, 19 huhti 2017, [hidden email] wrote:

> On Wed, Apr 19, 2017 at 06:39:18PM +0300, Alexander Bokovoy via samba-technical wrote:
> > We do SASL GSSAPI authentication against IPA LDAP server. The reason for
> > that is because cifs/... principal has special rights in LDAP to read
> > and write keys of TDO objects and ability to set up access to them for
> > SSSD on IPA master.
> >
> > Thus, BIND callback is really important to have to FreeIPA.
> >
> > Would a similar
> >
> >  void smbldap_set_bind_callback(struct smbldap_state*, bindproc, void  *binddata);
> >
> > where bindproc is what we have already
> >
> > int (*bind_callback)(LDAP *ldap_struct, struct smbldap_state *ldap_state, void *data);
> >
> > be acceptable?
>
> As a quick workaround, sure. I would however highly appreciate to get
> proper authentication (based on gensec?) into smbldap proper. That's
> one of the reasons why I started working on that: I want to get rid of
> the special code in source3/libads/ldap.c. That also does "proper"
> authentication, and I want that to use smbldap, or vice-versa. But
> because smbldap looks more basic to me, the initial idea is to layer
> ads_struct on top of smbldap. So smbldap needs to learn sasl.
I can make a patch that introduces SASL GSSAPI similar what we have in
ipasam. A general helper should be fine but I need to think more on
how to pass authentication information as

bool smbldap_set_creds(struct smbldap_state *ldap_state, bool anon, const char *dn, const char *secret);

is not enough -- we should probably move to a better way to specify
creds.

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On Wed, Apr 19, 2017 at 07:00:52PM +0300, Alexander Bokovoy wrote:

> > As a quick workaround, sure. I would however highly appreciate to get
> > proper authentication (based on gensec?) into smbldap proper. That's
> > one of the reasons why I started working on that: I want to get rid of
> > the special code in source3/libads/ldap.c. That also does "proper"
> > authentication, and I want that to use smbldap, or vice-versa. But
> > because smbldap looks more basic to me, the initial idea is to layer
> > ads_struct on top of smbldap. So smbldap needs to learn sasl.
> I can make a patch that introduces SASL GSSAPI similar what we have in
> ipasam. A general helper should be fine but I need to think more on
> how to pass authentication information as
>
> bool smbldap_set_creds(struct smbldap_state *ldap_state, bool anon, const char *dn, const char *secret);
>
> is not enough -- we should probably move to a better way to specify
> creds.

Right. That's why I referred to gensec. As much as I disagree with the
bloat and non-asynchrony of gensec_update it brings, this is our
solution to do such things.

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On ke, 19 huhti 2017, vl--- via samba-technical wrote:

> On Wed, Apr 19, 2017 at 07:00:52PM +0300, Alexander Bokovoy wrote:
> > > As a quick workaround, sure. I would however highly appreciate to get
> > > proper authentication (based on gensec?) into smbldap proper. That's
> > > one of the reasons why I started working on that: I want to get rid of
> > > the special code in source3/libads/ldap.c. That also does "proper"
> > > authentication, and I want that to use smbldap, or vice-versa. But
> > > because smbldap looks more basic to me, the initial idea is to layer
> > > ads_struct on top of smbldap. So smbldap needs to learn sasl.
> > I can make a patch that introduces SASL GSSAPI similar what we have in
> > ipasam. A general helper should be fine but I need to think more on
> > how to pass authentication information as
> >
> > bool smbldap_set_creds(struct smbldap_state *ldap_state, bool anon, const char *dn, const char *secret);
> >
> > is not enough -- we should probably move to a better way to specify
> > creds.
>
> Right. That's why I referred to gensec. As much as I disagree with the
> bloat and non-asynchrony of gensec_update it brings, this is our
> solution to do such things.
Can we look into gensec use at SambaXP?
--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On Wed, Apr 19, 2017 at 07:13:15PM +0300, Alexander Bokovoy via samba-technical wrote:
> > Right. That's why I referred to gensec. As much as I disagree with the
> > bloat and non-asynchrony of gensec_update it brings, this is our
> > solution to do such things.
> Can we look into gensec use at SambaXP?

Sure. A good prototype for what we need is in tldap_gensec_bind I
guess.

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On ke, 19 huhti 2017, vl--- via samba-technical wrote:
> On Wed, Apr 19, 2017 at 07:13:15PM +0300, Alexander Bokovoy via samba-technical wrote:
> > > Right. That's why I referred to gensec. As much as I disagree with the
> > > bloat and non-asynchrony of gensec_update it brings, this is our
> > > solution to do such things.
> > Can we look into gensec use at SambaXP?
>
> Sure. A good prototype for what we need is in tldap_gensec_bind I
> guess.
I've been trying to get to this over past two months but other tasks
interrupted me. As result, FreeIPA is currently not buildable against
Samba git master (and soon 4.7.0-RC1). So, to unblock
FreeIPA/Fedora/Samba 4.7 integration where a lot has depend on Samba 4.7
availability to allow FreeIPA to move to Python 3, I decided to propose
a function to give access to bind callback:

  smbldap_set_bind_callback(smbldap_state, callback, callback_state);

This would give us ability to work on smbldap/gensec/ads merge through
4.7.x timeframe.

Patch attached, please review.
--
/ Alexander Bokovoy

ab-samba-smbldap-bind-callback.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On Monday, 3 July 2017 11:26:13 CEST Alexander Bokovoy via samba-technical
wrote:
> On ke, 19 huhti 2017, vl--- via samba-technical wrote:
> > On Wed, Apr 19, 2017 at 07:13:15PM +0300, Alexander Bokovoy via samba-
technical wrote:

> > > > Right. That's why I referred to gensec. As much as I disagree with the
> > > > bloat and non-asynchrony of gensec_update it brings, this is our
> > > > solution to do such things.
> > >
> > > Can we look into gensec use at SambaXP?
> >
> > Sure. A good prototype for what we need is in tldap_gensec_bind I
> > guess.
>
> I've been trying to get to this over past two months but other tasks
> interrupted me. As result, FreeIPA is currently not buildable against
> Samba git master (and soon 4.7.0-RC1). So, to unblock
> FreeIPA/Fedora/Samba 4.7 integration where a lot has depend on Samba 4.7
> availability to allow FreeIPA to move to Python 3, I decided to propose
> a function to give access to bind callback:
>
>   smbldap_set_bind_callback(smbldap_state, callback, callback_state);
>
> This would give us ability to work on smbldap/gensec/ads merge through
> 4.7.x timeframe.
>
> Patch attached, please review.

The new signature file is missing.

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On ma, 03 heinä 2017, Andreas Schneider via samba-technical wrote:

>On Monday, 3 July 2017 11:26:13 CEST Alexander Bokovoy via samba-technical
>wrote:
>> On ke, 19 huhti 2017, vl--- via samba-technical wrote:
>> > On Wed, Apr 19, 2017 at 07:13:15PM +0300, Alexander Bokovoy via samba-
>technical wrote:
>> > > > Right. That's why I referred to gensec. As much as I disagree with the
>> > > > bloat and non-asynchrony of gensec_update it brings, this is our
>> > > > solution to do such things.
>> > >
>> > > Can we look into gensec use at SambaXP?
>> >
>> > Sure. A good prototype for what we need is in tldap_gensec_bind I
>> > guess.
>>
>> I've been trying to get to this over past two months but other tasks
>> interrupted me. As result, FreeIPA is currently not buildable against
>> Samba git master (and soon 4.7.0-RC1). So, to unblock
>> FreeIPA/Fedora/Samba 4.7 integration where a lot has depend on Samba 4.7
>> availability to allow FreeIPA to move to Python 3, I decided to propose
>> a function to give access to bind callback:
>>
>>   smbldap_set_bind_callback(smbldap_state, callback, callback_state);
>>
>> This would give us ability to work on smbldap/gensec/ads merge through
>> 4.7.x timeframe.
>>
>> Patch attached, please review.
>
>The new signature file is missing.
Sorry, new patch is attached.


--
/ Alexander Bokovoy

ab-samba-smbldap-bind-callback.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list

The commit message refers to libsmbclient.  Is that a typo, or is this
really exposed via libsmbclient somehow?

Andrew Bartlett


--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba





Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
On ti, 04 heinä 2017, Andrew Bartlett wrote:
>
> The commit message refers to libsmbclient.  Is that a typo, or is this
> really exposed via libsmbclient somehow?
Thank you, Andrew, for better eyes!

A fixed patch is attached.


--
/ Alexander Bokovoy

ab-samba-smbldap-bind-callback.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On ti, 04 heinä 2017, Andrew Bartlett wrote:
>
> The commit message refers to libsmbclient.  Is that a typo, or is this
> really exposed via libsmbclient somehow?
No, your eyes are better than mine. I'm embarrassed for not noticing it
up until now.
--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] some cleanups for smbldap.c

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, 2017-07-04 at 01:44 +0300, Alexander Bokovoy wrote:
> On ti, 04 heinä 2017, Andrew Bartlett wrote:
> >
> > The commit message refers to libsmbclient.  Is that a typo, or is
> > this
> > really exposed via libsmbclient somehow?
>
> Thank you, Andrew, for better eyes!
>
> A fixed patch is attached.

This seems reasonable.  I've reviewed and pushed this to autobuild
along with our traffic generator tool.  Hopefully it lands in a few
hours.

Thanks,

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba