[WIP] Signatures and encryption: Preliminary work #1257

corrideat · 2022-05-01T19:45:52Z

No description provided.

frontend/controller/actions/identity.js

frontend/utils/crypto.js

shared/domains/chelonia/chelonia.js

frontend/utils/crypto.js

frontend/utils/crypto.test.js

shared/domains/chelonia/GIMessage.js

shared/domains/chelonia/chelonia.js

.flowconfig

frontend/controller/actions/mailbox.js

shared/domains/chelonia/GIMessage.js

shared/domains/chelonia/internals.js

shared/domains/chelonia/chelonia.js

taoeffect

Very nice work @corrideat, excellent progress. 👍

Preliminary review done! Please see new comments as well as old unaddressed ones.

corrideat

.

shared/domains/chelonia/chelonia.js

taoeffect

Prelim review continued!

backend/routes.js

backend/zkppSalt.js

shared/domains/chelonia/internals.js

backend/zkppSalt.js

taoeffect · 2022-11-04T23:42:58Z

shared/domains/chelonia/chelonia.js

+  contractName: string;
+  contractID: string;
+  data: GIOpKeyRequestResponse;
+  signingKeyId: string;


With so many types, maybe it makes sense to move them into their own individual types.js file?

Yes, that sounds reasonable.

shared/domains/chelonia/chelonia.js

- Volatile state (for keys) in simulated state - Remove state when logging out

frontend/controller/actions/group.js

…essage signatures

shared/domains/chelonia/chelonia.js

taoeffect · 2023-01-24T22:10:23Z

frontend/views/containers/proposals/ProposalVoteOptions.vue

+            /* passPayload = createInvite({
              invitee: this.proposal.data.proposalData.member,
              creator: this.proposal.meta.username,
              expires: this.currentGroupState.settings.inviteExpiryProposal
-            })
+            }) */


Whenever you're ready to clean up code, if you could address this please (just a reminder to not forget it)

taoeffect · 2023-02-14T00:06:33Z

shared/domains/chelonia/GIMessage.js

-      manifest,
-      // the nonce makes it difficult to predict message contents
-      // and makes it easier to prevent conflicts during development
-      nonce: Math.random()


Why did you get rid of the nonce?

Because: (1) all signed messages (which should be all messages) should already be unique due to the randomness in the signature, (2) the nonce doesn't really make it difficult to predict message contents, (2 bis) even if it did, Math.random() isn't really unpredictable, (3) predicting message contents isn't bad in itself as the DB should be assumed to be public, (4) conflicts due to missing a nonce most certainly mean there's a bug.

OK, I think I'm going to add it back in because of #1503 and it sounds like there's nothing wrong with it.

Well, Math.random() was probably something that was wrong with it (if it needs to be cryptographically secure at least). I think that #1503 / #1486 probably point out to other issues.

Messages can be re-created by 'chelonia/private/out/publishEvent', causing the hash of the successful message to be different, resulting in a hanging promise.

I'd say that the conditions that lead to a hanging promise seem to be the issue here, as not having a nonce isn't the only reason why this might happen. In other words, hanging promises should be handled for what they are, but that's a separate issue.

Other than that, I don't think a nonce is actively bad here, just unnecessary. The danger is relying on it being unique, which can't be guaranteed (realistically).

Just an FYI this nonce was recently added back in a recent PR. In the future, if we want to get rid of it, it will require changing how chat pending messages work by setting the id in the data when creating a chat message. Each chat message creation would need to generate its own unique ID as part of the data of the message, and then it wouldn't need to rely on the GIMessage .id() API

backend/routes.js

taoeffect · 2023-02-16T00:06:00Z

backend/zkppSalt.js

+const recordPepper = 'pepper'
+const recordMasterKey = 'masterKey'
+const challengeSecret = 'secret'
+const registrationSecret = 'secret'


We don't want to have server admins dealing with anything related to cryptography unless for some reason they must and there's literally no way around it.

So what are these values and how can we avoid making the admin set them via configs?

