From 6dce62e749463cc59d85beb4738c4bd6cfe2ffd2 Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Thu, 16 Nov 2023 10:46:59 +0100 Subject: [PATCH 01/15] Add pwd verification cascading --- hack/bin/.envrc | 1 + libs/types-common/src/Data/Misc.hs | 6 + libs/wire-api/src/Wire/API/Password.hs | 204 +++++++++++++++--- .../test/unit/Test/Wire/API/Password.hs | 23 +- services/brig/src/Brig/API/User.hs | 5 +- services/brig/src/Brig/Effects/JwtTools.hs | 6 + services/brig/src/Brig/Provider/API.hs | 2 + services/galley/src/Galley/API/Util.hs | 2 +- 8 files changed, 209 insertions(+), 40 deletions(-) create mode 100644 hack/bin/.envrc diff --git a/hack/bin/.envrc b/hack/bin/.envrc new file mode 100644 index 0000000000..f8a82b5abd --- /dev/null +++ b/hack/bin/.envrc @@ -0,0 +1 @@ +use flake github:wireapp/ghc-flakr -Lv diff --git a/libs/types-common/src/Data/Misc.hs b/libs/types-common/src/Data/Misc.hs index 04a4e04644..23a404e2c0 100644 --- a/libs/types-common/src/Data/Misc.hs +++ b/libs/types-common/src/Data/Misc.hs @@ -59,6 +59,7 @@ module Data.Misc FutureWork (..), from64, readT, + showT, ) where @@ -367,6 +368,11 @@ readT :: Read a => Text -> Maybe a readT = readMaybe . Text.unpack {-# INLINE readT #-} +-- | Same as 'show' but works on 'Text' +showT :: Show a => a -> Text +showT = Text.pack . show +{-# INLINE showT #-} + -- | Decodes a base64 'Text' to a regular 'ByteString' (if possible) from64 :: Text -> Maybe ByteString from64 = hush . B64.decode . encodeUtf8 diff --git a/libs/wire-api/src/Wire/API/Password.hs b/libs/wire-api/src/Wire/API/Password.hs index 2fc9e88a83..d7d935e12c 100644 --- a/libs/wire-api/src/Wire/API/Password.hs +++ b/libs/wire-api/src/Wire/API/Password.hs @@ -16,17 +16,23 @@ -- You should have received a copy of the GNU Affero General Public License along -- with this program. If not, see . {-# OPTIONS_GHC -Wno-incomplete-uni-patterns #-} +{-# OPTIONS_GHC -Wno-unused-top-binds #-} module Wire.API.Password ( Password, + PasswordStatus (..), genPassword, mkSafePassword, + mkSafePasswordArgon2id, verifyPassword, + verifyPasswordWithStatus, unsafeMkPassword, ) where import Cassandra +import Crypto.Error +import Crypto.KDF.Argon2 qualified as Argon2 import Crypto.KDF.Scrypt as Scrypt import Crypto.Random import Data.ByteArray hiding (length) @@ -57,8 +63,15 @@ instance Cql Password where unsafeMkPassword :: Text -> Password unsafeMkPassword = Password +data PasswordStatus + = PasswordStatusOk + | PasswordStatusNeedsUpdate + deriving (Show, Eq) + ------------------------------------------------------------------------------- +type Argon2idOptions = Argon2.Options + data ScryptParameters = ScryptParameters { -- | Bytes to randomly generate as a unique salt, default is __32__ saltLength :: Word32, @@ -87,6 +100,16 @@ defaultParams = outputLength = 64 } +defaultOptions :: Argon2idOptions +defaultOptions = + Argon2.Options + { iterations = 5, + memory = 2 ^ (17 :: Int), + parallelism = 4, + variant = Argon2.Argon2id, + version = Argon2.Version13 + } + fromScrypt :: ScryptParameters -> Parameters fromScrypt scryptParams = Parameters @@ -107,57 +130,119 @@ genPassword = -- | Stretch a plaintext password so that it can be safely stored. mkSafePassword :: MonadIO m => PlainTextPassword' t -> m Password -mkSafePassword = fmap Password . hashPassword . Text.encodeUtf8 . fromPlainTextPassword +mkSafePassword = fmap Password . hashPasswordScrypt . Text.encodeUtf8 . fromPlainTextPassword + +mkSafePasswordArgon2id :: MonadIO m => PlainTextPassword' t -> m Password +mkSafePasswordArgon2id = fmap Password . hashPasswordArgon2id . Text.encodeUtf8 . fromPlainTextPassword -- | Verify a plaintext password from user input against a stretched -- password from persistent storage. verifyPassword :: PlainTextPassword' t -> Password -> Bool -verifyPassword plain opaque = +verifyPassword = (fst .) . verifyPasswordWithStatus + +verifyPasswordWithStatus :: PlainTextPassword' t -> Password -> (Bool, PasswordStatus) +verifyPasswordWithStatus plain opaque = let actual = fromPlainTextPassword plain expected = fromPassword opaque - in checkPassword actual expected + in -- TODO(elland): We'll use the pwd status later to update the hash behind the scenes + checkPassword actual expected -hashPassword :: MonadIO m => ByteString -> m Text -hashPassword password = do +hashPasswordArgon2id :: MonadIO m => ByteString -> m Text +hashPasswordArgon2id pwd = do salt <- newSalt $ fromIntegral defaultParams.saltLength - let key = hashPasswordWithSalt password salt + let key = hashPasswordWithOptions defaultOptions pwd salt + opts = + Text.intercalate + "," + [ "m=" <> showT defaultOptions.memory, + "t=" <> showT defaultOptions.iterations, + "p=" <> showT defaultOptions.parallelism + ] + pure $ + "$argon2" + <> Text.intercalate + "$" + [ variantToLetter defaultOptions.variant, + "v=" <> versionToNum defaultOptions.version, + opts, + encodeWithoutPadding salt, + encodeWithoutPadding key + ] + where + encodeWithoutPadding = Text.dropWhileEnd (== '=') . Text.decodeUtf8 . B64.encode + +hashPasswordScrypt :: MonadIO m => ByteString -> m Text +hashPasswordScrypt password = do + salt <- newSalt $ fromIntegral defaultParams.saltLength + let key = hashPasswordWithParams defaultParams password salt pure $ Text.intercalate "|" - [ "14", - "8", - "1", + [ showT defaultParams.rounds, + showT defaultParams.blockSize, + showT defaultParams.parallelism, Text.decodeUtf8 . B64.encode $ salt, Text.decodeUtf8 . B64.encode $ key ] -hashPasswordWithSalt :: ByteString -> ByteString -> ByteString -hashPasswordWithSalt password salt = hashPasswordWithParams defaultParams password salt - -hashPasswordWithParams :: - ( ByteArrayAccess password, - ByteArrayAccess salt - ) => - ScryptParameters -> - password -> - salt -> - ByteString -hashPasswordWithParams parameters password salt = convert (generate (fromScrypt parameters) password salt :: Bytes) - -checkPassword :: Text -> Text -> Bool -checkPassword actual expected = fromMaybe False $ do - (sparams, salt, hashedKey) <- parseScryptPasswordHashParams $ Text.encodeUtf8 expected - let producedKey = hashPasswordWithParams sparams (Text.encodeUtf8 actual) salt - pure $ hashedKey `constEq` producedKey +checkPassword :: Text -> Text -> (Bool, PasswordStatus) +checkPassword actual expected = + case parseArgon2idPasswordHashOptions expected of + Just (opts, salt, hashedKey) -> + let producedKey = hashPasswordWithOptions opts (Text.encodeUtf8 actual) salt + in (hashedKey `constEq` producedKey, PasswordStatusOk) + Nothing -> + case parseScryptPasswordHashParams $ Text.encodeUtf8 expected of + Just (sparams, saltS, hashedKeyS) -> + let producedKeyS = hashPasswordWithParams sparams (Text.encodeUtf8 actual) saltS + in (hashedKeyS `constEq` producedKeyS, PasswordStatusNeedsUpdate) + Nothing -> (False, PasswordStatusNeedsUpdate) newSalt :: MonadIO m => Int -> m ByteString newSalt i = liftIO $ getRandomBytes i {-# INLINE newSalt #-} +parseArgon2idPasswordHashOptions :: Text -> Maybe (Argon2idOptions, ByteString, ByteString) +parseArgon2idPasswordHashOptions passwordHash = do + let paramList = Text.split (== '$') passwordHash + guard (length paramList >= 5) + let (_ : variantT : vp : ps : sh : rest) = paramList + variant <- parseVariant variantT + case rest of + [hashedKey64] -> do + version <- parseVersion vp + parseAll variant version ps sh hashedKey64 + [] -> parseAll variant Argon2.Version10 vp ps sh + _ -> Nothing + where + parseVariant = splitMaybe "argon2" letterToVariant + parseVersion = splitMaybe "v=" numToVersion + +parseAll :: Argon2.Variant -> Argon2.Version -> Text -> Text -> Text -> Maybe (Argon2idOptions, ByteString, ByteString) +parseAll variant version parametersT salt64 hashedKey64 = do + (memory, iterations, parallelism) <- parseParameters parametersT + salt <- from64 $ unsafePad64 salt64 + hashedKey <- from64 $ unsafePad64 hashedKey64 + pure (Argon2.Options {..}, salt, hashedKey) + where + parseParameters paramsT = do + let paramsL = Text.split (== ',') paramsT + guard $ Imports.length paramsL == 3 + go paramsL (Nothing, Nothing, Nothing) + where + go [] (Just m, Just t, Just p) = Just (m, t, p) + go [] _ = Nothing + go (x : xs) (m, t, p) = + case Text.splitAt 2 x of + ("m=", i) -> go xs (readT i, t, p) + ("t=", i) -> go xs (m, readT i, p) + ("p=", i) -> go xs (m, t, readT i) + _ -> Nothing + parseScryptPasswordHashParams :: ByteString -> Maybe (ScryptParameters, ByteString, ByteString) parseScryptPasswordHashParams passwordHash = do let paramList = Text.split (== '|') . Text.decodeUtf8 $ passwordHash - guard $ length paramList == 5 + guard (length paramList == 5) let [ scryptRoundsT, scryptBlockSizeT, scryptParallelismT, @@ -176,3 +261,66 @@ parseScryptPasswordHashParams passwordHash = do salt, hashedKey ) + +------------------------------------------------------------------------------- + +hashPasswordWithOptions :: Argon2idOptions -> ByteString -> ByteString -> ByteString +hashPasswordWithOptions opts password salt = + case (Argon2.hash opts password salt 64) of + CryptoFailed cErr -> error $ "Impossible error: " <> show cErr + CryptoPassed hash -> hash + +hashPasswordWithParams :: + ( ByteArrayAccess password, + ByteArrayAccess salt + ) => + ScryptParameters -> + password -> + salt -> + ByteString +hashPasswordWithParams parameters password salt = convert (generate (fromScrypt parameters) password salt :: Bytes) + +-------------------------------------------------------------------------------- + +-- | Makes a letter out of the variant +variantToLetter :: Argon2.Variant -> Text +variantToLetter = \case + Argon2.Argon2i -> "i" + Argon2.Argon2d -> "d" + Argon2.Argon2id -> "id" + +-- | Parses the variant parameter in the encoded hash +letterToVariant :: Text -> Maybe Argon2.Variant +letterToVariant = \case + "i" -> Just Argon2.Argon2i + "d" -> Just Argon2.Argon2d + "id" -> Just Argon2.Argon2id + _ -> Nothing + +-- | Parses the "v=" parameter in the encoded hash +numToVersion :: Text -> Maybe Argon2.Version +numToVersion "16" = Just Argon2.Version10 +numToVersion "19" = Just Argon2.Version13 +numToVersion _ = Nothing + +-- | Makes number for the "v=" parameter in the encoded hash +versionToNum :: Argon2.Version -> Text +versionToNum Argon2.Version10 = "16" +versionToNum Argon2.Version13 = "19" + +-- | Strips the given 'match' if it matches and uses +-- the function on the remainder of the given text. +splitMaybe :: Text -> (Text -> Maybe a) -> Text -> Maybe a +splitMaybe match f t = + Text.stripPrefix match t >>= f + +-- | (UNSAFE) Pad a base64 text to "length `rem` 4 == 0" with "=" +-- +-- prop> \bs -> let b64 = encodeBase64 bs in unsafePad64 (T.dropWhileEnd (== '=') b64) == b64 +unsafePad64 :: Text -> Text +unsafePad64 t + | remains == 0 = t + | otherwise = t <> pad + where + remains = Text.length t `rem` 4 + pad = Text.replicate (4 - remains) "=" diff --git a/libs/wire-api/test/unit/Test/Wire/API/Password.hs b/libs/wire-api/test/unit/Test/Wire/API/Password.hs index c0958bb88f..331dc45959 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Password.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Password.hs @@ -27,23 +27,26 @@ import Wire.API.Password tests :: TestTree tests = testGroup "Password" $ - [ testCase "hash password" testHashPassword, - testCase "verify compat" verifyPasswordHashingRemainsCompatible + [ testCase "hash password argon2id" testHashPasswordArgon2id, + testCase "verify old scrypt password still works" testHashingOldScrypt ] -testHashPassword :: IO () -testHashPassword = do +testHashPasswordArgon2id :: IO () +testHashPasswordArgon2id = do pwd <- genPassword - hashed <- mkSafePassword pwd - let correct = verifyPassword pwd hashed + hashed <- mkSafePasswordArgon2id pwd + let (correct, status) = verifyPasswordWithStatus pwd hashed assertBool "Password could not be verified" correct + assertBool "Password could not be verified" (status == PasswordStatusOk) -verifyPasswordHashingRemainsCompatible :: IO () -verifyPasswordHashingRemainsCompatible = do - forConcurrently_ pwds $ \pwd -> +testHashingOldScrypt :: IO () +testHashingOldScrypt = + forConcurrently_ pwds $ \pwd -> do let orig = plainTextPassword8Unsafe (fst pwd) expected = unsafeMkPassword (snd pwd) - in assertBool "Oops" (verifyPassword orig expected) + (correct, status) = (verifyPasswordWithStatus orig expected) + assertBool "Password did not match hash." correct + assertBool "Password could not be verified" (status == PasswordStatusNeedsUpdate) where -- Password and hashes generated using the old code, but verified using the new one. pwds = diff --git a/services/brig/src/Brig/API/User.hs b/services/brig/src/Brig/API/User.hs index 6278c13d24..73d8d18898 100644 --- a/services/brig/src/Brig/API/User.hs +++ b/services/brig/src/Brig/API/User.hs @@ -1086,6 +1086,7 @@ changePassword uid cp = do (Nothing, _) -> lift . wrapClient $ Data.updatePassword uid newpw (Just _, Nothing) -> throwE InvalidCurrentPassword (Just pw, Just pw') -> do + -- We are updating the pwd here anyway, so we don't care about the pwd status unless (verifyPassword pw' pw) $ throwE InvalidCurrentPassword when (verifyPassword newpw pw) $ @@ -1135,7 +1136,8 @@ checkNewIsDifferent :: UserId -> PlainTextPassword' t -> ExceptT PasswordResetEr checkNewIsDifferent uid pw = do mcurrpw <- lift . wrapClient $ Data.lookupPassword uid case mcurrpw of - Just currpw | verifyPassword pw currpw -> throwE ResetPasswordMustDiffer + Just currpw + | (verifyPassword pw currpw) -> throwE ResetPasswordMustDiffer _ -> pure () mkPasswordResetKey :: @@ -1209,6 +1211,7 @@ deleteSelfUser uid pwd = do case actual of Nothing -> throwE DeleteUserInvalidPassword Just p -> do + -- We're deleting a user, no sense in updating their pwd, so we ignore pwd status unless (verifyPassword pw p) $ throwE DeleteUserInvalidPassword lift $ wrapHttpClient $ deleteAccount a >> pure Nothing diff --git a/services/brig/src/Brig/Effects/JwtTools.hs b/services/brig/src/Brig/Effects/JwtTools.hs index f31329c5aa..06b5229de1 100644 --- a/services/brig/src/Brig/Effects/JwtTools.hs +++ b/services/brig/src/Brig/Effects/JwtTools.hs @@ -6,7 +6,10 @@ import Brig.API.Types (CertEnrollmentError (..)) import Control.Monad.Trans.Except import Data.ByteString.Conversion import Data.Either.Extra +<<<<<<< HEAD import Data.Handle (Handle, fromHandle) +======= +>>>>>>> b9782cdb1 (Add pwd verification cascading) import Data.Id import Data.Jwt.Tools qualified as Jwt import Data.Misc (HttpsUrl) @@ -14,7 +17,10 @@ import Data.Nonce (Nonce) import Data.PEMKeys import Imports import Network.HTTP.Types (StdMethod (..)) +<<<<<<< HEAD import Network.HTTP.Types qualified as HTTP +======= +>>>>>>> b9782cdb1 (Add pwd verification cascading) import Polysemy import Wire.API.MLS.Credential (ClientIdentity (..)) import Wire.API.MLS.Epoch (Epoch (..)) diff --git a/services/brig/src/Brig/Provider/API.hs b/services/brig/src/Brig/Provider/API.hs index 65001303de..d6cb969815 100644 --- a/services/brig/src/Brig/Provider/API.hs +++ b/services/brig/src/Brig/Provider/API.hs @@ -477,6 +477,7 @@ deleteService :: deleteService pid sid del = do guardSecondFactorDisabled Nothing pass <- wrapClientE (DB.lookupPassword pid) >>= maybeBadCredentials + -- We don't care about pwd status when deleting things unless (verifyPassword (deleteServicePassword del) pass) $ throwStd (errorToWai @'E.BadCredentials) _ <- wrapClientE (DB.lookupService pid sid) >>= maybeServiceNotFound @@ -521,6 +522,7 @@ deleteAccount pid del = do guardSecondFactorDisabled Nothing prov <- wrapClientE (DB.lookupAccount pid) >>= maybeInvalidProvider pass <- wrapClientE (DB.lookupPassword pid) >>= maybeBadCredentials + -- We don't care about pwd status when deleting things unless (verifyPassword (deleteProviderPassword del) pass) $ throwStd (errorToWai @'E.BadCredentials) svcs <- wrapClientE $ DB.listServices pid diff --git a/services/galley/src/Galley/API/Util.hs b/services/galley/src/Galley/API/Util.hs index f3c0c1cfe7..491b542e58 100644 --- a/services/galley/src/Galley/API/Util.hs +++ b/services/galley/src/Galley/API/Util.hs @@ -81,7 +81,7 @@ import Wire.API.Event.Conversation import Wire.API.Federation.API import Wire.API.Federation.API.Galley import Wire.API.Federation.Error -import Wire.API.Password (verifyPassword) +import Wire.API.Password import Wire.API.Routes.Public.Galley.Conversation import Wire.API.Routes.Public.Util import Wire.API.Team.Member From c847afaec9c53f80399259c60d0b33104157ea97 Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Tue, 21 Nov 2023 16:24:00 +0100 Subject: [PATCH 02/15] Hi ci From 124351c1307b9d02e483a914a06ef230d50a4c0d Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Mon, 8 Jan 2024 16:34:58 +0100 Subject: [PATCH 03/15] WIP: auto-update pwd --- services/brig/src/Brig/Data/User.hs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/services/brig/src/Brig/Data/User.hs b/services/brig/src/Brig/Data/User.hs index d170ed4e42..9ccb592626 100644 --- a/services/brig/src/Brig/Data/User.hs +++ b/services/brig/src/Brig/Data/User.hs @@ -197,8 +197,13 @@ authenticate u pw = Just (_, PendingInvitation) -> throwE AuthPendingInvitation Just (Nothing, _) -> throwE AuthInvalidCredentials Just (Just pw', Active) -> - unless (verifyPassword pw pw') $ - throwE AuthInvalidCredentials + case verifyPasswordWithStatus pw pw' of + (False, _) -> throwE AuthInvalidCredentials + (True, PasswordStatusNeedsUpdate) -> do + case plainTextPassword8 . fromPlainTextPassword $ pw of + Nothing -> throwE AuthInvalidCredentials -- TODO(elland): Add new error type + Just pw8 -> updatePassword u pw8 -- update pwd in place + (True, _) -> pure () -- | Password reauthentication. If the account has a password, reauthentication -- is mandatory. If the account has no password, or is an SSO user, and no password is given, From 8fdba6bebf704a2b803eb973832612e162e84898 Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Tue, 9 Jan 2024 09:53:33 +0100 Subject: [PATCH 04/15] Update password hash in place when possible --- libs/wire-api/src/Wire/API/Error/Brig.hs | 3 +++ services/brig/src/Brig/API/Error.hs | 2 ++ services/brig/src/Brig/API/Types.hs | 1 + services/brig/src/Brig/Data/User.hs | 3 ++- services/brig/src/Brig/User/Auth.hs | 2 ++ 5 files changed, 10 insertions(+), 1 deletion(-) diff --git a/libs/wire-api/src/Wire/API/Error/Brig.hs b/libs/wire-api/src/Wire/API/Error/Brig.hs index 5552453b22..71eb5628a7 100644 --- a/libs/wire-api/src/Wire/API/Error/Brig.hs +++ b/libs/wire-api/src/Wire/API/Error/Brig.hs @@ -92,6 +92,7 @@ data BrigError | InvalidProvider | ProviderNotFound | TeamsNotFederating + | PasswordIsStale instance (Typeable (MapError e), KnownError (MapError e)) => IsSwaggerError (e :: BrigError) where addToOpenApi = addStaticErrorToSwagger @(MapError e) @@ -275,3 +276,5 @@ type instance MapError 'PendingInvitationNotFound = 'StaticError 404 "not-found" type instance MapError 'ConflictingInvitations = 'StaticError 409 "conflicting-invitations" "Multiple conflicting invitations to different teams exists." type instance MapError 'TeamsNotFederating = 'StaticError 403 "team-not-federating" "The target user is owned by a federated backend, but is not in an allow-listed team" + +type instance MapError 'PasswordIsStale = 'StaticError 403 "password-is-stale" "The password is too old, please update your password." diff --git a/services/brig/src/Brig/API/Error.hs b/services/brig/src/Brig/API/Error.hs index 11786491a9..09110cd970 100644 --- a/services/brig/src/Brig/API/Error.hs +++ b/services/brig/src/Brig/API/Error.hs @@ -125,6 +125,7 @@ loginError LoginFailed = StdError (errorToWai @'E.BadCredentials) loginError LoginSuspended = StdError (errorToWai @'E.AccountSuspended) loginError LoginEphemeral = StdError (errorToWai @'E.AccountEphemeral) loginError LoginPendingActivation = StdError (errorToWai @'E.AccountPending) +loginError LoginPasswordUpdateRequired = StdError (errorToWai @'E.PasswordIsStale) loginError (LoginThrottled wait) = RichError loginsTooFrequent @@ -144,6 +145,7 @@ authError AuthInvalidCredentials = StdError (errorToWai @'E.BadCredentials) authError AuthSuspended = StdError (errorToWai @'E.AccountSuspended) authError AuthEphemeral = StdError (errorToWai @'E.AccountEphemeral) authError AuthPendingInvitation = StdError (errorToWai @'E.AccountPending) +authError AuthStalePassword = StdError (errorToWai @'E.PasswordIsStale) reauthError :: ReAuthError -> Error reauthError ReAuthMissingPassword = StdError (errorToWai @'E.MissingAuth) diff --git a/services/brig/src/Brig/API/Types.hs b/services/brig/src/Brig/API/Types.hs index 34fb803bc6..ea7a6ec38e 100644 --- a/services/brig/src/Brig/API/Types.hs +++ b/services/brig/src/Brig/API/Types.hs @@ -153,6 +153,7 @@ data LoginError | LoginBlocked RetryAfter | LoginCodeRequired | LoginCodeInvalid + | LoginPasswordUpdateRequired data VerificationCodeError = VerificationCodeRequired diff --git a/services/brig/src/Brig/Data/User.hs b/services/brig/src/Brig/Data/User.hs index 9ccb592626..939c9bf1b4 100644 --- a/services/brig/src/Brig/Data/User.hs +++ b/services/brig/src/Brig/Data/User.hs @@ -107,6 +107,7 @@ data AuthError | AuthSuspended | AuthEphemeral | AuthPendingInvitation + | AuthStalePassword -- | Re-authentication errors. data ReAuthError @@ -201,7 +202,7 @@ authenticate u pw = (False, _) -> throwE AuthInvalidCredentials (True, PasswordStatusNeedsUpdate) -> do case plainTextPassword8 . fromPlainTextPassword $ pw of - Nothing -> throwE AuthInvalidCredentials -- TODO(elland): Add new error type + Nothing -> pure () -- throwE AuthStalePassword -- TODO(elland): allowed for now Just pw8 -> updatePassword u pw8 -- update pwd in place (True, _) -> pure () diff --git a/services/brig/src/Brig/User/Auth.hs b/services/brig/src/Brig/User/Auth.hs index c9f232888b..3bf146e8ee 100644 --- a/services/brig/src/Brig/User/Auth.hs +++ b/services/brig/src/Brig/User/Auth.hs @@ -146,6 +146,7 @@ login (PasswordLogin (PasswordLoginData li pw label code)) typ = do Data.authenticate uid pw `catchE` \case AuthInvalidUser -> loginFailed uid AuthInvalidCredentials -> loginFailed uid + AuthStalePassword -> loginFailedWith LoginPasswordUpdateRequired uid AuthSuspended -> throwE LoginSuspended AuthEphemeral -> throwE LoginEphemeral AuthPendingInvitation -> throwE LoginPendingActivation @@ -455,6 +456,7 @@ ssoLogin (SsoLogin uid label) typ = do ReAuthError e -> case e of AuthInvalidUser -> throwE LoginFailed AuthInvalidCredentials -> pure () + AuthStalePassword -> pure () AuthSuspended -> throwE LoginSuspended AuthEphemeral -> throwE LoginEphemeral AuthPendingInvitation -> throwE LoginPendingActivation From 45187d8e68b197db62bb0be9b55d5c2a5938c20d Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Tue, 9 Jan 2024 10:05:08 +0100 Subject: [PATCH 05/15] lint --- services/brig/src/Brig/Data/User.hs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/brig/src/Brig/Data/User.hs b/services/brig/src/Brig/Data/User.hs index 939c9bf1b4..0eff06bc92 100644 --- a/services/brig/src/Brig/Data/User.hs +++ b/services/brig/src/Brig/Data/User.hs @@ -201,9 +201,9 @@ authenticate u pw = case verifyPasswordWithStatus pw pw' of (False, _) -> throwE AuthInvalidCredentials (True, PasswordStatusNeedsUpdate) -> do - case plainTextPassword8 . fromPlainTextPassword $ pw of - Nothing -> pure () -- throwE AuthStalePassword -- TODO(elland): allowed for now - Just pw8 -> updatePassword u pw8 -- update pwd in place + -- TODO(elland): 6char pwd allowed for now + -- throwE AuthStalePassword in the future + for_ (plainTextPassword8 . fromPlainTextPassword $ pw) (updatePassword u) (True, _) -> pure () -- | Password reauthentication. If the account has a password, reauthentication From 637f212431246a0b08b7a79502bafe17b8b54c49 Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Tue, 9 Jan 2024 10:54:14 +0100 Subject: [PATCH 06/15] Reverted envrc --- hack/bin/.envrc | 1 - 1 file changed, 1 deletion(-) delete mode 100644 hack/bin/.envrc diff --git a/hack/bin/.envrc b/hack/bin/.envrc deleted file mode 100644 index f8a82b5abd..0000000000 --- a/hack/bin/.envrc +++ /dev/null @@ -1 +0,0 @@ -use flake github:wireapp/ghc-flakr -Lv From 736dba0e9b7d4c90d3199a3f7113bc82dc490607 Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Tue, 9 Jan 2024 14:24:41 +0100 Subject: [PATCH 07/15] Added changelog --- changelog.d/5-internal/wpb-5490-passive-password-migration | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelog.d/5-internal/wpb-5490-passive-password-migration diff --git a/changelog.d/5-internal/wpb-5490-passive-password-migration b/changelog.d/5-internal/wpb-5490-passive-password-migration new file mode 100644 index 0000000000..ce54896e95 --- /dev/null +++ b/changelog.d/5-internal/wpb-5490-passive-password-migration @@ -0,0 +1,4 @@ +Passively migrate user passwords from scrypt to argon2id. + +By passively we mean that whenever a user re-enters their passwords, if it was hashed using scrypt, it is then rehashed using argon2id and stored as such. +If that user has a legacy short password (under 8 characters in length), it does not migrate to argon2id. From 64b38266a25bb0082c68afb5fa61c786ddbc99ce Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Tue, 9 Jan 2024 14:37:44 +0100 Subject: [PATCH 08/15] hi ci From 078ef7f1a37c0434eb7c4590c9f874216bdcac03 Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Mon, 15 Jan 2024 09:35:17 +0100 Subject: [PATCH 09/15] Clean up todos. --- libs/wire-api/src/Wire/API/Password.hs | 3 +-- services/brig/src/Brig/Data/User.hs | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/libs/wire-api/src/Wire/API/Password.hs b/libs/wire-api/src/Wire/API/Password.hs index d7d935e12c..6efdfd2619 100644 --- a/libs/wire-api/src/Wire/API/Password.hs +++ b/libs/wire-api/src/Wire/API/Password.hs @@ -144,8 +144,7 @@ verifyPasswordWithStatus :: PlainTextPassword' t -> Password -> (Bool, PasswordS verifyPasswordWithStatus plain opaque = let actual = fromPlainTextPassword plain expected = fromPassword opaque - in -- TODO(elland): We'll use the pwd status later to update the hash behind the scenes - checkPassword actual expected + in checkPassword actual expected hashPasswordArgon2id :: MonadIO m => ByteString -> m Text hashPasswordArgon2id pwd = do diff --git a/services/brig/src/Brig/Data/User.hs b/services/brig/src/Brig/Data/User.hs index 0eff06bc92..92a6bacf52 100644 --- a/services/brig/src/Brig/Data/User.hs +++ b/services/brig/src/Brig/Data/User.hs @@ -201,7 +201,7 @@ authenticate u pw = case verifyPasswordWithStatus pw pw' of (False, _) -> throwE AuthInvalidCredentials (True, PasswordStatusNeedsUpdate) -> do - -- TODO(elland): 6char pwd allowed for now + -- FUTUREWORK(elland): 6char pwd allowed for now -- throwE AuthStalePassword in the future for_ (plainTextPassword8 . fromPlainTextPassword $ pw) (updatePassword u) (True, _) -> pure () From 6f466c6ed420d09ef8d04f826918ff5accff1dc3 Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Mon, 15 Jan 2024 15:33:14 +0100 Subject: [PATCH 10/15] review feedback --- libs/wire-api/src/Wire/API/Password.hs | 9 ++++++--- services/brig/src/Brig/API/Error.hs | 1 - services/brig/src/Brig/Data/User.hs | 1 - services/brig/src/Brig/User/Auth.hs | 2 -- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/libs/wire-api/src/Wire/API/Password.hs b/libs/wire-api/src/Wire/API/Password.hs index 6efdfd2619..e81d8b7cc3 100644 --- a/libs/wire-api/src/Wire/API/Password.hs +++ b/libs/wire-api/src/Wire/API/Password.hs @@ -161,7 +161,7 @@ hashPasswordArgon2id pwd = do "$argon2" <> Text.intercalate "$" - [ variantToLetter defaultOptions.variant, + [ variantToCode defaultOptions.variant, "v=" <> versionToNum defaultOptions.version, opts, encodeWithoutPadding salt, @@ -266,6 +266,9 @@ parseScryptPasswordHashParams passwordHash = do hashPasswordWithOptions :: Argon2idOptions -> ByteString -> ByteString -> ByteString hashPasswordWithOptions opts password salt = case (Argon2.hash opts password salt 64) of + -- CryptoFailed occurs when salt, output or input are too small/big. + -- since we control those values ourselves, it should never have a runtime error + -- unless we've caused it ourselves. CryptoFailed cErr -> error $ "Impossible error: " <> show cErr CryptoPassed hash -> hash @@ -282,8 +285,8 @@ hashPasswordWithParams parameters password salt = convert (generate (fromScrypt -------------------------------------------------------------------------------- -- | Makes a letter out of the variant -variantToLetter :: Argon2.Variant -> Text -variantToLetter = \case +variantToCode :: Argon2.Variant -> Text +variantToCode = \case Argon2.Argon2i -> "i" Argon2.Argon2d -> "d" Argon2.Argon2id -> "id" diff --git a/services/brig/src/Brig/API/Error.hs b/services/brig/src/Brig/API/Error.hs index 09110cd970..c043616262 100644 --- a/services/brig/src/Brig/API/Error.hs +++ b/services/brig/src/Brig/API/Error.hs @@ -145,7 +145,6 @@ authError AuthInvalidCredentials = StdError (errorToWai @'E.BadCredentials) authError AuthSuspended = StdError (errorToWai @'E.AccountSuspended) authError AuthEphemeral = StdError (errorToWai @'E.AccountEphemeral) authError AuthPendingInvitation = StdError (errorToWai @'E.AccountPending) -authError AuthStalePassword = StdError (errorToWai @'E.PasswordIsStale) reauthError :: ReAuthError -> Error reauthError ReAuthMissingPassword = StdError (errorToWai @'E.MissingAuth) diff --git a/services/brig/src/Brig/Data/User.hs b/services/brig/src/Brig/Data/User.hs index 92a6bacf52..64638d9af5 100644 --- a/services/brig/src/Brig/Data/User.hs +++ b/services/brig/src/Brig/Data/User.hs @@ -107,7 +107,6 @@ data AuthError | AuthSuspended | AuthEphemeral | AuthPendingInvitation - | AuthStalePassword -- | Re-authentication errors. data ReAuthError diff --git a/services/brig/src/Brig/User/Auth.hs b/services/brig/src/Brig/User/Auth.hs index 3bf146e8ee..c9f232888b 100644 --- a/services/brig/src/Brig/User/Auth.hs +++ b/services/brig/src/Brig/User/Auth.hs @@ -146,7 +146,6 @@ login (PasswordLogin (PasswordLoginData li pw label code)) typ = do Data.authenticate uid pw `catchE` \case AuthInvalidUser -> loginFailed uid AuthInvalidCredentials -> loginFailed uid - AuthStalePassword -> loginFailedWith LoginPasswordUpdateRequired uid AuthSuspended -> throwE LoginSuspended AuthEphemeral -> throwE LoginEphemeral AuthPendingInvitation -> throwE LoginPendingActivation @@ -456,7 +455,6 @@ ssoLogin (SsoLogin uid label) typ = do ReAuthError e -> case e of AuthInvalidUser -> throwE LoginFailed AuthInvalidCredentials -> pure () - AuthStalePassword -> pure () AuthSuspended -> throwE LoginSuspended AuthEphemeral -> throwE LoginEphemeral AuthPendingInvitation -> throwE LoginPendingActivation From 4f8941cfde1c89d1eef49c39fb5be7daf8dacd43 Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Tue, 16 Jan 2024 13:22:14 +0100 Subject: [PATCH 11/15] Added comment to default argon2id opts. --- libs/wire-api/src/Wire/API/Password.hs | 1 + services/brig/src/Brig/Effects/JwtTools.hs | 6 ------ 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/libs/wire-api/src/Wire/API/Password.hs b/libs/wire-api/src/Wire/API/Password.hs index e81d8b7cc3..f9188b6dcf 100644 --- a/libs/wire-api/src/Wire/API/Password.hs +++ b/libs/wire-api/src/Wire/API/Password.hs @@ -100,6 +100,7 @@ defaultParams = outputLength = 64 } +-- | These are the default values suggested, as extracted from the crypton library. defaultOptions :: Argon2idOptions defaultOptions = Argon2.Options diff --git a/services/brig/src/Brig/Effects/JwtTools.hs b/services/brig/src/Brig/Effects/JwtTools.hs index 06b5229de1..f31329c5aa 100644 --- a/services/brig/src/Brig/Effects/JwtTools.hs +++ b/services/brig/src/Brig/Effects/JwtTools.hs @@ -6,10 +6,7 @@ import Brig.API.Types (CertEnrollmentError (..)) import Control.Monad.Trans.Except import Data.ByteString.Conversion import Data.Either.Extra -<<<<<<< HEAD import Data.Handle (Handle, fromHandle) -======= ->>>>>>> b9782cdb1 (Add pwd verification cascading) import Data.Id import Data.Jwt.Tools qualified as Jwt import Data.Misc (HttpsUrl) @@ -17,10 +14,7 @@ import Data.Nonce (Nonce) import Data.PEMKeys import Imports import Network.HTTP.Types (StdMethod (..)) -<<<<<<< HEAD import Network.HTTP.Types qualified as HTTP -======= ->>>>>>> b9782cdb1 (Add pwd verification cascading) import Polysemy import Wire.API.MLS.Credential (ClientIdentity (..)) import Wire.API.MLS.Epoch (Epoch (..)) From fc6fe93beef6fbed52c27fddae515d6e613a53f2 Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Tue, 16 Jan 2024 14:00:23 +0100 Subject: [PATCH 12/15] hi ci From 1a6f4f4db36c087323b7267273230aca6f85ed6e Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Tue, 16 Jan 2024 16:24:18 +0100 Subject: [PATCH 13/15] Added test for password re-hash --- .../test/unit/Test/Wire/API/Password.hs | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/libs/wire-api/test/unit/Test/Wire/API/Password.hs b/libs/wire-api/test/unit/Test/Wire/API/Password.hs index 331dc45959..2104acdad4 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Password.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Password.hs @@ -28,6 +28,7 @@ tests :: TestTree tests = testGroup "Password" $ [ testCase "hash password argon2id" testHashPasswordArgon2id, + testCase "update pwd hash" testUpdateHash, testCase "verify old scrypt password still works" testHashingOldScrypt ] @@ -39,12 +40,30 @@ testHashPasswordArgon2id = do assertBool "Password could not be verified" correct assertBool "Password could not be verified" (status == PasswordStatusOk) +testUpdateHash :: IO () +testUpdateHash = do + let orig = plainTextPassword8Unsafe "Test password scrypt to argon2id." + -- password hashed with scrypt and random salt + expected = unsafeMkPassword "14|8|1|ktYx5i1DMOEfm+tXpw9i7ZVPdeqbxgxYxUbmDVLSAzQ=|Fzy0sNfXQQnJW98ncyN51PUChFWH1tpVJCxjz5JRZEReVa0//zJ6MeopiEh84Ny8lzwdvRPHDqnSS/lkPEB7Ow==" + -- password re-hashed with argon2id and re-used salt for simplicity + newHash = unsafeMkPassword "$argon2id$v=19$m=131072,t=5,p=4$ktYx5i1DMOEfm+tXpw9i7ZVPdeqbxgxYxUbmDVLSAzQ=$iS/9tVk49W8bO/APETqNzMmREerdETTvSXcA7nSpqrsGrV1N33+MVaKnhWhBHqIxM92HFPsV5GP0dpgCUHmJRg==" + -- verify password with scrypt + (correct, status) = verifyPasswordWithStatus orig expected + + assertBool "Password did not match hash." correct + assertBool "Password could not be verified" (status == PasswordStatusNeedsUpdate) + + -- verify again with argon2id + let (correctNew, statusNew) = verifyPasswordWithStatus orig newHash + assertBool "Password hash update failed." correctNew + assertBool "Password could not be verified" (statusNew == PasswordStatusOk) + testHashingOldScrypt :: IO () testHashingOldScrypt = forConcurrently_ pwds $ \pwd -> do let orig = plainTextPassword8Unsafe (fst pwd) expected = unsafeMkPassword (snd pwd) - (correct, status) = (verifyPasswordWithStatus orig expected) + (correct, status) = verifyPasswordWithStatus orig expected assertBool "Password did not match hash." correct assertBool "Password could not be verified" (status == PasswordStatusNeedsUpdate) where From 92deb3531bfcb1de1b7563e0661fa94aa2186785 Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Tue, 16 Jan 2024 16:48:33 +0100 Subject: [PATCH 14/15] Use assertEqual --- libs/wire-api/test/unit/Test/Wire/API/Password.hs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libs/wire-api/test/unit/Test/Wire/API/Password.hs b/libs/wire-api/test/unit/Test/Wire/API/Password.hs index 2104acdad4..43f5e5c772 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Password.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Password.hs @@ -38,7 +38,7 @@ testHashPasswordArgon2id = do hashed <- mkSafePasswordArgon2id pwd let (correct, status) = verifyPasswordWithStatus pwd hashed assertBool "Password could not be verified" correct - assertBool "Password could not be verified" (status == PasswordStatusOk) + assertEqual "Password could not be verified" status PasswordStatusOk testUpdateHash :: IO () testUpdateHash = do @@ -51,12 +51,12 @@ testUpdateHash = do (correct, status) = verifyPasswordWithStatus orig expected assertBool "Password did not match hash." correct - assertBool "Password could not be verified" (status == PasswordStatusNeedsUpdate) + assertEqual "Password could not be verified" status PasswordStatusNeedsUpdate -- verify again with argon2id let (correctNew, statusNew) = verifyPasswordWithStatus orig newHash assertBool "Password hash update failed." correctNew - assertBool "Password could not be verified" (statusNew == PasswordStatusOk) + assertEqual "Password could not be verified" statusNew PasswordStatusOk testHashingOldScrypt :: IO () testHashingOldScrypt = @@ -65,7 +65,7 @@ testHashingOldScrypt = expected = unsafeMkPassword (snd pwd) (correct, status) = verifyPasswordWithStatus orig expected assertBool "Password did not match hash." correct - assertBool "Password could not be verified" (status == PasswordStatusNeedsUpdate) + assertEqual "Password could not be verified" status PasswordStatusNeedsUpdate where -- Password and hashes generated using the old code, but verified using the new one. pwds = From 61b487a3c0400d58dacafe0ece503b144b6b84a6 Mon Sep 17 00:00:00 2001 From: Igor Ranieri Date: Wed, 17 Jan 2024 09:22:19 +0100 Subject: [PATCH 15/15] hi ci