Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace InsecureRequestWarning with standard logs #7512

Merged
merged 4 commits into from
Sep 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,8 @@
import requests
from google.protobuf.internal.decoder import _DecodeVarint32 # pylint: disable=E0611,E0401
from six import PY3, iteritems, itervalues, string_types
from urllib3.exceptions import InsecureRequestWarning

from ...utils.prometheus import metrics_pb2
from ...utils.warnings_util import disable_warnings_ctx
from .. import AgentCheck
from ..libs.prometheus import text_fd_to_metric_families

Expand Down Expand Up @@ -529,11 +527,14 @@ def poll(self, endpoint, pFormat=PrometheusFormat.PROTOBUF, headers=None):
elif self.ssl_ca_cert is False:
disable_insecure_warnings = True
verify = False

if not disable_insecure_warnings and not verify:
self.log.warning(u'An unverified HTTPS request is being made to %s', endpoint)

try:
with disable_warnings_ctx(InsecureRequestWarning, disable=disable_insecure_warnings):
response = requests.get(
endpoint, headers=headers, stream=False, timeout=self.prometheus_timeout, cert=cert, verify=verify
)
response = requests.get(
endpoint, headers=headers, stream=False, timeout=self.prometheus_timeout, cert=cert, verify=verify
)
except requests.exceptions.SSLError:
self.log.error("Invalid SSL settings for requesting %s endpoint", endpoint)
raise
Expand Down
5 changes: 5 additions & 0 deletions datadog_checks_base/datadog_checks/base/log.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
# Licensed under a 3-clause BSD style license (see LICENSE)
import logging
import sys
import warnings
from typing import Callable

from six import PY2, text_type
from urllib3.exceptions import InsecureRequestWarning

from .utils.common import to_native_string

Expand Down Expand Up @@ -155,6 +157,9 @@ def init_logging():
rootLogger.addHandler(AgentLogHandler())
rootLogger.setLevel(_get_py_loglevel(datadog_agent.get_config('log_level')))

# We log instead of emit warnings for unintentionally insecure HTTPS requests
warnings.simplefilter('ignore', InsecureRequestWarning)

# `requests` (used in a lot of checks) imports `urllib3`, which logs a bunch of stuff at the info level
# Therefore, pre emptively increase the default level of that logger to `WARN`
urllib_logger = logging.getLogger("requests.packages.urllib3")
Expand Down
12 changes: 3 additions & 9 deletions datadog_checks_base/datadog_checks/base/utils/http.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,11 @@
from requests_toolbelt.adapters import host_header_ssl
from six import PY2, iteritems, string_types
from six.moves.urllib.parse import urlparse
from urllib3.exceptions import InsecureRequestWarning

from ..config import is_affirmative
from ..errors import ConfigurationError
from .common import ensure_bytes, ensure_unicode
from .headers import get_default_headers, update_headers
from .warnings_util import disable_warnings_ctx

try:
from contextlib import ExitStack
Expand Down Expand Up @@ -272,8 +270,6 @@ def __init__(self, instance, init_config, remapper=None, logger=None):

# Context managers that should wrap all requests
self.request_hooks = []
if self.ignore_tls_warning:
self.request_hooks.append(self.handle_tls_warning)

if config['kerberos_keytab']:
self.request_hooks.append(lambda: handle_kerberos_keytab(config['kerberos_keytab']))
Expand Down Expand Up @@ -311,6 +307,9 @@ def _request(self, method, url, options):

new_options = self.populate_options(options)

if not self.ignore_tls_warning and not new_options['verify']:
self.logger.warning(u'An unverified HTTPS request is being made to %s', url)

extra_headers = options.pop('extra_headers', None)
if extra_headers is not None:
new_options['headers'] = new_options['headers'].copy()
Expand All @@ -336,11 +335,6 @@ def populate_options(self, options):

return options

@contextmanager
def handle_tls_warning(self):
with disable_warnings_ctx(InsecureRequestWarning, disable=True):
ofek marked this conversation as resolved.
Show resolved Hide resolved
yield

@property
def session(self):
if self._session is None:
Expand Down
46 changes: 0 additions & 46 deletions datadog_checks_base/datadog_checks/base/utils/warnings_util.py

This file was deleted.

77 changes: 56 additions & 21 deletions datadog_checks_base/tests/test_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
from requests import auth as requests_auth
from requests.exceptions import ConnectTimeout, ProxyError
from six import iteritems
from urllib3.exceptions import InsecureRequestWarning

from datadog_checks.base import AgentCheck, ConfigurationError
from datadog_checks.base.utils.http import STANDARD_FIELDS, RequestsWrapper
Expand Down Expand Up @@ -831,78 +830,114 @@ def test_instance_and_init_flag(self):

assert http.ignore_tls_warning is False

def test_default_no_ignore(self):
def test_default_no_ignore(self, caplog):
instance = {}
init_config = {}
http = RequestsWrapper(instance, init_config)

