Launchpad itself

Merge ~ines-almeida/launchpad:fetch-service-redact-certificate-in-logs into launchpad:master

  1. Git
  2. lp:~ines-almeida/launchpad
  3. fetch-service-redact-certificate-in-logs
  4. Merge into master
Proposed by Ines Almeida
Status: Merged
Approved by: Ines Almeida
Approved revision: 210d82e87fdd7f39177b75f7436d8f991197dfa2
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~ines-almeida/launchpad:fetch-service-redact-certificate-in-logs
Merge into: launchpad:master
Diff against target: 107 lines (+48/-12)
3 files modified
lib/lp/buildmaster/model/buildfarmjobbehaviour.py (+9/-0)
lib/lp/code/model/cibuildbehaviour.py (+0/-12)
lib/lp/snappy/tests/test_snapbuildbehaviour.py (+39/-0)
Reviewer Review Type Date Requested Status
Jürgen Gmach Approve
Review via email: mp+465326@code.launchpad.net

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.

Description of the change

Tests from CIBuilds that tested the redacted secrets still ran successfully.

No other build type has `secrets` in their args except for snap builds and ci builds, but IMO anything that goes under a "secrets" keywords should be redacted - so I updated it within the parent `redactXmlrpcArguments` method.

To post a comment you must log in.
Revision history for this message
Jürgen Gmach (jugmac00) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
2index e8d82d2..4dd258a 100644
3--- a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
4+++ b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
5@@ -12,6 +12,7 @@ import logging
6 import os
7 import tempfile
8 from collections import OrderedDict
9+from copy import deepcopy
10 from datetime import datetime
11
12 import transaction
13@@ -126,6 +127,14 @@ class BuildFarmJobBehaviourBase:
14
15 def redactXmlrpcArguments(self, args):
16 # we do not want to have secrets in logs
17+
18+ # we need to copy the input in order to avoid mutating `args` which
19+ # will be passed to the builders
20+ args = deepcopy(args)
21+ if args["args"].get("secrets"):
22+ for key in args["args"]["secrets"].keys():
23+ args["args"]["secrets"][key] = "<redacted>"
24+
25 return sanitise_urls(repr(args))
26
27 @defer.inlineCallbacks
28diff --git a/lib/lp/code/model/cibuildbehaviour.py b/lib/lp/code/model/cibuildbehaviour.py
29index 6cceb72..ba73848 100644
30--- a/lib/lp/code/model/cibuildbehaviour.py
31+++ b/lib/lp/code/model/cibuildbehaviour.py
32@@ -9,7 +9,6 @@ __all__ = [
33
34 import json
35 from configparser import NoSectionError
36-from copy import deepcopy
37 from typing import Any, Generator
38
39 from twisted.internet import defer
40@@ -119,17 +118,6 @@ class CIBuildBehaviour(BuilderProxyMixin, BuildFarmJobBehaviourBase):
41
42 ALLOWED_STATUS_NOTIFICATIONS = ["PACKAGEFAIL"]
43
44- def redactXmlrpcArguments(self, args):
45- # we do not want to have secrets in logs
46-
47- # we need to copy the input in order to avoid mutating `args` which
48- # will be passed to the builders
49- args = deepcopy(args)
50- if args["args"].get("secrets"):
51- for key in args["args"]["secrets"].keys():
52- args["args"]["secrets"][key] = "<redacted>"
53- return super().redactXmlrpcArguments(args)
54-
55 def getLogFileName(self):
56 return "buildlog_ci_%s_%s_%s.txt" % (
57 self.build.git_repository.name,
58diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
59index cb2269b..66fb638 100644
60--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
61+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
62@@ -419,6 +419,45 @@ class TestAsyncSnapBuildBehaviourFetchService(
63 self.assertEqual([], self.fetch_service_api.sessions.requests)
64
65 @defer.inlineCallbacks
66+ def test_requestFetchServiceSession_mitm_certficate_redacted(self):
67+ """The `fetch_service_mitm_certificate` field in the build arguments
68+ is redacted in the build logs."""
69+
70+ self.useFixture(
71+ FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
72+ )
73+ snap = self.factory.makeSnap(use_fetch_service=True)
74+ request = self.factory.makeSnapBuildRequest(snap=snap)
75+ job = self.makeJob(snap=snap, build_request=request)
76+ args = yield job.extraBuildArgs()
77+
78+ chroot_lfa = self.factory.makeLibraryFileAlias(db_only=True)
79+ job.build.distro_arch_series.addOrUpdateChroot(
80+ chroot_lfa, image_type=BuildBaseImageType.CHROOT
81+ )
82+ lxd_lfa = self.factory.makeLibraryFileAlias(db_only=True)
83+ job.build.distro_arch_series.addOrUpdateChroot(
84+ lxd_lfa, image_type=BuildBaseImageType.LXD
85+ )
86+ deferred = defer.Deferred()
87+ deferred.callback(None)
88+ job._worker.sendFileToWorker = MagicMock(return_value=deferred)
89+ job._worker.build = MagicMock(return_value=(None, None))
90+
91+ logger = BufferLogger()
92+ yield job.dispatchBuildToWorker(logger)
93+
94+ # Secrets exist within the arguments
95+ self.assertIn(
96+ "fake-cert", args["secrets"]["fetch_service_mitm_certificate"]
97+ )
98+ # But are redacted in the log output
99+ self.assertIn(
100+ "'fetch_service_mitm_certificate': '<redacted>'",
101+ logger.getLogBuffer(),
102+ )
103+
104+ @defer.inlineCallbacks
105 def test_endProxySession(self):
106 """By ending a fetch service session, metadata is retrieved from the
107 fetch service and saved to a file; and call to end the session is made.

Subscribers

People subscribed via source and target branches

to status/vote changes: