-
Notifications
You must be signed in to change notification settings - Fork 32
[PB-4610] Opaque PoC #1690
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
base: master
Are you sure you want to change the base?
[PB-4610] Opaque PoC #1690
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Deploying drive-web with
|
| Latest commit: |
435b604
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c7c06882.drive-web.pages.dev |
| Branch Preview URL: | https://opaque-poc.drive-web.pages.dev |
| import { AuthMethodTypes } from 'app/payment/types'; | ||
| import vpnAuthService from 'app/auth/services/vpnAuth.service'; | ||
| import envService from 'app/core/services/env.service'; | ||
| import { logInOpaque, is2FAorOpaqueNeeded } from '../../services/auth.opaque'; |
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.
This should be in the auth service, let's not create 'helper' files
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.
Done
|
|
||
| try { | ||
| const isTfaEnabled = await is2FANeeded(email); | ||
| const { tfaEnabled: isTfaEnabled, opaqueLogin } = await is2FAorOpaqueNeeded(email); |
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.
Could we hide these things under the same functions? Otherwise we mix component-related logic with authentication low-level details, which is far from ideal in terms of maintenance and testability. Let's hide these things under the auth layer.
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.
Done
| }; | ||
|
|
||
| const { token, user, mnemonic } = await authenticateUser(authParams); | ||
| const { token, user, mnemonic } = await (opaqueLogin ? logInOpaque(authParams) : authenticateUser(authParams)); |
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.
Same here
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.
Done
| import { generateUserSecrets, encryptUserKeysAndMnemonic, decryptUserKeysAndMnemonic } from './auth.crypto'; | ||
|
|
||
| describe('Test auth crypto functions', () => { | ||
| it('test enc/dec of user sercrets', async () => { |
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.
Use When-then legends and define clearly what each case is doing / testing
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.
Done
|
|
||
| const { encMnemonic, encKeys } = await encryptUserKeysAndMnemonic(keys, mnemonic, exportKey); | ||
|
|
||
| const { keys: dec_keys, mnemonic: dec_mnemonic } = await decryptUserKeysAndMnemonic( |
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.
Avoid using snake_case, the convention is camelCase
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.
Done
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.
When the PRs related to the SDK are merged, I will check this one again :)
internxt/sdk#335
larryrider
left a comment
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.
check the build action, it seems to be failing right now
|
@larryrider, hash library is not working with node less than 20, so tests on 18 fail due to failed build |
@TamaraFinogina We should then update those actions to use node 20 or 22 |
src/services/auth.opaque.ts
Outdated
| password: string, | ||
| twoFactorCode: string, | ||
| ): Promise<{ token: string; user: UserSettings; mnemonic: string; newToken: string }> => { | ||
| const { sessionID, user: encUser, sessionKey, exportKey } = await loginOpaque(email, password, twoFactorCode); |
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.
Not sure if calling that encUser is the best option since since the user isn't actually encrypted. What about loggedUser or smth like that?
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.
Done
src/app/auth/services/auth.opaque.ts
Outdated
| localStorageService.set('xNewToken', sessionID); | ||
| await setSessionKey(password, sessionKey); | ||
|
|
||
| Sentry.setUser({ |
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 Sentry can be safely removed. We do not use it anymore.
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.
Done
src/services/auth.opaque.ts
Outdated
| localStorageService.set('sessionKey', sessionKeyEnc); | ||
| localStorageService.set('sessionKeySalt', salt); | ||
| }; | ||
|
|
||
| export const getSessionKey = async (password: string): Promise<Uint8Array> => { | ||
| const sessionKeyEnc = localStorageService.get('sessionKey') || ''; | ||
| const salt = localStorageService.get('sessionKeySalt') || ''; |
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.
Perhaps you could create a function in localStorageService called setSessionKey and getSessionKey and manage there the keys/values instead? WDYT?
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.
Done
src/services/auth.opaque.ts
Outdated
| password, | ||
| }); | ||
| if (!loginResult) { | ||
| throw new Error('Login failed'); |
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.
Suggestion: Consider creating a custom error (e.g., LoginFailedError) so it’s clear that the error originates from our code. This makes debugging easier. You can use something like this as a reference.
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.
Done
|





Description
This PR is for the PoC for opaque login (just to agree on the API).
During login, we call
is2FAorOpaqueNeeded, which tells us if login should be with Opaque or not. The draft implementation for opaque auth is insrc/app/auth/services/auth.opaque.ts.Related Issues
Proof of Concept for PB-4610
Related Pull Requests
Checklist
Testing Process
No tests are added yet.
Additional Notes
To build, you need to link the SDK from the opaque PoC PR.