From e7a242f3e685e03690b883fb1f667087b2188792 Mon Sep 17 00:00:00 2001 From: Brandon Simmons Date: Tue, 19 May 2020 10:48:49 -0400 Subject: [PATCH] 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