[linux-cifs-client] Re: [PATCH 00/12] cifs: fix string conversions from wire format to local charset (try #3)

[Jeff Layton]

On Thu, 30 Apr 2009 09:27:41 -0500
Steve French <smfrench at gmail.com> wrote:

> On Thu, Apr 30, 2009 at 8:25 AM, Suresh Jayaraman <sjayaraman at suse.de> wrote:
> > I would also want to see patches 9 and 11 merged mainly because the cifs
> > code would look lot less confusing without them and they are currently
> > unusable. I think they could be part of some testing branch/branch
> > linux-next if there are strong reasons for keep them and we forsee some
> > future developments/changes.
> 
> NTLMSSP is the default mechanism for most real world (smb/cifs etc.)
> authentication
> so is probably the "safest" one to use.  It also is useful because it
> allows you to
> detect whether ntlmv2 is supported before we send the password hash
> Note that we should never be sending ntlm these days unless the
> server is very backlevel, but without ntlmssp it is hard to determine which
> mechanism to use.   The addition of Kerberos support to cifs was
> helpful to many,
> but in practice most target servers are not properly configured to handle
> Kerberos authentication, and thus doing NTLMSSP properly is too important
> to ignore.
> 
> The code which did NTLMSSP was written many, many years ago, and only does
> "raw ntlmssp" (not encapsulated in spnego) and does not use the code cleanup
> that went into the new session setup implementation in fs/cifs/sess.c
> Changes last year to SMBNegotiate may have accidently disabled this code.
> Jeff and I spent some time looking through these two old functions which send
> the NTLMSSP session setup requests (and which he did not want to convert in
> his patch set to the new string functions since he would like them to
> be moved/rewritten):
> 
> 1) CIFSNTLMSSPNegotiateSessSetup
> followed by
> 2) CIFSNTLMSSPAuthSessSetup
> 
> These two functions in current form are fairly large (640 lines
> total), but with Jeff's new
> helper functions and merged into fs/cifs/sess.c should be
> significantly smaller, and
> port easily. As we went through the code flow in these two functions,
> they look fairly
> straightforward though.   Basically they are a boring series of assignment
> statements to setup the blob, and the format of the NTLMSSP blob
> is straightforward (see fs/cifs/ntlmssp.h).
> 
> Instead of simply ripping out the code, we discussed moving them into
> helper functions
> (e.g. to create and initialize the blob, and to setup the unicode or
> ascii string areas
> in the blob).  Even after porting these two functions would continue
> to be disabled by default
> at runtime (/proc/fs/cifs/Exerimental) and within
> CONFIG_CIFS_EXPERIMENTAL ifdef until
> more users have a chance to test this.   A primary objection to the old NTLMSSP
> implementation was not the long series of assignment statements which
> are inevitable,
> but that it was written as two large functions instead of smaller
> ones, with cleaner
> exit handling, and less nesting.   Jeff and I also discussed that cifs
> eventually needs to do NTLMSSP in Spnego, and all three kernel SPNEGO
> implementations (cifs, firewall/nat, and nfs/sunrpc) are either bad or
> incomplete or both,
> but for various reasons we need to address this (not just in cifs), so
> it is likely that the
> asn support will get better over the next few releases (and with the
> blob code separated,
> we can reuse it for the spnego encapsulated flavor of ntlmssp).
> 
> I asked Jeff to send his patch series, which I will merge without the
> two changes
> unrelated to string handling, and then I will add NTLMSSP changes into
> new helper functions to create the NTLMSSP blob, then delete the old.
> 
> The remaining unmerged patch relates to how Windows "junctions" are recognized.
> (junctions are implemented as reparse points in Windows, and are
> somewhat similar
> to symlinks and are a fundamental inode type).  Before we merge that
> patch, someone
> needs to verify (e.g. using one of the standard windows utilities
> fsutil, ln.exe or linkd.exe)
> that earlier global changes to symlink code did not break this support
> for a common
> windows file type.


Don't forget about the duplicate code in CIFSSesSetup (or whatever the
studly caps are for it). That adds another few hundred lines or so.
Removing the NTLMSSP code would take a 1000 line chunk out of cifs, to
which I say "hurrah!".

On the readlink thing, I delved back into old-2.6-bkcvs tree and found
this:

commit 8199b6963e73ad209f40b4b3d4bc51a54818852d
Author: torvalds <torvalds>
Date:   Thu Sep 2 08:19:12 2004 +0000

    2004/09/01 23:35:11-05:00 stevef
    [CIFS] Fix CIFS symlink regression when long symlink paths
    
    Signed-of-by: Steve French (sfrench at us.ibm.com)
    
...at this point cifs_readlink was unhooked from cifs_symlink_inode_ops
and generic_symlink was hooked in. The cifs_readlink code was never
removed, however. If we've lived without this for 4+ years, I think it's
safe to remove it.

-- 
Jeff Layton <jlayton at redhat.com>