Password changes #2446

corrideat · 2024-12-06T20:01:29Z

Closes #1923

cypress · 2024-12-06T20:11:47Z

group-income Run #3597

Run Properties: Passed #3597 • a335b1484b ℹ️: Merge e8f52016cca709df74fd77b186835100933171f6 into 6b1ad586cc42852aabfbc98ef52c...

Project	`group-income`
Branch Review	`feature/password-changing`
Run status	`Passed #3597`
Run duration	`11m 32s`
Commit	`a335b1484b ℹ️: Merge e8f52016cca709df74fd77b186835100933171f6 into 6b1ad586cc42852aabfbc98ef52c...`
Committer	`Ricardo Iván Vieitez Parra`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`10`
Skipped	`0`
Passing	`112`
View all changes introduced in this branch ↗︎

taoeffect

Questions...

frontend/controller/actions/identity.js

shared/domains/chelonia/Secret.js

frontend/controller/app/identity.js

frontend/controller/actions/identity.js

shared/zkpp.js

shared/domains/chelonia/encryptedData.js

taoeffect

Preliminary review ready!

Also, could you please add one test to signup-and-login.spec.js to test the changing of the password? I.e. it can do the following:

After logging in, change the password.
Log out
Verify that logging back in with the old password doesn't work
Verify that logging back in with the new password does work

corrideat · 2024-12-14T20:24:52Z

I believe this is ready to merge, for the most part. ~~However, when you test it, you'll notice that the validation error messages seem to be default ones, like minLength. I can add the appropriate messages if you tell me what they should be.~~

corrideat · 2024-12-15T20:37:36Z

backend/routes.js

@@ -116,7 +116,24 @@ route.POST('/event', {
          }
        }
      }
+      const saltUpdateToken = request.headers['shelter-salt-update-token']


This implements the atomic update using a token.

corrideat · 2024-12-15T20:37:54Z

backend/routes.js

@@ -821,7 +838,7 @@ route.GET('/zkpp/{name}/contract_hash', {
  return Boom.internal('internal error')
 })

