From 6802ef3c5aa294a3242e5f8813808f6d40336e6e Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Wed, 30 Dec 2020 10:17:28 -0500 Subject: [PATCH 01/11] Add parameter for overriding get_tls_context() --- datadog_checks_base/datadog_checks/base/checks/base.py | 4 ++-- datadog_checks_base/datadog_checks/base/utils/tls.py | 9 +++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/datadog_checks_base/datadog_checks/base/checks/base.py b/datadog_checks_base/datadog_checks/base/checks/base.py index da7806aa103b2..0f2b71744537d 100644 --- a/datadog_checks_base/datadog_checks/base/checks/base.py +++ b/datadog_checks_base/datadog_checks/base/checks/base.py @@ -309,7 +309,7 @@ def http(self): return self._http - def get_tls_context(self, refresh=False): + def get_tls_context(self, refresh=False, overrides=None): # type: (bool) -> ssl.SSLContext """ Creates and cache an SSLContext instance based on user configuration. @@ -317,7 +317,7 @@ def get_tls_context(self, refresh=False): Since: Agent 7.24 """ if not hasattr(self, '_tls_context_wrapper'): - self._tls_context_wrapper = TlsContextWrapper(self.instance or {}, self.TLS_CONFIG_REMAPPER) + self._tls_context_wrapper = TlsContextWrapper(self.instance or {}, self.TLS_CONFIG_REMAPPER, overrides) if refresh: self._tls_context_wrapper.refresh_tls_context() diff --git a/datadog_checks_base/datadog_checks/base/utils/tls.py b/datadog_checks_base/datadog_checks/base/utils/tls.py index 2c3c54b439360..f85d273ed7294 100644 --- a/datadog_checks_base/datadog_checks/base/utils/tls.py +++ b/datadog_checks_base/datadog_checks/base/utils/tls.py @@ -35,8 +35,8 @@ class TlsContextWrapper(object): __slots__ = ('logger', 'config', 'tls_context') - def __init__(self, instance, remapper=None): - # type: (InstanceType, Dict[AnyStr, Dict[AnyStr, Any]]) -> None + def __init__(self, instance, remapper=None, overrides=None): + # type: (InstanceType, Dict[AnyStr, Dict[AnyStr, Any]], Dict[AnyStr, Any]) -> None default_fields = dict(STANDARD_FIELDS) # Populate with the default values @@ -91,6 +91,11 @@ def __init__(self, instance, remapper=None): config[field] = config[unique_name] del config[unique_name] + # Override existing config options if there exists any overrides + for overridden_field, data in iteritems(overrides): + if config[overridden_field]: + config[overridden_field] = data + self.config = config self.tls_context = self._create_tls_context() From be9cbaf9ab782adbce832782545e128449b8df53 Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Wed, 30 Dec 2020 10:57:16 -0500 Subject: [PATCH 02/11] Add comments to update --- datadog_checks_base/datadog_checks/base/checks/base.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/datadog_checks_base/datadog_checks/base/checks/base.py b/datadog_checks_base/datadog_checks/base/checks/base.py index 0f2b71744537d..61ee70a422b4d 100644 --- a/datadog_checks_base/datadog_checks/base/checks/base.py +++ b/datadog_checks_base/datadog_checks/base/checks/base.py @@ -12,7 +12,7 @@ import unicodedata from collections import defaultdict, deque from os.path import basename -from typing import TYPE_CHECKING, Any, Callable, DefaultDict, Deque, Dict, List, Optional, Sequence, Tuple, Union +from typing import TYPE_CHECKING, Any, AnyStr, Callable, DefaultDict, Deque, Dict, List, Optional, Sequence, Tuple, Union import yaml from six import binary_type, iteritems, text_type @@ -310,9 +310,11 @@ def http(self): return self._http def get_tls_context(self, refresh=False, overrides=None): - # type: (bool) -> ssl.SSLContext + # type: (bool, Dict[AnyStr, Any]) -> ssl.SSLContext """ Creates and cache an SSLContext instance based on user configuration. + Note that user configuration can be overridden by using `overrides`. + This should only be applied to older integration that manually set config values. Since: Agent 7.24 """ From b91a1a28313a3b7723d99544bc4174cf710c1f35 Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Wed, 30 Dec 2020 10:57:39 -0500 Subject: [PATCH 03/11] Add sample for override --- tls/datadog_checks/tls/tls.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tls/datadog_checks/tls/tls.py b/tls/datadog_checks/tls/tls.py index ffe10bf5cb1a2..0b23f3fe307a5 100644 --- a/tls/datadog_checks/tls/tls.py +++ b/tls/datadog_checks/tls/tls.py @@ -340,14 +340,18 @@ def tls_context(self): if self._tls_context is None: # https://docs.python.org/3/library/ssl.html#ssl.SSLContext # https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_TLS - self._tls_context = ssl.SSLContext(protocol=PROTOCOL_TLS_CLIENT) + + overrides = { + "tls_validate_hostname": False + } + self._tls_context = self.get_tls_context(overrides=overrides) # Run our own validation later on if need be # https://docs.python.org/3/library/ssl.html#ssl.SSLContext.check_hostname # # IMPORTANT: This must be set before verify_mode in Python 3.7+, see: # https://docs.python.org/3/library/ssl.html#ssl.SSLContext.check_hostname - self._tls_context.check_hostname = False + # self._tls_context.check_hostname = False # https://docs.python.org/3/library/ssl.html#ssl.SSLContext.verify_mode self._tls_context.verify_mode = ssl.CERT_REQUIRED if self._validate_cert else ssl.CERT_NONE From 1137e495562f0db4bec53df00316f1b671506242 Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Wed, 30 Dec 2020 10:58:13 -0500 Subject: [PATCH 04/11] Undo sample --- tls/datadog_checks/tls/tls.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tls/datadog_checks/tls/tls.py b/tls/datadog_checks/tls/tls.py index 0b23f3fe307a5..ffe10bf5cb1a2 100644 --- a/tls/datadog_checks/tls/tls.py +++ b/tls/datadog_checks/tls/tls.py @@ -340,18 +340,14 @@ def tls_context(self): if self._tls_context is None: # https://docs.python.org/3/library/ssl.html#ssl.SSLContext # https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_TLS - - overrides = { - "tls_validate_hostname": False - } - self._tls_context = self.get_tls_context(overrides=overrides) + self._tls_context = ssl.SSLContext(protocol=PROTOCOL_TLS_CLIENT) # Run our own validation later on if need be # https://docs.python.org/3/library/ssl.html#ssl.SSLContext.check_hostname # # IMPORTANT: This must be set before verify_mode in Python 3.7+, see: # https://docs.python.org/3/library/ssl.html#ssl.SSLContext.check_hostname - # self._tls_context.check_hostname = False + self._tls_context.check_hostname = False # https://docs.python.org/3/library/ssl.html#ssl.SSLContext.verify_mode self._tls_context.verify_mode = ssl.CERT_REQUIRED if self._validate_cert else ssl.CERT_NONE From 8f94c6f84a632412425df78a6bc64021d07d657b Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Wed, 30 Dec 2020 15:10:18 -0500 Subject: [PATCH 05/11] Add check for overrides = None --- datadog_checks_base/datadog_checks/base/utils/tls.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/datadog_checks_base/datadog_checks/base/utils/tls.py b/datadog_checks_base/datadog_checks/base/utils/tls.py index f85d273ed7294..f094b580add46 100644 --- a/datadog_checks_base/datadog_checks/base/utils/tls.py +++ b/datadog_checks_base/datadog_checks/base/utils/tls.py @@ -92,9 +92,10 @@ def __init__(self, instance, remapper=None, overrides=None): del config[unique_name] # Override existing config options if there exists any overrides - for overridden_field, data in iteritems(overrides): - if config[overridden_field]: - config[overridden_field] = data + if overrides: + for overridden_field, data in iteritems(overrides): + if config[overridden_field]: + config[overridden_field] = data self.config = config self.tls_context = self._create_tls_context() From eb729f52df37588c41b7ffd08a9388bce90158d2 Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Thu, 31 Dec 2020 09:41:31 -0500 Subject: [PATCH 06/11] Fix style --- .../datadog_checks/base/checks/base.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/datadog_checks_base/datadog_checks/base/checks/base.py b/datadog_checks_base/datadog_checks/base/checks/base.py index 61ee70a422b4d..d280e491c84dc 100644 --- a/datadog_checks_base/datadog_checks/base/checks/base.py +++ b/datadog_checks_base/datadog_checks/base/checks/base.py @@ -12,7 +12,20 @@ import unicodedata from collections import defaultdict, deque from os.path import basename -from typing import TYPE_CHECKING, Any, AnyStr, Callable, DefaultDict, Deque, Dict, List, Optional, Sequence, Tuple, Union +from typing import ( + TYPE_CHECKING, + Any, + AnyStr, + Callable, + DefaultDict, + Deque, + Dict, + List, + Optional, + Sequence, + Tuple, + Union, +) import yaml from six import binary_type, iteritems, text_type From 76a0da362bf8ead75c28820797c141aac3e28f17 Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Tue, 5 Jan 2021 16:19:35 -0500 Subject: [PATCH 07/11] Add tests for tls overrides --- .../datadog_checks/base/checks/base.py | 4 ++- .../datadog_checks/base/utils/tls.py | 11 +++---- datadog_checks_base/tests/test_tls.py | 32 +++++++++++++++++++ 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/datadog_checks_base/datadog_checks/base/checks/base.py b/datadog_checks_base/datadog_checks/base/checks/base.py index d280e491c84dc..1314780b761df 100644 --- a/datadog_checks_base/datadog_checks/base/checks/base.py +++ b/datadog_checks_base/datadog_checks/base/checks/base.py @@ -332,7 +332,9 @@ def get_tls_context(self, refresh=False, overrides=None): Since: Agent 7.24 """ if not hasattr(self, '_tls_context_wrapper'): - self._tls_context_wrapper = TlsContextWrapper(self.instance or {}, self.TLS_CONFIG_REMAPPER, overrides) + self._tls_context_wrapper = TlsContextWrapper( + self.instance or {}, self.TLS_CONFIG_REMAPPER, overrides=overrides + ) if refresh: self._tls_context_wrapper.refresh_tls_context() diff --git a/datadog_checks_base/datadog_checks/base/utils/tls.py b/datadog_checks_base/datadog_checks/base/utils/tls.py index f094b580add46..63761a944ac3c 100644 --- a/datadog_checks_base/datadog_checks/base/utils/tls.py +++ b/datadog_checks_base/datadog_checks/base/utils/tls.py @@ -39,6 +39,11 @@ def __init__(self, instance, remapper=None, overrides=None): # type: (InstanceType, Dict[AnyStr, Dict[AnyStr, Any]], Dict[AnyStr, Any]) -> None default_fields = dict(STANDARD_FIELDS) + # Override existing config options if there exists any overrides + if overrides: + for overridden_field, data in iteritems(overrides): + instance[overridden_field] = data + # Populate with the default values config = {field: instance.get(field, value) for field, value in iteritems(default_fields)} for field in STANDARD_FIELDS: @@ -91,12 +96,6 @@ def __init__(self, instance, remapper=None, overrides=None): config[field] = config[unique_name] del config[unique_name] - # Override existing config options if there exists any overrides - if overrides: - for overridden_field, data in iteritems(overrides): - if config[overridden_field]: - config[overridden_field] = data - self.config = config self.tls_context = self._create_tls_context() diff --git a/datadog_checks_base/tests/test_tls.py b/datadog_checks_base/tests/test_tls.py index e67c6fffd60d8..29d3625477bec 100644 --- a/datadog_checks_base/tests/test_tls.py +++ b/datadog_checks_base/tests/test_tls.py @@ -201,3 +201,35 @@ def test_client_key_expanded(self): with patch('ssl.SSLContext'), patch('os.path.expanduser') as mock_expand: check.get_tls_context() mock_expand.assert_called_with('~/foo') + + +class TestTLSContextOverrides: + def test_override_context(self): + instance = {'tls_cert': 'foo', 'tls_private_key': 'bar'} + check = AgentCheck('test', {}, [instance]) + + overrides = {'tls_cert': 'not_foo'} + with patch('ssl.SSLContext'): + context = check.get_tls_context(overrides=overrides) # type: MagicMock + context.load_cert_chain.assert_called_with('not_foo', keyfile='bar', password=None) + + def test_override_context_empty(self): + instance = {'tls_cert': 'foo', 'tls_private_key': 'bar'} + check = AgentCheck('test', {}, [instance]) + + overrides = {} + with patch('ssl.SSLContext'): + context = check.get_tls_context(overrides=overrides) # type: MagicMock + context.load_cert_chain.assert_called_with('foo', keyfile='bar', password=None) + + def test_override_context_wrapper_config(self): + instance = {'tls_verify': True} + overrides = {'tls_verify': False} + tls = TlsContextWrapper(instance, overrides=overrides) + assert tls.config['tls_verify'] is False + + def test_override_context_wrapper_config_empty(self): + instance = {'tls_verify': True} + overrides = {} + tls = TlsContextWrapper(instance, overrides=overrides) + assert tls.config['tls_verify'] is True From 466a690d565da957dd96f9fd230fec61eb2046cc Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Tue, 5 Jan 2021 16:27:59 -0500 Subject: [PATCH 08/11] Add conditional to not add non-existent config options --- datadog_checks_base/datadog_checks/base/utils/tls.py | 3 ++- datadog_checks_base/tests/test_tls.py | 12 ++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/datadog_checks_base/datadog_checks/base/utils/tls.py b/datadog_checks_base/datadog_checks/base/utils/tls.py index 63761a944ac3c..be91767b61e28 100644 --- a/datadog_checks_base/datadog_checks/base/utils/tls.py +++ b/datadog_checks_base/datadog_checks/base/utils/tls.py @@ -42,7 +42,8 @@ def __init__(self, instance, remapper=None, overrides=None): # Override existing config options if there exists any overrides if overrides: for overridden_field, data in iteritems(overrides): - instance[overridden_field] = data + if instance.get(overridden_field): + instance[overridden_field] = data # Populate with the default values config = {field: instance.get(field, value) for field, value in iteritems(default_fields)} diff --git a/datadog_checks_base/tests/test_tls.py b/datadog_checks_base/tests/test_tls.py index 29d3625477bec..f761375a8a122 100644 --- a/datadog_checks_base/tests/test_tls.py +++ b/datadog_checks_base/tests/test_tls.py @@ -233,3 +233,15 @@ def test_override_context_wrapper_config_empty(self): overrides = {} tls = TlsContextWrapper(instance, overrides=overrides) assert tls.config['tls_verify'] is True + + def test_override_instance_config(self): + instance = {'tls_verify': True} + overrides = {'tls_verify': False} + tls = TlsContextWrapper(instance, overrides=overrides) + assert instance['tls_verify'] is False + + def test_override_non_exist_instance_config(self): + instance = {'tls_verify': True} + overrides = {'fake_config': 'foo'} + tls = TlsContextWrapper(instance, overrides=overrides) + assert instance.get('fake_config') is None From c4baf6aa7b149e7bf814e5b7bb4032d42112dd25 Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Thu, 7 Jan 2021 11:13:22 -0500 Subject: [PATCH 09/11] Create new overridden_instance instead of modifying instance in place --- .../datadog_checks/base/utils/tls.py | 14 ++++++++------ datadog_checks_base/tests/test_tls.py | 8 ++------ 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/datadog_checks_base/datadog_checks/base/utils/tls.py b/datadog_checks_base/datadog_checks/base/utils/tls.py index be91767b61e28..29045536d09b9 100644 --- a/datadog_checks_base/datadog_checks/base/utils/tls.py +++ b/datadog_checks_base/datadog_checks/base/utils/tls.py @@ -40,17 +40,19 @@ def __init__(self, instance, remapper=None, overrides=None): default_fields = dict(STANDARD_FIELDS) # Override existing config options if there exists any overrides + overridden_instance = {field: data for field, data in iteritems(instance)} + if overrides: for overridden_field, data in iteritems(overrides): - if instance.get(overridden_field): - instance[overridden_field] = data + if overridden_instance.get(overridden_field): + overridden_instance[overridden_field] = data # Populate with the default values - config = {field: instance.get(field, value) for field, value in iteritems(default_fields)} + config = {field: overridden_instance.get(field, value) for field, value in iteritems(default_fields)} for field in STANDARD_FIELDS: unique_name = UNIQUE_FIELD_PREFIX + field - if unique_name in instance: - config[unique_name] = instance[unique_name] + if unique_name in overridden_instance: + config[unique_name] = overridden_instance[unique_name] if remapper is None: remapper = {} @@ -77,7 +79,7 @@ def __init__(self, instance, remapper=None, overrides=None): default = not default # Get value, with a possible default - value = instance.get(remapped_field, data.get('default', default)) + value = overridden_instance.get(remapped_field, data.get('default', default)) # Invert booleans if need be if data.get('invert'): diff --git a/datadog_checks_base/tests/test_tls.py b/datadog_checks_base/tests/test_tls.py index f761375a8a122..b206d35fb7685 100644 --- a/datadog_checks_base/tests/test_tls.py +++ b/datadog_checks_base/tests/test_tls.py @@ -227,6 +227,7 @@ def test_override_context_wrapper_config(self): overrides = {'tls_verify': False} tls = TlsContextWrapper(instance, overrides=overrides) assert tls.config['tls_verify'] is False + assert instance['tls_verify'] is True # Overrides should not affect the original instance def test_override_context_wrapper_config_empty(self): instance = {'tls_verify': True} @@ -234,14 +235,9 @@ def test_override_context_wrapper_config_empty(self): tls = TlsContextWrapper(instance, overrides=overrides) assert tls.config['tls_verify'] is True - def test_override_instance_config(self): - instance = {'tls_verify': True} - overrides = {'tls_verify': False} - tls = TlsContextWrapper(instance, overrides=overrides) - assert instance['tls_verify'] is False - def test_override_non_exist_instance_config(self): instance = {'tls_verify': True} overrides = {'fake_config': 'foo'} tls = TlsContextWrapper(instance, overrides=overrides) assert instance.get('fake_config') is None + assert tls.config['tls_verify'] is True From ce444a2b2a6d1b44897f66edc68d3cdde905576c Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Thu, 7 Jan 2021 11:16:41 -0500 Subject: [PATCH 10/11] Use deepcopy() --- datadog_checks_base/datadog_checks/base/utils/tls.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/datadog_checks_base/datadog_checks/base/utils/tls.py b/datadog_checks_base/datadog_checks/base/utils/tls.py index 29045536d09b9..fd7c7029a8c23 100644 --- a/datadog_checks_base/datadog_checks/base/utils/tls.py +++ b/datadog_checks_base/datadog_checks/base/utils/tls.py @@ -4,6 +4,7 @@ import logging import os import ssl +from copy import deepcopy from typing import TYPE_CHECKING, Any, AnyStr, Dict from six import iteritems @@ -40,7 +41,7 @@ def __init__(self, instance, remapper=None, overrides=None): default_fields = dict(STANDARD_FIELDS) # Override existing config options if there exists any overrides - overridden_instance = {field: data for field, data in iteritems(instance)} + overridden_instance = deepcopy(instance) if overrides: for overridden_field, data in iteritems(overrides): From 1247bb1657e3193bfa6c2bf428ff25be4243d5e3 Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Thu, 7 Jan 2021 11:21:36 -0500 Subject: [PATCH 11/11] Update to use instance instead of overridden_instance --- .../datadog_checks/base/utils/tls.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/datadog_checks_base/datadog_checks/base/utils/tls.py b/datadog_checks_base/datadog_checks/base/utils/tls.py index fd7c7029a8c23..ff8a1a5d03733 100644 --- a/datadog_checks_base/datadog_checks/base/utils/tls.py +++ b/datadog_checks_base/datadog_checks/base/utils/tls.py @@ -41,19 +41,19 @@ def __init__(self, instance, remapper=None, overrides=None): default_fields = dict(STANDARD_FIELDS) # Override existing config options if there exists any overrides - overridden_instance = deepcopy(instance) + instance = deepcopy(instance) if overrides: for overridden_field, data in iteritems(overrides): - if overridden_instance.get(overridden_field): - overridden_instance[overridden_field] = data + if instance.get(overridden_field): + instance[overridden_field] = data # Populate with the default values - config = {field: overridden_instance.get(field, value) for field, value in iteritems(default_fields)} + config = {field: instance.get(field, value) for field, value in iteritems(default_fields)} for field in STANDARD_FIELDS: unique_name = UNIQUE_FIELD_PREFIX + field - if unique_name in overridden_instance: - config[unique_name] = overridden_instance[unique_name] + if unique_name in instance: + config[unique_name] = instance[unique_name] if remapper is None: remapper = {} @@ -80,7 +80,7 @@ def __init__(self, instance, remapper=None, overrides=None): default = not default # Get value, with a possible default - value = overridden_instance.get(remapped_field, data.get('default', default)) + value = instance.get(remapped_field, data.get('default', default)) # Invert booleans if need be if data.get('invert'):