From c8b92a59b167b4b292528158cf55ea8dec97132f Mon Sep 17 00:00:00 2001 From: Gilbert Wong Date: Wed, 5 Sep 2018 19:06:34 +0800 Subject: [PATCH 1/5] check topic alias --- src/emqx_mqtt_caps.erl | 10 +++++-- src/emqx_protocol.erl | 66 ++++++++++++++++++++++-------------------- 2 files changed, 42 insertions(+), 34 deletions(-) diff --git a/src/emqx_mqtt_caps.erl b/src/emqx_mqtt_caps.erl index fdc29fae8..baec9920c 100644 --- a/src/emqx_mqtt_caps.erl +++ b/src/emqx_mqtt_caps.erl @@ -43,7 +43,9 @@ {mqtt_wildcard_subscription, true}]). -define(PUBCAP_KEYS, [max_qos_allowed, - mqtt_retain_available]). + mqtt_retain_available, + mqtt_topic_alias + ]). -define(SUBCAP_KEYS, [max_qos_allowed, max_topic_levels, mqtt_shared_subscription, @@ -60,6 +62,11 @@ do_check_pub(Props = #{qos := QoS}, [{max_qos_allowed, MaxQoS}|Caps]) -> true -> {error, ?RC_QOS_NOT_SUPPORTED}; false -> do_check_pub(Props, Caps) end; +do_check_pub(Props = #{ topic_alias := TopicAlias}, [{max_topic_alias, MaxTopicAlias}| Caps]) -> + case TopicAlias =< MaxTopicAlias andalso TopicAlias > 0 of + false -> {error, ?RC_TOPIC_ALIAS_INVALID}; + true -> do_check_pub(Props, Caps) + end; do_check_pub(#{retain := true}, [{mqtt_retain_available, false}|_Caps]) -> {error, ?RC_RETAIN_NOT_SUPPORTED}; do_check_pub(Props, [{mqtt_retain_available, _}|Caps]) -> @@ -136,4 +143,3 @@ with_env(Zone, Key, InitFun) -> Caps; ZoneCaps -> ZoneCaps end. - diff --git a/src/emqx_protocol.erl b/src/emqx_protocol.erl index ec104799e..898d0a04e 100644 --- a/src/emqx_protocol.erl +++ b/src/emqx_protocol.erl @@ -33,36 +33,36 @@ -export([shutdown/2]). -record(pstate, { - zone, - sendfun, - peername, - peercert, - proto_ver, - proto_name, - ackprops, - client_id, - is_assigned, - conn_pid, - conn_props, - ack_props, - username, - session, - clean_start, - topic_aliases, - packet_size, - will_topic, - will_msg, - keepalive, - mountpoint, - is_super, - is_bridge, - enable_ban, - enable_acl, - recv_stats, - send_stats, - connected, - connected_at - }). + zone, + sendfun, + peername, + peercert, + proto_ver, + proto_name, + ackprops, + client_id, + is_assigned, + conn_pid, + conn_props, + ack_props, + username, + session, + clean_start, + topic_aliases, + packet_size, + will_topic, + will_msg, + keepalive, + mountpoint, + is_super, + is_bridge, + enable_ban, + enable_acl, + recv_stats, + send_stats, + connected, + connected_at + }). -type(state() :: #pstate{}). -export_type([state/0]). @@ -631,9 +631,11 @@ check_publish(Packet, PState) -> run_check_steps([fun check_pub_caps/2, fun check_pub_acl/2], Packet, PState). -check_pub_caps(#mqtt_packet{header = #mqtt_packet_header{qos = QoS, retain = R}}, +check_pub_caps(#mqtt_packet{header = #mqtt_packet_header{qos = QoS, retain = Retain}, + variable = #mqtt_packet_publish{ properties = Properties}}, #pstate{zone = Zone}) -> - emqx_mqtt_caps:check_pub(Zone, #{qos => QoS, retain => R}). + #{'Topic-Alias' := TopicAlias} = Properties, + emqx_mqtt_caps:check_pub(Zone, #{qos => QoS, retain => Retain, topic_alias => TopicAlias}). check_pub_acl(_Packet, #pstate{is_super = IsSuper, enable_acl = EnableAcl}) when IsSuper orelse (not EnableAcl) -> From 876a983e93f7982b5624bd6c1b303e82f92c3c95 Mon Sep 17 00:00:00 2001 From: Gilbert Wong Date: Thu, 6 Sep 2018 13:37:26 +0800 Subject: [PATCH 2/5] Pub Packet delivered from client to server cannot contain sub id --- src/emqx_packet.erl | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/emqx_packet.erl b/src/emqx_packet.erl index 715526964..c3eab87e1 100644 --- a/src/emqx_packet.erl +++ b/src/emqx_packet.erl @@ -69,9 +69,9 @@ validate_packet_id(0) -> validate_packet_id(_) -> true. -validate_properties(?SUBSCRIBE, #{'Subscription-Identifier' := I}) - when I =< 0; I >= 16#FFFFFFF -> - error(subscription_identifier_invalid); +validate_properties(?SUBSCRIBE, #{'Subscription-Identifier' := _I}) -> + %% when I =< 0; I >= 16#FFFFFFF -> + error(protocol_error); validate_properties(?PUBLISH, # {'Topic-Alias':= I}) when I =:= 0 -> error(topic_alias_invalid); @@ -236,4 +236,3 @@ format_password(_Password) -> '******'. i(true) -> 1; i(false) -> 0; i(I) when is_integer(I) -> I. - From 9189d4ff41f00f6011fb2cc37eb11bebbb78bdea Mon Sep 17 00:00:00 2001 From: Gilbert Wong Date: Thu, 6 Sep 2018 14:24:07 +0800 Subject: [PATCH 3/5] catch topic_alias_invalid reasoncode --- src/emqx_protocol.erl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/emqx_protocol.erl b/src/emqx_protocol.erl index 898d0a04e..025fe9c93 100644 --- a/src/emqx_protocol.erl +++ b/src/emqx_protocol.erl @@ -312,9 +312,12 @@ process_packet(Packet = ?PUBLISH_PACKET(?QOS_0, Topic, _PacketId, _Payload), PSt case check_publish(Packet, PState) of {ok, PState1} -> do_publish(Packet, PState1); + {error, ?RC_TOPIC_ALIAS_INVALID} -> + ?LOG(error, "Protocol error - ~p", [?RC_TOPIC_ALIAS_INVALID], PState), + {error, ?RC_TOPIC_ALIAS_INVALID, PState}; {error, ReasonCode} -> ?LOG(warning, "Cannot publish qos0 message to ~s for ~s", [Topic, ReasonCode], PState), - {ok, PState} + {error, ReasonCode, PState} end; process_packet(Packet = ?PUBLISH_PACKET(?QOS_1, PacketId), PState) -> From c145cb89f406d4c481bedf459976fa93f09d9e84 Mon Sep 17 00:00:00 2001 From: Gilbert Wong Date: Thu, 6 Sep 2018 15:45:18 +0800 Subject: [PATCH 4/5] add validate_properties for PUBLISH and fix error for SUB --- src/emqx_packet.erl | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/emqx_packet.erl b/src/emqx_packet.erl index c3eab87e1..fc90cf492 100644 --- a/src/emqx_packet.erl +++ b/src/emqx_packet.erl @@ -69,12 +69,14 @@ validate_packet_id(0) -> validate_packet_id(_) -> true. -validate_properties(?SUBSCRIBE, #{'Subscription-Identifier' := _I}) -> - %% when I =< 0; I >= 16#FFFFFFF -> - error(protocol_error); -validate_properties(?PUBLISH, # {'Topic-Alias':= I}) +validate_properties(?SUBSCRIBE, #{'Subscription-Identifier' := I}) + when I =< 0; I >= 16#FFFFFFF -> + error(subscription_identifier_invalid); +validate_properties(?PUBLISH, #{'Topic-Alias':= I}) when I =:= 0 -> error(topic_alias_invalid); +validate_properties(?PUBLISH, #{'Subscription-Identifier' := _I}) -> + error(protocol_error); validate_properties(_, _) -> true. From 765ab5ad7b78450adae749266f24d6a50b50ecaa Mon Sep 17 00:00:00 2001 From: Gilbert Wong Date: Thu, 6 Sep 2018 19:09:29 +0800 Subject: [PATCH 5/5] Add condition to handle when mqx_topic_alias do not exist --- src/emqx_mqtt_caps.erl | 2 ++ src/emqx_protocol.erl | 60 +++++++++++++++++------------------ test/emqx_mqtt_caps_SUITE.erl | 32 ++++++++++++------- 3 files changed, 52 insertions(+), 42 deletions(-) diff --git a/src/emqx_mqtt_caps.erl b/src/emqx_mqtt_caps.erl index baec9920c..b8b7a5b3a 100644 --- a/src/emqx_mqtt_caps.erl +++ b/src/emqx_mqtt_caps.erl @@ -69,6 +69,8 @@ do_check_pub(Props = #{ topic_alias := TopicAlias}, [{max_topic_alias, MaxTopicA end; do_check_pub(#{retain := true}, [{mqtt_retain_available, false}|_Caps]) -> {error, ?RC_RETAIN_NOT_SUPPORTED}; +do_check_pub(Props, [{max_topic_alias, _} | Caps]) -> + do_check_pub(Props, Caps); do_check_pub(Props, [{mqtt_retain_available, _}|Caps]) -> do_check_pub(Props, Caps). diff --git a/src/emqx_protocol.erl b/src/emqx_protocol.erl index 025fe9c93..38f55d204 100644 --- a/src/emqx_protocol.erl +++ b/src/emqx_protocol.erl @@ -33,36 +33,36 @@ -export([shutdown/2]). -record(pstate, { - zone, - sendfun, - peername, - peercert, - proto_ver, - proto_name, - ackprops, - client_id, - is_assigned, - conn_pid, - conn_props, - ack_props, - username, - session, - clean_start, - topic_aliases, - packet_size, - will_topic, - will_msg, - keepalive, - mountpoint, - is_super, - is_bridge, - enable_ban, - enable_acl, - recv_stats, - send_stats, - connected, - connected_at - }). + zone, + sendfun, + peername, + peercert, + proto_ver, + proto_name, + ackprops, + client_id, + is_assigned, + conn_pid, + conn_props, + ack_props, + username, + session, + clean_start, + topic_aliases, + packet_size, + will_topic, + will_msg, + keepalive, + mountpoint, + is_super, + is_bridge, + enable_ban, + enable_acl, + recv_stats, + send_stats, + connected, + connected_at + }). -type(state() :: #pstate{}). -export_type([state/0]). diff --git a/test/emqx_mqtt_caps_SUITE.erl b/test/emqx_mqtt_caps_SUITE.erl index 8b840b91c..1d4c81b8d 100644 --- a/test/emqx_mqtt_caps_SUITE.erl +++ b/test/emqx_mqtt_caps_SUITE.erl @@ -38,7 +38,7 @@ t_get_set_caps(_) -> mqtt_wildcard_subscription => true }, Caps2 = Caps#{max_packet_size => 1048576}, - case emqx_mqtt_caps:get_caps(zone) of + case emqx_mqtt_caps:get_caps(zone) of Caps -> ok; Caps2 -> ok end, @@ -63,20 +63,28 @@ t_check_pub(_) -> {ok, _} = emqx_zone:start_link(), PubCaps = #{ max_qos_allowed => ?QOS_1, - mqtt_retain_available => false + mqtt_retain_available => false, + max_topic_alias => 4 }, emqx_zone:set_env(zone, '$mqtt_pub_caps', PubCaps), timer:sleep(100), + ct:log("~p", [emqx_mqtt_caps:get_caps(zone, publish)]), BadPubProps1 = #{ qos => ?QOS_2, retain => false - }, + }, {error, ?RC_QOS_NOT_SUPPORTED} = emqx_mqtt_caps:check_pub(zone, BadPubProps1), BadPubProps2 = #{ qos => ?QOS_1, retain => true - }, + }, {error, ?RC_RETAIN_NOT_SUPPORTED} = emqx_mqtt_caps:check_pub(zone, BadPubProps2), + BadPubProps3 = #{ + qos => ?QOS_1, + retain => false, + topic_alias => 5 + }, + {error, ?RC_TOPIC_ALIAS_INVALID} = emqx_mqtt_caps:check_pub(zone, BadPubProps3), PubProps = #{ qos => ?QOS_1, retain => false @@ -94,18 +102,18 @@ t_check_sub(_) -> mqtt_wildcard_subscription => true }, - ok = do_check_sub([{<<"client/stat">>, Opts}], [{<<"client/stat">>, Opts}]), + ok = do_check_sub([{<<"client/stat">>, Opts}], [{<<"client/stat">>, Opts}]), ok = do_check_sub(Caps#{max_qos_allowed => ?QOS_1}, [{<<"client/stat">>, Opts}], [{<<"client/stat">>, Opts#{qos => ?QOS_1}}]), - ok = do_check_sub(Caps#{max_topic_levels => 1}, - [{<<"client/stat">>, Opts}], + ok = do_check_sub(Caps#{max_topic_levels => 1}, + [{<<"client/stat">>, Opts}], [{<<"client/stat">>, Opts#{rc => ?RC_TOPIC_FILTER_INVALID}}]), - ok = do_check_sub(Caps#{mqtt_shared_subscription => false}, - [{<<"client/stat">>, Opts}], + ok = do_check_sub(Caps#{mqtt_shared_subscription => false}, + [{<<"client/stat">>, Opts}], [{<<"client/stat">>, Opts#{rc => ?RC_SHARED_SUBSCRIPTIONS_NOT_SUPPORTED}}]), - ok = do_check_sub(Caps#{mqtt_wildcard_subscription => false}, - [{<<"vlient/+/dsofi">>, Opts}], + ok = do_check_sub(Caps#{mqtt_wildcard_subscription => false}, + [{<<"vlient/+/dsofi">>, Opts}], [{<<"vlient/+/dsofi">>, Opts#{rc => ?RC_WILDCARD_SUBSCRIPTIONS_NOT_SUPPORTED}}]). - +