-route.POST('/zkpp/updatePasswordHash/{name}', {
+route.POST('/zkpp/{name}/updatePasswordHash', {


Renamed for consistency

corrideat · 2024-12-15T20:38:20Z

backend/zkppSalt.js

@@ -60,10 +63,23 @@ export const initZkpp = async () => {
  recordSecret = Buffer.from(hashStringArray('private/recordSecret', IKM)).toString('base64')
  challengeSecret = Buffer.from(hashStringArray('private/challengeSecret', IKM)).toString('base64')
  registrationSecret = Buffer.from(hashStringArray('private/registrationSecret', IKM)).toString('base64')
+  hashUpdateSecret = Buffer.from(hashStringArray('private/hashUpdateSecret', IKM)).toString('base64')


Used for encrypting the update token.

Are these strings like 'private/hashUpdateSecret' something that should be put in zkppConstants.js too?

Probably not, since they're only used here for initialisation purposes.

They also have no particular significance with regard to the protocol.

corrideat · 2024-12-15T20:38:59Z

backend/zkppSalt.js

+  }
+
+  return (cid: ?string) => {
+    return setZkppSaltRecord(contract, hashedPassword, authSalt, contractSalt, cid)


Two-step process to allow validating the token first before making the change.

corrideat · 2024-12-15T21:12:12Z

frontend/views/containers/user-settings/UserProfile.vue

@@ -73,6 +73,7 @@
        i18n.is-title-3(tag='h3' class='card-header') Delete account
        p
          i18n Deleting your account will erase all your data, and remove you from the groups you belong to.
+          | {{ ' ' }}


This fixes a display issue (there was no space)

corrideat · 2024-12-15T21:12:39Z

shared/domains/chelonia/encryptedData.js

@@ -8,10 +8,20 @@ import { isRawSignedData, signedIncomingData } from './signedData.js'
 const rootStateFn = () => sbp('chelonia/rootState')

 export interface EncryptedData<T> {
+  // The ID of the encryption key used


Explanation you requested added here.

corrideat · 2024-12-15T21:14:29Z

backend/zkppSalt.js


-    return true
+    return encryptContractSalt(c, JSON.stringify([oldContractSalt, token]))


Instead of returning true, we return the token for requesting an update. We also return the oldContractSalt because it's helpful for avoiding an extra request, as we need it to decrypt the old (current) IEK.

corrideat · 2024-12-15T21:16:33Z

backend/zkppSalt.js

-      if (!Array.isArray(recordObj) || recordObj.length !== 3 || !recordObj.reduce((acc, cv) => acc && typeof cv === 'string', true)) {
+      if (
+        !Array.isArray(recordObj) ||
+        (recordObj.length !== 3 && recordObj.length !== 4) ||


This is because before the record was a triplet, and now it can be a triplet or a 4-tuple (with the 4th entry being a CID, if there's been a password update)

corrideat · 2024-12-15T21:17:16Z

backend/zkppSalt.js

@@ -251,10 +273,10 @@ export const getContractSalt = async (contract: string, r: string, s: string, si
    throw new Error('getContractSalt: Bad challenge')
  }

-  return encryptContractSalt(c, contractSalt)
+  return encryptContractSalt(c, JSON.stringify([contractSalt, cid]))


We now return the contractSalt and the CID.

corrideat · 2024-12-15T21:18:28Z

frontend/controller/actions/identity.js

 export default (sbp('sbp/selectors/register', {
  'gi.actions/identity/create': async function ({
-    IPK,
-    IEK,
+    IPK: wIPK,


These changes aren't strictly related to this PR, rather something that slipped through the cracks in the past. This wraps secrets in Secret objects.

corrideat · 2024-12-15T21:19:55Z

frontend/controller/app/identity.js

@@ -187,7 +187,41 @@ export default (sbp('sbp/selectors/register', {
      'hc': Buffer.from(hc).toString('base64').replace(/\//g, '_').replace(/\+/g, '-').replace(/=*$/, '')
    })).toString()}`).then(handleFetchResult('text'))

-    return decryptContractSalt(c, contractHash)
+    // [contractSalt, cid]
+    return JSON.parse(decryptContractSalt(c, contractHash))


This is because of the API change, now instead of just contractSalt, it could include a CID.

corrideat · 2024-12-15T21:20:32Z

frontend/controller/app/identity.js

-        // TODO: Wrap IPK and IEK in "Secret"
-        IPK: serializeKey(IPK, true),
-        IEK: serializeKey(IEK, true),
+        IPK: new Secret(serializeKey(IPK, true)),


Implement the old TODO.

corrideat · 2024-12-15T21:21:21Z

frontend/controller/app/identity.js

+
+    const [newContractSalt, oldContractSalt, updateToken] = await sbp('gi.app/identity/updateSaltRequest', username, wOldPassword, wNewPassword)
+
+    const oldIPK = await deriveKeyFromPassword(EDWARDS25519SHA512BATCH, oldPassword, oldContractSalt)


We do this here to avoid ever sending the password to the SW.

corrideat · 2024-12-15T21:21:41Z

frontend/views/containers/access/PasswordModal.vue

@@ -14,7 +14,6 @@ modal-template(class='is-centered is-left-aligned' back-on-mobile=true ref='moda
      :value='form'
      :$v='$v'
      @enter='changePassword'
-      @input='(password) => { newPassword = password }'


These seemed to be wrong.

corrideat · 2024-12-15T21:22:08Z

frontend/views/containers/access/PasswordModal.vue

-        :disabled='$v.form.$invalid'
-      ) Change password
-
-  template(slot='errors') {{ form.response }}


This didn't work, and the slot doesn't exist. Replaced it with banner-scoped.

taoeffect

Really impressive work @corrideat!

It seems to work well in my tests in almost every browser. However, there seems to be an issue with Safari:

Steps:

Create account u1 in Firefox Dev Edition, with group
Open Safari, attempt to sign in as u1

Results:

Server log shows:

[17:00:50.538] DEBUG (14007): 127.0.0.1: GET /assets/js/undefined/name/u1 --> 404
[17:01:15.697] DEBUG (14007): [pubsub] Pinging clients
[17:01:45.698] DEBUG (14007): [pubsub] Pinging clients
[17:02:15.510] DEBUG (14007): 127.0.0.1: GET /assets/js/undefined/name/u1 --> 404
[17:02:15.699] DEBUG (14007): [pubsub] Pinging clients

taoeffect · 2024-12-16T01:09:04Z

frontend/views/containers/access/PasswordModal.vue

+      })().finally(() => {
+        this.processing = false
+      })


Nice job preventing accidental double-clicks!

frontend/setupChelonia.js

taoeffect · 2024-12-18T19:48:07Z

backend/routes.js

+      let updateSalts
+      if (saltUpdateToken) {
+        // If we've got a salt update token (i.e., a password change), fetch
+        // the username associated to the contract to see if they match, and
+        // then validate the token
+        const name = request.headers['shelter-name']
+        const namedContractID = name && await sbp('backend/db/lookupName', name)
+        if (namedContractID !== deserializedHEAD.contractID) {
+          throw new Error('Mismatched contract ID and name')
+        }
+        updateSalts = await redeemSaltUpdateToken(name, saltUpdateToken)
+      }
      await sbp('backend/server/handleEntry', deserializedHEAD, request.payload)
+      // If it's a salt update, do it now after handling the message. This way
+      // we make it less likely that someone will end up locked out from their
+      // identity contract.
+      await updateSalts?.(deserializedHEAD.hash)


Claude:

There's a potential race condition between validating the token and applying the update. If multiple requests come in with the same token, the validation could succeed for both but only one should be allowed to update. The token validation and update should be atomic.

Is he full of it?

Potentially it could happen, however, not really, because the token only applies to one contract and we can't write to the same contract simultaneously (or shouldn't be able to), so handleEntry will throw.

taoeffect

Amazing!

Add gi.app/identity/changePassword

57eb477

corrideat requested a review from taoeffect December 6, 2024 20:01

corrideat self-assigned this Dec 6, 2024

Update salts and rotate keys

6a5a0d5

corrideat force-pushed the feature/password-changing branch from 7c5c849 to 6a5a0d5 Compare December 7, 2024 18:07

Generate authSalt and contractSalt

140c826

corrideat force-pushed the feature/password-changing branch from b6358a9 to 140c826 Compare December 7, 2024 19:39

Key storage

355528c

taoeffect reviewed Dec 7, 2024

View reviewed changes

frontend/controller/actions/identity.js Outdated Show resolved Hide resolved

frontend/controller/actions/identity.js Outdated Show resolved Hide resolved

Merge branch 'master' into feature/password-changing

ffac7d8

corrideat force-pushed the feature/password-changing branch from bb86fa5 to e4d05fd Compare December 11, 2024 16:33

WIP Password change

e88c818

corrideat force-pushed the feature/password-changing branch from e4d05fd to e88c818 Compare December 11, 2024 16:55

corrideat added 4 commits December 11, 2024 18:35

Merge branch 'master' into feature/password-changing

2285233

Re-enable PW change form, better Secret class

882d00e

Readability, encrypt all past IEKs

3572c04

Types and comments

355eb78

corrideat commented Dec 13, 2024

View reviewed changes

frontend/controller/actions/identity.js Outdated Show resolved Hide resolved