[PATCHES] next MIT Kerberos enablement work patchset

Fri Apr 27 17:40:41 MDT 2012

On Fri, 2012-04-27 at 13:23 -0400, simo wrote:
> 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.

OK.

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

Let's keep it out of the tree until we have that discussion. 

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

I remember seeing the option, but never looked into it much.  What were
the problems there?  

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

I understand you have concerns about 'samba only' smb.conf files.
However, it is reasonable that we only use this file if there is
krb5.conf in the same directory as the smb.conf?  A client-only
configuration would not have this generated by provision.  This is only
another file prepended to the list of possible config files, not the
replacement.  It has made Samba4 AD configurations generated by
provision much more reliable, which is why it was done.

If that isn't enough for you, then please leave this patch out until we
discuss removing this support entirely, or add an smb.conf option, or
replace it with some verification that users who have run provision have
installed a krb5.conf in an appropriate way. 

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

Please make sure you have addressed the concerns I mention above, but
other than that, I look forward to working with you on the next series.

Thanks,

Andrew Bartlett

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