Re: a small patch

[Aris Adamantiadis <aris@xxxxxxxxxxxx>]

Hi Xi,

Here are some comment about your patches. First of all, thank you for
contributing to libssh.
When possible, you should split the patches in small hunks so it's
easier to select and comment.
http://www.clearchain.com/blog/posts/splitting-a-patch gives a nice
tool to do this.

> 
> * libssh runs into a wrong state when connecting to a server that
> supports keyboard-interactive (e.g., linux.mit.edu), "authenticated"
> before asking for password.
> 
> In ssh_userauth_kbdint (auth.c), first the code checks if
> session->kbdint == NULL, then goes to ask_userauth to send
> "ssh-userauth" and call ssh_handle_packets, where it will be
> dispatched to ssh_packet_userauth_pk_ok.  Note that session->kbdint is
> still NULL, session->auth_state will then be set to
> SSH_AUTH_STATE_PK_OK, though it should have been SSH_AUTH_STATE_INFO.
> One possible fix is to initialize session->kbinit right after the NULL
> check.

While I acknowledge the problem, I will not apply your patch, I think
it's not the appropriate place to set the pointer. I will find a better
solution, thanks for pointing this anyway.

> 
> * two possible infinite loops in auth.c:218 and auth1.c:38, which can
> be avoided by checking the return value of ssh_handle_packets.
Applied
> 
> * sample.c:451 calls ssh_userauth_none then later calls
> authenticate_console.  actually authenticate_console will invoke
> ssh_userauth_none again, which fails some server (e.g., bbs@xxxxxx).
ssh_userauth_none is an important call at this step because it enables
the banner that some servers send. I will move the fix back to
ssh_userauth_none() which should not send twice the request.

> 
> The attached patch also includes the fixes mentioned in my previous email.
> 
in packet1.c, I moved the session state change in ssh_auth1_handler to
avoid breaking the layering of the callback mechanism.

So in short, I will rewrite the ssh_userauth_none and
ssh_userauth_kbdint() patches. Thanks for your contribution.

Aris
> -xi
> 
> On Wed, Apr 21, 2010 at 5:08 AM, Aris Adamantiadis <aris@xxxxxxxxxxxx> wrote:
>> Hi Xi,
>>
>> Thanks for your patch. Libssh master is currently in architectural
>> rework and SSH1 has been a little less polished than the rest. I'm happy
>>  you managed to make it work.
>>
>> I have yet to verify that the \r\n -> \n thing does not break anything
>> on openssh and cisco SSH.
>>
>> Regards,
>>
>> Aris
>>
>> Xi Wang a écrit :
>>> Cool.  Here goes a few more fixes when I was trying libssh with
>>> guest@xxxxxxxxxxx, an ssh-based forum using v1.
>>>
>>> * banner/identification: ssh_send_banner (client.c).  libssh reports
>>> crc error after sending the session key.  The actual problem is that
>>> libssh uses "...\r\n" in its identification, while some sever doesn't
>>> parse the identification correctly and stops at "\r".  Since the ssh
>>> specification (http://www.snailbook.com/docs/protocol-1.5.txt) says it
>>> should be "...\n", the patch removes "\r".
>>>
>>> * hang (packet1.c:337).  libssh hangs after authentication.  I guess
>>> it's because libssh doesn't run into the correct state, i.e., from
>>> SSH_SESSION_STATE_AUTHENTICATING to SSH_SESSION_STATE_AUTHENTICATED.
>>> Not sure if the fix is at the right place.
>>>
>>> * close: ssh_connection_callback (client.c:645).  When in error state
>>> (e.g., the remote server closes the connection), libssh/samplessh
>>> doesn't close automatically.  Not sure if this fix is necessary.
>>>
>>> * samplessh hangs (sample.c), for example, when channel_poll returns -1 (error).
>>>
>>> Thanks.
>>>
>>> - xi
>>>
>>> On Tue, Apr 20, 2010 at 7:32 AM, Andreas Schneider <mail@xxxxxxxxxxxx> wrote:
>>>> On Tuesday 20 April 2010 13:05:45 you wrote:
>>>>> Hi,
>>>> Hi Xi,
>>>>
>>>>> Attached is a patch that addresses the following problems.
>>>>>
>>>>> * two memory leaks: enc_session (kex.c:781), session->packet_callbacks
>>>>> (session.c:209);
>>>>>
>>>>> * openssl configuration problem: use OPENSSL_FOUND instead of
>>>>> CRYPTO_FOUND, which doesn't exist in my cmake 2.8.
>>>>>
>>>>> * compile warnings: avoid using the name "signal" (channels.c);
>>>>>
>>>>> * link error when WITH_SERVER=0 (packet.c:59);
>>>>>
>>>>> * minor compile error (torture_options.c).
>>>> thank you very much for your patch. I've split it up in smaller patches and
>>>> pushed it to the repository.
>>>>
>>>>
>>>> Cheers,
>>>>
>>>>        -- andreas
>>>>
>>>>
>>>>
>>
>>