Address review comments and improve auth

hkaj · hkaj · commit 242b9a82b75b · 2017-02-07T13:00:48.000Z
Auth now chooses between cert (#1) and bearer token (#2) instead of sending both. Also renamed ssl --> tls for correctness
diff --git a/checks.d/kubernetes.py b/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
diff --git a/conf.d/kubernetes.yaml.example b/conf.d/kubernetes.yaml.example
@@ -69,29 +69,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
diff --git a/tests/checks/mock/test_kubernetes.py b/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='tkn')
+    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, 'bearer_token': 'tkn'}),
+            ({'kubelet_tls_verify': False}, {'kubelet_verify': False, 'bearer_token': 'tkn'}),
+            ({'kubelet_tls_verify': True}, {'kubelet_verify': True, 'bearer_token': 'tkn'}),
+            ({'kubelet_tls_verify': 'foo.pem'}, {'kubelet_verify': 'foo.pem', 'bearer_token': 'tkn'}),
+            ({'kubelet_cert': 'foo.pem'}, {'kubelet_verify': 'foo.pem', 'bearer_token': 'tkn'}),
             ({'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'), 'bearer_token': 'tkn'}),
+            ({'kubelet_tls_verify': True, 'kubelet_client_crt': 'client.crt'}, {'kubelet_verify': True, 'bearer_token': 'tkn'}),
+            ({'kubelet_client_crt': 'client.crt'}, {'kubelet_verify': True, 'bearer_token': 'tkn'})
         ]
         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, 'bearer_token': 'tkn'}
+        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, 'bearer_token': 'tkn'})
 
     ##### 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,65 +458,58 @@ 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):
@@ -551,7 +553,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 +570,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 +582,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')
diff --git a/utils/kubernetes/kubeutil.py b/utils/kubernetes/kubeutil.py

Original file line number	Diff line number	Diff line change
`@@ -69,29 +69,32 @@ instances:`
`69`	`69`	`# kubelet port. It needs to be set if you are not using a default one (10250 or 10255)`
`70`	`70`	`# kubelet_port: 10255`
`71`	`71`	`#`
`72`		`- # SSL configuration for querying kubelet.`
	`72`	`+ # TLS configuration for querying kubelet.`
`73`	`73`	`#`
`74`	`74`	`# Available options are:`
`75`		`- # - simple SSL validation (with readily available certificates)`
`76`		`- # - disabling SSL validation`
	`75`	`+ # (server auth)`
	`76`	`+ # - simple TLS validation (with readily available certificates)`
	`77`	`+ # - disabling TLS validation`
`77`	`78`	`# - providing the server certificate`
	`79`	`+ # (client auth)`
`78`	`80`	`# - providing a client cert/key pair`
	`81`	`+ # - using a service account bearer token`
`79`	`82`	`#`
`80`		`- # The default is to try and use the read-only port that doesn't require SSL`
`81`		`- # And to fall back to the HTTPS API with simple SSL validation.`
	`83`	`+ # The default is to try and use the read-only port that doesn't require TLS`
	`84`	`+ # And to fall back to the HTTPS API with simple TLS validation.`
`82`	`85`	`#`
`83`		`- # Explicitly disabling ssl_verify should be used with caution:`
	`86`	`+ # Explicitly disabling tls_verify should be used with caution:`
`84`	`87`	`# if an attacker sniffs the agent requests they will see the agent's service account bearer token.`
`85`	`88`	`#`
`86`		`- # Providing a server cert enables SSL validation.`
	`89`	`+ # Providing a server cert enables TLS validation.`
`87`	`90`	`#`
`88`	`91`	`# It is possible to providing a client cert/key pair for client auth.`
`89`		`- # The agent also uses its pod bearer token (if available) when querying kubelet and the apiserver.`
	`92`	`+ # If no such cert is passed, the agent authenticates using its pod bearer token (if available) when querying kubelet and the apiserver.`
`90`	`93`	`#`
`91`	`94`	`# The recommended way to pass certificates and keys to the agent is by using`
`92`	`95`	`# "Secret" Kubernetes object (and to mount them as volumes).`
`93`	`96`	`#`
`94`		`- # kubelet_ssl_verify: True`
	`97`	`+ # kubelet_tls_verify: True`
`95`	`98`	`# kubelet_cert: /path/to/ca.pem`
`96`	`99`	`# kubelet_client_crt: /path/to/client.crt`
`97`	`100`	`# kubelet_client_key: /path/to/client.key`