Skip to content
This repository has been archived by the owner on Mar 13, 2022. It is now read-only.

Refresh exec-based API credentials when they expire #250

Merged
merged 1 commit into from
Nov 30, 2021
Merged

Refresh exec-based API credentials when they expire #250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 16 additions & 22 deletions config/kube_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,8 @@ def _load_gcp_token(self, provider):
self._refresh_gcp_token()

self.token = "Bearer %s" % provider['config']['access-token']
if 'expiry' in provider['config']:
self.expiry = parse_rfc3339(provider['config']['expiry'])
return self.token

def _refresh_gcp_token(self):
Expand Down Expand Up @@ -483,8 +485,7 @@ def _load_from_exec_plugin(self):
status = ExecProvider(self._user['exec']).run()
if 'token' in status:
self.token = "Bearer %s" % status['token']
return True
if 'clientCertificateData' in status:
elif 'clientCertificateData' in status:
# https://kubernetes.io/docs/reference/access-authn-authz/authentication/#input-and-output-formats
# Plugin has provided certificates instead of a token.
if 'clientKeyData' not in status:
Expand All @@ -504,10 +505,13 @@ def _load_from_exec_plugin(self):
file_base_path=base_path,
base64_file_content=False,
temp_file_path=self._temp_file_path).as_file()
return True
logging.error('exec: missing token or clientCertificateData field '
'in plugin output')
return None
else:
logging.error('exec: missing token or clientCertificateData '
'field in plugin output')
return None
if 'expirationTimestamp' in status:
self.expiry = parse_rfc3339(status['expirationTimestamp'])
return True
except Exception as e:
logging.error(str(e))

Expand Down Expand Up @@ -560,25 +564,15 @@ def _load_cluster_info(self):
if 'insecure-skip-tls-verify' in self._cluster:
self.verify_ssl = not self._cluster['insecure-skip-tls-verify']

def _using_gcp_auth_provider(self):
return self._user and \
'auth-provider' in self._user and \
'name' in self._user['auth-provider'] and \
self._user['auth-provider']['name'] == 'gcp'

def _set_config(self, client_configuration):
if self._using_gcp_auth_provider():
# GCP auth tokens must be refreshed regularly, but swagger expects
# a constant token. Replace the swagger-generated client config's
# get_api_key_with_prefix method with our own to allow automatic
# token refresh.
def _gcp_get_api_key(*args):
return self._load_gcp_token(self._user['auth-provider'])
client_configuration.get_api_key_with_prefix = _gcp_get_api_key
if 'token' in self.__dict__:
# Note: this line runs for GCP auth tokens as well, but this entry
# will not be updated upon GCP token refresh.
client_configuration.api_key['authorization'] = self.token

def _refresh_api_key(client_configuration):
if ('expiry' in self.__dict__ and _is_expired(self.expiry)):
self._load_authentication()
self._set_config(client_configuration)
client_configuration.refresh_api_key_hook = _refresh_api_key
Copy link
Member

Choose a reason for hiding this comment

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

Is the indentation correct? Do we want to install the hook only when token is set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reasoning here was that only token-based authentication types would require refreshing -- other types (for example, certificate-based authentication) are expected to use stable credentials and therefore wouldn't need this hook.

However, if that's not a valid assumption, I don't think it would cause any harm to install this hook in all cases, other than perhaps running the refresh hook too often. Happy to update in that case!

# copy these keys directly from self to configuration object
keys = ['host', 'ssl_ca_cert', 'cert_file', 'key_file', 'verify_ssl']
for key in keys:
Expand Down
91 changes: 60 additions & 31 deletions config/kube_config_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
from kubernetes.client import Configuration

