[PATCHES] next MIT Kerberos enablement work patchset

simo idra at samba.org
Fri Apr 27 11:23:05 MDT 2012


On Fri, 2012-04-27 at 11:21 +1000, Andrew Bartlett wrote: 
> On Thu, 2012-04-26 at 19:11 -0400, simo wrote:
> > Please take a look at the latest patches for MIt Kerberos support here:
> > https://git.samba.org/idra/samba.git/?p=idra/samba.git;a=shortlog;h=refs/heads/waf-mit-work
> > 
> > I plan to push all those not marked WIP if they pass autobuild.
> 
> Simo,
> 
> First, thanks for posting these, I do appreciate it.  As I mentioned, I
> have some concerns, and so would like to see some aspects of these
> patches reworked:
> 
> Explicitly set HAVE_LIBGSSAPI if either gssapi or gssapi_krb5 are found
> https://git.samba.org/idra/samba.git/?p=idra/samba.git;a=commitdiff;h=fc12fba8828d5731766db17c80f66b483d442cc1
> 
> As I already commented in objection to the patch that introduced
> HAVE_LIBGSSAPI, I do not want use to return to having multiple HAVE_
> defines for 'is this a build with support for krb5/gssapi'.  We do not
> support krb5 without gssapi, and so far HAVE_KRB5 has been selected as
> that indicator.  As such, rather than this patch, I ask that
> HAVE_LIBGSSAPI be changed to HAVE_KRB5.

There is a reason why Alexander did split these, we are preparing an
explanation about the whole process we are going through with these
patch series and hopefully it will also explain it.
Will send it to samba-technical once it's ready.
I would still push this patch for now as it just fixes a build issue
only with MIT and has no impact whatsoever on the current default build
with Heimdal and rather change again stuff later in one go if needed.
However i can also keep this specific patch in my WIP queue for now if
you feel really strongly about it, it will incovenience others also
working on the MIT integration effort but not too terribly.


> Move kerberos_kinit_keyblock_cc to krb5samba lib
> https://git.samba.org/idra/samba.git/?p=idra/samba.git;a=commitdiff;h=7ddeb293425789ce36468c2368454fd2c95c126c
> 
> It seems that supporting kinit from a keyblock is causing you some
> dramas.  I wonder if it would be better not to support this at all?
> There is no sensible use case for this - the closest we have is support
> for a Samba domain join prepared with a version of Samba so old that it
> didn't write the plaintext trust account password into the
> secrets.tdb.  
> 
> To be clear, I don't object to the patch, but I am quite willing to
> loose this feature if it helps. 

Well the code is done, so I'll leave it there for now, IT would be nice
to remove this thing completely though, it's so rarely used and makes
code so ackward ... but I won't touch it for now unless we find it
really gets in the way at some point.

> s4-auth-krb: smb_rd_req_return_stuff is used only in gensec_krb5
> https://git.samba.org/idra/samba.git/?p=idra/samba.git;a=commitdiff;h=8d5771a24fc7953055eb5669e8fefb24f909e91f
> 
> This code was placed in a separate file specifically:
> -/* This file for code taken from the Heimdal code, to preserve licence
> */
> 
> You may wish to change the subsystem this file is linked into to be just
> the gssapi_krb5 module.  This module is not only used for testing, it is
> also a key part of support for the kpasswd server.  You may choose to
> make the compilation of this module dependent on Heimdal, in the same
> way that you will presumably be disabling the kdc and kpasswd server. 

Ok I changed the patch to keep the code in a separate file and moved it
to the gensec_krb5 subsystem.

> krb-init: define out heimdal specific stuff in mitkrb build
> https://git.samba.org/idra/samba.git/?p=idra/samba.git;a=commitdiff;h=18a2ee18ee3b0321638f1b101eb577daa3bc392e
> 
> I have two issues with this patch.  I don't like how much inline #ifdef
> is being introduced  here an in the other patches, as we have gone to
> great pains in Samba to have libraries like clikrb5.c that contain the
> #ifdefs as much as possible.

Yes and as you can see we are concentrating even more by putting all
krb5 (and only krb5) wrapper in the krb5samba lib, and keeping all
defines in there.

