[Brig] Move password verification to the AuthenticationSubsystem, move to Argon2id with default settings. #4271

elland · 2024-09-26T07:26:19Z

https://wearezeta.atlassian.net/browse/WPB-9746

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

mdimjasevic

Here is a partial review

mdimjasevic · 2024-10-03T09:02:10Z

libs/wire-api/src/Wire/API/Password.hs

-  salt <- newSalt 32
+  salt <- newSalt 16


I don't have the context, but why this change?

mdimjasevic · 2024-10-03T09:03:55Z

libs/wire-api/src/Wire/API/Password.hs

+        -- CryptoFailed occurs when salt, output or input are too small/big.
+        -- since we control those values ourselves, it should never have a runtime error
+        CryptoFailed cErr -> error $ "Impossible error: " <> show cErr


So still something to change given the comment?

mdimjasevic · 2024-10-03T09:04:58Z

libs/wire-api/src/Wire/API/Password.hs

+
+-------------------------------------------------------------------------------
+-- Generate test passwords, benchmark
+
+genTestPasswords :: IO [(Text, Text)]
+genTestPasswords = replicateM 100 do
+  pwd <- genPassword
+  hash <- mkSafePassword pwd
+  pure (fromPlainTextPassword pwd, fromPassword hash)


This sounds like it belongs to a test module instead.

Requires access to private functions, unsafe to deconstruct the opaque password type. I should document it.

mdimjasevic · 2024-10-03T09:07:03Z

libs/wire-subsystems/src/Wire/AuthenticationSubsystem.hs

 import Wire.UserKeyStore

 data AuthenticationSubsystem m a where
-  VerifyPassword :: Local UserId -> PlainTextPassword6 -> AuthenticationSubsystem m ()
+  VerifyPasswordE :: Local UserId -> PlainTextPassword6 -> AuthenticationSubsystem m ()


What is "E" in the name?

mdimjasevic · 2024-10-03T09:26:16Z

libs/wire-subsystems/src/Wire/AuthenticationSubsystem.hs

+  VerifyPassword :: PlainTextPassword6 -> Password -> AuthenticationSubsystem m (Bool, PasswordStatus)
+  VerifyUserPassword :: UserId -> PlainTextPassword6 -> AuthenticationSubsystem r (Bool, PasswordStatus)
+  VerifyProviderPassword :: ProviderId -> PlainTextPassword6 -> AuthenticationSubsystem r (Bool, PasswordStatus)


How is the 6-character password topic related to the Scrypt vs. Argon2id topic? Does the choice of algorithm apply the minimal length?

mdimjasevic · 2024-10-03T09:56:08Z

libs/wire-subsystems/src/Wire/AuthenticationSubsystem/Interpreter.hs

+    PasswordStore.lookupHashedProviderPassword pid
+      >>= maybe (throw AuthenticationSubsystemBadCredentials) pure


Suggested change

PasswordStore.lookupHashedProviderPassword pid

>>= maybe (throw AuthenticationSubsystemBadCredentials) pure

PasswordStore.lookupHashedProviderPassword pid >>= noteS @'AuthenticationSubsystemBadCredentials

mdimjasevic · 2024-10-03T09:56:38Z

libs/wire-subsystems/src/Wire/AuthenticationSubsystem/Interpreter.hs

+    PasswordStore.lookupHashedPassword uid
+      >>= maybe (throw AuthenticationSubsystemBadCredentials) pure


mdimjasevic · 2024-10-03T09:58:48Z

libs/wire-subsystems/src/Wire/PasswordStore/Cassandra.hs

@@ -16,6 +16,12 @@ interpretPasswordStore casClient =
    runEmbedded (runClient casClient) . \case
      UpsertHashedPassword uid password -> embed $ updatePasswordImpl uid password
      LookupHashedPassword uid -> embed $ lookupPasswordImpl uid
+      LookupHashedProviderPassword pid -> embed $ lookupProviderPasswordImpl pid


Why is there a need to treat provider password differently, i.e., why do we have this action?

Yes, different table.

mdimjasevic · 2024-10-03T10:01:09Z

services/brig/brig.integration.yaml

-    suspendTimeout: 4
+    suspendTimeout: 10


Why this change?

mdimjasevic · 2024-10-03T10:02:52Z

services/brig/src/Brig/API/OAuth.hs

@@ -100,6 +106,7 @@ registerOAuthClient (OAuthClientConfig name uri) = do
    createSecret :: (MonadIO m) => m OAuthClientPlainTextSecret
    createSecret = OAuthClientPlainTextSecret <$> rand32Bytes

+    -- TODO(elland): figure out why


Something still to do.

mdimjasevic · 2024-10-03T10:05:23Z

Have you searched for all usages of mkSafePasswordScrypt and checked that's as expected?

mdimjasevic

Some more comments inlined.

Given that this is changing the default hashing algorithm, and we forgot to do it before in some places, what would be good tests to add to capture this change?

mdimjasevic · 2024-10-03T11:05:19Z

services/brig/src/Brig/Data/User.hs

 authenticate u pw =
+  -- TODO: Move this logic into auth subsystem.


