clone #141 -1
retitle -1 missing public SSH key file should throw an error
severity -1 minor
tag #141 pending
fixed #141 4.0.1.1
thanks

Hi Anders,

On Sa 20 Apr 2013 20:52:39 CEST Mike Gabriel wrote:

> Analysis of the bug:
> When autologin is enabled, SshMasterConnection::userAuth() will react by
> calling userAuthAuto(), which will look for ssh keys and if you, like me,
> have an ssh key with a passphrase, it will want to try out this key by
> asking for the passphrase (despite having ssh-agent running). If it does
> not find a key, it also asks for a passphrase, at least on my system. The
> reasons for this aren't really important here, in my oppinion. The
> important question here is why it even looks for other keys when the nice
> broker has provided a key. Further analysis and testing showed me that
> after userAuthAuto() exists without having gotten a proper key loaded (by
> pressing Cancel on the dialog box), userAuth() will then test if a key is
> loaded. And because httpbrokerclient has recieved a key and put it into the
> config-variable, a key IS available. This key is then used for login and
> all is good. Looking closer at the code revealed that setting
> config->autologin to true was actually not needed at all, and is the
> culprit here. If autologin is false, then userAuth() will still see that
> there is a key loaded, and happily log in the user.

Thanks for this detailled analysis. It indeed put me on some trail  
that worked.

> My naive fix for this bug:
> In ONMainWindow::startSession(), make setting the autologin variable
> dependent upon not being in brokerMode:
>
> diff --git a/onmainwindow.cpp b/onmainwindow.cpp
> index 31dbc17..bc2b70f 100644
> --- a/onmainwindow.cpp
> +++ b/onmainwindow.cpp
> @@ -3249,8 +3249,9 @@ bool ONMainWindow::startSession ( const QString& sid )
>
>      QString cmd=st->setting()->value ( sid+"/command",
>                                         ( QVariant ) QString::null
> ).toString();
> -    autologin=st->setting()->value ( sid+"/autologin",
> -                                     ( QVariant ) false ).toBool();
> +    if (!brokerMode)
> +        autologin=st->setting()->value ( sid+"/autologin",
> +                                         ( QVariant ) false ).toBool();
>      krblogin=st->setting()->value ( sid+"/krblogin",
>                                      ( QVariant ) false ).toBool();
>  #ifdef Q_OS_LINUX
>
> I can't say what other consequences this might have, not knowing the code
> well enough, but initial tests on my system shows that it works. This patch
> is against git/master btw.

The above fix is not appropriate as it will disable the autologin  
feature completely when x2goclient is in broker mode. That works for  
your setup, but is not a generic solution.

My approach is here:
http://code.x2go.org/gitweb?p=x2goclient.git;a=commitdiff;h=fe4408b12c982b81c56c52f37230865f4e9f41ea

@Alex: please cross-check. Thanks!

Greets,
Mike


-- 

DAS-NETZWERKTEAM
mike gabriel, rothenstein 5, 24214 neudorf-bornstein
fon: +49 (1520) 1976 148

GnuPG Key ID 0x25771B31
mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de

freeBusy:
https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xfb