Repository navigation
csapi-mqtt, mqtt-hivemq: wildcard subscriptions on event topics - #267
tipatterson-dev wants to merge 7 commits into
Conversation
c31a4c9 to
b7c37b2
Compare
0d2c2ee to
b52954a
Compare
b52954a to
9cb5bba
Compare
| // SHA-256 of earlier default configs, without whitespace. They disable wildcards. | ||
| static final Set<String> OLD_DEFAULT_CONFIGS = Set.of( | ||
| "4508192cfda8841d6193ae7c30343c1ebfef1919dfc0b172ab839c9c9fddb157", | ||
| "42f5b9f635f4b759de3eae362f8735b7bd0abe872318980ee6ba309057e86b3a"); |
There was a problem hiding this comment.
Why do we need to check old default configs SHAs? Can we just turn wildcards off if they are disabled in config or below xml is completely missing from config file?
| configFolder.mkdirs(); | ||
|
|
||
| // if no config was provided, copy default config | ||
| // an unchanged earlier default config is replaced, to enable wildcards |
There was a problem hiding this comment.
Oh I see why now. I don't think we should enable wildcards automatically if an older default config is used. It can be confusing. Did you have a use case in mind?
There was a problem hiding this comment.
No, honestly, this was a missed Claude-ism
| static final long ACCESS_CACHE_MS = 10_000; | ||
| static final int ACCESS_CACHE_MAX = 10_000; | ||
| final Map<String, CachedAccess> accessCache = new ConcurrentHashMap<>(); | ||
| LongSupplier clockMs = System::currentTimeMillis; |
There was a problem hiding this comment.
We should use a Guava cache instead of reinventing the wheel here. It would greatly simplify the code. Guava caches can be configure to expire entries after a certain time of when a certain number of entries is reached.
There was a problem hiding this comment.
I am not familiar with those or really Guava in general. I will look into it and switch over once I do some learning.
| } | ||
| catch (SecurityException e) | ||
| { | ||
| return Access.DENIED; |
There was a problem hiding this comment.
We could avoid throwing exceptions here by doing our own check with security.hasPermission() like in doCheckDeleted()
There was a problem hiding this comment.
And if we do that, I don't think we need a separate a doCheckDeleted method actually.
9cb5bba to
3edd922
Compare
|
Rebased on master and addressed the review:
|
Accepts MQTT wildcard topic filters (
+,#) on Resource Event and Batch Resource Event topics (/req/mqtt/subscriptions). A filter covers every authorized matching topic, also for resources created later, and users get only the events of resources they can read through the CS API.Closes #255. Closes #269. Part of #249.
Stacked on #266 (Batch Resource Events, #258), which is merged. The base is
master. Also depends on #262 (#257), which is merged into this branch: the delivery check below trusts the list of accepted exact subscriptions, and #262 stops a rejected subscription from being added to that list. #262, #276, #261, #263, #264 and #266 are merged.This PR also contains the fix for #269 (subscriptions after a handler restart). The fix was in #277, which is closed: the code here is the same, and the wildcard check for restored subscriptions needs it. See "Fix for #269" below.
Module split (#274)
The Part 3 binding is in the new module
sensorhub-service-csapi-mqtt(#274, from #261 up). No PR in the stack changessensorhub-service-consys-mqtt, so nodes that use the legacy topics do not change, in any merge order.Why a check for each message
A wildcard filter can match the topics of resources that the user cannot read. The broker sends each matching message to each matching subscriber, so a check at subscribe time is not sufficient (
/req/mqtt/subscriptions: users SHALL NOT receive resource information through MQTT that they are not authorized to retrieve through HTTP). The server now checks each message before it is sent through a wildcard subscription.Changes
sensorhub-comm-mqtt2.1.0:IMqttHandler.supportsWildcards()andIMqttHandler.canDeliver(userID, topic). Both default to false, so other handlers (for example SensorThings) keep rejecting wildcards.sensorhub-service-mqtt-hivemq0.2.0:OshDeliveryFilter(HiveMQ outbound publish interceptor). A message on a topic that the client subscribed to exactly is sent at once. Any other message matched a wildcard filter, and the handler of its topic decides if it is sent. The check runs outside the HiveMQ threads. On error or timeout the message is not sent.onSubscribefor them.OshExtensionnow keeps the user of each client subscription, andregisterHandlercallsonSubscribefor the existing subscriptions under the prefix of the new handler. IfonSubscribefails (for example, the resource is gone or the user lost the permission), the subscription is removed from the client in HiveMQ.config.xmlandconfig-tls.xmlallow wildcard subscriptions. The MQTT Server does not change a config that is in the config folder. A node from an earlier version keeps its config, which does not allow wildcards, and the MQTT Server logs a warning at start.sensorhub-service-csapi-mqtt0.2.0 (the Part 3 module, see csapi-mqtt: move the Part 3 binding to its own module #274):TopicMapper.parseFilter: checks the MQTT wildcard rules, and accepts a filter only if it can match a Resource Event or Batch Resource Event topic. Filters with:dataare rejected (/req/mqtt/subscriptions: the service description declares restrictions on wildcards for Resource Data topics).canDeliver: Resource Data is never sent through a wildcard subscription. An event is sent only if the user has the permission to read its resource, directly or from a system above it. This is the permission check of a CS API GET by ID (hasPermission), without a read of the resource. For a deleted datastream or control stream, osh-core cannot find the systems above it, so the systems of the topic path are used. Decisions are kept for 10 seconds in a Guava cache (10,000 entries at most).Known limits
exposedResources), a user with the permission still gets its events.{nodeId}/systems/{id}carries top-level systems only. A subscription with the ID of a subsystem gets no events.#also matches the:datatopics that other clients subscribe to. These messages are checked and not sent, which adds load with high-rate data.Testing
TestOshExtension: exact subscriptions bypass the check, wildcard matches ask the handler, no handler or a handler error drops the message, handlers without wildcard support reject filters. For mqtt-hivemq: subscriptions stop getting messages after a handler module restarts #269: a restarted handler gets the existing subscriptions with the correct users, a new SUBSCRIBE to the same topic does not count the client twice, a subscription that the handler refuses is removed from the client, only the topics of the new handler are restored, and a restored wildcard filter is removed when the new handler has no wildcard support.TestMqttServerConfig: the bundled config allows wildcards, a config that disables them is flagged (also with other line endings and indentation), a config without the element is not flagged.TestTopicMapper: accepted filters and the resource to check, invalid filters (syntax,:data, no matching event topic, unsupported types).TestWildcardDelivery: readable and unreadable resources, permission on the resource or on a system above it, deleted resources (systems of the topic path), batch events checked on the datastream, no Resource Data, cache expiry and size limit.TestAsyncApiHandler: wildcard section in the description.node1/systems/+: not authorized; filter under a denied system: not authorized;:datafilter and a filter that matches no event topic: rejectednode1/systems/+: create and delete events of both new systemsnode1/systems/+: create and delete of the allowed system only; nothing for the denied systemnode1/#: batch events of readable datastreams and events of the allowed system; no:datamessages while another client had an exact:datasubscription/api/and Part 3 data, events and wildcards underapi/work, with no cross-talk; with only one module, the other module's topics are rejected; an old config that names the legacy classes starts the legacy moduleStack