Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MQTT topics with path parameters required to match guarded identity #1387

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

epieffe
Copy link

@epieffe epieffe commented Jan 29, 2025

The goal of this PR is to allow secure access to some MQTT client specific topics for publish and/or subscribe.

For publish or subscribe routes:

  • mqtt server can define path parameters for any segment in a topic name
  • mqtt server can enforce that a path parameter matches the client's guarded identity (e.g. via jwt guard)

Example configuration:

mqtt_server0:
  type: mqtt
  kind: server
  routes:
    - when:
        - publish:
          - topic: taxi/{id}/location
            params:
              id: ${guarded['jwt'].identity}
        - subscribe:
          - topic: taxi/{id}/update
            params:
              id: ${guarded['jwt'].identity}
      exit: mqtt_kafka_proxy0

Fixes #1382

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We often test via spec scripts to verify implementation behavior instead of tightly coupled unit tests.
The intent is to be able to refactor the code, but still have stable tests to verify new implementation. Tightly coupled tests are typically forced to change when the code is refactored, preventing the stable basis to verify consistency.

Let's discuss further over Slack community as needed on this feedback.

@@ -39,7 +39,7 @@ public MqttRouteConfig(
this.id = route.id;
this.when = route.when.stream()
.map(MqttConditionConfig.class::cast)
.map(MqttConditionMatcher::new)
.map(conf -> new MqttConditionMatcher(conf, route.guarded))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.map(conf -> new MqttConditionMatcher(conf, route.guarded))
.map(c -> new MqttConditionMatcher(this, c))

So you can use route.identity(guard, authorization) from MqttConditionMatcher constructor (see below).

Add Map<String, LongFunction<String>> identities private field and populate in constructor.

Add String identity(String guard, long authorization) to MqttRouteConfig.

String identity(
    String guard,
    long authorization)
{
    return identities.getOrDefault(guard, a -> null).apply(authorization);
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks!

Copy link
Author

@epieffe epieffe Feb 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am reopening this because this caused a bug while I was applying the last suggestions to the PR:

When we pass this to the MqttConditionMatcher constructor, it receives an instance of MqttRouteConfig that is not completely initialized yet. This might produce unexpected behaviour when accessing uninitialized MqttRouteConfig fields inside the MqttConditionMatcher constructor.

In this specific case, the MqttConditionMatcher constructor calls the route.identity() method, but the route.identities field is not yet initialized at this time, and this produces a NullPointerException.

To solve this I moved the when field initialization at the bottom of the MqttRouteConfig constructor, so that all the other fields are initialized when calling the MqttConditionMatcher constructor with this as an input. But this still feels kind of wierd, so maybe it is better to pass the route.identities field to the MqttConditionMatcher constructor, rather than the whole MqttRouteConfig object which is not fully initialized? 🤔

Let me know!

@epieffe epieffe force-pushed the feature/mqtt-topic-param branch 2 times, most recently from 2f9095f to d15893e Compare February 9, 2025 01:13
@epieffe epieffe force-pushed the feature/mqtt-topic-param branch 3 times, most recently from b9738b8 to 084e920 Compare February 23, 2025 01:47
@epieffe epieffe force-pushed the feature/mqtt-topic-param branch from 084e920 to d0c53ea Compare February 23, 2025 01:59
@epieffe epieffe force-pushed the feature/mqtt-topic-param branch from d0c53ea to d526962 Compare February 25, 2025 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support mqtt topics with path parameters required to match guarded identity
2 participants