-
Notifications
You must be signed in to change notification settings - Fork 5k
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
feat: notifications - user storage controller #23353
Changes from 1 commit
d505f1a
d3ab72b
c7eb286
dc5afda
7263dfe
59ce18c
877a219
2d3aec5
1e4c550
85ebd42
c9afbcd
79b3eb3
41bc10d
bcdd42e
0a38e01
727fc47
46bf656
1e6791c
22c6e13
c92fe51
1f1a942
a580941
9c4115d
dd8ab52
a3f0f83
9e7ace6
509bfd4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import { SnapId } from '@metamask/snaps-sdk'; | ||
import { HandlerType } from '@metamask/snaps-utils'; | ||
|
||
const snapId = 'npm:@metamask/message-signing-snap' as SnapId; | ||
|
||
export function createSnapSignMessageRequest(message: `metamask:${string}`) { | ||
return { | ||
snapId, | ||
origin: '', | ||
handler: HandlerType.OnRpcRequest, | ||
request: { | ||
jsonrpc: '2.0', | ||
method: 'signMessage', | ||
params: { message }, | ||
id: 1, | ||
}, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
import sjcl from 'sjcl'; | ||
Prithpal-Sooriya marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Encryption / Decryption taken from - https://github.com/luke-park/SecureCompatibleEncryptionExamples/blob/master/TypeScript/SCEE-sjcl.ts | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's my full analysis I shared internally on Tuesday for the record Threats to RAMSRAMS: https://en.wikipedia.org/wiki/Reliability,_availability,_maintainability_and_safety Reliability
Recommendation:Introduce protections against corrupting the payload. Availability
MaintainabilityIf we end up needing to fix/replace/alter the encryption or derivation code, the payload saved as UserStorage will become unusable. RecommendationA migration procedure in case encryption or derivation implementation needs to change needs to be part of the design from the start. Safety
1.1. It demands a lot of unnecessary powers because of how it generates entropy, so whenever it's compromised, it can be used not only to weaken our encryption, but also for other exploits, with the powers it is granted in policy
One problem is that compatibility tables say PBKDF2 is unsupported in Safari and that might impact MM-mobile unless an implementation is provided by a good polyfill.
Browsing open issues doesn't build confidence either. Recommendation
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TY for the RAMS analysis. We have replaced You can see additional tickets we've created on this here. Will ensure that we cover all things mentioned in this doc. |
||
class EncryptorDecryptor { | ||
#BITS_PER_WORD = 32; | ||
Prithpal-Sooriya marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#ALGORITHM_NONCE_SIZE = 3; // 32-bit words. | ||
|
||
#ALGORITHM_KEY_SIZE = 4; // 32-bit words. | ||
|
||
#PBKDF2_SALT_SIZE = 4; // 32-bit words. | ||
|
||
#PBKDF2_ITERATIONS = 32767; | ||
Prithpal-Sooriya marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
encryptString = (plaintext: string, password: string): string => { | ||
// Generate a 128-bit salt using a CSPRNG. | ||
const salt = sjcl.random.randomWords(this.#PBKDF2_SALT_SIZE); | ||
|
||
// Derive a key using PBKDF2. | ||
const key = sjcl.misc.pbkdf2( | ||
password, | ||
salt, | ||
this.#PBKDF2_ITERATIONS, | ||
this.#ALGORITHM_KEY_SIZE * this.#BITS_PER_WORD, | ||
); | ||
|
||
// Encrypt and prepend salt. | ||
const plaintextRaw = sjcl.codec.utf8String.toBits(plaintext); | ||
const ciphertextAndNonceAndSalt = sjcl.bitArray.concat( | ||
salt, | ||
this.#encrypt(plaintextRaw, key), | ||
); | ||
|
||
return sjcl.codec.base64.fromBits(ciphertextAndNonceAndSalt); | ||
}; | ||
|
||
decryptString = ( | ||
base64CiphertextAndNonceAndSalt: string, | ||
password: string, | ||
): string => { | ||
// Decode the base64. | ||
const ciphertextAndNonceAndSalt = sjcl.codec.base64.toBits( | ||
base64CiphertextAndNonceAndSalt, | ||
); | ||
|
||
// Create buffers of salt and ciphertextAndNonce. | ||
const salt = sjcl.bitArray.bitSlice( | ||
ciphertextAndNonceAndSalt, | ||
0, | ||
this.#PBKDF2_SALT_SIZE * this.#BITS_PER_WORD, | ||
); | ||
|
||
const ciphertextAndNonce = sjcl.bitArray.bitSlice( | ||
ciphertextAndNonceAndSalt, | ||
this.#PBKDF2_SALT_SIZE * this.#BITS_PER_WORD, | ||
ciphertextAndNonceAndSalt.length * this.#BITS_PER_WORD, | ||
); | ||
|
||
// Derive the key using PBKDF2. | ||
const key = sjcl.misc.pbkdf2( | ||
password, | ||
salt, | ||
this.#PBKDF2_ITERATIONS, | ||
this.#ALGORITHM_KEY_SIZE * this.#BITS_PER_WORD, | ||
); | ||
|
||
// Decrypt and return result. | ||
return sjcl.codec.utf8String.fromBits( | ||
this.#decrypt(ciphertextAndNonce, key), | ||
); | ||
}; | ||
|
||
#encrypt = (plaintext: sjcl.BitArray, key: number[]): sjcl.BitArray => { | ||
// Generate a 96-bit nonce using a CSPRNG. | ||
const nonce = sjcl.random.randomWords(this.#ALGORITHM_NONCE_SIZE); | ||
|
||
// Encrypt and prepend nonce. | ||
const ciphertext = sjcl.mode.gcm.encrypt( | ||
// eslint-disable-next-line new-cap | ||
new sjcl.cipher.aes(key), | ||
plaintext, | ||
nonce, | ||
); | ||
|
||
return sjcl.bitArray.concat(nonce, ciphertext); | ||
}; | ||
|
||
#decrypt = ( | ||
ciphertextAndNonce: sjcl.BitArray, | ||
key: number[], | ||
): sjcl.BitArray => { | ||
// Create buffers of nonce and ciphertext. | ||
const nonce = sjcl.bitArray.bitSlice( | ||
ciphertextAndNonce, | ||
0, | ||
this.#ALGORITHM_NONCE_SIZE * this.#BITS_PER_WORD, | ||
); | ||
|
||
const ciphertext = sjcl.bitArray.bitSlice( | ||
ciphertextAndNonce, | ||
this.#ALGORITHM_NONCE_SIZE * this.#BITS_PER_WORD, | ||
ciphertextAndNonce.length * this.#BITS_PER_WORD, | ||
); | ||
|
||
// Decrypt and return result. | ||
// eslint-disable-next-line new-cap | ||
return sjcl.mode.gcm.decrypt(new sjcl.cipher.aes(key), ciphertext, nonce); | ||
}; | ||
} | ||
|
||
const encryption = new EncryptorDecryptor(); | ||
export default encryption; | ||
|
||
export function createSHA256Hash(data: string): string { | ||
const hashedData = sjcl.hash.sha256.hash(data); | ||
return sjcl.codec.hex.fromBits(hashedData); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import nock from 'nock'; | ||
import { USER_STORAGE_ENDPOINT, GetUserStorageResponse } from '../services'; | ||
import { createEntryPath } from '../schema'; | ||
import { MOCK_ENCRYPTED_STORAGE_DATA, MOCK_STORAGE_KEY } from './mockStorage'; | ||
|
||
export const MOCK_USER_STORAGE_NOTIFICATIONS_ENDPOINT = `${USER_STORAGE_ENDPOINT}${createEntryPath( | ||
'notification_settings', | ||
MOCK_STORAGE_KEY, | ||
)}`; | ||
|
||
type MockReply = { | ||
status: number; | ||
body?: Record<string, unknown>; | ||
}; | ||
|
||
const MOCK_GET_USER_STORAGE_RESPONSE: GetUserStorageResponse = { | ||
HashedKey: 'HASHED_KEY', | ||
Data: MOCK_ENCRYPTED_STORAGE_DATA, | ||
}; | ||
export function mockEndpointGetUserStorage(mockReply?: MockReply) { | ||
const reply = mockReply ?? { | ||
status: 200, | ||
body: MOCK_GET_USER_STORAGE_RESPONSE, | ||
}; | ||
|
||
const mockEndpoint = nock(MOCK_USER_STORAGE_NOTIFICATIONS_ENDPOINT) | ||
.get('') | ||
.reply(reply.status, reply.body); | ||
|
||
return mockEndpoint; | ||
} | ||
|
||
export function mockEndpointUpsertUserStorage( | ||
mockReply?: Pick<MockReply, 'status'>, | ||
) { | ||
const mockEndpoint = nock(MOCK_USER_STORAGE_NOTIFICATIONS_ENDPOINT) | ||
.put('') | ||
.reply(mockReply?.status ?? 204); | ||
return mockEndpoint; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import encryption, { createSHA256Hash } from '../encryption'; | ||
|
||
export const MOCK_STORAGE_KEY_SIGNATURE = 'mockStorageKey'; | ||
export const MOCK_STORAGE_KEY = createSHA256Hash(MOCK_STORAGE_KEY_SIGNATURE); | ||
export const MOCK_STORAGE_DATA = JSON.stringify({ hello: 'world' }); | ||
export const MOCK_ENCRYPTED_STORAGE_DATA = encryption.encryptString( | ||
MOCK_STORAGE_DATA, | ||
MOCK_STORAGE_KEY, | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
import { createSHA256Hash } from './encryption'; | ||
|
||
type UserStorageEntry = { path: string; entryName: string }; | ||
|
||
/** | ||
* The User Storage Endpoint requires a path and an entry name. | ||
* Developers can provide additional paths by extending this variable below | ||
*/ | ||
export const USER_STORAGE_ENTRIES = { | ||
notification_settings: { | ||
path: 'notifications', | ||
entryName: 'notification_settings', | ||
}, | ||
} satisfies Record<string, UserStorageEntry>; | ||
|
||
export type UserStorageEntryKeys = keyof typeof USER_STORAGE_ENTRIES; | ||
|
||
/** | ||
* Constructs a unique entry path for a user. | ||
* This can be done due to the uniqueness of the storage key (no users will share the same storage key). | ||
* The users entry is a unique hash that cannot be reversed. | ||
* | ||
* @param entryKey | ||
* @param storageKey | ||
* @returns | ||
*/ | ||
export function createEntryPath( | ||
entryKey: UserStorageEntryKeys, | ||
storageKey: string, | ||
): string { | ||
const entry = USER_STORAGE_ENTRIES[entryKey]; | ||
if (!entry) { | ||
throw new Error(`user-storage - invalid entry provided: ${entryKey}`); | ||
} | ||
|
||
const hashedKey = createSHA256Hash(entry.entryName + storageKey); | ||
return `/${entry.path}/${hashedKey}`; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
import { | ||
mockEndpointGetUserStorage, | ||
mockEndpointUpsertUserStorage, | ||
} from './mocks/mockServices'; | ||
import { | ||
MOCK_ENCRYPTED_STORAGE_DATA, | ||
MOCK_STORAGE_DATA, | ||
MOCK_STORAGE_KEY, | ||
} from './mocks/mockStorage'; | ||
import { | ||
GetUserStorageResponse, | ||
getUserStorage, | ||
upsertUserStorage, | ||
} from './services'; | ||
|
||
describe('user-storage/services.ts - getUserStorage() tests', () => { | ||
test('returns user storage data', async () => { | ||
const mockGetUserStorage = mockEndpointGetUserStorage(); | ||
const result = await actCallGetUserStorage(); | ||
|
||
mockGetUserStorage.done(); | ||
expect(result).toBe(MOCK_STORAGE_DATA); | ||
}); | ||
|
||
test('returns null if endpoint does not have entry', async () => { | ||
const mockGetUserStorage = mockEndpointGetUserStorage({ status: 404 }); | ||
const result = await actCallGetUserStorage(); | ||
|
||
mockGetUserStorage.done(); | ||
expect(result).toBe(null); | ||
}); | ||
|
||
test('returns null if endpoint fails', async () => { | ||
const mockGetUserStorage = mockEndpointGetUserStorage({ status: 500 }); | ||
const result = await actCallGetUserStorage(); | ||
|
||
mockGetUserStorage.done(); | ||
expect(result).toBe(null); | ||
}); | ||
|
||
test('returns null if unable to decrypt data', async () => { | ||
const badResponseData: GetUserStorageResponse = { | ||
HashedKey: 'MOCK_HASH', | ||
Data: 'Bad Encrypted Data', | ||
}; | ||
const mockGetUserStorage = mockEndpointGetUserStorage({ | ||
status: 200, | ||
body: badResponseData, | ||
}); | ||
const result = await actCallGetUserStorage(); | ||
|
||
mockGetUserStorage.done(); | ||
expect(result).toBe(null); | ||
}); | ||
|
||
function actCallGetUserStorage() { | ||
return getUserStorage({ | ||
bearerToken: 'MOCK_BEARER_TOKEN', | ||
|
||
entryKey: 'notification_settings', | ||
storageKey: MOCK_STORAGE_KEY, | ||
}); | ||
} | ||
}); | ||
|
||
describe('user-storage/services.ts - upsertUserStorage() tests', () => { | ||
test('invokes upsert endpoint with no errors', async () => { | ||
const mockUpsertUserStorage = mockEndpointUpsertUserStorage(); | ||
await actCallUpsertUserStorage(); | ||
|
||
expect(mockUpsertUserStorage.isDone()).toBe(true); | ||
}); | ||
|
||
test('throws error if unable to upsert user storage', async () => { | ||
const mockUpsertUserStorage = mockEndpointUpsertUserStorage({ | ||
status: 500, | ||
}); | ||
|
||
await expect(actCallUpsertUserStorage()).rejects.toThrow(); | ||
mockUpsertUserStorage.done(); | ||
}); | ||
|
||
function actCallUpsertUserStorage() { | ||
return upsertUserStorage(MOCK_ENCRYPTED_STORAGE_DATA, { | ||
bearerToken: 'MOCK_BEARER_TOKEN', | ||
|
||
entryKey: 'notification_settings', | ||
storageKey: MOCK_STORAGE_KEY, | ||
}); | ||
} | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
import encryption from './encryption'; | ||
import { UserStorageEntryKeys, createEntryPath } from './schema'; | ||
|
||
export const USER_STORAGE_API = process.env.USER_STORAGE_API || ''; | ||
export const USER_STORAGE_ENDPOINT = `${USER_STORAGE_API}/api/v1/userstorage`; | ||
|
||
export type GetUserStorageResponse = { | ||
HashedKey: string; | ||
Data: string; | ||
}; | ||
|
||
export type UserStorageOptions = { | ||
bearerToken: string; | ||
entryKey: UserStorageEntryKeys; | ||
storageKey: string; | ||
}; | ||
|
||
export async function getUserStorage( | ||
opts: UserStorageOptions, | ||
): Promise<string | null> { | ||
const path = createEntryPath(opts.entryKey, opts.storageKey); | ||
const url = new URL(`${USER_STORAGE_ENDPOINT}${path}`); | ||
|
||
const encryptedData = await fetch(url.toString(), { | ||
headers: { | ||
'Content-Type': 'application/json', | ||
Authorization: `Bearer ${opts.bearerToken}`, | ||
}, | ||
}) | ||
.then((r) => { | ||
if (r.status === 404) { | ||
// Acceptable Error | ||
return null; | ||
} | ||
if (r.status !== 200) { | ||
throw new Error('Unable to get User Storage'); | ||
} | ||
return r.json(); | ||
}) | ||
.then((r: GetUserStorageResponse | null) => r?.Data) | ||
.catch(() => null); | ||
|
||
if (!encryptedData) { | ||
return null; | ||
} | ||
|
||
try { | ||
const decryptedData = encryption.decryptString( | ||
encryptedData, | ||
opts.storageKey, | ||
); | ||
return decryptedData; | ||
} catch { | ||
return null; | ||
} | ||
} | ||
|
||
Prithpal-Sooriya marked this conversation as resolved.
Show resolved
Hide resolved
|
||
export async function upsertUserStorage( | ||
data: string, | ||
opts: UserStorageOptions, | ||
): Promise<void> { | ||
const encryptedData = encryption.encryptString(data, opts.storageKey); | ||
const path = createEntryPath(opts.entryKey, opts.storageKey); | ||
const url = new URL(`${USER_STORAGE_ENDPOINT}${path}`); | ||
|
||
const res = await fetch(url.toString(), { | ||
method: 'PUT', | ||
headers: { | ||
'Content-Type': 'application/json', | ||
Authorization: `Bearer ${opts.bearerToken}`, | ||
}, | ||
body: JSON.stringify({ data: encryptedData }), | ||
}); | ||
|
||
if (!res.ok) { | ||
throw new Error('user-storage - unable to upsert data'); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need a wider discussion on this.
Context
All MetaMask Clients (mobile, extension, portfolio) that want to use UserStorage must all use the same encryption/decryption mechanism.
Any data you store must be encrypted, and any data you retrieve must be decrypted.
This is what was approved on Portfolio - basic symmetric encryption using the storage key as the password.
Discussion
Can we also use this for extension? What are the risks? If there are other encryption mechanisms that we need, we will need it to be compatible for all metamask clients and also need to port Portfolio to use the same encryption mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the storage key a symmetric key?
Also, we should rather use a KDF instead of a PBKDF if we are already working with cryptographic material.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep the storage key is symmetric. And yep flexible to change to KDF.
TBH if there is a need to change to asymmetric encryption/decryption I think we can - I'll note this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a usecase for assymetric cryptography here. Care to elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - I don't see a need for asymmetric encryption - was open to it if it is recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks all! Closing this discussion.
As discussed we have a Threat Modelling Document here
Also tickets here