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

Add senderHmac and shouldPush to MessageV2 #519

Merged
merged 21 commits into from
Jan 25, 2024
Merged

Add senderHmac and shouldPush to MessageV2 #519

merged 21 commits into from
Jan 25, 2024

Conversation

rygine
Copy link
Collaborator

@rygine rygine commented Jan 12, 2024

context: https://community.xmtp.org/t/xip-35-message-sender-identifier-in-topic/509/2

in this PR:

  • added shouldPush to ContentCodec type
  • added shouldPush method to TextCodec and CompositeCodec
  • added senderHmac and shouldPush to MessageV2
  • added encryption methods for generating, importing, and exporting HMAC keys
  • added encryption methods for generating and verifying HMAC signatures
  • added getV2ConversationHmacKeys to InMemoryKeystore to retrieve conversation topics and their associated HMAC keys

TODO:

  • confirm encryption correctness
  • add unit tests

@rygine rygine requested a review from a team January 12, 2024 23:35
src/keystore/InMemoryKeystore.ts Show resolved Hide resolved
encrypted: ciphertext,
senderHmac: await generateHmacSignature(
keyMaterial,
new TextEncoder().encode(contentTopic),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the hkdf salt/domain, I would suggest we also include some kind of time-dependent value, such as the number of months since the Unix epoch: xmtp/XIPs#35 (comment). This ensures that we have automatic key rotation

Copy link
Contributor

@neekolas neekolas Jan 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My worry about the time-dependent value is the complexity of keeping the push server up to date with the correct keys. If we had it roll over at 12am on the first of the month UTC, we'd guarantee that push notifications would not be correctly filtered for some time at the start of each month. Basically would be broken until the app starts up again, which requires integrators to have some sort of wakeup scheduled to refresh keys if they aren't comfortable with extended breakages each month. That's going to be a bigger ask for integrators.

The way to fix this is to have each user pass in not just the current month's key but also the next N months keys for each topic. That's fine, but it makes things more complicated from the push server side. Need to handle expirations etc. Clients would also need to be aware of their last upload time and periodically refresh.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of discussion:

  1. We can pass (prev_hmac_key, current_hmac_key, next_hmac_key). That solves any roll over issues.
  2. We can expose an API that maps (list of topics to subscribe pushes for) -> (hmac keys for topics). Integrators simply need to call it any time the topic list changes, as well as periodically, and upload it if it changes. This only needs to happen every 30 days at most.

src/keystore/InMemoryKeystore.ts Outdated Show resolved Hide resolved
Copy link

cloudflare-workers-and-pages bot commented Jan 17, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 12895cb
Status: ✅  Deploy successful!
Preview URL: https://72b91495.xmtp-js.pages.dev
Branch Preview URL: https://rygine-sender-hmac.xmtp-js.pages.dev

View logs

@rygine rygine force-pushed the rygine/sender-hmac branch from 42f16a9 to b6b388f Compare January 18, 2024 15:52
src/crypto/encryption.ts Outdated Show resolved Hide resolved
src/crypto/encryption.ts Outdated Show resolved Hide resolved
@rygine rygine force-pushed the rygine/sender-hmac branch 2 times, most recently from 08384e3 to 86063f6 Compare January 19, 2024 23:02
@rygine rygine changed the title Add sender HMAC to MessageV2 Add senderHmac and shouldPush to MessageV2 Jan 20, 2024
Copy link

@Bren2010 Bren2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment on the content of the HMAC, otherwise this makes sense

@rygine rygine force-pushed the rygine/sender-hmac branch from 5320fa9 to 5c59492 Compare January 25, 2024 18:25
@rygine rygine force-pushed the rygine/sender-hmac branch from 5c59492 to 12895cb Compare January 25, 2024 19:15
@rygine rygine merged commit 06993ee into beta Jan 25, 2024
7 checks passed
@rygine rygine deleted the rygine/sender-hmac branch January 25, 2024 20:10
Copy link
Contributor

🎉 This PR is included in version 11.4.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants