Address review comments and improve auth

Auth now chooses between cert (#1) and bearer token (#2)
instead of sending both. Also renamed ssl --> tls for correctness

Author: hkaj

checks.d/kubernetes.py

@@ -103,7 +103,7 @@ def _perform_kubelet_checks(self, url):
         service_check_base = NAMESPACE + '.kubelet.check'
         is_ok = True
         try:
-            req = self.kubeutil.retrieve_kubelet_url(url)
+            req = self.kubeutil.perform_kubelet_query(url)
             for line in req.iter_lines():
 
                 # avoid noise; this check is expected to fail since we override the container hostname

conf.d/kubernetes.yaml.example

@@ -66,29 +66,32 @@ instances:
   # kubelet port. It needs to be set if you are not using a default one (10250 or 10255)
   # kubelet_port: 10255
   #
-  # SSL configuration for querying kubelet.
+  # TLS configuration for querying kubelet.
   #
   # Available options are:
-  # - simple SSL validation (with readily available certificates)
-  # - disabling SSL validation
+  # (server auth)
+  # - simple TLS validation (with readily available certificates)
+  # - disabling TLS validation
   # - providing the server certificate
+  # (client auth)
   # - providing a client cert/key pair
+  # - using a service account bearer token
   #
-  # The default is to try and use the read-only port that doesn't require SSL
-  # And to fall back to the HTTPS API with simple SSL validation.
+  # The default is to try and use the read-only port that doesn't require TLS
+  # And to fall back to the HTTPS API with simple TLS validation.
   #
-  # Explicitly disabling ssl_verify should be used with caution:
+  # Explicitly disabling tls_verify should be used with caution:
   # if an attacker sniffs the agent requests they will see the agent's service account bearer token.
   #
-  # Providing a server cert enables SSL validation.
+  # Providing a server cert enables TLS validation.
   #
   # It is possible to providing a client cert/key pair for client auth.
-  # The agent also uses its pod bearer token (if available) when querying kubelet and the apiserver.
+  # If no such cert is passed, the agent authenticates using its pod bearer token (if available) when querying kubelet and the apiserver.
   #
   # The recommended way to pass certificates and keys to the agent is by using
   # "Secret" Kubernetes object (and to mount them as volumes).
   #
-  # kubelet_ssl_verify: True
+  # kubelet_tls_verify: True
   # kubelet_cert: /path/to/ca.pem
   # kubelet_client_crt: /path/to/client.crt
   # kubelet_client_key: /path/to/client.key

tests/checks/mock/test_kubernetes.py

@@ -385,22 +385,31 @@ class TestKubeutil(unittest.TestCase):
     def setUp(self, _locate_kubelet):
         self.kubeutil = KubeUtil()
 
-    def test_init_ssl_settings(self):
+    @mock.patch('os.path.exists', return_value=True)
+    @mock.patch('utils.kubernetes.kubeutil.KubeUtil.get_auth_token', return_value=None)
+    def test_init_tls_settings(self, *args):
+        # kubelet
         instances = [
             # (instance, expected_result)
-            ({}, {'verify': True}),
-            ({'kubelet_ssl_verify': False}, {'verify': False}),
-            ({'kubelet_ssl_verify': True}, {'verify': True}),
-            ({'kubelet_ssl_verify': 'foo.pem'}, {'verify': 'foo.pem'}),
-            ({'kubelet_cert': 'foo.pem'}, {'verify': 'foo.pem'}),
+            ({}, {'kubelet_verify': True}),
+            ({'kubelet_tls_verify': False}, {'kubelet_verify': False}),
+            ({'kubelet_tls_verify': True}, {'kubelet_verify': True}),
+            ({'kubelet_tls_verify': 'foo.pem'}, {'kubelet_verify': 'foo.pem'}),
+            ({'kubelet_cert': 'foo.pem'}, {'kubelet_verify': 'foo.pem'}),
             ({'kubelet_client_crt': 'client.crt', 'kubelet_client_key': 'client.key'},
-                {'verify': True, 'kubelet_client_cert': ('client.crt', 'client.key')}),
-            ({'kubelet_ssl_verify': True, 'kubelet_client_crt': 'client.crt'}, {'verify': True}),
-            ({'kubelet_client_crt': 'client.crt'}, {'verify': True})
+                {'kubelet_verify': True, 'kubelet_client_cert': ('client.crt', 'client.key')}),
+            ({'kubelet_tls_verify': True, 'kubelet_client_crt': 'client.crt'}, {'kubelet_verify': True}),
+            ({'kubelet_client_crt': 'client.crt'}, {'kubelet_verify': True})
         ]
         for instance, result in instances:
-            self.assertEqual(self.kubeutil._init_ssl_settings(instance), result)
+            self.assertEqual(self.kubeutil._init_tls_settings(instance), result)
 
+        # apiserver
+        instance = {'apiserver_client_crt': 'foo.crt', 'apiserver_client_key': 'foo.key'}
+        expected_res = {'apiserver_client_cert': ('foo.crt', 'foo.key'), 'kubelet_verify': True}
+        self.assertEqual(self.kubeutil._init_tls_settings(instance), expected_res)
+        with mock.patch('utils.kubernetes.kubeutil.os.path.exists', return_value=False):
+            self.assertEqual(self.kubeutil._init_tls_settings(instance), {'kubelet_verify': True})
 
     ##### Test _locate_kubelet #####
 
