[linux-cifs-client] Kerberos5 support in cifs pathset [PATCH: 2/4] enables extended security in NEG... and SESSION_SETUP... requests when mounting with krb5i option

[Jeff Layton]

On Thu, 25 Oct 2007 17:21:32 +0400
"Q (Igor Mammedov)" <qwerty0987654321 at mail.ru> wrote:

> Jeff Layton wrote:
>   > Our current thinking is that we should offload all of the ASN1 
> parsing to
> > userspace. There's no way to reasonably handle kerberos tickets
> > directly from kernel space. Since we're going to have to upcall
> > anyway, we might as well just ship everything there.
> > 
> > Still, there might be a place to do some simple parsing of ASN1
> > blobs. It might help avoid upcalls when we're just looking for
> > something trivial, or for NTLMSSP.
> 
> Simple pre parsing not only helps to decide whether do upcall or not, 
> but can help to
> determine if negotiation has been accepted by server or we need
> continue negotiation.
> 

I'd think that we can probably determine success or failure by the
actual return code from the NegotiateProtocol reply itself. If it fails
with certain return codes (maybe NT_STATUS_MORE_PROCESSING_REQUIRED?),
then we'll know to hand the blob back to userspace and continue the
negotiation.

I could be wrong on this though, I'm no SPNEGO expert, and have been
relying heavily on knowledge of others for this ;-)

> > Steve, thoughts?
> > 
> >> diff -b -B -N -u -r linux-2.6.22.9/fs/cifs/cifssmb.c
> >> linux-2.6.22.9/fs/cifs.krb5/cifssmb.c ---
> >> linux-2.6.22.9/fs/cifs/cifssmb.c	2007-09-26
> >> 22:03:01.000000000 +0400 +++
> >> linux-2.6.22.9/fs/cifs.krb5/cifssmb.c	2007-10-18
> >> 13:37:11.000000000 +0400 @@ -434,7 +434,7 @@ pSMB->hdr.Mid =
> >> GetNextMid(server); pSMB->hdr.Flags2 |= (SMBFLG2_UNICODE |
> >> SMBFLG2_ERR_STATUS);
> >> -	if ((secFlags & CIFSSEC_MUST_KRB5) == CIFSSEC_MUST_KRB5)
> >> +	if ((secFlags & CIFSSEC_MAY_KRB5) == CIFSSEC_MAY_KRB5)
> >>  		pSMB->hdr.Flags2 |= SMBFLG2_EXT_SEC;
> >>  	
> > 
> > Not sure that that is right...MAY_KRB5 can be set in conjunction
> > with other MAY flags.
> 
> Upstream has more correct for doing this. Besides CIFSSEC_MUST_KRB5 
> isn't used in code at all. Looks like I was the first who tried to
> use it.
> 

Ok, that part has been in flux recently. I find all of the CIFSSEC_*
flags to be very hard to follow, personally. I don't think I've got it
right with my patch either. I pretty much left them alone for now and
have been testing by setting SecurityFlags to 0x8009.

>   >> diff -b -B -N -u -r linux-2.6.22.9/fs/cifs/connect.c 
> linux-2.6.22.9/fs/cifs.krb5/connect.c
> >> --- linux-2.6.22.9/fs/cifs/connect.c	2007-09-26
> >> 22:03:01.000000000 +0400 +++
> >> linux-2.6.22.9/fs/cifs.krb5/connect.c	2007-10-22
> >> 19:49:48.000000000 +0400 @@ -3227,6 +3227,10 @@ }
> >>  	}
> >>  
> >> +	if( ses->server->secType == Kerberos ) {
> >> +		smb_buffer->Flags2 |= SMBFLG2_EXT_SEC;
> >> +	}
> >> +
> >>  	if(ses->server->secMode & 
> >>  			(SECMODE_SIGN_REQUIRED |
> >> SECMODE_SIGN_ENABLED)) smb_buffer->Flags2 |=
> >> SMBFLG2_SECURITY_SIGNATURE; @@ -3412,6 +3416,12 @@
> >>  		else if (extended_security
> >>  				&& (pSesInfo->capabilities 
> >>  					& CAP_EXTENDED_SECURITY)
> >> +				&& (pSesInfo->server->secType ==
> >> Kerberos)) {
> >> +			rc = CIFS_SessSetup(xid, pSesInfo,
> >> +					    first_time, nls_info);
> >> +		} else if (extended_security
> >> +				&& (pSesInfo->capabilities 
> >> +					& CAP_EXTENDED_SECURITY)
> >>  				&& (pSesInfo->server->secType ==
> >> NTLMSSP)) { rc = -EOPNOTSUPP;
> >>  		} else if (extended_security
> >>
> >>
> > 
> > 
> > Not a comment on your patch, but we seem to have a lot of redundant
> > code in this file. When experimEnabled < 2, we only call
> > CIFS_SessSetup, so the above hunk will only come into effect if
> > someone resets /proc/fs/cifs/Experimental . It's not clear to me
> > whether some of this code is an effort to break up CIFS_SessSetup,
> > or vestigial code that should probably be removed...
> 
> Yes, It's not necessary code now and with experimEnabled < 2 it will 
> work just fine. But finely we could end up with separate session
> setup function for kerberos if we will consider implementation of
> multi-stage negotiation.
> 

Yes. CIFS is full of really big functions. I'm all for breaking them
up into smaller ones where reasonable. It would be nice to either get
that fixed up for good (and remove CIFS_SessSetup) or maybe wrap it in
some #define's so that it's clear that it's experimental code.

Either way, my patch doesn't touch the experimental functions for now...

Thanks,
-- 
Jeff Layton <jlayton at redhat.com>