From d747bc1148027143a4e606ab3255e873859453b4 Mon Sep 17 00:00:00 2001 From: Brandon Simmons Date: Tue, 19 May 2020 10:48:49 -0400 Subject: [PATCH 1/2] Tighten up handling of admin secret, more docs Store the admin secret only as a hash to prevent leaking the secret inadvertently, and to prevent timing attacks on the secret. NOTE: best practice for stored user passwords is a function with a tunable cost like bcrypt, but our threat model is quite different (even if we thought we could reasonably protect the secret from an attacker who could read arbitrary regions of memory), and bcrypt is far too slow (by design) to perform on each request. We'd have to rely on our (technically savvy) users to choose high entropy passwords in any case. Referencing #4736 --- server/src-lib/Hasura/App.hs | 2 +- server/src-lib/Hasura/Server/Auth.hs | 166 +++++++++++--------- server/src-lib/Hasura/Server/Auth/JWT.hs | 5 + server/src-lib/Hasura/Server/Init.hs | 8 +- server/src-lib/Hasura/Server/Init/Config.hs | 8 +- 5 files changed, 108 insertions(+), 81 deletions(-) diff --git a/server/src-lib/Hasura/App.hs b/server/src-lib/Hasura/App.hs index 4b97ccee1d969..9c7286da8d594 100644 --- a/server/src-lib/Hasura/App.hs +++ b/server/src-lib/Hasura/App.hs @@ -227,7 +227,7 @@ runHGEServer ServeOptions{..} InitCtx{..} initTime = do let sqlGenCtx = SQLGenCtx soStringifyNum Loggers loggerCtx logger _ = _icLoggers - authModeRes <- runExceptT $ mkAuthMode soAdminSecret soAuthHook soJwtSecret soUnAuthRole + authModeRes <- runExceptT $ setupAuthMode soAdminSecret soAuthHook soJwtSecret soUnAuthRole _icHttpManager logger authMode <- either (printErrExit . T.unpack) return authModeRes diff --git a/server/src-lib/Hasura/Server/Auth.hs b/server/src-lib/Hasura/Server/Auth.hs index c75d3e7d09342..565754ef9a226 100644 --- a/server/src-lib/Hasura/Server/Auth.hs +++ b/server/src-lib/Hasura/Server/Auth.hs @@ -4,13 +4,14 @@ module Hasura.Server.Auth ( getUserInfo , getUserInfoWithExpTime , AuthMode (..) - , mkAuthMode - , AdminSecret (..) - -- WebHook related + , setupAuthMode + , AdminSecretHash + , hashAdminSecret + -- * WebHook related , AuthHookType (..) , AuthHookG (..) , AuthHook - -- JWT related + -- * JWT related , RawJWT , JWTConfig (..) , JWTCtx (..) @@ -25,7 +26,9 @@ import Data.IORef (newIORef) import Data.Time.Clock (UTCTime) import Hasura.Server.Version (HasVersion) +import qualified Crypto.Hash as Crypto import qualified Data.Text as T +import qualified Data.Text.Encoding as T import qualified Network.HTTP.Client as H import qualified Network.HTTP.Types as N @@ -48,39 +51,64 @@ class (Monad m) => UserAuthentication m where -> AuthMode -> m (Either QErr (UserInfo, Maybe UTCTime)) -newtype AdminSecret - = AdminSecret { getAdminSecret :: T.Text } - deriving (Show, Eq) - - +-- | The hashed admin password. 'hashAdminSecret' is our public interface for +-- constructing the secret. +-- +-- To prevent misuse and leaking we keep this opaque and don't provide +-- instances that could leak information. Likewise for 'AuthMode'. +-- +-- Although this exists only in memory we store only a hash of the admin secret +-- primarily in order to: +-- +-- - prevent theoretical timing attacks from a naive `==` +-- - prevent misuse or inadvertent leaking of the secret +-- +-- NOTE: if we could scrub memory of admin secret (from argv and envp) somehow, +-- this would additionally harden against attacks that could read arbitrary +-- memory, so long as the secret was strong. I'm not sure that's attainable. +newtype AdminSecretHash = AdminSecretHash (Crypto.Digest Crypto.SHA512) + deriving (Ord, Eq) + +hashAdminSecret :: T.Text -> AdminSecretHash +hashAdminSecret = AdminSecretHash . Crypto.hash . T.encodeUtf8 + +-- | The methods we'll use to derive roles for authenticating requests. +-- +-- @Maybe RoleName@ below is the optionally-defined role for the +-- unauthenticated (anonymous) user. +-- +-- See: https://hasura.io/docs/1.0/graphql/manual/auth/authentication/unauthenticated-access.html data AuthMode = AMNoAuth - | AMAdminSecret !AdminSecret !(Maybe RoleName) - | AMAdminSecretAndHook !AdminSecret !AuthHook - | AMAdminSecretAndJWT !AdminSecret !JWTCtx !(Maybe RoleName) - deriving (Show, Eq) - -mkAuthMode + | AMAdminSecret !AdminSecretHash !(Maybe RoleName) + | AMAdminSecretAndHook !AdminSecretHash !AuthHook + | AMAdminSecretAndJWT !AdminSecretHash !JWTCtx !(Maybe RoleName) + +-- | Validate the user's requested authentication configuration, launching any +-- required maintenance threads for JWT etc. +-- +-- This must only be run once, on launch. +setupAuthMode :: ( HasVersion , MonadIO m , MonadError T.Text m ) - => Maybe AdminSecret + => Maybe AdminSecretHash -> Maybe AuthHook -> Maybe JWTConfig -> Maybe RoleName -> H.Manager -> Logger Hasura -> m AuthMode -mkAuthMode mAdminSecret mWebHook mJwtSecret mUnAuthRole httpManager logger = - case (mAdminSecret, mWebHook, mJwtSecret) of +setupAuthMode mAdminSecretHash mWebHook mJwtSecret mUnAuthRole httpManager logger = + case (mAdminSecretHash, mWebHook, mJwtSecret) of (Nothing, Nothing, Nothing) -> return AMNoAuth - (Just key, Nothing, Nothing) -> return $ AMAdminSecret key mUnAuthRole - (Just key, Just hook, Nothing) -> unAuthRoleNotReqForWebHook >> - return (AMAdminSecretAndHook key hook) - (Just key, Nothing, Just jwtConf) -> do - jwtCtx <- mkJwtCtx jwtConf httpManager logger - return $ AMAdminSecretAndJWT key jwtCtx mUnAuthRole + (Just hash, Nothing, Nothing) -> return $ AMAdminSecret hash mUnAuthRole + (Just hash, Just hook, Nothing) -> unAuthRoleNotReqForWebHook >> + return (AMAdminSecretAndHook hash hook) + (Just hash, Nothing, Just jwtConf) -> do + jwtCtx <- mkJwtCtx jwtConf + return $ AMAdminSecretAndJWT hash jwtCtx mUnAuthRole (Nothing, Just _, Nothing) -> throwError $ "Fatal Error : --auth-hook (HASURA_GRAPHQL_AUTH_HOOK)" <> requiresAdminScrtMsg @@ -99,45 +127,38 @@ mkAuthMode mAdminSecret mWebHook mJwtSecret mUnAuthRole httpManager logger = "Fatal Error: --unauthorized-role (HASURA_GRAPHQL_UNAUTHORIZED_ROLE) is not allowed" <> " when --auth-hook (HASURA_GRAPHQL_AUTH_HOOK) is set" --- | Given the 'JWTConfig' (the user input of JWT configuration), create the 'JWTCtx' (the runtime JWT config used) -mkJwtCtx - :: ( HasVersion - , MonadIO m - , MonadError T.Text m - ) - => JWTConfig - -> H.Manager - -> Logger Hasura - -> m JWTCtx -mkJwtCtx JWTConfig{..} httpManager logger = do - jwkRef <- case jcKeyOrUrl of - Left jwk -> liftIO $ newIORef (JWKSet [jwk]) - Right url -> getJwkFromUrl url - let claimsFmt = fromMaybe JCFJson jcClaimsFormat - return $ JWTCtx jwkRef jcClaimNs jcAudience claimsFmt jcIssuer - 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 - getJwkFromUrl url = do - ref <- liftIO $ newIORef $ JWKSet [] - maybeExpiry <- withJwkError $ updateJwkRef logger httpManager url ref - case maybeExpiry of - Nothing -> return ref - Just time -> do - void $ liftIO $ forkImmortal "jwkRefreshCtrl" logger $ - jwkRefreshCtrl logger httpManager url ref (convertDuration time) - return ref - - withJwkError act = do - res <- runExceptT act - case res of - Right r -> return r - Left err -> case err of - -- when fetching JWK initially, except expiry parsing error, all errors are critical - JFEHttpException _ msg -> throwError msg - JFEHttpError _ _ _ e -> throwError e - JFEJwkParseError _ e -> throwError e - JFEExpiryParseError _ _ -> return Nothing + -- | Given the 'JWTConfig' (the user input of JWT configuration), create + -- the 'JWTCtx' (the runtime JWT config used) + mkJwtCtx :: (HasVersion, MonadIO m, MonadError T.Text m) => JWTConfig -> m JWTCtx + mkJwtCtx JWTConfig{..} = do + jwkRef <- case jcKeyOrUrl of + Left jwk -> liftIO $ newIORef (JWKSet [jwk]) + Right url -> getJwkFromUrl url + let claimsFmt = fromMaybe JCFJson jcClaimsFormat + return $ JWTCtx jwkRef jcClaimNs jcAudience claimsFmt jcIssuer + 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 + getJwkFromUrl url = do + ref <- liftIO $ newIORef $ JWKSet [] + maybeExpiry <- withJwkError $ updateJwkRef logger httpManager url ref + case maybeExpiry of + Nothing -> return ref + Just time -> do + void $ liftIO $ forkImmortal "jwkRefreshCtrl" logger $ + jwkRefreshCtrl logger httpManager url ref (convertDuration time) + return ref + + withJwkError act = do + res <- runExceptT act + case res of + Right r -> return r + Left err -> case err of + -- when fetching JWK initially, except expiry parsing error, all errors are critical + JFEHttpException _ msg -> throwError msg + JFEHttpError _ _ _ e -> throwError e + JFEJwkParseError _ e -> throwError e + JFEExpiryParseError _ _ -> return Nothing getUserInfo :: (HasVersion, MonadIO m, MonadError QErr m) @@ -148,6 +169,7 @@ getUserInfo -> m UserInfo getUserInfo l m r a = fst <$> getUserInfoWithExpTime l m r a +-- | Authenticate the request using the headers and the configured 'AuthMode'. getUserInfoWithExpTime :: forall m. (HasVersion, MonadIO m, MonadError QErr m) => Logger Hasura @@ -159,8 +181,8 @@ getUserInfoWithExpTime logger manager rawHeaders = \case AMNoAuth -> withNoExpTime $ mkUserInfoFallbackAdminRole UAuthNotSet - AMAdminSecret adminSecretSet maybeUnauthRole -> - withAuthorization adminSecretSet $ withNoExpTime $ + AMAdminSecret realAdminSecretHash maybeUnauthRole -> + withAuthorization realAdminSecretHash $ withNoExpTime $ -- Consider unauthorized role, if not found raise admin secret header required exception case maybeUnauthRole of Nothing -> throw401 $ adminSecretHeader <> "/" @@ -168,11 +190,11 @@ getUserInfoWithExpTime logger manager rawHeaders = \case Just unAuthRole -> mkUserInfo (URBPreDetermined unAuthRole) UAdminSecretNotSent sessionVariables - AMAdminSecretAndHook adminSecretSet hook -> - withAuthorization adminSecretSet $ userInfoFromAuthHook logger manager hook rawHeaders + AMAdminSecretAndHook realAdminSecretHash hook -> + withAuthorization realAdminSecretHash $ userInfoFromAuthHook logger manager hook rawHeaders - AMAdminSecretAndJWT adminSecretSet jwtSecret unAuthRole -> - withAuthorization adminSecretSet $ processJwt jwtSecret rawHeaders unAuthRole + AMAdminSecretAndJWT realAdminSecretHash jwtSecret unAuthRole -> + withAuthorization realAdminSecretHash $ processJwt jwtSecret rawHeaders unAuthRole where mkUserInfoFallbackAdminRole adminSecretState = @@ -182,8 +204,8 @@ getUserInfoWithExpTime logger manager rawHeaders = \case sessionVariables = mkSessionVariables rawHeaders withAuthorization - :: AdminSecret -> m (UserInfo, Maybe UTCTime) -> m (UserInfo, Maybe UTCTime) - withAuthorization adminSecretSet actionIfNoAdminSecret = do + :: AdminSecretHash -> m (UserInfo, Maybe UTCTime) -> m (UserInfo, Maybe UTCTime) + withAuthorization realAdminSecretHash actionIfNoAdminSecret = do let maybeRequestAdminSecret = foldl1 (<|>) $ map (`getSessionVariableValue` sessionVariables) [adminSecretHeader, deprecatedAccessKeyHeader] @@ -192,7 +214,7 @@ getUserInfoWithExpTime logger manager rawHeaders = \case case maybeRequestAdminSecret of Nothing -> actionIfNoAdminSecret Just requestAdminSecret -> do - when (requestAdminSecret /= getAdminSecret adminSecretSet) $ throw401 $ + when (hashAdminSecret requestAdminSecret /= realAdminSecretHash) $ throw401 $ "invalid " <> adminSecretHeader <> "/" <> deprecatedAccessKeyHeader withNoExpTime $ mkUserInfoFallbackAdminRole UAdminSecretSent diff --git a/server/src-lib/Hasura/Server/Auth/JWT.hs b/server/src-lib/Hasura/Server/Auth/JWT.hs index 52e1f288d51fb..19c98228b25c0 100644 --- a/server/src-lib/Hasura/Server/Auth/JWT.hs +++ b/server/src-lib/Hasura/Server/Auth/JWT.hs @@ -71,6 +71,7 @@ instance J.ToJSON JWTConfigClaims where toJSON (ClaimNsPath nsPath) = J.String . T.pack $ encodeJSONPath nsPath toJSON (ClaimNs ns) = J.String ns +-- | The JWT configuration we got from the user. data JWTConfig = JWTConfig { jcKeyOrUrl :: !(Either Jose.JWK URI) @@ -80,6 +81,10 @@ data JWTConfig , jcIssuer :: !(Maybe Jose.StringOrURI) } deriving (Show, Eq) +-- | The validated runtime JWT configuration returned by 'mkJwtCtx' in 'setupAuthMode'. +-- +-- This is also evidence that the 'jwkRefreshCtrl' thread is running, if an +-- expiration schedule could be determined. data JWTCtx = JWTCtx { jcxKey :: !(IORef Jose.JWKSet) diff --git a/server/src-lib/Hasura/Server/Init.hs b/server/src-lib/Hasura/Server/Init.hs index 133557cbb4981..6032528a863c5 100644 --- a/server/src-lib/Hasura/Server/Init.hs +++ b/server/src-lib/Hasura/Server/Init.hs @@ -643,17 +643,17 @@ parseServerHost = optional $ strOption ( long "server-host" <> help "Host on which graphql-engine will listen (default: *)" ) -parseAccessKey :: Parser (Maybe AdminSecret) +parseAccessKey :: Parser (Maybe AdminSecretHash) parseAccessKey = - optional $ AdminSecret <$> + optional $ hashAdminSecret <$> strOption ( long "access-key" <> metavar "ADMIN SECRET KEY (DEPRECATED: USE --admin-secret)" <> help (snd adminSecretEnv) ) -parseAdminSecret :: Parser (Maybe AdminSecret) +parseAdminSecret :: Parser (Maybe AdminSecretHash) parseAdminSecret = - optional $ AdminSecret <$> + optional $ hashAdminSecret <$> strOption ( long "admin-secret" <> metavar "ADMIN SECRET KEY" <> help (snd adminSecretEnv) diff --git a/server/src-lib/Hasura/Server/Init/Config.hs b/server/src-lib/Hasura/Server/Init/Config.hs index 8a8e0c1d7c809..c40fbacf0438d 100644 --- a/server/src-lib/Hasura/Server/Init/Config.hs +++ b/server/src-lib/Hasura/Server/Init/Config.hs @@ -37,7 +37,7 @@ data RawServeOptions impl , rsoHost :: !(Maybe HostPreference) , rsoConnParams :: !RawConnParams , rsoTxIso :: !(Maybe Q.TxIsolation) - , rsoAdminSecret :: !(Maybe AdminSecret) + , rsoAdminSecret :: !(Maybe AdminSecretHash) , rsoAuthHook :: !RawAuthHook , rsoJwtSecret :: !(Maybe JWTConfig) , rsoUnAuthRole :: !(Maybe RoleName) @@ -79,7 +79,7 @@ data ServeOptions impl , soHost :: !HostPreference , soConnParams :: !Q.ConnParams , soTxIso :: !Q.TxIsolation - , soAdminSecret :: !(Maybe AdminSecret) + , soAdminSecret :: !(Maybe AdminSecretHash) , soAuthHook :: !(Maybe AuthHook) , soJwtSecret :: !(Maybe JWTConfig) , soUnAuthRole :: !(Maybe RoleName) @@ -217,8 +217,8 @@ instance FromEnv AuthHookType where instance FromEnv Int where fromEnv = maybe (Left "Expecting Int value") Right . readMaybe -instance FromEnv AdminSecret where - fromEnv = Right . AdminSecret . T.pack +instance FromEnv AdminSecretHash where + fromEnv = Right . hashAdminSecret . T.pack instance FromEnv RoleName where fromEnv string = case mkRoleName (T.pack string) of From 5e373505618fe76aa2623804af19a27139df2bac Mon Sep 17 00:00:00 2001 From: Brandon Simmons Date: Thu, 28 May 2020 12:18:26 -0400 Subject: [PATCH 2/2] Refactor and unit test authentication code paths (closes #4736) The bulk of changes here is some shifting of code around and a little parameterizing of functions for easier testing. Also: comments, some renaming for clarity/less-chance-for-misue. --- CHANGELOG.md | 1 + .../manual/auth/authentication/jwt.rst | 6 +- server/graphql-engine.cabal | 5 +- .../Hasura/GraphQL/Execute/LiveQuery/Plan.hs | 2 + server/src-lib/Hasura/GraphQL/Explain.hs | 3 + server/src-lib/Hasura/Server/Auth.hs | 63 ++- server/src-lib/Hasura/Server/Auth/JWT.hs | 166 +++---- server/src-test/Hasura/Server/AuthSpec.hs | 408 ++++++++++++++++++ server/src-test/Main.hs | 2 + server/tests-py/test_jwt.py | 4 +- 10 files changed, 560 insertions(+), 100 deletions(-) create mode 100644 server/src-test/Hasura/Server/AuthSpec.hs diff --git a/CHANGELOG.md b/CHANGELOG.md index 56a0bd4649d1e..634e48798547e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,6 +73,7 @@ Read more about the session argument for computed fields in the [docs](https://h - server: compile with GHC 8.10.1, closing a space leak with subscriptions. (close #4517) (#3388) - server: fixes an issue where introspection queries with variables would fail because of caching (fix #4547) - server: avoid loss of precision when passing values in scientific notation (fix #4733) +- server: raise error on startup when `--unauthorized-role` is ignored (#4736) - server: fix mishandling of GeoJSON inputs in subscriptions (fix #3239) - server: fix importing of allow list query from metadata (fix #4687) - server: flush log buffer during shutdown (#4800) diff --git a/docs/graphql/manual/auth/authentication/jwt.rst b/docs/graphql/manual/auth/authentication/jwt.rst index d8eb2a619c5eb..c5da375892414 100644 --- a/docs/graphql/manual/auth/authentication/jwt.rst +++ b/docs/graphql/manual/auth/authentication/jwt.rst @@ -25,8 +25,8 @@ verified by the GraphQL engine, to authorize and get metadata about the request :alt: Authentication using JWT The JWT is decoded, the signature is verified, then it is asserted that the -current role of the user (if specified in the request) is in the list of allowed roles. -If the current role is not specified in the request, then the default role is applied. +requested role of the user (if specified in the request) is in the list of allowed roles. +If the desired role is not specified in the request, then the default role is applied. If the authorization passes, then all of the ``x-hasura-*`` values in the claim are used for the permissions system. @@ -60,7 +60,7 @@ the following: 1. A ``x-hasura-default-role`` field : indicating the default role of that user i.e. the role that will be used in case ``x-hasura-role`` header is not passed. 2. A ``x-hasura-allowed-roles`` field : a list of allowed roles for the user i.e. acceptable values of the - ``x-hasura-role`` header. + ``x-hasura-role`` header. The ``x-hasura-default-role`` specified should be a member of this list. The claims in the JWT can have other ``x-hasura-*`` fields where their values can only be strings. You can use these ``x-hasura-*`` fields in your diff --git a/server/graphql-engine.cabal b/server/graphql-engine.cabal index 26e0b4224b3f5..32bccebb2e28b 100644 --- a/server/graphql-engine.cabal +++ b/server/graphql-engine.cabal @@ -244,6 +244,7 @@ library -- Exposed for testing: , Hasura.Server.Telemetry.Counters , Data.Parser.JSONPath + , Hasura.Server.Auth.JWT , Hasura.RQL.Types , Hasura.RQL.Types.Run @@ -261,7 +262,6 @@ library , Hasura.Incremental.Internal.Dependency , Hasura.Incremental.Internal.Rule - , Hasura.Server.Auth.JWT , Hasura.Server.Auth.WebHook , Hasura.Server.Middleware , Hasura.Server.CheckUpdates @@ -434,7 +434,9 @@ test-suite graphql-engine-tests , hspec-core >=2.6.1 && <3 , hspec-expectations-lifted , http-client + , http-types , http-client-tls + , jose , lifted-base , monad-control , mtl @@ -460,6 +462,7 @@ test-suite graphql-engine-tests Hasura.RQL.MetadataSpec Hasura.Server.MigrateSpec Hasura.Server.TelemetrySpec + Hasura.Server.AuthSpec -- Benchmarks related to caching (e.g. the plan cache). -- diff --git a/server/src-lib/Hasura/GraphQL/Execute/LiveQuery/Plan.hs b/server/src-lib/Hasura/GraphQL/Execute/LiveQuery/Plan.hs index 18cb6962a2a3d..bbd11c12ced02 100644 --- a/server/src-lib/Hasura/GraphQL/Execute/LiveQuery/Plan.hs +++ b/server/src-lib/Hasura/GraphQL/Execute/LiveQuery/Plan.hs @@ -340,6 +340,8 @@ explainLiveQueryPlan :: (MonadTx m, MonadIO m) => LiveQueryPlan -> m LiveQueryPl explainLiveQueryPlan plan = do let parameterizedPlan = _lqpParameterizedPlan plan queryText = Q.getQueryText . unMultiplexedQuery $ _plqpQuery parameterizedPlan + -- CAREFUL!: an `EXPLAIN ANALYZE` here would actually *execute* this + -- query, maybe resulting in privilege escalation: explainQuery = Q.fromText $ "EXPLAIN (FORMAT TEXT) " <> queryText cohortId <- newCohortId explanationLines <- map runIdentity <$> executeQuery explainQuery [(cohortId, _lqpVariables plan)] diff --git a/server/src-lib/Hasura/GraphQL/Explain.hs b/server/src-lib/Hasura/GraphQL/Explain.hs index f491a88f46a51..18ae998c4eeb7 100644 --- a/server/src-lib/Hasura/GraphQL/Explain.hs +++ b/server/src-lib/Hasura/GraphQL/Explain.hs @@ -105,6 +105,8 @@ explainField userInfo gCtx sqlGenCtx actionExecuter fld = resolvedAST <- RS.traverseQueryRootFldAST (resolveVal userInfo) unresolvedAST let (query, remoteJoins) = RS.toPGQuery resolvedAST txtSQL = Q.getQueryText query + -- CAREFUL!: an `EXPLAIN ANALYZE` here would actually *execute* this + -- query, resulting in potential privilege escalation: withExplain = "EXPLAIN (FORMAT TEXT) " <> txtSQL -- Reject if query contains any remote joins when (remoteJoins /= mempty) $ throw400 NotSupported "Remote relationships are not allowed in explain query" @@ -128,6 +130,7 @@ explainGQLQuery -> GQLExplain -> m EncJSON explainGQLQuery pgExecCtx sc sqlGenCtx enableAL actionExecuter (GQLExplain query userVarsRaw maybeIsRelay) = do + -- NOTE!: we will be executing what follows as though admin role. See e.g. notes in explainField: userInfo <- mkUserInfo (URBFromSessionVariablesFallback adminRoleName) UAdminSecretSent sessionVariables (execPlan, queryReusability) <- runReusabilityT $ E.getExecPlanPartial userInfo sc queryType enableAL query diff --git a/server/src-lib/Hasura/Server/Auth.hs b/server/src-lib/Hasura/Server/Auth.hs index 565754ef9a226..7a70ce75d0afd 100644 --- a/server/src-lib/Hasura/Server/Auth.hs +++ b/server/src-lib/Hasura/Server/Auth.hs @@ -19,6 +19,9 @@ module Hasura.Server.Auth , processJwt , updateJwkRef , UserAuthentication (..) + + -- * Exposed for testing + , getUserInfoWithExpTime_ ) where import Control.Concurrent.Extended (forkImmortal) @@ -35,7 +38,7 @@ import qualified Network.HTTP.Types as N import Hasura.Logging import Hasura.Prelude import Hasura.RQL.Types -import Hasura.Server.Auth.JWT +import Hasura.Server.Auth.JWT hiding (processJwt_) import Hasura.Server.Auth.WebHook import Hasura.Server.Utils import Hasura.Session @@ -60,15 +63,17 @@ class (Monad m) => UserAuthentication m where -- Although this exists only in memory we store only a hash of the admin secret -- primarily in order to: -- --- - prevent theoretical timing attacks from a naive `==` +-- - prevent theoretical timing attacks from a naive `==` check -- - prevent misuse or inadvertent leaking of the secret -- --- NOTE: if we could scrub memory of admin secret (from argv and envp) somehow, --- this would additionally harden against attacks that could read arbitrary --- memory, so long as the secret was strong. I'm not sure that's attainable. newtype AdminSecretHash = AdminSecretHash (Crypto.Digest Crypto.SHA512) deriving (Ord, Eq) +-- We don't want to be able to leak the secret hash. This is a dummy instance +-- to support 'Show AuthMode' which we want for testing. +instance Show AdminSecretHash where + show _ = "(error \"AdminSecretHash hidden\")" + hashAdminSecret :: T.Text -> AdminSecretHash hashAdminSecret = AdminSecretHash . Crypto.hash . T.encodeUtf8 @@ -83,6 +88,7 @@ data AuthMode | AMAdminSecret !AdminSecretHash !(Maybe RoleName) | AMAdminSecretAndHook !AdminSecretHash !AuthHook | AMAdminSecretAndJWT !AdminSecretHash !JWTCtx !(Maybe RoleName) + deriving (Show, Eq) -- | Validate the user's requested authentication configuration, launching any -- required maintenance threads for JWT etc. @@ -102,13 +108,19 @@ setupAuthMode -> m AuthMode setupAuthMode mAdminSecretHash mWebHook mJwtSecret mUnAuthRole httpManager logger = case (mAdminSecretHash, mWebHook, mJwtSecret) of - (Nothing, Nothing, Nothing) -> return AMNoAuth (Just hash, Nothing, Nothing) -> return $ AMAdminSecret hash mUnAuthRole - (Just hash, Just hook, Nothing) -> unAuthRoleNotReqForWebHook >> - return (AMAdminSecretAndHook hash hook) (Just hash, Nothing, Just jwtConf) -> do jwtCtx <- mkJwtCtx jwtConf return $ AMAdminSecretAndJWT hash jwtCtx mUnAuthRole + -- Nothing below this case uses unauth role. Throw a fatal error if we would otherwise ignore + -- that parameter, lest users misunderstand their auth configuration: + _ | isJust mUnAuthRole -> throwError $ + "Fatal Error: --unauthorized-role (HASURA_GRAPHQL_UNAUTHORIZED_ROLE)" + <> requiresAdminScrtMsg + <> " and is not allowed when --auth-hook (HASURA_GRAPHQL_AUTH_HOOK) is set" + + (Nothing, Nothing, Nothing) -> return AMNoAuth + (Just hash, Just hook, Nothing) -> return $ AMAdminSecretAndHook hash hook (Nothing, Just _, Nothing) -> throwError $ "Fatal Error : --auth-hook (HASURA_GRAPHQL_AUTH_HOOK)" <> requiresAdminScrtMsg @@ -122,10 +134,6 @@ setupAuthMode mAdminSecretHash mWebHook mJwtSecret mUnAuthRole httpManager logge requiresAdminScrtMsg = " requires --admin-secret (HASURA_GRAPHQL_ADMIN_SECRET) or " <> " --access-key (HASURA_GRAPHQL_ACCESS_KEY) to be set" - unAuthRoleNotReqForWebHook = - when (isJust mUnAuthRole) $ throwError $ - "Fatal Error: --unauthorized-role (HASURA_GRAPHQL_UNAUTHORIZED_ROLE) is not allowed" - <> " when --auth-hook (HASURA_GRAPHQL_AUTH_HOOK) is set" -- | Given the 'JWTConfig' (the user input of JWT configuration), create -- the 'JWTCtx' (the runtime JWT config used) @@ -177,12 +185,30 @@ getUserInfoWithExpTime -> [N.Header] -> AuthMode -> m (UserInfo, Maybe UTCTime) -getUserInfoWithExpTime logger manager rawHeaders = \case +getUserInfoWithExpTime = getUserInfoWithExpTime_ userInfoFromAuthHook processJwt + +-- Broken out for testing with mocks: +getUserInfoWithExpTime_ + :: forall m _Manager _Logger_Hasura. (MonadIO m, MonadError QErr m) + => (_Logger_Hasura -> _Manager -> AuthHook -> [N.Header] -> m (UserInfo, Maybe UTCTime)) + -- ^ mock 'userInfoFromAuthHook' + -> (JWTCtx -> [N.Header] -> Maybe RoleName -> m (UserInfo, Maybe UTCTime)) + -- ^ mock 'processJwt' + -> _Logger_Hasura + -> _Manager + -> [N.Header] + -> AuthMode + -> m (UserInfo, Maybe UTCTime) +getUserInfoWithExpTime_ userInfoFromAuthHook_ processJwt_ logger manager rawHeaders = \case AMNoAuth -> withNoExpTime $ mkUserInfoFallbackAdminRole UAuthNotSet + -- If hasura was started with an admin secret we: + -- - check if a secret was sent in the request + -- - if so, check it and authorize as admin else fail + -- - if not proceed with either webhook or JWT auth if configured AMAdminSecret realAdminSecretHash maybeUnauthRole -> - withAuthorization realAdminSecretHash $ withNoExpTime $ + checkingSecretIfSent realAdminSecretHash $ withNoExpTime $ -- Consider unauthorized role, if not found raise admin secret header required exception case maybeUnauthRole of Nothing -> throw401 $ adminSecretHeader <> "/" @@ -191,21 +217,22 @@ getUserInfoWithExpTime logger manager rawHeaders = \case mkUserInfo (URBPreDetermined unAuthRole) UAdminSecretNotSent sessionVariables AMAdminSecretAndHook realAdminSecretHash hook -> - withAuthorization realAdminSecretHash $ userInfoFromAuthHook logger manager hook rawHeaders + checkingSecretIfSent realAdminSecretHash $ userInfoFromAuthHook_ logger manager hook rawHeaders AMAdminSecretAndJWT realAdminSecretHash jwtSecret unAuthRole -> - withAuthorization realAdminSecretHash $ processJwt jwtSecret rawHeaders unAuthRole + checkingSecretIfSent realAdminSecretHash $ processJwt_ jwtSecret rawHeaders unAuthRole where + -- CAREFUL!: mkUserInfoFallbackAdminRole adminSecretState = mkUserInfo (URBFromSessionVariablesFallback adminRoleName) adminSecretState sessionVariables sessionVariables = mkSessionVariables rawHeaders - withAuthorization + checkingSecretIfSent :: AdminSecretHash -> m (UserInfo, Maybe UTCTime) -> m (UserInfo, Maybe UTCTime) - withAuthorization realAdminSecretHash actionIfNoAdminSecret = do + checkingSecretIfSent realAdminSecretHash actionIfNoAdminSecret = do let maybeRequestAdminSecret = foldl1 (<|>) $ map (`getSessionVariableValue` sessionVariables) [adminSecretHeader, deprecatedAccessKeyHeader] diff --git a/server/src-lib/Hasura/Server/Auth/JWT.hs b/server/src-lib/Hasura/Server/Auth/JWT.hs index 19c98228b25c0..a5ecbce4e68fd 100644 --- a/server/src-lib/Hasura/Server/Auth/JWT.hs +++ b/server/src-lib/Hasura/Server/Auth/JWT.hs @@ -10,6 +10,11 @@ module Hasura.Server.Auth.JWT , updateJwkRef , jwkRefreshCtrl , defaultClaimNs + + -- * Exposed for testing + , processJwt_ + , allowedRolesClaim + , defaultRoleClaim ) where import Control.Exception (try) @@ -88,6 +93,7 @@ data JWTConfig data JWTCtx = JWTCtx { jcxKey :: !(IORef Jose.JWKSet) + -- ^ This needs to be a mutable variable for 'updateJwkRef'. , jcxClaimNs :: !JWTConfigClaims , jcxAudience :: !(Maybe Jose.Audience) , jcxClaimsFormat :: !JWTClaimsFormat @@ -105,6 +111,8 @@ data HasuraClaims } deriving (Show, Eq) $(J.deriveJSON (J.aesonDrop 3 J.snakeCase) ''HasuraClaims) + +-- NOTE: these must stay lowercase; TODO consider using "Data.CaseInsensitive" allowedRolesClaim :: T.Text allowedRolesClaim = "x-hasura-allowed-roles" @@ -211,6 +219,15 @@ updateJwkRef (Logger logger) manager url jwkRef = do -- | Process the request headers to verify the JWT and extract UserInfo from it +-- +-- Iff no "Authorization" header was passed, we will fall back to the +-- unauthenticated user role [1], if one was configured at server start. +-- +-- When no 'x-hasura-user-role' is specified in the request, the mandatory +-- 'x-hasura-default-role' [2] from the JWT claims will be used. + +-- [1]: https://hasura.io/docs/1.0/graphql/manual/auth/authentication/unauthenticated-access.html +-- [2]: https://hasura.io/docs/1.0/graphql/manual/auth/authentication/jwt.html#the-spec processJwt :: ( MonadIO m , MonadError QErr m) @@ -218,67 +235,84 @@ processJwt -> HTTP.RequestHeaders -> Maybe RoleName -> m (UserInfo, Maybe UTCTime) -processJwt jwtCtx headers mUnAuthRole = +processJwt = processJwt_ processAuthZHeader + +-- Broken out for testing with mocks: +processJwt_ + :: (MonadError QErr m) + => (_JWTCtx -> BLC.ByteString -> m (J.Object, Maybe UTCTime)) + -- ^ mock 'processAuthZHeader' + -> _JWTCtx + -> HTTP.RequestHeaders + -> Maybe RoleName + -> m (UserInfo, Maybe UTCTime) +processJwt_ processAuthZHeader_ jwtCtx headers mUnAuthRole = maybe withoutAuthZHeader withAuthZHeader mAuthZHeader where mAuthZHeader = find (\h -> fst h == CI.mk "Authorization") headers - withAuthZHeader (_, authzHeader) = - processAuthZHeader jwtCtx headers $ BL.fromStrict authzHeader - + withAuthZHeader (_, authzHeader) = do + (hasuraClaims, expTimeM) <- processAuthZHeader_ jwtCtx $ BL.fromStrict authzHeader + + -- filter only x-hasura claims and convert to lower-case + let claimsMap = Map.filterWithKey (\k _ -> isSessionVariable k) + $ Map.fromList $ map (first T.toLower) + $ Map.toList hasuraClaims + + HasuraClaims allowedRoles defaultRole <- parseHasuraClaims claimsMap + -- see if there is a x-hasura-role header, or else pick the default role. + -- The role returned is unauthenticated at this point: + let requestedRole = fromMaybe defaultRole $ + getRequestHeader userRoleHeader headers >>= mkRoleName . bsToTxt + + when (requestedRole `notElem` allowedRoles) $ + throw400 AccessDenied "Your requested role is not in allowed roles" + let finalClaims = + Map.delete defaultRoleClaim . Map.delete allowedRolesClaim $ claimsMap + + -- transform the map of text:aeson-value -> text:text + metadata <- parseJwtClaim (J.Object finalClaims) "x-hasura-* claims" + userInfo <- mkUserInfo (URBPreDetermined requestedRole) UAdminSecretNotSent $ + mkSessionVariablesText $ Map.toList metadata + pure (userInfo, expTimeM) + withoutAuthZHeader = do unAuthRole <- maybe missingAuthzHeader return mUnAuthRole - userInfo <- mkUserInfo (URBPreDetermined unAuthRole) UAdminSecretNotSent $ mkSessionVariables headers + userInfo <- mkUserInfo (URBPreDetermined unAuthRole) UAdminSecretNotSent $ + mkSessionVariables headers pure (userInfo, Nothing) - missingAuthzHeader = - throw400 InvalidHeaders "Missing Authorization header in JWT authentication mode" + where + missingAuthzHeader = + throw400 InvalidHeaders "Missing Authorization header in JWT authentication mode" +-- Parse and verify the 'Authorization' header, returning the raw claims +-- object, and the expiration, if any. processAuthZHeader :: ( MonadIO m , MonadError QErr m) => JWTCtx - -> HTTP.RequestHeaders -> BLC.ByteString - -> m (UserInfo, Maybe UTCTime) -processAuthZHeader jwtCtx headers authzHeader = do + -> m (J.Object, Maybe UTCTime) +processAuthZHeader jwtCtx@JWTCtx{jcxClaimNs, jcxClaimsFormat} authzHeader = do -- try to parse JWT token from Authorization header jwt <- parseAuthzHeader -- verify the JWT claims <- liftJWTError invalidJWTError $ verifyJwt jwtCtx $ RawJWT jwt - let claimsFmt = jcxClaimsFormat jwtCtx - expTimeM = fmap (\(Jose.NumericDate t) -> t) $ claims ^. Jose.claimExp + let expTimeM = fmap (\(Jose.NumericDate t) -> t) $ claims ^. Jose.claimExp -- see if the hasura claims key exists in the claims map let mHasuraClaims = - case jcxClaimNs jwtCtx of + case jcxClaimNs of ClaimNs k -> Map.lookup k $ claims ^. Jose.unregisteredClaims ClaimNsPath path -> parseIValueJsonValue $ executeJSONPath path (J.toJSON $ claims ^. Jose.unregisteredClaims) hasuraClaimsV <- maybe claimsNotFound return mHasuraClaims - -- get hasura claims value as an object. parse from string possibly - hasuraClaims <- parseObjectFromString claimsFmt hasuraClaimsV - - -- filter only x-hasura claims and convert to lower-case - let claimsMap = Map.filterWithKey (\k _ -> isSessionVariable k) - $ Map.fromList $ map (first T.toLower) - $ Map.toList hasuraClaims - - HasuraClaims allowedRoles defaultRole <- parseHasuraClaims claimsMap - let roleName = getCurrentRole defaultRole - - when (roleName `notElem` allowedRoles) currRoleNotAllowed - let finalClaims = - Map.delete defaultRoleClaim . Map.delete allowedRolesClaim $ claimsMap - - -- transform the map of text:aeson-value -> text:text - metadata <- decodeJSON $ J.Object finalClaims - userInfo <- mkUserInfo (URBPreDetermined roleName) UAdminSecretNotSent $ - mkSessionVariablesText $ Map.toList metadata - pure (userInfo, expTimeM) + -- return hasura claims value as an object. parse from string possibly + (, expTimeM) <$> parseObjectFromString hasuraClaimsV where parseAuthzHeader = do let tokenParts = BLC.words authzHeader @@ -286,8 +320,8 @@ processAuthZHeader jwtCtx headers authzHeader = do ["Bearer", jwt] -> return jwt _ -> malformedAuthzHeader - parseObjectFromString claimsFmt jVal = - case (claimsFmt, jVal) of + parseObjectFromString jVal = + case (jcxClaimsFormat, jVal) of (JCFStringifiedJson, J.String v) -> either (const $ claimsErr $ strngfyErr v) return $ J.eitherDecodeStrict $ T.encodeUtf8 v @@ -304,7 +338,7 @@ processAuthZHeader jwtCtx headers authzHeader = do where claimsLocation :: Text claimsLocation = - case jcxClaimNs jwtCtx of + case jcxClaimNs of ClaimNsPath path -> T.pack $ "claims_namespace_path " <> encodeJSONPath path ClaimNs ns -> "claims_namespace " <> ns @@ -313,15 +347,6 @@ processAuthZHeader jwtCtx headers authzHeader = do parseIValueJsonValue (J.IError _ _) = Nothing parseIValueJsonValue (J.ISuccess v) = Just v - -- see if there is a x-hasura-role header, or else pick the default role - getCurrentRole defaultRole = - let mUserRole = getRequestHeader userRoleHeader headers - in fromMaybe defaultRole $ mUserRole >>= mkRoleName . bsToTxt - - decodeJSON val = case J.fromJSON val of - J.Error e -> throw400 JWTInvalidClaims ("x-hasura-* claims: " <> T.pack e) - J.Success a -> return a - liftJWTError :: (MonadError e' m) => (e -> e') -> ExceptT e m a -> m a liftJWTError ef action = do res <- runExceptT action @@ -332,10 +357,8 @@ processAuthZHeader jwtCtx headers authzHeader = do malformedAuthzHeader = throw400 InvalidHeaders "Malformed Authorization header" - currRoleNotAllowed = - throw400 AccessDenied "Your current role is not in allowed roles" claimsNotFound = do - let claimsNsError = case jcxClaimNs jwtCtx of + let claimsNsError = case jcxClaimNs of ClaimNsPath path -> T.pack $ "claims not found at claims_namespace_path: '" <> encodeJSONPath path <> "'" ClaimNs ns -> "claims key: '" <> ns <> "' not found" @@ -343,37 +366,19 @@ processAuthZHeader jwtCtx headers authzHeader = do -- parse x-hasura-allowed-roles, x-hasura-default-role from JWT claims -parseHasuraClaims - :: (MonadError QErr m) - => J.Object -> m HasuraClaims +parseHasuraClaims :: forall m. (MonadError QErr m) => J.Object -> m HasuraClaims parseHasuraClaims claimsMap = do - let mAllowedRolesV = Map.lookup allowedRolesClaim claimsMap - allowedRolesV <- maybe missingAllowedRolesClaim return mAllowedRolesV - allowedRoles <- parseJwtClaim (J.fromJSON allowedRolesV) errMsg - - let mDefaultRoleV = Map.lookup defaultRoleClaim claimsMap - defaultRoleV <- maybe missingDefaultRoleClaim return mDefaultRoleV - defaultRole <- parseJwtClaim (J.fromJSON defaultRoleV) errMsg - - return $ HasuraClaims allowedRoles defaultRole + HasuraClaims <$> + parseClaim allowedRolesClaim "should be a list of roles" <*> + parseClaim defaultRoleClaim "should be a single role name" where - missingAllowedRolesClaim = - let msg = "JWT claim does not contain " <> allowedRolesClaim - in throw400 JWTRoleClaimMissing msg - - missingDefaultRoleClaim = - let msg = "JWT claim does not contain " <> defaultRoleClaim - in throw400 JWTRoleClaimMissing msg - - errMsg _ = "invalid " <> allowedRolesClaim <> "; should be a list of roles" - - parseJwtClaim :: (MonadError QErr m) => J.Result a -> (String -> Text) -> m a - parseJwtClaim res errFn = - case res of - J.Success val -> return val - J.Error e -> throw400 JWTInvalidClaims $ errFn e - + parseClaim :: J.FromJSON a => Text -> Text -> m a + parseClaim claim hint = do + claimV <- maybe missingClaim return $ Map.lookup claim claimsMap + parseJwtClaim claimV $ "invalid " <> claim <> "; " <> hint + where + missingClaim = throw400 JWTRoleClaimMissing $ "JWT claim does not contain " <> claim -- | Verify the JWT against given JWK verifyJwt @@ -468,3 +473,12 @@ instance J.FromJSON JWTConfig where invalidJwk msg = fail ("Invalid JWK: " <> msg) failJSONPathParsing err = fail $ "invalid JSON path claims_namespace_path error: " ++ err + + +-- Utility: +parseJwtClaim :: (J.FromJSON a, MonadError QErr m) => J.Value -> Text -> m a +parseJwtClaim v errMsg = + case J.fromJSON v of + J.Success val -> return val + J.Error e -> throw400 JWTInvalidClaims $ errMsg <> ": " <> T.pack e + diff --git a/server/src-test/Hasura/Server/AuthSpec.hs b/server/src-test/Hasura/Server/AuthSpec.hs new file mode 100644 index 0000000000000..7c5f5ca4e8970 --- /dev/null +++ b/server/src-test/Hasura/Server/AuthSpec.hs @@ -0,0 +1,408 @@ +module Hasura.Server.AuthSpec (spec) where + +import Hasura.Prelude +import Hasura.Server.Version +import Hasura.Logging + +import qualified Crypto.JOSE.JWK as Jose +import qualified Data.Aeson as J +import Data.Aeson ((.=)) +import qualified Network.HTTP.Types as N + +import Hasura.RQL.Types +import Hasura.Server.Auth hiding (getUserInfoWithExpTime, processJwt) +import Hasura.Server.Utils +import Hasura.Session +import Hasura.Server.Auth.JWT hiding (processJwt) +import Test.Hspec + +spec :: Spec +spec = do + getUserInfoWithExpTimeTests + setupAuthModeTests + +-- Unit test the core of our authentication code. This doesn't test the details +-- of resolving roles from JWT or webhook. +getUserInfoWithExpTimeTests :: Spec +getUserInfoWithExpTimeTests = describe "getUserInfo" $ do + ---- FUNCTION UNDER TEST: + let getUserInfoWithExpTime + :: J.Object + -- ^ For JWT, inject the raw claims object as though returned from 'processAuthZHeader' + -- acting on an 'Authorization' header from the request + -> [N.Header] -> AuthMode -> IO (Either Code RoleName) + getUserInfoWithExpTime claims rawHeaders = + runExceptT + . withExceptT qeCode -- just look at Code for purposes of tests + . fmap _uiRole -- just look at RoleName for purposes of tests + . fmap fst -- disregard Nothing expiration + . getUserInfoWithExpTime_ userInfoFromAuthHook processJwt () () rawHeaders + where + -- mock authorization callbacks: + userInfoFromAuthHook _ _ _hook _reqHeaders = do + (, Nothing) <$> _UserInfo "hook" + where + -- we don't care about details here; we'll just check role name in tests: + _UserInfo nm = + mkUserInfo (URBFromSessionVariablesFallback $ mkRoleNameE nm) + UAdminSecretNotSent + (mkSessionVariables mempty) + processJwt = processJwt_ $ + -- processAuthZHeader: + \_jwtCtx _authzHeader -> return (claims , Nothing) + + let setupAuthMode'E a b c d = + either (const $ error "fixme") id <$> setupAuthMode' a b c d + + let ourUnauthRole = mkRoleNameE "an0nymous" + + + describe "started without admin secret" $ do + it "gives admin by default" $ do + mode <- setupAuthMode'E Nothing Nothing Nothing Nothing + getUserInfoWithExpTime mempty [] mode + `shouldReturn` Right adminRoleName + it "allows any requested role" $ do + mode <- setupAuthMode'E Nothing Nothing Nothing Nothing + getUserInfoWithExpTime mempty [(userRoleHeader, "r00t")] mode + `shouldReturn` Right (mkRoleNameE "r00t") + + + describe "admin secret only" $ do + describe "unauth role NOT set" $ do + mode <- runIO $ setupAuthMode'E (Just $ hashAdminSecret "secret") Nothing Nothing Nothing + + it "accepts when admin secret matches" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "secret")] mode + `shouldReturn` Right adminRoleName + it "accepts when admin secret matches, honoring role request" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "secret"), (userRoleHeader, "r00t")] mode + `shouldReturn` Right (mkRoleNameE "r00t") + + it "rejects when doesn't match" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "bad secret")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [(adminSecretHeader, "")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [("blah", "blah"), (adminSecretHeader, "blah")] mode + `shouldReturn` Left AccessDenied + -- with deprecated header: + getUserInfoWithExpTime mempty [(deprecatedAccessKeyHeader, "bad secret")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [(deprecatedAccessKeyHeader, "")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [("blah", "blah"), (deprecatedAccessKeyHeader, "blah")] mode + `shouldReturn` Left AccessDenied + + it "rejects when no secret sent, since no fallback unauth role" $ do + getUserInfoWithExpTime mempty [] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [(userRoleHeader, "r00t"), (userRoleHeader, "admin")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [(userRoleHeader, "r00t")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [("blah", "blah")] mode + `shouldReturn` Left AccessDenied + + describe "unauth role set" $ do + mode <- runIO $ + setupAuthMode'E (Just $ hashAdminSecret "secret") Nothing Nothing (Just ourUnauthRole) + it "accepts when admin secret matches" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "secret")] mode + `shouldReturn` Right adminRoleName + getUserInfoWithExpTime mempty [(adminSecretHeader, "secret"), ("heh", "heh")] mode + `shouldReturn` Right adminRoleName + it "accepts when admin secret matches, honoring role request" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "secret"), (userRoleHeader, "r00t")] mode + `shouldReturn` Right (mkRoleNameE "r00t") + + it "rejects when doesn't match" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "bad secret")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [(adminSecretHeader, "")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [("blah", "blah"), (adminSecretHeader, "blah")] mode + `shouldReturn` Left AccessDenied + -- with deprecated header: + getUserInfoWithExpTime mempty [(deprecatedAccessKeyHeader, "bad secret")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [(deprecatedAccessKeyHeader, "")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [("blah", "blah"), (deprecatedAccessKeyHeader, "blah")] mode + `shouldReturn` Left AccessDenied + + it "accepts when no secret sent and unauth role defined" $ do + getUserInfoWithExpTime mempty [] mode + `shouldReturn` Right ourUnauthRole + getUserInfoWithExpTime mempty [("heh", "heh")] mode + `shouldReturn` Right ourUnauthRole + -- FIXME MAYBE (see NOTE (*)) + getUserInfoWithExpTime mempty [(userRoleHeader, "r00t")] mode + `shouldReturn` Right ourUnauthRole + + + -- Unauthorized role is not supported for webhook + describe "webhook" $ do + mode <- runIO $ + setupAuthMode'E (Just $ hashAdminSecret "secret") (Just fakeAuthHook) Nothing Nothing + + it "accepts when admin secret matches" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "secret")] mode + `shouldReturn` Right adminRoleName + it "accepts when admin secret matches, honoring role request" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "secret"), (userRoleHeader, "r00t")] mode + `shouldReturn` Right (mkRoleNameE "r00t") + + it "rejects when admin secret doesn't match" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "bad secret")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [(adminSecretHeader, "")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [("blah", "blah"), (adminSecretHeader, "blah")] mode + `shouldReturn` Left AccessDenied + -- with deprecated header: + getUserInfoWithExpTime mempty [(deprecatedAccessKeyHeader, "bad secret")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [(deprecatedAccessKeyHeader, "")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [("blah", "blah"), (deprecatedAccessKeyHeader, "blah")] mode + `shouldReturn` Left AccessDenied + + it "authenticates with webhook when no admin secret sent" $ do + getUserInfoWithExpTime mempty [] mode + `shouldReturn` Right (mkRoleNameE "hook") + getUserInfoWithExpTime mempty [("blah", "blah")] mode + `shouldReturn` Right (mkRoleNameE "hook") + getUserInfoWithExpTime mempty [(userRoleHeader, "hook")] mode + `shouldReturn` Right (mkRoleNameE "hook") + + -- FIXME MAYBE (see NOTE (*)) + it "ignores requested role, uses webhook role" $ do + getUserInfoWithExpTime mempty [(userRoleHeader, "r00t"), (userRoleHeader, "admin")] mode + `shouldReturn` Right (mkRoleNameE "hook") + getUserInfoWithExpTime mempty [(userRoleHeader, "r00t")] mode + `shouldReturn` Right (mkRoleNameE "hook") + + + -- helper for generating mocked up verified JWT token claims, as though returned by 'processAuthZHeader': + let unObject l = case J.object l of + J.Object o -> o + _ -> error "impossible" + + describe "JWT" $ do + describe "unauth role NOT set" $ do + mode <- runIO $ + setupAuthMode'E (Just $ hashAdminSecret "secret") Nothing (Just fakeJWTConfig) Nothing + + it "accepts when admin secret matches" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "secret")] mode + `shouldReturn` Right adminRoleName + it "accepts when admin secret matches, honoring role request" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "secret"), (userRoleHeader, "r00t")] mode + `shouldReturn` Right (mkRoleNameE "r00t") + + it "rejects when admin secret doesn't match" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "bad secret")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [(adminSecretHeader, "")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [("blah", "blah"), (adminSecretHeader, "blah")] mode + `shouldReturn` Left AccessDenied + -- with deprecated header: + getUserInfoWithExpTime mempty [(deprecatedAccessKeyHeader, "bad secret")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [(deprecatedAccessKeyHeader, "")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [("blah", "blah"), (deprecatedAccessKeyHeader, "blah")] mode + `shouldReturn` Left AccessDenied + + it "rejects when admin secret not sent and no 'Authorization' header" $ do + getUserInfoWithExpTime mempty [("blah", "blah")] mode + `shouldReturn` Left InvalidHeaders + getUserInfoWithExpTime mempty [] mode + `shouldReturn` Left InvalidHeaders + + describe "unauth role set" $ do + mode <- runIO $ + setupAuthMode'E (Just $ hashAdminSecret "secret") Nothing + (Just fakeJWTConfig) (Just ourUnauthRole) + + it "accepts when admin secret matches" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "secret")] mode + `shouldReturn` Right adminRoleName + getUserInfoWithExpTime mempty [(adminSecretHeader, "secret"), ("heh", "heh")] mode + `shouldReturn` Right adminRoleName + it "accepts when admin secret matches, honoring role request" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "secret"), (userRoleHeader, "r00t")] mode + `shouldReturn` Right (mkRoleNameE "r00t") + + it "rejects when admin secret doesn't match" $ do + getUserInfoWithExpTime mempty [(adminSecretHeader, "bad secret")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [(adminSecretHeader, "")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [("blah", "blah"), (adminSecretHeader, "blah")] mode + `shouldReturn` Left AccessDenied + -- with deprecated header: + getUserInfoWithExpTime mempty [(deprecatedAccessKeyHeader, "bad secret")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [(deprecatedAccessKeyHeader, "")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime mempty [("blah", "blah"), (deprecatedAccessKeyHeader, "blah")] mode + `shouldReturn` Left AccessDenied + + it "authorizes as unauth role when no 'Authorization' header" $ do + getUserInfoWithExpTime mempty [("blah", "blah")] mode + `shouldReturn` Right ourUnauthRole + getUserInfoWithExpTime mempty [] mode + `shouldReturn` Right ourUnauthRole + + describe "when Authorization header sent, and no admin secret" $ do + modeA <- runIO $ setupAuthMode'E (Just $ hashAdminSecret "secret") Nothing + (Just fakeJWTConfig) (Just ourUnauthRole) + modeB <- runIO $ setupAuthMode'E (Just $ hashAdminSecret "secret") Nothing + (Just fakeJWTConfig) Nothing + + -- Here the unauth role does not come into play at all, so map same tests over both modes: + forM_ [(modeA, "with unauth role set"), (modeB, "with unauth role NOT set")] $ + \(mode, modeMsg) -> describe modeMsg $ do + + it "authorizes successfully with JWT when requested role allowed" $ do + let claim = unObject [ allowedRolesClaim .= (["editor","user", "mod"] :: [Text]) + , defaultRoleClaim .= ("user" :: Text) + ] + getUserInfoWithExpTime claim [("Authorization", "IGNORED"), (userRoleHeader, "editor")] mode + `shouldReturn` Right (mkRoleNameE "editor") + -- Uses the defaultRoleClaim: + getUserInfoWithExpTime claim [("Authorization", "IGNORED")] mode + `shouldReturn` Right (mkRoleNameE "user") + + it "rejects when requested role is not allowed" $ do + let claim = unObject [ allowedRolesClaim .= (["editor","user", "mod"] :: [Text]) + , defaultRoleClaim .= ("user" :: Text) + ] + getUserInfoWithExpTime claim [("Authorization", "IGNORED"), (userRoleHeader, "r00t")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime claim [("Authorization", "IGNORED"), (userRoleHeader, "admin")] mode + `shouldReturn` Left AccessDenied + + -- A corner case, but the behavior seems desirable: + it "always rejects when token has empty allowedRolesClaim" $ do + let claim = unObject [ allowedRolesClaim .= ([] :: [Text]), defaultRoleClaim .= ("user" :: Text) ] + getUserInfoWithExpTime claim [("Authorization", "IGNORED"), (userRoleHeader, "admin")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime claim [("Authorization", "IGNORED"), (userRoleHeader, "user")] mode + `shouldReturn` Left AccessDenied + getUserInfoWithExpTime claim [("Authorization", "IGNORED")] mode + `shouldReturn` Left AccessDenied + + it "rejects when token doesn't have proper allowedRolesClaim and defaultRoleClaim" $ do + let claim0 = unObject [ allowedRolesClaim .= (["editor","user", "mod"] :: [Text]) ] + claim1 = unObject [ defaultRoleClaim .= ("user" :: Text) ] + claim2 = unObject [] + for_ [claim0, claim1, claim2] $ \claim -> + getUserInfoWithExpTime claim [("Authorization", "IGNORED")] mode + `shouldReturn` Left JWTRoleClaimMissing + + -- (*) FIXME NOTE (re above): + -- + -- Ideally we should always return AccessDenied if the role we would + -- otherwise have returned does not match the requested role (from the + -- 'userRoleHeader'). + -- + -- This would harden a bit against bugs, makes the spec simpler, but + -- especially is better UX since in the current behavior the user can't be + -- sure which role their query is operating as (and in the worst case we + -- might e.g. delete rows the user didn't intend) + -- + -- But this is a breaking change we need to think a little more about; + -- users might be relying on the behavior, e.g. just hardcoding a dev role + -- into clients. + + +-- Some very basic unit tests of AuthMode construction and error modes +setupAuthModeTests :: Spec +setupAuthModeTests = describe "setupAuthMode" $ do + let secret = hashAdminSecret "secret" + unauthRole = mkRoleNameE "anon" + + -- These are all various error cases, except for the AMNoAuth mode: + it "with no admin secret provided" $ do + setupAuthMode' Nothing Nothing Nothing Nothing + `shouldReturn` (Right AMNoAuth) + -- We insist on an admin secret in order to use webhook or JWT auth: + setupAuthMode' Nothing Nothing (Just fakeJWTConfig) Nothing + `shouldReturn` Left () + setupAuthMode' Nothing (Just fakeAuthHook) Nothing Nothing + `shouldReturn` Left () + -- ...and we can't have both: + setupAuthMode' Nothing (Just fakeAuthHook) (Just fakeJWTConfig) Nothing + `shouldReturn` Left () + -- If the unauthenticated role was set but would otherwise be ignored this + -- should be an error (for now), since users might expect all access to use + -- the specified role. This first case would be the real worrying one: + setupAuthMode' Nothing Nothing Nothing (Just unauthRole) + `shouldReturn` Left () + setupAuthMode' Nothing Nothing (Just fakeJWTConfig) (Just unauthRole) + `shouldReturn` Left () + setupAuthMode' Nothing (Just fakeAuthHook) Nothing (Just unauthRole) + `shouldReturn` Left () + setupAuthMode' Nothing (Just fakeAuthHook) (Just fakeJWTConfig) (Just unauthRole) + `shouldReturn` Left () + + it "with admin secret provided" $ do + setupAuthMode' (Just secret) Nothing Nothing Nothing + `shouldReturn` Right (AMAdminSecret secret Nothing) + setupAuthMode' (Just secret) Nothing Nothing (Just unauthRole) + `shouldReturn` Right (AMAdminSecret secret $ Just unauthRole) + + setupAuthMode' (Just secret) Nothing (Just fakeJWTConfig) Nothing >>= \case + Right (AMAdminSecretAndJWT s _ Nothing) -> do + s `shouldBe` secret + _ -> expectationFailure "AMAdminSecretAndJWT" + setupAuthMode' (Just secret) Nothing (Just fakeJWTConfig) (Just unauthRole) >>= \case + Right (AMAdminSecretAndJWT s _ ur) -> do + s `shouldBe` secret + ur `shouldBe` Just unauthRole + _ -> expectationFailure "AMAdminSecretAndJWT" + + setupAuthMode' (Just secret) (Just fakeAuthHook) Nothing Nothing + `shouldReturn` Right (AMAdminSecretAndHook secret fakeAuthHook) + -- auth hook can't make use of unauthenticated role for now (no good UX): + setupAuthMode' (Just secret) (Just fakeAuthHook) Nothing (Just unauthRole) + `shouldReturn` Left () + -- we can't have both: + setupAuthMode' (Just secret) (Just fakeAuthHook) (Just fakeJWTConfig) Nothing + `shouldReturn` Left () + setupAuthMode' (Just secret) (Just fakeAuthHook) (Just fakeJWTConfig) (Just unauthRole) + `shouldReturn` Left () + +fakeJWTConfig :: JWTConfig +fakeJWTConfig = + let jcKeyOrUrl = Left (Jose.fromOctets []) + jcClaimNs = ClaimNs "" + jcAudience = Nothing + jcClaimsFormat = Nothing + jcIssuer = Nothing + in JWTConfig{..} + +fakeAuthHook :: AuthHook +fakeAuthHook = AuthHookG "http://fake" AHTGet + +mkRoleNameE :: Text -> RoleName +mkRoleNameE = fromMaybe (error "fixme") . mkRoleName + +setupAuthMode' + :: Maybe AdminSecretHash + -> Maybe AuthHook + -> Maybe JWTConfig + -> Maybe RoleName + -> IO (Either () AuthMode) +setupAuthMode' mAdminSecretHash mWebHook mJwtSecret mUnAuthRole = + withVersion (VersionDev "fake") $ + -- just throw away the error message for ease of testing: + fmap (either (const $ Left ()) Right) $ + runExceptT $ + setupAuthMode mAdminSecretHash mWebHook mJwtSecret mUnAuthRole + -- NOTE: this won't do any http or launch threads if we don't specify JWT URL: + (error "H.Manager") (Logger $ void . return) diff --git a/server/src-test/Main.hs b/server/src-test/Main.hs index f6c87130713aa..21e0183bb477b 100644 --- a/server/src-test/Main.hs +++ b/server/src-test/Main.hs @@ -34,6 +34,7 @@ import qualified Hasura.IncrementalSpec as IncrementalSpec -- import qualified Hasura.RQL.MetadataSpec as MetadataSpec import qualified Hasura.Server.MigrateSpec as MigrateSpec import qualified Hasura.Server.TelemetrySpec as TelemetrySpec +import qualified Hasura.Server.AuthSpec as AuthSpec data TestSuites = AllSuites !RawConnInfo @@ -64,6 +65,7 @@ unitSpecs = do -- describe "Hasura.RQL.Metadata" MetadataSpec.spec -- Commenting until optimizing the test in CI describe "Data.Time" TimeSpec.spec describe "Hasura.Server.Telemetry" TelemetrySpec.spec + describe "Hasura.Server.Auth" AuthSpec.spec buildPostgresSpecs :: (HasVersion) => RawConnInfo -> IO Spec buildPostgresSpecs pgConnOptions = do diff --git a/server/tests-py/test_jwt.py b/server/tests-py/test_jwt.py index 74aa08ea26f78..012092fcc5477 100644 --- a/server/tests-py/test_jwt.py +++ b/server/tests-py/test_jwt.py @@ -76,7 +76,7 @@ def test_jwt_invalid_role_in_request_header(self, hge_ctx, endpoint): 'code': 'access-denied', 'path': '$' }, - 'message': 'Your current role is not in allowed roles' + 'message': 'Your requested role is not in allowed roles' }] } self.conf['url'] = endpoint @@ -133,7 +133,7 @@ def test_jwt_invalid_allowed_roles_in_claim(self, hge_ctx, endpoint): 'code': 'jwt-invalid-claims', 'path': '$' }, - 'message': 'invalid x-hasura-allowed-roles; should be a list of roles' + 'message': 'invalid x-hasura-allowed-roles; should be a list of roles: parsing [] failed, expected Array, but encountered String' }] } self.conf['url'] = endpoint