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

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?

-- 
Jeff Layton <[email protected]>