Recent kerberos refactoring

[Alexander Bokovoy]

Hi Andrew,

On Fri, Apr 13, 2012 at 00:37, Andrew Bartlett <abartlet at samba.org> wrote:
> On Thu, 2012-04-12 at 17:02 +0300, Alexander Bokovoy wrote:
>> Hi Jelmer,
>>
>> On Thu, Apr 12, 2012 at 16:54, Jelmer Vernooij <jelmer at samba.org> wrote:
>> > Hi Alexander, Andreas,
>> >
>> > This breaks the build against system Heimdal:
>> >
>> > charis:~/src/samba% make
>> > WAF_MAKE=1 ./buildtools/bin/waf build
>> > Waf: Entering directory `/home/jelmer/src/samba.git1/bin'
>> > Checking project rules ...
>> > Unknown dependency 'k5crypto' in 'addns'
>> This needs to be fixed, yes.
>>
>>
>> >> commit dda0531aae70e78e815fda8c594213369e76a847
>> >> Author: Alexander Bokovoy <ab at samba.org>
>> >> Date:   Tue Apr 3 11:22:15 2012 +0300
>> >>
>> >>     WAF: Add support for stopping processing before end of wscript{_*}
>> >>
>> >>     WAF scripts are written in Python and Python has no simple way
>> >>     to stop program execution other than using exceptions.
>> >>
>> >>     This change adds WscriptCheckSkipped exception and its handling in
>> >>     core WAF code. When any of wscript{_*} throws WscriptCheckSkipped
>> >>     exception, WAF simply continues to process next wscript in queue
>> >>     rather than breaking build.
>> >>
>> >>     WscriptCheckSkipped exception can be used to perform early bail out
>> >>     of configuration/build target checks if certain dependency is not available
>> >>     when the default checks are way more numerous than a check for this
>> >>     particular dependency. This is to avoid 'if ...' indenting for large
>> >>     blocks of existing code which also muddens git history for nothing.
>> >>
>> >>     Signed-off-by: Andreas Schneider <asn at samba.org>
>> > Is this one really necessary? It doesn't actually seem to be used
>> > anywhere yet.
>> "yet" is the correct word. It is preparatory part for MIT krb5 reuse
>> but see below for reasoning.
>
> One question:  Were these patches proposed somewhere that I wasn't aware
> of?  I would have appreciated the chance to comment on some of these
> changes.
I see in a separate email discussion with Simo you touched most parts already.
We gathered patches in
https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/master-mit-krb5

>> > Wouldn't it make more sense to just conditionally add build files,
>> > rather than allowing the rest of them to be skipped halfway through? The
>> > nice thing about if statements is that it's fairly clear that something
>> > is conditional; having an exception being raised halfway through the
>> > file makes it unobvious what is going exactly without reading the file
>> > all the way from the beginning.
>> If you do conditionals, whole git history for those wscripts will be
>> overridden with formatting changes for nothing.
>> Practically, whole heimdal wscripts will have to be if-ed and I'm not
>> really ready for doing those indents just for such purpose.
>
> If you have to if-ed the whole hiemdal wscript, then I think you are
> doing something wrong.  We can already use a system Heimdal, and the
> same logic to skip building those libs should be able to be used for
> MIT.
Right now the way system heimdal support is implemented is to process
heimdal_build and differentiate from embedded heimdal by a lot of
USING_SYSTEM_* defines. One of them, USING_SYSTEM_KRB5, is common for
system krb5 libraries which are bigger than libkdc.

This is not the best approach and basically fails for system MIT Krb5
libraries as it assumes completely different use case. Jelmer proposed
to use separate wscript_build_* files but I haven't looked at that
yet; also splitting up build definitions and scatter them across
multiple files is worse from maintenance perspective -- I've seen
already people complaining there are so many waf scripts that it is
hard to grasp whole picture.  We are working on it with Jelmer, see
below.

What is more important here is a logic that pulls server-side code
into client-side libraries. For example, in source4/auth/, there are
auth_session, auth4_sam, and pyauth subsystems that require
server-side code. auth_session is pulled into gensec_schannel and that
one is pulled into libnet, a pure client side library now that I
separated extract_keytab code which was only used in python bindings
to libnet and which took HDB driver in, together with Heimdal KDC
code.

Moving further, auth/gensec now requires auth_system_session which
requires auth_session and pulls samdb from Samba4 server-side which,
again, links in auth_session and auth4_sam. Besides being cyclic, it
pulls samdb linking into libsmb in source3.

These dependencies need to be fixed. In most cases a solution could be
in making interfaces clear of server-specific arguments and instead
either pass a callback where required or allow using an "object" that
encapsulates proper callbacks and states and two initializers: for
server-side and client-side uses. Or split libraries even more. For
example, credentials_secrets.c operates on server side and it is
folded into pycredentials, Python bindings for credentials library
which is necessary for client operations on any Python client. I'd
rather separated it, as well as POPT_CREDENTIALS subsystem which in
addition is pulled into WMI sample client which is definitely not
supposed to be running only on Samba4 server.

> Additionally, the current patch to simply skip all of the heimdal_build
> directory either shows that this isn't required, or is incorrect.
Skips in case system krb5 is found. It is a preparation in order to
even get to the untangling of dependencies as Simo pointed in the
other thread. We need also to split out use of USING_SYSTEM_KRB5 as
that clearly has now overloaded meaning. I'm working with Jelmer on
getting system heimdal build covered, I have a prototype patch already
in works.

>> Making an exception and properly catching it is way better. We can add
>> more verbose output telling why processing stopped and at which line.
>
> I also would prefer to avoid the exceptions approach.  I'm less worried
> about the history (splitting the files up may be an option so git sees a
> copy), but am worried that we should require as little special knowledge
> as possible when working on wscript files.
>
> Additionally, changes to wafadmin should be checked with ita, as we want
> to ensure we don't take an approach that will make it harder to move to
> a new upstream waf version in future.
I think those 4 lines which introduce exception handling will be good
addition to waf upstream, I'm intending to submit it there. It is
reasonable alternative to forcing people to split out their waf
receipts in multiple smaller files.
-- 
/ Alexander Bokovoy