[PATCHES] SSH: Fix several client bugs

[Jan Cholasta]

Hi,

the attached patches fix https://fedorahosted.org/sssd/ticket/1897. 
See commit messages for more info. Backward compatibility with older 
clients is maintained.

Honza

[Jakub Hrozek]

On Fri, Apr 26, 2013 at 02:49:05PM +0200, Jan Cholasta wrote:
> Hi,
>
> the attached patches fix
> https://fedorahosted.org/sssd/ticket/1897. See commit messages for
> more info. Backward compatibility with older clients is maintained.
>
> Honza
>
> -- 
> Jan Cholasta

[PATCH 1/4] UTIL: Add function sss_names_init_static
The functionality is OK, but I would prefer a different name..maybe
something like sss_names_init_ex or sss_names_init_pattern ? _static
sounds to me like the patterns are hardcoded internally.

[PATCH 2/4] SSH: Fix parsing of names from client requests
Can you please set is_user=false in sss_ssh_cmd_get_host_pubkeys()
explicitly so that a search for is_user makes it clear what it is set to
in different functions?
> @@ -675,6 +676,7 @@ static errno_t
>  ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx)
>  {
>      struct cli_ctx *cctx = cmd_ctx->cctx;
> struct ssh_ctx *ssh_ctx = (struct ssh_ctx *)cctx->rctx->pvt_ctx;

Please use talloc_get_type here so that if there was a type mismatch, we
would get an error message in the core file.

[PATCH 3/4] SSH: Use separate field for domain name in client
Looks good to me and Honza confirmed that the changes are
backwards-compatible.

[PATCH 4/4] SSH: Do not skip domains with use_fully_qualified_names
Ack
Why were the domains skipped in the first place?

[Jan Cholasta]

On 7.5.2013 10:30, Jakub Hrozek wrote:
> On Fri, Apr 26, 2013 at 02:49:05PM +0200, Jan Cholasta wrote:
> > Hi,
> >
> > the attached patches fix
> > https://fedorahosted.org/sssd/ticket/1897. See commit messages for
> > more info. Backward compatibility with older clients is maintained.
> >
> > Honza
> >
> > --
> > Jan Cholasta
>
> [PATCH 1/4] UTIL: Add function sss_names_init_static
> The functionality is OK, but I would prefer a different name..maybe
> something like sss_names_init_ex or sss_names_init_pattern ? _static
> sounds to me like the patterns are hardcoded internally.

I renamed the function to sss_names_init_from_args. I have also moved 
default value assignment back to sss_names_init, as I think it makes 
more sense to have it in there.
> [PATCH 2/4] SSH: Fix parsing of names from client requests
> Can you please set is_user=false in sss_ssh_cmd_get_host_pubkeys()
> explicitly so that a search for is_user makes it clear what it is set to
> in different functions?

Sure.
> > @@ -675,6 +676,7 @@ static errno_t
> >   ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx)
> >   {
> >       struct cli_ctx *cctx = cmd_ctx->cctx;
> > struct ssh_ctx *ssh_ctx = (struct ssh_ctx *)cctx->rctx->pvt_ctx;
>
> Please use talloc_get_type here so that if there was a type mismatch, we
> would get an error message in the core file.

Done.
> [PATCH 3/4] SSH: Use separate field for domain name in client
> Looks good to me and Honza confirmed that the changes are
> backwards-compatible.
>
> [PATCH 4/4] SSH: Do not skip domains with use_fully_qualified_names
> Ack
> Why were the domains skipped in the first place?

I initially handled hosts exactly the same way as users. As it turns 
out, that was not a very good idea.

Updated patches attached.

Honza

[Jakub Hrozek]

On Tue, May 07, 2013 at 11:51:53AM +0200, Jan Cholasta wrote:
> On 7.5.2013 10:30, Jakub Hrozek wrote:
> > On Fri, Apr 26, 2013 at 02:49:05PM +0200, Jan Cholasta wrote:
> > > Hi,
> > >
> > > the attached patches fix
> > > https://fedorahosted.org/sssd/ticket/1897. See commit messages for
> > > more info. Backward compatibility with older clients is maintained.
> > >
> > > Honza
> > >
> > > --
> > > Jan Cholasta
> >
> > [PATCH 1/4] UTIL: Add function sss_names_init_static
> > The functionality is OK, but I would prefer a different name..maybe
> > something like sss_names_init_ex or sss_names_init_pattern ? _static
> > sounds to me like the patterns are hardcoded internally.
>
> I renamed the function to sss_names_init_from_args. I have also
> moved default value assignment back to sss_names_init, as I think it
> makes more sense to have it in there.
> > [PATCH 2/4] SSH: Fix parsing of names from client requests
> > Can you please set is_user=false in sss_ssh_cmd_get_host_pubkeys()
> > explicitly so that a search for is_user makes it clear what it is set to
> > in different functions?
>
> Sure.
> > > @@ -675,6 +676,7 @@ static errno_t
> > >  ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx)
> > >  {
> > >      struct cli_ctx *cctx = cmd_ctx->cctx;
> > > struct ssh_ctx *ssh_ctx = (struct ssh_ctx *)cctx->rctx->pvt_ctx;
> >
> > Please use talloc_get_type here so that if there was a type mismatch, we
> > would get an error message in the core file.
>
> Done.
> > [PATCH 3/4] SSH: Use separate field for domain name in client
> > Looks good to me and Honza confirmed that the changes are
> > backwards-compatible.
> >
> > [PATCH 4/4] SSH: Do not skip domains with use_fully_qualified_names
> > Ack
> > Why were the domains skipped in the first place?
>
> I initially handled hosts exactly the same way as users. As it turns
> out, that was not a very good idea.
>
> Updated patches attached.
>
> Honza

Works for me, Ack.

[Jakub Hrozek]

On Tue, May 07, 2013 at 02:07:07PM +0200, Jakub Hrozek wrote:
> > Updated patches attached.
> >
> > Honza
>
> Works for me, Ack.

Pushed to master.