Merge branch 'maint'

6 files changed, 110 insertions, 38 deletions

diff --git a/lib/diameter/doc/src/diameter_app.xml b/lib/diameter/doc/src/diameter_app.xml
index 36a560b33c..90a36f8d53 100644
--- a/lib/diameter/doc/src/diameter_app.xml
+++ b/lib/diameter/doc/src/diameter_app.xml
@@ -565,7 +565,8 @@ Equivalent to</p>
 </pre>
 <p>
 where <c>Avps</c> sets the Origin-Host, Origin-Realm, the specified
-Result-Code and (if the request contained one) Session-Id AVP's.</p>
+Result-Code and (if the request contained one) Session-Id AVP's, and
+possibly Failed-AVP as described below.</p>
 
 <p>
 Returning a value other than 3xxx or 5xxx will cause the request
@@ -573,6 +574,14 @@ process in question to fail, as will returning a 5xxx value if the
 peer connection in question has been configured with the RFC 3588
 common dictionary <c>diameter_gen_base_rfc3588</c>.
 (Since RFC 3588 only allows 3xxx values in an answer-message.)</p>
+
+<p>
+When returning 5xxx, Failed-AVP will be populated with the AVP of the
+first matching Result-Code/AVP pair in the <c>errors</c> field of the
+argument &packet;, if found.
+If this is not appropriate then an answer-message should be
+constructed explicitly and returned in a <c>reply</c> tuple
+instead.</p>
 </item>
 
 <tag><c>{relay, Opts}</c></tag>
@@ -592,8 +601,7 @@ header of the relayed request.</p>
 The returned <c>Opts</c> should not specify <c>detach</c>.
 A subsequent &handle_answer;
 callback for the relayed request must return its first
