[PATCH] sudo: print message if old protocol is used

[Pavel Březina]

I believe the patch explains itself.

[Simo Sorce]

On Thu, 2012-11-15 at 12:13 +0100, Pavel Březina wrote:
>  @@ -185,9 +186,14 @@ static int sudosrv_cmd(enum sss_sudo_type type,
>  struct cli_ctx *cli_ctx)
>       }
>   
>       /* if protocol is invalid return */
>  -    if (cli_ctx->cli_protocol_version->version !=
>  SSS_SUDO_PROTOCOL_VERSION) {
>  -        DEBUG(SSSDBG_FATAL_FAILURE, ("Invalid protocol! [%d]\n",
>  -              cli_ctx->cli_protocol_version->version));
>  +    if (protocol == 0) {
>  +        DEBUG(SSSDBG_FATAL_FAILURE, ("Protocol [%d] is not secure. "
>  +              "SSSD does not allow to use this protocol.\n",
>  protocol));
>  +        ret = EFAULT;
>  +        goto done;
>  +    } else if (protocol != SSS_SUDO_PROTOCOL_VERSION) {
>  +        DEBUG(SSSDBG_FATAL_FAILURE, ("Invalid protocol version [%d]!
>  \n",
>  +                                     protocol));
>           ret = EFAULT;
>           goto done;
>   
Nitpicking, but for this kind of situations I think a switch/case
statement would be much more clear (also avoids adding a variable as you
use it once):

switch(cli_ctx->cli_protocol_version->version) {
case 0:
    DEBUG(..);
    break;
case SSS_SUDO_PROTOCOL_VERSION:
    break;
default:
    DEBUG("invalid"...);
    break;
}

Simo.
-- 
Simo Sorce * Red Hat, Inc * New York

[Pavel Březina]

On 11/15/2012 02:30 PM, Simo Sorce wrote:
>  On Thu, 2012-11-15 at 12:13 +0100, Pavel Březina wrote:
> > @@ -185,9 +186,14 @@ static int sudosrv_cmd(enum sss_sudo_type type,
> > struct cli_ctx *cli_ctx)
> >       }
> >
> >       /* if protocol is invalid return */
> > -    if (cli_ctx->cli_protocol_version->version !=
> > SSS_SUDO_PROTOCOL_VERSION) {
> > -        DEBUG(SSSDBG_FATAL_FAILURE, ("Invalid protocol! [%d]\n",
> > -              cli_ctx->cli_protocol_version->version));
> > +    if (protocol == 0) {
> > +        DEBUG(SSSDBG_FATAL_FAILURE, ("Protocol [%d] is not secure. "
> > +              "SSSD does not allow to use this protocol.\n",
> > protocol));
> > +        ret = EFAULT;
> > +        goto done;
> > +    } else if (protocol != SSS_SUDO_PROTOCOL_VERSION) {
> > +        DEBUG(SSSDBG_FATAL_FAILURE, ("Invalid protocol version [%d]!
> > \n",
> > +                                     protocol));
> >           ret = EFAULT;
> >           goto done;
> >
>  Nitpicking, but for this kind of situations I think a switch/case
>  statement would be much more clear (also avoids adding a variable as you
>  use it once):
>
>  switch(cli_ctx->cli_protocol_version->version) {
>  case 0:
>       DEBUG(..);
>       break;
>  case SSS_SUDO_PROTOCOL_VERSION:
>       break;
>  default:
>       DEBUG("invalid"...);
>       break;
>  }
>
>  Simo.
>  
OK. New patch is attached.

[Jakub Hrozek]

On Thu, Nov 15, 2012 at 05:54:26PM +0100, Pavel Březina wrote:
>  >Nitpicking, but for this kind of situations I think a
> switch/case
>  >statement would be much more clear (also avoids adding a variable as you
>  >use it once):
>  >
>  >switch(cli_ctx->cli_protocol_version->version) {
>  >case 0:
>  >     DEBUG(..);
>  >     break;
>  >case SSS_SUDO_PROTOCOL_VERSION:
>  >     break;
>  >default:
>  >     DEBUG("invalid"...);
>  >     break;
>  >}
>  >
>  >Simo.
>  >
>  
>  OK. New patch is attached.
>   
Ack

[Jakub Hrozek]

On Thu, Nov 29, 2012 at 01:36:42PM +0100, Jakub Hrozek wrote:
>  On Thu, Nov 15, 2012 at 05:54:26PM +0100, Pavel Březina wrote:
>  > >Nitpicking, but for this kind of situations I think a switch/case
>  > >statement would be much more clear (also avoids adding a variable as you
>  > >use it once):
>  > >
>  > >switch(cli_ctx->cli_protocol_version->version) {
>  > >case 0:
>  > >     DEBUG(..);
>  > >     break;
>  > >case SSS_SUDO_PROTOCOL_VERSION:
>  > >     break;
>  > >default:
>  > >     DEBUG("invalid"...);
>  > >     break;
>  > >}
>  > >
>  > >Simo.
>  > >
>  > 
>  > OK. New patch is attached.
>  > 
>  
>  Ack 
Pushed to master.