[SQSERVICES-1931] wire server allow backend to enforce stronger password restrictions when setting a password by battermann · Pull Request #3137 · wireapp/wire-server

battermann · 2023-03-08T16:39:27Z

https://wearezeta.atlassian.net/browse/SQSERVICES-1931

Changed PlainTextPassword type so that it contains a text range according to the previous constraint with a min of 6 characters
Introduced a new type PlainTextPasswordMinLength8 with a text range with a min of 8 characters
Used the new type with min length 8 in the following places:
- NewProviderResponse
- NewUser
- NewRawUser
- PasswordChange
- CompletePasswordReset
Updated golden tests
Updated integration tests
Added integration tests which verifies that passwords with less than 8 chars are rejected with a 400 status code for the following scenarios:
- register user
- set/change password
- password reset
Introduced convenience functions plainTextPasswordUnsafe and plainTextPasswordLegacyUnsafe to use where we need to construct a password from a string (in the prod code, these should be used with caution)
toLegacy exists to convert from the new password type to the legacy type, which should always succeed because the constraints of the new type are stronger

It's currently not tested that login with an existing password with a length of less 8 characters still works. But according to how this is implemented, there is no reason it should not. Unfortunately, this would be very difficult to test, as it is not possible to create test users with shorter passwords anymore.

Checklist

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

battermann · 2023-03-09T15:06:28Z

I am open to other naming suggestions, e.g. call the old/legacy password type PlainTextPasswordLegacy and the new one just PlainTextPassword?

…-to-enforce-stronger-password-restrictions-when-setting-a-password

fisx

running gundeck integration tests locally i get this:

    Tokens
      register a push token:                                          FAIL
        Exception: Safe.readNote no parse on "NO_HEADER_VALUE", unable to parse Location header
        CallStack (from HasCallStack):
          readNote, called at test/integration/API.hs:1253:7 in main:API
        Use -p '!/RealAWS/&&/Tokens.register a push token/' to rerun this test only.
      unregister a push token:                                        FAIL
        Exception: Safe.readNote no parse on "NO_HEADER_VALUE", unable to parse Location header
        CallStack (from HasCallStack):
          readNote, called at test/integration/API.hs:1253:7 in main:API
        Use -p '!/RealAWS/&&/unregister a push token/' to rerun this test only.
    Websocket pingpong
      data-level pings produce pongs:                                 OK (1.00s)
      control pings with payload produce pongs with the same payload: OK (1.00s)
      data non-pings are ignored:                                     OK (2.00s)
    Redis migration
      redis migration should work:                                    FAIL
        Exception: Error executing request: Safe.readNote no parse on "NO_HEADER_VALUE", unable to parse Location header
        CallStack (from HasCallStack):
          readNote, called at test/integration/API.hs:1253:7 in main:API
        CallStack (from HasCallStack):
          error, called at src/Bilge/Assert.hs:101:18 in bilge-0.22.0-inplace:Bilge.Assert
          <!!, called at src/Bilge/Assert.hs:109:19 in bilge-0.22.0-inplace:Bilge.Assert
          !!!, called at test/integration/API.hs:966:7 in main:API
        Use -p '!/RealAWS/&&/redis migration should work/' to rerun this test only.

it doesn't look related, but it's only happening on this branch, not on develop.

[...] it is not possible to create test users with shorter passwords anymore.

you could either call cassandra directly and inject the short password, or implement an /i/-endpoint that allows this. the former used to happen in the spar tests, but somebody removed it, so maybe we should aim for the second option.

let's pair on this on monday?

fisx · 2023-03-10T14:11:07Z

libs/types-common/src/Data/Misc.hs

+type PlainTextPassword = PlainTextPassword' (6 :: Nat)
+
+type PlainTextPasswordMinLength8 = PlainTextPassword' (8 :: Nat)


Suggested change

type PlainTextPassword = PlainTextPassword' (6 :: Nat)

type PlainTextPasswordMinLength8 = PlainTextPassword' (8 :: Nat)

type PlainTextPassword6 = PlainTextPassword' (6 :: Nat)

type PlainTextPassword = PlainTextPassword' (8 :: Nat)

The old passwords should have the more awkward name, the passwords we really want to deal with everywhere the simpler name.

fisx · 2023-03-10T14:12:48Z

libs/types-common/src/Data/Misc.hs

+plainTextPasswordLegacy :: Text -> Maybe PlainTextPassword
+plainTextPasswordLegacy = fmap PlainTextPassword' . checked
+
+plainTextPasswordLegacyUnsafe :: Text -> PlainTextPassword
+plainTextPasswordLegacyUnsafe = PlainTextPassword' . unsafeRange
+
+plainTextPassword :: Text -> Maybe PlainTextPasswordMinLength8
+plainTextPassword = fmap PlainTextPassword' . checked
+
+plainTextPasswordUnsafe :: Text -> PlainTextPasswordMinLength8
+plainTextPasswordUnsafe = PlainTextPassword' . unsafeRange
+
+fromPlainTextPassword :: PlainTextPassword' t -> Text
+fromPlainTextPassword = fromRange . fromPlainTextPassword'
+
+-- | Convert a 'PlainTextPasswordMinLength8' to a legacy 'PlainTextPassword'.
+toLegacy :: PlainTextPasswordMinLength8 -> PlainTextPassword
+toLegacy = PlainTextPassword' . unsafeRange . fromPlainTextPassword