from .config_exception import ConfigException
from .dateutil import parse_rfc3339
from .dateutil import format_rfc3339, parse_rfc3339
from .kube_config import (ENV_KUBECONFIG_PATH_SEPARATOR, CommandTokenSource,
ConfigNode, FileOrData, KubeConfigLoader,
KubeConfigMerger, _cleanup_temp_files,
Expand Down Expand Up @@ -346,9 +346,12 @@ def test_get_with_name_on_duplicate_name(self):
class FakeConfig:

FILE_KEYS = ["ssl_ca_cert", "key_file", "cert_file"]
IGNORE_KEYS = ["refresh_api_key_hook"]

def __init__(self, token=None, **kwargs):
self.api_key = {}
# Provided by the OpenAPI-generated Configuration class
self.refresh_api_key_hook = None
if token:
self.api_key['authorization'] = token

Expand All @@ -358,6 +361,8 @@ def __eq__(self, other):
if len(self.__dict__) != len(other.__dict__):
return
for k, v in self.__dict__.items():
if k in self.IGNORE_KEYS:
continue
if k not in other.__dict__:
return
if k in self.FILE_KEYS:
Expand Down Expand Up @@ -956,17 +961,15 @@ def test_load_user_token(self):

def test_gcp_no_refresh(self):
fake_config = FakeConfig()
# swagger-generated config has this, but FakeConfig does not.
self.assertFalse(hasattr(fake_config, 'get_api_key_with_prefix'))
self.assertIsNone(fake_config.refresh_api_key_hook)
KubeConfigLoader(
config_dict=self.TEST_KUBE_CONFIG,
active_context="gcp",
get_google_credentials=lambda: _raise_exception(
"SHOULD NOT BE CALLED")).load_and_set(fake_config)
# Should now be populated with a gcp token fetcher.
self.assertIsNotNone(fake_config.get_api_key_with_prefix)
self.assertIsNotNone(fake_config.refresh_api_key_hook)
self.assertEqual(TEST_HOST, fake_config.host)
# For backwards compatibility, authorization field should still be set.
Copy link
Member

Choose a reason for hiding this comment

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

nit: why do we remove this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test tests GCP token refresh functionality -- before this patch, Configuration.get_api_key_with_prefix() was completely replaced with a GCP-specific monkeypatch. api_key['authorization'] was never used, however, it was still set to a valid token for backwards compatibility, as this comment describes.

However, this patch refactors the GCP token refresh logic to use the new refresh_api_key_hook functionality, which requires less custom code for GCP. With this patch, api_key['authorization'] is used to hold the actual current GCP access token, as it is with other authentication types -- so we still assert that this value is set, but it's no longer only for backwards compatibility -- it's used to hold the actual token. This comment is removed to avoid confusion as it refers to logic which this patch factors out.

self.assertEqual(BEARER_TOKEN_FORMAT % TEST_DATA_BASE64,
fake_config.api_key['authorization'])

Expand Down Expand Up @@ -997,7 +1000,7 @@ def cred(): return None
self.assertEqual(BEARER_TOKEN_FORMAT % TEST_ANOTHER_DATA_BASE64,
loader.token)

def test_gcp_get_api_key_with_prefix(self):
def test_gcp_refresh_api_key_hook(self):
class cred_old:
token = TEST_DATA_BASE64
expiry = DATETIME_EXPIRY_PAST
Expand All @@ -1015,15 +1018,13 @@ class cred_new:
get_google_credentials=_get_google_credentials)
loader.load_and_set(fake_config)
original_expiry = _get_expiry(loader, "expired_gcp_refresh")
# Call GCP token fetcher.
token = fake_config.get_api_key_with_prefix()
# Refresh the GCP token.
fake_config.refresh_api_key_hook(fake_config)
new_expiry = _get_expiry(loader, "expired_gcp_refresh")

self.assertTrue(new_expiry > original_expiry)
self.assertEqual(BEARER_TOKEN_FORMAT % TEST_ANOTHER_DATA_BASE64,
loader.token)
self.assertEqual(BEARER_TOKEN_FORMAT % TEST_ANOTHER_DATA_BASE64,
token)

def test_oidc_no_refresh(self):
loader = KubeConfigLoader(
Expand Down Expand Up @@ -1383,6 +1384,38 @@ def test_user_exec_auth(self, mock):
active_context="exec_cred_user").load_and_set(actual)
self.assertEqual(expected, actual)

