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

diameter: default port and transport for DiameterURI #9321

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

Conversation

wmnsk
Copy link

@wmnsk wmnsk commented Jan 21, 2025

This PR consists of two commits;

This fixes the crash when decoding a DiameterURI without a port number. The crash always happens if an Diameter answer with an AVP of type DiameterURI without port number is decoded, since the rfc option cannot be specified when handling an answer.

When rfc is missing in the option, the transport part of the diameter_uri becomes an empty atom (''), which causes the diameter module to fail to re-encode the decoded AVP (because it won't match diameter_types:'DiameterURI'/3 properly).

This sets tcp as the default transport as described in RFC6733 instead of leaving it empty.

Some notes/thoughts;

  • The first clause (portnr(<<>>, aaa, #{rfc := 6733}), transport(<<>>, #{rfc := 6733})) can be removed if it's preferable.
  • This can be tested by removing rfc => 6733 from diameter_codec_test:opts. I have no idea how to add a case for this.
  • Perhaps adding rfc => 6733 in the decode option by default is another way to solve this, but it may require wider consideration.

Please advise 🙂

Debug log w/ redbug:

% 13:46:05.976 <0.32401.513>(dead)
% diameter_types:'DiameterURI'(decode, <<"aaa://ims.foo.com">>, #{app_dictionary => dia_3gpp_Cx,decode_format => map,failed_avp => false,
  incoming_maxlen => 16777215,module => dia_3gpp,
  restrict_connections => nodes,
  sequence => {0,28},
  share_peers => false,spawn_opt => [],strict_arities => false,
  strict_mbit => true,string_decode => false,traffic_counters => true,
  use_shared_peers => false})

% 13:46:05.976 <0.32401.513>(dead)
% diameter_types:scan_uri(<<"aaa://ims.foo.com">>, #{app_dictionary => dia_3gpp_Cx,decode_format => map,failed_avp => false,
  incoming_maxlen => 16777215,module => dia_3gpp,
  restrict_connections => nodes,
  sequence => {0,28},
  share_peers => false,spawn_opt => [],strict_arities => false,
  strict_mbit => true,string_decode => false,traffic_counters => true,
  use_shared_peers => false})

% 13:46:05.976 <0.32401.513>(dead)
% re:run(<<"aaa://ims.foo.com">>, "^(aaas?)://([-a-zA-Z0-9.]{1,255})(:0{0,5}([0-9]{1,5}))?(;transport=(tcp|sctp|udp))?(;protocol=(diameter|radius|tacacs\\+))?$", [{capture,[1,2,4,6,8],binary}])

% 13:46:05.976 <0.32401.513>(dead)
% re:run/3 -> {match,[<<"aaa">>,<<"ims.foo.com">>,<<>>,<<>>,<<>>]}

% 13:46:05.976 <0.32401.513>(dead)
% diameter_types:to_atom(<<"aaa">>)

% 13:46:05.976 <0.32401.513>(dead)
% diameter_types:to_atom/1 -> aaa

% 13:46:05.976 <0.32401.513>(dead)
% diameter_types:portnr(<<>>, aaa, #{app_dictionary => dia_3gpp_Cx,decode_format => map,failed_avp => false,
  incoming_maxlen => 16777215,module => dia_3gpp,
  restrict_connections => nodes,
  sequence => {0,28},
  share_peers => false,spawn_opt => [],strict_arities => false,
  strict_mbit => true,string_decode => false,traffic_counters => true,
  use_shared_peers => false})

% 13:46:05.976 <0.32401.513>(dead)
% diameter_types:portnr/3 -> {error,badarg}

% 13:46:05.976 <0.32401.513>(dead)
% diameter_types:scan_uri/2 -> {error,badarg}

% 13:46:05.976 <0.32401.513>(dead)
% diameter_types:'DiameterURI'/3 -> {error,badarg}

This fixes the crash when decoding a DiameterURI without a port number.
The crash always happens if an Diameter answer with an AVP of type
DiameterURI without port number is decoded, since the `rfc` option
cannot be specified when handling an answer.
@CLAassistant
Copy link

CLAassistant commented Jan 21, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

github-actions bot commented Jan 21, 2025

CT Test Results

  2 files   34 suites   13m 8s ⏱️
132 tests 129 ✅ 3 💤 0 ❌
160 runs  157 ✅ 3 💤 0 ❌

Results for commit d69f935.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

When `rfc` is missing in the option, the transport part of the
diameter_uri becomes an empty atom (''), which causes the diameter
module to fail to re-encode the decoded AVP (because it won't match
diameter_types:'DiameterURI'/3 properly).

This sets `tcp` as the default transport as described in RFC6733
instead of leaving it empty.
@wmnsk wmnsk changed the title diameter: DiameterURI port defaults to 3868 diameter: default port and transport for DiameterURI Jan 21, 2025
@rickard-green rickard-green added the team:PS Assigned to OTP team PS label Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants