Re: [PATCH 09/11] cifs: allow mixed secTypes on a socket

On Fri, 7 May 2010 13:18:29 +0200
Jeff Layton <[email protected]> wrote:

> On Fri, 7 May 2010 02:51:50 -0500
> Steve French <[email protected]> wrote:
> 
> > On Fri, May 7, 2010 at 2:31 AM, Jeff Layton <[email protected]> wrote:
> > >> > @@ -978,42 +978,39 @@ cifs_parse_mount_options(char *options, const char
> > >> > *devname,
> > >> >                                cERROR(1, "no security value specified");
> > >> >                                continue;
> > >> >                        } else if (strnicmp(value, "krb5i", 5) == 0) {
> > >> > -                               vol->secFlg |= CIFSSEC_MAY_KRB5 |
> > >> > -                                       CIFSSEC_MUST_SIGN;
> > >> > +                               vol->sectype = Kerberos;
> > >> > +                               vol->sign = true;
> > >> >                        } else if (strnicmp(value, "krb5p", 5) == 0) {
> > >> > -                               /* vol->secFlg |= CIFSSEC_MUST_SEAL |
> > >> > -                                       CIFSSEC_MAY_KRB5; */
> > >> >                                cERROR(1, "Krb5 cifs privacy not
> > >> > supported");
> > >> >                                return 1;
> > >> >                        } else if (strnicmp(value, "krb5", 4) == 0) {
> > >> > -                               vol->secFlg |= CIFSSEC_MAY_KRB5;
> > >> > +                               vol->sectype = Kerberos;
> > >> >
> > >>
> > >>
> > >> Doesn't this change behavior a lot - user can no longer pass in a list of
> > >> more than one acceptable security options as before (e.g. sec=krb5,
> > >> sec=ntlmv2i)
> > >> and can only specify one (but doesn't know ahead of time which the server
> > >> supports).
> > >>
> > >> Also curious if it changed how plaintext was handled (if build with weak
> > >> password support allowed, which I don't want to encourage) - IIRC we sent
> > >> this via the older (LANMAN only) style session setup.
> > >>
> > >>
> > >
> > > It does change that behavior. I didn't realize that was a use case we
> > > were interested in supporting.
> > >
> > > Note that the existing code:
> > >
> > > 1) doesn't take the order of sec= options into account to indicate the
> > > preference (this one will, with the well-established behavior that last
> > > option specified wins)
> > >
> > > 2) doesn't try multiple auth types in turn. It's going to pick one and
> > > go with it.
> > >
> > > If it is important to allow specification of multiple sec= options,
> > > could you clarify what should the behavior be when someone does this
> > 
> > 1) If no sec options specified on mount, it chooses among the available
> > mechanisms (configurable globally in /proc/fs/cifs - a distro could
> > populate this pseudofile at boot time from a configuration file, if
> > they want to
> > restrict or expand the available mechanisms) and picks from among
> > those the server supports.
> > 
> 
> Right. That behavior should be preserved by this patchset (with some
> sensible modifications like auto-enabling signing).
> 
> > 2) If one sec option is specified on mount, that mechanism is used
> > if the server supports it, otherwise fails
> > 
> 
> Same with this patch.
> 
> > 3) If more than one sec option is specified, the behavior should
> > be similar to temporarily setting the available mechanisms
> > in /proc/fs/cifs  (cifs.ko picks among those based on
> > what the server would support).   General idea with specifying
> > multiple sec options - the user says which mechanisms
> > are acceptable to it, and as long as the server supports them,
> > the user lets cifs.ko decide which is "best"
> > 
> 
> This tosses out that behavior. Do you feel it's important to preserve
> it somehow?
> 

FWIW, I'll also note that the sec= documentation is pretty sparse:

       sec=
           Security mode. Allowed values are: (blah)

...so I'm not sure that we have any sort of "social contract" to
preserve the existing behavior of multiple sec= options.

-- 
Jeff Layton <[email protected]>