[PATCHES] next MIT Kerberos enablement work patchset

Andrew Bartlett abartlet at samba.org
Thu Apr 26 19:21:37 MDT 2012


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.


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. 


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. 


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.

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. 


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. 


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?


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. 


Finally, thanks again for giving me the chance to look over these.  If
you can post a revised branch, I'm sure I'll be in a much better
position to ack the changes.

I wish you the very best with this challenging task, and look forward to
seeing the end result!

Andrew Bartlett

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



More information about the samba-technical mailing list