[BREAKING] Reverse SOFTWARE_ATTRIBUTE_OPT to avoid inverse logic (#1598)

As part of looking at #1588 , I figured that sending `SOFTWARE`
attribute is also part of a problem as it increases messages sent out by
coturn and thus increasing amplification factor. For 4.6.2, the
additional size is 24 bytes (4 bytes attribute header, and 20 bytes for
"Coturn-4.6.2 'Gorst'")

If we are to use an example from #1588, "A 62 byte request will be met
with Coturn’s 401 Unauthorized response which is 150 bytes, a factor of
~2.42." - without SOFTWARE the response will be 126 bytes which reduces
amplification factor to ~2.

As I observed with multiple providers using coturn - some of the are
sending it. Meaning, they do not set `--no-software-attribute` - most
probably due to lack of clarity about this setting.

I believe sending SOFTWARE_ATTRIBUTE should be off by default which is
hinted in the RFC
(https://datatracker.ietf.org/doc/html/rfc8489#section-16.1.2)

Detailed changes:
- Extract setting the attribute into a function to avoid code
duplication
- This option is now not reloadable
- The option is now called `software_attribute` because inverse logic
creates multiple double-not in the code which makes it harder to read.
- `no-software_attribute` is still functional but marked as deprecated
in documentation

Test Plan:
- Run local tests with different cli arguments (new and deprecated) and
confirm SOFTWARE attribute is off by default, and added when arguments
say so

README.turnserver

@@ -168,7 +168,10 @@ Flags:
 
 -o, --daemon		Run server as daemon.
 
---no-software-attribute	Production mode: hide the software version.
+--no-software-attribute	DEPRECATED. See "--software-attribute".
+
+--software-attribute	Send SOFTWARE_ATTRIBUTE on messages that can have it. Disabled by default.
+		Equals to deprecated option "--no-software-attribute false".	
 
 -f, --fingerprint	Use fingerprints in the TURN messages. If an incoming request
 			contains a fingerprint, then TURN server will always add

docker/coturn/turnserver.conf

@@ -601,14 +601,14 @@ syslog
 #
 #stun-only
 
-# Option to hide software version. Enhance security when used in production.
+# Option to show software version - disabled by default. 
 # Revealing the specific software version of the agent through the
 # SOFTWARE attribute might allow them to become more vulnerable to
 # attacks against software that is known to contain security holes.
 # Implementers SHOULD make usage of the SOFTWARE attribute a
-# configurable option (https://tools.ietf.org/html/rfc5389#section-16.1.2)
+# configurable option (https://datatracker.ietf.org/doc/html/rfc8489#section-16.1.2)
 #
-#no-software-attribute
+#software-attribute
 
 # Option to suppress STUN functionality, only TURN requests will be processed.
 # Run as TURN server only, all STUN requests will be ignored.

src/apps/relay/mainrelay.c

@@ -121,7 +121,7 @@ turn_params_t turn_params = {
     //////////////// Common params ////////////////////
     TURN_VERBOSE_NONE, /* verbose */
     0,                 /* turn_daemon */
-    0,                 /* no_software_attribute */
+    false,             /* software_attribute */
     0,                 /* web_admin_listen_on_workers */
 
     0, /* do_not_use_config_file */
@@ -1029,7 +1029,8 @@ static char Usage[] =
     " -v, --verbose					'Moderate' verbose mode.\n"
     " -V, --Verbose					Extra verbose mode, very annoying (for debug purposes only).\n"
     " -o, --daemon					Start process as daemon (detach from current shell).\n"
-    " --no-software-attribute	 		Production mode: hide the software version (formerly --prod).\n"
+    " --no-software-attribute	 		DEPRECATED Production mode: hide the software version.\n"
+    " --software-attribute	 		Enable sending software attribute (for debugging).\n"
     " -f, --fingerprint				Use fingerprints in the TURN messages.\n"
     " -a, --lt-cred-mech				Use the long-term credential mechanism.\n"
     " -z, --no-auth					Do not use any credential mechanism, allow anonymous access.\n"
@@ -1496,7 +1497,8 @@ enum EXTRA_OPTS {
   ADMIN_USER_QUOTA_OPT,
   SERVER_NAME_OPT,
   OAUTH_OPT,
-  NO_SOFTWARE_ATTRIBUTE_OPT,
+  SOFTWARE_ATTRIBUTE_OPT,
+  DEPRECATED_NO_SOFTWARE_ATTRIBUTE_OPT,
   NO_HTTP_OPT,
   SECRET_KEY_OPT,
   ACME_REDIRECT_OPT,
@@ -1578,8 +1580,8 @@ static const struct myoption long_options[] = {
     {"verbose", optional_argument, NULL, 'v'},
     {"Verbose", optional_argument, NULL, 'V'},
     {"daemon", optional_argument, NULL, 'o'},
-    /* deprecated: */ {"prod", optional_argument, NULL, NO_SOFTWARE_ATTRIBUTE_OPT},
-    {"no-software-attribute", optional_argument, NULL, NO_SOFTWARE_ATTRIBUTE_OPT},
+    /* deprecated: */ {"no-software-attribute", optional_argument, NULL, DEPRECATED_NO_SOFTWARE_ATTRIBUTE_OPT},
+    {"software-attribute", optional_argument, NULL, SOFTWARE_ATTRIBUTE_OPT},
     {"fingerprint", optional_argument, NULL, 'f'},
     {"check-origin-consistency", optional_argument, NULL, CHECK_ORIGIN_CONSISTENCY_OPT},
     {"no-udp", optional_argument, NULL, NO_UDP_OPT},
@@ -2170,8 +2172,11 @@ static void set_option(int c, char *value) {
       anon_credentials = 1;
     }
     break;
-  case NO_SOFTWARE_ATTRIBUTE_OPT:
-    turn_params.no_software_attribute = get_bool_value(value);
+  case DEPRECATED_NO_SOFTWARE_ATTRIBUTE_OPT:
+    turn_params.software_attribute = !(bool)get_bool_value(value);
+    break;
+  case SOFTWARE_ATTRIBUTE_OPT:
+    turn_params.software_attribute = (bool)get_bool_value(value);
     break;
   case 'f':
     turn_params.fingerprint = get_bool_value(value);

src/apps/relay/mainrelay.h

@@ -210,7 +210,7 @@ typedef struct _turn_params_ {
 
   int verbose;
   int turn_daemon;
-  int no_software_attribute;
+  bool software_attribute;
   int web_admin_listen_on_workers;
 
   int do_not_use_config_file;

src/apps/relay/netengine.c

@@ -1657,8 +1657,8 @@ static void setup_relay_server(struct relay_server *rs, ioa_engine_handle e, int
       DONT_FRAGMENT_SUPPORTED, start_user_check, check_new_allocation_quota, release_allocation_quota,
       turn_params.external_ip, &turn_params.check_origin, &turn_params.no_tcp_relay, &turn_params.no_udp_relay,
       &turn_params.stale_nonce, &turn_params.max_allocate_lifetime, &turn_params.channel_lifetime,
-      &turn_params.permission_lifetime, &turn_params.stun_only, &turn_params.no_stun,
-      &turn_params.no_software_attribute, &turn_params.web_admin_listen_on_workers, &turn_params.alternate_servers_list,
+      &turn_params.permission_lifetime, &turn_params.stun_only, &turn_params.no_stun, turn_params.software_attribute,
+      &turn_params.web_admin_listen_on_workers, &turn_params.alternate_servers_list,
       &turn_params.tls_alternate_servers_list, &turn_params.aux_servers_list, turn_params.udp_self_balance,
       &turn_params.no_multicast_peers, &turn_params.allow_loopback_peers, &turn_params.ip_whitelist,
       &turn_params.ip_blacklist, send_socket_to_relay, &turn_params.secure_stun, &turn_params.mobility,

src/apps/relay/turn_admin_server.c

@@ -1658,7 +1658,7 @@ static void https_finish_page(struct str_buffer *sb, ioa_socket_handle s, int cc
   str_buffer_append(sb, "</body>\r\n</html>\r\n");
 
   send_str_from_ioa_socket_tcp(s, "HTTP/1.1 200 OK\r\nServer: ");
-  if (!turn_params.no_software_attribute) {
+  if (turn_params.software_attribute) {
     send_str_from_ioa_socket_tcp(s, TURN_SOFTWARE);
   }
   send_str_from_ioa_socket_tcp(s, "\r\n");

src/server/ns_turn_server.c

@@ -75,13 +75,23 @@ static inline int get_family(int stun_family, ioa_engine_handle e, ioa_socket_ha
 ////////////////////////////////////////////////
 
 const char *get_version(turn_turnserver *server) {
-  if (server && !*server->no_software_attribute) {
+  if (server && server->software_attribute) {
     return (const char *)TURN_SOFTWARE;
   } else {
     return (const char *)"None";
   }
 }
 
+void maybe_add_software_attribute(turn_turnserver *server, ioa_network_buffer_handle nbh) {
+  if (server->software_attribute) {
+    const char *field = get_version(server);
+    size_t fsz = strlen(get_version(server));
+    size_t len = ioa_network_buffer_get_size(nbh);
+    stun_attr_add_str(ioa_network_buffer_data(nbh), &len, STUN_ATTRIBUTE_SOFTWARE, field, fsz);
+    ioa_network_buffer_set_size(nbh, len);
+  }
+}
+
 #define MAX_NUMBER_OF_UNKNOWN_ATTRS (128)
 
 int TURN_MAX_ALLOCATE_TIMEOUT = 60;
@@ -1768,13 +1778,7 @@ static int handle_turn_refresh(turn_turnserver *server, ts_ur_super_session *ss,
                                       (uint8_t *)ss->s_mobile_id, strlen(ss->s_mobile_id));
                     ioa_network_buffer_set_size(nbh, len);
 
-                    if (!(*server->no_software_attribute)) {
-                      const uint8_t *field = (const uint8_t *)get_version(server);
-                      size_t fsz = strlen(get_version(server));
-                      size_t len = ioa_network_buffer_get_size(nbh);
-                      stun_attr_add_str(ioa_network_buffer_data(nbh), &len, STUN_ATTRIBUTE_SOFTWARE, field, fsz);
-                      ioa_network_buffer_set_size(nbh, len);
-                    }
+                    maybe_add_software_attribute(server, nbh);
 
                     if (message_integrity) {
                       size_t len = ioa_network_buffer_get_size(nbh);
@@ -2252,13 +2256,7 @@ static void tcp_peer_accept_connection(ioa_socket_handle s, void *arg) {
 
     ioa_network_buffer_set_size(nbh, len);
 
-    if (!(*server->no_software_attribute)) {
-      const uint8_t *field = (const uint8_t *)get_version(server);
-      size_t fsz = strlen(get_version(server));
-      size_t len = ioa_network_buffer_get_size(nbh);
-      stun_attr_add_str(ioa_network_buffer_data(nbh), &len, STUN_ATTRIBUTE_SOFTWARE, field, fsz);
-      ioa_network_buffer_set_size(nbh, len);
-    }
+    maybe_add_software_attribute(server, nbh);
 
     if ((server->fingerprint) || ss->enforce_fingerprints) {
       size_t len = ioa_network_buffer_get_size(nbh);
@@ -2530,13 +2528,7 @@ int turnserver_accept_tcp_client_data_connection(turn_turnserver *server, tcp_co
       }
     }
 
-    if (!(*server->no_software_attribute)) {
-      size_t fsz = strlen(get_version(server));
-      const uint8_t *field = (const uint8_t *)get_version(server);
-      size_t len = ioa_network_buffer_get_size(nbh);
-      stun_attr_add_str(ioa_network_buffer_data(nbh), &len, STUN_ATTRIBUTE_SOFTWARE, field, fsz);
-      ioa_network_buffer_set_size(nbh, len);
-    }
+    maybe_add_software_attribute(server, nbh);
 
     if (message_integrity && ss) {
       size_t len = ioa_network_buffer_get_size(nbh);
@@ -3814,13 +3806,7 @@ static int handle_turn_command(turn_turnserver *server, ts_ur_super_session *ss,
                           (unsigned long long)(ss->id));
           }
 
-          if (!(*server->no_software_attribute)) {
-            const uint8_t *field = (const uint8_t *)get_version(server);
-            size_t fsz = strlen(get_version(server));
-            size_t len = ioa_network_buffer_get_size(nbh);
-            stun_attr_add_str(ioa_network_buffer_data(nbh), &len, STUN_ATTRIBUTE_SOFTWARE, field, fsz);
-            ioa_network_buffer_set_size(nbh, len);
-          }
+          maybe_add_software_attribute(server, nbh);
 
           send_turn_message_to(server, nbh, &response_origin, &response_destination);
           stun_report_binding(ss, STUN_PROMETHEUS_METRIC_TYPE_RESPONSE);
@@ -3920,13 +3906,7 @@ static int handle_turn_command(turn_turnserver *server, ts_ur_super_session *ss,
       *resp_constructed = 1;
     }
 
-    if (!(*server->no_software_attribute)) {
-      const uint8_t *field = (const uint8_t *)get_version(server);
-      size_t fsz = strlen(get_version(server));
-      size_t len = ioa_network_buffer_get_size(nbh);
-      stun_attr_add_str(ioa_network_buffer_data(nbh), &len, STUN_ATTRIBUTE_SOFTWARE, field, fsz);
-      ioa_network_buffer_set_size(nbh, len);
-    }
+    maybe_add_software_attribute(server, nbh);
 
     if (message_integrity) {
       size_t len = ioa_network_buffer_get_size(nbh);
@@ -4855,13 +4835,7 @@ static void peer_input_handler(ioa_socket_handle s, int event_type, ioa_net_data
     stun_attr_add_addr_str(ioa_network_buffer_data(nbh), &len, STUN_ATTRIBUTE_XOR_PEER_ADDRESS, &(in_buffer->src_addr));
     ioa_network_buffer_set_size(nbh, len);
 
-    if (!(*server->no_software_attribute)) {
-      const uint8_t *field = (const uint8_t *)get_version(server);
-      size_t fsz = strlen(get_version(server));
-      size_t len = ioa_network_buffer_get_size(nbh);
-      stun_attr_add_str(ioa_network_buffer_data(nbh), &len, STUN_ATTRIBUTE_SOFTWARE, field, fsz);
-      ioa_network_buffer_set_size(nbh, len);
-    }
+    maybe_add_software_attribute(server, nbh);
 
     if ((server->fingerprint) || ss->enforce_fingerprints) {
       size_t len = ioa_network_buffer_get_size(nbh);
@@ -4916,7 +4890,7 @@ void init_turn_server(turn_turnserver *server, turnserver_id id, int verbose, io
                       get_user_key_cb userkeycb, check_new_allocation_quota_cb chquotacb,
                       release_allocation_quota_cb raqcb, ioa_addr *external_ip, vintp check_origin, vintp no_tcp_relay,
                       vintp no_udp_relay, vintp stale_nonce, vintp max_allocate_lifetime, vintp channel_lifetime,
-                      vintp permission_lifetime, vintp stun_only, vintp no_stun, vintp no_software_attribute,
+                      vintp permission_lifetime, vintp stun_only, vintp no_stun, bool software_attribute,
                       vintp web_admin_listen_on_workers, turn_server_addrs_list_t *alternate_servers_list,
                       turn_server_addrs_list_t *tls_alternate_servers_list, turn_server_addrs_list_t *aux_servers_list,
                       int self_udp_balance, vintp no_multicast_peers, vintp allow_loopback_peers,
@@ -4977,7 +4951,7 @@ void init_turn_server(turn_turnserver *server, turnserver_id id, int verbose, io
   server->permission_lifetime = permission_lifetime;
   server->stun_only = stun_only;
   server->no_stun = no_stun;
-  server->no_software_attribute = no_software_attribute;
+  server->software_attribute = software_attribute;
   server->web_admin_listen_on_workers = web_admin_listen_on_workers;
 
   server->dont_fragment = dont_fragment;

src/server/ns_turn_server.h

@@ -134,7 +134,7 @@ struct _turn_turnserver {
   vintp permission_lifetime;
   vintp stun_only;
   vintp no_stun;
-  vintp no_software_attribute;
+  bool software_attribute;
   vintp web_admin_listen_on_workers;
   vintp secure_stun;
   turn_credential_type ct;
@@ -217,7 +217,7 @@ void init_turn_server(
     int fingerprint, dont_fragment_option_t dont_fragment, get_user_key_cb userkeycb,
     check_new_allocation_quota_cb chquotacb, release_allocation_quota_cb raqcb, ioa_addr *external_addr,
     vintp check_origin, vintp no_tcp_relay, vintp no_udp_relay, vintp stale_nonce, vintp max_allocate_lifetime,
-    vintp channel_lifetime, vintp permission_lifetime, vintp stun_only, vintp no_stun, vintp no_software_attribute,
+    vintp channel_lifetime, vintp permission_lifetime, vintp stun_only, vintp no_stun, bool software_attribute,
     vintp web_admin_listen_on_workers, turn_server_addrs_list_t *alternate_servers_list,
     turn_server_addrs_list_t *tls_alternate_servers_list, turn_server_addrs_list_t *aux_servers_list,
     int self_udp_balance, vintp no_multicast_peers, vintp allow_loopback_peers, ip_range_list_t *ip_whitelist,