launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/debug-snap-upload-auth into lp:launchpad.

Commit message:
Log some useful information from authorising macaroons while uploading snaps to the store, to make it easier to diagnose problems.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/debug-snap-upload-auth/+merge/325449
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/debug-snap-upload-auth into lp:launchpad.

=== modified file 'lib/lp/snappy/model/snapstoreclient.py'
--- lib/lp/snappy/model/snapstoreclient.py	2017-04-18 17:54:03 +0000
+++ lib/lp/snappy/model/snapstoreclient.py	2017-06-10 20:12:07 +0000
@@ -10,6 +10,7 @@
     'SnapStoreClient',
     ]
 
+import base64
 import json
 try:
     from json.decoder import JSONDecodeError
@@ -81,19 +82,44 @@
     # The union of the base64 and URL-safe base64 alphabets.
     allowed_chars = set(string.digits + string.letters + "+/=-_")
 
-    def __init__(self, root_macaroon_raw, unbound_discharge_macaroon_raw=None):
+    def __init__(self, root_macaroon_raw, unbound_discharge_macaroon_raw=None,
+                 logger=log):
         self.root_macaroon_raw = root_macaroon_raw
         self.unbound_discharge_macaroon_raw = unbound_discharge_macaroon_raw
-
-    @classmethod
-    def _makeAuthParam(cls, key, value):
+        self.logger = logger
+
+    def _logMacaroon(self, macaroon_name, macaroon_raw):
+        """Log relevant information from the authorising macaroons.
+
+        This shouldn't be trusted for anything since we can't verify the
+        macaroons here, but it's helpful when debugging.
+        """
+        macaroon = Macaroon.deserialize(macaroon_raw)
+        for caveat in macaroon.first_party_caveats():
+            try:
+                _, key, value = caveat.caveat_id.split("|")
+                if key == "account":
+                    account = json.loads(
+                        base64.b64decode(value).decode("UTF-8"))
+                    if "openid" in account:
+                        self.logger.debug(
+                            "%s macaroon: OpenID identifier: %s" %
+                            (macaroon_name, account["openid"]))
+                elif key == "package_id":
+                    self.logger.debug(
+                        "%s macaroon: snap-ids: %s" % (macaroon_name, value))
+            except ValueError:
+                pass
+
+    def _makeAuthParam(self, key, value):
         # Check framing.
-        if not set(key).issubset(cls.allowed_chars):
+        if not set(key).issubset(self.allowed_chars):
             raise InvalidStoreSecretsError(
                 "Key contains unsafe characters: %r" % key)
-        if not set(value).issubset(cls.allowed_chars):
+        if not set(value).issubset(self.allowed_chars):
             # Don't include secrets in exception arguments.
             raise InvalidStoreSecretsError("Value contains unsafe characters")
+        self._logMacaroon(key, value)
         return '%s="%s"' % (key, value)
 
     @property

=== modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
--- lib/lp/snappy/tests/test_snapstoreclient.py	2017-04-18 17:54:03 +0000
+++ lib/lp/snappy/tests/test_snapstoreclient.py	2017-06-10 20:12:07 +0000
@@ -7,6 +7,7 @@
 
 __metaclass__ = type
 
+import base64
 from cgi import FieldStorage
 import hashlib
 import io
@@ -40,6 +41,7 @@
 
 from lp.services.config import config
 from lp.services.features.testing import FeatureFixture
+from lp.services.log.logger import BufferLogger
 from lp.services.memcache.interfaces import IMemcacheClient
 from lp.services.timeline.requesttimeline import get_request_timeline
 from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS
@@ -129,12 +131,41 @@
         # Test _makeAuthParam's behaviour directly in case somebody somehow
         # convinces Macaroon.serialize to emit data that breaks framing.
         self.assertRaises(
-            InvalidStoreSecretsError, MacaroonAuth._makeAuthParam,
+            InvalidStoreSecretsError, MacaroonAuth(None)._makeAuthParam,
             'ev"il', 'good')
         self.assertRaises(
-            InvalidStoreSecretsError, MacaroonAuth._makeAuthParam,
+            InvalidStoreSecretsError, MacaroonAuth(None)._makeAuthParam,
             'good', 'ev"il')
 
+    def test_logging(self):
+        r = Request()
+        root_key = hashlib.sha256("root").hexdigest()
+        root_macaroon = Macaroon(key=root_key)
+        discharge_key = hashlib.sha256("discharge").hexdigest()
+        discharge_caveat_id = '{"secret": "thing"}'
+        root_macaroon.add_third_party_caveat(
+            "sso.example", discharge_key, discharge_caveat_id)
+        root_macaroon.add_first_party_caveat(
+            "store.example|package_id|{}".format(
+                json.dumps(["example-package"])))
+        unbound_discharge_macaroon = Macaroon(
+            location="sso.example", key=discharge_key,
+            identifier=discharge_caveat_id)
+        unbound_discharge_macaroon.add_first_party_caveat(
+            "sso.example|account|{}".format(
+                base64.b64encode(json.dumps({
+                    "openid": "1234567",
+                    "email": "user@xxxxxxxxxxx",
+                    }))))
+        logger = BufferLogger()
+        MacaroonAuth(
+            root_macaroon.serialize(),
+            unbound_discharge_macaroon.serialize(), logger=logger)(r)
+        self.assertEqual(
+            ['DEBUG root macaroon: snap-ids: ["example-package"]',
+             'DEBUG discharge macaroon: OpenID identifier: 1234567'],
+            logger.getLogBuffer().splitlines())
+
 
 class RequestMatches(Matcher):
     """Matches a request with the specified attributes."""