From 19325f5e28e720da71156208dc5954e9c570b858 Mon Sep 17 00:00:00 2001
From: Gardener CI/CD <christian.cwienk@sap.com>
Date: Fri, 5 Jan 2024 23:01:58 +0100
Subject: [PATCH] define http-request-timeouts (+ refactor)

As suggested by `requests`'s documentation [0], define time-outs for
http-requests. This change was inspired/suggested by bandit [1][2]. This
change addresses all findings of `medium` severity. In addition, switch
to using `requests.sessions.Session` where reasonable (this is the case
if there is a reasonable likelihood for a session to actually be used
more than once). Also, refactor some quirky code.

Considerations w.r.t. redundantly defining timeout parameter
------------------------------------------------------------

As discussed on `requests`'s GitHub-Repository (e.g. at [3]), there is
no means (except, of course, through "monkey-patching") to define a
default timeout, neither globally, nor e.g. for a session. While it is
an option to implement a custom transport-adaptor (and use it to inject
timeout parameter into issued requests), this approach seems rather
heavyweight, and will add a lot of boilerplate code.

Therefore, it seems like the best available option to pass `timeout`
parameter to each individual invocation of `requests`'s
"request-issueing-functions" (request, get, put, ...).

One might feel the urge to deduplicate the specified timeout values.
However, there are reasons against this:
- will also add boilerplate (one extra import stmt)
- it is unlikely that timeouts will ever (have to) be changed (more
  likely: specific individual timeouts might require "tuning" - which is
  very easy with the chosen approach)

Therefore - assuming the chosen de-facto default timeout value proves to
be "good enough" - it seems like an adequate choice to redundantly
define timeout values.

[0]
https://requests.readthedocs.io/en/latest/user/advanced/#timeouts
[1]
https://bandit.readthedocs.io/en/1.7.6/plugins/b113_request_without_timeout.html
[2]
https://bandit.readthedocs.io/
[3]
https://github.com/psf/requests/issues/3070
---
 ccc/secrets_server.py                   |  2 +-
 cfg_mgmt/azure.py                       |  3 ++
 cfg_mgmt/btp_application_certificate.py | 31 +++++++++++++++----
 cfg_mgmt/btp_service_binding.py         | 32 ++++++++++++++++----
 cfg_mgmt/github.py                      | 40 +++++++++++++------------
 checkmarx/client.py                     | 19 ++++++++++--
 concourse/client/api.py                 |  7 ++---
 ctt/util.py                             |  1 +
 delivery/client.py                      | 37 +++++++++++++++--------
 http_requests.py                        |  5 ++++
 oci/client.py                           |  8 +++++
 protecode/client.py                     |  7 +++++
 12 files changed, 143 insertions(+), 49 deletions(-)

diff --git a/ccc/secrets_server.py b/ccc/secrets_server.py
index 966c500cf..75ef1311c 100644
--- a/ccc/secrets_server.py
+++ b/ccc/secrets_server.py
@@ -131,7 +131,7 @@ def retrieve_secrets(self):
                     return json.load(f)
 
         request_url = urljoin(self.url, self.concourse_secret_name)
-        response = requests.get(request_url)
+        response = requests.get(request_url, timeout=(4, 31))
 
         if not response.ok:
         # pylint: enable=no-member
diff --git a/cfg_mgmt/azure.py b/cfg_mgmt/azure.py
index dae85e3fc..bdac395e5 100644
--- a/cfg_mgmt/azure.py
+++ b/cfg_mgmt/azure.py
@@ -69,6 +69,7 @@ def _retrieve_password_credentials(
     response = requests.get(
         url=f'{azure_app_root_url}/{service_principal.object_id()}',
         headers={'Authorization': f'Bearer {access_token}'},
+        timeout=(4, 31),
     )
     response_dict = response.json()
     if not (password_credentials := response_dict.get('passwordCredentials')):
@@ -99,6 +100,7 @@ def _add_password_credential(
         f'{azure_app_root_url}/{service_principal.object_id()}/addPassword',
         json=body,
         headers={'Authorization': f'Bearer {access_token}'},
+        timeout=(4, 31),
     )
     response.raise_for_status()
     response_dict = response.json()
@@ -119,6 +121,7 @@ def _remove_password_credential(
         f'{azure_app_root_url}/{service_principal.object_id()}/removePassword',
         json={"keyId": key_id},
         headers={'Authorization': f'Bearer {access_token}'},
+        timeout=(4, 31),
     )
     # successful request returns "204 No content"
     response.raise_for_status()
diff --git a/cfg_mgmt/btp_application_certificate.py b/cfg_mgmt/btp_application_certificate.py
index 9b77ea7f7..5aca5f20c 100644
--- a/cfg_mgmt/btp_application_certificate.py
+++ b/cfg_mgmt/btp_application_certificate.py
@@ -45,7 +45,12 @@ def _setup_oauth_token(self):
         headers = {
             'Accept': 'application/json',
         }
-        resp = requests.post(f'{uaa["url"]}/oauth/token', data=data, headers=headers)
+        resp = requests.post(
+            f'{uaa["url"]}/oauth/token',
+            data=data,
+            headers=headers,
+            timeout=(4, 31),
+        )
         resp.raise_for_status()
         self.access_token = resp.json()['access_token']
 
@@ -60,7 +65,12 @@ def create_client_certificate_chain(self, csr_pem: str, validity_in_days: int) -
             'validity': {'type': 'DAYS', 'value': validity_in_days},
         }}
         url = self.credentials['certificateservice']['profileurl']
