Support for specifying STUN server: input validation (1/3) (#1645)

Related #1460.

This PR adds the backend validation logic for processing the
user-provided input values for the STUN server (server + port). These
values will later [come from the
UI](#1647).

## Notes

- I added comments in the `janus.jcfg` file to shed some more light on
how the config parameters work.
- Based on these rules, I implemented the request validators.
- I created a unified, public `parse_h264_stun_address` validation, to
enforce that they are either both present or both absent.
- The `_SERVER_PATTERN` is intentionally broad, I’m not sure it would be
worth to have a stricter validation here. (At least I couldn’t find any
reasonably simple way, without either making the regex more complex, or
pulling in an external library.) For our purposes, it’s probably enough
to provide a sanity check, rather than a bullet-proof validation.
- The IP address validation can be done conveniently via the `ipaddress`
native Python package.

<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1645"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>

app/request_parsers/errors.py

@@ -18,5 +18,9 @@ class InvalidVideoSettingError(Error):
     pass
 
 
+class InvalidVideoSettingStunAddressError(Error):
+    code = 'INVALID_STUN_ADDRESS'
+
+
 class UnsupportedPastedCharacterError(Error):
     pass

app/request_parsers/video_settings.py

@@ -1,7 +1,12 @@
+import ipaddress
+import re
+
 import db.settings
 from request_parsers import errors
 from request_parsers import json
 
+_DOMAIN_PATTERN = re.compile(r'^[0-9a-z-.]{1,255}$')
+
 
 def parse_frame_rate(request):
     # pylint: disable=unbalanced-tuple-unpacking
@@ -57,6 +62,68 @@ def parse_streaming_mode(request):
             'The video streaming mode must be `MJPEG` or `H264`.') from e
 
 
+def parse_h264_stun_address(request):
+    """Parses the STUN address (server + port) from a request.
+
+    The server can be a valid domain name, or an IP address (IPv4 / IPv6). The
+    server and port values must either both be present, or both absent.
+
+    Args:
+        request: A Flask request object.
+
+    Returns:
+        A tuple containing (1) the server as string, and (2) the port as int.
+
+    Raises:
+        InvalidVideoSettingStunAddress
+    """
+    # pylint: disable=unbalanced-tuple-unpacking
+    (
+        server,
+        port,
+    ) = json.parse_json_body(request,
+                             required_fields=['h264StunServer', 'h264StunPort'])
+    server = _parse_h264_stun_server(server)
+    port = _parse_h264_stun_port(port)
+    if (server is None and port is not None) or (server is not None and
+                                                 port is None):
+        raise errors.InvalidVideoSettingStunAddressError(
+            'The server and port values must either both be given, or both '
+            'absent.')
+    return server, port
+
+
+def _parse_h264_stun_server(server):
+    if server is None:
+        return None
+    if not isinstance(server, str):
+        raise errors.InvalidVideoSettingStunAddressError(
+            'The server value must be of type string.')
+    try:
+        ipaddress.ip_address(server)
+        return server
+    except ValueError:
+        pass
+    if not _DOMAIN_PATTERN.match(server):
+        raise errors.InvalidVideoSettingStunAddressError(
+            'The server must be a valid domain name or IP address (IPv4/IPv6).')
+    return server
+
+
+def _parse_h264_stun_port(port):
+    if port is None:
+        return None
+    try:
+        port = _as_int(port)
+        if not 1 <= port <= 65535:
+            raise ValueError
+    except ValueError as e:
+        raise errors.InvalidVideoSettingStunAddressError(
+            'The port must be a positive integer, not greater than 65535.') \
+            from e
+    return port
+
+
 def _as_int(string):
     # Note: We need to cast the value to a string first otherwise the int
     # function forces floats into integers by simply cutting off the fractional

app/request_parsers/video_settings_test.py

@@ -158,3 +158,173 @@ def test_reject_incorrect_key(self):
         with self.assertRaises(errors.MissingFieldError):
             video_settings.parse_streaming_mode(
                 make_mock_request({'something': 'MJPEG'}))
+
+
+class VideoH264StunAddressParserTest(unittest.TestCase):
+
+    def test_accept_absent_values(self):
+        self.assertEqual((None, None),
+                         video_settings.parse_h264_stun_address(
+                             make_mock_request({
+                                 'h264StunServer': None,
+                                 'h264StunPort': None
+                             })))
+
+    def test_accept_valid_values(self):
+        self.assertEqual(('stun.example.com', 5672),
+                         video_settings.parse_h264_stun_address(
+                             make_mock_request({
+                                 'h264StunServer': 'stun.example.com',
+                                 'h264StunPort': 5672
+                             })))
+        self.assertEqual(
+            ('a', 5672),  # Smallest possible hostname
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'a',
+                    'h264StunPort': 5672
+                })))
+        self.assertEqual(
+            ('a' * 255, 5672),  # Longest possible hostname
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'a' * 255,
+                    'h264StunPort': 5672
+                })))
+        self.assertEqual(
+            ('stun.com', 1),  # Smallest possible port
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'stun.com',
+                    'h264StunPort': 1
+                })))
+        self.assertEqual(
+            ('stun.com', 65535),  # Longest possible port
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'stun.com',
+                    'h264StunPort': 65535
+                })))
+        self.assertEqual(
+            ('192.168.12.82', 15985),  # IPv4
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': '192.168.12.82',
+                    'h264StunPort': 15985
+                })))
+        self.assertEqual(
+            ('0000:0000:0000:0000:0000:ffff:c0a8:0c52', 3478),  # IPv6 (regular)
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': '0000:0000:0000:0000:0000:ffff:c0a8:0c52',
+                    'h264StunPort': 3478
+                })))
+        self.assertEqual(
+            ('::ffff:e4:1:c0a8:c52', 3478),  # IPv6 with shorthand
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': '::ffff:e4:1:c0a8:c52',
+                    'h264StunPort': 3478
+                })))
+
+    def test_reject_partial_values(self):
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'stun.example.com',
+                    'h264StunPort': None
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': None,
+                    'h264StunPort': 5672
+                }))
+
+    def test_reject_invalid_server(self):
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'EXAMPLE.ORG',
+                    'h264StunPort': 5672
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'https://example.org',
+                    'h264StunPort': 5672
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'example.org/stun',
+                    'h264StunPort': 5672
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'stun@example',
+                    'h264StunPort': 5672
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': '',
+                    'h264StunPort': 5672
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'a' * 256,  # Too long
+                    'h264StunPort': 5672
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': False,
+                    'h264StunPort': 5672
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 5672,
+                    'h264StunPort': 5672
+                }))
+
+    def test_reject_invalid_port(self):
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'stun.example.com',
+                    'h264StunPort': 0
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'stun.example.com',
+                    'h264StunPort': -1
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'stun.example.com',
+                    'h264StunPort': 65536
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'stun.example.com',
+                    'h264StunPort': 5672.0
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'stun.example.com',
+                    'h264StunPort': 'foo'
+                }))
+        with self.assertRaises(errors.InvalidVideoSettingStunAddressError):
+            video_settings.parse_h264_stun_address(
+                make_mock_request({
+                    'h264StunServer': 'stun.example.com',
+                    'h264StunPort': False
+                }))

debian-pkg/usr/share/tinypilot/templates/janus.jcfg.j2

@@ -21,7 +21,13 @@ loggers: {
 
 {% if janus_stun_server and janus_stun_port -%}
 nat: {
+	# The server value can be either a fully qualified domain name (such as
+	# stun.example.org), or an IP address (IPv4 or IPv6).
 	stun_server = "{{ janus_stun_server }}"
+	# Janus itself doesn’t enforce the port value, and would default to 3478 if
+	# the port number is not given. In order to be independent of this behavior,
+	# and to make the server address more explicit in the UI, we request our users
+	# to always specify both values.
 	stun_port = {{ janus_stun_port }}
 }
 {% endif -%}