Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat] Allow configuring argon2id parameters #4291

Open
wants to merge 42 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
1c80e2b
Removed wrong comment.
elland Oct 9, 2024
815481e
Thread password hashing options through the env.
elland Oct 10, 2024
6cd9fb4
Renamed make cmd to avoid accidentally cleaning the whole thing.
elland Oct 10, 2024
1419f59
Cleaning up.
elland Oct 10, 2024
50de042
Clean up from review
elland Oct 10, 2024
0277ca6
Update libs/wire-subsystems/test/unit/Wire/MockInterpreters/HashPassw…
elland Oct 10, 2024
c100cb6
Restore necessary reauth logic
elland Oct 10, 2024
448d81a
Added a changelog.
elland Oct 10, 2024
bd0a1a7
Add docs, beef up changelog entrie(s).
fisx Oct 11, 2024
bfe5384
Fix: default parameters.
fisx Oct 11, 2024
80c2964
Merge remote-tracking branch 'origin/develop' into configurable-argon
fisx Oct 11, 2024
44b726b
Fix: do not take a Nothing for options when hashing passwords!
fisx Oct 11, 2024
42a0dcb
cp info from changelog to docs.
fisx Oct 11, 2024
1f4d18e
Fixed typo.
elland Oct 14, 2024
5ab90fe
Update changelog.d/2-features/add-config-for-pwd-hash
fisx Oct 14, 2024
4083a34
Fix: remove one more spurious call to scrypt.
fisx Oct 14, 2024
c9aab24
wip: Test new params for CI
elland Oct 14, 2024
85a9e03
Change the right place
elland Oct 14, 2024
7154034
Merge remote-tracking branch 'origin/develop' into configurable-argon
elland Oct 14, 2024
d678d9b
Fix/Add more tests for ssoLogin.
elland Oct 14, 2024
9679e00
Make argon2id also fast in local integration tests.
fisx Oct 14, 2024
54d2912
mv argon2id dummy settings from prod to CI.
fisx Oct 14, 2024
62a352b
Restore Missing Auth error.
elland Oct 15, 2024
09ca3d2
Update changelog.d/2-features/add-config-for-pwd-hash
fisx Oct 15, 2024
b04b685
Update docs/src/developer/reference/config-options.md
fisx Oct 15, 2024
3846046
Fixed new test expectation.
elland Oct 15, 2024
73da097
Added logging for the hash opts.
elland Oct 15, 2024
ba549eb
Log also for reg user creation.
elland Oct 15, 2024
dd891b3
More aggressive logging.
elland Oct 15, 2024
ab4fea8
charts/brig: Allow configuring password hashing options
elland Oct 16, 2024
16723dc
hack/helm_vars: Optimize password hashing options so tests run fast
elland Oct 16, 2024
d2ccad4
charts/brig: Set defaults for PasswordHashingOptions
elland Oct 16, 2024
cff50f7
wire-{api,subsystems}: Simplify password hashing options
elland Oct 16, 2024
e0fa88d
brig: Adapt for simplified password hashing options
elland Oct 16, 2024
a3d5bfe
galley: Allow configuring password hashing options
elland Oct 16, 2024
ec11876
hack/helm_var: Configure password hashing options for galley
elland Oct 16, 2024
5ff3b18
brig: Removed debug logging
elland Oct 16, 2024
50c6685
Lint out redundant language pragma.
elland Oct 16, 2024
721e2ff
Update changelog and docs for galley and brig
elland Oct 16, 2024
dc7f299
Lint+
elland Oct 16, 2024
e256d56
galley: Local integration config missing.
elland Oct 17, 2024
d73113c
wip: Fix authentication not handling errors correctly for decrementing
elland Oct 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ install: init
./hack/bin/cabal-run-all-tests.sh
./hack/bin/cabal-install-artefacts.sh all

.PHONY: clean-rabbit
clean-rabbit:
.PHONY: rabbit-clean
rabbit-clean:
rabbitmqadmin -f pretty_json list queues vhost name messages | jq -r '.[] | "rabbitmqadmin delete queue name=\(.name) --vhost=\(.vhost)"' | bash

# Clean
Expand All @@ -59,7 +59,7 @@ full-clean: clean
rm -rf ~/.cache/hie-bios
rm -rf ./dist-newstyle ./.env
direnv reload
clean-rabbit
rabbit-clean
@echo -e "\n\n*** NOTE: you may want to also 'rm -rf ~/.cabal/store \$$CABAL_DIR/store', not sure.\n"

.PHONY: clean
Expand Down
18 changes: 18 additions & 0 deletions changelog.d/0-release-notes/configurable-argon
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
Password hashing is now done using argon2id instead of scrypt. The argon2id parameters can be configured using these options:

```yaml
brig:
optSettings:
setPasswordHashingOptions:
iterations: ...
memory: ... # memory needed in KiB
parallelism: ...
galley:
settings:
passwordHashingOptions:
iterations: ...
memory: ... # memory needed in KiB
parallelism: ...
```

These have default values, which should work for most deployments. Please see documentation on config-options for more.
1 change: 1 addition & 0 deletions changelog.d/2-features/add-config-for-pwd-hash
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow configuring Argon2id parameters
1 change: 1 addition & 0 deletions charts/brig/templates/configmap.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -368,5 +368,6 @@ data:
{{- if .setOAuthMaxActiveRefreshTokens }}
setOAuthMaxActiveRefreshTokens: {{ .setOAuthMaxActiveRefreshTokens }}
{{- end }}
setPasswordHashingOptions: {{ toYaml .setPasswordHashingOptions | nindent 8 }}
{{- end }}
{{- end }}
7 changes: 6 additions & 1 deletion charts/brig/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ metrics:
enabled: false
# This is not supported for production use, only here for testing:
# preStop:
# exec:
# exec:
# command: ["sh", "-c", "curl http://acme.example"]
config:
logLevel: Info
Expand Down Expand Up @@ -150,6 +150,11 @@ config:
setDisabledAPIVersions: [ development ]
setFederationStrategy: allowNone
setFederationDomainConfigsUpdateFreq: 10
# Options for Argon2id version 19
setPasswordHashingOptions:
iterations: 1
parallelism: 32
memory: 180224 # 176 MiB
smtp:
passwordFile: /etc/wire/brig/secrets/smtp-password.txt
proxy: {}
Expand Down
1 change: 1 addition & 0 deletions charts/galley/templates/configmap.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ data:
{{- if .settings.guestLinkTTLSeconds }}
guestLinkTTLSeconds: {{ .settings.guestLinkTTLSeconds }}
{{- end }}
passwordHashingOptions: {{ toYaml .settings.passwordHashingOptions | nindent 8 }}
featureFlags:
sso: {{ .settings.featureFlags.sso }}
legalhold: {{ .settings.featureFlags.legalhold }}
Expand Down
5 changes: 5 additions & 0 deletions charts/galley/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ config:
# The lifetime of a conversation guest link in seconds. Must be a value 0 < x <= 31536000 (365 days)
# Default is 31536000 (365 days) if not set
guestLinkTTLSeconds: 31536000
# Options for Argon2id version 19
passwordHashingOptions:
iterations: 1
parallelism: 32
memory: 180224 # 176 MiB
featureFlags: # see #RefConfigOptions in `/docs/reference` (https://github.com/wireapp/wire-server/)
appLock:
defaults:
Expand Down
47 changes: 47 additions & 0 deletions docs/src/developer/reference/config-options.md
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,53 @@ optSettings:
setOAuthMaxActiveRefreshTokens: 10
```

#### Argon2id password hashing parameters

Since release 5.6.0, wire-server hashes passwords with
[argon2id](https://datatracker.ietf.org/doc/html/rfc9106) at rest. If
you do not do anything, the default parameters will be used, which
are:

```yaml
setPasswordHashingOptions:
iterations: 1
memory: 180224 # memory needed in kibibytes (1 kibibyte is 2^10 bytes)
parallelism: 32
```

The default will be adjusted to new developments in hashing algorithm
security from time to time.

The password hashing options are set for brig and galley:

```yaml
brig:
optSettings:
setPasswordHashingOptions:
iterations: ...
memory: ... # memory needed in KiB
parallelism: ...
galley:
settings:
passwordHashingOptions:
iterations: ...
memory: ... # memory needed in KiB
parallelism: ...
```

**Performance implications:** scrypt takes ~80ms on a realistic test
system, and argon2id with default settings takes ~500ms. This is a
runtime increase by a factor of ~6. This happens every time a
password is entered by the user: during login, password reset,
deleting a device, etc. (It does **NOT** happen during any other
cryptographic operations like session key update or message
de-/encryption.)

The settings are a trade-off between resilience against brute force
attacks and password secrecy. For most systems this should be safe
and not need more hardware resources for brig, but you may want to
form your own opinion.

#### Disabling API versions

It is possible to disable one ore more API versions. When an API version is disabled it won't be advertised on the `GET /api-version` endpoint, neither in the `supported`, nor in the `development` section. Requests made to any endpoint of a disabled API version will result in the same error response as a request made to an API version that does not exist.
Expand Down
13 changes: 13 additions & 0 deletions hack/helm_vars/wire-server/values.yaml.gotmpl
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,12 @@ brig:
setOAuthEnabled: true
setOAuthRefreshTokenExpirationTimeSecs: 14515200 # 24 weeks
setOAuthMaxActiveRefreshTokens: 10
# These values are insecure, against anyone getting hold of the hash,
# but its not a concern for the integration tests.
setPasswordHashingOptions:
iterations: 1
parallelism: 4
memory: 32 # This needs to be at least 8 * parallelism.
aws:
sesEndpoint: http://fake-aws-ses:4569
sqsEndpoint: http://fake-aws-sqs:4568
Expand Down Expand Up @@ -258,6 +264,13 @@ galley:
federationDomain: integration.example.com
disabledAPIVersions: []

# These values are insecure, against anyone getting hold of the hash,
# but its not a concern for the integration tests.
passwordHashingOptions:
iterations: 1
parallelism: 4
memory: 32 # This needs to be at least 8 * parallelism.

featureFlags:
sso: disabled-by-default # this needs to be the default; tests can enable it when needed.
legalhold: whitelist-teams-and-implicit-consent
Expand Down
19 changes: 19 additions & 0 deletions libs/types-common/src/Util/Options.hs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE TemplateHaskell #-}

-- This file is part of the Wire Server implementation.
Expand Down Expand Up @@ -146,3 +147,21 @@ getOptions desc mp defaultPath = do

parseAWSEndpoint :: ReadM AWSEndpoint
parseAWSEndpoint = readerAsk >>= maybe (error "Could not parse AWS endpoint") pure . fromByteString . fromString

data PasswordHashingOptions = PasswordHashingOptions
{ iterations :: !Word32,
memory :: !Word32,
parallelism :: !Word32
}
deriving (Show, Generic)

instance FromJSON PasswordHashingOptions where
parseJSON =
withObject
"PasswordHashingOptions"
( \obj -> do
iterations <- obj .: "iterations"
memory <- obj .: "memory"
parallelism <- obj .: "parallelism"
pure (PasswordHashingOptions {..})
)
3 changes: 0 additions & 3 deletions libs/wire-api/src/Wire/API/Error/Brig.hs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ data BrigError
| LastIdentity
| NoPassword
| ChangePasswordMustDiffer
| PasswordAuthenticationFailed
| TooManyTeamInvitations
| CannotJoinMultipleTeams
| InsufficientTeamPermissions
Expand Down Expand Up @@ -254,8 +253,6 @@ type instance MapError 'NoPassword = 'StaticError 403 "no-password" "The user ha

type instance MapError 'ChangePasswordMustDiffer = 'StaticError 409 "password-must-differ" "For password change, new and old password must be different."

type instance MapError 'PasswordAuthenticationFailed = 'StaticError 403 "password-authentication-failed" "Password authentication failed."
fisx marked this conversation as resolved.
Show resolved Hide resolved

type instance MapError 'TooManyTeamInvitations = 'StaticError 403 "too-many-team-invitations" "Too many team invitations for this team"

type instance MapError 'CannotJoinMultipleTeams = 'StaticError 403 "cannot-join-multiple-teams" "Cannot accept invitations from multiple teams"
Expand Down
43 changes: 19 additions & 24 deletions libs/wire-api/src/Wire/API/Password.hs
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ module Wire.API.Password
verifyPassword,
verifyPasswordWithStatus,
PasswordReqBody (..),
argon2OptsFromHashingOpts,

-- * Only for testing
hashPasswordArgon2idWithSalt,
hashPasswordArgon2idWithOptions,
mkSafePasswordScrypt,
parsePassword,
)
Expand All @@ -52,6 +52,7 @@ import Data.Text qualified as Text
import Data.Text.Encoding qualified as Text
import Imports
import OpenSSL.Random (randBytes)
import Util.Options

-- | A derived, stretched password that can be safely stored.
data Password
Expand Down Expand Up @@ -120,19 +121,6 @@ defaultScryptParams =
outputLength = 64
}

-- | Recommended in the RFC as the second choice: https://www.rfc-editor.org/rfc/rfc9106.html#name-parameter-choice
-- The first choice takes ~1s to hash passwords which seems like too much.
defaultOptions :: Argon2.Options
defaultOptions =
Argon2.Options
{ iterations = 1,
-- TODO: fix this after meeting with Security
memory = 2 ^ (17 :: Int),
parallelism = 32,
variant = Argon2.Argon2id,
version = Argon2.Version13
}

fromScrypt :: ScryptParameters -> Parameters
fromScrypt scryptParams =
Parameters
Expand All @@ -142,6 +130,16 @@ fromScrypt scryptParams =
outputLength = 64
}

argon2OptsFromHashingOpts :: PasswordHashingOptions -> Argon2.Options
argon2OptsFromHashingOpts PasswordHashingOptions {..} =
Argon2.Options
{ variant = Argon2.Argon2id,
version = Argon2.Version13,
iterations = iterations,
memory = memory,
parallelism = parallelism
}

-------------------------------------------------------------------------------

-- | Generate a strong, random plaintext password of length 16
Expand All @@ -154,8 +152,8 @@ genPassword =
mkSafePasswordScrypt :: (MonadIO m) => PlainTextPassword' t -> m Password
mkSafePasswordScrypt = fmap ScryptPassword . hashPasswordScrypt . Text.encodeUtf8 . fromPlainTextPassword

mkSafePassword :: (MonadIO m) => PlainTextPassword' t -> m Password
mkSafePassword = fmap Argon2Password . hashPasswordArgon2id . Text.encodeUtf8 . fromPlainTextPassword
mkSafePassword :: (MonadIO m) => Argon2.Options -> PlainTextPassword' t -> m Password
mkSafePassword opts = fmap Argon2Password . hashPasswordArgon2id opts . Text.encodeUtf8 . fromPlainTextPassword

-- | Verify a plaintext password from user input against a stretched
-- password from persistent storage.
Expand Down Expand Up @@ -190,16 +188,13 @@ encodeScryptPassword ScryptHashedPassword {..} =
Text.decodeUtf8 . B64.encode $ hashedKey
]

hashPasswordArgon2id :: (MonadIO m) => ByteString -> m Argon2HashedPassword
hashPasswordArgon2id pwd = do
hashPasswordArgon2id :: (MonadIO m) => Argon2.Options -> ByteString -> m Argon2HashedPassword
hashPasswordArgon2id opts pwd = do
salt <- newSalt 16
pure $! hashPasswordArgon2idWithSalt salt pwd

hashPasswordArgon2idWithSalt :: ByteString -> ByteString -> Argon2HashedPassword
hashPasswordArgon2idWithSalt = hashPasswordArgon2idWithOptions defaultOptions
pure $! hashPasswordArgon2idWithSalt opts salt pwd

hashPasswordArgon2idWithOptions :: Argon2.Options -> ByteString -> ByteString -> Argon2HashedPassword
hashPasswordArgon2idWithOptions opts salt pwd = do
hashPasswordArgon2idWithSalt :: Argon2.Options -> ByteString -> ByteString -> Argon2HashedPassword
hashPasswordArgon2idWithSalt opts salt pwd = do
let hashedKey = hashPasswordWithOptions opts pwd salt
in Argon2HashedPassword {..}

Expand Down
1 change: 0 additions & 1 deletion libs/wire-api/src/Wire/API/Routes/Public/Spar.hs
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,6 @@ sparResponseURI (Just tid) =
type APIScim =
OmitDocs :> "v2" :> ScimSiteAPI SparTag
:<|> "auth-tokens"
:> CanThrow 'PasswordAuthenticationFailed
:> CanThrow 'CodeAuthenticationFailed
:> CanThrow 'CodeAuthenticationRequired
:> APIScimToken
Expand Down
15 changes: 11 additions & 4 deletions libs/wire-api/src/Wire/API/User.hs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ module Wire.API.User
SelfProfile (..),
-- User (should not be here)
User (..),
isSamlUser,
userId,
userDeleted,
userEmail,
Expand Down Expand Up @@ -584,6 +585,12 @@ data User = User
deriving (Arbitrary) via (GenericUniform User)
deriving (ToJSON, FromJSON, S.ToSchema) via (Schema User)

isSamlUser :: User -> Bool
isSamlUser usr = do
case usr.userIdentity of
Just (SSOIdentity (UserSSOId _) _) -> True
_ -> False

userId :: User -> UserId
userId = qUnqualified . userQualifiedId

Expand Down Expand Up @@ -1418,8 +1425,8 @@ instance (res ~ PutSelfResponses) => AsUnion res (Maybe UpdateProfileError) wher

-- | The payload for setting or changing a password.
data PasswordChange = PasswordChange
{ cpOldPassword :: Maybe PlainTextPassword6,
cpNewPassword :: PlainTextPassword8
{ oldPassword :: Maybe PlainTextPassword6,
newPassword :: PlainTextPassword8
}
deriving stock (Eq, Show, Generic)
deriving (Arbitrary) via (GenericUniform PasswordChange)
Expand All @@ -1435,9 +1442,9 @@ instance ToSchema PasswordChange where
)
. object "PasswordChange"
$ PasswordChange
<$> cpOldPassword
<$> oldPassword
.= maybe_ (optField "old_password" schema)
<*> cpNewPassword
<*> newPassword
.= field "new_password" schema

data ChangePasswordError
Expand Down
Loading
Loading