From 3c31c758ad14f1f50165dd6d08f8c7b8042f56e9 Mon Sep 17 00:00:00 2001 From: Jin Date: Fri, 22 Apr 2022 13:33:21 -0700 Subject: [PATCH 01/10] [Bug 193694420] adding validation of token url and service account impersonation url (#1) [b-193694420] adding url validation for token url and service account impersonation url * adding coverage of empty hostname to invalid token url * Add more tests to enlarge the coverage Co-authored-by: Jin Qin --- google/auth/external_account.py | 48 ++++++++++++++++++++++++++++++++ tests/test_external_account.py | 49 ++++++++++++++++++++++++++++++++- 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/google/auth/external_account.py b/google/auth/external_account.py index cbd0baf4e..a6716b79b 100644 --- a/google/auth/external_account.py +++ b/google/auth/external_account.py @@ -34,6 +34,7 @@ import re import six +from urllib.parse import urlparse from google.auth import _helpers from google.auth import credentials @@ -51,6 +52,21 @@ # Cloud resource manager URL used to retrieve project information. _CLOUD_RESOURCE_MANAGER = "https://cloudresourcemanager.googleapis.com/v1/projects/" +# Token url patterns +_TOKEN_URL_PATTERNS = [ + "^[^\\.\\s\\/\\\\]+\\.sts\\.googleapis\\.com$", + "^sts\\.googleapis\\.com$", + "^sts\\.[^\\.\\s\\/\\\\]+\\.googleapis\\.com$", + "^[^\\.\\s\\/\\\\]+\\-sts\\.googleapis\\.com$" +] + +# Service account impersonation url patterns +_SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS = [ + "^[^\\.\\s\\/\\\\]+\\.iamcredentials\\.googleapis\\.com$", + "^iamcredentials\\.googleapis\\.com$", + "^iamcredentials\\.[^\\.\\s\\/\\\\]+\\.googleapis\\.com$", + "^[^\\.\\s\\/\\\\]+\\-iamcredentials\\.googleapis\\.com$" +] @six.add_metaclass(abc.ABCMeta) class Credentials(credentials.Scoped, credentials.CredentialsWithQuotaProject): @@ -114,6 +130,10 @@ def __init__( self._default_scopes = default_scopes self._workforce_pool_user_project = workforce_pool_user_project + Credentials.validate_token_url(token_url) + 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 @@ -413,3 +433,31 @@ def _initialize_impersonated_credentials(self): quota_project_id=self._quota_project_id, iam_endpoint_override=self._service_account_impersonation_url, ) + + @staticmethod + def validate_token_url(token_url): + if not Credentials.is_valid_url(_TOKEN_URL_PATTERNS, token_url): + raise ValueError("The provided token URL is invalid.") + + @staticmethod + def validate_service_account_impersonation_url(url): + if not Credentials.is_valid_url(_SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, url): + raise ValueError("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. + """ + try: + uri = urlparse(url) + except: + return False + + if not uri.scheme or uri.scheme != "https": + return False + + if not uri.hostname: + return False + + return any(re.compile(p).match(uri.hostname.lower()) for p in patterns) diff --git a/tests/test_external_account.py b/tests/test_external_account.py index 3897aef15..a8c644c74 100644 --- a/tests/test_external_account.py +++ b/tests/test_external_account.py @@ -275,9 +275,29 @@ 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 url doesn't throw exception, a None value should be return + assert not external_account.Credentials.validate_token_url(self.TOKEN_URL) + + def test_token_url_pattern_matching(self): + # matching *.sts.googleapis.com + assert external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https://auth.sts.googleapis.com/v1/token") + # matching sts.googleapis.com + assert external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https://sts.googleapis.com/v1/token") + # matching sts.*.googleapis.com + assert external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https://sts.auth.googleapis.com/v1/token") + # matching *-sts.googleapis.com + assert external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https://auth-sts.googleapis.com/v1/token") + # invalid url cannot match + assert not external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https:///v1/token") + assert not external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https://some-invalid-url/v1/token") + def test_default_state(self): - credentials = self.make_credentials() + credentials = self.make_credentials(service_account_impersonation_url=self.SERVICE_ACCOUNT_IMPERSONATION_URL) + # Token url and service account impersonation url should + assert credentials._token_url + assert credentials._service_account_impersonation_url # Not token acquired yet assert not credentials.token assert not credentials.valid @@ -289,6 +309,33 @@ def test_default_state(self): assert credentials.requires_scopes assert not credentials.quota_project_id + 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( From 5682255ccbcdd96ec5b42d6a99949cf2e6a09b9b Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Fri, 22 Apr 2022 16:24:38 -0700 Subject: [PATCH 02/10] adding coverage for service account impersonate url validation --- tests/test_external_account.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/test_external_account.py b/tests/test_external_account.py index a8c644c74..c7cdb1b05 100644 --- a/tests/test_external_account.py +++ b/tests/test_external_account.py @@ -292,6 +292,27 @@ def test_token_url_pattern_matching(self): assert not external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https:///v1/token") assert not external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https://some-invalid-url/v1/token") + def test_service_account_impersonation_url_matching(self): + # matching *.iamcredentials.googleapis.com + assert external_account.Credentials.is_valid_url(external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, + "https://fooauth.iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken" + ) + # matching iamcredentials.googleapis.com + assert external_account.Credentials.is_valid_url(external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, + "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken" + ) + # matching iamcredentials.*.googleapis.com + assert external_account.Credentials.is_valid_url(external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, + "https://iamcredentials.fooauth.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken" + ) + # matching *-iamcredentials.googleapis.com + assert external_account.Credentials.is_valid_url(external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, + "https://us-east1-iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken" + ) + # invalid url cannot match + assert not external_account.Credentials.is_valid_url(external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, "https:///v1/accesstoken") + assert not external_account.Credentials.is_valid_url(external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, "https://some-invalid-url/v1") + def test_default_state(self): credentials = self.make_credentials(service_account_impersonation_url=self.SERVICE_ACCOUNT_IMPERSONATION_URL) From d5beae75e4dd25212bf523063db3dc407a8876f4 Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Fri, 22 Apr 2022 16:51:05 -0700 Subject: [PATCH 03/10] using urllib3 instead of urllib since it's the project test requirements included --- google/auth/external_account.py | 4 ++-- tests/test_external_account.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/google/auth/external_account.py b/google/auth/external_account.py index a6716b79b..154e45d85 100644 --- a/google/auth/external_account.py +++ b/google/auth/external_account.py @@ -34,7 +34,7 @@ import re import six -from urllib.parse import urlparse +from urllib3.util import parse_url from google.auth import _helpers from google.auth import credentials @@ -450,7 +450,7 @@ 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. """ try: - uri = urlparse(url) + uri = parse_url(url) except: return False diff --git a/tests/test_external_account.py b/tests/test_external_account.py index c7cdb1b05..9eb3cc45f 100644 --- a/tests/test_external_account.py +++ b/tests/test_external_account.py @@ -316,7 +316,7 @@ def test_service_account_impersonation_url_matching(self): def test_default_state(self): credentials = self.make_credentials(service_account_impersonation_url=self.SERVICE_ACCOUNT_IMPERSONATION_URL) - # Token url and service account impersonation url should + # Token url and service account impersonation url should be set assert credentials._token_url assert credentials._service_account_impersonation_url # Not token acquired yet From 32f0c9e9d9394959ab7e14e4443d6ca6f45bf369 Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Tue, 26 Apr 2022 22:12:11 +0000 Subject: [PATCH 04/10] fix format --- google/auth/external_account.py | 21 ++++++---- tests/test_external_account.py | 73 ++++++++++++++++++++++----------- 2 files changed, 63 insertions(+), 31 deletions(-) diff --git a/google/auth/external_account.py b/google/auth/external_account.py index 154e45d85..2a427cbcf 100644 --- a/google/auth/external_account.py +++ b/google/auth/external_account.py @@ -57,7 +57,7 @@ "^[^\\.\\s\\/\\\\]+\\.sts\\.googleapis\\.com$", "^sts\\.googleapis\\.com$", "^sts\\.[^\\.\\s\\/\\\\]+\\.googleapis\\.com$", - "^[^\\.\\s\\/\\\\]+\\-sts\\.googleapis\\.com$" + "^[^\\.\\s\\/\\\\]+\\-sts\\.googleapis\\.com$", ] # Service account impersonation url patterns @@ -65,9 +65,10 @@ "^[^\\.\\s\\/\\\\]+\\.iamcredentials\\.googleapis\\.com$", "^iamcredentials\\.googleapis\\.com$", "^iamcredentials\\.[^\\.\\s\\/\\\\]+\\.googleapis\\.com$", - "^[^\\.\\s\\/\\\\]+\\-iamcredentials\\.googleapis\\.com$" + "^[^\\.\\s\\/\\\\]+\\-iamcredentials\\.googleapis\\.com$", ] + @six.add_metaclass(abc.ABCMeta) class Credentials(credentials.Scoped, credentials.CredentialsWithQuotaProject): """Base class for all external account credentials. @@ -132,7 +133,9 @@ def __init__( Credentials.validate_token_url(token_url) if service_account_impersonation_url: - Credentials.validate_service_account_impersonation_url(service_account_impersonation_url) + Credentials.validate_service_account_impersonation_url( + service_account_impersonation_url + ) if self._client_id: self._client_auth = utils.ClientAuthentication( @@ -433,7 +436,7 @@ def _initialize_impersonated_credentials(self): quota_project_id=self._quota_project_id, iam_endpoint_override=self._service_account_impersonation_url, ) - + @staticmethod def validate_token_url(token_url): if not Credentials.is_valid_url(_TOKEN_URL_PATTERNS, token_url): @@ -441,8 +444,12 @@ def validate_token_url(token_url): @staticmethod def validate_service_account_impersonation_url(url): - if not Credentials.is_valid_url(_SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, url): - raise ValueError("The provided service account impersonation URL is invalid.") + if not Credentials.is_valid_url( + _SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, url + ): + raise ValueError( + "The provided service account impersonation URL is invalid." + ) @staticmethod def is_valid_url(patterns, url): @@ -451,7 +458,7 @@ def is_valid_url(patterns, url): """ try: uri = parse_url(url) - except: + except Exception: return False if not uri.scheme or uri.scheme != "https": diff --git a/tests/test_external_account.py b/tests/test_external_account.py index 9eb3cc45f..079cb8827 100644 --- a/tests/test_external_account.py +++ b/tests/test_external_account.py @@ -278,43 +278,70 @@ def assert_resource_manager_request_kwargs( def test_valid_token_url_shall_pass_validation(self): # valid url doesn't throw exception, a None value should be return assert not external_account.Credentials.validate_token_url(self.TOKEN_URL) - + def test_token_url_pattern_matching(self): # matching *.sts.googleapis.com - assert external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https://auth.sts.googleapis.com/v1/token") + assert external_account.Credentials.is_valid_url( + external_account._TOKEN_URL_PATTERNS, + "https://auth.sts.googleapis.com/v1/token", + ) # matching sts.googleapis.com - assert external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https://sts.googleapis.com/v1/token") + assert external_account.Credentials.is_valid_url( + external_account._TOKEN_URL_PATTERNS, "https://sts.googleapis.com/v1/token" + ) # matching sts.*.googleapis.com - assert external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https://sts.auth.googleapis.com/v1/token") + assert external_account.Credentials.is_valid_url( + external_account._TOKEN_URL_PATTERNS, + "https://sts.auth.googleapis.com/v1/token", + ) # matching *-sts.googleapis.com - assert external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https://auth-sts.googleapis.com/v1/token") + assert external_account.Credentials.is_valid_url( + external_account._TOKEN_URL_PATTERNS, + "https://auth-sts.googleapis.com/v1/token", + ) # invalid url cannot match - assert not external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https:///v1/token") - assert not external_account.Credentials.is_valid_url(external_account._TOKEN_URL_PATTERNS, "https://some-invalid-url/v1/token") + assert not external_account.Credentials.is_valid_url( + external_account._TOKEN_URL_PATTERNS, "https:///v1/token" + ) + assert not external_account.Credentials.is_valid_url( + external_account._TOKEN_URL_PATTERNS, "https://some-invalid-url/v1/token" + ) def test_service_account_impersonation_url_matching(self): # matching *.iamcredentials.googleapis.com - assert external_account.Credentials.is_valid_url(external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, - "https://fooauth.iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken" + assert external_account.Credentials.is_valid_url( + external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, + "https://fooauth.iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken", ) # matching iamcredentials.googleapis.com - assert external_account.Credentials.is_valid_url(external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, - "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken" + assert external_account.Credentials.is_valid_url( + external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, + "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken", ) # matching iamcredentials.*.googleapis.com - assert external_account.Credentials.is_valid_url(external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, - "https://iamcredentials.fooauth.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken" + assert external_account.Credentials.is_valid_url( + external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, + "https://iamcredentials.fooauth.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken", ) # matching *-iamcredentials.googleapis.com - assert external_account.Credentials.is_valid_url(external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, - "https://us-east1-iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken" + assert external_account.Credentials.is_valid_url( + external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, + "https://us-east1-iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken", ) # invalid url cannot match - assert not external_account.Credentials.is_valid_url(external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, "https:///v1/accesstoken") - assert not external_account.Credentials.is_valid_url(external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, "https://some-invalid-url/v1") + assert not external_account.Credentials.is_valid_url( + external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, + "https:///v1/accesstoken", + ) + assert not external_account.Credentials.is_valid_url( + external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, + "https://some-invalid-url/v1", + ) def test_default_state(self): - credentials = self.make_credentials(service_account_impersonation_url=self.SERVICE_ACCOUNT_IMPERSONATION_URL) + credentials = self.make_credentials( + service_account_impersonation_url=self.SERVICE_ACCOUNT_IMPERSONATION_URL + ) # Token url and service account impersonation url should be set assert credentials._token_url @@ -336,12 +363,10 @@ def test_invalid_token_url(self): audience=self.AUDIENCE, subject_token_type=self.SUBJECT_TOKEN_TYPE, token_url="https:///v1/token", - credential_source=self.CREDENTIAL_SOURCE + credential_source=self.CREDENTIAL_SOURCE, ) - assert excinfo.match( - "The provided token URL is invalid." - ) + assert excinfo.match("The provided token URL is invalid.") def test_invalid_service_account_impersonate_url(self): with pytest.raises(ValueError) as excinfo: @@ -350,9 +375,9 @@ def test_invalid_service_account_impersonate_url(self): 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 + service_account_impersonation_url=12345, # create an exception by sending to parse url ) - + assert excinfo.match( "The provided service account impersonation URL is invalid." ) From ba78e441235100e3bc98905549142200f00886dc Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Wed, 27 Apr 2022 23:38:32 +0000 Subject: [PATCH 05/10] addressing comments --- google/auth/external_account.py | 35 +++---- tests/test_external_account.py | 157 +++++++++++++++++++------------- 2 files changed, 111 insertions(+), 81 deletions(-) diff --git a/google/auth/external_account.py b/google/auth/external_account.py index 2a427cbcf..d8e0cae2c 100644 --- a/google/auth/external_account.py +++ b/google/auth/external_account.py @@ -52,22 +52,6 @@ # Cloud resource manager URL used to retrieve project information. _CLOUD_RESOURCE_MANAGER = "https://cloudresourcemanager.googleapis.com/v1/projects/" -# Token url patterns -_TOKEN_URL_PATTERNS = [ - "^[^\\.\\s\\/\\\\]+\\.sts\\.googleapis\\.com$", - "^sts\\.googleapis\\.com$", - "^sts\\.[^\\.\\s\\/\\\\]+\\.googleapis\\.com$", - "^[^\\.\\s\\/\\\\]+\\-sts\\.googleapis\\.com$", -] - -# Service account impersonation url patterns -_SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS = [ - "^[^\\.\\s\\/\\\\]+\\.iamcredentials\\.googleapis\\.com$", - "^iamcredentials\\.googleapis\\.com$", - "^iamcredentials\\.[^\\.\\s\\/\\\\]+\\.googleapis\\.com$", - "^[^\\.\\s\\/\\\\]+\\-iamcredentials\\.googleapis\\.com$", -] - @six.add_metaclass(abc.ABCMeta) class Credentials(credentials.Scoped, credentials.CredentialsWithQuotaProject): @@ -439,11 +423,25 @@ def _initialize_impersonated_credentials(self): @staticmethod def validate_token_url(token_url): + _TOKEN_URL_PATTERNS = [ + "^[^\\.\\s\\/\\\\]+\\.sts\\.googleapis\\.com$", + "^sts\\.googleapis\\.com$", + "^sts\\.[^\\.\\s\\/\\\\]+\\.googleapis\\.com$", + "^[^\\.\\s\\/\\\\]+\\-sts\\.googleapis\\.com$", + ] + if not Credentials.is_valid_url(_TOKEN_URL_PATTERNS, token_url): raise ValueError("The provided token URL is invalid.") @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$", + ] + if not Credentials.is_valid_url( _SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, url ): @@ -461,10 +459,7 @@ def is_valid_url(patterns, url): except Exception: return False - if not uri.scheme or uri.scheme != "https": - return False - - if not uri.hostname: + 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) diff --git a/tests/test_external_account.py b/tests/test_external_account.py index 079cb8827..95f481127 100644 --- a/tests/test_external_account.py +++ b/tests/test_external_account.py @@ -276,67 +276,102 @@ def assert_resource_manager_request_kwargs( assert "body" not in request_kwargs def test_valid_token_url_shall_pass_validation(self): - # valid url doesn't throw exception, a None value should be return - assert not external_account.Credentials.validate_token_url(self.TOKEN_URL) - - def test_token_url_pattern_matching(self): - # matching *.sts.googleapis.com - assert external_account.Credentials.is_valid_url( - external_account._TOKEN_URL_PATTERNS, - "https://auth.sts.googleapis.com/v1/token", - ) - # matching sts.googleapis.com - assert external_account.Credentials.is_valid_url( - external_account._TOKEN_URL_PATTERNS, "https://sts.googleapis.com/v1/token" - ) - # matching sts.*.googleapis.com - assert external_account.Credentials.is_valid_url( - external_account._TOKEN_URL_PATTERNS, - "https://sts.auth.googleapis.com/v1/token", - ) - # matching *-sts.googleapis.com - assert external_account.Credentials.is_valid_url( - external_account._TOKEN_URL_PATTERNS, - "https://auth-sts.googleapis.com/v1/token", - ) - # invalid url cannot match - assert not external_account.Credentials.is_valid_url( - external_account._TOKEN_URL_PATTERNS, "https:///v1/token" - ) - assert not external_account.Credentials.is_valid_url( - external_account._TOKEN_URL_PATTERNS, "https://some-invalid-url/v1/token" - ) - - def test_service_account_impersonation_url_matching(self): - # matching *.iamcredentials.googleapis.com - assert external_account.Credentials.is_valid_url( - external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, - "https://fooauth.iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken", - ) - # matching iamcredentials.googleapis.com - assert external_account.Credentials.is_valid_url( - external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, - "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken", - ) - # matching iamcredentials.*.googleapis.com - assert external_account.Credentials.is_valid_url( - external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, - "https://iamcredentials.fooauth.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken", - ) - # matching *-iamcredentials.googleapis.com - assert external_account.Credentials.is_valid_url( - external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, - "https://us-east1-iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/foo@google.com:generateAccessToken", - ) - # invalid url cannot match - assert not external_account.Credentials.is_valid_url( - external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, - "https:///v1/accesstoken", - ) - assert not external_account.Credentials.is_valid_url( - external_account._SERVICE_ACCOUNT_IMPERSONATION_URL_PATTERNS, - "https://some-invalid-url/v1", - ) + valid_urls = [ + "https://sts.googleapis.com", + "https://us-east-1.sts.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.googleapis.com/path?query", + ] + + for url in valid_urls: + # A valid url shouldn't throw exception and a None value should be returned + assert not external_account.Credentials.validate_token_url(url) + + def test_invalid_token_url_shall_throw_exceptions(self): + invalid_urls = [ + "https://iamcredentials.googleapis.com", + "sts.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-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://us-east-1.sts.googleapis.com.evil.com", + ] + + 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 = [ + "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", + ] + + for url in valid_urls: + # A valid url shouldn't throw exception and a None value should be returned + assert not external_account.Credentials.validate_service_account_impersonation_url( + url + ) + + def test_invalid_service_account_impersonate_url_shall_throw_exceptions(self): + invalid_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", + ] + + 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( From 9c68168c4ddd79c1202a16051afefd635a835e8c Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Thu, 28 Apr 2022 18:02:35 +0000 Subject: [PATCH 06/10] remove redundant --- tests/test_external_account.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/test_external_account.py b/tests/test_external_account.py index 95f481127..7457d5891 100644 --- a/tests/test_external_account.py +++ b/tests/test_external_account.py @@ -289,7 +289,7 @@ def test_valid_token_url_shall_pass_validation(self): for url in valid_urls: # A valid url shouldn't throw exception and a None value should be returned - assert not external_account.Credentials.validate_token_url(url) + external_account.Credentials.validate_token_url(url) def test_invalid_token_url_shall_throw_exceptions(self): invalid_urls = [ @@ -335,9 +335,7 @@ def test_valid_service_account_impersonation_url_shall_pass_validation(self): for url in valid_urls: # A valid url shouldn't throw exception and a None value should be returned - assert not external_account.Credentials.validate_service_account_impersonation_url( - url - ) + external_account.Credentials.validate_service_account_impersonation_url(url) def test_invalid_service_account_impersonate_url_shall_throw_exceptions(self): invalid_urls = [ From 89ed17e9a239244fb3773c4bf833bcd0ae304cb2 Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Fri, 29 Apr 2022 00:15:56 +0000 Subject: [PATCH 07/10] fix tests since python 3.6 cannot have same string constants interpret as 3.7+ --- tests/test_external_account.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_external_account.py b/tests/test_external_account.py index 7457d5891..c866931f1 100644 --- a/tests/test_external_account.py +++ b/tests/test_external_account.py @@ -309,7 +309,7 @@ def test_invalid_token_url_shall_throw_exceptions(self): "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://us-\ -1.sts.googleapis.com", "https://-sts.googleapis.com", "https://us-east-1.sts.googleapis.com.evil.com", ] @@ -344,7 +344,7 @@ def test_invalid_service_account_impersonate_url_shall_throw_exceptions(self): "https://", "http://iamcredentials.googleapis.com", "https://iamcre.dentials.googleapis.com", - "https://us-eas\t-1.iamcredentials.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", @@ -355,7 +355,7 @@ def test_invalid_service_account_impersonate_url_shall_throw_exceptions(self): "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://us-\ -1.iamcredentials.googleapis.com", "https://-iamcredentials.googleapis.com", "https://us-east-1.iamcredentials.googleapis.com.evil.com", ] From 4840ffaa676fcd092bbe0b8f8f1b876eeb73b63d Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Mon, 2 May 2022 22:09:59 +0000 Subject: [PATCH 08/10] fix lint and fix the flaky test in an alternate way --- google/auth/external_account.py | 13 ++++++++++++- tests/test_external_account.py | 8 ++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/google/auth/external_account.py b/google/auth/external_account.py index d8e0cae2c..85e803e34 100644 --- a/google/auth/external_account.py +++ b/google/auth/external_account.py @@ -454,6 +454,11 @@ 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: @@ -462,4 +467,10 @@ def is_valid_url(patterns, url): 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) + # return any(re.compile(p).match(uri.hostname.lower()) for p in patterns) + for p in patterns: + if re.compile(p).match(uri.hostname.lower()): + print(f"{p} matches {uri.hostname.lower()}") + return True + + return False diff --git a/tests/test_external_account.py b/tests/test_external_account.py index c866931f1..067fb59b6 100644 --- a/tests/test_external_account.py +++ b/tests/test_external_account.py @@ -298,7 +298,7 @@ def test_invalid_token_url_shall_throw_exceptions(self): "https://", "http://sts.googleapis.com", "https://st.s.googleapis.com", - "https://us-eas\\t-1.sts.googleapis.com", + "https://us-eas\t-1.sts.googleapis.com", "https:/us-east-1.sts.googleapis.com", "https://US-WE/ST-1-sts.googleapis.com", "https://sts-us-east-1.googleapis.com", @@ -309,7 +309,7 @@ def test_invalid_token_url_shall_throw_exceptions(self): "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://us- -1.sts.googleapis.com", "https://-sts.googleapis.com", "https://us-east-1.sts.googleapis.com.evil.com", ] @@ -344,7 +344,7 @@ def test_invalid_service_account_impersonate_url_shall_throw_exceptions(self): "https://", "http://iamcredentials.googleapis.com", "https://iamcre.dentials.googleapis.com", - "https://us-eas\\t-1.iamcredentials.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", @@ -355,7 +355,7 @@ def test_invalid_service_account_impersonate_url_shall_throw_exceptions(self): "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://us- -1.iamcredentials.googleapis.com", "https://-iamcredentials.googleapis.com", "https://us-east-1.iamcredentials.googleapis.com.evil.com", ] From e2ff166a48c32fb26ec408bd4d8dd7d0838fb7f8 Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Mon, 2 May 2022 23:03:00 +0000 Subject: [PATCH 09/10] revert the debugging output --- google/auth/external_account.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/google/auth/external_account.py b/google/auth/external_account.py index 85e803e34..797289d8c 100644 --- a/google/auth/external_account.py +++ b/google/auth/external_account.py @@ -467,10 +467,5 @@ def is_valid_url(patterns, url): 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) - for p in patterns: - if re.compile(p).match(uri.hostname.lower()): - print(f"{p} matches {uri.hostname.lower()}") - return True - - return False + return any(re.compile(p).match(uri.hostname.lower()) for p in patterns) + \ No newline at end of file From c2e204ec555e5cb7e09955e3a3d0f9a23fa296fc Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Mon, 2 May 2022 17:23:24 -0700 Subject: [PATCH 10/10] fix lint --- google/auth/external_account.py | 1 - 1 file changed, 1 deletion(-) diff --git a/google/auth/external_account.py b/google/auth/external_account.py index 797289d8c..97aca1089 100644 --- a/google/auth/external_account.py +++ b/google/auth/external_account.py @@ -468,4 +468,3 @@ def is_valid_url(patterns, url): return False return any(re.compile(p).match(uri.hostname.lower()) for p in patterns) - \ No newline at end of file