From c14bcb6967c6dec1188a71432968a203007ec371 Mon Sep 17 00:00:00 2001 From: Karthikeyan Chinnakonda Date: Wed, 13 Jan 2021 14:08:13 +0530 Subject: [PATCH] server: accept new config `allowed_skew` in JWT config to provide leeway in JWT expiry fixes https://github.com/hasura/graphql-engine/issues/2109 This PR accepts a new config `allowed_skew` in the JWT config to provide for some leeway while comparing the JWT expiry time. GitOrigin-RevId: ef50cf77d8e2780478685096ed13794b5c4c9de4 --- .circleci/test-server.sh | 15 +++++ CHANGELOG.md | 1 + docs/graphql/core/auth/authentication/jwt.rst | 9 ++- server/src-lib/Hasura/Server/Auth.hs | 2 +- server/src-lib/Hasura/Server/Auth/JWT.hs | 64 +++++++++++------- server/src-test/Hasura/Server/AuthSpec.hs | 1 + server/tests-py/test_jwt.py | 67 ++++++++++++++++++- 7 files changed, 129 insertions(+), 30 deletions(-) diff --git a/.circleci/test-server.sh b/.circleci/test-server.sh index 1e972761c76..ceae5c6ed0c 100755 --- a/.circleci/test-server.sh +++ b/.circleci/test-server.sh @@ -428,6 +428,21 @@ kill_hge_servers unset HASURA_GRAPHQL_JWT_SECRET +echo -e "\n$(time_elapsed): <########## TEST GRAPHQL-ENGINE WITH ADMIN SECRET AND JWT (with JWT config allowing for leeway) #####################################>\n" +TEST_TYPE="jwt-with-expiry-time-leeway" + +export HASURA_GRAPHQL_JWT_SECRET="$(jq -n --arg key "$(cat $OUTPUT_FOLDER/ssl/jwt_public.key)" '{ type: "RS512", key: $key , allowed_skew: 60}')" + +run_hge_with_args serve +wait_for_port 8080 + +pytest -n 1 -vv --hge-urls "$HGE_URL" --pg-urls "$HASURA_GRAPHQL_DATABASE_URL" --hge-key="$HASURA_GRAPHQL_ADMIN_SECRET" --hge-jwt-key-file="$OUTPUT_FOLDER/ssl/jwt_private.key" --hge-jwt-conf="$HASURA_GRAPHQL_JWT_SECRET" test_jwt.py::TestJWTExpirySkew + +kill_hge_servers + +unset HASURA_GRAPHQL_JWT_SECRET + + # test with CORS modes diff --git a/CHANGELOG.md b/CHANGELOG.md index c9f6d0ece7c..253ad9341cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,6 +89,7 @@ and be accessible according to the permissions that were configured for the role - server: fix issue when the `relationships` field in `objects` field is passed `[]` in the `set_custom_types` API (fix #6357) - server: fix issue with event triggers defined on a table which is partitioned (fixes #6261) - server: fix issue with non-optional fields of the remote schema being added as optional in the graphql-engine (fix #6401) +- server: accept new config `allowed_skew` in JWT config to provide leeway for JWT expiry (fixes #2109) - server: fix issue with query actions with relationship with permissions configured on the remote table (fix #6385) - console: allow user to cascade Postgres dependencies when dropping Postgres objects (close #5109) (#5248) - console: mark inconsistent remote schemas in the UI (close #5093) (#5181) diff --git a/docs/graphql/core/auth/authentication/jwt.rst b/docs/graphql/core/auth/authentication/jwt.rst index afa4ad37cae..5a58787e72f 100644 --- a/docs/graphql/core/auth/authentication/jwt.rst +++ b/docs/graphql/core/auth/authentication/jwt.rst @@ -134,7 +134,8 @@ JSON object: "claims_format": "json|stringified_json", "audience": , "issuer": "", - "claims_map": "" + "claims_map": "", + "allowed_skew": "" } (``type``, ``key``) pair or ``jwk_url``, **one of them has to be present**. @@ -494,6 +495,12 @@ The corresponding JWT config should be: In the above example, the ``x-hasura-allowed-roles`` and ``x-hasura-default-role`` values are set in the JWT config and the value of the ``x-hasura-user-id`` is a JSON path to the value in the JWT token. +``allowed_skew`` +^^^^^^^^^^^^^^^^ + +``allowed_skew`` is an optional field to provide some leeway (to account for clock skews) while comparing the JWT expiry time. This field +expects an integer value which will be the number of seconds of the skew value. + Examples ^^^^^^^^ diff --git a/server/src-lib/Hasura/Server/Auth.hs b/server/src-lib/Hasura/Server/Auth.hs index e949107df09..4a86a4d7163 100644 --- a/server/src-lib/Hasura/Server/Auth.hs +++ b/server/src-lib/Hasura/Server/Auth.hs @@ -156,7 +156,7 @@ setupAuthMode mAdminSecretHash mWebHook mJwtSecret mUnAuthRole httpManager logge jwkRef <- case jcKeyOrUrl of Left jwk -> liftIO $ newIORef (JWKSet [jwk]) Right url -> getJwkFromUrl url - return $ JWTCtx jwkRef jcAudience jcIssuer jcClaims + return $ JWTCtx jwkRef jcAudience jcIssuer jcClaims jcAllowedSkew where -- if we can't find any expiry time for the JWK (either in @Expires@ header or @Cache-Control@ -- header), do not start a background thread for refreshing the JWK diff --git a/server/src-lib/Hasura/Server/Auth/JWT.hs b/server/src-lib/Hasura/Server/Auth/JWT.hs index 8005a1c3163..67fe319c077 100644 --- a/server/src-lib/Hasura/Server/Auth/JWT.hs +++ b/server/src-lib/Hasura/Server/Auth/JWT.hs @@ -153,7 +153,7 @@ instance J.FromJSON JWTCustomClaimsMap where let withNotFoundError sessionVariable = let errorMsg = T.unpack $ sessionVariableToText sessionVariable <> " is expected but not found" - in maybe (fail errorMsg) pure $ Map.lookup (sessionVariableToText sessionVariable) obj + in onNothing (Map.lookup (sessionVariableToText sessionVariable) obj) (fail errorMsg) allowedRoles <- withNotFoundError allowedRolesClaim >>= J.parseJSON defaultRole <- withNotFoundError defaultRoleClaim >>= J.parseJSON @@ -182,10 +182,11 @@ data JWTClaims -- | The JWT configuration we got from the user. data JWTConfig = JWTConfig - { jcKeyOrUrl :: !(Either Jose.JWK URI) - , jcAudience :: !(Maybe Jose.Audience) - , jcIssuer :: !(Maybe Jose.StringOrURI) - , jcClaims :: !JWTClaims + { jcKeyOrUrl :: !(Either Jose.JWK URI) + , jcAudience :: !(Maybe Jose.Audience) + , jcIssuer :: !(Maybe Jose.StringOrURI) + , jcClaims :: !JWTClaims + , jcAllowedSkew :: !(Maybe NominalDiffTime) } deriving (Show, Eq) -- | The validated runtime JWT configuration returned by 'mkJwtCtx' in 'setupAuthMode'. @@ -194,16 +195,17 @@ data JWTConfig -- expiration schedule could be determined. data JWTCtx = JWTCtx - { jcxKey :: !(IORef Jose.JWKSet) + { jcxKey :: !(IORef Jose.JWKSet) -- ^ This needs to be a mutable variable for 'updateJwkRef'. - , jcxAudience :: !(Maybe Jose.Audience) - , jcxIssuer :: !(Maybe Jose.StringOrURI) - , jcxClaims :: !JWTClaims + , jcxAudience :: !(Maybe Jose.Audience) + , jcxIssuer :: !(Maybe Jose.StringOrURI) + , jcxClaims :: !JWTClaims + , jcxAllowedSkew :: !(Maybe NominalDiffTime) } deriving (Eq) instance Show JWTCtx where - show (JWTCtx _ audM iss claims) = - show ["", show audM, show iss, show claims] + show (JWTCtx _ audM iss claims allowedSkew) = + show ["", show audM, show iss, show claims, show allowedSkew] data HasuraClaims = HasuraClaims @@ -249,7 +251,7 @@ updateJwkRef -> IORef Jose.JWKSet -> m (Maybe NominalDiffTime) updateJwkRef (Logger logger) manager url jwkRef = do - let urlT = T.pack $ show url + let urlT = tshow url infoMsg = "refreshing JWK from endpoint: " <> urlT liftIO $ logger $ JwkRefreshLog LevelInfo (Just infoMsg) Nothing res <- try $ do @@ -415,7 +417,7 @@ processAuthZHeader jwtCtx authzHeader = do onLeft res (throwError . ef) invalidJWTError e = - err400 JWTInvalid $ "Could not verify JWT: " <> T.pack (show e) + err400 JWTInvalid $ "Could not verify JWT: " <> tshow e malformedAuthzHeader = throw400 InvalidHeaders "Malformed Authorization header" @@ -429,14 +431,15 @@ parseClaimsMap parseClaimsMap unregisteredClaims jcxClaims = case jcxClaims of JCNamespace namespace claimsFormat -> do - claimsV <- maybe (claimsNotFound namespace) pure $ case namespace of - ClaimNs k -> Map.lookup k unregisteredClaims - ClaimNsPath path -> iResultToMaybe $ executeJSONPath path (J.toJSON unregisteredClaims) + claimsV <- flip onNothing (claimsNotFound namespace) $ + case namespace of + ClaimNs k -> Map.lookup k unregisteredClaims + ClaimNsPath path -> iResultToMaybe $ executeJSONPath path (J.toJSON unregisteredClaims) -- get hasura claims value as an object. parse from string possibly claimsObject <- parseObjectFromString namespace claimsFormat claimsV -- filter only x-hasura claims - let claimsMap = mapKeys mkSessionVariable $ + let claimsMap = mapKeys mkSessionVariable $ Map.filterWithKey (const . isSessionVariable) claimsObject pure claimsMap @@ -461,8 +464,8 @@ parseClaimsMap unregisteredClaims jcxClaims = <> sessionVariableToText k <> " not found" case claimObj of JWTCustomClaimsMapJSONPath path defaultVal -> - maybe (onNothing (J.String <$> defaultVal) throwClaimErr) pure - $ iResultToMaybe $ executeJSONPath path claimsObjValue + onNothing (iResultToMaybe $ executeJSONPath path claimsObjValue) $ + (onNothing (J.String <$> defaultVal) throwClaimErr) JWTCustomClaimsMapStatic claimStaticValue -> pure $ J.String claimStaticValue pure $ Map.fromList [ @@ -525,10 +528,15 @@ verifyJwt ctx (RawJWT rawJWT) = do t <- liftIO getCurrentTime Jose.verifyClaimsAt config key t jwt where + validationSettingsWithSkew = + case jcxAllowedSkew ctx of + Just allowedSkew -> Jose.defaultJWTValidationSettings audCheck & set Jose.allowedSkew allowedSkew + -- In `Jose.defaultJWTValidationSettings`, the `allowedSkew` is 0 + Nothing -> Jose.defaultJWTValidationSettings audCheck + config = case jcxIssuer ctx of - Nothing -> Jose.defaultJWTValidationSettings audCheck - Just iss -> Jose.defaultJWTValidationSettings audCheck - & set Jose.issuerPredicate (== iss) + Nothing -> validationSettingsWithSkew + Just iss -> validationSettingsWithSkew & set Jose.issuerPredicate (== iss) audCheck audience = -- dont perform the check if there are no audiences in the conf case jcxAudience ctx of @@ -537,7 +545,7 @@ verifyJwt ctx (RawJWT rawJWT) = do instance J.ToJSON JWTConfig where - toJSON (JWTConfig keyOrUrl aud iss claims) = + toJSON (JWTConfig keyOrUrl aud iss claims allowedSkew) = let keyOrUrlPairs = case keyOrUrl of Left _ -> [ "type" J..= J.String "" , "key" J..= J.String "" @@ -554,7 +562,9 @@ instance J.ToJSON JWTConfig where in J.object $ keyOrUrlPairs <> [ "audience" J..= aud , "issuer" J..= iss - ] <> claimsPairs + ] + <> claimsPairs + <> (maybe [] (\skew -> ["allowed_skew" J..= skew]) allowedSkew) -- | Parse from a json string like: -- | `{"type": "RS256", "key": ""}` @@ -570,6 +580,7 @@ instance J.FromJSON JWTConfig where jwkUrl <- o J..:? "jwk_url" claimsFormat <- o J..:? "claims_format" J..!= defaultClaimsFormat claimsMap <- o J..:? "claims_map" + allowedSkew <- o J..:? "allowed_skew" hasuraClaimsNs <- case (claimsNsPath,claimsNs) of @@ -587,8 +598,9 @@ instance J.FromJSON JWTConfig where pure $ Left key (Nothing, Just url) -> pure $ Right url - pure $ JWTConfig keyOrUrl aud iss $ - maybe (JCNamespace hasuraClaimsNs claimsFormat) JCMap claimsMap + let jwtClaims = maybe (JCNamespace hasuraClaimsNs claimsFormat) JCMap claimsMap + + pure $ JWTConfig keyOrUrl aud iss jwtClaims allowedSkew where parseKey keyType rawKey = diff --git a/server/src-test/Hasura/Server/AuthSpec.hs b/server/src-test/Hasura/Server/AuthSpec.hs index 3c1e06780f9..f3aaa51894c 100644 --- a/server/src-test/Hasura/Server/AuthSpec.hs +++ b/server/src-test/Hasura/Server/AuthSpec.hs @@ -557,6 +557,7 @@ fakeJWTConfig = jcAudience = Nothing jcIssuer = Nothing jcClaims = JCNamespace (ClaimNs "") JCFJson + jcAllowedSkew = Nothing in JWTConfig{..} fakeAuthHook :: AuthHook diff --git a/server/tests-py/test_jwt.py b/server/tests-py/test_jwt.py index 012092fcc54..86387a2edb9 100644 --- a/server/tests-py/test_jwt.py +++ b/server/tests-py/test_jwt.py @@ -1,4 +1,4 @@ -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone import math import json import time @@ -38,6 +38,69 @@ def mk_claims(conf, claims): else: return claims +@pytest.mark.parametrize('endpoint', ['/v1/graphql', '/v1alpha1/graphql']) +class TestJWTExpirySkew(): + + def test_jwt_expiry_leeway(self, hge_ctx, endpoint): + hasura_claims = mk_claims(hge_ctx.hge_jwt_conf, { + 'x-hasura-user-id': '1', + 'x-hasura-default-role': 'user', + 'x-hasura-allowed-roles': ['user'], + }) + claims_namespace_path = None + if 'claims_namespace_path' in hge_ctx.hge_jwt_conf_dict: + claims_namespace_path = hge_ctx.hge_jwt_conf_dict['claims_namespace_path'] + + if not 'allowed_skew' in hge_ctx.hge_jwt_conf_dict: + pytest.skip("This test expects 'allowed_skew' to be set in the JWT config" ) + + self.claims = mk_claims_with_namespace_path(self.claims,hasura_claims,claims_namespace_path) + exp = datetime.now(timezone.utc) - timedelta(seconds = 30) + self.claims['exp'] = round(exp.timestamp()) + token = jwt.encode(self.claims, hge_ctx.hge_jwt_key, algorithm='RS512').decode('utf-8') + self.conf['headers']['Authorization'] = 'Bearer ' + token + self.conf['response'] = { + 'data': { + 'article': [{ + 'id': 1, + 'title': 'Article 1', + 'content': 'Sample article content 1', + 'is_published': False, + 'author': { + 'id': 1, + 'name': 'Author 1' + } + }] + } + } + self.conf['url'] = endpoint + self.conf['status'] = 200 + print ("conf is ", self.conf) + check_query(hge_ctx, self.conf, add_auth=False,claims_namespace_path=claims_namespace_path) + + @pytest.fixture(autouse=True) + def transact(self, setup): + self.dir = 'queries/graphql_query/permissions' + with open(self.dir + '/user_select_query_unpublished_articles.yaml') as c: + self.conf = yaml.safe_load(c) + curr_time = datetime.utcnow() + exp_time = curr_time + timedelta(hours=10) + self.claims = { + 'sub': '1234567890', + 'name': 'John Doe', + 'iat': math.floor(curr_time.timestamp()), + 'exp': math.floor(exp_time.timestamp()) + } + + @pytest.fixture(scope='class') + def setup(self, request, hge_ctx): + self.dir = 'queries/graphql_query/permissions' + st_code, resp = hge_ctx.v1q_f(self.dir + '/setup.yaml') + assert st_code == 200, resp + yield + st_code, resp = hge_ctx.v1q_f(self.dir + '/teardown.yaml') + assert st_code == 200, resp + @pytest.mark.parametrize('endpoint', ['/v1/graphql', '/v1alpha1/graphql']) class TestJWTBasic(): @@ -133,7 +196,7 @@ class TestJWTBasic(): 'code': 'jwt-invalid-claims', 'path': '$' }, - 'message': 'invalid x-hasura-allowed-roles; should be a list of roles: parsing [] failed, expected Array, but encountered String' + 'message': 'invalid x-hasura-allowed-roles; should be a list of roles: parsing [] failed, expected Array, but encountered String' }] } self.conf['url'] = endpoint