-        resp = requests.post(url, json=data, headers=headers)
+        resp = requests.post(
+            url,
+            json=data,
+            headers=headers,
+            timeout=(4, 31),
+        )
         resp.raise_for_status()
         logger.info('Created certificate')
         return resp.json()['certificate-response']
@@ -108,7 +118,11 @@ def put_certificate(self, cert_pem: str, desc: str, scopes: list[str]) -> str:
             'base64': cert_pem,
         }
         with self._session_with_cert() as session:
-            resp = session.put(self.url, json=data)
+            resp = session.put(
+                self.url,
+                json=data,
+                timeout=(4, 31),
+            )
         resp.raise_for_status()
         id = resp.json()['certificateId']
         logger.info(f'Added certificate {id}')
@@ -119,7 +133,11 @@ def delete_certificate(self, common_name: str, cert_id: str):
             'certificateId': cert_id,
         }
         with self._session_with_cert() as session:
-            resp = session.delete(self.url, json=data)
+            resp = session.delete(
+                self.url,
+                json=data,
+                timeout=(4, 31),
+            )
         resp.raise_for_status()
         logger.info(f'Deleted certificate {common_name} ({cert_id})')
 
@@ -136,7 +154,10 @@ def list_certificates_by_base(
         common_name_base: str
     ) -> typing.Generator[CertInfo, None, None]:
         with self._session_with_cert() as session:
-            resp = session.get(self.clienturl)
+            resp = session.get(
+                self.clienturl,
+                timeout=(4, 31),
+            )
         resp.raise_for_status()
         for item in resp.json():
             id = item['dnId']
diff --git a/cfg_mgmt/btp_service_binding.py b/cfg_mgmt/btp_service_binding.py
index b2f87f5c8..4695a4343 100644
--- a/cfg_mgmt/btp_service_binding.py
+++ b/cfg_mgmt/btp_service_binding.py
@@ -65,7 +65,11 @@ def delete_service_binding(self, name: str, id: str):
             'Authorization': f'Bearer {self.access_token}',
         }
         url = f'{self.sm_url}/v1/service_bindings/{id}'
-        resp = requests.delete(url, headers=headers)
+        resp = requests.delete(
+            url,
+            headers=headers,
+            timeout=(4, 31),
+        )
         if not resp.ok:
             msg = f'delete_service_binding failed: {resp.status_code} {resp.text}'
             logger.error(msg)
@@ -83,7 +87,12 @@ def create_service_binding(self, instance_id: str, binding_name: str) \
             'Accept': 'application/json',
             'Authorization': f'Bearer {self.access_token}',
         }
-        resp = requests.post(url, json=data, headers=headers)
+        resp = requests.post(
+            url,
+            json=data,
+            headers=headers,
+            timeout=(4, 31),
+        )
         if not resp.ok:
             msg = f'create_service_binding failed: {resp.status_code} {resp.text}'
             logger.error(msg)
@@ -99,7 +108,11 @@ def get_service_binding(self, id: str) -> ServiceBindingDetailedInfo:
             'Accept': 'application/json',
             'Authorization': f'Bearer {self.access_token}',
         }
-        resp = requests.get(url, headers=headers)
+        resp = requests.get(
+            url,
+            headers=headers,
+            timeout=(4, 31),
+        )
         if not resp.ok:
             msg = f'get_service_binding failed: {resp.status_code} {resp.text}'
             logger.error(msg)
@@ -113,7 +126,11 @@ def get_service_bindings(self) -> list[ServiceBindingInfo]:
             'Accept': 'application/json',
             'Authorization': f'Bearer {self.access_token}',
         }
-        resp = requests.get(url, headers=headers)
+        resp = requests.get(
+            url,
+            headers=headers,
+            timeout=(4, 31),
+        )
         if not resp.ok:
             msg = f'get_service_bindings failed: {resp.status_code} {resp.text}'
             logger.error(msg)
@@ -156,7 +173,12 @@ def _get_oauth_token(credentials: dict) -> str:
     headers = {
         'Accept': 'application/json',
     }
-    resp = requests.post(url, data=data, headers=headers)
+    resp = requests.post(
+        url,
+        data=data,
+        headers=headers,
+        timeout=(4, 31),
+    )
     if not resp.ok:
         msg = f'_get_oauth_token failed: {resp.status_code} {resp.reason}'
         logger.error(msg)
diff --git a/cfg_mgmt/github.py b/cfg_mgmt/github.py
index 88f56533a..d2801bd6c 100644
--- a/cfg_mgmt/github.py
+++ b/cfg_mgmt/github.py
@@ -3,7 +3,6 @@
 import enum
 import hashlib
 import logging
-import sys
 import typing
 
 import requests