Still something to do.

mdimjasevic · 2024-10-03T11:08:28Z

services/brig/src/Brig/Data/User.hs

@@ -225,10 +231,10 @@ reauthenticate u pw =
  where
    maybeReAuth pw' = case pw of
      Nothing -> do
-        musr <- lookupUser NoPendingInvitations u
+        musr <- wrapClientE $ lookupUser NoPendingInvitations u


Don't we have an equivalent in the UserStore effect?

mdimjasevic · 2024-10-03T11:10:55Z

services/brig/src/Brig/Provider/API.hs

@@ -265,20 +273,31 @@ activateAccountKey key val = do
      lift $ sendApprovalConfirmMail name email
      pure . Just $ Public.ProviderActivationResponse email

-getActivationCodeH :: (Member GalleyAPIAccess r, Member VerificationCodeSubsystem r) => EmailAddress -> (Handler r) Code.KeyValuePair
+getActivationCodeH ::


We could drop this wai-routes convention of ending handler names with "H" now that you've already changed the line.

echoes-hq bot added the echoes: security Security related changes or defects label Sep 26, 2024

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 26, 2024

fisx changed the title ~~Move password verification to the AuthenticationSubsystem, move to Argon2id with default settings.~~ [WPB-9746] Move password verification to the AuthenticationSubsystem, move to Argon2id with default settings. Sep 26, 2024

elland force-pushed the wpb-9746/password branch from cdb06a4 to 8189763 Compare September 26, 2024 12:02

elland changed the title ~~[WPB-9746] Move password verification to the AuthenticationSubsystem, move to Argon2id with default settings.~~ [Brig] Move password verification to the AuthenticationSubsystem, move to Argon2id with default settings. Sep 26, 2024

elland force-pushed the wpb-9746/password branch 2 times, most recently from f6c408d to 1bdae8c Compare September 26, 2024 13:31

elland added 7 commits October 2, 2024 08:31

Use Argon2id instead of scrypt, with default params.

96bca78

Added verifyPassword to subsystem

9bdf76b

Improved handling of pwds between bots and users.

9af31f4

[brig] Use auth subsystem to verify pwds.

212207c

Adapt argon2id params.

4b9f248

Adjusted params again, updated tests.

636f898

Fixed test.

b8a0ef3

elland force-pushed the wpb-9746/password branch from 586b48e to b8a0ef3 Compare October 2, 2024 06:41

Added changelog.

7aa6812

elland force-pushed the wpb-9746/password branch from 40496f4 to 7aa6812 Compare October 2, 2024 09:30

Fixed bug with provider pwd.

e4cb90f

elland force-pushed the wpb-9746/password branch from d87fbc1 to e4cb90f Compare October 2, 2024 12:45

elland added 7 commits October 2, 2024 14:58

Increase tolerance for local user suspension in integration tests.

1981bfa

Use Scrypt for OAuth.

db815ec

[wip] Use scrypt in select places.

8641ba5

Clean up pragma.

05f403c

Extract rabbit queue into own make cmd.

069f289

Updating provider pwd to argon.

30702ba

Restored argon for provider new acc.

c96dda8

mdimjasevic reviewed Oct 3, 2024

View reviewed changes

Test using only Scrypt.

5ced70d

elland force-pushed the wpb-9746/password branch from da03847 to 5ced70d Compare October 3, 2024 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Brig] Move password verification to the AuthenticationSubsystem, move to Argon2id with default settings. #4271

[Brig] Move password verification to the AuthenticationSubsystem, move to Argon2id with default settings. #4271

elland commented Sep 26, 2024 •

edited

Loading

mdimjasevic left a comment

mdimjasevic Oct 3, 2024

mdimjasevic Oct 3, 2024

mdimjasevic Oct 3, 2024

elland Oct 3, 2024

mdimjasevic Oct 3, 2024

mdimjasevic Oct 3, 2024

mdimjasevic Oct 3, 2024

mdimjasevic Oct 3, 2024

mdimjasevic Oct 3, 2024

elland Oct 3, 2024

mdimjasevic Oct 3, 2024

mdimjasevic Oct 3, 2024

mdimjasevic commented Oct 3, 2024

mdimjasevic left a comment

mdimjasevic Oct 3, 2024

mdimjasevic Oct 3, 2024

mdimjasevic Oct 3, 2024

		PasswordStore.lookupHashedProviderPassword pid
		>>= maybe (throw AuthenticationSubsystemBadCredentials) pure

		PasswordStore.lookupHashedPassword uid
		>>= maybe (throw AuthenticationSubsystemBadCredentials) pure

		authenticate u pw =
		-- TODO: Move this logic into auth subsystem.

[Brig] Move password verification to the AuthenticationSubsystem, move to Argon2id with default settings. #4271

Are you sure you want to change the base?

[Brig] Move password verification to the AuthenticationSubsystem, move to Argon2id with default settings. #4271

Conversation

elland commented Sep 26, 2024 • edited Loading

Checklist

mdimjasevic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdimjasevic commented Oct 3, 2024

mdimjasevic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elland commented Sep 26, 2024 •

edited

Loading