launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:redact-signing-oops into launchpad:master.

Commit message:
Redact encrypted data from signing proxy OOPSes

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/386504

For encrypted requests to the signing service, it doesn't make sense to include the body data in timeline actions: we can't do anything with it since it's encrypted, and it may be very large.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:redact-signing-oops into launchpad:master.

diff --git a/lib/lp/services/signing/proxy.py b/lib/lp/services/signing/proxy.py
index 9c8ad6d..3147b5a 100644
--- a/lib/lp/services/signing/proxy.py
+++ b/lib/lp/services/signing/proxy.py
@@ -76,12 +76,17 @@ class SigningServiceClient:
             include a X-Response-Nonce, and returns back an encrypted
             response JSON.
         """
+        headers = kwargs.get("headers", {})
+
         timeline = get_request_timeline(get_current_browser_request())
+        redacted_kwargs = dict(kwargs)
+        if "X-Client-Public-Key" in headers and "data" in redacted_kwargs:
+            # The data will be encrypted, and possibly also very large.
+            del redacted_kwargs["data"]
         action = timeline.start(
             "services-signing-proxy-%s" % method, "%s %s" %
-            (path, json.dumps(kwargs)))
+            (path, json.dumps(redacted_kwargs)))
 
-        headers = kwargs.get("headers", {})
         response_nonce = None
         if "X-Response-Nonce" in headers:
             response_nonce = base64.b64decode(headers["X-Response-Nonce"])
diff --git a/lib/lp/services/signing/tests/test_proxy.py b/lib/lp/services/signing/tests/test_proxy.py
index 6abc64d..9163c05 100644
--- a/lib/lp/services/signing/tests/test_proxy.py
+++ b/lib/lp/services/signing/tests/test_proxy.py
@@ -18,8 +18,11 @@ from nacl.public import (
 from nacl.utils import random
 import responses
 from testtools.matchers import (
+    AfterPreprocessing,
     ContainsDict,
     Equals,
+    MatchesListwise,
+    MatchesStructure,
     )
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -34,6 +37,7 @@ from lp.services.signing.interfaces.signingserviceclient import (
     )
 from lp.services.signing.proxy import SigningServiceClient
 from lp.testing import TestCaseWithFactory
+from lp.testing.fixture import CaptureTimeline
 from lp.testing.layers import ZopelessLayer
 
 
@@ -57,6 +61,10 @@ class SigningServiceResponseFactory:
         self.nonce = random(Box.NONCE_SIZE)
         self.b64_nonce = base64.b64encode(self.nonce).decode("UTF-8")
 
+        self.response_nonce = random(Box.NONCE_SIZE)
+        self.b64_response_nonce = base64.b64encode(
+            self.response_nonce).decode("UTF-8")
+
         self.generated_public_key = PrivateKey.generate().public_key
         self.b64_generated_public_key = base64.b64encode(
             bytes(self.generated_public_key))
@@ -112,11 +120,10 @@ class SigningServiceResponseFactory:
         """
         # Patch SigningServiceClient._makeResponseNonce to return always the
         # same nonce, to simplify the tests.
-        response_nonce = random(Box.NONCE_SIZE)
         test_case.useFixture(MockPatch(
             'lp.services.signing.proxy.SigningServiceClient.'
             '_makeResponseNonce',
-            return_value=response_nonce))
+            return_value=self.response_nonce))
 
         responses.add(
             responses.GET, self.getUrl("/service-key"),
@@ -132,14 +139,14 @@ class SigningServiceResponseFactory:
             body=self._encryptPayload({
                 'fingerprint': self.generated_fingerprint,
                 'public-key': self.b64_generated_public_key.decode('utf8')
-            }, nonce=response_nonce),
+            }, nonce=self.response_nonce),
             status=201)
 
         responses.add(
             responses.POST, self.getUrl("/inject"),
             body=self._encryptPayload({
                 'fingerprint': self.generated_fingerprint,
-            }, nonce=response_nonce),
+            }, nonce=self.response_nonce),
             status=200)
 
         call_counts = {'/sign': 0}
@@ -150,7 +157,7 @@ class SigningServiceResponseFactory:
                 self.signed_msg_template % call_counts['/sign'])
             data = {'signed-message': signed.decode('utf8'),
                     'public-key': self.b64_generated_public_key.decode('utf8')}
