From 502189ba42469d3332bc0658caa2bd0de1e3fcb9 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 24 Aug 2015 16:14:49 +0200 Subject: Add service_opt() strict_mbit There are differing opinions on whether or not reception of an arbitrary AVP setting the M-bit is an error. 1.3.4 of RFC 6733 says this about how an existing Diameter application may be modified: o The M-bit allows the sender to indicate to the receiver whether or not understanding the semantics of an AVP and its content is mandatory. If the M-bit is set by the sender and the receiver does not understand the AVP or the values carried within that AVP, then a failure is generated (see Section 7). It is the decision of the protocol designer when to develop a new Diameter application rather than extending Diameter in other ways. However, a new Diameter application MUST be created when one or more of the following criteria are met: M-bit Setting An AVP with the M-bit in the MUST column of the AVP flag table is added to an existing Command/Application. An AVP with the M-bit in the MAY column of the AVP flag table is added to an existing Command/Application. The point here is presumably interoperability: that the command grammar should specify explicitly what mandatory AVPs much be understood, and that anything more is an error. On the other hand, 3.2 says thus about command grammars: avp-name = avp-spec / "AVP" ; The string "AVP" stands for *any* arbitrary AVP ; Name, not otherwise listed in that Command Code ; definition. The inclusion of this string ; is recommended for all CCFs to allow for ; extensibility. This renders 1.3.4 pointless unless "*any* AVP" is qualified by "not setting the M-bit", since the sender can effectively violate 1.3.4 without this necessitating an error at the receiver. If clients add arbitrary AVPs setting the M-bit then request handling becomes more implementation-dependent. The current interpretation in diameter is strict: if a command grammar doesn't explicitly allow an AVP setting the M-bit then reception of such an AVP is regarded as an error. The strict_mbit option now allows this behaviour to be changed, false turning all responsibility for the M-bit over to the user. --- lib/diameter/doc/src/diameter.xml | 43 ++++++++++++++++++++++++++++++ lib/diameter/include/diameter_gen.hrl | 8 ++++-- lib/diameter/src/base/diameter.erl | 1 + lib/diameter/src/base/diameter_codec.erl | 10 ++++--- lib/diameter/src/base/diameter_config.erl | 3 +++ lib/diameter/src/base/diameter_service.erl | 4 ++- lib/diameter/src/base/diameter_traffic.erl | 4 ++- 7 files changed, 65 insertions(+), 8 deletions(-) diff --git a/lib/diameter/doc/src/diameter.xml b/lib/diameter/doc/src/diameter.xml index 854bc5b432..b0cff32c9a 100644 --- a/lib/diameter/doc/src/diameter.xml +++ b/lib/diameter/doc/src/diameter.xml @@ -912,6 +912,49 @@ Options monitor and link are ignored.

Defaults to the empty list.

+ +{strict_mbit, boolean()} + +

+Whether or not to regard an AVP setting the M-bit as erroneous when +the command grammar in question does not explicitly allow the AVP. +If true then such AVPs are regarded as 5001 errors, +DIAMETER_AVP_UNSUPPORTED. +If false then the M-bit is ignored and policing +it becomes the receiver's responsibility.

+ +

+Defaults to true.

+ + +

+RFC 6733 is unclear about the semantics of the M-bit. +One the one hand, the CCF specification in section 3.2 documents AVP +in a command grammar as meaning any arbitrary AVP; on the +other hand, 1.3.4 states that AVPs setting the M-bit cannot be added +to an existing command: the modified command must instead be +placed in a new Diameter application.

+

+The reason for the latter is presumably interoperability: +allowing arbitrary AVPs setting the M-bit in a command makes its +interpretation implementation-dependent, since there's no +guarantee that all implementations will understand the same set of +arbitrary AVPs in the context of a given command. +However, interpreting AVP in a command grammar as any +AVP, regardless of M-bit, renders 1.3.4 meaningless, since the receiver +can simply ignore any AVP it thinks isn't relevant, regardless of the +sender's intent.

+

