Hono: MQTT/AMQP adapter: Deleted/disabled devices should not be able to connect

[kaniyan]

A device has been registered and credentials for that device has also been added. Then the device is deleted but not the corresponding credentials. In this case, the deleted device is still able to subscribe to control endpoint, receive commands and send command responses.

[sophokles73]

If a device is deleted (I assume you are referring to the process of removing its registration information from the Device Registration service) then the device should no longer be able to connect to the MQTT adapter at all, and thus not be able to subscribe to the command topic.

I suspect that the behavior you experience is merely because the registration information (the assertion, to be precise) is cached in the protocol adapter for a configurable amount of time. We (currently) do not invalidate the cache if the underlying information is removed from the registry. Can you validate this by e.g. restarting the MQTT adapter after deleting the registration information and see if the device can still connect? You should also be able to simply wait for the cache to expire (5 minutes?) instead of restarting the adapter.

[kaniyan]

If a device is deleted (I assume you are referring to the process of removing its registration information from the Device Registration service) then the device should no longer be able to connect to the MQTT adapter at all, and thus not be able to subscribe to the command topic.

I suspect that the behavior you experience is merely because the registration information (the assertion, to be precise) is cached in the protocol adapter for a configurable amount of time. We (currently) do not invalidate the cache if the underlying information is removed from the registry. Can you validate this by e.g. restarting the MQTT adapter after deleting the registration information and see if the device can still connect? You should also be able to simply wait for the cache to expire (5 minutes?) instead of restarting the adapter.

I tried by restarting the MQTT adapter and I am still able to connect to the Mqtt adapter, subscribe to the control endpoint, receive commands and send responses.

[kaniyan]

In Mqtt adapter, currently device registration assertion getRegistrationAssertion() is not being done during subscription and uploading command response. FMPOV this needs to be done.

[sophokles73]

No, it should be done as part of establishing the connection. If a device is disabled or doesn't even exist, then it should not be able to connect to the MQTT adapter. If we make sure that this is the case then I do not see why we should need to do the check on every interaction ...

[kaniyan]

Currently for telemetry and event messages, this check is being done while receiving those messages. I agree that it should be done as part of establishing the connection.

[sophokles73]

Would you like to create a PR for fixing this?

[kaniyan]

sure.

[kaniyan]

As per this solution, we check the device registration at the time of the connection establishment. Assume a device, which is connected to the Mqtt adapter, is deleted. In this case, the deleted device,
that remains connected, can still perform telemetry, event and command operations until the Mqtt adapter is restarted or the device reconnects. I think that this scenario (Handling deletion of connected devices) is also applicable to other adapters.

[sysexcontrol]

I had similar thoughts already and kind of agree to what @kaniyan outlines here.
How about a compromise solution :

• check during connection establishment
• check again for any downstream or upstream operations every "once in a while", e.g. all 10 or 30 minutes

Would require an associated timestamp or something similar to be stored per connection and evaluated when handling such an operation.

Makes things a little more complicated, but for security reasons we do not have a choice, do we?
Another choice would be to broadcast an event if a device was deleted or disabled (this case was not discussed so far IMHO) - but this is making things even more complicated.

[sophokles73]

Makes things a little more complicated, but for security reasons we do not have a choice, do we?
Another choice would be to broadcast an event if a device was deleted or disabled (this case was not discussed so far IMHO) - but this is making things even more complicated.

This, indeed would make things more complicated. BTW we have discussed all of this already a long time ago (in a Galaxy far, far away ...): https://github.com/eclipse/hono/issues/293

So, first and foremost we must make sure that a device cannot connect if it is non-existing/disabled. Can we agree on that? We also need to make sure that we make the same check in other protocol adapters that use a connection oriented protocol (i.e. the AMQP adapter). IMHO this should be the scope of the (first) PR. We can then discuss, if and how we want to add more checks as part of subscription handling and/or upload of data, ok?

[sysexcontrol]

So, first and foremost we must make sure that a device cannot connect if it is non-existing/disabled. Can we agree on that? We also need to make sure that we make the same check in other protocol adapters that use a connection oriented protocol (i.e. the AMQP adapter). IMHO this should be the scope of the (first) PR.

Fully agree to that :+1:

We can then discuss, if and how we want to add more checks as part of subscription handling and/or upload of data, ok?

Reasonable plan IMHO.

[sophokles73]

@kaniyan I think we have addressed and resolved the most imminent issue, i.e. rejecting connect attempts from disabled/deleted devices. The other problems discussed in this issue are still valid. However, for reasons of clarity, I would prefer to have separate/new issues being created for them, e.g. preventing disabled/deleted (but already connected) devices from subscribing to commands and actively disconnecting devices that have been disabled/deleted (see #293). Can you please create those issues and then close this issue, if you agree that we have addressed the underlying problem?

[kaniyan]

@sophokles73 :+1: I will create new issues for other points and close this issue.