> However, the major issue I have is the behaviour change:
>  +       /* The MIT Kerberos build relies on using the system krb5.conf
> file.
> +        * If you really want to use another file please set KRB5_CONFIG
> +        * accordingly. */
> +#ifdef SAMBA4_USES_HEIMDAL
> 
> I really think that the choice of compiled library should not change our
> behaviour in this way.  We should, within reason have the same behaviour
> if a system Heimdal is used for the components that you are trying to
> support with a system MIT krb5.
> 
> If a particular deployment situation requires that we not use the local
> krb5.conf first, then an option to support this may well be a very good
> idea.  Or similarly, if this only makes sense in the S4 DC, then an
> option (say, in smb.conf, or a server role to cover the AD DC) to enable
> it would make sense. 

Ok so the deal here is that we are working on 'client side' support, and
for any client it is imperative to be consistent. Using a non-standard
krb5.conf by default is wrong. Jeremy did it in samba3 as well sometimes
ago and it was a disaster.

So going forward I think we need to discuss changing this code entirely
to never use a non system krb5.conf file. It may be a nice option on a
devel system, so I am ok if you define a different file and pass it in
via KRB5_CONFIG at bin/samba startup. But using a different file by
default is just a recipe for headaches for admins and users.

I am not going to address this now though, and the 'SAMBA4_USES_HEIMDAL'
guards around the code will make it clear we need to revisit this part,
see below for more explanation about this.

> keytab_copy: Fix style, whitespaces
> https://git.samba.org/idra/samba.git/?p=idra/samba.git;a=commitdiff;h=9334b944763a538c065e30e74f70748acea8e9b0
> 
> This may be less important with your later patch to change this to
> compile with MIT krb5 compat wrappers, but the reason this file was left
> in original style was to make it easier to copy a new version in from 
> Heimdal if required. 

Given I needed to change it anyway, I did the style fixes. If you notice
I didn't fix other code that I wasn't going to touch (was really tempted
to re-indent keytab_copy.c -> keytab_util.c ...)

> s4-auth-krb: Disable code in MIT build
> https://git.samba.org/idra/samba.git/?p=idra/samba.git;a=commitdiff;h=f339cd6e59501234cf45a7dfad0f7d085520ae76
> 
> Should these be defines based on the particular missing functions, or be
> replaced with null wrappers?

Yes and no.
So if you look carefully at the patches you will see that for all the
wrappers we put in the krb5samba lib we have proper defines and
configure checks.
Where you see SAMBA4_USES_HEIMDAL defined you are looking at area that
will need to be worked on again later. But in some cases we need to tak
shortcuts to make the problem tractable. By using SAMBA4_USES_HEIMDAL we
have a clear trace to follow later on to go and address the issues at a
more fundamental level and try to remove these defines.
So for now we need to keep these, with the understanding that further
work down the road will address them as much as possible.

> Split normal kinit from s4u2 flavored kinit
> https://git.samba.org/idra/samba.git/?p=idra/samba.git;a=commitdiff;h=9803c47bcec7028640f3f465e900eaea90888d2c
> 
> This seems like a sensible way to deal with what is clearly an API mess,
> with decode_Ticket() calls etc.  It's a pity however, as might be a
> better way to deal with getting user info in winbindd in the long run. 

Actually I have to say I really dislike this code. GSSAPI should be used
to perform s4u2 funciont and not low level krb5 code, gssapi simply
knows better and we want to delegate maintanenace and knowledge about
how this stuff works to gssapi wherever possible. At the moment I am
thinking about this as an interim step, with the idea of coming back
later and see if we can change the code to use gssapi via gensec to do
it. I simply moved it away w/o implementing it because the only real
user of this code is the cifs-proxy as far as I can see, and it is a
feature that we can break in the MIT build for a while. If moving these
operations to gssapi will not be pssible for whatever reason will will
have to decide whether to not have the feature at all with MIT or make
the convert the code.

-

Ok I see no real objections except for the first patch at this point.
More patches are piling up, I will probably push this stuff on Monday,
and keep moving on to other parts of the code.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>



More information about the samba-technical mailing list