TLS tickets and FreeRTOS workers

[ikostoski]

Two new API functions added:

HTTPSServer->enableTLSTickets(uint32_t liftimeSeconds = 86400, bool useHardwareRNG = false)
Which enables HTTPS server generating and accepting RFC5077 TLS tickets from clients for session negotiation vs. complete renegotiation for each TLS connection

HTTPServer->enableWorkers(uint8_t numWorkers = 2, size_t taskStackSize = HTTPS_CONN_TASK_STACK_SIZE, int taskPriority = HTTPS_CONN_TASK_PRIORITY);
Which creates n-FreeRTOS tasks that will takeover processing of server's loop() method. More than one task will enable parallel processing of connections as ESP32 has two cores and/or if single resourceNode's handler function is busy doing some longer processing

[fhessel]

Sorry it took so long, but this was quite a bit of code, and as you've merged the develop branch into your feature branch, GitHub displays those things as incoming changes, too. I hope I didn't mix things up that aren't relevant for this PR.

Many of the remarks are just styling issues, some of them are related to file descriptors (where 0 is actually valid), and some points may need a bit of discussion. I'd suggest to address those individually on the commented lines.

What I still need/want to do before merging is some performance testing in a more controlled environment (e.g. with a Python script to compare it the current master). The results for the example page in the browser already look great, but I want to play around a bit with client pool sizes, multiple clients and see how the server reacts in absence of Content-Length headers. The latter is only important until #15 gets implemented, I think.

And just so you know: I've a local copy of your branch that is already prepared for merging into develop, so you do not need to take care of the merge conflicts. I'll just wait before pushing it as that makes it easier for you to add more commits to the PR.

[fhessel]

This should be image/jpeg

Suggested change

	res->setHeader("Content-Type", "image/jpg");
	res->setHeader("Content-Type", "image/jpeg");

[fhessel]

I'd maybe use std::string filename = req->getRequestString() here and filename.c_str() when passing it as parameter. I'm aware of the confusion with Arduino's String and the standard C++ std::string, but the rest of the code doesn't use the Arduino variant, either, to allow better portability.

[fhessel]

I'd be happier with a snprintf(taskHandle, sizeof(taskHandle), ... if we can assume C++ 11

[fhessel]

Also, %p could be used again.

[fhessel]

Suggested change

	sprintf(taskHandle, "0x%08x", (uint32_t)xTaskGetCurrentTaskHandle());
	snprintf(taskHandle, sizeof(taskHandle), "%p", (uint32_t)xTaskGetCurrentTaskHandle());

[fhessel]

I see that it's handled by HTTPServer::stop(), but I think a destructor that waits for run() to terminate if _running==true would be a good idea, as run() accesses class members of this worker instance and the task is otherwise decoupled from the instance. So, at the moment, it depends on the user of the class to assure that the task has finished before calling delete on the worker.

Or at least something like this, to identify wrong usage:

Suggested change

	HTTPWorker::~HTTPWorker() {
	if (_running) {
	HTTPS_LOGE("Deleting worker while task is running!")
	}
	}

[fhessel]

This means the worker is actually _running before start() is called.

[fhessel]

I'd prefer is as a prefix to methods returning booleans which start with a verb. When I first read that method name, I thought it would correct the length, in the sense of fix or adjust.

(If you apply this suggestion, also apply the ones in HTTPResponse.cpp and HTTPConnection.cpp)

Suggested change

	bool correctContentLength();
	bool isCorrectContentLength();

[fhessel]

(see HTTPResponse.hpp)

