From 9c496e2939c470452d9868ff325d3c07d2babbe8 Mon Sep 17 00:00:00 2001
From: "James E. Blair" <jim@acmegating.com>
Date: Fri, 1 Dec 2023 10:27:06 -0800
Subject: [PATCH] Redact k8s connection info in node list

The node list (web and cli) displays the connection port for the
node, but the k8s drivers use that to send service account
credential info to zuul.

To avoid exposing this to users if operators have chosen to make
the nodepool-launcher webserver accessible, redact the connection
port if it is not an integer.

This also affects the command-line nodepool-list in the same way.

Change-Id: I7a309f95417d47612e40d983b3a2ec6ee4d0183a
---
 nodepool/status.py                            | 10 ++++-
 nodepool/tests/unit/test_driver_kubernetes.py | 39 +++++++++++++++++++
 nodepool/tests/unit/test_webapp.py            |  2 +
 3 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/nodepool/status.py b/nodepool/status.py
index bb4b1cde9..d7d1d0348 100644
--- a/nodepool/status.py
+++ b/nodepool/status.py
@@ -142,6 +142,14 @@ def node_list(zk, node_id=None):
         else:
             if zk.getNodeLockContenders(node):
                 locked = "locked"
+        port = node.connection_port
+        try:
+            int(port)
+        except (ValueError, TypeError):
+            # The port field is being used to carry connection
+            # information which may contain credentials (e.g., k8s
+            # service account).  Suppress it.
+            port = "redacted"
         values = [
             node.id,
             node.provider,
@@ -157,7 +165,7 @@ def node_list(zk, node_id=None):
             node.private_ipv4,
             node.az,
             node.username,
-            node.connection_port,
+            port,
             node.launcher,
             node.allocated_to,
             node.hold_job,
diff --git a/nodepool/tests/unit/test_driver_kubernetes.py b/nodepool/tests/unit/test_driver_kubernetes.py
index 5b4919a1a..6170d8e34 100644
--- a/nodepool/tests/unit/test_driver_kubernetes.py
+++ b/nodepool/tests/unit/test_driver_kubernetes.py
@@ -15,8 +15,10 @@
 # limitations under the License.
 
 import fixtures
+import json
 import logging
 import time
+from urllib import request
 
 from nodepool import tests
 from nodepool.zk import zookeeper as zk
@@ -572,3 +574,40 @@ class TestDriverKubernetes(tests.DBTestCase):
 
         servers = manager.listNodes()
         self.assertEqual(len(servers), 1)
+
+    def test_node_list_k8s_json(self):
+        # TODO: generalize the k8s test infrastructure (fake client,
+        # etc) and move this to test_webapp
+        configfile = self.setup_config('kubernetes.yaml')
+        pool = self.useNodepool(configfile, watermark_sleep=1)
+        self.startPool(pool)
+        webapp = self.useWebApp(pool, port=0)
+        webapp.start()
+        port = webapp.server.socket.getsockname()[1]
+
+        req = zk.NodeRequest()
+        req.state = zk.REQUESTED
+        req.node_types.append('kubernetes-namespace')
+        self.zk.storeNodeRequest(req)
+
+        self.log.debug("Waiting for request %s", req.id)
+        req = self.waitForNodeRequest(req)
+        self.assertEqual(req.state, zk.FULFILLED)
+
+        req = request.Request(
+            "http://localhost:%s/node-list" % port)
+        req.add_header('Accept', 'application/json')
+        f = request.urlopen(req)
+        self.assertEqual(f.info().get('Content-Type'),
+                         'application/json')
+        data = f.read()
+        objs = json.loads(data.decode('utf8'))
+        # We don't check the value of 'locked' because we may get a
+        # cached value returned.
+        self.assertDictContainsSubset({'id': '0000000000',
+                                       'ipv6': None,
+                                       'label': ['kubernetes-namespace'],
+                                       'provider': 'kubespray',
+                                       'public_ipv4': None,
+                                       'connection_port': 'redacted',
+                                       'state': 'ready'}, objs[0])
diff --git a/nodepool/tests/unit/test_webapp.py b/nodepool/tests/unit/test_webapp.py
index b7aa47f30..1e0b4dc70 100644
--- a/nodepool/tests/unit/test_webapp.py
+++ b/nodepool/tests/unit/test_webapp.py
@@ -232,6 +232,7 @@ class TestWebApp(tests.DBTestCase):
                                        'label': ['fake-label'],
                                        'provider': 'fake-provider',
                                        'public_ipv4': 'fake',
+                                       'connection_port': 22,
                                        'state': 'ready'}, objs[0])
         self.assertTrue('locked' in objs[0])
         # specify valid node_id
@@ -251,6 +252,7 @@ class TestWebApp(tests.DBTestCase):
                                        'label': ['fake-label'],
                                        'provider': 'fake-provider',
                                        'public_ipv4': 'fake',
+                                       'connection_port': 22,
                                        'state': 'ready'}, objs[0])
         self.assertTrue('locked' in objs[0])
         # node_id not found