Fix for fake client on first GSM async SSL connection

[nikiromagnoli]

Hello everyone, I'm here to provide a fix (or its inspiration) to you great guys.

## SCENARIO I was happily using MKRGSM library to register on GSM/GPRS and connect to HTTPS host for data transfer, asynchronously. So far so good. I updated MKRGSM and Arduino SAMD library to latest version (respectively 1.4.2 and 1.8.3; unfortunately I can't tell the previously installed version, they should be 1.3.3 and 1.6.21 but I really cannot bet on it), and noticed that first connection to the HTTPS host started to give back a not-connected client despite the "ready()" cheering it was all OK.

## ENVIRONMENT IDE: Visual Studio w/ Visual Micro extension Arduino SAMD library: 1.8.3 MKRGSM library: 1.4.2

## REQUIREMENTS

• GSM/GPRS connection
• HTTPS endpoint
• asynchronous GSM client

## WHAT'S GOING ON I debugged and found the problem was introduced along with the GSMSSLClient being able to import root certificates. In short what happens is that if one tries to connect asynchronously to SSL host, the ready method (only on first connection attempt) returns 1 after it loads the certificates but before it library actually creates a new socket for the connection.

In deep this happens:

int GSMSSLClient::connect(const char* host, uint16_t port)
{
  _certIndex = 0;
  _state = SSL_CLIENT_STATE_LOAD_ROOT_CERT;

  return connectSSL(host, port);
}

Calling GSMSSLClient::connect(...) sets state for class GSMSSLClient and forwards to its connectSSL that in turn forwards to GSMClient::connect(...). Now, in GSMClient::connect(...) is the following code:

int GSMClient::connect()
{
  if (_socket != -1) {
    stop();
  }

  if (_synch) {                                     // <-- look here
    while (ready() == 0);
  } else if (ready() == 0) {
    return 0;
  }

  _state = CLIENT_STATE_CREATE_SOCKET;

  if (_synch) {
    while (ready() == 0) {
      delay(100);
    }

    if (_socket == -1) {
      return 0;
    }
  }

  return 1;
}

The flow closes any previous opened socket and wait for it. Then it moves to set the new state for GSMClient to "create socket". This works only in synchronous mode. If client is declared "asynchronous", the flow quits right away there where is the flag comment (leaving GSMClient::_state equal to 0) because that ready will handle the GSMSSLClient::ready() that imports the certificates so it'll return 0 for sure (as it is working) so quitting the GSMClient::connect(...) method. Then the snippet in GSMSSLClient::ready():

int GSMSSLClient::ready()
{
  if (_rootCertsLoaded) {
    // root certs loaded already, continue to regular GSMClient
    return GSMClient::ready();
  }

[...]

rightly moves the control to GSMClient when certificates are loaded, but at the first connection attempt GSMClient::_state is 0 (as stated above, that is CLIENT_STATE_IDLE), so the code in GSMClient::ready():

int GSMClient::ready()
{
  int ready = MODEM.ready();

  if (ready == 0) {
    return 0;
  }

  switch (_state) {
    case CLIENT_STATE_IDLE:             // <-- match case
    default: {
      break;
    }

[...]

returns and you have a ready() saying "It's all OK" when actually there's no socket ready, and client.connected() returns false indeed. This leads to bugs as users expect "ready" to say "OK" when client is actually OK, when instead it is not.

## THE FIX My first solution to this is simple (opened to improvements!), I'd replace the code in GSMClient::connect():

if (_synch) {
    while (ready() == 0);
  } else if (ready() == 0) {
    return 0;
  }

with

while (ready() == 0);

because I think there's no clue on returning from connect method while connection has not been initiated yet, even in asynchronous mode.

## HOW TO REPRODUCE I'm providing the following self-contained sketch to test this. Just remember to provide PIN for SIM, APN credentials and an HTTPS URL (I use an AWS HTTPS API gateway test endpoint for example). When bug shows you should read "Er, no. Client is not connected actually." . When bug does not show you should read "Client is connected, fine." .

#include <Arduino.h>
#include <MKRGSM.h>

// remember here to provide SIM_PIN, GPRS_APN, GPRS_USERNAME, GPRS_PASSWORD as needed
#include "arduino_secrets.h"

GSM gsm;
GPRS gprs;
GSMSSLClient client(false);		// <--  set as "asynchronous"

const char * url = "xxxxxxxxxx.execute-api.yyyyyyyyy.amazonaws.com";

void setup()
{
	// wait for peer
	Serial.begin(9600, SERIAL_8N1);
	while( !Serial );

	Serial.println("OK, let's begin connecting to GSM...");

	// register to GSM network
	int ready = gsm.begin(SIM_PIN, true, false);
	while( (ready = gsm.ready()) == 0 );

	if( ready > 1 )
	{
		Serial.print("Modem begin failed with code ");
		Serial.println(ready);
		while( true );
	}
	
	Serial.println("GSM OK. Now attach to GPRS...");

	// attach to GPRS profile
	ready = gprs.attachGPRS(GPRS_APN, GPRS_USERNAME, GPRS_PASSWORD, false);
	while( (ready = gprs.ready()) == 0 );

	if( ready > 1 )
	{
		Serial.print("Attach to GPRS failed with code ");
		Serial.println(ready);
		while( true );
	}

	Serial.println("Good. Now let's connect to SSL enabled host...");

	// connect to SSL host
	ready = client.connect(url, 443);
	while( (ready = client.ready()) == 0 );

	if( ready > 1 )
	{
		Serial.print("Connection to host failed with code ");
		Serial.println(ready);
		while( true );
	}
	else
	{
		// now client should be connected: let's check it out
		if( client.connected() )
		{
			Serial.println("Client is connected, fine.");
			client.stop();
		}
		else
		{
			Serial.println("Er, no. Client is not connected actually.");
		}
	}

	gprs.detachGPRS();
	gsm.secureShutdown();
}

void loop()
{
}

Essay is over. HTH. Thank you.

[nikiromagnoli]

Using this fix I managed to connect by MQTTs to AWS IoTCore broker (which requires certificate authentication). Did someone reproduce the bug?

[CLAassistant]

All committers have signed the CLA.