It's unclear to me if these values must be different from server to server or whether they don't need to be. The consequences / explanations aren't explained anywhere.

So what are these values and how can we avoid making the admin set them via configs?

These are secrets the server uses internally for deriving certain values that must be opaque to the client. As these values need to be persistent, I can't come up with a way of avoiding them entirely (although a solution could be for them to be set to some default value).9

const recordPepper = 'pepper'

Perhaps the least security-critical value. Used to derive a record ID which clients / 3rd parties cannot guess for storing the ZKPP

const recordMasterKey = 'masterKey'

Used to encrypt ZKPP records. If this value is leaked, 3rd parties with access to the database could defeat the protocol.

const challengeSecret = 'secret'

If this value is known, the protocol can be defeated by providing valid answers to challenges.

const registrationSecret = 'secret'

Similarly, but for registration.

So what about making them random and storing them locally in files? Would that work?

Sure, it could be a part of the installation process, and could be automated.

I'm going to use this convention to mark threads as needing their commentary transferred (as comments) to the code.

➡️ code comments

Note from call: we can get rid of recordPepper, just use private/rid/${contractID}

challengeSecret is the key to the keyed hash function in step 3 of the protocol.

taoeffect · 2023-03-05T21:34:52Z

frontend/views/pages/GroupChat.vue