-            return 201, {}, self._encryptPayload(data, response_nonce)
+            return 201, {}, self._encryptPayload(data, self.response_nonce)
 
         responses.add_callback(
             responses.POST, self.getUrl("/sign"),
@@ -174,6 +181,21 @@ class SigningServiceProxyTest(TestCaseWithFactory, TestWithFixtures):
         client = removeSecurityProxy(getUtility(ISigningServiceClient))
         self.addCleanup(client._cleanCaches)
 
+        self.timeline = self.useFixture(CaptureTimeline()).timeline
+
+    def assertTimeline(self, expected_details):
+        matchers = []
+        for method, path, kwargs in expected_details:
+            matchers.append(MatchesStructure(
+                category=Equals("services-signing-proxy-%s" % method),
+                detail=AfterPreprocessing(
+                    lambda detail: detail.split(" ", 1),
+                    MatchesListwise([
+                        Equals(path),
+                        AfterPreprocessing(json.loads, Equals(kwargs)),
+                        ]))))
+        self.assertThat(self.timeline.actions, MatchesListwise(matchers))
+
     @responses.activate
     def test_get_service_public_key(self):
         self.response_factory.addResponses(self)
@@ -194,6 +216,8 @@ class SigningServiceProxyTest(TestCaseWithFactory, TestWithFixtures):
         self.assertEqual(
             self.response_factory.getUrl("/service-key"), call.request.url)
 
+        self.assertTimeline([("GET", "/service-key", {})])
+
     @responses.activate
     def test_get_nonce(self):
         self.response_factory.addResponses(self)
@@ -211,6 +235,8 @@ class SigningServiceProxyTest(TestCaseWithFactory, TestWithFixtures):
         self.assertEqual(
             self.response_factory.getUrl("/nonce"), call.request.url)
 
+        self.assertTimeline([("POST", "/nonce", {})])
+
     @responses.activate
     def test_generate_unknown_key_type_raises_exception(self):
         self.response_factory.addResponses(self)
@@ -255,9 +281,26 @@ class SigningServiceProxyTest(TestCaseWithFactory, TestWithFixtures):
         self.assertThat(http_generate.request.headers, ContainsDict({
             "Content-Type": Equals("application/x-boxed-json"),
             "X-Client-Public-Key": Equals(config.signing.client_public_key),
-            "X-Nonce": Equals(self.response_factory.b64_nonce)}))
+            "X-Nonce": Equals(self.response_factory.b64_nonce),
+            "X-Response-Nonce": Equals(
+                self.response_factory.b64_response_nonce),
+            }))
         self.assertIsNotNone(http_generate.request.body)
 
+        self.assertTimeline([
+            ("POST", "/nonce", {}),
+            ("GET", "/service-key", {}),
+            ("POST", "/generate", {
+                "headers": {
+                    "Content-Type": "application/x-boxed-json",
+                    "X-Client-Public-Key": config.signing.client_public_key,
+                    "X-Nonce": self.response_factory.b64_nonce,
+                    "X-Response-Nonce":
+                        self.response_factory.b64_response_nonce,
+                    },
+                }),
+            ])
+
     @responses.activate
     def test_sign_invalid_mode(self):
         signing = getUtility(ISigningServiceClient)
@@ -313,9 +356,26 @@ class SigningServiceProxyTest(TestCaseWithFactory, TestWithFixtures):
         self.assertThat(http_sign.request.headers, ContainsDict({
             "Content-Type": Equals("application/x-boxed-json"),
             "X-Client-Public-Key": Equals(config.signing.client_public_key),
-            "X-Nonce": Equals(self.response_factory.b64_nonce)}))
+            "X-Nonce": Equals(self.response_factory.b64_nonce),
+            "X-Response-Nonce": Equals(
+                self.response_factory.b64_response_nonce),
+            }))
         self.assertIsNotNone(http_sign.request.body)
 
+        self.assertTimeline([
+            ("POST", "/nonce", {}),
+            ("GET", "/service-key", {}),
+            ("POST", "/sign", {
+                "headers": {
+                    "Content-Type": "application/x-boxed-json",
+                    "X-Client-Public-Key": config.signing.client_public_key,
+                    "X-Nonce": self.response_factory.b64_nonce,
+                    "X-Response-Nonce":
+                        self.response_factory.b64_response_nonce,
+                    },
+                }),
+            ])
+
         # It should have returned the correct JSON content, with signed
         # message from the API and the public-key.
         self.assertEqual(2, len(data))
@@ -365,9 +425,26 @@ class SigningServiceProxyTest(TestCaseWithFactory, TestWithFixtures):
         self.assertThat(http_inject.request.headers, ContainsDict({
             "Content-Type": Equals("application/x-boxed-json"),
             "X-Client-Public-Key": Equals(config.signing.client_public_key),
-            "X-Nonce": Equals(self.response_factory.b64_nonce)}))
+            "X-Nonce": Equals(self.response_factory.b64_nonce),
+            "X-Response-Nonce": Equals(
+                self.response_factory.b64_response_nonce),
+            }))
         self.assertIsNotNone(http_inject.request.body)
 
+        self.assertTimeline([
+            ("POST", "/nonce", {}),
+            ("GET", "/service-key", {}),
+            ("POST", "/inject", {
+                "headers": {
+                    "Content-Type": "application/x-boxed-json",
+                    "X-Client-Public-Key": config.signing.client_public_key,
+                    "X-Nonce": self.response_factory.b64_nonce,
+                    "X-Response-Nonce":
+                        self.response_factory.b64_response_nonce,
+                    },
+                }),
+            ])
+
     @responses.activate
     def test_inject_invalid_key_type(self):
         signing = getUtility(ISigningServiceClient)