Where does the value of the timeout parameter in `client_net_recv_timeout` come from?

[Bascy]

We are having troubles with downloading files through an EthernetClient - SSLClient - ArduinoHttpClient connection because the timeout parameter with which client_net_recv_timeout is called is 0. This means the method will not wait for data to be received and that is causing problems. But I cannot find where the timeout parameter can be set ...

The client_net_recv_timeout method is only used in code when setting a callback in mbedtls_ssl_set_bio() (line335) and I do see a timeout parameter in the call to start_ssl_client() but that does not seem to be used anywhere.

How can I make this work?

[RobertByrnes]

HI @Bascy

For now I would try changing the reference to setTimeout in SSLClient.h to:

int setTimeout(uint32_t seconds);

and adding the implementation of this function to SSLClient.cpp:

int SSLClient::setTimeout(uint32_t seconds) { _timeout = seconds * 1000; }

If that works then please let me know or open a PR with it in. We can then look at some refactoring as there will be no need to pass it around through function params if it is encapsulated with the class...

[Bascy]

Implementing the SSLClient::setTimeout didn't change anything. the value set with that method is not propagated to the client_net_recv_timeout() method.

[RobertByrnes]

Ok need to spend longer figure out what gets passed where....however, I wonder if you try setting timeout in the client_net_recv_timeout() to _timeout, or add a conditional (obviously you'd have to setTimeout earlier)

if (timeout == 0) { timeout = _timeout; }

at the top of the function. Could be worth a try. The underlying mbedtls calls this callback and is responsible for passing in the parameters. I'd be curious to know what timeout is getting passed as - must be 0?

[Bascy]

That is the solution we choose for now, but I don't like having to hardcode a number like that in a external repository. And besides that, I am curious where the timeout parameter value comes from and why it cannot be managed from the SSLClient. BTW SSLClient::_timeout field is not available in the ssl_client.cpp source

int client_net_recv_timeout(void* ctx, unsigned char* buf,
                            size_t len, uint32_t timeout) {
  Client* client = (Client*)ctx;

  //Set a default timeout as it seems it is not always set the way we use the ssl client
  // a timeout of 0
  uint32_t _timeout = 100;
  if (timeout > 0) {
    _timeout = timeout;
  }

  if (!client) {
    log_e("Uninitialised!");
    return -1;
  }
  unsigned long start = millis();
  unsigned long tms = start + _timeout;
  do {
    int pending = client->available();
    //Ethernet(?) never reads larger chunks than 2048, so no need waiting any longer
    if (pending > 2048) {
      break;
    }

    //read last bit or wait on timeout
    if (pending < len) {
      delay(1);
    } else {
      break;
    }
  } while (millis() < tms);

  int result = client->read(buf, len);

  if (!result) {
    return MBEDTLS_ERR_SSL_WANT_READ;
  }

  log_v("SSL client RX (received=%d expected=%d in %dms)", result, len, millis() - start);

  if (result > 0) {
    //esp_log_buffer_hexdump_internal("SSL.RD", buf, (uint16_t)result, ESP_LOG_VERBOSE);
  }

  return result;
}

[RobertByrnes]

I am still thinking it might be managable from SSLClient context - as this is passed to mbedtls. I am struggling to find where in mbedtls it actually calls the callback though ...

typedef struct sslclient_context {
  Client* client;

  mbedtls_ssl_context ssl_ctx;
  mbedtls_ssl_config ssl_conf;

  mbedtls_ctr_drbg_context drbg_ctx;
  mbedtls_entropy_context entropy_ctx;

  mbedtls_x509_crt ca_cert;
  mbedtls_x509_crt client_cert;
  mbedtls_pk_context client_key;

  unsigned long handshake_timeout;
} sslclient_context;

mbedtls_ssl_context ssl_ctx; the callback function gets passed into here...

[RobertByrnes]

That is the solution we choose for now, but I don't like having to hardcode a number like that in a external repository. And besides that, I am curious where the timeout parameter value comes from and why it cannot be managed from the SSLClient. BTW SSLClient::_timeout field is not available in the ssl_client.cpp source

int client_net_recv_timeout(void* ctx, unsigned char* buf,
                            size_t len, uint32_t timeout) {
  Client* client = (Client*)ctx;

  //Set a default timeout as it seems it is not always set the way we use the ssl client
  // a timeout of 0
  uint32_t _timeout = 100;
  if (timeout > 0) {
    _timeout = timeout;
  }

  if (!client) {
    log_e("Uninitialised!");
    return -1;
  }
  unsigned long start = millis();
  unsigned long tms = start + _timeout;
  do {
    int pending = client->available();
    //Ethernet(?) never reads larger chunks than 2048, so no need waiting any longer
    if (pending > 2048) {
      break;
    }

    //read last bit or wait on timeout
    if (pending < len) {
      delay(1);
    } else {
      break;
    }
  } while (millis() < tms);

  int result = client->read(buf, len);

  if (!result) {
    return MBEDTLS_ERR_SSL_WANT_READ;
  }

  log_v("SSL client RX (received=%d expected=%d in %dms)", result, len, millis() - start);

  if (result > 0) {
    //esp_log_buffer_hexdump_internal("SSL.RD", buf, (uint16_t)result, ESP_LOG_VERBOSE);
  }

  return result;
}

@Bascy Does this workaround solve the issue for now?

[Bascy]

yes it does

[Bascy]

I am still thinking it might be managable from SSLClient context - as this is passed to mbedtls. I am struggling to find where in mbedtls it actually calls the callback though ...

typedef struct sslclient_context {
  Client* client;

  mbedtls_ssl_context ssl_ctx;
  mbedtls_ssl_config ssl_conf;

  mbedtls_ctr_drbg_context drbg_ctx;
  mbedtls_entropy_context entropy_ctx;

  mbedtls_x509_crt ca_cert;
  mbedtls_x509_crt client_cert;
  mbedtls_pk_context client_key;

  unsigned long handshake_timeout;
} sslclient_context;

mbedtls_ssl_context ssl_ctx; the callback function gets passed into here...

It must be called from int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len );

