Re: [PATCH] Add request_denied and unimplemented_packet callbacks on server side

[Nicolas Viennot <nicolas@xxxxxxxxxxx>]

Hi Aris,

Let me know if I can do anything to facilitate the review process.

Thanks,
Nico.

On Fri, Mar 4, 2016 at 4:46 AM, Aris Adamantiadis <aris@xxxxxxxxxxxx> wrote:
> Hi Andreas, Nicolas
>
> Let me some more time to review this patch.
>
> Aris
>
> On 2/03/16 13:21, Andreas Schneider wrote:
>> On Saturday 27 February 2016 02:41:34 Nicolas Viennot wrote:
>>> When dealing with keepalive packets sent from the server, the clients
>>> reply. Having a callback on that reply allows measuring client latency.
>>> However, clients behave differently:
>>> * libssh sends an unimplemented packet reply to keepalive requests.
>>> * other clients like openssh sends a request denied packet to keepalive
>>> requests.
>>> These callbacks allow the server to measure the latency to the clients when
>>> sending keepalive requests.
>>
>> Looks fine for me, Aris?
>>
>>
>>> ---
>>>  include/libssh/callbacks.h | 25 +++++++++++++++++++++++++
>>>  src/channels.c             |  5 +++++
>>>  src/packet.c               |  8 ++++++--
>>>  3 files changed, 36 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/libssh/callbacks.h b/include/libssh/callbacks.h
>>> index 20c540e..22f8b08 100644
>>> --- a/include/libssh/callbacks.h
>>> +++ b/include/libssh/callbacks.h
>>> @@ -296,6 +296,21 @@ typedef int (*ssh_gssapi_accept_sec_ctx_callback)
>>> (ssh_session session,
>>>  typedef int (*ssh_gssapi_verify_mic_callback) (ssh_session session,
>>>   ssh_string mic, void *mic_buffer, size_t mic_buffer_size, void *userdata);
>>>
>>> +/**
>>> + * @brief Handle a notification from the client that it received an
>>> unimplemented packet.
>>> + * @param session current session handler
>>> + * @param seq packet sequence number
>>> + * @param userdata Userdata to be passed to the callback function.
>>> + */
>>> +typedef void (*ssh_client_unimplemented_packet_callback) (ssh_session
>>> session, uint32_t seq, void *userdata);
>>> +
>>> +/**
>>> + * @brief Handle an unhandled client request denied callback.
>>> + * @param session current session handler
>>> + * @param userdata Userdata to be passed to the callback function.
>>> + */
>>> +typedef void (*ssh_client_request_denied_callback) (ssh_session
>>> session, void *userdata);
>>> +
>>>
>>>  /**
>>>   * This structure can be used to implement a libssh server, with
>>> appropriate callbacks.
>>> @@ -345,6 +360,16 @@ struct ssh_server_callbacks_struct {
>>>    /* This function will be called when a MIC needs to be verified.
>>>     */
>>>    ssh_gssapi_verify_mic_callback gssapi_verify_mic_function;
>>> +
>>> +  /** This function will be called when the client notifies the server of
>>> an +   * unimplemented packet.
>>> +   */
>>> +  ssh_client_unimplemented_packet_callback
>>> client_unimplemented_packet_function;
>>> +
>>> +  /** This function will be called when the client notifies the server of
>>> an +   * non handled deny of request.
>>> +   */
>>> +  ssh_client_request_denied_callback client_request_denied_function;
>>>  };
>>>  typedef struct ssh_server_callbacks_struct *ssh_server_callbacks;
>>>
>>> diff --git a/src/channels.c b/src/channels.c
>>> index ffcb1d5..6a179f9 100644
>>> --- a/src/channels.c
>>> +++ b/src/channels.c
>>> @@ -2039,6 +2039,11 @@ SSH_PACKET_CALLBACK(ssh_request_denied){
>>>    if(session->global_req_state != SSH_CHANNEL_REQ_STATE_PENDING){
>>>      SSH_LOG(SSH_LOG_RARE, "SSH_REQUEST_DENIED received in incorrect state
>>> %d", session->global_req_state);
>>> +
>>> +    if (ssh_callbacks_exists(session->server_callbacks,
>>> client_request_denied_function)){
>>> +      session->server_callbacks->client_request_denied_function(
>>> +        session, session->server_callbacks->userdata);
>>> +    }
>>>    } else {
>>>      session->global_req_state=SSH_CHANNEL_REQ_STATE_DENIED;
>>>    }
>>> diff --git a/src/packet.c b/src/packet.c
>>> index 7d6957c..7b98811 100644
>>> --- a/src/packet.c
>>> +++ b/src/packet.c
>>> @@ -464,8 +464,7 @@ SSH_PACKET_CALLBACK(ssh_packet_unimplemented){
>>>      uint32_t seq;
>>>      int rc;
>>>
>>> -    (void)session; /* unused */
>>> -    (void)type;
>>> +    (void)type; /* unused */
>>>      (void)user;
>>>
>>>      rc = ssh_buffer_unpack(packet, "d", &seq);
>>> @@ -477,6 +476,11 @@ SSH_PACKET_CALLBACK(ssh_packet_unimplemented){
>>>      SSH_LOG(SSH_LOG_RARE,
>>>              "Received SSH_MSG_UNIMPLEMENTED (sequence number %d)",seq);
>>>
>>> +    if (ssh_callbacks_exists(session->server_callbacks,
>>> client_unimplemented_packet_function)){
>>> +      session->server_callbacks->client_unimplemented_packet_function(
>>> +        session, seq, session->server_callbacks->userdata);
>>> +    }
>>> +
>>>      return SSH_PACKET_USED;
>>>  }
>
>