Handle the possible different exceptions when an application tries to establish a session but an error is thrown

[ivanhercaz]

EstablishSessionStatus.keymismatch is always returned when the session can't be established, even if it isn't because a wrong proof of possession. It is reported in #11 and this PR tries to handle this situation.

• a979596 is just a proposal of the idea, not the definitive way to handle the errors.
• The ideal would be to have some standardized response to catch the exceptions, but it must be confirmed reviewing the response using one or another bluetooth packages in the TransportBle.

Tasks

• Detect the possible errors when the device is connected but throws errors that mustn't match with the keymismatch.
• Confirm the possible errors with more than one package (e.g., flutter_ble_lib_ios_15 and flutter_blue_plus).
• Agree the most descriptive values for the errors.

Errors detected

All this errors currently returns keymismatch, the first one is the correct one, the rest of them must have their specific value.

Incorrect proof of possession

Tested with flutter_blue_plus, not tested with the example of this package.

I/flutter (17973): │ ⚠️ [ESP PROV] => type 'Null' is not a subtype of type 'FutureOr<Uint8List>'

Must returns the EstablishSessionStatus.keymismatch.

With correct proof of possession

Tested with flutter_blue_plus, not tested with the example of this package.

I/flutter (17509): │ ⚠️ [ESP PROV] => Exception: Unexpected state

What must be returned? An EstablishSessionStatus.unexpectederror as a generic error without more information?

Wrong MTU/buffer length

I/flutter (21478): │ ⚠️ [ESP PROV] => Exception: Empty response

This exception also returns a KeyMistmatch. It happens when MTU is increased over 512. I am using flutter_blue_plus and 512 is the default MTU in the connect() method, although I think it isn't a specific issue of that package and it may happen with any other Bluetooth plugin.

It must be researched more. It may return something like EstablishSessionStatus.wrongbufferlength or EstablishSessionStatus.mtuerror.

Alternative idea

Thinking about the complexity of catching this errors, as mentioned in the second point of the introduction of this PR, an alternative may be to return a generic error that the user must catch according to the bluetooth package used in the TransportBle.

[ivanhercaz]

I was thinking about the alternative idea I commented in the main message of the PR. In the case we check that there are different exceptions, as I think it is, depending of the bluetooth package used, what if we implement a necessary catch function that must throw an EstablishSessionStatus?

In transport_ble.dart we could add a method like the next one:

Future<EstablishSessionStatus> catchConnectedSessionStatusError(Exception error, StackTrace stackTrace)

So instead of

esp_provisioning_ble/lib/src/esp_prov.dart