+Beware of confusing mandatory in the sense of the M-bit with mandatory +in the sense of the command grammar. +The former is a semantic requirement: that the receiver understand the +semantics of the AVP in the context in question. +The latter is a syntactic requirement: whether or not the AVP must +occur in the message in question.

+ + + + {string_decode, boolean()} diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl index ac2126cdc5..39ccdc9873 100644 --- a/lib/diameter/include/diameter_gen.hrl +++ b/lib/diameter/include/diameter_gen.hrl @@ -30,7 +30,10 @@ %% Key to a value in the process dictionary that determines whether or %% not an unrecognized AVP setting the M-bit should be regarded as an -%% error or not. See is_strict/0. +%% error or not. See is_strict/0. This is only used to relax M-bit +%% interpretation inside Grouped AVPs not setting the M-bit. The +%% service_opt() strict_mbit can be used to disable the check +%% globally. -define(STRICT_KEY, strict). %% Key that says whether or not we should do a best-effort decode @@ -447,7 +450,8 @@ relax(_, _) -> false. is_strict() -> - false /= getr(?STRICT_KEY). + diameter_codec:getopt(strict_mbit) + andalso false /= getr(?STRICT_KEY). %% relax/1 %% diff --git a/lib/diameter/src/base/diameter.erl b/lib/diameter/src/base/diameter.erl index 010f977b97..9d71bcbbf8 100644 --- a/lib/diameter/src/base/diameter.erl +++ b/lib/diameter/src/base/diameter.erl @@ -311,6 +311,7 @@ call(SvcName, App, Message) -> | {sequence, sequence() | evaluable()} | {share_peers, remotes()} | {string_decode, boolean()} + | {strict_mbit, boolean()} | {incoming_maxlen, message_length()} | {use_shared_peers, remotes()} | {spawn_opt, list()}. diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl index f900bb0c5e..aab8b5887e 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -76,9 +76,10 @@ setopts(Opts) when is_list(Opts) -> lists:foreach(fun setopt/1, Opts). -%% Decode stringish types to string()? The default true is for -%% backwards compatibility. -setopt({string_decode = K, false = B}) -> +%% The default string_decode true is for backwards compatibility. +setopt({K, false = B}) + when K == string_decode; + K == strict_mbit -> setopt(K, B); %% Regard anything but the generated RFC 3588 dictionary as modern. @@ -96,7 +97,8 @@ setopt(Key, Value) -> getopt(Key) -> case get({diameter, Key}) of - undefined when Key == string_decode -> + undefined when Key == string_decode; + Key == strict_mbit -> true; undefined when Key == rfc -> 6733; diff --git a/lib/diameter/src/base/diameter_config.erl b/lib/diameter/src/base/diameter_config.erl index 89b26707ad..242b6b4d08 100644 --- a/lib/diameter/src/base/diameter_config.erl +++ b/lib/diameter/src/base/diameter_config.erl @@ -646,6 +646,7 @@ make_config(SvcName, Opts) -> {?NOMASK, sequence}, {nodes, restrict_connections}, {16#FFFFFF, incoming_maxlen}, + {true, strict_mbit}, {true, string_decode}, {[], spawn_opt}]), @@ -684,12 +685,14 @@ opt(K, false = B) K == use_shared_peers; K == monitor; K == restrict_connections; + K == strict_mbit; K == string_decode -> B; opt(K, true = B) when K == share_peers; K == use_shared_peers; + K == strict_mbit; K == string_decode -> B; diff --git a/lib/diameter/src/base/diameter_service.erl b/lib/diameter/src/base/diameter_service.erl index a31cef2c8c..7d4a24f7a0 100644 --- a/lib/diameter/src/base/diameter_service.erl +++ b/lib/diameter/src/base/diameter_service.erl @@ -128,6 +128,7 @@ | {share_peers, diameter:remotes()} %% broadcast to | {use_shared_peers, diameter:remotes()} %% use from | {restrict_connections, diameter:restriction()} + | {strict_mbit, boolean()} | {string_decode, boolean()} | {incoming_maxlen, diameter:message_length()}]}). %% shared_peers reflects the peers broadcast from remote nodes. @@ -697,7 +698,8 @@ service_options(Opts) -> ?RESTRICT)}, {spawn_opt, proplists:get_value(spawn_opt, Opts, [])}, {string_decode, proplists:get_value(string_decode, Opts, true)}, - {incoming_maxlen, proplists:get_value(incoming_maxlen, Opts, 16#FFFFFF)}]. + {incoming_maxlen, proplists:get_value(incoming_maxlen, Opts, 16#FFFFFF)}, + {strict_mbit, proplists:get_value(strict_mbit, Opts, true)}]. %% The order of options is significant since we match against the list. mref(false = No) -> diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 230a05fa11..6be2c35d48 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -79,6 +79,7 @@ apps :: [#diameter_app{}], sequence :: diameter:sequence(), codec :: [{string_decode, boolean()} + | {strict_mbit, boolean()} | {incoming_maxlen, diameter:message_length()}]}). %% Note that incoming_maxlen is currently handled in diameter_peer_fsm, %% so that any message exceeding the maximum is discarded. Retain the @@ -108,7 +109,8 @@ make_recvdata([SvcName, PeerT, Apps, SvcOpts | _]) -> sequence = Mask, codec = [T || {K,_} = T <- SvcOpts, lists:member(K, [string_decode, - incoming_maxlen])]}. + incoming_maxlen, + strict_mbit])]}. %% --------------------------------------------------------------------------- %% peer_up/1 -- cgit v1.2.3 From f616bf9f03feb8d9601a4c6ece552e13ec492fb8 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 7 Sep 2015 16:57:30 +0200 Subject: Fix watchdog function_clause Commit 4f365c07 introduced the error on set_watchdog/2, as a consequence of timeout/1 returning stop, which only happens with accepting transports with {restrict_connections, false}. --- lib/diameter/src/base/diameter_watchdog.erl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/diameter/src/base/diameter_watchdog.erl b/lib/diameter/src/base/diameter_watchdog.erl index 3c0d8f6f6e..ea8b2fdb0e 100644 --- a/lib/diameter/src/base/diameter_watchdog.erl +++ b/lib/diameter/src/base/diameter_watchdog.erl @@ -541,13 +541,13 @@ set_watchdog(#watchdog{tref = undefined} = S) -> %% Timer already set: start at new one only at expiry. set_watchdog(#watchdog{} = S) -> - S#watchdog{tref = diameter_lib:now()}; - -set_watchdog(stop = No) -> - No. + S#watchdog{tref = diameter_lib:now()}. %% set_watchdog/2 +set_watchdog(_, stop = No) -> + No; + set_watchdog(Ms, #watchdog{tw = TwInit} = S) -> S#watchdog{tref = erlang:start_timer(tw(TwInit, Ms), self(), tw)}. -- cgit v1.2.3 From ae57bf9b81a996889676c01656445824cd4b3fa7 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 10 Sep 2015 14:56:22 +0200 Subject: Update appup for 18.1 OTP-12947 strict_mbit service_opt() OTP-12969 diameter_watchdog function_clause No load order requirements. --- lib/diameter/src/diameter.appup.src | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/diameter/src/diameter.appup.src b/lib/diameter/src/diameter.appup.src index 788ea790fa..b77043d983 100644 --- a/lib/diameter/src/diameter.appup.src +++ b/lib/diameter/src/diameter.appup.src @@ -52,8 +52,10 @@ {load_module, diameter_lib}, {load_module, diameter_peer}, {load_module, diameter_reg}, + {load_module, diameter_traffic}, {load_module, diameter_service}, {load_module, diameter_sync}, + {load_module, diameter}, {load_module, diameter_gen_base_rfc6733}, {load_module, diameter_gen_acct_rfc6733}, {load_module, diameter_gen_base_rfc3588}, @@ -89,8 +91,10 @@ {load_module, diameter_gen_base_rfc3588}, {load_module, diameter_gen_acct_rfc6733}, {load_module, diameter_gen_base_rfc6733}, + {load_module, diameter}, {load_module, diameter_sync}, {load_module, diameter_service}, + {load_module, diameter_traffic}, {load_module, diameter_reg}, {load_module, diameter_peer}, {load_module, diameter_lib}, -- cgit v1.2.3