removing wrappers and the krb5 refactor

[Andrew Bartlett]

On Thu, 2012-04-12 at 15:24 +0200, Andreas Schneider wrote:
> The branch, master has been updated
>        via  bcc16f1 s4-libnet: split export_keytab in a separate python module to avoid pulling in HDB dependency
>        via  dda0531 WAF: Add support for stopping processing before end of wscript{_*}
>        via  1f1e427 clikrb5: Move pure krb wrapper functions from libads to clikrb5.
>        via  46ab219 gse: Remove unnecessary header.
>        via  a925c2c srv_keytab: Pass krb5_context directly, it's all we use anyways.
>        via  c13c065 krb5_wrap: krb5_string_to_key / krb5_encrypt_block are deprecated.
>        via  70c303a auth-krb: Move pac related util functions in a single place.
>        via  3fd6ded auth-krb: Make functions static.
>        via  d857e39 auth-krb: Use simpler method to extract keytype.
>        via  88d5d5c auth-krb: Nove oid packet check to gensec_util.
>        via  f116262 s4-auth-krb: Remove dependency on credentials too.
>        via  a46e465 s4-auth-krb: Remove unneded dependency on kerberos_util.
>        via  aedbd6b s4-auth-krb: Simplify salt_princ handling.
>        via  6de578a s4-auth-krb: Move function to db-glue.c and make it static.
>        via  b226955 s4-auth-krb: Move keytab functions in a separate file.
>        via  7d203f7 s4-auth-krb: Streamline and cleanup code to make it readable.
>        via  23d54e7 s4-auth-krb: streamline and rename enctype functions
>        via  6f7fa0b s4-auth-krb: Make kerberos_enctype_bitmap_to_enctype static.
>        via  60905c8 s4-auth-krb: Make kerberos_enctype_bitmap_to_enctypes static.
>        via  670dbde s4-auth-krb: Move function into more appropriate header.
>        via  70f1cd6 s4-auth-krb: Make cli_credentials_invalidate_client_gss_creds static.
>        via  b574e7c s4-auth-krb: Make impersonate_principal_from_credentials static.
>        via  93aa451 gensec_gssapi: keep private header file close to the actual code
>        via  6ab0dfe krb5_wrap: remove duplicate declaration and dead ifdef
>        via  c761654 s4-ldb: use KRB5_KEY macros to access key elements.
>        via  011540b wafsamba: point out that local heimdal paths are not included when USING_SYSTEM_KRB5 gets set.
>        via  1fedb0a waf: when USING_SYSTEM_KRB5 environment variable is set, dont configure local heimdal.
>        via  d82aab6 waf: when building with system krb5, we do not need to build local heimdal.
>        via  60f192a s3-waf: remove requirement of having --enable-developer for running system krb5 checks.
>       from  81d1749 Remove overly complex attemt to define blkcnt_t and blksize_t. AC_CHECK_TYPE should just do it.
> 
> http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master
> 
> 

> commit a925c2c48d07cd4f074325954d933e194b4704d8
> Author: Simo Sorce <idra at samba.org>
> Date:   Sun Apr 1 19:08:15 2012 -0400
> 
>     srv_keytab: Pass krb5_context directly, it's all we use anyways.
>     
>     Signed-off-by: Andreas Schneider <asn at samba.org>

> commit a46e465ce0d05d9b2e0ff016aa8db14dc149153b
> Author: Simo Sorce <idra at samba.org>
> Date:   Sat Mar 31 03:23:19 2012 -0400
> 
>     s4-auth-krb: Remove unneded dependency on kerberos_util.
>     
>     Signed-off-by: Andreas Schneider <asn at samba.org>
> 
> commit aedbd6bf8e4029c2089652d0f0a80777bc856f89
> Author: Simo Sorce <idra at samba.org>
> Date:   Sat Mar 31 01:27:02 2012 -0400
> 
>     s4-auth-krb: Simplify salt_princ handling.
>     
>     This allows us to make parse_principal static in kerbeors_util again and
>     avoid a silly game where we alloc containers and set destrcutors only to
>     release the whole thing at the end of the function.
>     
>     Signed-off-by: Andreas Schneider <asn at samba.org>

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?  

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).

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

Thanks,

Andrew Bartlett

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