@@ -419,7 +428,7 @@ def test_locate_kubelet_no_auth_no_ssl(self, _get_hostname):
             ({'kubelet_port': '1337'}, 'http://test_docker_host:1337'),
             ({'host': 'test_explicit_host', 'kubelet_port': '1337'}, 'http://test_explicit_host:1337')
         ]
-        with mock.patch('utils.kubernetes.kubeutil.KubeUtil.retrieve_kubelet_url', return_value=True):
+        with mock.patch('utils.kubernetes.kubeutil.KubeUtil.perform_kubelet_query', return_value=True):
             for instance, result in no_auth_no_ssl_instances:
                 self.assertEqual(self.kubeutil._locate_kubelet(instance), result)
 
@@ -434,13 +443,13 @@ def test_locate_kubelet_no_auth_no_verify(self, _get_hostname):
         ]
 
         def side_effect(url):
-            """Mock KubeUtil.retrieve_kubelet_url"""
+            """Mock KubeUtil.perform_kubelet_query"""
             if url.startswith('https://'):
                 return True
             else:
                 raise Exception()
 
-        with mock.patch('utils.kubernetes.kubeutil.KubeUtil.retrieve_kubelet_url', side_effect=side_effect):
+        with mock.patch('utils.kubernetes.kubeutil.KubeUtil.perform_kubelet_query', side_effect=side_effect):
             for instance, result in no_auth_no_verify_instances:
                 self.assertEqual(self.kubeutil._locate_kubelet(instance), result)
 
@@ -449,85 +458,65 @@ def side_effect(url):
     @mock.patch('utils.kubernetes.kubeutil.KubeUtil.get_auth_token', return_value='foo')
     def test_locate_kubelet_verify_and_auth(self, *args):
         """
-        Test kubelet connection with SSL. Also look for auth token.
+        Test kubelet connection with TLS. Also look for auth token.
         """
         no_auth_instances = [
-            # instance, ssl_settings, expected_result
-            ({}, {'verify': True}, 'https://test_k8s_host:10250'),
-            ({'kubelet_port': '1337'}, {'verify': 'test.pem'}, 'https://test_k8s_host:1337'),
+            # instance, tls_settings, expected_result
+            ({}, {'kubelet_verify': True}, 'https://test_k8s_host:10250'),
+            ({'kubelet_port': '1337'}, {'kubelet_verify': 'test.pem'}, 'https://test_k8s_host:1337'),
             (
                 {'host': 'test_explicit_host'},
-                {'verify': True, 'kubelet_client_cert': ('client.crt', 'client.key')},
+                {'kubelet_verify': True, 'kubelet_client_cert': ('client.crt', 'client.key')},
                 'https://test_explicit_host:10250'
             ),
             (
                 {'host': 'test_explicit_host', 'kubelet_port': '1337'},
-                {'verify': True},
+                {'kubelet_verify': True},
                 'https://test_explicit_host:1337'
             ),
         ]
 
         def side_effect(url, **kwargs):
-            """Mock KubeUtil.retrieve_kubelet_url"""
+            """Mock KubeUtil.perform_kubelet_query"""
             if url.startswith('https://') and '10255' not in url:
                 return True
             else:
                 raise Exception()
 
-        # no auth / SSL with verify
-        for instance, ssl_settings, result in no_auth_instances:
+        # no auth / TLS with verify
+        for instance, tls_settings, result in no_auth_instances:
             with mock.patch('utils.kubernetes.kubeutil.requests') as req:
                 req.get = mock.MagicMock(side_effect=side_effect)
-                self.kubeutil.ssl_settings = ssl_settings
+                self.kubeutil.tls_settings = tls_settings
                 self.assertEqual(self.kubeutil._locate_kubelet(instance), result)
                 req.get.assert_called_with(result + '/healthz',  # test endpoint
                     timeout=10,
-                    verify=ssl_settings.get('verify', False),
-                    cert=ssl_settings.get('kubelet_client_cert'),
-                    headers={'Authorization': 'Bearer foo'},  # auth
+                    verify=tls_settings.get('kubelet_verify', True),
+                    headers={'Authorization': 'Bearer foo'} if 'kubelet_client_cert' not in tls_settings else None,
+                    cert=tls_settings.get('kubelet_client_cert'),
                     params={'verbose': True}
                 )
 
     @mock.patch('utils.kubernetes.kubeutil.KubeUtil.get_auth_token', return_value='foo')
     def test_get_node_hostname(self, _get_auth_tkn):
         node_lists = [
             (json.loads(Fixtures.read_file('filtered_node_list_1_4.json', string_escape=False)), 'ip-10-0-0-179'),
-            ({'items': [{'foo': 'bar'}]}, None)
-        ]
-
-        exception_node_lists = [
-            {'items': []},
-            {'items': [{'foo': 'bar'}, {'bar': 'foo'}]}
+            ({'items': [{'foo': 'bar'}]}, None),
+            ({'items': []}, None),
+            ({'items': [{'foo': 'bar'}, {'bar': 'foo'}]}, None)
         ]
 
         for node_list, expected_result in node_lists:
             with mock.patch('utils.kubernetes.kubeutil.KubeUtil.retrieve_json_auth', return_value=node_list):
                 self.assertEqual(self.kubeutil.get_node_hostname('ip-10-0-0-179'), expected_result)
 
