Changeset d510103 in trunk

Timestamp:

2023-05-23T18:53:22Z (2 years ago)

Author:

GitHub <noreply@…>

Branches:

master

Children:

839140c, 96670de, f4a099c

Parents:

41131ca (diff), b03db14 (diff)
Note: this is a merge changeset, the changes displayed below correspond to the merge itself.
Use the (diff) links above to see all the changes relative to each parent.

git-author:

Itamar Turner-Trauring <itamar@…> (2023-05-23 18:53:22)

git-committer:

GitHub <noreply@…> (2023-05-23 18:53:22)

Message:

Merge pull request #1301 from tahoe-lafs/4027-invalid-unicode

Invalid unicode in Authorization header should give better response

Fixes ticket:4027

Files:

• newsfragments/4027.minor (added)
• src/allmydata/storage/http_server.py (modified) (8 diffs)
• src/allmydata/test/test_storage_http.py (modified) (2 diffs)

src/allmydata/storage/http_server.py ¶

@@ -5 +5 @@
 from __future__ import annotations
 
-from typing import Any, Callable, Union, cast
+from typing import Any, Callable, Union, cast, Optional
 from functools import wraps
 from base64 import b64decode
@@ -76 +76 @@
 
     If too few secrets were given, or too many, a ``ClientSecretsException`` is
-    raised.
+    raised; its text is sent in the HTTP response.
     """
     string_key_to_enum = {e.value: e for e in Secrets}
@@ -85 +85 @@
             key = string_key_to_enum[string_key]
             value = b64decode(string_value)
+            if value == b"":
+                raise ClientSecretsException(
+                    "Failed to decode secret {}".format(string_key)
+                )
             if key in (Secrets.LEASE_CANCEL, Secrets.LEASE_RENEW) and len(value) != 32:
                 raise ClientSecretsException("Lease secrets must be 32 bytes long")
@@ -92 +96 @@
     if result.keys() != required_secrets:
         raise ClientSecretsException(
-            "Expected {} secrets, got {}".format(required_secrets, result.keys())
+            "Expected {} in X-Tahoe-Authorization headers, got {}".format(
+                [r.value for r in required_secrets], list(result.keys())
+            )
         )
     return result
@@ -117 +123 @@
                 try:
                     # Check Authorization header:
+                    try:
+                        auth_header = request.requestHeaders.getRawHeaders(
+                            "Authorization", [""]
+                        )[0].encode("utf-8")
+                    except UnicodeError:
+                        raise _HTTPError(http.BAD_REQUEST, "Bad Authorization header")
                     if not timing_safe_compare(
-                        request.requestHeaders.getRawHeaders("Authorization", [""])[
-                            0
-                        ].encode("utf-8"),
+                        auth_header,
                         swissnum_auth_header(self._swissnum),
                     ):
-                        raise _HTTPError(http.UNAUTHORIZED)
+                        raise _HTTPError(
+                            http.UNAUTHORIZED, "Wrong Authorization header"
+                        )
 
                     # Check secrets:
@@ -131 +143 @@
                     try:
                         secrets = _extract_secrets(authorization, required_secrets)
-                    except ClientSecretsException:
-                        raise _HTTPError(http.BAD_REQUEST)
+                    except ClientSecretsException as e:
+                        raise _HTTPError(http.BAD_REQUEST, str(e))
 
                     # Run the business logic:
@@ -273 +285 @@
     """
 
-    def __init__(self, code: int):
+    def __init__(self, code: int, body: Optional[str] = None):
+        Exception.__init__(self, (code, body))
         self.code = code
+        self.body = body
 
 
@@ -500 +514 @@
         """Handle ``_HTTPError`` exceptions."""
         request.setResponseCode(failure.value.code)
-        return b""
+        if failure.value.body is not None:
+            return failure.value.body
+        else:
+            return b""
 
     @app.handle_errors(CDDLValidationError)

src/allmydata/test/test_storage_http.py ¶

@@ -258 +258 @@
     _swissnum = SWISSNUM_FOR_TEST  # Match what the test client is using
 
+    @_authorized_route(_app, {}, "/noop", methods=["GET"])
+    def noop(self, request, authorization):
+        return "noop"
+
     @_authorized_route(_app, {Secrets.UPLOAD}, "/upload_secret", methods=["GET"])
     def validate_upload_secret(self, request, authorization):
@@ -341 +345 @@
         self._http_server.clock = self.client._clock
 
+    def test_bad_swissnum_from_client(self) -> None:
+        """
+        If the swissnum is invalid, a BAD REQUEST response code is returned.
+        """
+        headers = Headers()
+        # The value is not UTF-8.
+        headers.addRawHeader("Authorization", b"\x00\xFF\x00\xFF")
+        response = result_of(
+            self.client._treq.request(
+                "GET",
+                DecodedURL.from_text("http://127.0.0.1/noop"),
+                headers=headers,
+            )
+        )
+        self.assertEqual(response.code, 400)
+
+    def test_bad_secret(self) -> None:
+        """
+        If the secret is invalid (not base64), a BAD REQUEST
+        response code is returned.
+        """
+        bad_secret = b"upload-secret []<>"
+        headers = Headers()
+        headers.addRawHeader(
+            "X-Tahoe-Authorization",
+            bad_secret,
+        )
+        response = result_of(
+            self.client.request(
+                "GET",
+                DecodedURL.from_text("http://127.0.0.1/upload_secret"),
+                headers=headers,
+            )
+        )
+        self.assertEqual(response.code, 400)
+
     def test_authorization_enforcement(self):
         """
         The requirement for secrets is enforced by the ``_authorized_route``
         decorator; if they are not given, a 400 response code is returned.
+
+        Note that this refers to ``X-Tahoe-Authorization``, not the
+        ``Authorization`` header used for the swissnum.
         """
         # Without secret, get a 400 error.