test: Fix safe_sessions comments and add pinning test (#28249)

Add pinning test for SafeCookieData values, and update SafeSessions
middleware comments to match code.

Main comment changes:

- Fix description of cookie structure:
    - Specify hash algorithm (SHA256, not "H")
    - Don't try to describe internals of TimestampSigner; description was
      incorrect in several ways: Did not include string delimiters under
      base64 (there's JSON in there); did not include the actual MAC
      portion. Just describe general effect and shape of output.
    - Add missing trailing pipe delimiter in signed data hash input
- Use phrase "intermediate key" rather than the less familiar term "usage
  key"

openedx/core/djangoapps/safe_sessions/middleware.py

@@ -3,22 +3,29 @@ This module defines SafeSessionMiddleware that makes use of a
 SafeCookieData that cryptographically binds the user to the session id
 in the cookie.
 
-The implementation is inspired by the proposal in the following paper:
-http://www.cse.msu.edu/~alexliu/publications/Cookie/cookie.pdf
+The implementation is inspired in part by the proposal in the paper
+<http://www.cse.msu.edu/~alexliu/publications/Cookie/cookie.pdf>
+but deviates in a number of ways; mostly it just uses the technique
+of an intermediate key for HMAC.
 
 Note: The proposed protocol protects against replay attacks by
+use of channel binding—specifically, by
 incorporating the session key used in the SSL connection.  However,
 this does not suit our needs since we want the ability to reuse the
-same cookie over multiple SSL connections.  So instead, we mitigate
+same cookie over multiple browser sessions, and in any case the server
+will be behind a load balancer and won't have access to the correct
+SSL session information.  So instead, we mitigate
 replay attacks by enforcing session cookie expiration
 (via TimestampSigner) and assuming SESSION_COOKIE_SECURE (see below).
 
 We use django's built-in Signer class, which makes use of a built-in
-salted_hmac function that derives a usage-specific key from the
+salted_hmac function that derives an intermediate key from the
 server's SECRET_KEY, as proposed in the paper.
 
-Note: The paper proposes deriving a usage-specific key from the
+Note: The paper proposes deriving an intermediate key from the
 session's expiration time in order to protect against volume attacks.
+(Note that these hypothetical attacks would only succeed if HMAC-SHA1
+were found to be weak, and there is presently no indication of this.)
 However, since django does not always use an expiration time, we
 instead use a random key salt to prevent volume attacks.
 
@@ -30,28 +37,25 @@ of the session cookies.  Django instead relies on the browser to honor
 session cookie expiration.
 
 The resulting safe cookie data that gets stored as the value in the
-session cookie is a tuple of:
-    (
-        version,
-        session_id,
-        key_salt,
-        signature
-    )
+session cookie is:
 
-    where signature is:
-        signed_data : base64(HMAC_SHA1(signed_data, usage_key))
+    version '|' session_id '|' key_salt '|' signed_hash
 
-    where signed_data is:
-        H(version | session_id | user_id) : timestamp
+where signed_hash is a structure incorporating the following value and
+a MAC (via TimestampSigner):
 
-    where usage_key is:
-        SHA1(key_salt + 'signer' + settings.SECRET_KEY)
+    SHA256(version '|' session_id '|' user_id '|')
+
+TimestampSigner uses HMAC-SHA1 to derive a key from key_salt and the
+server's SECRET_KEY; see django.core.signing for more details on the
+structure of the output (which takes the form of colon-delimited
+Base64.)
 
 Note: We assume that the SESSION_COOKIE_SECURE setting is set to
 TRUE to prevent inadvertent leakage of the session cookie to a
 person-in-the-middle.  The SESSION_COOKIE_SECURE flag indicates
 to the browser that the cookie should be sent only over an
-SSL-protected channel.  Otherwise, a session hijacker could copy
+SSL-protected channel.  Otherwise, a connection eavesdropper could copy
 the entire cookie and use it to impersonate the victim.
 
 Custom Attributes:
@@ -121,7 +125,7 @@ class SafeCookieData:
             session_id (string): Unique and unguessable session
                 identifier to which this safe cookie data is bound.
             key_salt (string): A securely generated random string that
-                is used to derive a usage-specific secret key for
+                is used to derive an intermediate secret key for
                 signing the safe cookie data to protect against volume
                 attacks.
             signature (string): Cryptographically created signature
@@ -185,9 +189,10 @@ class SafeCookieData:
 
     def sign(self, user_id):
         """
-        Computes the signature of this safe cookie data.
-        A signed value of hash(version | session_id | user_id):timestamp
-        with a usage-specific key derived from key_salt.
+        Signs the safe cookie data for this user using a timestamped signature
+        and an intermediate key derived from key_salt and server's SECRET_KEY.
+        Value under signature is the hexadecimal string from
+        SHA256(version '|' session_id '|' user_id '|').
         """
         data_to_sign = self._compute_digest(user_id)
         self.signature = signing.dumps(data_to_sign, salt=self.key_salt)
@@ -214,7 +219,7 @@ class SafeCookieData:
 
     def _compute_digest(self, user_id):
         """
-        Returns hash(version | session_id | user_id |)
+        Returns SHA256(version '|' session_id '|' user_id '|') hex string.
         """
         hash_func = sha256()
         for data_item in [self.version, self.session_id, user_id]:

openedx/core/djangoapps/safe_sessions/tests/test_safe_cookie_data.py

@@ -201,3 +201,38 @@ class TestSafeCookieData(TestSafeSessionsLogMixin, TestCase):
         digest = self.safe_cookie_data._compute_digest(self.user_id)
         setattr(self.safe_cookie_data, field_name, incorrect_field_value)
         assert digest != self.safe_cookie_data._compute_digest(self.user_id)
+
+    #---- Test roundtrip with pinned values ----#
+
+    def test_pinned_values(self):
+        """
+        Compute a cookie with all inputs held constant and assert that the
+        exact output never changes. This protects against unintentional
+        changes to the algorithm.
+        """
+        user_id = '8523'
+        session_id = 'SSdtIGEgc2Vzc2lvbiE'
+        a_random_string = 'HvGnjXf1b3jU'
+        timestamp = 1626895850
+
+        module = 'openedx.core.djangoapps.safe_sessions.middleware'
+        with patch(f"{module}.signing.time.time", return_value=timestamp):
+            with patch(f"{module}.get_random_string", return_value=a_random_string):
+                safe_cookie_data = SafeCookieData.create(session_id, user_id)
+        serialized_value = str(safe_cookie_data)
+
+        # **IMPORTANT**: If a change to the algorithm causes this test
+        # to start failing, you will either need to allow both the old
+        # and new format or all users will become logged out upon
+        # deploy of the changes.
+        #
+        # Also assumes SECRET_KEY is '85920908f28904ed733fe576320db18cabd7b6cd'
+        # (set in lms or cms.envs.test)
+        assert serialized_value == (
+            "1"
+            "|SSdtIGEgc2Vzc2lvbiE"
+            "|HvGnjXf1b3jU"
+            "|ImExZWZiNzVlZGFmM2FkZWZmYjM4YjI0ZmZkOWU4MzExODU0MTk4NmVlNGRiYzBlODdhYWUzOGM5MzVlNzk4NjUi"
+            ":1m6Hve"
+            ":OMhY2FL2pudJjSSXChtI-zR8QVA"
+        )