-        for node_list in exception_node_lists:
-            with mock.patch('utils.kubernetes.kubeutil.KubeUtil.retrieve_json_auth', return_value=node_list):
-                self.assertRaises(Exception, self.kubeutil.get_node_hostname, 'ip-10-0-0-179')
-
     @mock.patch('utils.kubernetes.KubeUtil.retrieve_pods_list', side_effect=['foo'])
     @mock.patch('utils.kubernetes.KubeUtil.extract_kube_labels')
     def test_get_kube_labels(self, extract_kube_labels, retrieve_pods_list):
         self.kubeutil.get_kube_labels(excluded_keys='bar')
         retrieve_pods_list.assert_called_once()
         extract_kube_labels.assert_called_once_with('foo', excluded_keys='bar')
 
-    @mock.patch('utils.kubernetes.kubeutil.KubeUtil.get_auth_token', return_value=None)
-    def test_init_tls_settings(self, get_tkn):
-        self.assertEqual(self.kubeutil._init_tls_settings({}), {})
-        self.assertEqual(self.kubeutil._init_tls_settings({'apiserver_client_crt': 'foo.crt'}), {})
-
-        instance = {'apiserver_client_crt': 'foo.crt', 'apiserver_client_key': 'foo.key'}
-        with mock.patch('utils.kubernetes.kubeutil.os.path.exists', return_value=True):
-            expected_res = {'apiserver_client_cert': ('foo.crt', 'foo.key')}
-            self.assertEqual(self.kubeutil._init_tls_settings(instance), expected_res)
-
-        with mock.patch('utils.kubernetes.kubeutil.os.path.exists', return_value=False):
-            self.assertEqual(self.kubeutil._init_tls_settings(instance), {})
-
     def test_extract_kube_labels(self):
         """
         Test with both 1.1 and 1.2 version payloads
@@ -551,7 +540,7 @@ def test_extract_kube_labels(self):
         labels = set(inn for out in res.values() for inn in out)
         self.assertEqual(len(labels), 3)
 
-    @mock.patch('utils.kubernetes.kubeutil.KubeUtil.retrieve_kubelet_url')
+    @mock.patch('utils.kubernetes.kubeutil.KubeUtil.perform_kubelet_query')
     def test_retrieve_pods_list(self, retrieve_url):
         self.kubeutil.retrieve_pods_list()
         retrieve_url.assert_called_twice_with(self.kubeutil.pods_list_url, verbose=True, timeout=10)
@@ -568,7 +557,7 @@ def test_retrieve_metrics(self, retrieve_json):
 
     @mock.patch('utils.kubernetes.kubeutil.KubeUtil.get_auth_token', return_value='foo')
     @mock.patch('utils.kubernetes.kubeutil.requests')
-    def test_retrieve_kubelet_url(self, req, _get_auth_tkn):
+    def test_perform_kubelet_query(self, req, _get_auth_tkn):
         base_params = {'timeout': 10, 'verify': False,
             'params': {'verbose': True}, 'cert': None, 'headers': None}
 
@@ -580,15 +569,15 @@ def test_retrieve_kubelet_url(self, req, _get_auth_tkn):
         instances = [
             ('http://test.com', {}, dict(base_params.items() + verify_true.items())),
             ('https://test.com', {}, dict(base_params.items() + verify_true.items() + auth_token_header.items())),
-            ('https://test.com', {'verify': True}, dict(base_params.items() + verify_true.items() + auth_token_header.items())),
-            ('https://test.com', {'verify': 'kubelet.pem'}, dict(base_params.items() + verify_cert.items() + auth_token_header.items())),
+            ('https://test.com', {'kubelet_verify': True}, dict(base_params.items() + verify_true.items() + auth_token_header.items())),
+            ('https://test.com', {'kubelet_verify': 'kubelet.pem'}, dict(base_params.items() + verify_cert.items() + auth_token_header.items())),
             ('https://test.com', {'kubelet_client_cert': ('client.crt', 'client.key')},
-                dict(base_params.items() + verify_true.items() + client_cert.items() + auth_token_header.items())),
+                dict(base_params.items() + verify_true.items() + client_cert.items())),
         ]
         for url, ssl_context, expected_params in instances:
             req.get.reset_mock()
-            self.kubeutil.ssl_settings = ssl_context
-            self.kubeutil.retrieve_kubelet_url(url)
+            self.kubeutil.tls_settings = ssl_context
+            self.kubeutil.perform_kubelet_query(url)
             req.get.assert_called_with(url, **expected_params)
 
     @mock.patch('utils.kubernetes.kubeutil.requests')