Decryption failure not mapped to an appropriate error?

[Jc2k]

Sometimes decryption fails for HA users and homekit tries to append a bool to a bytearray:

Update for switch.p1eu_4 fails
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/homeassistant/helpers/entity.py", line 221, in async_update_ha_state
    await self.async_device_update()
  File "/usr/local/lib/python3.6/site-packages/homeassistant/helpers/entity.py", line 349, in async_device_update
    await self.hass.async_add_executor_job(self.update)
  File "/usr/local/lib/python3.6/concurrent/futures/thread.py", line 56, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/local/lib/python3.6/site-packages/homeassistant/components/homekit_controller/__init__.py", line 197, in update
    data = pairing.list_accessories_and_characteristics()
  File "/config/deps/lib/python3.6/site-packages/homekit/controller.py", line 273, in list_accessories_and_characteristics
    response = self.session.get('/accessories')
  File "/config/deps/lib/python3.6/site-packages/homekit/controller.py", line 573, in get
    return self.sec_http.get(url)
  File "/config/deps/lib/python3.6/site-packages/homekit/http_impl/secure_http.py", line 65, in get
    return self._handle_request(data)
  File "/config/deps/lib/python3.6/site-packages/homekit/http_impl/secure_http.py", line 89, in _handle_request
    return self._read_response()
  File "/config/deps/lib/python3.6/site-packages/homekit/http_impl/secure_http.py", line 154, in _read_response
    response.parse(decrypted)
  File "/config/deps/lib/python3.6/site-packages/homekit/http_impl/response.py", line 38, in parse
    self._raw_response += part
TypeError: can't concat bool to bytearray

[Jc2k]

Looking at the code I can't really see how this could happen:

            decrypted = self.decrypt_block(length, block, tag)
            # TODO how to react to False?
            if tmp is not False:
                response.parse(decrypted)

decrypt_block would have to be returning True o_O??

[jlusiardi]

Ok this sounds interesting...

The TypeError says part must be bool but must not be False because of tmp is not False. So you are right, decrypt_block would have to return True. This would be the case if chacha20_aead_decrypt returns True. I cannot see, where this should come from? It has only to returns in it (one False the other one returning the plain text (which one line above in the assert gets a call with len() and that doesn't work with bool)

Can any of the HA users perhaps add some print/logging to get the values that are actually there? I guess we must be missing some cast/auto conversion issues.

Does this happen with Bluetooth or IP accessories?

[koreth]

      decrypted = self.decrypt_block(length, block, tag)
      # TODO how to react to False?
      if tmp is not False:
          response.parse(decrypted)

Should that be if decrypted is not False:?

[jlusiardi]

ok, now you make me feel blind... I guess this one happened when I refactored the decrypt_block into a function of its own.

@Jc2k could this be the reason for the strange behavior?

@koreth thanks a lot!

[Jc2k]

Oh my word, how did my eyes misparse that bit of code. I had totally read it as if decrypted is not False already. Yes, that explains the strange error message. The current if clause may as well be if True right now, so when decrypt does fail we'll try to concat a bool. Good spot, @koreth!!

But I think we need to do more than that as the proper fix. Whenever we have seen this error in HA it has been "part 2" of the problem after the connection has already timed out or reset and is already non-functional. And the spec says:

Once session security has been established, if the accessory encounters a decryption failure then it must immediately close the connection used for the session.

I think if decryption fails we need to raise an exception, and like with AccessoryDisconnectedError it should close the current session.

[jlusiardi]

So we should basically raise a new exception and close the socket?

[Jc2k]

Yep I think so 👍🏻

[jlusiardi]

I think I did some improvements in #105. Would you mind have a look @Jc2k ?

[jlusiardi]

I think this can be closed