[Bascy]

Another question on this method:

In the while loop, the method available() will return 1 if the client is not connected (anymore) Shouldn't we handle this value of pending separately by returning a 0 or MBEDTLS_ERR_SSL_WANT_READ ?

do {
    int pending = client->available(); // this can return 1 if the connection is closed!
    //Ethernet(?) never reads larger chunks than 2048, so no need waiting any longer
    if (pending > 2048) {
      break;
    }

    //read last bit or wait on timeout
    if (pending < len) {
      delay(1);
    } else {
      break;
    }
  } while (millis() < tms);

[RobertByrnes]

@Bascy

docs regarding this and use of mbedtls_ssl_set_bio()

I wonder if the client is missing a call to:

mbedtls_ssl_conf_read_timeout(&ssl_client->ssl_conf, timeout); // timeout should be in milliseconds.

Looks like this should happen before call to mbedtls_ssl_read() so can be added immediately after call to mbedtls_ssl_set_bio() in start_ssl_client(), somewhere around line 336 of ssl_client.cpp.

could be worth a try...

if that works then no need to mess around inside client_net_recv_timeout() and instead can implement a proper fix with a constructor or a setter in the SSLCLient.

[RobertByrnes]

For now you could trying using the SSLClient::setHandshakeTimeout() method to set the timeout and then passing:

mbedtls_ssl_conf_read_timeout(&ssl_client->ssl_conf, ssl_client->handshake_timeout);

at least we know if is working then...

[Bascy]

This works! I'll create a PR for this

Have you seen my other question regarding available() returning 1 when not connected? [Edit] Never mind, I now see it only returns 1 if a byte has been peeked and that byte should of course be read