launchpad-reviewers team mailing list archive

[Ines Almeida <mp+465326@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/launchpad:fetch-service-redact-certificate-in-logs into launchpad:master.

Commit message:
Redact `fetch_service_mitm_certificate` in build logs
    
When running a fetch service build, we send the certficate from buildd-manager to buildd, and log it. This redacts the certificate.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/465326
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:fetch-service-redact-certificate-in-logs into launchpad:master.

diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
index e8d82d2..4dd258a 100644
--- a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
@@ -12,6 +12,7 @@ import logging
 import os
 import tempfile
 from collections import OrderedDict
+from copy import deepcopy
 from datetime import datetime
 
 import transaction
@@ -126,6 +127,14 @@ class BuildFarmJobBehaviourBase:
 
     def redactXmlrpcArguments(self, args):
         # we do not want to have secrets in logs
+
+        # we need to copy the input in order to avoid mutating `args` which
+        # will be passed to the builders
+        args = deepcopy(args)
+        if args["args"].get("secrets"):
+            for key in args["args"]["secrets"].keys():
+                args["args"]["secrets"][key] = "<redacted>"
+
         return sanitise_urls(repr(args))
 
     @defer.inlineCallbacks
diff --git a/lib/lp/code/model/cibuildbehaviour.py b/lib/lp/code/model/cibuildbehaviour.py
index 6cceb72..ba73848 100644
--- a/lib/lp/code/model/cibuildbehaviour.py
+++ b/lib/lp/code/model/cibuildbehaviour.py
@@ -9,7 +9,6 @@ __all__ = [
 
 import json
 from configparser import NoSectionError
-from copy import deepcopy
 from typing import Any, Generator
 
 from twisted.internet import defer
@@ -119,17 +118,6 @@ class CIBuildBehaviour(BuilderProxyMixin, BuildFarmJobBehaviourBase):
 
     ALLOWED_STATUS_NOTIFICATIONS = ["PACKAGEFAIL"]
 
-    def redactXmlrpcArguments(self, args):
-        # we do not want to have secrets in logs
-
-        # we need to copy the input in order to avoid mutating `args` which
-        # will be passed to the builders
-        args = deepcopy(args)
-        if args["args"].get("secrets"):
-            for key in args["args"]["secrets"].keys():
-                args["args"]["secrets"][key] = "<redacted>"
-        return super().redactXmlrpcArguments(args)
-
     def getLogFileName(self):
         return "buildlog_ci_%s_%s_%s.txt" % (
             self.build.git_repository.name,
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index cb2269b..66fb638 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -419,6 +419,45 @@ class TestAsyncSnapBuildBehaviourFetchService(
         self.assertEqual([], self.fetch_service_api.sessions.requests)
 
     @defer.inlineCallbacks
+    def test_requestFetchServiceSession_mitm_certficate_redacted(self):
+        """The `fetch_service_mitm_certificate` field in the build arguments
+        is redacted in the build logs."""
+
+        self.useFixture(
+            FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
+        )
+        snap = self.factory.makeSnap(use_fetch_service=True)
+        request = self.factory.makeSnapBuildRequest(snap=snap)
+        job = self.makeJob(snap=snap, build_request=request)
+        args = yield job.extraBuildArgs()
+
+        chroot_lfa = self.factory.makeLibraryFileAlias(db_only=True)
+        job.build.distro_arch_series.addOrUpdateChroot(
+            chroot_lfa, image_type=BuildBaseImageType.CHROOT
+        )
+        lxd_lfa = self.factory.makeLibraryFileAlias(db_only=True)
+        job.build.distro_arch_series.addOrUpdateChroot(
+            lxd_lfa, image_type=BuildBaseImageType.LXD
+        )
+        deferred = defer.Deferred()
+        deferred.callback(None)
+        job._worker.sendFileToWorker = MagicMock(return_value=deferred)
+        job._worker.build = MagicMock(return_value=(None, None))
+
+        logger = BufferLogger()
+        yield job.dispatchBuildToWorker(logger)
+
+        # Secrets exist within the arguments
+        self.assertIn(
+            "fake-cert", args["secrets"]["fetch_service_mitm_certificate"]
+        )
+        # But are redacted in the log output
+        self.assertIn(
+            "'fetch_service_mitm_certificate': '<redacted>'",
+            logger.getLogBuffer(),
+        )
+
+    @defer.inlineCallbacks
     def test_endProxySession(self):
         """By ending a fetch service session, metadata is retrieved from the
         fetch service and saved to a file; and call to end the session is made.