It's fine to use PlainTextPasswordLegacy instead of PlainTextPassword6, but please be consistent and name the constructors after the types they are constructing.

fisx · 2023-03-10T14:16:07Z

libs/types-common/src/Data/Misc.hs


-instance Arbitrary PlainTextPassword where
+instance Arbitrary (PlainTextPassword' (8 :: Nat)) where
  -- TODO: why 6..1024? For tests we might want invalid passwords as well, e.g. 3 chars


This TODO is obsolete: we don't have to test for errors that are caught by the type checker.

fisx · 2023-03-13T10:04:43Z

before b2adfe0:

$ make ci package=gundeck TASTY_PATTERN='!/RealAWS/&&/redis migration should work/'
[...]
Gundeck
  API tests
    Redis migration
      redis migration should work: 2023-03-13T10:01:10Z, D, Connecting to 127.0.0.1:9042
FAIL
        Exception: Error executing request: Safe.readNote no parse on "NO_HEADER_VALUE", unable to parse Location header
        CallStack (from HasCallStack):
          readNote, called at test/integration/API.hs:1253:7 in main:API
        CallStack (from HasCallStack):
          error, called at src/Bilge/Assert.hs:101:18 in bilge-0.22.0-inplace:Bilge.Assert
          <!!, called at src/Bilge/Assert.hs:109:19 in bilge-0.22.0-inplace:Bilge.Assert
          !!!, called at test/integration/API.hs:966:7 in main:API

1 out of 1 tests failed (0.07s)
[...]

after b2adfe0:

$ make ci package=gundeck TASTY_PATTERN='!/RealAWS/&&/redis migration should work/'
[...]
Gundeck
  API tests
    Redis migration
      redis migration should work: 2023-03-13T10:02:01Z, D, Connecting to 127.0.0.1:9042
OK (0.33s)

All 1 tests passed (0.35s)

I'd really like to know how this didn't trigger on CI or on your machine @battermann...

fisx · 2023-03-13T10:12:04Z

ah, maybe something to do with RealAWS?

find services/ libs/ tools/ -name '*.hs' -exec perl -i -pe 's/\bPlainTextPassword\b([^'\''])/PlainTextPassword6\1/g' {} \;

find services/ libs/ tools/ -name '*.hs' -exec perl -i -pe 's/\bPlainTextPasswordMinLength8\b/PlainTextPassword8/g' {} \;

find services/ libs/ tools/ -name '*.hs' -exec perl -i -pe 's/\bplainTextPasswordLegacy\b/plainTextPassword6/g' {} \; find services/ libs/ tools/ -name '*.hs' -exec perl -i -pe 's/\bplainTextPasswordLegacyUnsafe\b/plainTextPassword6Unsafe/g' {} \; find services/ libs/ tools/ -name '*.hs' -exec perl -i -pe 's/\bplainTextPassword\b/plainTextPassword8/g' {} \; find services/ libs/ tools/ -name '*.hs' -exec perl -i -pe 's/\bplainTextPasswordUnsafe\b/plainTextPassword8Unsafe/g' {} \; find services/ libs/ tools/ -name '*.hs' -exec perl -i -pe 's/\btoLegacy\b/plainTextPassword8To6/g' {} \;

fisx

LGTM now! :)

please take a look at the changes (i think commit-by-commit is easiest, and you can ignore all the automatic renaming noise), and merge if you agree with everything.

battermann · 2023-03-13T14:34:23Z

[...] it is not possible to create test users with shorter passwords anymore.

you could either call cassandra directly and inject the short password, or implement an /i/-endpoint that allows this. the former used to happen in the spar tests, but somebody removed it, so maybe we should aim for the second option.

let's pair on this on monday?

I added this test.

* Downgrade to our fork of http2 It seems the released version of http2 is causing issues with streaming of assets via federator. * Add CHANGELOG entry

* Fix ES reset command in Makefile * fixup! Fix ES reset command in Makefile

* Upgrade cachix to 1.3.1 * Trivial change to force rebuilding of haddocks

…#3148) This reverts commit fd78663.

* Add `flakyTestCase` command and use it. This should make life slightly more bearable for everybody including concourse, while still allowing to run the pending tests locally by setting `RUN_FLAKY_TESTS=1`. * Make sanitize-pr faster by only looking at changed files.

