Merge pull request buildbot#7968 from tdesveaux/issue/7833

Fix buildbot#7833: Secrets are not obfuscated in "Status" in case of failure

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()
 

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

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

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)
@@ -679,7 +691,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 +706,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 +903,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 +961,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():

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):
@@ -947,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 = []
@@ -1220,7 +1223,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 +1507,27 @@ 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 <s3cr3t>'")
+        summary = "'echo <s3cr3t>'"
+        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)
+        )
+        self.expect_outcome(result=FAILURE, state_string="'echo <s3cr3t>' (failure)")
+        summary = "'echo <s3cr3t>' (failure)"
+        self.expect_result_summary({'step': summary})
+        self.expect_build_result_summary({'step': summary, 'build': summary})
+        await self.run_step()

master/buildbot/util/__init__.py

@@ -454,15 +454,15 @@ 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)):
             # If command was bytes, be gentle in
             # trying to covert it.
             w = bytes2unicode(w, errors="replace")
             stringWords.append(w)
+        else:
+            stringWords.append(repr(w))
     words = stringWords
 
     if not words:

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`)

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.