Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect timeout with SSL #8899

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,34 @@ void loop() {

std::unique_ptr<BearSSL::WiFiClientSecure> client(new BearSSL::WiFiClientSecure);

#if 1 // 1=secure, 0=insecure

client->setFingerprint(fingerprint_sni_cloudflaressl_com);

// date needs to be setup
configTime("UTC", "pool.ntp.org");
time_t now;
while (time(&now) < 24 * 3600) {
Serial.println("waiting for NTP time to be set...");
delay(1000);
}
Serial.printf("date: %s", ctime(&now));

#else
// Or, if you happy to ignore the SSL certificate, then use the following line instead:
// client->setInsecure();
client->setInsecure();
#endif

HTTPClient https;
https.setTimeout(4000); // or: client->setTimeout(4000);
client->setHandshakeTimeout(10000);

#if 1
constexpr int sslbufsize = 1024;
bool mfln = client->probeMaxFragmentLength(jigsaw_host, jigsaw_port, sslbufsize);
Serial.printf("Can reduce SSL footprint to %d bytes in RAM: %s\n", sslbufsize, mfln ? "yes" : "no");
if (mfln) { client->setBufferSizes(sslbufsize, sslbufsize); }
#endif

Serial.print("[HTTPS] begin...\n");
if (https.begin(*client, jigsaw_host, jigsaw_port)) { // HTTPS
Expand Down
14 changes: 10 additions & 4 deletions libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ bool HTTPClient::begin(WiFiClient &client, const String& url) {

_port = (protocol == "https" ? 443 : 80);
_client = client.clone();
if (_deferredTimeout) {
_client->setTimeout(_deferredTimeout);
}

return beginInternal(url, protocol.c_str());
}
Expand All @@ -109,6 +112,9 @@ bool HTTPClient::begin(WiFiClient &client, const String& host, uint16_t port, co
}

_client = client.clone();
if (_deferredTimeout) {
_client->setTimeout(_deferredTimeout);
}

clear();

Expand Down Expand Up @@ -308,10 +314,12 @@ void HTTPClient::setAuthorization(String auth)
*/
void HTTPClient::setTimeout(uint16_t timeout)
{
_tcpTimeout = timeout;
if(connected()) {
_client->setTimeout(timeout);
}
else {
_deferredTimeout = timeout;
}
}

/**
Expand Down Expand Up @@ -794,8 +802,6 @@ bool HTTPClient::connect(void)
return false;
}

_client->setTimeout(_tcpTimeout);

if(!_client->connect(_host.c_str(), _port)) {
DEBUG_HTTPCLIENT("[HTTP-Client] failed connect to %s:%u\n", _host.c_str(), _port);
return false;
Expand Down Expand Up @@ -978,7 +984,7 @@ int HTTPClient::handleHeaderResponse()
}

} else {
if((millis() - lastDataTime) > _tcpTimeout) {
if((millis() - lastDataTime) > _client.get()->getTimeout()) {
return HTTPC_ERROR_READ_TIMEOUT;
}
esp_yield();
Expand Down
4 changes: 1 addition & 3 deletions libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@
#define DEBUG_HTTPCLIENT(...) do { (void)0; } while (0)
#endif

#define HTTPCLIENT_DEFAULT_TCP_TIMEOUT (5000)

/// HTTP client errors
#define HTTPC_ERROR_CONNECTION_FAILED (-1)
#define HTTPC_ERROR_SEND_HEADER_FAILED (-2)
Expand Down Expand Up @@ -256,7 +254,7 @@ class HTTPClient
String _host;
uint16_t _port = 0;
bool _reuse = true;
uint16_t _tcpTimeout = HTTPCLIENT_DEFAULT_TCP_TIMEOUT;
uint16_t _deferredTimeout = 0;
bool _useHTTP10 = false;

String _uri;
Expand Down
6 changes: 6 additions & 0 deletions libraries/ESP8266WiFi/src/WiFiClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ size_t WiFiClient::write(Stream& stream)
return 0;
}
// core up to 2.7.4 was equivalent to this
_client->setTimeout(_timeout);
return stream.sendAll(this);
}

Expand Down Expand Up @@ -266,11 +267,13 @@ int WiFiClient::read()

int WiFiClient::read(uint8_t* buf, size_t size)
{
_client->setTimeout(_timeout);
return (int)_client->read((char*)buf, size);
}

int WiFiClient::read(char* buf, size_t size)
{
_client->setTimeout(_timeout);
return (int)_client->read(buf, size);
}

Expand All @@ -279,6 +282,7 @@ int WiFiClient::peek()
if (!available())
return -1;

_client->setTimeout(_timeout);
return _client->peek();
}

Expand All @@ -300,6 +304,7 @@ size_t WiFiClient::peekBytes(uint8_t *buffer, size_t length) {
count = length;
}

_client->setTimeout(_timeout);
return _client->peekBytes((char *)buffer, count);
}

Expand Down Expand Up @@ -454,6 +459,7 @@ const char* WiFiClient::peekBuffer ()
// return number of byte accessible by peekBuffer()
size_t WiFiClient::peekAvailable ()
{
_client->setTimeout(_timeout);
return _client? _client->peekAvailable(): 0;
}

Expand Down
31 changes: 18 additions & 13 deletions libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,6 @@ extern "C" {
namespace BearSSL {

void WiFiClientSecureCtx::_clear() {
// TLS handshake may take more than the 5 second default timeout
_timeout = 15000;

_sc = nullptr;
_sc_svr = nullptr;
_eng = nullptr;
Expand All @@ -83,7 +80,7 @@ void WiFiClientSecureCtx::_clear() {
_now = 0; // You can override or ensure time() is correct w/configTime
_ta = nullptr;
setBufferSizes(16384, 512); // Minimum safe
_handshake_done = false;
_set_handshake_done(false); // refreshes _timeout
_recvapp_buf = nullptr;
_recvapp_len = 0;
_oom_err = false;
Expand Down Expand Up @@ -204,7 +201,13 @@ bool WiFiClientSecureCtx::stop(unsigned int maxWaitMs) {
}

bool WiFiClientSecureCtx::flush(unsigned int maxWaitMs) {
auto savedNormal = _normalTimeout;
auto savedHandshake = _handshakeTimeout;
_normalTimeout = maxWaitMs;
_handshakeTimeout = maxWaitMs;
(void) _run_until(BR_SSL_SENDAPP);
_normalTimeout = savedNormal;
_handshakeTimeout = savedHandshake;
mcspr marked this conversation as resolved.
Show resolved Hide resolved
return WiFiClient::flush(maxWaitMs);
}

Expand Down Expand Up @@ -245,8 +248,7 @@ void WiFiClientSecureCtx::_freeSSL() {
_recvapp_buf = nullptr;
_recvapp_len = 0;
// This connection is toast
_handshake_done = false;
_timeout = 15000;
_set_handshake_done(false); // refreshes _timeout
}

bool WiFiClientSecureCtx::_clientConnected() {
Expand Down Expand Up @@ -461,8 +463,9 @@ size_t WiFiClientSecureCtx::peekBytes(uint8_t *buffer, size_t length) {
return 0;
}

_updateStreamTimeout();
_startMillis = millis();
while ((_pollRecvBuffer() < (int) length) && ((millis() - _startMillis) < 5000)) {
while ((_pollRecvBuffer() < (int)length) && ((millis() - _startMillis) < _timeout)) {
yield();
}

Expand All @@ -483,7 +486,10 @@ int WiFiClientSecureCtx::_run_until(unsigned target, bool blocking) {
return -1;
}

esp8266::polledTimeout::oneShotMs loopTimeout(_timeout);
// _run_until() is called prior to inherited read/write methods
// -> refreshing _timeout here, which is also used by ancestors
DEBUG_BSSL("_run_until starts, timeout=%lu\n", _updateStreamTimeout());
esp8266::polledTimeout::oneShotMs loopTimeout(_updateStreamTimeout());

for (int no_work = 0; blocking || no_work < 2;) {
optimistic_yield(100);
Expand Down Expand Up @@ -602,15 +608,15 @@ int WiFiClientSecureCtx::_run_until(unsigned target, bool blocking) {
}

bool WiFiClientSecureCtx::_wait_for_handshake() {
_handshake_done = false;
_set_handshake_done(false); // refreshes _timeout
while (!_handshake_done && _clientConnected()) {
int ret = _run_until(BR_SSL_SENDAPP);
if (ret < 0) {
DEBUG_BSSL("_wait_for_handshake: failed\n");
break;
}
if (br_ssl_engine_current_state(_eng) & BR_SSL_SENDAPP) {
_handshake_done = true;
_set_handshake_done(true); // refreshes _timeout
}
optimistic_yield(1000);
}
Expand Down Expand Up @@ -1206,8 +1212,7 @@ bool WiFiClientSecureCtx::_connectSSL(const char* hostName) {
_x509_insecure = nullptr;
_x509_knownkey = nullptr;

// reduce timeout after successful handshake to fail fast if server stop accepting our data for whathever reason
if (ret) _timeout = 5000;
// _timeout has been refreshed to normal operation as _handshake_done turned to true

return ret;
}
Expand Down Expand Up @@ -1673,4 +1678,4 @@ bool WiFiClientSecure::probeMaxFragmentLength(IPAddress ip, uint16_t port, uint1
return _SendAbort(probe, supportsLen);
}

};
}; // namespace BearSSL
62 changes: 43 additions & 19 deletions libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@

namespace BearSSL {

class WiFiClientSecure;

class WiFiClientSecureCtx : public WiFiClient {
public:
WiFiClientSecureCtx();
Expand Down Expand Up @@ -147,6 +149,10 @@ class WiFiClientSecureCtx : public WiFiClient {
// consume bytes after use (see peekBuffer)
virtual void peekConsume (size_t consume) override;

void setNormalTimeout (unsigned long timeout) { _normalTimeout = timeout; }
void setHandshakeTimeout (unsigned long timeout) { _handshakeTimeout = timeout; }
unsigned long getHandshakeTimeout () const { return _handshakeTimeout; }

protected:
bool _connectSSL(const char *hostName); // Do initial SSL handshake

Expand Down Expand Up @@ -235,6 +241,14 @@ class WiFiClientSecureCtx : public WiFiClient {
bool _installServerX509Validator(const X509List *client_CA_ta); // Setup X509 client cert validation, if supplied

uint8_t *_streamLoad(Stream& stream, size_t size);

// timeout management

unsigned long _updateStreamTimeout () { return _timeout = _handshake_done? _normalTimeout: _handshakeTimeout; }
void _set_handshake_done (bool handshake_done) { _handshake_done = handshake_done; _updateStreamTimeout(); }

unsigned long _normalTimeout = 5000, _handshakeTimeout = 15000;

}; // class WiFiClientSecureCtx


Expand Down Expand Up @@ -263,24 +277,24 @@ class WiFiClientSecure : public WiFiClient {
std::unique_ptr<WiFiClient> clone() const override { return std::unique_ptr<WiFiClient>(new WiFiClientSecure(*this)); }

uint8_t status() override { return _ctx->status(); }
int connect(IPAddress ip, uint16_t port) override { return _ctx->connect(ip, port); }
int connect(const String& host, uint16_t port) override { return _ctx->connect(host, port); }
int connect(const char* name, uint16_t port) override { return _ctx->connect(name, port); }

uint8_t connected() override { return _ctx->connected(); }
size_t write(const uint8_t *buf, size_t size) override { return _ctx->write(buf, size); }
size_t write_P(PGM_P buf, size_t size) override { return _ctx->write_P(buf, size); }
size_t write(const char *buf) { return write((const uint8_t*)buf, strlen(buf)); }
size_t write_P(const char *buf) { return write_P((PGM_P)buf, strlen_P(buf)); }
size_t write(Stream& stream) /* Note this is not virtual */ { return _ctx->write(stream); }
int read(uint8_t *buf, size_t size) override { return _ctx->read(buf, size); }
int available() override { return _ctx->available(); }
int availableForWrite() override { return _ctx->availableForWrite(); }
int read() override { return _ctx->read(); }
int peek() override { return _ctx->peek(); }
size_t peekBytes(uint8_t *buffer, size_t length) override { return _ctx->peekBytes(buffer, length); }
bool flush(unsigned int maxWaitMs) { return _ctx->flush(maxWaitMs); }
bool stop(unsigned int maxWaitMs) { return _ctx->stop(maxWaitMs); }
int connect(IPAddress ip, uint16_t port) override { uto(); return _ctx->connect(ip, port); }
int connect(const String& host, uint16_t port) override { uto(); return _ctx->connect(host, port); }
int connect(const char* name, uint16_t port) override { uto(); return _ctx->connect(name, port); }

uint8_t connected() override { uto(); return _ctx->connected(); }
size_t write(const uint8_t *buf, size_t size) override { uto(); return _ctx->write(buf, size); }
size_t write_P(PGM_P buf, size_t size) override { uto(); return _ctx->write_P(buf, size); }
size_t write(const char *buf) { uto(); return write((const uint8_t*)buf, strlen(buf)); }
size_t write_P(const char *buf) { uto(); return write_P((PGM_P)buf, strlen_P(buf)); }
size_t write(Stream& stream) /* Note this is not virtual */ { uto(); return _ctx->write(stream); }
int read(uint8_t *buf, size_t size) override { uto(); return _ctx->read(buf, size); }
int available() override { uto(); return _ctx->available(); }
int availableForWrite() override { uto(); return _ctx->availableForWrite(); }
int read() override { uto(); return _ctx->read(); }
int peek() override { uto(); return _ctx->peek(); }
size_t peekBytes(uint8_t *buffer, size_t length) override { uto(); return _ctx->peekBytes(buffer, length); }
bool flush(unsigned int maxWaitMs) { uto(); return _ctx->flush(maxWaitMs); }
bool stop(unsigned int maxWaitMs) { uto(); return _ctx->stop(maxWaitMs); }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d-a-v/Arduino@ssltimeout...mcspr:esp8266-Arduino:ssltimeout ?

reiterating on the matrix discussion - no need to touch _timeout when not necessary, ctx class can have its own timeout state
implementing method override should fix the after-connect setting of timeout as well

Copy link
Collaborator Author

@d-a-v d-a-v Apr 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_ctx is not accessible from user side.
sslclient.setTimeout() can be called from the Stream class (Stream& s = sslclient; s.setTimeout(1234);)
Stream::setTimeout is not virtual and I'm not sure that modifying Arduino API is not going to be a breaking change but you tell me.
Stream::_timeout updates from client side are ignored until forwarded to the context _ctx.
Considering that user can update this timeout anytime, I placed these many calls where _timeout is used.

edit This commit reverts this one where there was an unwanted pointer of the client stream inside the context to help reaching user timeout.

edit2 now Stream::setTimeout() is virtual

void flush() override { (void)flush(0); }
void stop() override { (void)stop(0); }

Expand Down Expand Up @@ -349,7 +363,7 @@ class WiFiClientSecure : public WiFiClient {
virtual bool hasPeekBufferAPI () const override { return true; }

// return number of byte accessible by peekBuffer()
virtual size_t peekAvailable () override { return _ctx->available(); }
virtual size_t peekAvailable () override { return available(); }

// return a pointer to available data buffer (size = peekAvailable())
// semantic forbids any kind of read() before calling peekConsume()
Expand All @@ -358,6 +372,10 @@ class WiFiClientSecure : public WiFiClient {
// consume bytes after use (see peekBuffer)
virtual void peekConsume (size_t consume) override { return _ctx->peekConsume(consume); }

// allowing user to set timeout used during handshake
void setHandshakeTimeout (unsigned long timeout) { _ctx->setHandshakeTimeout(timeout); }
unsigned long getHandshakeTimeout () const { return _ctx->getHandshakeTimeout(); }

private:
std::shared_ptr<WiFiClientSecureCtx> _ctx;

Expand All @@ -375,6 +393,12 @@ class WiFiClientSecure : public WiFiClient {
_ctx(new WiFiClientSecureCtx(client, chain, sk, iobuf_in_size, iobuf_out_size, cache, client_CA_ta, tls_min, tls_max)) {
}

// (because Stream::setTimeout() is not virtual,)
// forward user timeout from Stream:: to SSL context
// this is internally called on every user operations
inline void uto () { _ctx->setNormalTimeout(_timeout); }


}; // class WiFiClientSecure

}; // namespace BearSSL
Expand Down