@@ -190,7 +190,7 @@ export default ({
      })
      if (this.isDirectMessage(chatRoomId)) {
        this.updateCurrentChatRoomID(chatRoomId)
-      } else if (chatRoomId && chatRoomId !== this.currentChatRoomId) {
+      } else if (chatRoomId) {


I'm currently working on fixing a merge conflict with this.

What was the reason this change was made?

I'm leaning toward simply accepting the changes from Alex's DMs PR over this, but if there was a good reason for this would like to know.

@Silver-IT maybe you can comment here as to the purpose of this && chatRoomId !== this.currentChatRoomId ?

It's nice catch, actually. As I added debounce to refreshContent function in ChatMain.vue, this is not needed.
I have found more unnecessary codes in this watch, so I am going to update this in the next PR which I am currently working on. It's for fixing Issue #1493.

I want you to just follow the current master branch so that I can optimize it later.

@Silver-IT 👍

taoeffect · 2023-03-06T00:44:22Z

frontend/model/contracts/group.js

+      async sideEffect ({ data, meta, contractID }, { state: Rstate }) {
+        const rootState = sbp('state/vuex/state')
+        const contracts = rootState.contracts || {}
+        const { identityContractID } = rootState.loggedIn
+        const userState = rootState[identityContractID]
+        const state = rootState[contractID]
+
+        if (rootState[data.chatRoomID]?._volatile) {
+          return
+        }
+
+        await sbp('chelonia/out/keyRequest', {
+          originatingContractID: identityContractID,
+          originatingContractName: contracts[identityContractID].type,
+          contractID: data.chatRoomID,
+          contractName: 'gi.contracts/chatroom',
+          signingKey: state._volatile?.keys?.[(((Object.values(Object(state._vm?.authorizedKeys)): any): GIKey[]).find((k) => k?.meta?.type === 'csk')?.id: ?string)],
+          innerSigningKeyId: ((Object.values(userState._vm.authorizedKeys): any): GIKey[]).find((k) => k.meta?.type === 'csk')?.id,
+          encryptionKeyId: ((Object.values(userState._vm.authorizedKeys): any): GIKey[]).find((k) => k.meta?.type === 'cek')?.id,
+          hooks: {
+            prepublish: null,
+            postpublish: null
+          }
+        })


Could you add a comment explaining the specific scenario that's being handled here?

I'll do; the specific scenario here is joining chatrooms (should be the general channel in this case) . This is required because joining a chatroom is a two-step process (i.e., first a group is joined, we get the list of chatrooms and then request to join) due to them being encrypted. In the future, this could be optimised so that multiple contracts can be 'joined' (for want of a better word) in a single operation

Update: we agreed that this code should be removed so that a different method of obtaining the chatroom key can be implemented instead (by pre-sharing the key in the group contract using OP_KEYSHARE)

taoeffect · 2023-03-06T01:19:59Z

frontend/model/contracts/group.js

+          originatingContractName: contracts[identityContractID].type,
+          contractID: data.chatRoomID,
+          contractName: 'gi.contracts/chatroom',
+          signingKey: state._volatile?.keys?.[(((Object.values(Object(state._vm?.authorizedKeys)): any): GIKey[]).find((k) => k?.meta?.type === 'csk')?.id: ?string)],


Which key is being used here? It seems to say "pick the first CSK of the authorizedKeys"? Is that right? If so, why would that be correct? If not, what is going on?

That's correct: it does "pick the first CSK of the authorizedKeys". Not sure what you mean by 'why would that be correct?' though. What scenarios do you have in mind that would not make this correct? If it's the "pick the first CSK" part that's confusing, you can read it as "pick the ~~first~~ CSK", as there should be only one CSK at any given time (perhaps need to check the key management code to ensure this holds, but it should hold).

taoeffect · 2023-03-06T01:20:38Z

frontend/model/contracts/group.js

+          contractID: data.chatRoomID,
+          contractName: 'gi.contracts/chatroom',
+          signingKey: state._volatile?.keys?.[(((Object.values(Object(state._vm?.authorizedKeys)): any): GIKey[]).find((k) => k?.meta?.type === 'csk')?.id: ?string)],
+          innerSigningKeyId: ((Object.values(userState._vm.authorizedKeys): any): GIKey[]).find((k) => k.meta?.type === 'csk')?.id,


Similar question here, a comment would be helpful here explaining which key is being picked and why.

This is finding the CSK by name under the assumption that there is only one CSK (which holds for group contracts)

Seems like something that we should have a cleaner way of doing?

taoeffect · 2023-03-06T01:38:33Z

shared/domains/chelonia/GIMessage.js

-    signatureFn?: Function = defaultSignatureFn
+    {
+      contractID,
+      originatingContractID,


What is this originatingContractID, and which Wiki document discusses it?

contractID is the contract being modified. originatingContractID is the contract initiating the interaction (used for cross-contract communication). In other words, contractID is self-referential in a given contract and originatingContractID refers to the contract that wrote to it. This is documented in the Key sharing document.

shared/domains/chelonia/internals.js

shared/domains/chelonia/chelonia.js

taoeffect · 2023-03-20T23:39:50Z

shared/domains/chelonia/GIMessage.js

-      message
+      head,
+      message,
+      decryptedValue: op.length === 3 ? op[2] : op[1],


Why is decryptedValue always being set?

This is the relevant commit: 2f70afa
In particular, see the change made to frontend/views/containers/chatroom/ChatMain.vue

Decrypted value is required to introspect messages. For compatibility purposes with the previous API, that didn't use decryptedValue API as an argument, decryptedValue is set automatically. This was for being cautious and is likely the argument can be assumed to be present.

taoeffect · 2023-05-05T02:05:15Z

Closing in favor of #1521

corrideat added 2 commits May 1, 2022 17:08

Signatures and encryption: Preliminary work

ebe6ee0

Fix typo

05976eb

taoeffect mentioned this pull request May 2, 2022

[WIP] Feature/password state #1210

Closed

taoeffect reviewed May 2, 2022

View reviewed changes

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

taoeffect reviewed May 2, 2022

View reviewed changes

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

taoeffect reviewed May 2, 2022

View reviewed changes

frontend/utils/crypto.js Outdated Show resolved Hide resolved

taoeffect reviewed May 2, 2022

View reviewed changes

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

taoeffect reviewed May 2, 2022

View reviewed changes

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

snowteamer reviewed May 6, 2022

View reviewed changes

Updated tests, added key ops, added keys to contracts

c690fe3

taoeffect reviewed May 21, 2022

View reviewed changes

.flowconfig Outdated Show resolved Hide resolved

taoeffect reviewed May 21, 2022

View reviewed changes

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

taoeffect reviewed May 21, 2022

View reviewed changes

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

taoeffect reviewed May 21, 2022

View reviewed changes

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

taoeffect reviewed May 21, 2022

View reviewed changes

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

taoeffect reviewed May 21, 2022

View reviewed changes

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

taoeffect requested changes May 21, 2022

View reviewed changes

corrideat added 2 commits June 5, 2022 21:52

New OP_KEYSHARE

93274b3

Merge branch 'master' into feature/password-state-new

b1e3763

corrideat commented Jun 7, 2022

View reviewed changes

taoeffect reviewed Jun 7, 2022

View reviewed changes

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

Broken changes

0e366f7

taoeffect mentioned this pull request Jul 6, 2022

Move display names into group profile [$100 bounty] #1329

Closed

corrideat added 2 commits July 28, 2022 10:21

ZKPP password salt

94a7601

ZKPP: Frontend implementation

e9e3851

taoeffect marked this pull request as draft August 15, 2022 10:55

Merge branch 'master' into feature/password-state-new

c6802d4

corrideat force-pushed the feature/password-state-new branch from 309ad07 to c6802d4 Compare August 15, 2022 12:11

corrideat added 2 commits August 15, 2022 14:18

Prevent Grunt from running

1051639

OP_KEY_REQUEST

e86df63

Invite link state, some bugfixes

579474c

taoeffect requested changes Nov 4, 2022

View reviewed changes

taoeffect reviewed Nov 4, 2022

View reviewed changes

backend/zkppSalt.js Show resolved Hide resolved

taoeffect reviewed Nov 4, 2022

View reviewed changes

backend/zkppSalt.js Show resolved Hide resolved

taoeffect reviewed Nov 4, 2022

View reviewed changes

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

- Implement feedback from PR

64e34d3

- Volatile state (for keys) in simulated state - Remove state when logging out

taoeffect reviewed Nov 8, 2022

View reviewed changes

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

corrideat added 3 commits November 14, 2022 16:55

Updates: better approach to withEnv as suggestion or code + Enforce m…

b44dcb2

…essage signatures

Bugfixes: login, group joining

42df852

Functionality for joining chat

ce5223e

taoeffect reviewed Jan 19, 2023

View reviewed changes

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

State management: support chat

2f70afa

taoeffect reviewed Jan 24, 2023

View reviewed changes

corrideat added 2 commits January 30, 2023 13:55

Bugfixes and PoC for tests

8fa39c1

Merge branch 'master' into feature/password-state-new

a6a3ebd

taoeffect reviewed Feb 14, 2023

View reviewed changes

taoeffect mentioned this pull request Feb 14, 2023

Listening for message hash in prepublish hooks isn't safe! #1487

Closed

taoeffect reviewed Feb 15, 2023

View reviewed changes

backend/routes.js Show resolved Hide resolved

taoeffect reviewed Feb 16, 2023

View reviewed changes

taoeffect reviewed Mar 5, 2023

View reviewed changes

taoeffect reviewed Mar 6, 2023

View reviewed changes

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

taoeffect reviewed Mar 6, 2023

View reviewed changes

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

taoeffect reviewed Mar 20, 2023

View reviewed changes

taoeffect closed this May 5, 2023

[WIP] Signatures and encryption: Preliminary work #1257

[WIP] Signatures and encryption: Preliminary work #1257

Conversation

corrideat commented May 1, 2022

taoeffect left a comment

Choose a reason for hiding this comment

corrideat left a comment

Choose a reason for hiding this comment

taoeffect left a comment

Choose a reason for hiding this comment

taoeffect Nov 4, 2022 • 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 Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

taoeffect Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taoeffect Mar 3, 2023 • 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

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

corrideat Apr 1, 2023 • 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

corrideat Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

taoeffect commented May 5, 2023

taoeffect Nov 4, 2022 •

edited

Loading

taoeffect Mar 30, 2023 •

edited

Loading

taoeffect Feb 16, 2023 •

edited

Loading

taoeffect Mar 3, 2023 •

edited

Loading

corrideat Apr 1, 2023 •

edited

Loading

corrideat Apr 1, 2023 •

edited

Loading