Suggested change

	if (res.correctContentLength()) {
	if (res.isCorrectContentLength()) {

[fhessel]

This should not be += size but += whatever writeBytesInternal() returns.

[fhessel]

Failure of allocation is not handled.

[fhessel]

To me, it's not clear how this separation between accepting only the raw socket or performing the complete TLS handshake provides an advantage for checking if there is data.

HTTP Case:

• accept() runs the regular three-way-handshake, and the server is then able to check whether the connection is idling or not by selecting the socket or trying to read.
  → No advantage of separation

HTTPS Case:

• accept() does only the three-way-handshake. In this case, there will always be data on the line, which is the ClientHello record of the TLS connection.
• SSL_accept() removes the TLS handshake and the wrapping of the application data, so we need to call that function to see if there's application data on the line
  → We need both before we can know if the client actually uses this connection.

And if it's about the memory overhead of the pending TLS connection, I'd just drop the idling connection before accepting the new one.

[fhessel]

I now tried a while to compare the performance of the previous version of the server against this PR in various configurations. But using the following sketch and these scripts to measure the performance (to have more control over the number of clients and connection pool properties), I get really strange behavior when exceeding the 2 connections or workers, even on boards with an additional 4MB of RAM and even for HTTP.

main.cpp

#include <Arduino.h>

#define WIFI_SSID "..."
#define WIFI_PSK "..."

#define HTTPONLY 0
#define MAX_CONNECTIONS 2
#define WORKERS 2
#define TLSTICKETS 0

// Include certificate data (see note above)
#if (HTTPONLY!=1)
#include "cert.h"
#include "private_key.h"
#endif

// Binary data for the favicon
#include "favicon.h"

// We will use wifi
#include <WiFi.h>

#include <SPIFFS.h>

// Includes for the server
#if (HTTPONLY!=1)
#include <HTTPSServer.hpp>
#include <SSLCert.hpp>
#else
#include <HTTPServer.hpp>
#endif
#include <HTTPRequest.hpp>
#include <HTTPResponse.hpp>

// The HTTPS Server comes in a separate namespace. For easier use, include it here.
using namespace httpsserver;

// Create an SSL certificate object from the files included above
#if (HTTPONLY!=1)
SSLCert cert = SSLCert(
  example_crt_DER, example_crt_DER_len,
  example_key_DER, example_key_DER_len
);

// Create an SSL-enabled server that uses the certificate
// The contstructor takes some more parameters, but we go for default values here.
HTTPSServer server = HTTPSServer(&cert, 443, MAX_CONNECTIONS);
#else
HTTPServer server = HTTPServer(80, MAX_CONNECTIONS);
#endif

// Declare some handler functions for the various URLs on the server
// The signature is always the same for those functions. They get two parameters,
// which are pointers to the request data (read request body, headers, ...) and
// to the response data (write response, set status code, ...)
void handleRoot(HTTPRequest * req, HTTPResponse * res);
void handleCat(HTTPRequest * req, HTTPResponse * res);
void handleSmall(HTTPRequest * req, HTTPResponse * res);
void handleFavicon(HTTPRequest * req, HTTPResponse * res);
void handle404(HTTPRequest * req, HTTPResponse * res);

unsigned long lastReconnect = 0;

void setup() {
  // For logging
  Serial.begin(115200);

  if(!SPIFFS.begin()) {
    while(true) {
      Serial.println("SPIFFS failed.");
      delay(5000);
    }
  }

  // Connect to WiFi
  Serial.println("Setting up WiFi");
  WiFi.begin(WIFI_SSID, WIFI_PSK);
  while (WiFi.status() != WL_CONNECTED) {
    if (millis()-lastReconnect > 10000) {
      lastReconnect = millis();
      WiFi.disconnect();
      delay(500);
      Serial.print(":");
      WiFi.begin(WIFI_SSID, WIFI_PSK);
    } else {
      delay(500);
      Serial.print(".");
    }
  }
  Serial.print("Connected. IP=");
  Serial.println(WiFi.localIP());

  // For every resource available on the server, we need to create a ResourceNode
  // The ResourceNode links URL and HTTP method to a handler function
  ResourceNode * nodeRoot    = new ResourceNode("/", "GET", &handleRoot);
  ResourceNode * nodeCat     = new ResourceNode("/cats/*", "GET", &handleCat);
  ResourceNode * nodeSmall   = new ResourceNode("/small/*", "GET", &handleSmall);
  ResourceNode * nodeFavicon = new ResourceNode("/favicon.ico", "GET", &handleFavicon);
  ResourceNode * node404     = new ResourceNode("", "GET", &handle404);

  // Add the root node to the server
  server.registerNode(nodeRoot);
  server.registerNode(nodeCat);
  server.registerNode(nodeSmall);

  // Add the favicon
  server.registerNode(nodeFavicon);

  // Add the 404 not found node to the server.
  // The path is ignored for the default node.
  server.setDefaultNode(node404);

#if TLSTICKETS==1 && HTTPONLY==0
  server.enableTLSTickets();
#endif

#if WORKERS>0
  server.enableWorkers(WORKERS);
#endif

  Serial.println("Starting server...");
  server.start();
  if (server.isRunning()) {
    Serial.println("Server ready.");
  }
}

void loop() {
  // This call will let the server do its work
#if WORKERS==0
  server.loop();
#else
  delay(1000);
#endif

  if (WiFi.status() != WL_CONNECTED) {
    if (millis()-lastReconnect > 10000) {
      Serial.println("Connection lost, reconnecting...");
      WiFi.disconnect();
      delay(500);
      WiFi.begin(WIFI_SSID, WIFI_PSK);
      lastReconnect = millis();
    }
  }
}

void handleRoot(HTTPRequest * req, HTTPResponse * res) {
  // Status code is 200 OK by default.
  // We want to deliver a simple HTML page, so we send a corresponding content type:
  res->setHeader("Content-Type", "text/html");

  // The response implements the Print interface, so you can use it just like
  // you would write to Serial etc.
  res->println("<!DOCTYPE html>");
  res->println("<html>");
  res->println("<head><title>Hello World!</title></head>");
  res->println("<body>");
  res->println("<h1>Hello World!</h1>");
  res->print("<p>Your server is running for ");
  // A bit of dynamic data: Show the uptime
  res->print((int)(millis()/1000), DEC);
  res->println(" seconds.</p>");
  res->println("</body>");
  res->println("</html>");
}

void handleSmall(HTTPRequest * req, HTTPResponse * res) {
  /* The files in that directory are all 128 byte sized text files */
  std::string smallfile = "/smallfiles/" + req->getParams()->getUrlParameter(0);
  if (SPIFFS.exists(smallfile.c_str())) {
    File f = SPIFFS.open(smallfile.c_str(), "r");
    if (f) {
      res->setHeader("Content-Length", intToString(f.size()));
      res->setHeader("Content-Type", "text/plain");
      uint8_t buffer[512];
      size_t buflen;
      size_t offset = 0;
      while((buflen = f.read(buffer, sizeof(buffer)))>0) {
        offset = 0;
        while (buflen > offset) {
          offset += res->write(buffer+offset, buflen-offset);
        }
      }
      f.close();
    } else {
      res->setStatusCode(500);
      res->setStatusText("Internal server error");
      res->println("Error reading a small file.");
    }
  } else {
    res->setStatusCode(404);
    res->setStatusText("Not found");
    res->println("This file does not exist on this server.");
  }
}

void handleCat(HTTPRequest * req, HTTPResponse * res) {
  std::string catfile = "/cats/" + req->getParams()->getUrlParameter(0);
  if (SPIFFS.exists(catfile.c_str())) {
    File f = SPIFFS.open(catfile.c_str(), "r");
    if (f) {
      res->setHeader("Content-Length", intToString(f.size()));
      res->setHeader("Content-Type", "image/jpeg");
      uint8_t buffer[512];
      size_t buflen;
      size_t offset = 0;
      while((buflen = f.read(buffer, sizeof(buffer)))>0) {
        offset = 0;
        while (buflen > offset) {
          offset += res->write(buffer+offset, buflen-offset);
        }
      }
      f.close();
    } else {
      res->setStatusCode(500);
      res->setStatusText("Internal server error");
      res->println("Cat is currently catting around.");
    }
  } else {
    res->setStatusCode(404);
    res->setStatusText("Not found");
    res->println("This cat does not exist on this server.");
  }
}

void handleFavicon(HTTPRequest * req, HTTPResponse * res) {
	// Set Content-Type
	res->setHeader("Content-Type", "image/vnd.microsoft.icon");
	// Write data from header file
	res->write(FAVICON_DATA, FAVICON_LENGTH);
}

void handle404(HTTPRequest * req, HTTPResponse * res) {
  // Discard request body, if we received any
  // We do this, as this is the default node and may also server POST/PUT requests
  req->discardRequestBody();

  // Set the response status
  res->setStatusCode(404);
  res->setStatusText("Not Found");

  // Set content type of the response
  res->setHeader("Content-Type", "text/html");

  // Write a tiny HTTP page
  res->println("<!DOCTYPE html>");
  res->println("<html>");
  res->println("<head><title>Not Found</title></head>");
  res->println("<body><h1>404 Not Found</h1><p>The requested resource was not found on this server.</p></body>");
  res->println("</html>");
}

This sketch runs without problems on the current master, for arbitrary numbers of concurrent connections (as long as it doesn't hit the RAM boundary). But for more connections, it seems to get stuck before sending out packets (e.g. the server log says the connection has been finished, but the response in Wireshark arrives 7 seconds later, it doesn't accept connections directly) etc. And without enabling workers (which would be the backward-compatibility scenario), some resources aren't delivered even for a single client with a single connection. That would be a problem for everyone updating the library.

Do you want to do further debugging on this or should I try to fix these issues and add to this PR?