PEK rotation improvements. #2584

corrideat · 2025-02-01T22:22:36Z

Closes #2570.

Testing instructions:

Create three users (alice, bob, carol)
alice creates a group G1
bob creates a group G2
carol joins G1
carol leaves G1. There are no warning or errors for /rotateKeys. The pek for carol (in _vm.authorizedKeys) has been rotated.
carol joins G2
bob observes that the username for carol appears in G2.

Closes #2570.

taoeffect · 2025-02-01T22:24:59Z

backend/push.js

@@ -239,6 +239,7 @@ export const postEvent = async (subscription: Object, event: ?string): Promise<v
  })

  if (!req.ok) {
+    console.debug('Error sending push notification', subscription.id, req.status)


Should this be console.debug or something higher like log or info?

Maybe. I thought debug is appropriate since it's by definition a user-defined endpoint. Nothing stops a user from reporting an endpoint like https://www.bankofamerica.com/, and since it's a third-party endpoint, we also can't control the SLA.

cypress · 2025-02-02T03:34:18Z

group-income Run #3884

Run Properties: Passed #3884 • 8103a06b0e ℹ️: Merge bdf61237a1d8c4b388141391892b0e98c51be000 into 5b89d7022aca2200db3d2fb01f94...

Project	`group-income`
Branch Review	`2570-failure-to-show-username-for-contract-upon-join`
Run status	`Passed #3884`
Run duration	`12m 13s`
Commit	`8103a06b0e ℹ️: Merge bdf61237a1d8c4b388141391892b0e98c51be000 into 5b89d7022aca2200db3d2fb01f94...`
Committer	`Ricardo Iván Vieitez Parra`
View all properties for this run ↗︎

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

corrideat · 2025-02-02T16:31:49Z

frontend/views/containers/chatroom/ChatMain.vue

@@ -947,6 +947,8 @@ export default ({
        }
        sbp('gi.actions/identity/kv/setChatRoomReadUntil', {
          contractID: chatRoomID, messageHash, createdHeight
+        }).catch(e => {


corrideat · 2025-02-02T16:32:10Z

shared/domains/chelonia/chelonia.js

@@ -537,6 +538,20 @@ export default (sbp('sbp/selectors/register', {
    const keyId = findSuitableSecretKeyId(contractIDOrState, permissions, purposes, ringLevel, allowedActions)
    return keyId
  },
+  'chelonia/contract/setPendingKeyRevocation': function (contractID: string, names: string[]) {


Improvement as per contract TODOs.

corrideat · 2025-02-02T16:33:55Z

shared/domains/chelonia/chelonia.js

@@ -1693,9 +1708,17 @@ async function outEncryptedOrUnencryptedAction (
  if (opType === GIMessage.OP_ACTION_ENCRYPTED && !params.encryptionKeyId) {
    throw new Error('OP_ACTION_ENCRYPTED requires an encryption key ID be given')
  }
+  if (params.encryptionKey) {


This new parameter is needed to designate a raw key to use for encryption in OP_ATOMIC. The current implementation of GIMessage / encryptedData / signedData / chelonia isn't advanced enough to automatically switch to the new key on outgoing messages. The workaround is designating the key as a raw key.

corrideat · 2025-02-02T16:34:55Z

shared/domains/chelonia/encryptedData.js

@@ -108,7 +108,7 @@ const decryptData = function (height: number, data: any, additionalKeys: Object,
  const key = additionalKeys[eKeyId]

  if (!key) {
-    throw new ChelErrorDecryptionKeyNotFound(`Key ${eKeyId} not found`)
+    throw new ChelErrorDecryptionKeyNotFound(`Key ${eKeyId} not found`, { cause: eKeyId })


Adding the cause here helps identify the key that failed.

corrideat · 2025-02-02T16:36:01Z

test/cypress/support/commands.js

@@ -113,7 +113,7 @@ Cypress.Commands.add('getByDT', (element, otherSelector = '') => {
 cySbpCheckCommand('giNoPendingGroupKeyShares', (sbp) => {
  const state = sbp('state/vuex/state')
  const pending = Object.keys(state.contracts)
-    .filter(contractID => state[contractID]._vm.type === 'gi.contracts/group')
+    .filter(contractID => state[contractID]._vm?.type === 'gi.contracts/group')


For some reason, sometimes _vm was undefined in the tests, causing this to fail.

corrideat · 2025-02-02T16:36:56Z

shared/domains/chelonia/internals.js

@@ -2091,6 +2116,15 @@ const handleEvent = {
    }
    this.config.reactiveSet(state.contracts[contractID], 'HEAD', hash)
    this.config.reactiveSet(state.contracts[contractID], 'height', height)
+    if (missingDecryptionKeyId) {


This adds a log of missed decryption keys, which can be used to more accurately determine when a contract needs to be re-synced.

corrideat · 2025-02-02T16:38:33Z

shared/domains/chelonia/internals.js

@@ -2046,17 +2066,22 @@ const handleEvent = {
      return acc
    }

-    const actionsOpV = ((msg: any): GIOpAtomic).reduce(reducer, [])
+    const actionsOpV = (Number.isFinite(errorIndex)
+      // $FlowFixMe[incompatible-call]


The side-effects sometimes get called if there was an error (i.e., ChelErrorWarning has this behaviour). This only executes side-effects up to the point where process succeeded in OP_ATOMIC.

OK, so based on the call here's how we've agreed conceptually this code should work:

There are two types of "errors" that we need to consider:

"Ignoring" errors

"Failure" errors

Example: OP_KEY_ADD

IGNORING: an error is thrown because we wanted to add a key but the key we wanted to add is already there. This is not a hard error, it's an ignoring error. We don't care that the operation failed in this case because the intent was accomplished.

FAILURE: an error is thrown because we wanted to add a key that doesn't exist.

Example: OP_ACTION_ENCRYPTED

IGNORING: An error is thrown because we don't have the key to decrypt the action. We ignore it.

FAILURE: An error is thrown by the process function during processing.

Handling these in OP_ATOMIC

ALL errors of class "IGNORING" should be ignored. They should not impact our ability to process the rest of the operations in the OP_ATOMIC. No matter how many of these are thrown, it doesn't affect the rest of the operations.

ANY error of class "FAILURE" will call the rest of the operations to fail and the state to be reverted to prior to the OP_ATOMIC. No side-effects should be run. Because an intention failed.

Feel free to copy this 👆 as a comment in the code.

corrideat · 2025-02-02T16:39:08Z

shared/domains/chelonia/internals.js

+            }
+            await opFns[u[0]](u[1])
+          } catch (e) {
+            e[errorIndexSymbol] = i


Point where the error occurred.

corrideat · 2025-02-02T16:39:58Z

frontend/model/getters.js

-          return
-        }
-        const attributes = state[contractID].attributes
+        const attributes = state[contractID]?.attributes


This fixes the ProfileCard not showing if the username isn't known.

corrideat · 2025-02-02T16:40:58Z

frontend/main.js

@@ -49,6 +49,16 @@ console.info('CONTRACTS_VERSION:', process.env.CONTRACTS_VERSION)
 console.info('LIGHTWEIGHT_CLIENT:', process.env.LIGHTWEIGHT_CLIENT)
 console.info('NODE_ENV:', process.env.NODE_ENV)

+if (process.env.CI) {


Add logging information on CI to get a better stack trace and identify missing error handling.

corrideat · 2025-02-02T16:41:54Z

frontend/controller/actions/index.js

@@ -191,8 +187,9 @@ sbp('sbp/selectors/register', {
        contractID,
        contractName,
        data: [
-          ...keyShares.map((data) => ['chelonia/out/keyShare', { data: encryptedOutgoingData(contractID, CEKid, data) }]),
-          ['chelonia/out/keyUpdate', { data: updatedKeys }]
+          ...(keyShares[0] ?? []),


Function updated to support arbitrary operations. This also makes this function generic and no longer GI-specific.

corrideat · 2025-02-02T16:42:30Z

frontend/controller/actions/identity.js

+    return [
+      undefined, // Nothing before OP_KEY_UPDATE
+      [
+        // Re-encrypt attributes with the new PEK


This is 'the fix' for this issue, i.e., re-encrypting attributes.

taoeffect

Preliminary review based on call ready!

taoeffect · 2025-02-03T18:22:53Z

shared/domains/chelonia/internals.js

@@ -2046,17 +2066,22 @@ const handleEvent = {
      return acc
    }

-    const actionsOpV = ((msg: any): GIOpAtomic).reduce(reducer, [])
+    const actionsOpV = (Number.isFinite(errorIndex)
+      // $FlowFixMe[incompatible-call]


OK, so based on the call here's how we've agreed conceptually this code should work:

There are two types of "errors" that we need to consider:

"Ignoring" errors

"Failure" errors

Example: OP_KEY_ADD

IGNORING: an error is thrown because we wanted to add a key but the key we wanted to add is already there. This is not a hard error, it's an ignoring error. We don't care that the operation failed in this case because the intent was accomplished.

FAILURE: an error is thrown because we wanted to add a key that doesn't exist.

Example: OP_ACTION_ENCRYPTED

IGNORING: An error is thrown because we don't have the key to decrypt the action. We ignore it.

FAILURE: An error is thrown by the process function during processing.

Handling these in OP_ATOMIC

ALL errors of class "IGNORING" should be ignored. They should not impact our ability to process the rest of the operations in the OP_ATOMIC. No matter how many of these are thrown, it doesn't affect the rest of the operations.

ANY error of class "FAILURE" will call the rest of the operations to fail and the state to be reverted to prior to the OP_ATOMIC. No side-effects should be run. Because an intention failed.

Feel free to copy this 👆 as a comment in the code.

frontend/controller/actions/index.js

frontend/controller/actions/chatroom.js

frontend/main.js

frontend/model/contracts/group.js

frontend/model/contracts/identity.js

taoeffect · 2025-02-03T20:26:39Z

frontend/model/contracts/identity.js

+        if (!state.chatRooms) {
+          // When creating a DM, we may not have the `.chatRooms` property
+          state.chatRooms = Object.create(null)
+        }


Why would we not have the .chatRooms property? It's created by the constructor

That requires the constructor being called. It won't be if the decryption key isn't available (for example, because it's been rotated).

Could you add a comment here explaining this? I think most people reading this code will have no idea about this.

shared/domains/chelonia/chelonia.js

shared/domains/chelonia/internals.js

taoeffect · 2025-02-05T21:47:19Z

shared/domains/chelonia/internals.js

+            }
+            await opFns[u[0]](u[1])
+          } catch (e) {
+            if (e) {


Why is there an if (e) thing here...? In what situation can this not be defined?

throw undefined, throw null, etc.

Wouldn't it still make sense then to do something here, like log a console.error in an else { .. } branch?

shared/domains/chelonia/internals.js

taoeffect · 2025-02-05T21:58:24Z

shared/domains/chelonia/internals.js

@@ -801,7 +845,9 @@ export default (sbp('sbp/selectors/register', {
                    key: deserializeKey(decrypted),
                    transient
                  }]))
-                  if (
+                  if (missingDecryptionKeyIds?.includes(key.id)) {


Can you add a comment here explaining what this is doing?

shared/domains/chelonia/internals.js

taoeffect

My review is ready!

Claude's review of `internals.js`

After reviewing the diff and the full file, here are a few potential issues and observations:

In the new code for handling errors in OP_ATOMIC, there's a potential issue with the missingDecryptionKeyIds handling. The code checks if missingDecryptionKeyIds is a Set, but later in the file, it's treated as an array. This inconsistency could lead to runtime errors.

// In OP_ATOMIC:
const missingDecryptionKeyIds = missingDecryptionKeyIdsMap.get(message)
if (missingDecryptionKeyIds) {
  missingDecryptionKeyIds.add(e.cause)
} else {
  missingDecryptionKeyIdsMap.set(message, new Set([e.cause]))
}

// Later in applyProcessResult:
if (Array.isArray(missingDecryptionKeyIdsForMessage)) { // This expects an array, not a Set
  // ...
}

The error handling in OP_ATOMIC might have an issue with the nested conditions:

if (e) {
  if (e.name === 'ChelErrorDecryptionKeyNotFound') {
    // ...
  } else {
    // ...
  }
  // ...
  if (e.name === 'ChelErrorWarning') continue
}
throw e

The throw e is outside the first if (e) block, which means it will always throw the error even if e is falsy. It should probably be inside the first if (e) block.

There's a potential memory leak with the WeakMap usage. While WeakMaps are good for garbage collection, the code should ensure that all message objects are properly cleaned up when they're no longer needed. The code does include cleanup in the finally block, but it might be worth adding additional cleanup in error cases.
In the new code that handles missing decryption key IDs, there's a duplicate height update:

if (Array.isArray(missingDecryptionKeyIdsForMessage)) {
  // ...
  missingDecryptionKeyIdsForMessage.forEach(keyId => {
    if (missingDecryptionKeyIds.includes(keyId)) return
    missingDecryptionKeyIds.push(keyId)
  })
  this.config.reactiveSet(state.contracts[contractID], 'height', height) // This is redundant
}

This height update is redundant because it's already being set earlier in the function:

this.config.reactiveSet(state.contracts[contractID], 'height', height)

These issues should be addressed to improve the robustness of the code.

Gemini 2.0 Flash Thinking's Review of `internals.js`

…-upon-join

taoeffect · 2025-02-06T17:37:45Z

test/cypress/integration/group-member-removal.spec.js

+      // TODO: Sometimes it seems to get stuck here when bypassing the UI
+      bypassUI: false


It doesn't seem like this fixed the failing build, so can this be returned to true?

Prior commit failure: https://cloud.cypress.io/projects/q6whky/runs/3885/test-results?actions=%5B%5D&browsers=%5B%5D&groups=%5B%5D&isFlaky=%5B%5D&modificationDateRange=%7B%22startDate%22%3A%221970-01-01%22%2C%22endDate%22%3A%222038-01-19%22%7D&orderBy=EXECUTION_ORDER&oses=%5B%5D&specs=%5B%5D&statuses=%5B%7B%22value%22%3A%22FAILED%22%2C%22label%22%3A%22FAILED%22%7D%5D&testingTypesEnum=%5B%5D

This commit: https://cloud.cypress.io/projects/q6whky/runs/3886/test-results?actions=%5B%5D&browsers=%5B%5D&groups=%5B%5D&isFlaky=%5B%5D&modificationDateRange=%7B%22startDate%22%3A%221970-01-01%22%2C%22endDate%22%3A%222038-01-19%22%7D&orderBy=EXECUTION_ORDER&oses=%5B%5D&specs=%5B%5D&statuses=%5B%7B%22value%22%3A%22FAILED%22%2C%22label%22%3A%22FAILED%22%7D%5D&testingTypesEnum=%5B%5D

taoeffect · 2025-02-06T20:42:01Z

test/cypress/integration/group-member-removal.spec.js

+  it('user3 can see the username for userBot', () => {
+    cy.giLogin(`user3-${userId}`, { bypassUI: true })
+    assertMembersCount(3)
+
+    cy.getByDT('groupMembers').contains('userbot-')
+    cy.window().its('sbp').then(sbp => {
+      const rootState = sbp('state/vuex/state')
+      const userbotId = rootState.namespaceLookups[`userbot-${userId}`]
+      const PEKs = Object.values(rootState[userbotId]._vm.authorizedKeys).filter(key => key.name === 'pek')
+      if (PEKs.length < 2) throw new RangeError('Expected the PEK to have been rotated')
+    })
+  })


Nice tests 👍

PEK rotation improvements.

e08995f

Closes #2570.

corrideat requested a review from taoeffect February 1, 2025 22:22

corrideat self-assigned this Feb 1, 2025

taoeffect reviewed Feb 1, 2025

View reviewed changes

Bugfixes

0d4dd67

corrideat commented Feb 2, 2025

View reviewed changes

taoeffect requested changes Feb 3, 2025

View reviewed changes

corrideat added 2 commits February 3, 2025 19:16

Improvements to OP_ATOMIC

a020203

Fix conflict

3cea61c

taoeffect reviewed Feb 3, 2025

View reviewed changes

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

taoeffect reviewed Feb 3, 2025

View reviewed changes

frontend/controller/actions/chatroom.js Show resolved Hide resolved

taoeffect reviewed Feb 3, 2025

View reviewed changes

frontend/main.js Show resolved Hide resolved

taoeffect reviewed Feb 3, 2025

View reviewed changes

frontend/model/contracts/group.js Outdated Show resolved Hide resolved

taoeffect reviewed Feb 3, 2025

View reviewed changes

frontend/model/contracts/group.js Outdated Show resolved Hide resolved

taoeffect reviewed Feb 3, 2025

View reviewed changes

frontend/model/contracts/identity.js Show resolved Hide resolved

taoeffect reviewed Feb 3, 2025

View reviewed changes

shared/domains/chelonia/chelonia.js Outdated Show resolved Hide resolved

taoeffect reviewed Feb 3, 2025

View reviewed changes

shared/domains/chelonia/chelonia.js Outdated Show resolved Hide resolved

taoeffect reviewed Feb 3, 2025

View reviewed changes

shared/domains/chelonia/internals.js Outdated Show resolved Hide resolved

taoeffect reviewed Feb 5, 2025

View reviewed changes

shared/domains/chelonia/internals.js Outdated Show resolved Hide resolved

taoeffect reviewed Feb 5, 2025

View reviewed changes

shared/domains/chelonia/internals.js Outdated Show resolved Hide resolved

taoeffect reviewed Feb 5, 2025

View reviewed changes

shared/domains/chelonia/internals.js Outdated Show resolved Hide resolved

taoeffect reviewed Feb 5, 2025

View reviewed changes

shared/domains/chelonia/internals.js Outdated Show resolved Hide resolved

taoeffect reviewed Feb 5, 2025

View reviewed changes

shared/domains/chelonia/internals.js Outdated Show resolved Hide resolved

taoeffect requested changes Feb 5, 2025

View reviewed changes

corrideat added 6 commits February 6, 2025 12:27

Add test for PEK rotation

3ca5dd5

Feedback

c3346ff

Move setPendingKeyRevocation call into side effect

bdf6123

Events for failed chatroom events

6eaa497

Merge branch 'master' into 2570-failure-to-show-username-for-contract…

55dcd3a

…-upon-join

Quickfix

1301735

taoeffect reviewed Feb 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PEK rotation improvements. #2584

PEK rotation improvements. #2584

corrideat commented Feb 1, 2025 •

edited

Loading

taoeffect Feb 1, 2025

corrideat Feb 1, 2025

cypress bot commented Feb 2, 2025 •

edited

Loading

corrideat Feb 2, 2025

corrideat Feb 2, 2025

corrideat Feb 2, 2025

corrideat Feb 2, 2025

corrideat Feb 2, 2025

corrideat Feb 2, 2025

corrideat Feb 2, 2025

taoeffect Feb 3, 2025 •

edited

Loading

corrideat Feb 2, 2025

corrideat Feb 2, 2025

corrideat Feb 2, 2025

corrideat Feb 2, 2025

corrideat Feb 2, 2025

taoeffect left a comment

taoeffect Feb 3, 2025 •

edited

Loading

taoeffect Feb 3, 2025

corrideat Feb 6, 2025

taoeffect Feb 6, 2025

taoeffect Feb 5, 2025

corrideat Feb 6, 2025

taoeffect Feb 6, 2025

taoeffect Feb 5, 2025

taoeffect left a comment

taoeffect Feb 6, 2025

taoeffect Feb 6, 2025

		// TODO: Sometimes it seems to get stuck here when bypassing the UI
		bypassUI: false

PEK rotation improvements. #2584

Are you sure you want to change the base?

PEK rotation improvements. #2584

Conversation

corrideat commented Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Feb 2, 2025 • edited Loading

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

taoeffect Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Example: OP_KEY_ADD

Example: OP_ACTION_ENCRYPTED

Handling these in OP_ATOMIC

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

taoeffect left a comment

Choose a reason for hiding this comment

taoeffect Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Example: OP_KEY_ADD

Example: OP_ACTION_ENCRYPTED

Handling these in OP_ATOMIC

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

taoeffect left a comment

Choose a reason for hiding this comment

Claude's review of internals.js

Gemini 2.0 Flash Thinking's Review of internals.js

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corrideat commented Feb 1, 2025 •

edited

Loading

cypress bot commented Feb 2, 2025 •

edited

Loading

taoeffect Feb 3, 2025 •

edited

Loading

Example: `OP_KEY_ADD`

Example: `OP_ACTION_ENCRYPTED`

Handling these in `OP_ATOMIC`

taoeffect Feb 3, 2025 •

edited

Loading

Example: `OP_KEY_ADD`

Example: `OP_ACTION_ENCRYPTED`

Handling these in `OP_ATOMIC`

Claude's review of `internals.js`

Gemini 2.0 Flash Thinking's Review of `internals.js`