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

On Thu, 6 May 2010 17:29:17 -0500
Steve French <[email protected]> wrote:

> On Sat, Apr 24, 2010 at 6:57 AM, Jeff Layton <[email protected]> wrote:
> 
> > Allow secType to be set on a per-session basis. This allows us to mix
> > secTypes on a single socket. Also, add a "sign" bool to cifsSesInfo
> > and determine whether signing should be enabled at session setup
> > time.
> >
> >  enum securityEnum {
> > -       PLAINTXT = 0,           /* Legacy with Plaintext passwords */
> > +       Undefined = 0,          /* Uninitialized */
> > +       Anonymous,              /* Anonymous login */
> > +       Plaintext,              /* Legacy with plaintext passwords */
> >        LANMAN,                 /* Legacy LANMAN auth */
> >        NTLM,                   /* Legacy NTLM012 auth with NTLM hash */
> >        NTLMv2,                 /* Legacy NTLM auth with NTLMv2 hash */
> > -       RawNTLMSSP,             /* NTLMSSP without SPNEGO, NTLMv2 hash */
> > -/*     NTLMSSP, */ /* can use rawNTLMSSP instead of NTLMSSP via SPNEGO */
> > +       NTLMSSP,                /* NTLMSSP, NTLMv1 hash */
> >        Kerberos,               /* Kerberos via SPNEGO */
> >  };
> >
> >
> I like how this adds anonymous, and starts that with 1 rather than 0 - makes
> sense.
> 
> > @@ -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?

-- 
Jeff Layton <[email protected]>