Lines 42 to 45 in e0948a5

	} catch (e) {
	if (await transport.checkConnect()) {
	return EstablishSessionStatus.keymismatch;
	} else {

We could return something like:

 } catch (error, stacktrace) { 
   if (await transport.checkConnect()) { 
     return transport.catchConnectedSessionStatusError(error, stacktrace); 
   } else { 

Then the developer who implements the package have the responsibility to adapt it to the current behavior of its package and, in case it has specific exceptions for that, it can also be implemented. The minimum necessary implementation could be something simple as:

Future<EstablishSessionStatus> catchConnectedSessionStatusError(Exception error, StackTrace stackTrace) => error.contains('Null') ? EstablishSessionStatus.keymismatch : EstablishSessionStatus.unexpectederror

---

It is just an idea, but while I was writing I was losing a bit of confidence on it, because of how are currently handled the exceptions in the try block and because I think I mix some facts and data, because the origin and what are the messages of the exceptions can be are a bit confusing. Let's see in another comment to not mix.

[ivanhercaz]

We could also simplify and adjust to the current flow of the try loop:

esp_provisioning_ble/lib/src/esp_prov.dart

Lines 27 to 41 in e0948a5

	try {
	SessionData responseData = SessionData();
	while (await transport.checkConnect()) {
	var request = await security.securitySession(responseData);
	if (request == null) {
	return EstablishSessionStatus.connected;
	}
	var response = await transport.sendReceive(
	'prov-session', request.writeToBuffer());
	if (response.isEmpty) {
	throw Exception('Empty response');
	}
	responseData = SessionData.fromBuffer(response);
	}
	return EstablishSessionStatus.disconnected;

And incorrect proof of possession can be detected by checking if the response is Null so:

     if (response.isEmpty) { 
       // Here it could return another exception like `EstablishSessionStatus.wrongbufferlength` if we achieve it is the only reason for this condition
       throw Exception('Empty response'); 
     } else if (response == Null) { 
       return EstablishSessionStatus.keymismatch; 
     }

And finally in the catch:

      if (await transport.checkConnect()) {
        return EstablishSessionStatus.unexpectedstate;
      } else {

[ogabrielinacio]

@ivanhercaz can you verify to me if the package flutter_blue_plus give us some error code like

"// I/flutter (21772): BleError (Error code: 401, ATT error code: 4, iOS error code: null, Android error code: null, reason: GATT exception from MAC='XX:XX:XX:XX:XX:XX',
//status 4 (GATT_INVALID_PDU), type BleGattOperation{description='CHARACTERISTIC_WRITE'}.
//(Look up status 0x04 here https://cs.android.com/android/platform/superproject/+/master:packages/modules/Bluetooth/system/stack/include/gatt_api.h),
//internal message: null, device ID: B0:A7:32:D8:8D:86, service UUID: 021a9004-0382-4aea-bff4-6b3f1c5adfb4, characteristic UUID: 021aff51-0382-4aea-bff4-6b3f1c5adfb4, descriptor UUID: null)"

Or some similar to this error message?

the error code 401 indicates an authentication problem, so we can give the error to the developer based on this error code

So we can do somethin like that :
`
final regex = RegExp(r"Error code:\s*(\d+)");
final match = regex.firstMatch(errorMessage);
int? errorCode;
if (match != null) {
errorCode = int.parse(match.group(1)!);
debugPrint("Error Code: $errorCode");
} else {
debugPrint("Error Code not found in the string");
}

    debugPrint("______________________");
    debugPrint(errorMessage);
    debugPrint("______________________");

    if (errorCode == 401) {
      return EstablishSessionStatus.keymismatch;
    } else {
      return EstablishSessionStatus.unknownerror;
    }` 

we can improve this code later, this is just a test code

we have to find something in common that the Bluetooth packets will return, so we can standardize it for the developer

@ivanhercaz I made a request on LinkedIn too, there we can exchange contacts and communicate better to talk about the future of the package

[ivanhercaz]

@ivanhercaz I made a request on LinkedIn too, there we can exchange contacts and communicate better to talk about the future of the package

Hi @ogabrielinacio! Excuse me, I have been very very busy and now I am bit ill with cold hehe. I don't usually use LinkedIn, I use more Telegram (same as here, like ivanhercaz), I also have Signal too in case you prefer, but if you want, you can send me an email to [email protected] and I will share with you my user.

I will try to take a look to everything as soon as possible but I can't promise a date 😿

[ivanhercaz]

@ivanhercaz can you verify to me if the package flutter_blue_plus give us some error code like

"// I/flutter (21772): BleError (Error code: 401, ATT error code: 4, iOS error code: null, Android error code: null, reason: GATT exception from MAC='XX:XX:XX:XX:XX:XX', //status 4 (GATT_INVALID_PDU), type BleGattOperation{description='CHARACTERISTIC_WRITE'}. //(Look up status 0x04 here https://cs.android.com/android/platform/superproject/+/master:packages/modules/Bluetooth/system/stack/include/gatt_api.h), //internal message: null, device ID: B0:A7:32:D8:8D:86, service UUID: 021a9004-0382-4aea-bff4-6b3f1c5adfb4, characteristic UUID: 021aff51-0382-4aea-bff4-6b3f1c5adfb4, descriptor UUID: null)"

Or some similar to this error message?

the error code 401 indicates an authentication problem, so we can give the error to the developer based on this error code

Yes! Flutter Blue Plus (FBP) and any stable bluetooth package must implement at least the handling of the existent and possible GATT errors. FBP has a list of the possible errors on Android and iOS and descriptions for each one, take a look.

Just a note, error code 4 or 40* are not related with authentication errors (more about it below).

So we can do somethin like that :

final regex = RegExp(r"Error code:\s*(\d+)");
final match = regex.firstMatch(errorMessage);
int? errorCode;

if (match != null) {
  errorCode = int.parse(match.group(1)!);
  debugPrint("Error Code: $errorCode");
} else {
  debugPrint("Error Code not found in the string");
}
debugPrint("______________________");
debugPrint(errorMessage);
debugPrint("______________________");

if (errorCode == 401) {
  return EstablishSessionStatus.keymismatch;
} else {
  return EstablishSessionStatus.unknownerror;
}

Oh! I see, you catch the first number then return it. The problem I see with that is that the package would suppose that the first number is the error code, but we still have to adapt it to different cases.

For example, in the case of FlutterBleLib for iOS 15 the exception returns a BleError class that has the next information (just write here the related to our case):

• errorCode.
• attErrorCode.

From both, attErrorCode points to the GATT Error, the errorCode points to another error, that what have in common is that the first digit matches with the attErrorCode. In the case example you give above:

• errorCode = 401, so it is a BleErrorCode.characteristicWriteFailed.
• attErrorCode = 4, so it is a GATT_INVALID_PDU, which usually means the PDU is wrong. Why? I don't know because I haven't deep in that error more than I have need, because it seems a hard error, but the most probably cause I have found it is related with the MTU and a characteristic write error as the errorCode points. Take a look to Is EstablishSessionStatus.KeyMismatch being returned not only when an incorrect proof of possession is used? #11.

In the case of FBP we have FlutterBluePlusException, which returns the next information (as above, just the related with this issue):

• code.

This code is the same as the attErrorCode of FlutterBleLib for iOS 15, so it points to the GATT error. FBP seems more abstract catching this errors and just notice you about the GATT error and then give you context information:

• function, the function in which the error was catched.
• description, the mentioned GATT error descriptions I shared above.

In summary FBP returns a string with the next format 'FlutterBluePlusException | $function | $sPlatform-code: $code | $description'.

So if we apply the regexp we end with a 401 in one case and 4 in another.

we have to find something in common that the Bluetooth packets will return, so we can standardize it for the developer

Totally agree about that...

What about just returning the exception in case it isn't one of esp_provisioning_ble? I mean:

• Is it an EstablishSession.keymismatch, nice! Return it.
• Isn't it an ``EstablishSession.keymismatch`, return the exception given by the bluetooth package.

It may be not the best option, but what I am thinking it is that we are trying to match possible errors that are handle in a different way by each bluetooth package, so maybe, as esp_provisioning_ble doesn't impose the use of a specific bluetooth package, the responsibility of handle these kind of errors can be the user's responsibility. And I think that part it is important because the error itself it isn't part of the ESP provisioning, it is part of an incorrect implementation of the TransportBle.

What do you think about it?

Of course, we could write a section or even a guide with some notes about possible errors and how to handle them in known bluetooth packages.

[ivanhercaz]

What about just returning the exception in case it isn't one of esp_provisioning_ble? I mean:

* Is it an `EstablishSession.keymismatch`, nice! Return it.

* Isn't it an `EstablishSession.keymismatch`, return the exception given by the bluetooth package.

It may be not the best option, but what I am thinking it is that we are trying to match possible errors that are handle in a different way by each bluetooth package, so maybe, as esp_provisioning_ble doesn't impose the use of a specific bluetooth package, the responsibility of handle these kind of errors can be the user's responsibility. And I think that part it is important because the error itself it isn't part of the ESP provisioning, it is part of an incorrect implementation of the TransportBle.

I have though more about it: the unique related errors with the EstablishSession are the one that we already have in the Enum: keymistmatch or disconnected, taking into account that EstablishSession is not an Enum error-only related, it is just for notice the state in which it is the session with the device, no?

If there is another error that it isn't related directly with the EstablishSession state, instead of add a lot of possible errors (e.g., mtuerror, wrongBufferLength, etc.), that could make hard to main to adapt them to many possibilities, we could just add one value: EstablishSession.unexpectecError, and before this value is returned we can log the error given by the bluetooth package used.

In that case we would have that a specific EstablishSession value that warns the user there is an error that it isn't handled by the esp_provisiong_ble package. This approach doesn't go far from the current behavior, just fixes the problem of "all possible errors on EstablishSession is keymismatch or disconnected".

The main problem I see here is that if we log the exception message that was thrown, but we also wants to allow the users to handle by themselves the error in their implementation we must return the value and the error thrown, maybe something like:

# A set
{EstablishSession.unexpectedError, "exception_error_catched"}

# Or a map
{EstablishSession.unexpectedError: "exception_error_catched"}

But that will require to change the signature of the establishSession() method, so we must think about it. We must be sure because I think it could be a breaking change that will require a major version if we introduce it.

[ivanhercaz]

Okay... Now I see by you said the 401 error was an authentication error:

I/flutter (12933): EstablishSession Error:
I/flutter (12933): BleError (Error code: 401, ATT error code: 4, iOS error code: null, Android error code: null, reason: GATT exception from MAC='XX:XX:XX:XX:XX:XX', status 4 (GATT_INVALID_PDU), type BleGattOperation{description='CHARACTERISTIC_WRITE'}. (Look up status 0x04 here https://cs.android.com/android/platform/superproject/+/master:packages/modules/Bluetooth/system/stack/include/gatt_api.h), internal message: null, device ID: 40:4C:CA:41:36:3A, service UUID: 021a9004-0382-4aea-bff4-6b3f1c5adfb4, characteristic UUID: 021aff51-0382-4aea-bff4-6b3f1c5adfb4, descriptor UUID: null)

This happens when a wrong POP is introduced... It is a characteristic failure but as we know there is a wrong POP, we associate it with an authentication error. In this case we can wait and deep more in how we manage the session establishment or we can return a {EstablishSession.keymismatch, "exception_error_catched"} and just the let the user to check it if it is necessary (of course, explaining the reason in the documentation).

It has been really hard to find the correct way to handle this situation.

[ivanhercaz]

I am reviewing it again, but checking the ESP32 protocomm.

establishSession() internally builds a SessionData from the received buffer:

esp_provisioning_ble/lib/src/esp_prov.dart

Line 39 in 58a268f

responseData = SessionData.fromBuffer(response);

This SessionData corresponsd with the session.proto:

esp_provisioning_ble/lib/src/protos/session.proto

Lines 15 to 21 in 58a268f

	message SessionData {
	SecSchemeVersion sec_ver = 2; /*!< Type of security */
	oneof proto {
	Sec0Payload sec0 = 10; /*!< Payload data in case of security 0 */
	Sec1Payload sec1 = 11; /*!< Payload data in case of security 1 */
	}
	}

And, for example, in the sec1.proto we can see has a Status field:

esp_provisioning_ble/lib/src/protos/sec1.proto

Lines 11 to 14 in 58a268f

	message SessionResp1 {
	Status status = 1;
	bytes device_verify_data = 3;
	}

esp_provisioning_ble/lib/src/protos/sec1.proto

Lines 22 to 26 in 58a268f

	message SessionResp0 {
	Status status = 1;
	bytes device_pubkey = 2;
	bytes device_random = 3;
	}

And this Status corresponds with the one defined in constants.proto,

esp_provisioning_ble/lib/src/protos/constants.proto

Lines 5 to 14 in 58a268f

	enum Status {
	Success = 0;
	InvalidSecScheme = 1;
	InvalidProto = 2;
	TooManySessions = 3;
	InvalidArgument = 4;
	InternalError = 5;
	CryptoError = 6;
	InvalidSession = 7;
	}

And many of its values, if not all, seems related to possible responses of a ESP32 when trying to establish the session.

Then, if we check the generated Dart files from the protocomm (that's really awesome and useful), we can see that SessionData has a payload depending of the security level:

esp_provisioning_ble/lib/src/protos/generated/session.pb.dart

Lines 40 to 53 in 58a268f

	static final $pb.BuilderInfo _i = $pb.BuilderInfo(
	_omitMessageNames ? '' : 'SessionData',
	createEmptyInstance: create)
	..oo(0, [10, 11])
	..e<SecSchemeVersion>(
	2, _omitFieldNames ? '' : 'secVer', $pb.PbFieldType.OE,
	defaultOrMaker: SecSchemeVersion.SecScheme0,
	valueOf: SecSchemeVersion.valueOf,
	enumValues: SecSchemeVersion.values)
	..aOM<$1.Sec0Payload>(10, _omitFieldNames ? '' : 'sec0',
	subBuilder: $1.Sec0Payload.create)
	..aOM<$2.Sec1Payload>(11, _omitFieldNames ? '' : 'sec1',
	subBuilder: $2.Sec1Payload.create)
	..hasRequiredFields = false;

If we look for Sec1Payload, in sec1.pb.dart, there are different classes to handle the response. If we check, for example, SessionResp1:

https://github.com/ogabrielinacio/esp_provisioning_ble/blob/58a268fd8401ab2cf5d4f81a28ef6a5d681ae931/lib/src/protos/generated/sec1.pb.dart#L83C16-L92

It is possible to see there is a status key we could use to handle this, as well as the package is already using some of them like secVer in security1.dart file!

esp_provisioning_ble/lib/src/security1.dart

Lines 97 to 101 in 58a268f

	Future<SessionData> setup0Response(SessionData responseData) async {
	SessionData setupResp = responseData;
	if (setupResp.secVer != SecSchemeVersion.SecScheme1) {
	throw Exception('Invalid sec scheme');
	}

And, after all this research and checking better how works a session, how it is defined and more, I think the approach of this PR may be... completely wrong. Because, after all of this, I think we should first handle all this possible states during and after establishing a session with the ESP32.

It doesn't seem something hard to implement but it will require time and, probably, even more time for testing all the possibilities.

What do you think, @ogabrielinacio? I think we should rethink a bit the EstablishSession enum.