…on (#3134) * FS-1530: Allow partial success when removing users from conversations * FS-1530 Adding tests for deleting conversations and removing members * FS-1530 Formatting and hlint * Hi CI * HI CI --------- Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com> Co-authored-by: Igor Ranieri <igor@elland.me>

As defined here: https://wearezeta.atlassian.net/wiki/spaces/CORE/pages/730759311/Frida+release#Backend-package-versions

This reverts commit cad8b61.

…-to-enforce-stronger-password-restrictions-when-setting-a-password

…ord restrictions when setting a password (wireapp#3137)

battermann added 2 commits March 8, 2023 14:59

range

9bb0151

using pw with range everywhere

a0aff3f

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Mar 8, 2023

battermann added 2 commits March 9, 2023 14:37

using min length password in relevant places

c31a899

changelog

7d5468e

battermann marked this pull request as ready for review March 9, 2023 14:54

fisx added 2 commits March 10, 2023 15:27

Generalize password type instances.

51c4fa7

Merge branch 'develop' into SQSERVICES-1931-wire-server-allow-backend…

29f4602

…-to-enforce-stronger-password-restrictions-when-setting-a-password

fisx reviewed Mar 10, 2023

View reviewed changes

fisx added 2 commits March 13, 2023 09:13

hi ci

1731b48

fix integration test.

b2adfe0

fisx added 3 commits March 13, 2023 12:17

automatic renames (1)

fca8388

find services/ libs/ tools/ -name '*.hs' -exec perl -i -pe 's/\bPlainTextPassword\b([^'\''])/PlainTextPassword6\1/g' {} \;

automatic renames (2)

fe632a7

find services/ libs/ tools/ -name '*.hs' -exec perl -i -pe 's/\bPlainTextPasswordMinLength8\b/PlainTextPassword8/g' {} \;

fisx approved these changes Mar 13, 2023

View reviewed changes

test login with 6 char password still works

becc36e

fisx and others added 10 commits March 15, 2023 10:49

hi ci

8abfe18

Downgrade to our fork of http2 (#3141)

3766cbd

* Downgrade to our fork of http2 It seems the released version of http2 is causing issues with streaming of assets via federator. * Add CHANGELOG entry

Add release note (#3146)

952b67f

Add changelog for Release 2023-03-06

25400f5

Fix gundeck leak (#3136)

55ccf1c

Fix ES reset command in Makefile (#3114)

c21f8e3

* Fix ES reset command in Makefile * fixup! Fix ES reset command in Makefile

Upgrade cachix to 1.3.1 (#3144)

82f4d8f

* Upgrade cachix to 1.3.1 * Trivial change to force rebuilding of haddocks

Revert "Use openssl instead of tls in federator http2 client (#3051)" (…

5ba26cc

…#3148) This reverts commit fd78663.

Add docs for creating diagrams in markdown files

91b315e

lepsa and others added 7 commits March 15, 2023 10:39

[SQSERVICES-1942] Fix DPoP access token error propagation (2/2) (#3142)

2127ded

OAuth (#2989)

01838ca

Set versions in Helm charts for Frida (BUND release)

974cd63

As defined here: https://wearezeta.atlassian.net/wiki/spaces/CORE/pages/730759311/Frida+release#Backend-package-versions

Revert "Set versions in Helm charts for Frida (BUND release)"

4d69dc0

This reverts commit cad8b61.

resolved merge issue

c2c94c5

Merge branch 'develop' into SQSERVICES-1931-wire-server-allow-backend…

613919b

…-to-enforce-stronger-password-restrictions-when-setting-a-password

battermann merged commit 1cb370d into develop Mar 15, 2023

battermann deleted the SQSERVICES-1931-wire-server-allow-backend-to-enforce-stronger-password-restrictions-when-setting-a-password branch March 15, 2023 15:59

zebot mentioned this pull request Apr 17, 2023

Release 2023-04-17 - (expected chart version 4.35.0) #3230

Merged

lepsa pushed a commit to lepsa/wire-server that referenced this pull request Nov 28, 2023

[SQSERVICES-1931] wire server allow backend to enforce stronger passw…

ca8e838

…ord restrictions when setting a password (wireapp#3137)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SQSERVICES-1931] wire server allow backend to enforce stronger password restrictions when setting a password#3137

[SQSERVICES-1931] wire server allow backend to enforce stronger password restrictions when setting a password#3137
battermann merged 29 commits intodevelopfrom
SQSERVICES-1931-wire-server-allow-backend-to-enforce-stronger-password-restrictions-when-setting-a-password

battermann commented Mar 8, 2023 •

edited

Loading

Uh oh!

battermann commented Mar 9, 2023

Uh oh!

fisx left a comment

Uh oh!

fisx Mar 10, 2023

Uh oh!

fisx Mar 10, 2023

Uh oh!

fisx Mar 10, 2023

Uh oh!

fisx commented Mar 13, 2023

Uh oh!

fisx commented Mar 13, 2023

Uh oh!

fisx left a comment

Uh oh!

battermann commented Mar 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Comments

		type PlainTextPassword = PlainTextPassword' (6 :: Nat)

		type PlainTextPasswordMinLength8 = PlainTextPassword' (8 :: Nat)

Conversation

battermann commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

battermann commented Mar 9, 2023

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

fisx Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

fisx Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

fisx Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

fisx commented Mar 13, 2023

Uh oh!

fisx commented Mar 13, 2023

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

battermann commented Mar 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Comments

battermann commented Mar 8, 2023 •

edited

Loading