From 7f866e6369b234e67bc3c5ab404680b094a9f825 Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Tue, 6 Aug 2024 12:00:47 +0200 Subject: [PATCH 1/6] test: fakemaster make_master can now setup a FakeSecretStorage --- master/buildbot/test/fake/fakemaster.py | 10 ++++++++++ master/buildbot/test/fake/secrets.py | 9 +++++++++ 2 files changed, 19 insertions(+) diff --git a/master/buildbot/test/fake/fakemaster.py b/master/buildbot/test/fake/fakemaster.py index b3d61f854f62..5b8c64829bbd 100644 --- a/master/buildbot/test/fake/fakemaster.py +++ b/master/buildbot/test/fake/fakemaster.py @@ -13,6 +13,7 @@ # # Copyright Buildbot Team Members +from __future__ import annotations import weakref from unittest import mock @@ -22,6 +23,7 @@ from buildbot import config from buildbot.data.graphql import GraphQLConnector +from buildbot.secrets.manager import SecretManager from buildbot.test import fakedb from buildbot.test.fake import bworkermanager from buildbot.test.fake import endpoint @@ -31,6 +33,7 @@ from buildbot.test.fake import pbmanager from buildbot.test.fake.botmaster import FakeBotMaster from buildbot.test.fake.machine import FakeMachineManager +from buildbot.test.fake.secrets import FakeSecretStorage from buildbot.util import service @@ -136,6 +139,7 @@ def make_master( wantData=False, wantRealReactor=False, wantGraphql=False, + with_secrets: dict | None = None, url=None, **kwargs, ): @@ -171,4 +175,10 @@ def make_master( master.graphql.reconfigServiceWithBuildbotConfig(master.config) except ImportError: pass + if with_secrets is not None: + secret_service = SecretManager() + secret_service.services = [FakeSecretStorage(secretdict=with_secrets)] + # This should be awaited, but no other call to `setServiceParent` are awaited here + secret_service.setServiceParent(master) + return master diff --git a/master/buildbot/test/fake/secrets.py b/master/buildbot/test/fake/secrets.py index b399f46f7ef8..20cfd9533bae 100644 --- a/master/buildbot/test/fake/secrets.py +++ b/master/buildbot/test/fake/secrets.py @@ -1,10 +1,19 @@ +from __future__ import annotations + from buildbot.secrets.providers.base import SecretProviderBase class FakeSecretStorage(SecretProviderBase): name = "SecretsInFake" + def __init__(self, *args, secretdict: dict | None = None, **kwargs): + super().__init__(*args, **kwargs, secretdict=secretdict) + self._setup_secrets(secretdict=secretdict) + def reconfigService(self, secretdict=None): + self._setup_secrets(secretdict=secretdict) + + def _setup_secrets(self, secretdict: dict | None = None): if secretdict is None: secretdict = {} self.allsecrets = secretdict From 3de88aff8f8fc4842f936e29caad55ceea0706de Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Tue, 27 Aug 2024 13:54:59 +0200 Subject: [PATCH 2/6] test: add support to test buildstep summaries --- master/buildbot/test/steps.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/master/buildbot/test/steps.py b/master/buildbot/test/steps.py index 3cc59206b75f..b8ff6f6636a8 100644 --- a/master/buildbot/test/steps.py +++ b/master/buildbot/test/steps.py @@ -679,7 +679,7 @@ def setup_test_build_step(self, want_data=True, want_db=False, want_mq=False): self.worker = worker.FakeWorker(self.master) self.worker.attached(None) - self._steps = [] + self._steps: list[buildstep.BuildStep] = [] self.build = None # expectations @@ -694,6 +694,8 @@ def setup_test_build_step(self, want_data=True, want_db=False, want_mq=False): self._exp_test_result_sets = [] self._exp_test_results = [] self._exp_build_data = {} + self._exp_result_summaries = [] + self._exp_build_result_summaries = [] def tear_down_test_build_step(self): pass @@ -889,6 +891,12 @@ def expect_test_result_sets(self, sets): def expect_test_results(self, results): self._exp_test_results = results + def expect_result_summary(self, *summaries): + self._exp_result_summaries.extend(summaries) + + def expect_build_result_summary(self, *summaries): + self._exp_build_result_summaries.extend(summaries) + def add_run_process_expect_env(self, d): self._master_run_process_expect_env.update(d) @@ -941,6 +949,16 @@ def run_step(self): f"{stepStateString[stepids[0]]!r}", ) + if self._exp_result_summaries and (exp_summary := self._exp_result_summaries.pop(0)): + step_result_summary = yield step.getResultSummary() + self.assertEqual(exp_summary, step_result_summary) + + if self._exp_build_result_summaries and ( + exp_build_summary := self._exp_build_result_summaries.pop(0) + ): + step_build_result_summary = yield step.getBuildResultSummary() + self.assertEqual(exp_build_summary, step_build_result_summary) + properties = self.build.getProperties() for pn, (pv, ps) in self.exp_properties.items(): From 8ddbe8a7121f96f338aa3bf36eb80f2e180935fe Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Tue, 6 Aug 2024 11:59:56 +0200 Subject: [PATCH 3/6] test: setup reproduction for #7833 "Secrets are not obfuscated in Status in case of failure" --- master/buildbot/test/steps.py | 16 +++++++-- .../test/unit/process/test_buildstep.py | 36 ++++++++++++++++++- 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/master/buildbot/test/steps.py b/master/buildbot/test/steps.py index b8ff6f6636a8..64b81091fec6 100644 --- a/master/buildbot/test/steps.py +++ b/master/buildbot/test/steps.py @@ -13,6 +13,8 @@ # # Copyright Buildbot Team Members +from __future__ import annotations + import stat import tarfile from io import BytesIO @@ -656,7 +658,13 @@ class TestBuildStepMixin: @ivar properties: build properties (L{Properties} instance) """ - def setup_test_build_step(self, want_data=True, want_db=False, want_mq=False): + def setup_test_build_step( + self, + want_data=True, + want_db=False, + want_mq=False, + with_secrets: dict | None = None, + ): if not hasattr(self, 'reactor'): raise RuntimeError('Reactor has not yet been setup for step') @@ -666,7 +674,11 @@ def setup_test_build_step(self, want_data=True, want_db=False, want_mq=False): self._expected_commands_popped = 0 self.master = fakemaster.make_master( - self, wantData=want_data, wantDb=want_db, wantMq=want_mq + self, + wantData=want_data, + wantDb=want_db, + wantMq=want_mq, + with_secrets=with_secrets, ) self.patch(runprocess, "create_process", self._patched_create_process) diff --git a/master/buildbot/test/unit/process/test_buildstep.py b/master/buildbot/test/unit/process/test_buildstep.py index 9fd5f743dadc..966b3eafe29c 100644 --- a/master/buildbot/test/unit/process/test_buildstep.py +++ b/master/buildbot/test/unit/process/test_buildstep.py @@ -32,6 +32,7 @@ from buildbot.process import remotecommand from buildbot.process.buildstep import create_step_from_step_or_factory from buildbot.process.locks import get_real_locks_from_accesses +from buildbot.process.properties import Secret from buildbot.process.properties import renderer from buildbot.process.results import ALL_RESULTS from buildbot.process.results import CANCELLED @@ -54,6 +55,7 @@ from buildbot.test.util import config from buildbot.test.util import interfaces from buildbot.util.eventual import eventually +from buildbot.util.twisted import async_to_deferred class NewStyleStep(buildstep.BuildStep): @@ -1220,7 +1222,7 @@ class TestShellMixin( @defer.inlineCallbacks def setUp(self): self.setup_test_reactor() - yield self.setup_test_build_step() + yield self.setup_test_build_step(with_secrets={"s3cr3t": "really_safe_string"}) def tearDown(self): return self.tear_down_test_build_step() @@ -1504,3 +1506,35 @@ def test_getResultSummary(self): self.setup_step(SimpleShellCommand(command=['a', ['b', 'c']])) self.get_nth_step(0).results = SUCCESS self.assertEqual(self.get_nth_step(0).getResultSummary(), {'step': "'a b ...'"}) + + @async_to_deferred + async def test_step_with_secret_success(self): + self.setup_step(SimpleShellCommand(command=["echo", Secret("s3cr3t")])) + self.expect_commands( + ExpectShell( + workdir="wkdir", + command=["echo", "really_safe_string"], + ).exit(0) + ) + self.expect_outcome(result=SUCCESS, state_string="'echo '") + # FIXME: faulty, does not obfuscate secret + faulty_summary = "'echo really_safe_string'" + self.expect_result_summary({'step': faulty_summary}) + self.expect_build_result_summary({'step': faulty_summary}) + await self.run_step() + + @async_to_deferred + async def test_step_with_secret_failure(self): + self.setup_step(SimpleShellCommand(command=["echo", Secret("s3cr3t")])) + self.expect_commands( + ExpectShell( + workdir="wkdir", + command=["echo", "really_safe_string"], + ).exit(1) + ) + self.expect_outcome(result=FAILURE, state_string="'echo ' (failure)") + # FIXME: faulty, does not obfuscate secret + faulty_summary = "'echo really_safe_string' (failure)" + self.expect_result_summary({'step': faulty_summary}) + self.expect_build_result_summary({'step': faulty_summary, 'build': faulty_summary}) + await self.run_step() From e7ad21e920697166ca7b1443e06831a7c655e72d Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Tue, 6 Aug 2024 11:11:40 +0200 Subject: [PATCH 4/6] process: Fix ShellMixin handling of secrets Would log clear secrets, and display them in step description Fix #7833 --- master/buildbot/process/buildstep.py | 5 ++++ .../test/unit/process/test_buildstep.py | 25 +++++++------------ .../buildstep-summary-secret-leak.bugfix | 1 + 3 files changed, 15 insertions(+), 16 deletions(-) create mode 100644 newsfragments/buildstep-summary-secret-leak.bugfix diff --git a/master/buildbot/process/buildstep.py b/master/buildbot/process/buildstep.py index 28b0150f0e78..9deeeae331ff 100644 --- a/master/buildbot/process/buildstep.py +++ b/master/buildbot/process/buildstep.py @@ -409,6 +409,8 @@ def getResultSummary(self): elif self.max_lines_reached: stepsumm += " (max lines reached)" + if self.build is not None: + stepsumm = self.build.properties.cleanupTextFromSecrets(stepsumm) return {'step': stepsumm} @defer.inlineCallbacks @@ -1039,6 +1041,9 @@ def getResultSummary(self): summary += " (timed out)" elif self.max_lines_reached: summary += " (max lines)" + + if self.build is not None: + summary = self.build.properties.cleanupTextFromSecrets(summary) return {'step': summary} return super().getResultSummary() diff --git a/master/buildbot/test/unit/process/test_buildstep.py b/master/buildbot/test/unit/process/test_buildstep.py index 966b3eafe29c..b476b684503b 100644 --- a/master/buildbot/test/unit/process/test_buildstep.py +++ b/master/buildbot/test/unit/process/test_buildstep.py @@ -949,6 +949,7 @@ def testRunRaisesException(self): step.master.reactor = self.reactor step.build = mock.Mock() step.build._locks_to_acquire = [] + step.build.properties.cleanupTextFromSecrets = lambda s: s step.build.builder.botmaster.getLockFromLockAccesses = mock.Mock(return_value=[]) step.locks = [] step.renderables = [] @@ -1511,30 +1512,22 @@ def test_getResultSummary(self): async def test_step_with_secret_success(self): self.setup_step(SimpleShellCommand(command=["echo", Secret("s3cr3t")])) self.expect_commands( - ExpectShell( - workdir="wkdir", - command=["echo", "really_safe_string"], - ).exit(0) + ExpectShell(workdir="wkdir", command=["echo", 'really_safe_string']).exit(0) ) self.expect_outcome(result=SUCCESS, state_string="'echo '") - # FIXME: faulty, does not obfuscate secret - faulty_summary = "'echo really_safe_string'" - self.expect_result_summary({'step': faulty_summary}) - self.expect_build_result_summary({'step': faulty_summary}) + summary = "'echo '" + self.expect_result_summary({'step': summary}) + self.expect_build_result_summary({'step': summary}) await self.run_step() @async_to_deferred async def test_step_with_secret_failure(self): self.setup_step(SimpleShellCommand(command=["echo", Secret("s3cr3t")])) self.expect_commands( - ExpectShell( - workdir="wkdir", - command=["echo", "really_safe_string"], - ).exit(1) + ExpectShell(workdir="wkdir", command=["echo", 'really_safe_string']).exit(1) ) self.expect_outcome(result=FAILURE, state_string="'echo ' (failure)") - # FIXME: faulty, does not obfuscate secret - faulty_summary = "'echo really_safe_string' (failure)" - self.expect_result_summary({'step': faulty_summary}) - self.expect_build_result_summary({'step': faulty_summary, 'build': faulty_summary}) + summary = "'echo ' (failure)" + self.expect_result_summary({'step': summary}) + self.expect_build_result_summary({'step': summary, 'build': summary}) await self.run_step() diff --git a/newsfragments/buildstep-summary-secret-leak.bugfix b/newsfragments/buildstep-summary-secret-leak.bugfix new file mode 100644 index 000000000000..543db3e7cc06 --- /dev/null +++ b/newsfragments/buildstep-summary-secret-leak.bugfix @@ -0,0 +1 @@ +Fix ``Build`` summary containing non obfuscated ``Secret`` values present in a failed ``BuildStep`` summary (:issue:`7833`) From 59c3ccd0f69e9d2b5ed72310c85158c73ef9e6af Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Tue, 6 Aug 2024 11:09:56 +0200 Subject: [PATCH 5/6] util: command_to_string do not remove instances, use their repr --- master/buildbot/util/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/master/buildbot/util/__init__.py b/master/buildbot/util/__init__.py index 8f73415d40ac..90c045b1a231 100644 --- a/master/buildbot/util/__init__.py +++ b/master/buildbot/util/__init__.py @@ -454,8 +454,6 @@ def command_to_string(command): # flatten any nested lists words = flatten(words, (list, tuple)) - # strip instances and other detritus (which can happen if a - # description is requested before rendering) stringWords = [] for w in words: if isinstance(w, (bytes, str)): @@ -463,6 +461,8 @@ def command_to_string(command): # trying to covert it. w = bytes2unicode(w, errors="replace") stringWords.append(w) + else: + stringWords.append(repr(w)) words = stringWords if not words: From a8212bb2be2cf223baca6d716ff6f63d71bc8f7b Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Tue, 6 Aug 2024 16:41:25 +0200 Subject: [PATCH 6/6] worker: use command with resolved obfuscation as command_id --- worker/buildbot_worker/runprocess.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/worker/buildbot_worker/runprocess.py b/worker/buildbot_worker/runprocess.py index e111ff77b9fc..828e0c68aa81 100644 --- a/worker/buildbot_worker/runprocess.py +++ b/worker/buildbot_worker/runprocess.py @@ -308,8 +308,6 @@ def __init__( @param useProcGroup: (default True) use a process group for non-PTY process invocations """ - self.command_id = command - if logfiles is None: logfiles = {} @@ -321,6 +319,9 @@ def obfus(w): return w command = [obfus(w) for w in command] + + self.command_id = command + # We need to take unicode commands and arguments and encode them using # the appropriate encoding for the worker. This is mostly platform # specific, but can be overridden in the worker's buildbot.tac file.