with pytest.warns(InsecureRequestWarning):
with caplog.at_level(logging.DEBUG), mock.patch('requests.get'):
http.get('https://www.google.com', verify=False)

def test_ignore(self):
expected_message = 'An unverified HTTPS request is being made to https://www.google.com'
for _, level, message in caplog.record_tuples:
if level == logging.WARNING and message == expected_message:
break
else:
raise AssertionError('Expected WARNING log with message `{}`'.format(expected_message))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit)

Looks like we are repeating a lot of logic like this in test.

Maybe we should have some kind of utils to avoid duplication.

        expected_message = 'An unverified HTTPS request is being made to https://www.google.com'
        for _, level, message in caplog.record_tuples:
            if level == logging.WARNING and message == expected_message:
                break
        else:
            raise AssertionError('Expected WARNING log with message `{}`'.format(expected_message))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, I'll add one in a separate PR after this is merged


def test_ignore(self, caplog):
instance = {'tls_ignore_warning': True}
init_config = {}
http = RequestsWrapper(instance, init_config)

with pytest.warns(None) as record:
with caplog.at_level(logging.DEBUG), mock.patch('requests.get'):
http.get('https://www.google.com', verify=False)

assert all(not issubclass(warning.category, InsecureRequestWarning) for warning in record)
expected_message = 'An unverified HTTPS request is being made to https://www.google.com'
for _, _, message in caplog.record_tuples:
assert message != expected_message

def test_default_no_ignore_session(self):
def test_default_no_ignore_session(self, caplog):
instance = {'persist_connections': True}
init_config = {}
http = RequestsWrapper(instance, init_config)

with pytest.warns(InsecureRequestWarning):
with caplog.at_level(logging.DEBUG), mock.patch('requests.get'):
http.get('https://www.google.com', verify=False)

def test_ignore_session(self):
expected_message = 'An unverified HTTPS request is being made to https://www.google.com'
for _, level, message in caplog.record_tuples:
if level == logging.WARNING and message == expected_message:
break
else:
raise AssertionError('Expected WARNING log with message `{}`'.format(expected_message))

def test_ignore_session(self, caplog):
instance = {'tls_ignore_warning': True, 'persist_connections': True}
init_config = {}
http = RequestsWrapper(instance, init_config)

with pytest.warns(None) as record:
with caplog.at_level(logging.DEBUG), mock.patch('requests.get'):
http.get('https://www.google.com', verify=False)

assert all(not issubclass(warning.category, InsecureRequestWarning) for warning in record)
expected_message = 'An unverified HTTPS request is being made to https://www.google.com'
for _, _, message in caplog.record_tuples:
assert message != expected_message

def test_init_ignore(self):
def test_init_ignore(self, caplog):
instance = {}
init_config = {'tls_ignore_warning': True}
http = RequestsWrapper(instance, init_config)

with pytest.warns(None) as record:
with caplog.at_level(logging.DEBUG), mock.patch('requests.get'):
http.get('https://www.google.com', verify=False)

assert all(not issubclass(warning.category, InsecureRequestWarning) for warning in record)
expected_message = 'An unverified HTTPS request is being made to https://www.google.com'
for _, _, message in caplog.record_tuples:
assert message != expected_message

def test_default_init_no_ignore(self):
def test_default_init_no_ignore(self, caplog):
instance = {}
init_config = {'tls_ignore_warning': False}
http = RequestsWrapper(instance, init_config)

with pytest.warns(InsecureRequestWarning):
with caplog.at_level(logging.DEBUG), mock.patch('requests.get'):
http.get('https://www.google.com', verify=False)

def test_instance_ignore(self):
expected_message = 'An unverified HTTPS request is being made to https://www.google.com'
for _, level, message in caplog.record_tuples:
if level == logging.WARNING and message == expected_message:
break
else:
raise AssertionError('Expected WARNING log with message `{}`'.format(expected_message))

def test_instance_ignore(self, caplog):
instance = {'tls_ignore_warning': True}
init_config = {'tls_ignore_warning': False}
http = RequestsWrapper(instance, init_config)

with pytest.warns(None) as record:
with caplog.at_level(logging.DEBUG), mock.patch('requests.get'):
http.get('https://www.google.com', verify=False)

assert all(not issubclass(warning.category, InsecureRequestWarning) for warning in record)
expected_message = 'An unverified HTTPS request is being made to https://www.google.com'
for _, _, message in caplog.record_tuples:
assert message != expected_message

def test_instance_no_ignore(self):
def test_instance_no_ignore(self, caplog):
instance = {'tls_ignore_warning': False}
init_config = {'tls_ignore_warning': True}
http = RequestsWrapper(instance, init_config)

with pytest.warns(InsecureRequestWarning):
with caplog.at_level(logging.DEBUG), mock.patch('requests.get'):
http.get('https://www.google.com', verify=False)