@@ -102,14 +101,16 @@ def _rotate_oauth_token(
     client_secret = '18867509d956965542b521a529a79bb883344c90'
 
     request_url = f'{github_api_url}/applications/{client_id}/token'
-    request_kwargs = {
-        'url': request_url,
-        'auth': (client_id, client_secret), # basic auth
-        'json': {'access_token': token_to_rotate},
-    }
+    session = requests.sessions.Session()
+    session.auth = (client_id, client_secret)
+
     # check token (to catch the case if the token not being associated with the git-credentials-
     # manager)
-    resp = requests.post(**request_kwargs)
+    resp = session.post(
+        request_url,
+        json={'access_token': token_to_rotate},
+        timeout=(4, 31),
+    )
     if not resp.ok:
         # No way for the user to see the oAuth token that belongs to the app, but at least checking
         # whether the given token is currently associated with the app can be done.
@@ -118,11 +119,15 @@ def _rotate_oauth_token(
             'oAuth token belong to the git-credentials-manager application and is valid?'
         )
         return None
+
     # fire request that causes the actual refresh (i.e. "rotation") to happen.
     # Note: If successful, old token is immediately invalidated.
-    resp = requests.patch(**request_kwargs)
-    if not resp.ok:
-        resp.raise_for_status()
+    resp = session.patch(
+        request_url,
+        json={'access_token': token_to_rotate},
+        timeout=(4, 31),
+    )
+    resp.raise_for_status()
 
     response_content = resp.json()
 
@@ -184,15 +189,12 @@ def rotate_cfg_element(
                     f"'{credential.username}' of github-config '{cfg_to_rotate.name()}' "
                     '- will not attempt rotation.'
                 )
-        except:
-            exception = sys.exception()
-            if isinstance(exception, requests.exceptions.HTTPError):
-                logger.error(
-                    f'Error when trying to refresh oAuth token: {exception}. Response from server: '
-                    f'{exception.response}'
-                )
-            else:
-                logger.error(f'Error when trying to refresh oAuth token: {exception}')
+        except requests.exceptions.HTTPError as http_error:
+            logger.error(f'Error while trying to refresh oAuth token: {http_error=}')
+            if i == 0:
+                raise
+        except Exception as e:
+            logger.error(f'Error while trying to refresh oAuth token: {e}')
 
             # we only abort here if the first rotation failed (assuming that the token was not
             # refreshed). Otherwise we keep on rotating keys/tokens to prevent the previously
diff --git a/checkmarx/client.py b/checkmarx/client.py
index 681effc0c..2d3064025 100644
--- a/checkmarx/client.py
+++ b/checkmarx/client.py
@@ -83,13 +83,14 @@ def __init__(self, checkmarx_cfg: model.checkmarx.CheckmarxConfig):
         self.routes = CheckmarxRoutes(base_url=checkmarx_cfg.base_url())
         self.config = checkmarx_cfg
         self.auth = None
+        self.session = requests.sessions.Session()
 
     def _auth(self):
         if self.auth and self.auth.is_valid():
             return self.auth
 
         creds = self.config.credentials()
-        res = requests.post(
+        res = self.session.post(
             self.routes.auth(),
             data={
                 'username': creds.qualified_username(),
@@ -99,7 +100,10 @@ def _auth(self):
                 'scope': creds.scope(),
                 'grant_type': 'password',
             },
+            timeout=(4, 31),
         )
+        res.raise_for_status()
+
         res = cxmodel.AuthResponse(**res.json())
         res.expires_at = datetime.datetime.fromtimestamp(
             datetime.datetime.now().timestamp() + res.expires_in - 10
@@ -121,7 +125,18 @@ def request(
         if 'Accept' not in headers:
             headers['Accept'] = f'application/json;v={api_version}'
 
-        res = requests.request(method=method, headers=headers, *args, **kwargs)
+        try:
+            timeout = kwargs.pop('timeout')
+        except KeyError:
+            timeout = (4, 31)
+
+        res = self.session.request(
+            method=method,
+            headers=headers,
+            timeout=timeout,
+            *args,
+            **kwargs,
+        )
 
         if not res.ok:
             msg = f'{method} request to {res.url=} failed with {res.status_code=} {res.reason=}'
diff --git a/concourse/client/api.py b/concourse/client/api.py
index 5176eae88..8a42f8fb7 100644
--- a/concourse/client/api.py
+++ b/concourse/client/api.py
@@ -105,12 +105,8 @@ class ConcourseApiBase:
     def __init__(
         self,
         routes: ConcourseApiRoutesBase,
-        request_builder: AuthenticatedRequestBuilder=None,
         verify_ssl=False,
     ):
-        if request_builder:
-            logger.warning('passing-in requests-builder is deprecated')
-
         self.routes = routes
         self.request_builder = None
         self.verify_ssl = verify_ssl
@@ -156,7 +152,8 @@ def login(self, username: str, passwd: str):
             url=login_url,
             data=form_data,
             auth=(AUTH_TOKEN_REQUEST_USER, AUTH_TOKEN_REQUEST_PWD),
-            headers={'content-type': 'application/x-www-form-urlencoded'}
+            headers={'content-type': 'application/x-www-form-urlencoded'},
+            timeout=(4, 31),
         )
         response.raise_for_status()
         auth_token = response.json()[self.AUTH_TOKEN_ATTRIBUTE_NAME]
diff --git a/ctt/util.py b/ctt/util.py
index e9d66961a..3fa12cfe6 100644
--- a/ctt/util.py
+++ b/ctt/util.py
@@ -89,6 +89,7 @@ def sign_with_signing_server(
         headers=headers,
         data=content,
         verify=root_ca_cert_path,
+        timeout=(4, 31),
     )
     response.raise_for_status()
 
diff --git a/delivery/client.py b/delivery/client.py
index 448a68652..1a2fad754 100644
--- a/delivery/client.py
+++ b/delivery/client.py
@@ -102,6 +102,7 @@ def __init__(
         routes: DeliveryServiceRoutes,
     ):
         self._routes = routes
+        self.session = requests.sessions.Session()
 
     def component_descriptor(
         self,
@@ -121,9 +122,10 @@ def component_descriptor(
         if version_filter is not None:
             params['version_filter'] = version_filter
 
-        res = requests.get(
+        res = self.session.get(
             url=self._routes.component_descriptor(),
             params=params,
+            timeout=(4, 31),
         )
 
         res.raise_for_status()
@@ -154,9 +156,10 @@ def greatest_component_versions(
         if version_filter is not None:
             params['version_filter'] = version_filter
 
-        res = requests.get(
+        res = self.session.get(
             url=self._routes.greatest_component_versions(),
             params=params,
+            timeout=(4, 31),
         )
 
         res.raise_for_status()
@@ -167,7 +170,7 @@ def upload_metadata(
         self,
         data: typing.Iterable[dso.model.ArtefactMetadata],
     ):
-        res = requests.post(
+        res = self.session.post(
             url=self._routes.upload_metadata(),
             json={'entries': [
                 dataclasses.asdict(
@@ -175,6 +178,7 @@ def upload_metadata(
                     dict_factory=ci.util.dict_to_json_factory,
                 ) for artefact_metadata in data
             ]},
+            timeout=(4, 31),
         )
 
         res.raise_for_status()
@@ -183,7 +187,7 @@ def update_metadata(
         self,
         data: typing.Iterable[dso.model.ArtefactMetadata],
     ):
-        res = requests.put(
+        res = self.session.put(
             url=self._routes.update_metadata(),
             json={'entries': [
                 dataclasses.asdict(
@@ -191,6 +195,7 @@ def update_metadata(
                     dict_factory=ci.util.dict_to_json_factory,
                 ) for artefact_metadata in data
             ]},
+            timeout=(4, 31),
         )
 
         res.raise_for_status()
@@ -257,9 +262,10 @@ def component_responsibles(
         else:
             logger.info(f'{params=}')
 
-        resp = requests.get(
+        resp = self.session.get(
             url=url,
             params=params,
+            timeout=(4, 31),
         )
 
         resp.raise_for_status()
@@ -283,8 +289,9 @@ def component_responsibles(
         return responsibles, statuses
 
     def sprints(self) -> list[dm.Sprint]:
-        raw = requests.get(
-            url=self._routes.sprint_infos(),
+        raw = self.session.get(
+            self._routes.sprint_infos(),
+            timeout=(4, 31),
         ).json()['sprints']
 
         return [
@@ -300,9 +307,10 @@ def sprint_current(self, offset: int=0, before: datetime.date=None) -> dm.Sprint
             else:
                 extra_args['before'] = before
 
-        resp = requests.get(
+        resp = self.session.get(
             url=self._routes.sprint_current(),
             params={'offset': offset, **extra_args},
+            timeout=(4, 31),
         )
 
         resp.raise_for_status()
@@ -330,10 +338,11 @@ def query_metadata_raw(
             ]
         }
 
-        res = requests.post(
+        res = self.session.post(
             url=self._routes.query_metadata(),
             json=query,
             params=params,
+            timeout=(4, 31),
         )
 
         return res.json()
@@ -341,7 +350,10 @@ def query_metadata_raw(
     def os_release_infos(self, os_id: str, absent_ok=False) -> list[dm.OsReleaseInfo]:
         url = self._routes.os_branches(os_id=os_id)
 
-        res = requests.get(url)
+        res = self.session.get(
+            url,
+            timeout=(4, 31),
+        )
 
         if not absent_ok:
             res.raise_for_status()
@@ -367,14 +379,15 @@ def components_metadata(
         '''
         url = self._routes.components_metadata()
 
-        resp = requests.get(
+        resp = self.session.get(
             url=url,
             params={
                 'name': component_name,
                 'version': component_version,
                 'type': metadata_types,
                 'select': select,
-            }
+            },
+            timeout=(4, 31),
         )
 
         resp.raise_for_status()
diff --git a/http_requests.py b/http_requests.py
index d5d4b3280..3bc3337e8 100644
--- a/http_requests.py
+++ b/http_requests.py
@@ -176,12 +176,17 @@ def _request(self,
         if 'data' in kwargs:
             if 'content-type' not in headers:
                 headers['content-type'] = 'application/x-yaml'
+        try:
+            timeout = kwargs.pop('timeout')
+        except KeyError:
+            timeout = (4, 31)
 
         result = method(
             url,
             headers=headers,
             auth=self.auth,
             verify=self.verify_ssl,
+            timeout=timeout,
             **kwargs
         )
 
diff --git a/oci/client.py b/oci/client.py
index 40f96b5bc..5803807f3 100644
--- a/oci/client.py
+++ b/oci/client.py
@@ -300,6 +300,7 @@ def _authenticate(
         res = self.session.get(
             url=url,
             verify=not self.disable_tls_validation,
+            timeout=(4, 31),
         )
 
         auth_challenge = www_authenticate.parse(res.headers.get('www-authenticate'))
@@ -339,6 +340,7 @@ def _authenticate(
             url=realm,
             verify=not self.disable_tls_validation,
             auth=auth,
+            timeout=(4, 31),
         )
 
         if not res.ok:
@@ -418,11 +420,17 @@ def _request(
             },
         )
 
+        try:
+            timeout = kwargs.pop('timeout')
+        except KeyError:
+            timeout = (4, 31)
+
         res = self.session.request(
             method=method,
             url=url,
             auth=auth,
             headers=headers,
+            timeout=timeout,
             **kwargs,
         )
         if not res.ok and warn_if_not_ok:
diff --git a/protecode/client.py b/protecode/client.py
index a1a4bb24b..ba22a81ca 100644
--- a/protecode/client.py
+++ b/protecode/client.py
@@ -165,11 +165,18 @@ def _request(self, method, *args, **kwargs):
             )
         else:
             raise NotImplementedError(auth_scheme)
+
+        try:
+            timeout = kwargs.pop('timeout')
+        except KeyError:
+            timeout = (4, 31)
+
         return partial(
             method,
             verify=self._tls_verify,
             cookies=cookies,
             headers=headers,
+            timeout=timeout,
         )(*args, **kwargs)
 
     @check_http_code