@mock.patch('kubernetes.config.kube_config.ExecProvider.run')
def test_user_exec_auth_with_expiry(self, mock):
expired_token = "expired"
current_token = "current"
mock.side_effect = [
{
"token": expired_token,
"expirationTimestamp": format_rfc3339(DATETIME_EXPIRY_PAST)
},
{
"token": current_token,
"expirationTimestamp": format_rfc3339(DATETIME_EXPIRY_FUTURE)
}
]

fake_config = FakeConfig()
self.assertIsNone(fake_config.refresh_api_key_hook)

KubeConfigLoader(
config_dict=self.TEST_KUBE_CONFIG,
active_context="exec_cred_user").load_and_set(fake_config)
# The kube config should use the first token returned from the
# exec provider.
self.assertEqual(fake_config.api_key["authorization"],
BEARER_TOKEN_FORMAT % expired_token)
# Should now be populated with a method to refresh expired tokens.
self.assertIsNotNone(fake_config.refresh_api_key_hook)
# Refresh the token; the kube config should be updated.
fake_config.refresh_api_key_hook(fake_config)
self.assertEqual(fake_config.api_key["authorization"],
BEARER_TOKEN_FORMAT % current_token)

@mock.patch('kubernetes.config.kube_config.ExecProvider.run')
def test_user_exec_auth_certificates(self, mock):
mock.return_value = {
Expand Down Expand Up @@ -1412,7 +1445,6 @@ def test_user_cmd_path(self):
KubeConfigLoader(
config_dict=self.TEST_KUBE_CONFIG,
active_context="contexttestcmdpath").load_and_set(actual)
del actual.get_api_key_with_prefix
self.assertEqual(expected, actual)

def test_user_cmd_path_empty(self):
Expand Down Expand Up @@ -1490,31 +1522,28 @@ def test__get_kube_config_loader_dict_no_persist(self):
class TestKubernetesClientConfiguration(BaseTestCase):
# Verifies properties of kubernetes.client.Configuration.
# These tests guard against changes to the upstream configuration class,
# since GCP authorization overrides get_api_key_with_prefix to refresh its
# token regularly.
# since GCP and Exec authorization use refresh_api_key_hook to refresh
# their tokens regularly.

def test_get_api_key_with_prefix_exists(self):
self.assertTrue(hasattr(Configuration, 'get_api_key_with_prefix'))
def test_refresh_api_key_hook_exists(self):
self.assertTrue(hasattr(Configuration(), 'refresh_api_key_hook'))
Copy link
Member

Choose a reason for hiding this comment

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

Why do we use Configuration() instead of Configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is made because this patch changes the way this code interacts with the upstream Configuration class to refresh the API tokens. Before, we overrode the get_api_key_with_prefix() method, but now we override Configuration.refresh_api_key_hook, which is created in Configuration.__init__() here: https://github.com/kubernetes-client/python/blob/master/kubernetes/client/configuration.py#L99.

Because that property is only created when Configuration is initialized, we have to switch to Configuration() here.


def test_get_api_key_with_prefix_returns_token(self):
expected_token = 'expected_token'
config = Configuration()
config.api_key['authorization'] = expected_token
self.assertEqual(expected_token,
config.get_api_key_with_prefix('authorization'))

def test_auth_settings_calls_get_api_key_with_prefix(self):
def test_get_api_key_calls_refresh_api_key_hook(self):
identifier = 'authorization'
expected_token = 'expected_token'
old_token = 'old_token'
config = Configuration(
api_key={identifier: old_token},
api_key_prefix={identifier: 'Bearer'}
)

def refresh_api_key_hook(client_config):
self.assertEqual(client_config, config)
client_config.api_key[identifier] = expected_token
config.refresh_api_key_hook = refresh_api_key_hook

def fake_get_api_key_with_prefix(identifier):
self.assertEqual('authorization', identifier)
return expected_token
config = Configuration()
config.api_key['authorization'] = old_token
config.get_api_key_with_prefix = fake_get_api_key_with_prefix
self.assertEqual(expected_token,
config.auth_settings()['BearerToken']['value'])
self.assertEqual('Bearer ' + expected_token,
config.get_api_key_with_prefix(identifier))


class TestKubeConfigMerger(BaseTestCase):
Expand Down