[PATCH] documentation fixes and keytab handling regression

[Alexander Bokovoy]

On la, 03 joulu 2016, Alexander Bokovoy wrote:
> 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.
sent too early -- 'the keytab name is obviously either relative or
absolute, just did not have a prefix'.


-- 
/ Alexander Bokovoy