From 05a6c7f161e01840a89c965b44a33db47b5b155b Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Wed, 1 Feb 2023 18:54:30 +0000 Subject: [PATCH 1/3] fix: Remove 3PI config url validation --- google/auth/external_account.py | 61 ------------ tests/test_aws.py | 34 ------- tests/test_external_account.py | 158 -------------------------------- tests/test_identity_pool.py | 34 ------- tests/test_pluggable.py | 34 ------- 5 files changed, 321 deletions(-) diff --git a/google/auth/external_account.py b/google/auth/external_account.py index d24b22837..646e31340 100644 --- a/google/auth/external_account.py +++ b/google/auth/external_account.py @@ -35,7 +35,6 @@ import re import six -from urllib3.util import parse_url from google.auth import _helpers from google.auth import credentials @@ -127,14 +126,6 @@ def __init__( self._default_scopes = default_scopes self._workforce_pool_user_project = workforce_pool_user_project - Credentials.validate_token_url(token_url) - if token_info_url: - Credentials.validate_token_url(token_info_url, url_type="token info") - if service_account_impersonation_url: - Credentials.validate_service_account_impersonation_url( - service_account_impersonation_url - ) - if self._client_id: self._client_auth = utils.ClientAuthentication( utils.ClientAuthType.basic, self._client_id, self._client_secret @@ -434,58 +425,6 @@ def _initialize_impersonated_credentials(self): ), ) - @staticmethod - def validate_token_url(token_url, url_type="token"): - _TOKEN_URL_PATTERNS = [ - "^[^\\.\\s\\/\\\\]+\\.sts(?:\\.mtls)?\\.googleapis\\.com$", - "^sts(?:\\.mtls)?\\.googleapis\\.com$", - "^sts\\.[^\\.\\s\\/\\\\]+(?:\\.mtls)?\\.googleapis\\.com$", - "^[^\\.\\s\\/\\\\]+\\-sts(?:\\.mtls)?\\.googleapis\\.com$", - "^sts\\-[^\\.\\s\\/\\\\]+\\.p(?:\\.mtls)?\\.googleapis\\.com$", - ] - - if not Credentials.is_valid_url(_TOKEN_URL_PATTERNS, token_url): - raise exceptions.InvalidResource( - "The provided {} URL is invalid.".format(url_type) - ) - - @staticmethod - def validate_service_account_impersonation_url(url): - _SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS = [ - "^[^\\.\\s\\/\\\\]+\\.iamcredentials\\.googleapis\\.com$", - "^iamcredentials\\.googleapis\\.com$", - "^iamcredentials\\.[^\\.\\s\\/\\\\]+\\.googleapis\\.com$", - "^[^\\.\\s\\/\\\\]+\\-iamcredentials\\.googleapis\\.com$", - "^iamcredentials\\-[^\\.\\s\\/\\\\]+\\.p\\.googleapis\\.com$", - ] - - if not Credentials.is_valid_url( - _SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, url - ): - raise exceptions.InvalidResource( - "The provided service account impersonation URL is invalid." - ) - - @staticmethod - def is_valid_url(patterns, url): - """ - Returns True if the provided URL's scheme is HTTPS and the host comforms to at least one of the provided patterns. - """ - # Check specifically for whitespcaces: - # Some python3.6 will parse the space character into %20 and pass the regex check which shouldn't be passed - if not url or len(str(url).split()) > 1: - return False - - try: - uri = parse_url(url) - except Exception: - return False - - if not uri.scheme or uri.scheme != "https" or not uri.hostname: - return False - - return any(re.compile(p).match(uri.hostname.lower()) for p in patterns) - @classmethod def from_info(cls, info, **kwargs): """Creates a Credentials instance from parsed external account info. diff --git a/tests/test_aws.py b/tests/test_aws.py index 400412660..7d87bdba2 100644 --- a/tests/test_aws.py +++ b/tests/test_aws.py @@ -1085,16 +1085,6 @@ def test_token_info_url_custom(self): assert credentials.token_info_url == (url + "/introspect") - def test_token_info_url_bad(self): - for url in INVALID_TOKEN_URLS: - with pytest.raises(ValueError) as excinfo: - self.make_credentials( - credential_source=self.CREDENTIAL_SOURCE.copy(), - token_info_url=(url + "/introspect"), - ) - - assert excinfo.match(r"The provided token info URL is invalid\.") - def test_token_info_url_negative(self): credentials = self.make_credentials( credential_source=self.CREDENTIAL_SOURCE.copy(), token_info_url=None @@ -1111,16 +1101,6 @@ def test_token_url_custom(self): assert credentials._token_url == (url + "/token") - def test_token_url_bad(self): - for url in INVALID_TOKEN_URLS: - with pytest.raises(ValueError) as excinfo: - self.make_credentials( - credential_source=self.CREDENTIAL_SOURCE.copy(), - token_url=(url + "/token"), - ) - - assert excinfo.match(r"The provided token URL is invalid\.") - def test_service_account_impersonation_url_custom(self): for url in VALID_SERVICE_ACCOUNT_IMPERSONATION_URLS: credentials = self.make_credentials( @@ -1134,20 +1114,6 @@ def test_service_account_impersonation_url_custom(self): url + SERVICE_ACCOUNT_IMPERSONATION_URL_ROUTE ) - def test_service_account_impersonation_url_bad(self): - for url in INVALID_SERVICE_ACCOUNT_IMPERSONATION_URLS: - with pytest.raises(ValueError) as excinfo: - self.make_credentials( - credential_source=self.CREDENTIAL_SOURCE.copy(), - service_account_impersonation_url=( - url + SERVICE_ACCOUNT_IMPERSONATION_URL_ROUTE - ), - ) - - assert excinfo.match( - r"The provided service account impersonation URL is invalid\." - ) - def test_retrieve_subject_token_missing_region_url(self): # When AWS_REGION envvar is not available, region_url is required for # determining the current AWS region. diff --git a/tests/test_external_account.py b/tests/test_external_account.py index 78a272b6a..c8900a493 100644 --- a/tests/test_external_account.py +++ b/tests/test_external_account.py @@ -65,101 +65,6 @@ "//iam.googleapis.com/locations//workforcePool/pool-id/providers/provider-id", ] -VALID_TOKEN_URLS = [ - "https://sts.googleapis.com", - "https://sts.mtls.googleapis.com", - "https://us-east-1.sts.googleapis.com", - "https://us-east-1.sts.mtls.googleapis.com", - "https://US-EAST-1.sts.googleapis.com", - "https://sts.us-east-1.googleapis.com", - "https://sts.US-WEST-1.googleapis.com", - "https://us-east-1-sts.googleapis.com", - "https://US-WEST-1-sts.googleapis.com", - "https://US-WEST-1-sts.mtls.googleapis.com", - "https://us-west-1-sts.googleapis.com/path?query", - "https://sts-us-east-1.p.googleapis.com", - "https://sts-us-east-1.p.mtls.googleapis.com", -] -INVALID_TOKEN_URLS = [ - "https://iamcredentials.googleapis.com", - "https://mtls.iamcredentials.googleapis.com", - "sts.googleapis.com", - "mtls.sts.googleapis.com", - "mtls.googleapis.com", - "https://", - "http://sts.googleapis.com", - "https://st.s.googleapis.com", - "https://us-eas\t-1.sts.googleapis.com", - "https:/us-east-1.sts.googleapis.com", - "https:/us-east-1.mtls.sts.googleapis.com", - "https://US-WE/ST-1-sts.googleapis.com", - "https://sts-us-east-1.googleapis.com", - "https://sts-US-WEST-1.googleapis.com", - "testhttps://us-east-1.sts.googleapis.com", - "https://us-east-1.sts.googleapis.comevil.com", - "https://us-east-1.us-east-1.sts.googleapis.com", - "https://us-ea.s.t.sts.googleapis.com", - "https://sts.googleapis.comevil.com", - "hhttps://us-east-1.sts.googleapis.com", - "https://us- -1.sts.googleapis.com", - "https://-sts.googleapis.com", - "https://-mtls.googleapis.com", - "https://us-east-1.sts.googleapis.com.evil.com", - "https://sts.pgoogleapis.com", - "https://p.googleapis.com", - "https://sts.p.com", - "https://sts.p.mtls.com", - "http://sts.p.googleapis.com", - "https://xyz-sts.p.googleapis.com", - "https://sts-xyz.123.p.googleapis.com", - "https://sts-xyz.p1.googleapis.com", - "https://sts-xyz.p.foo.com", - "https://sts-xyz.p.foo.googleapis.com", - "https://sts-xyz.mtls.p.foo.googleapis.com", - "https://sts-xyz.p.mtls.foo.googleapis.com", -] -VALID_SERVICE_ACCOUNT_IMPERSONATION_URLS = [ - "https://iamcredentials.googleapis.com", - "https://us-east-1.iamcredentials.googleapis.com", - "https://US-EAST-1.iamcredentials.googleapis.com", - "https://iamcredentials.us-east-1.googleapis.com", - "https://iamcredentials.US-WEST-1.googleapis.com", - "https://us-east-1-iamcredentials.googleapis.com", - "https://US-WEST-1-iamcredentials.googleapis.com", - "https://us-west-1-iamcredentials.googleapis.com/path?query", - "https://iamcredentials-us-east-1.p.googleapis.com", -] -INVALID_SERVICE_ACCOUNT_IMPERSONATION_URLS = [ - "https://sts.googleapis.com", - "iamcredentials.googleapis.com", - "https://", - "http://iamcredentials.googleapis.com", - "https://iamcre.dentials.googleapis.com", - "https://us-eas\t-1.iamcredentials.googleapis.com", - "https:/us-east-1.iamcredentials.googleapis.com", - "https://US-WE/ST-1-iamcredentials.googleapis.com", - "https://iamcredentials-us-east-1.googleapis.com", - "https://iamcredentials-US-WEST-1.googleapis.com", - "testhttps://us-east-1.iamcredentials.googleapis.com", - "https://us-east-1.iamcredentials.googleapis.comevil.com", - "https://us-east-1.us-east-1.iamcredentials.googleapis.com", - "https://us-ea.s.t.iamcredentials.googleapis.com", - "https://iamcredentials.googleapis.comevil.com", - "hhttps://us-east-1.iamcredentials.googleapis.com", - "https://us- -1.iamcredentials.googleapis.com", - "https://-iamcredentials.googleapis.com", - "https://us-east-1.iamcredentials.googleapis.com.evil.com", - "https://iamcredentials.pgoogleapis.com", - "https://p.googleapis.com", - "https://iamcredentials.p.com", - "http://iamcredentials.p.googleapis.com", - "https://xyz-iamcredentials.p.googleapis.com", - "https://iamcredentials-xyz.123.p.googleapis.com", - "https://iamcredentials-xyz.p1.googleapis.com", - "https://iamcredentials-xyz.p.foo.com", - "https://iamcredentials-xyz.p.foo.googleapis.com", -] - class CredentialsImpl(external_account.Credentials): def __init__(self, **kwargs): @@ -350,44 +255,6 @@ def assert_resource_manager_request_kwargs( assert request_kwargs["headers"] == headers assert "body" not in request_kwargs - def test_valid_token_url_shall_pass_validation(self): - valid_urls = VALID_TOKEN_URLS - - for url in valid_urls: - # A valid url shouldn't throw exception and a None value should be returned - external_account.Credentials.validate_token_url(url) - - def test_invalid_token_url_shall_throw_exceptions(self): - invalid_urls = INVALID_TOKEN_URLS - - for url in invalid_urls: - # An invalid url should throw a ValueError exception - with pytest.raises(ValueError) as excinfo: - external_account.Credentials.validate_token_url(url) - - assert excinfo.match("The provided token URL is invalid.") - - def test_valid_service_account_impersonation_url_shall_pass_validation(self): - valid_urls = VALID_SERVICE_ACCOUNT_IMPERSONATION_URLS - - for url in valid_urls: - # A valid url shouldn't throw exception and a None value should be returned - external_account.Credentials.validate_service_account_impersonation_url(url) - - def test_invalid_service_account_impersonate_url_shall_throw_exceptions(self): - invalid_urls = INVALID_SERVICE_ACCOUNT_IMPERSONATION_URLS - - for url in invalid_urls: - # An invalid url should throw a ValueError exception - with pytest.raises(ValueError) as excinfo: - external_account.Credentials.validate_service_account_impersonation_url( - url - ) - - assert excinfo.match( - "The provided service account impersonation URL is invalid." - ) - def test_default_state(self): credentials = self.make_credentials( service_account_impersonation_url=self.SERVICE_ACCOUNT_IMPERSONATION_URL @@ -409,31 +276,6 @@ def test_default_state(self): # Token info url not set yet assert not credentials.token_info_url - def test_invalid_token_url(self): - with pytest.raises(ValueError) as excinfo: - CredentialsImpl( - audience=self.AUDIENCE, - subject_token_type=self.SUBJECT_TOKEN_TYPE, - token_url="https:///v1/token", - credential_source=self.CREDENTIAL_SOURCE, - ) - - assert excinfo.match("The provided token URL is invalid.") - - def test_invalid_service_account_impersonate_url(self): - with pytest.raises(ValueError) as excinfo: - CredentialsImpl( - audience=self.AUDIENCE, - subject_token_type=self.SUBJECT_TOKEN_TYPE, - token_url=self.TOKEN_URL, - credential_source=self.CREDENTIAL_SOURCE, - service_account_impersonation_url=12345, # create an exception by sending to parse url - ) - - assert excinfo.match( - "The provided service account impersonation URL is invalid." - ) - def test_nonworkforce_with_workforce_pool_user_project(self): with pytest.raises(ValueError) as excinfo: CredentialsImpl( diff --git a/tests/test_identity_pool.py b/tests/test_identity_pool.py index 0b0156eb0..6651f0b5c 100644 --- a/tests/test_identity_pool.py +++ b/tests/test_identity_pool.py @@ -759,16 +759,6 @@ def test_token_info_url_custom(self): assert credentials.token_info_url == url + "/introspect" - def test_token_info_url_bad(self): - for url in INVALID_TOKEN_URLS: - with pytest.raises(ValueError) as excinfo: - self.make_credentials( - credential_source=self.CREDENTIAL_SOURCE_JSON.copy(), - token_info_url=(url + "/introspect"), - ) - - assert excinfo.match(r"The provided token info URL is invalid.") - def test_token_info_url_negative(self): credentials = self.make_credentials( credential_source=self.CREDENTIAL_SOURCE_JSON.copy(), token_info_url=None @@ -785,16 +775,6 @@ def test_token_url_custom(self): assert credentials._token_url == (url + "/token") - def test_token_url_bad(self): - for url in INVALID_TOKEN_URLS: - with pytest.raises(ValueError) as excinfo: - self.make_credentials( - credential_source=self.CREDENTIAL_SOURCE_JSON.copy(), - token_url=(url + "/token"), - ) - - assert excinfo.match(r"The provided token URL is invalid\.") - def test_service_account_impersonation_url_custom(self): for url in VALID_SERVICE_ACCOUNT_IMPERSONATION_URLS: credentials = self.make_credentials( @@ -808,20 +788,6 @@ def test_service_account_impersonation_url_custom(self): url + SERVICE_ACCOUNT_IMPERSONATION_URL_ROUTE ) - def test_service_account_impersonation_url_bad(self): - for url in INVALID_SERVICE_ACCOUNT_IMPERSONATION_URLS: - with pytest.raises(ValueError) as excinfo: - self.make_credentials( - credential_source=self.CREDENTIAL_SOURCE_JSON.copy(), - service_account_impersonation_url=( - url + SERVICE_ACCOUNT_IMPERSONATION_URL_ROUTE - ), - ) - - assert excinfo.match( - r"The provided service account impersonation URL is invalid\." - ) - def test_refresh_text_file_success_without_impersonation_ignore_default_scopes( self, ): diff --git a/tests/test_pluggable.py b/tests/test_pluggable.py index cd553da83..e9b3d9a86 100644 --- a/tests/test_pluggable.py +++ b/tests/test_pluggable.py @@ -413,16 +413,6 @@ def test_token_info_url_custom(self): assert credentials.token_info_url == url + "/introspect" - def test_token_info_url_bad(self): - for url in INVALID_TOKEN_URLS: - with pytest.raises(ValueError) as excinfo: - self.make_pluggable( - credential_source=self.CREDENTIAL_SOURCE.copy(), - token_info_url=(url + "/introspect"), - ) - - assert excinfo.match(r"The provided token info URL is invalid.") - def test_token_info_url_negative(self): credentials = self.make_pluggable( credential_source=self.CREDENTIAL_SOURCE.copy(), token_info_url=None @@ -439,16 +429,6 @@ def test_token_url_custom(self): assert credentials._token_url == (url + "/token") - def test_token_url_bad(self): - for url in INVALID_TOKEN_URLS: - with pytest.raises(ValueError) as excinfo: - self.make_pluggable( - credential_source=self.CREDENTIAL_SOURCE.copy(), - token_url=(url + "/token"), - ) - - assert excinfo.match(r"The provided token URL is invalid\.") - def test_service_account_impersonation_url_custom(self): for url in VALID_SERVICE_ACCOUNT_IMPERSONATION_URLS: credentials = self.make_pluggable( @@ -462,20 +442,6 @@ def test_service_account_impersonation_url_custom(self): url + SERVICE_ACCOUNT_IMPERSONATION_URL_ROUTE ) - def test_service_account_impersonation_url_bad(self): - for url in INVALID_SERVICE_ACCOUNT_IMPERSONATION_URLS: - with pytest.raises(ValueError) as excinfo: - self.make_pluggable( - credential_source=self.CREDENTIAL_SOURCE.copy(), - service_account_impersonation_url=( - url + SERVICE_ACCOUNT_IMPERSONATION_URL_ROUTE - ), - ) - - assert excinfo.match( - r"The provided service account impersonation URL is invalid\." - ) - @mock.patch.dict(os.environ, {"GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES": "1"}) def test_retrieve_subject_token_successfully(self, tmpdir): ACTUAL_CREDENTIAL_SOURCE_EXECUTABLE_OUTPUT_FILE = tmpdir.join( From 12be31b8284d9b82380e28be5df07087cd2f553a Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Fri, 3 Feb 2023 18:17:25 +0000 Subject: [PATCH 2/3] add security consideration in doc --- docs/user-guide.rst | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/user-guide.rst b/docs/user-guide.rst index 682b58a76..a6d5d8474 100644 --- a/docs/user-guide.rst +++ b/docs/user-guide.rst @@ -823,6 +823,16 @@ in the target project. Using `ImpersonatedCredentials` will allow the source_cre to assume the identity of a target_principal that does have access. +Security considerations on configuration External and Impersonated credentials +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ + +Note that this library does not perform any validation on the token_url, +token_info_url, or service_account_impersonation_url fields of the credential +configuration. It is not recommended to use a credential configuration that you +did not generate with the gcloud CLI unless you verify that the URL fields point +to a googleapis.com domain. + + Downscoped credentials ++++++++++++++++++++++ From 5ccd727832804b109aa521c63e0eb4bb87e797af Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Tue, 7 Feb 2023 23:53:44 +0000 Subject: [PATCH 3/3] Break the security consideration into workload and woorkforce section --- docs/user-guide.rst | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/docs/user-guide.rst b/docs/user-guide.rst index a6d5d8474..0cb119127 100644 --- a/docs/user-guide.rst +++ b/docs/user-guide.rst @@ -548,6 +548,16 @@ For AWS providers, use :meth:`aws.Credentials.from_info ['https://www.googleapis.com/auth/cloud-platform']) +Security considerations +~~~~~~~~~~~~~~~~~~~~~~~ + +Note that this library does not perform any validation on the token_url, +token_info_url, or service_account_impersonation_url fields of the credential +configuration. It is not recommended to use a credential configuration that you +did not generate with the gcloud CLI unless you verify that the URL fields point +to a googleapis.com domain. + + External credentials (Workforce identity federation) ++++++++++++++++++++++++++++++++++++++++++++++++++++ @@ -793,6 +803,13 @@ Cloud resources from an OIDC or SAML provider. https://cloud.google.com/iam/docs/workforce-identity-federation#workforce-pools-user-project +Note that this library does not perform any validation on the token_url, +token_info_url, or service_account_impersonation_url fields of the credential +configuration. It is not recommended to use a credential configuration that you +did not generate with the gcloud CLI unless you verify that the URL fields point +to a googleapis.com domain. + + Impersonated credentials ++++++++++++++++++++++++ @@ -823,16 +840,6 @@ in the target project. Using `ImpersonatedCredentials` will allow the source_cre to assume the identity of a target_principal that does have access. -Security considerations on configuration External and Impersonated credentials -++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ - -Note that this library does not perform any validation on the token_url, -token_info_url, or service_account_impersonation_url fields of the credential -configuration. It is not recommended to use a credential configuration that you -did not generate with the gcloud CLI unless you verify that the URL fields point -to a googleapis.com domain. - - Downscoped credentials ++++++++++++++++++++++