[PATCH] documentation fixes and keytab handling regression

[Alexander Bokovoy]

On pe, 02 joulu 2016, Jeremy Allison wrote:
> On Fri, Dec 02, 2016 at 10:08:02PM +0200, Alexander Bokovoy wrote:
> > On pe, 02 joulu 2016, Jeremy Allison wrote:
> > > On Fri, Dec 02, 2016 at 12:56:03PM +0200, Alexander Bokovoy wrote:
> > > > Hi,
> > > > 
> > > > attached two patches improve documentation for 'logon script' and
> > > > 'dedicated keytab file' options.
> > > > 
> > > > The second patch also fixes a regression introduced by the commit
> > > > c2f5c30b which broke specifying storage access format for keytabs.
> > > 
> > > NAK on the second patch. Sorry, but I don't understand what
> > > you're trying to check for w.r.t. relative/absolute paths
> > > here.
> > > 
> > > The original code refuses all paths that don't start with '/'
> > > (i.e. don't allow relative paths).
> > > 
> > > I'm assuming you want to allow paths of the form:
> > > 
> > > /absol/ute/path
> > > FILE:/absol/ute/path
> > > WRFILE:/absol/ute/path
> > > 
> > > but disallow paths of the form:
> > > 
> > > rela/tive/path
> > > FILE:rela/tive/path
> > > WRFILE:rela/tive/path
> > > 
> > > In which case the logic should be:
> > > 
> > > 	if (keytab_name_req != NULL) {
> > > 		if (keytab_name_req[0] != '/') {
> > > 			/*
> > > 			 * Might still be an absolute path, but
> > > 			 * prefixed by FILE:/ or WRFILE:/
> > > 			 */
> > > 			bool good_file = (strncmp(keytab_name_req, "FILE:/", 6) == 0);
> > > 			bool good_wrfile = (strncmp(keytab_name_req, "WRFILE:/", 8) == 0);
> > > 
> > > 			if (!good_file && !good_wrfile) {
> > > 				/* Nope - relative path. Disallow. */
> > > 				return KRB5_KT_BADNAME;
> > > 			}
> > > 		}
> > > 	}
> > > 
> > > Is this correct ?
> > The very same logic is in the smb_krb5_kt_open_relative() which
> > smb_krb5_kt_open() calls to after this check. However,
> > smb_krb5_kt_open_relative() supports also other prefixes, including
> > MEMORY: and ANY:. Thus, with your approach you would break the code that
> > might want to pull in a keytab created by Samba code somewhere else in
> > the process memory using MEMORY: prefix. Actually, it will not be able
> > to create such keytab at all, due to this change, unless
> > smb_krb5_kt_open_relative() is called directly.
> 
> That's the point isn't it ? I thought users who wanted
> MEMORY: and ANY: call smb_krb5_kt_open_relative().
> 
> There are such in:
> 
> source3/libnet/libnet_keytab.c
> source4/kdc/ktutil.c
> source4/libnet/libnet_export_keytab.c
> 
> anyone who doesn't calls smb_krb5_kt_open().
> 
> I want to understand what you're trying to
> do here before we make code changes.
As I said, I'm trying to fix the regression -- Fedora 25 with Samba
4.5.x now broke FreeIPA deployments.

Looking at smb_krb5_kt_open_relative() and smb_krb5_kt_open(), though,
I'm not sure what's the purpose of the whole '/' check in
smb_krb5_kt_open() -- had it not be there, smb_krb5_kt_open_relative()
would equally do the justice and only accept absolute paths to WRFILE:
and FILE: prefixed keytabs already.

I'm not really sure why it is named _relative(), though. There is
nothing there for relative paths at all. If you passed the keytab name,
it gets analyzed whether it is prefixed with WRFILE:/ or FILE:/ and if
not, either FILE: or WRFILE: is prepended to the path and then keytab
gets open. In the latter case the keytab name is obviously relative.

It would also break for MEMORY: keytabs, as that case is not handled
right in the code path for when the keytab name is passed in.

If you don't pass the keytab name, _relative() does try to obtain the
name of the default keytab and parse it. Here it expects all kinds of
prefixes but there is nothing for the 'relative' paths there either. 

It seems to me that smb_krb5_kt_open() refactoring would be to eliminate
the distinction between the two as it is not simply useful at all.


-- 
/ Alexander Bokovoy