expected_message = 'An unverified HTTPS request is being made to https://www.google.com'
for _, level, message in caplog.record_tuples:
if level == logging.WARNING and message == expected_message:
break
else:
raise AssertionError('Expected WARNING log with message `{}`'.format(expected_message))


class TestSession:
def test_default_none(self):
Expand Down
19 changes: 12 additions & 7 deletions datadog_checks_base/tests/test_openmetrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
from prometheus_client.core import CounterMetricFamily, GaugeMetricFamily, HistogramMetricFamily, SummaryMetricFamily
from prometheus_client.samples import Sample
from six import iteritems
from urllib3.exceptions import InsecureRequestWarning

from datadog_checks.base import ensure_bytes
from datadog_checks.checks.openmetrics import OpenMetricsBaseCheck
Expand Down Expand Up @@ -2495,7 +2494,7 @@ def test_metadata_transformer(mocked_openmetrics_check_factory, text_data, datad
datadog_agent.assert_metadata_count(len(version_metadata))


def test_ssl_verify_not_raise_warning(mocked_openmetrics_check_factory, text_data):
def test_ssl_verify_not_raise_warning(caplog, mocked_openmetrics_check_factory, text_data):
instance = dict(
{
'prometheus_url': 'https://www.example.com',
Expand All @@ -2507,14 +2506,17 @@ def test_ssl_verify_not_raise_warning(mocked_openmetrics_check_factory, text_dat
check = mocked_openmetrics_check_factory(instance)
scraper_config = check.get_scraper_config(instance)

with pytest.warns(None) as record:
with caplog.at_level(logging.DEBUG):
resp = check.send_request('https://httpbin.org/get', scraper_config)

assert "httpbin.org" in resp.content.decode('utf-8')
assert all(not issubclass(warning.category, InsecureRequestWarning) for warning in record)

expected_message = 'An unverified HTTPS request is being made to https://httpbin.org/get'
for _, _, message in caplog.record_tuples:
assert message != expected_message

def test_send_request_with_dynamic_prometheus_url(mocked_openmetrics_check_factory, text_data):

def test_send_request_with_dynamic_prometheus_url(caplog, mocked_openmetrics_check_factory, text_data):
instance = dict(
{
'prometheus_url': 'https://www.example.com',
Expand All @@ -2529,11 +2531,14 @@ def test_send_request_with_dynamic_prometheus_url(mocked_openmetrics_check_facto
# `prometheus_url` changed just before calling `send_request`
scraper_config['prometheus_url'] = 'https://www.example.com/foo/bar'

with pytest.warns(None) as record:
with caplog.at_level(logging.DEBUG):
resp = check.send_request('https://httpbin.org/get', scraper_config)

assert "httpbin.org" in resp.content.decode('utf-8')
assert all(not issubclass(warning.category, InsecureRequestWarning) for warning in record)

expected_message = 'An unverified HTTPS request is being made to https://httpbin.org/get'
for _, _, message in caplog.record_tuples:
assert message != expected_message


def test_http_handler(mocked_openmetrics_check_factory):
Expand Down
23 changes: 14 additions & 9 deletions datadog_checks_base/tests/test_prometheus.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import requests
from six import iteritems, iterkeys
from six.moves import range
from urllib3.exceptions import InsecureRequestWarning

from datadog_checks.checks.prometheus import PrometheusCheck, UnknownFormatError
from datadog_checks.utils.prometheus import metrics_pb2, parse_metric_family
Expand Down Expand Up @@ -1960,22 +1959,28 @@ def test_text_filter_input():
assert filtered == expected_out


def test_ssl_verify_not_raise_warning(mocked_prometheus_check, text_data):
def test_ssl_verify_not_raise_warning(caplog, mocked_prometheus_check, text_data):
check = mocked_prometheus_check

with pytest.warns(None) as record:
with caplog.at_level(logging.DEBUG):
resp = check.poll('https://httpbin.org/get')

assert "httpbin.org" in resp.content.decode('utf-8')
assert all(not issubclass(warning.category, InsecureRequestWarning) for warning in record)
assert 'httpbin.org' in resp.content.decode('utf-8')

expected_message = 'An unverified HTTPS request is being made to https://httpbin.org/get'
for _, _, message in caplog.record_tuples:
assert message != expected_message

def test_ssl_verify_not_raise_warning_cert_false(mocked_prometheus_check, text_data):

def test_ssl_verify_not_raise_warning_cert_false(caplog, mocked_prometheus_check, text_data):
check = mocked_prometheus_check
check.ssl_ca_cert = False

with pytest.warns(None) as record:
with caplog.at_level(logging.DEBUG):
resp = check.poll('https://httpbin.org/get')

assert "httpbin.org" in resp.content.decode('utf-8')
assert all(not issubclass(warning.category, InsecureRequestWarning) for warning in record)
assert 'httpbin.org' in resp.content.decode('utf-8')

expected_message = 'An unverified HTTPS request is being made to https://httpbin.org/get'
for _, _, message in caplog.record_tuples:
assert message != expected_message
Loading