From 12e50ab46492deff5cc2de68d4042b4abdbb29f7 Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Fri, 5 May 2017 16:11:51 -0700 Subject: [PATCH 1/3] Fix clock skew calculations Previously, the clock skew adjusted in the wrong direction. It would cause us to consider credentials whose expiration time had already pass according to the system clock to still be sent to the server. This is the opposite of what we wanted to happen. This fixes it so that we report that credentials are expired slightly before the system clock thinks they've expired. --- google/auth/credentials.py | 2 +- tests/test_app_engine.py | 4 ++-- tests/test_credentials.py | 16 +++++++--------- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/google/auth/credentials.py b/google/auth/credentials.py index 6fb89d830..5e23bde80 100644 --- a/google/auth/credentials.py +++ b/google/auth/credentials.py @@ -58,7 +58,7 @@ def expired(self): """ # Err on the side of reporting expiration early so that we avoid # the 403-refresh-retry loop. - adjusted_now = _helpers.utcnow() - _helpers.CLOCK_SKEW + adjusted_now = _helpers.utcnow() + _helpers.CLOCK_SKEW return self.expiry is not None and self.expiry <= adjusted_now @property diff --git a/tests/test_app_engine.py b/tests/test_app_engine.py index 136a914c9..2b957f0da 100644 --- a/tests/test_app_engine.py +++ b/tests/test_app_engine.py @@ -112,10 +112,10 @@ def test_service_account_email_explicit(self, app_identity_mock): @mock.patch( 'google.auth._helpers.utcnow', - return_value=datetime.datetime.min + _helpers.CLOCK_SKEW) + return_value=datetime.datetime.min) def test_refresh(self, now_mock, app_identity_mock): token = 'token' - ttl = 100 + ttl = _helpers.CLOCK_SKEW_SECS + 100 app_identity_mock.get_access_token.return_value = token, ttl credentials = app_engine.Credentials(scopes=['email']) diff --git a/tests/test_credentials.py b/tests/test_credentials.py index c2ff844a4..b5a540dd8 100644 --- a/tests/test_credentials.py +++ b/tests/test_credentials.py @@ -38,21 +38,19 @@ def test_expired_and_valid(): assert credentials.valid assert not credentials.expired - # Set the expiration in the past, but because of clock skew accomodation - # the credentials should still be valid. + # Set the expiration to one second more than now plus the clock skew + # accomodation. These credentials should be valid. credentials.expiry = ( - datetime.datetime.utcnow() - + datetime.datetime.utcnow() + + _helpers.CLOCK_SKEW + datetime.timedelta(seconds=1)) assert credentials.valid assert not credentials.expired - # Set the credentials far enough in the past to exceed the clock skew - # accomodation. They should now be expired. - credentials.expiry = ( - datetime.datetime.utcnow() - - _helpers.CLOCK_SKEW - - datetime.timedelta(seconds=1)) + # Set the credentials expiration to now. Because of the clock skew + # accomodation, these credentials should report as expired. + credentials.expiry = datetime.datetime.utcnow() assert not credentials.valid assert credentials.expired From 28d19725f5c3d8d8b58f0d6cd245b21f21ba6708 Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Mon, 8 May 2017 09:31:09 -0700 Subject: [PATCH 2/3] Address review comments --- google/auth/credentials.py | 11 +++++++---- tests/test_iam.py | 3 ++- tests/transport/test_grpc.py | 3 ++- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/google/auth/credentials.py b/google/auth/credentials.py index 5e23bde80..dc94741dc 100644 --- a/google/auth/credentials.py +++ b/google/auth/credentials.py @@ -56,10 +56,13 @@ def expired(self): Note that credentials can be invalid but not expired becaue Credentials with :attr:`expiry` set to None is considered to never expire. """ - # Err on the side of reporting expiration early so that we avoid - # the 403-refresh-retry loop. - adjusted_now = _helpers.utcnow() + _helpers.CLOCK_SKEW - return self.expiry is not None and self.expiry <= adjusted_now + if not self.expiry: + return False + + # Remove 5 minutes from expiry to err on the side of reporting + # expiration early so that we avoid the 403-refresh-retry loop. + skewed_expiry = self.expiry - _helpers.CLOCK_SKEW + return _helpers.utcnow() >= skewed_expiry @property def valid(self): diff --git a/tests/test_iam.py b/tests/test_iam.py index f7767887b..97fa90755 100644 --- a/tests/test_iam.py +++ b/tests/test_iam.py @@ -20,6 +20,7 @@ import pytest from six.moves import http_client +from google.auth import _helpers from google.auth import exceptions from google.auth import iam from google.auth import transport @@ -42,7 +43,7 @@ def __init__(self): super(CredentialsImpl, self).__init__() self.token = 'token' # Force refresh - self.expiry = datetime.datetime.min + self.expiry = datetime.datetime.min + _helpers.CLOCK_SKEW def refresh(self, request): pass diff --git a/tests/transport/test_grpc.py b/tests/transport/test_grpc.py index 7a3cc0a7f..a494ee518 100644 --- a/tests/transport/test_grpc.py +++ b/tests/transport/test_grpc.py @@ -17,6 +17,7 @@ import mock import pytest +from google.auth import _helpers from google.auth import credentials try: import google.auth.transport.grpc @@ -56,7 +57,7 @@ def test_call_no_refresh(self): def test_call_refresh(self): credentials = MockCredentials() - credentials.expiry = datetime.datetime.min + credentials.expiry = datetime.datetime.min + _helpers.CLOCK_SKEW request = mock.Mock() plugin = google.auth.transport.grpc.AuthMetadataPlugin( From 03222424e10283e1bad78c6f9eeebd14656e83d8 Mon Sep 17 00:00:00 2001 From: Jon Wayne Parrott Date: Mon, 8 May 2017 09:34:02 -0700 Subject: [PATCH 3/3] 3->1 --- google/auth/credentials.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/auth/credentials.py b/google/auth/credentials.py index dc94741dc..74d678821 100644 --- a/google/auth/credentials.py +++ b/google/auth/credentials.py @@ -60,7 +60,7 @@ def expired(self): return False # Remove 5 minutes from expiry to err on the side of reporting - # expiration early so that we avoid the 403-refresh-retry loop. + # expiration early so that we avoid the 401-refresh-retry loop. skewed_expiry = self.expiry - _helpers.CLOCK_SKEW return _helpers.utcnow() >= skewed_expiry