May be our plugin can be the cause? Anyway I’m still investigating.
Riccardo
Da: Justin Bertram <jbert...@apache.org>
Data: venerdì, 8 aprile 2022 21:58
A: users@activemq.apache.org <users@activemq.apache.org>
Oggetto: Re: Artemis security plugin doesn't allow to change clientId
That's weird. There's a test in the ActiveMQ Artemis test-suite that
exercises this use-case and it's running fine. I'll see if I can set up
Kapua locally and reproduce the failure you're seeing.
Justin
On Fri, Apr 8, 2022 at 10:51 AM Modanese, Riccardo
<riccardo.modan...@eurotech.com.invalid> wrote:
> Hi Justin I got a NPE while broker is handling “normal” stealing link:
>
> 2022-04-08 15:23:37,540 ERROR
> [org.apache.activemq.artemis.core.protocol.mqtt] AMQ834002: Error
> processing control packet:
> MqttConnectMessage[fixedHeader=MqttFixedHeader[messageType=CONNECT,
> isDup=false, qosLevel=AT_MOST_ONCE, isRetain=false, remainingLength=42],
> variableHeader=MqttConnectVariableHeader[name=MQTT, version=4,
> hasUserName=true, hasPassword=true, isWillRetain=false, isWillFlag=false,
> isCleanSession=true, keepAliveTimeSeconds=60],
> payload=MqttConnectPayload[clientIdentifier=clientId, willTopic=null,
> willMessage=null, userName=acme-2, password=[75, 101, 101, 112, 67, 97,
> 108, 109, 49, 50, 51, 46]]]: java.lang.NullPointerException
> at
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTUtil.validateClientId(MQTTUtil.java:524)
> [artemis-mqtt-protocol-2.22.0-SNAPSHOT.jar:2.22.0-SNAPSHOT]
> at
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTProtocolHandler.handleConnect(MQTTProtocolHandler.java:228)
> [artemis-mqtt-protocol-2.22.0-SNAPSHOT.jar:2.22.0-SNAPSHOT]
> at
> org.apache.activemq.artemis.core.protocol.mqtt.MQTTProtocolHandler.act(MQTTProtocolHandler.java:153)
> [artemis-mqtt-protocol-2.22.0-SNAPSHOT.jar:2.22.0-SNAPSHOT]
> at
> org.apache.activemq.artemis.utils.actors.Actor.doTask(Actor.java:33)
> [artemis-commons-2.22.0-SNAPSHOT.jar:]
> at
> org.apache.activemq.artemis.utils.actors.ProcessorBase.executePendingTasks(ProcessorBase.java:65)
> [artemis-commons-2.22.0-SNAPSHOT.jar:]
> at
> java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
> [java.base:]
> at
> java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
> [java.base:]
> at
> org.apache.activemq.artemis.utils.ActiveMQThreadFactory$1.run(ActiveMQThreadFactory.java:118)
> [artemis-commons-2.22.0-SNAPSHOT.jar:]
>
>
> if (connection != null) {
> MQTTSession existingSession =
> session.getProtocolManager().getSessionState(clientId).getSession();
> if (session.getVersion() == MQTTVersion.MQTT_5) {
>
> existingSession.getProtocolHandler().sendDisconnect(MQTTReasonCodes.SESSION_TAKEN_OVER);
> }
> // [MQTT-3.1.4-2] If the client ID represents a client already
> connected to the server then the server MUST disconnect the existing client
> existingSession.getConnectionManager().disconnect(false);//NPE
> }
> so the connection is not null but has no session defined from what I can
> understand.
>
> If could be helpful I created a new branch on my Kapua fork (
> https://github.com/riccardomodanese/kapua/tree/upgrade-artemis-2_21)
> linked to Artemis 2.22.0-SNAPSHOT (I built Artemis locally from main branch
> + cherry-pick your commit)
> The test I run was: RunDeviceBrokerI9nTest (see DeviceBrokerI9n.feature)
>
> Looking forward for your feedback!
>
> Riccardo
>
>
>
>
>
>
> Da: Modanese, Riccardo <riccardo.modan...@eurotech.com>
> Data: venerdì, 8 aprile 2022 08:42
> A: users@activemq.apache.org <users@activemq.apache.org>
> Oggetto: R: Artemis security plugin doesn't allow to change clientId
> Sure, I’ll test asap thanks!
> (I’m currently doing my testing on 2.19, I don’t expect conflicts if I
> cherry-pick the commit)
>
> Da: Justin Bertram <jbert...@apache.org>
> Data: venerdì, 8 aprile 2022 03:05
> A: users@activemq.apache.org <users@activemq.apache.org>
> Oggetto: Re: Artemis security plugin doesn't allow to change clientId
> I just sent a PR [1] for this. Riccardo, could you try this out and see if
> it works for you?
>
>
> Justin
>
> [1] https://github.com/apache/activemq-artemis/pull/4021
>
> On Thu, Apr 7, 2022 at 2:04 PM Justin Bertram <jbert...@apache.org> wrote:
>
> > I went ahead and opened ARTEMIS-3770 [1] for this work.
> >
> >
> > Justin
> >
> > [1] https://issues.apache.org/jira/browse/ARTEMIS-3770
> >
> > On Thu, Apr 7, 2022 at 12:35 PM Justin Bertram <jbert...@apache.org>
> > wrote:
> >
> >> Technically speaking, an implementation of
> >> o.a.a.a.s.c.s.ActiveMQSecurityManager5 like you have *is* allowed to
> change
> >> the client ID on the o.a.a.a.s.c.p.RemotingConnection it receives (just
> as
> >> you are doing in your implementation). The problem is that MQTT
> >> implementation doesn't use this client ID and instead overwrites it with
> >> the value from the MQTT CONNECT packet. However, I think the code could
> be
> >> refactored fairly easily to accommodate this use-case. Can you open a
> Jira?
> >>
> >>
> >> Justin
> >>
> >> On Thu, Apr 7, 2022 at 11:19 AM Modanese, Riccardo
> >> <riccardo.modan...@eurotech.com.invalid> wrote:
> >>
> >>> Hello,
> >>> we are moving a security plugin from ActiveMQ 5.x broker to Artemis
> >>> 2.x.
> >>> To summarize the use case:
> >>> we need to prefix the MQTT client id provided during the connect
> >>> with the account name (something like account_name|client_id) to allow
> >>> devices with the same clientId, but different accounts, to connect to
> the
> >>> broker without triggering the stealing link.
> >>>
> >>> Doing that with the ActiveMQ was possible. With Artemis SecurityPlugin
> >>> any clientId set through the proper RemotingConnection setter has no
> effect
> >>> (
> >>>
> https://github.com/riccardomodanese/kapua/blob/feature-artemisAuthentication/broker/artemis/plugin/src/main/java/org/eclipse/kapua/broker/artemis/plugin/security/SecurityPlugin.java#L140
> >>> ).
> >>> Also the fully qualified queue name still use the “original” clientId
> >>> without the account_name prefix
> >>>
> >>> We received a suggestion to use the interceptor but unfortunately the
> >>> MQTTConnect is final and has all the fields final so we cannot change
> the
> >>> clientId
> >>> We tried, just as experiment, using reflection to change the
> >>> accessibility (no security manager) and it seems to work. But,
> obviously,
> >>> is just an experiment and cannot be used in a real environment.
> >>>
> >>> The MQTTConnect message is created by MQTTDecoder (
> >>>
> https://github.com/netty/netty/blob/4.1/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java#L534
> )
> >>> but changing this part to introduce a callback that allows to change
> the
> >>> decoded clientId is out of the scope of this layer IMHO.
> >>>
> >>> If someone has suggestion or, better, a solution please tell me!
> >>>
> >>> Thanks!
> >>>
> >>> Riccardo Modanese
> >>>
> >>>
>
