fix: rename CLOCK_SKEW and separate client/server user case (#863)

arithmetic1728 · web-flow · commit 738611bd2914 · 2021-09-09T17:09:54.000-07:00
* fix: rename CLOCK_SKEW and separate client/server user case

* update clock skew to 20s
diff --git a/google/auth/_helpers.py b/google/auth/_helpers.py
@@ -20,8 +20,11 @@
 import urllib
 
 
-CLOCK_SKEW_SECS = 60  # 60 seconds
-CLOCK_SKEW = datetime.timedelta(seconds=CLOCK_SKEW_SECS)
+# Token server doesn't provide a new a token when doing refresh unless the
+# token is expiring within 30 seconds, so refresh threshold should not be
+# more than 30 seconds. Otherwise auth lib will send tons of refresh requests
+# until 30 seconds before the expiration, and cause a spike of CPU usage.
+REFRESH_THRESHOLD = datetime.timedelta(seconds=20)
 
 
 def copy_docstring(source_class):
diff --git a/google/auth/credentials.py b/google/auth/credentials.py
@@ -62,7 +62,7 @@ def expired(self):
 
         # Remove 10 seconds from expiry to err on the side of reporting
         # expiration early so that we avoid the 401-refresh-retry loop.
-        skewed_expiry = self.expiry - _helpers.CLOCK_SKEW
+        skewed_expiry = self.expiry - _helpers.REFRESH_THRESHOLD
         return _helpers.utcnow() >= skewed_expiry
 
     @property
diff --git a/google/auth/jwt.py b/google/auth/jwt.py
@@ -167,12 +167,14 @@ def decode_header(token):
     return header
 
 
-def _verify_iat_and_exp(payload):
+def _verify_iat_and_exp(payload, clock_skew_in_seconds=0):
     """Verifies the ``iat`` (Issued At) and ``exp`` (Expires) claims in a token
     payload.
 
     Args:
         payload (Mapping[str, str]): The JWT payload.
+        clock_skew_in_seconds (int): The clock skew used for `iat` and `exp`
+            validation.
 
     Raises:
         ValueError: if any checks failed.
@@ -188,7 +190,7 @@ def _verify_iat_and_exp(payload):
     iat = payload["iat"]
     # Err on the side of accepting a token that is slightly early to account
     # for clock skew.
-    earliest = iat - _helpers.CLOCK_SKEW_SECS
+    earliest = iat - clock_skew_in_seconds
     if now < earliest:
         raise ValueError(
             "Token used too early, {} < {}. Check that your computer's clock is set correctly.".format(
@@ -200,12 +202,12 @@ def _verify_iat_and_exp(payload):
     exp = payload["exp"]
     # Err on the side of accepting a token that is slightly out of date
     # to account for clow skew.
-    latest = exp + _helpers.CLOCK_SKEW_SECS
+    latest = exp + clock_skew_in_seconds
     if latest < now:
         raise ValueError("Token expired, {} < {}".format(latest, now))
 
 
-def decode(token, certs=None, verify=True, audience=None):
+def decode(token, certs=None, verify=True, audience=None, clock_skew_in_seconds=0):
     """Decode and verify a JWT.
 
     Args:
@@ -221,6 +223,8 @@ def decode(token, certs=None, verify=True, audience=None):
         audience (str or list): The audience claim, 'aud', that this JWT should
             contain. Or a list of audience claims. If None then the JWT's 'aud'
             parameter is not verified.
+        clock_skew_in_seconds (int): The clock skew used for `iat` and `exp`
+            validation.
 
     Returns:
         Mapping[str, str]: The deserialized JSON payload in the JWT.
@@ -271,7 +275,7 @@ def decode(token, certs=None, verify=True, audience=None):
         raise ValueError("Could not verify token signature.")
 
     # Verify the issued at and created times in the payload.
-    _verify_iat_and_exp(payload)
+    _verify_iat_and_exp(payload, clock_skew_in_seconds)
 
     # Check audience.
     if audience is not None:
diff --git a/google/oauth2/credentials.py b/google/oauth2/credentials.py
@@ -270,7 +270,7 @@ def refresh(self, request):
                 raise exceptions.RefreshError(
                     "The refresh_handler returned expiry is not a datetime object."
                 )
-            if _helpers.utcnow() >= expiry - _helpers.CLOCK_SKEW:
+            if _helpers.utcnow() >= expiry - _helpers.REFRESH_THRESHOLD:
                 raise exceptions.RefreshError(
                     "The credentials returned by the refresh_handler are "
                     "already expired."
@@ -359,7 +359,7 @@ def from_authorized_user_info(cls, info, scopes=None):
                 expiry.rstrip("Z").split(".")[0], "%Y-%m-%dT%H:%M:%S"
             )
         else:
-            expiry = _helpers.utcnow() - _helpers.CLOCK_SKEW
+            expiry = _helpers.utcnow() - _helpers.REFRESH_THRESHOLD
 
         # process scopes, which needs to be a seq
         if scopes is None and "scopes" in info:
diff --git a/tests/compute_engine/test_credentials.py b/tests/compute_engine/test_credentials.py
@@ -64,7 +64,7 @@ def test_default_state(self):
 
     @mock.patch(
         "google.auth._helpers.utcnow",
-        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW,
+        return_value=datetime.datetime.min + _helpers.REFRESH_THRESHOLD,
     )
     @mock.patch("google.auth.compute_engine._metadata.get", autospec=True)
     def test_refresh_success(self, get, utcnow):
@@ -98,7 +98,7 @@ def test_refresh_success(self, get, utcnow):
 
     @mock.patch(
         "google.auth._helpers.utcnow",
-        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW,
+        return_value=datetime.datetime.min + _helpers.REFRESH_THRESHOLD,
     )
     @mock.patch("google.auth.compute_engine._metadata.get", autospec=True)
     def test_refresh_success_with_scopes(self, get, utcnow):
diff --git a/tests/oauth2/test_credentials.py b/tests/oauth2/test_credentials.py
@@ -115,7 +115,7 @@ def test_invalid_refresh_handler(self):
     @mock.patch("google.oauth2.reauth.refresh_grant", autospec=True)
     @mock.patch(
         "google.auth._helpers.utcnow",
-        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW,
+        return_value=datetime.datetime.min + _helpers.REFRESH_THRESHOLD,
     )
     def test_refresh_success(self, unused_utcnow, refresh_grant):
         token = "token"
@@ -175,7 +175,7 @@ def test_refresh_no_refresh_token(self):
     @mock.patch("google.oauth2.reauth.refresh_grant", autospec=True)
     @mock.patch(
         "google.auth._helpers.utcnow",
-        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW,
+        return_value=datetime.datetime.min + _helpers.REFRESH_THRESHOLD,
     )
     def test_refresh_with_refresh_token_and_refresh_handler(
         self, unused_utcnow, refresh_grant
@@ -361,7 +361,7 @@ def test_refresh_with_refresh_handler_invalid_expiry(self):
 
     @mock.patch("google.auth._helpers.utcnow", return_value=datetime.datetime.min)
     def test_refresh_with_refresh_handler_expired_token(self, unused_utcnow):
-        expected_expiry = datetime.datetime.min + _helpers.CLOCK_SKEW
+        expected_expiry = datetime.datetime.min + _helpers.REFRESH_THRESHOLD
         # Simulate refresh handler returns an expired token.
         refresh_handler = mock.Mock(return_value=("TOKEN", expected_expiry))
         scopes = ["email", "profile"]
@@ -391,7 +391,7 @@ def test_refresh_with_refresh_handler_expired_token(self, unused_utcnow):
     @mock.patch("google.oauth2.reauth.refresh_grant", autospec=True)
     @mock.patch(
         "google.auth._helpers.utcnow",
-        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW,
+        return_value=datetime.datetime.min + _helpers.REFRESH_THRESHOLD,
     )
     def test_credentials_with_scopes_requested_refresh_success(
         self, unused_utcnow, refresh_grant
@@ -457,7 +457,7 @@ def test_credentials_with_scopes_requested_refresh_success(
     @mock.patch("google.oauth2.reauth.refresh_grant", autospec=True)
     @mock.patch(
         "google.auth._helpers.utcnow",
-        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW,
+        return_value=datetime.datetime.min + _helpers.REFRESH_THRESHOLD,
     )
     def test_credentials_with_only_default_scopes_requested(
         self, unused_utcnow, refresh_grant
@@ -521,7 +521,7 @@ def test_credentials_with_only_default_scopes_requested(
     @mock.patch("google.oauth2.reauth.refresh_grant", autospec=True)
     @mock.patch(
         "google.auth._helpers.utcnow",
-        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW,
+        return_value=datetime.datetime.min + _helpers.REFRESH_THRESHOLD,
     )
     def test_credentials_with_scopes_returned_refresh_success(
         self, unused_utcnow, refresh_grant
@@ -588,7 +588,7 @@ def test_credentials_with_scopes_returned_refresh_success(
     @mock.patch("google.oauth2.reauth.refresh_grant", autospec=True)
     @mock.patch(
         "google.auth._helpers.utcnow",
-        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW,
+        return_value=datetime.datetime.min + _helpers.REFRESH_THRESHOLD,
     )
     def test_credentials_with_scopes_refresh_failure_raises_refresh_error(
         self, unused_utcnow, refresh_grant
diff --git a/tests/test_credentials.py b/tests/test_credentials.py
@@ -46,7 +46,9 @@ def test_expired_and_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() + _helpers.CLOCK_SKEW + datetime.timedelta(seconds=1)
+        datetime.datetime.utcnow()
+        + _helpers.REFRESH_THRESHOLD
+        + datetime.timedelta(seconds=1)
     )
 
     assert credentials.valid
diff --git a/tests/test_downscoped.py b/tests/test_downscoped.py
@@ -669,7 +669,9 @@ def test_before_request_expired(self, utcnow):
         # Set the expiration to one second more than now plus the clock skew
         # accommodation. These credentials should be valid.
         credentials.expiry = (
-            datetime.datetime.min + _helpers.CLOCK_SKEW + datetime.timedelta(seconds=1)
+            datetime.datetime.min
+            + _helpers.REFRESH_THRESHOLD
+            + datetime.timedelta(seconds=1)
         )
 
         assert credentials.valid
diff --git a/tests/test_external_account.py b/tests/test_external_account.py
@@ -976,7 +976,9 @@ def test_before_request_expired(self, utcnow):
         # Set the expiration to one second more than now plus the clock skew
         # accomodation. These credentials should be valid.
         credentials.expiry = (
-            datetime.datetime.min + _helpers.CLOCK_SKEW + datetime.timedelta(seconds=1)
+            datetime.datetime.min
+            + _helpers.REFRESH_THRESHOLD
+            + datetime.timedelta(seconds=1)
         )
 
         assert credentials.valid
@@ -1027,7 +1029,9 @@ def test_before_request_impersonation_expired(self, utcnow):
         # Set the expiration to one second more than now plus the clock skew
         # accomodation. These credentials should be valid.
         credentials.expiry = (
-            datetime.datetime.min + _helpers.CLOCK_SKEW + datetime.timedelta(seconds=1)
+            datetime.datetime.min
+            + _helpers.REFRESH_THRESHOLD
+            + datetime.timedelta(seconds=1)
         )
 
         assert credentials.valid
diff --git a/tests/test_iam.py b/tests/test_iam.py
@@ -45,7 +45,7 @@ def __init__(self):
             super(CredentialsImpl, self).__init__()
             self.token = "token"
             # Force refresh
-            self.expiry = datetime.datetime.min + _helpers.CLOCK_SKEW
+            self.expiry = datetime.datetime.min + _helpers.REFRESH_THRESHOLD
 
         def refresh(self, request):
             pass
diff --git a/tests/test_impersonated_credentials.py b/tests/test_impersonated_credentials.py
@@ -211,11 +211,11 @@ def test_refresh_source_credentials(self, time_skew):
         credentials = self.make_credentials(lifetime=None)
 
         # Source credentials is refreshed only if it is expired within
-        # _helpers.CLOCK_SKEW from now. We add a time_skew to the expiry, so
+        # _helpers.REFRESH_THRESHOLD from now. We add a time_skew to the expiry, so
         # source credentials is refreshed only if time_skew <= 0.
         credentials._source_credentials.expiry = (
             _helpers.utcnow()
-            + _helpers.CLOCK_SKEW
+            + _helpers.REFRESH_THRESHOLD
             + datetime.timedelta(seconds=time_skew)
         )
         credentials._source_credentials.token = "Token"
@@ -238,7 +238,7 @@ def test_refresh_source_credentials(self, time_skew):
             assert not credentials.expired
 
             # Source credentials is refreshed only if it is expired within
-            # _helpers.CLOCK_SKEW
+            # _helpers.REFRESH_THRESHOLD
             if time_skew > 0:
                 source_cred_refresh.assert_not_called()
             else:
diff --git a/tests/test_jwt.py b/tests/test_jwt.py
@@ -197,7 +197,7 @@ def test_decode_bad_token_too_early(token_factory):
         }
     )
     with pytest.raises(ValueError) as excinfo:
-        jwt.decode(token, PUBLIC_CERT_BYTES)
+        jwt.decode(token, PUBLIC_CERT_BYTES, clock_skew_in_seconds=59)
     assert excinfo.match(r"Token used too early")
 
 
@@ -210,10 +210,40 @@ def test_decode_bad_token_expired(token_factory):
         }
     )
     with pytest.raises(ValueError) as excinfo:
-        jwt.decode(token, PUBLIC_CERT_BYTES)
+        jwt.decode(token, PUBLIC_CERT_BYTES, clock_skew_in_seconds=59)
     assert excinfo.match(r"Token expired")
 
 
+def test_decode_success_with_no_clock_skew(token_factory):
+    token = token_factory(
+        claims={
+            "exp": _helpers.datetime_to_secs(
+                _helpers.utcnow() + datetime.timedelta(seconds=1)
+            ),
+            "iat": _helpers.datetime_to_secs(
+                _helpers.utcnow() - datetime.timedelta(seconds=1)
+            ),
+        }
+    )
+
+    jwt.decode(token, PUBLIC_CERT_BYTES)
+
+
+def test_decode_success_with_custom_clock_skew(token_factory):
+    token = token_factory(
+        claims={
+            "exp": _helpers.datetime_to_secs(
+                _helpers.utcnow() + datetime.timedelta(seconds=2)
+            ),
+            "iat": _helpers.datetime_to_secs(
+                _helpers.utcnow() - datetime.timedelta(seconds=2)
+            ),
+        }
+    )
+
+    jwt.decode(token, PUBLIC_CERT_BYTES, clock_skew_in_seconds=1)
+
+
 def test_decode_bad_token_wrong_audience(token_factory):
     token = token_factory()
     audience = "audience2@example.com"
diff --git a/tests/transport/test_grpc.py b/tests/transport/test_grpc.py
@@ -80,7 +80,7 @@ def test_call_no_refresh(self):
 
     def test_call_refresh(self):
         credentials = CredentialsStub()
-        credentials.expiry = datetime.datetime.min + _helpers.CLOCK_SKEW
+        credentials.expiry = datetime.datetime.min + _helpers.REFRESH_THRESHOLD
         request = mock.create_autospec(transport.Request)
 
         plugin = google.auth.transport.grpc.AuthMetadataPlugin(credentials, request)
diff --git a/tests_async/oauth2/test_credentials_async.py b/tests_async/oauth2/test_credentials_async.py
@@ -62,7 +62,7 @@ def test_default_state(self):
     @mock.patch("google.oauth2._reauth_async.refresh_grant", autospec=True)
     @mock.patch(
         "google.auth._helpers.utcnow",
-        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW,
+        return_value=datetime.datetime.min + _helpers.REFRESH_THRESHOLD,
     )
     @pytest.mark.asyncio
     async def test_refresh_success(self, unused_utcnow, refresh_grant):
@@ -124,7 +124,7 @@ async def test_refresh_no_refresh_token(self):
     @mock.patch("google.oauth2._reauth_async.refresh_grant", autospec=True)
     @mock.patch(
         "google.auth._helpers.utcnow",
-        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW,
+        return_value=datetime.datetime.min + _helpers.REFRESH_THRESHOLD,
     )
     @pytest.mark.asyncio
     async def test_credentials_with_scopes_requested_refresh_success(
@@ -188,7 +188,7 @@ async def test_credentials_with_scopes_requested_refresh_success(
     @mock.patch("google.oauth2._reauth_async.refresh_grant", autospec=True)
     @mock.patch(
         "google.auth._helpers.utcnow",
-        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW,
+        return_value=datetime.datetime.min + _helpers.REFRESH_THRESHOLD,
     )
     @pytest.mark.asyncio
     async def test_credentials_with_scopes_returned_refresh_success(
@@ -251,7 +251,7 @@ async def test_credentials_with_scopes_returned_refresh_success(
     @mock.patch("google.oauth2._reauth_async.refresh_grant", autospec=True)
     @mock.patch(
         "google.auth._helpers.utcnow",
-        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW,
+        return_value=datetime.datetime.min + _helpers.REFRESH_THRESHOLD,
     )
     @pytest.mark.asyncio
     async def test_credentials_with_scopes_refresh_failure_raises_refresh_error(
diff --git a/tests_async/test_credentials_async.py b/tests_async/test_credentials_async.py
@@ -46,7 +46,9 @@ def test_expired_and_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() + _helpers.CLOCK_SKEW + datetime.timedelta(seconds=1)
+        datetime.datetime.utcnow()
+        + _helpers.REFRESH_THRESHOLD
+        + datetime.timedelta(seconds=1)
     )
 
     assert credentials.valid