-argument, the <c>#diameter_packet{}</c> record containing the answer
-message.
+argument, the &packet; containing the answer message.
 Note that the <c>extra</c> option can be specified to supply arguments
 that can distinguish the relay case from others if so desired.
 Any other return value (for example, from a
diff --git a/lib/diameter/doc/src/diameter_transport.xml b/lib/diameter/doc/src/diameter_transport.xml
index d347396ef8..1618d05c47 100644
--- a/lib/diameter/doc/src/diameter_transport.xml
+++ b/lib/diameter/doc/src/diameter_transport.xml
@@ -137,15 +137,14 @@ passed to the former.</p>
 
 <p>
 The start function should use the <c>Host-IP-Address</c> list in
-<c>Svc</c> and/or <c>Config</c> to select an appropriate list of local
-IP addresses, and should return this list if different from the
-<c>Svc</c> addresses.
+<c>Svc</c> and/or <c>Config</c> to select and return an appropriate
+list of local IP addresses.
 In the connecting case, the local address list can instead be
 communicated in a <c>connected</c> message (see &MESSAGES; below)
 following connection establishment.
 In either case, the local address list is used to populate
 <c>Host-IP-Address</c> AVPs in outgoing capabilities exchange
-messages.</p>
+messages if <c>Host-IP-Address</c> is unspecified.</p>
 
 <p>
 A transport process must implement the message interface documented below.
diff --git a/lib/diameter/src/base/diameter_capx.erl b/lib/diameter/src/base/diameter_capx.erl
index 9a443fead0..4b821f5139 100644
--- a/lib/diameter/src/base/diameter_capx.erl
+++ b/lib/diameter/src/base/diameter_capx.erl
@@ -282,9 +282,26 @@ build_CEA(_, LCaps, RCaps, Dict, CEA) ->
         [] ->
             Dict:'#set-'({'Result-Code', ?NOSECURITY}, CEA);
         [_] = IS ->
-            Dict:'#set-'({'Inband-Security-Id', IS}, CEA)
+            Dict:'#set-'({'Inband-Security-Id', inband_security(IS)}, CEA)
     end.
 
+%% Only set Inband-Security-Id if different from the default, since
+%% RFC 6733 recommends against the AVP:
+%%
+%% 6.10.  Inband-Security-Id AVP
+%%
+%%    The Inband-Security-Id AVP (AVP Code 299) is of type Unsigned32 and
+%%    is used in order to advertise support of the security portion of the
+%%    application.  The use of this AVP in CER and CEA messages is NOT
+%%    RECOMMENDED.  Instead, discovery of a Diameter entity's security
+%%    capabilities can be done either through static configuration or via
+%%    Diameter Peer Discovery as described in Section 5.2.
+
+inband_security([?NO_INBAND_SECURITY]) ->
+    [];
+inband_security([_] = IS) ->
+    IS.
+
 %% common_security/2
 
 common_security(#diameter_caps{inband_security_id = LS},
diff --git a/lib/diameter/src/base/diameter_peer_fsm.erl b/lib/diameter/src/base/diameter_peer_fsm.erl
index 6be4259510..4e55864168 100644
--- a/lib/diameter/src/base/diameter_peer_fsm.erl
+++ b/lib/diameter/src/base/diameter_peer_fsm.erl
@@ -233,20 +233,21 @@ start_transport(Addrs0, T) ->
         {TPid, Addrs, Tmo, Data} ->
             erlang:monitor(process, TPid),
             q_next(TPid, Addrs0, Tmo, Data),
-            {TPid, addrs(Addrs, Addrs0)};
+            {TPid, Addrs};
         No ->
             exit({shutdown, No})
     end.
 
-addrs([], Addrs0) ->
-    Addrs0;
-addrs(Addrs, _) ->
-    Addrs.
-
-svc(Svc, []) ->
-    Svc;
-svc(Svc, Addrs) ->
-    readdr(Svc, Addrs).
+svc(#diameter_service{capabilities = LCaps0} = Svc, Addrs) ->
+    #diameter_caps{host_ip_address = Addrs0}
+        = LCaps0,
+    case Addrs0 of
+        [] ->
+            LCaps = LCaps0#diameter_caps{host_ip_address = Addrs},
+            Svc#diameter_service{capabilities = LCaps};
+        [_|_] ->
+            Svc
+    end.
 
 readdr(#diameter_service{capabilities = LCaps0} = Svc, Addrs) ->
     LCaps = LCaps0#diameter_caps{host_ip_address = Addrs},
@@ -360,7 +361,7 @@ transition({diameter, {TPid, connected, Remote, LAddrs}},
                   service = Svc}
            = S) ->
     transition({diameter, {TPid, connected, Remote}},
-               S#state{service = readdr(Svc, LAddrs)});
+               S#state{service = svc(Svc, LAddrs)});
 
 %% Connection from peer.
 transition({diameter, {TPid, connected}},
@@ -702,13 +703,13 @@ build_answer('CER',
         N -> {cea(CEA, N, Dict0), [fun open/5, Pkt,
                                                SupportedApps,
                                                Caps,
-                                               {accept, hd([_] = IS)}]}
+                                               {accept, inband_security(IS)}]}
     catch
         ?FAILURE(Reason) ->
             rejected(Reason, {'CER', Reason, Caps, Pkt}, S)
     end;
 
-%% The error checks below are similar to those in diameter_service for
+%% The error checks below are similar to those in diameter_traffic for
 %% other messages. Should factor out the commonality.
 
 build_answer(Type,
@@ -719,6 +720,11 @@ build_answer(Type,
     RC = rc(H, Es),
     {answer(Type, RC, Es, S), post(Type, RC, Pkt, S)}.
 
+inband_security([]) ->
+    ?NO_INBAND_SECURITY;
+inband_security([IS]) ->
+    IS.
+
 cea(CEA, ok, _) ->
     CEA;
 cea(CEA, 2001, _) ->
@@ -742,7 +748,14 @@ rejected(N, T, S) ->
     rejected({N, []}, T, S).
 
 answer(Type, RC, Es, S) ->
-    set(answer(Type, RC, S), failed_avp([A || {_,A} <- Es])).
+    set(answer(Type, RC, S), failed_avp(RC, Es)).
+
+failed_avp(RC, [{RC, Avp} | _]) ->
+    [{'Failed-AVP', [{'AVP', [Avp]}]}];
+failed_avp(RC, [_ | Es]) ->
+    failed_avp(RC, Es);
+failed_avp(_, [] = No) ->
+    No.
 
 answer(Type, RC, S) ->
     answer_message(answer(Type, S), RC).
@@ -762,13 +775,6 @@ is_origin({N, _}) ->
         orelse N == 'Origin-Realm'
         orelse N == 'Origin-State-Id'.
 
-%% failed_avp/1
-
-failed_avp([] = No) ->
-    No;
-failed_avp(Avps) ->
-    [{'Failed-AVP', [[{'AVP', Avps}]]}].
-
 %% set/2
 
 set(Ans, []) ->
@@ -784,7 +790,7 @@ rc(#diameter_header{is_error = true}, _) ->
     3008;  %% DIAMETER_INVALID_HDR_BITS
 
 rc(_, [Bs|_])
-  when is_bitstring(Bs) ->
+  when is_bitstring(Bs) ->  %% from old code
     3009;  %% DIAMETER_INVALID_HDR_BITS
 
 rc(#diameter_header{version = ?DIAMETER_VERSION}, Es) ->
diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl
index 820d37535a..0b15e68ec7 100644
--- a/lib/diameter/src/base/diameter_traffic.erl
+++ b/lib/diameter/src/base/diameter_traffic.erl
@@ -479,10 +479,9 @@ answer_message(RC,
                #diameter_caps{origin_host  = {OH,_},
                               origin_realm = {OR,_}},
                Dict0,
-               #diameter_packet{avps = Avps}
-               = Pkt) ->
+               Pkt) ->
     ?LOG({error, RC}, Pkt),
-    {Dict0, answer_message(OH, OR, RC, Dict0, Avps)}.
+    {Dict0, answer_message(OH, OR, RC, Dict0, Pkt)}.
 
 %% resend/7
 
@@ -861,12 +860,14 @@ failed(Rec, FailedAvp, Dict) ->
 
 %% answer_message/5
 
-answer_message(OH, OR, RC, Dict0, Avps) ->
+answer_message(OH, OR, RC, Dict0, #diameter_packet{avps = Avps,
+                                                   errors = Es}) ->
     {Code, _, Vid} = Dict0:avp_header('Session-Id'),
     ['answer-message', {'Origin-Host', OH},
                        {'Origin-Realm', OR},
-                       {'Result-Code', RC}
-                       | session_id(Code, Vid, Dict0, Avps)].
+                       {'Result-Code', RC}]
+        ++ session_id(Code, Vid, Dict0, Avps)
+        ++ failed_avp(RC, Es).
 
 session_id(Code, Vid, Dict0, Avps)
   when is_list(Avps) ->
@@ -878,6 +879,15 @@ session_id(Code, Vid, Dict0, Avps)
             []
     end.
 
+%% Note that this should only match 5xxx result codes currently but
+%% don't bother distinguishing this case.
+failed_avp(RC, [{RC, Avp} | _]) ->
+    [{'Failed-AVP', [{'AVP', [Avp]}]}];
+failed_avp(RC, [_ | Es]) ->
+    failed_avp(RC, Es);
+failed_avp(_, [] = No) ->
+    No.
+
 %% find_avp/3
 
 find_avp(Code, Vid, Avps)
diff --git a/lib/diameter/test/diameter_3xxx_SUITE.erl b/lib/diameter/test/diameter_3xxx_SUITE.erl
index 0ec0d5020f..071b1a1177 100644
--- a/lib/diameter/test/diameter_3xxx_SUITE.erl
+++ b/lib/diameter/test/diameter_3xxx_SUITE.erl
@@ -43,6 +43,7 @@
          send_invalid_hdr_bits/1,
          send_missing_avp/1,
          send_ignore_missing_avp/1,
+         send_5xxx_missing_avp/1,
          send_double_error/1,
          send_3xxx/1,
          send_5xxx/1,
@@ -139,6 +140,7 @@ tc() ->
      send_invalid_hdr_bits,
      send_missing_avp,
      send_ignore_missing_avp,
+     send_5xxx_missing_avp,
      send_double_error,
      send_3xxx,
      send_5xxx].
@@ -279,6 +281,32 @@ send_ignore_missing_avp([_,_]) ->
 send_ignore_missing_avp(Config) ->
     send_ignore_missing_avp(?group(Config)).
 
+%% send_5xxx_missing_avp/1
+%%
+%% Send a request with a missing AVP that a callback answers
+%% with {answer_message, 5005}.
+
+%% RFC 6733 allows 5xxx in an answer-message.
+send_5xxx_missing_avp([_, rfc6733]) ->
+    #'diameter_base_answer-message'{'Result-Code' = 5005,  %% MISSING_AVP
+                                    'Failed-AVP' = [_],
+                                    'AVP' = []}
+        = call();
+
+%% RFC 3588 doesn't: sending answer fails.
+send_5xxx_missing_avp([_, rfc3588]) ->
+    {error, timeout} = call();
+
+%% Callback answers, ignores the error
+send_5xxx_missing_avp([_,_]) ->
+    #diameter_base_STA{'Result-Code' = 2001,  %% SUCCESS
+                       'Failed-AVP' = [],
+                       'AVP' = []}
+        = call();
+
+send_5xxx_missing_avp(Config) ->
+    send_5xxx_missing_avp(?group(Config)).
+
 %% send_double_error/1
 %%
 %% Send a request with both an invalid E-bit and a missing AVP.
@@ -403,7 +431,8 @@ prepare(Pkt0, Caps, send_double_error) ->
 
 prepare(Pkt, Caps, T)
   when T == send_missing_avp;
-       T == send_ignore_missing_avp ->
+       T == send_ignore_missing_avp;
+       T == send_5xxx_missing_avp ->
     Req = sta(Pkt, Caps),
     dehost(diameter_codec:encode(?DICT, Pkt#diameter_packet{msg = Req})).
 
@@ -487,7 +516,10 @@ request(T, Req, Caps)
 
 request(send_ignore_missing_avp, Req, Caps) ->
     {reply, #diameter_packet{msg = answer(Req, Caps),
-                             errors = false}}.  %% ignore errors
+                             errors = false}};  %% ignore errors
+
+request(send_5xxx_missing_avp, _Req, _Caps) ->
+    {answer_message, 5005}.  %% MISSING_AVP
 
 answer(Req, Caps) ->
     #diameter_base_STR{'Session-Id' = SId}