removing wrappers and the krb5 refactor

[Andrew Bartlett]

On Thu, 2012-04-12 at 20:56 -0400, simo wrote:
> On Fri, 2012-04-13 at 08:00 +1000, Andrew Bartlett wrote: 
> > Simo and Andreas,
> > 
> > I'm sorry I wasn't given a chance to comment on this major re-factor, as
> > I do have a passionate interest in our kerberos code, and this is the
> > first I've heard of patches to restructure this area.
> > 
> > As I support these efforts, and I would appreciate it if you could
> > include me in your review of further changes in this area.  
> > 
> > On the specific changes mentioned above, I'm wondering what the major
> > issue was with the various krb5 wrappers you removed, so I can better
> > understand your patches?  
> 
> We are working on making it possible to build as much as possible of the
> samba4 client (and non-DC related server) stuff with system Kerberos
> libraries.
> 
> As part of this goal we are going to make sure that dependency hell that
> now force almost all the code to link against internal libkdc is
> resolved.
> 
> A first step is to clean up dependencies around krb5 wrappers so that we
> end up with clean wrappers that depend exclusively on krb5 libraries and
> (for now) loadparm, and nothing else. The patches I pushed in this case
> remove an obnoxious ldb dependency in core krb wrappers (which I also
> intend to move i a more common location at some point), and clean up
> other similar dependencies.
> 
> There is still a lot of code with wrong layers of abstraction or even
> reversed dependencies that affect gensec, we will soon start attacking
> that interface too. I will make sure to post patchsets in advance for
> your information when we get to that.

Thanks,

> The goal is to avoid dragging in the whole server dependencies in client
> libraries, as the current code has even pure external dependencies like
> openchange libs end up depending and trying to access things like the
> secrets database or the ldb database. All these dependencies need to be
> broken and appropriate callbacks be built so that they are reversed and
> called only in the right cases, so that we can build client libraries
> excluding Heimdal KDC bits and have them working against a system
> Kerberos library.

Thank you very much for taking on this.  Dependencies that were
reasonable as we try to build an AD DC don't always make sense for other
use cases, and it is important to sort out a sensible set of
dependencies for the many other ways that Samba4, and the python
bindings in particular are now being used.

> > In particular, the system to enrol the principal and keytab in talloc
> > wrappers was specifically designed to avoid leaking memory in error
> > cases, and as such I would have preferred it was retained, and used more
> > extensively.  It also wraps the result in a structure that can be
> > predeclared, rather than a typedef. 
> > 
> > On the smb_krb5_context, this is less important, but again serves as a
> > simple wrapper to avoid the krb5_context typedef (it is also used to
> > potentially specify certain additional options).
> 
> I carefully considered a few key elements before breaking them, with
> special care to dependency issues, readability of the code to someone
> familiar with gssapi and krb5 libraries, and actual effectiveness of the
> wrappers.
> If you look carefully though you will see that these wrappers were
> avoided in closely controlled code and their "non"-use is limited mostly
> to static functions within a single file.

I would prefer that smb_krb5_update_keytab() and
smb_krb5_create_memory_keytab() deal with the talloc-enrolled container.

I'm sure the code handles the error cases properly, but if it is
possible to return these particular routines to dealing with struct
keytab_container I would very much appreciate it.

> > Perhaps if you could lay out your general approach and proposed outcome,
> > some of this would be clearer.
> 
> See above.

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org