Skip to content

feat: Insomnia vault key management UI[INS-4715]#8296

Merged
cwangsmv merged 5 commits into
developfrom
feat/insomnai-vault-setting-ui
Feb 17, 2025
Merged

feat: Insomnia vault key management UI[INS-4715]#8296
cwangsmv merged 5 commits into
developfrom
feat/insomnai-vault-setting-ui

Conversation

@cwangsmv

Copy link
Copy Markdown
Contributor

Changes:
As part of the Insomnia vault feature, this PR includes the UI changes for vault key management.
The vault key is used to encrypt local secret environment variables.

Add a new UI in Preferences page for vault key generation:
Screenshot 2025-01-15 at 10 18 10

When user has logged on other devices/re-login, user needs to enter the vault key for validation. We also provide reset feature if the vault key is lost.
Screenshot 2025-01-15 at 10 18 17

There will be two new options related to vault key:
User could choose to save the encrypted vault key locally to avoid entering vault key after re-login. The vault key is encrypted using OS native secret manager(like KeyChain in MacOS).
There's another option to choose whether to allow pre-request/post-response scripts to access secret environment variables
Screenshot 2025-01-15 at 10 18 54

@cwangsmv
cwangsmv requested review from ihexxa and jackkav January 15, 2025 02:35
ihexxa
ihexxa previously approved these changes Jan 15, 2025

@ihexxa ihexxa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall lgtm from my side as I've reviewed this part several rounds in #8233

Comment thread packages/insomnia/src/main/window-utils.ts Outdated
Comment thread packages/insomnia/src/ui/components/settings/vault-key-panel.tsx Outdated
"@fortawesome/free-solid-svg-icons": "^6.5.2",
"@fortawesome/react-fontawesome": "^0.2.0",
"@getinsomnia/api-client": "0.0.4",
"@getinsomnia/srp-js": "1.0.0-alpha.1",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as this was removed from the project over a year ago, we should discuss the need for this further with @gatzjames

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Introduce this lib back is for key generation and validation using SRP protocol. I've checked that website is also using this for e2ee with same purpose.

@jackkav

jackkav commented Jan 20, 2025

Copy link
Copy Markdown
Contributor

Nice job so far.

I'm curious about the choice to use the localstorage.ts mechanism for persistence, its a little confusing because its not real localstorage its a clone of the api that writes to file. Have we considered alternative persistence stores?
Would regular window.localStorage suffice?
What persistence story are we looking for?

There's more diving I can do into the UI components but I want to get a better high level view first.

Are there any concerns or ideas have you considered but chosen to scope out of this PR?

Comment on lines +25 to +62
const getLocalStorage = () => {
if (!localStorage) {
localStorage = initLocalStorage();
}
return localStorage;
};

const setSecret = async (key: string, secret: string) => {
try {
const secretStorage = getLocalStorage();
const encrypted = encryptString(secret);
secretStorage.setItem(key, encrypted);
} catch (error) {
console.error(`Can not save secret ${error.toString()}`);
return Promise.reject(error);
}
};

const getSecret = async (key: string) => {
try {
const secretStorage = getLocalStorage();
const encrypted = secretStorage.getItem(key, '');
return encrypted === '' ? null : decryptString(encrypted);
} catch (error) {
console.error(`Can not get secret ${error.toString()}`);
return Promise.reject(null);
}
};

const deleteSecret = async (key: string) => {
try {
const secretStorage = getLocalStorage();
secretStorage.deleteItem(key);
} catch (error) {
console.error(`Can not delele secret ${error.toString()}`);
return Promise.reject(error);
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❤️

Comment thread packages/insomnia/src/ui/components/settings/vault-key-panel.tsx
@cwangsmv

cwangsmv commented Jan 21, 2025

Copy link
Copy Markdown
Contributor Author

Nice job so far.

I'm curious about the choice to use the localstorage.ts mechanism for persistence, its a little confusing because its not real localstorage its a clone of the api that writes to file. Have we considered alternative persistence stores? Would regular window.localStorage suffice? What persistence story are we looking for?

There's more diving I can do into the UI components but I want to get a better high level view first.

Are there any concerns or ideas have you considered but chosen to scope out of this PR?

This is a trade-off to replace keytar with Electron's safeStorage api.

The requirement is that we expose an option for user to save the encrypted vault key(symmetric key for encryption) locally. So whenever user logout and login again, the user do not need to manual input the vault key, we will decrypt and load the vault key automatically.
By using Electron's safeStorage api, we can leverage OS native secret manager for encryption, but it does not provide a storage API for persistence which keytar provided before.

The reason why I am not using window.localStorage is that Electron's safeStorage api is executed in main process. window.localStorage is not accessible in main process.
So I need a persistent file storage in main process to save the vault key.
By using localstorage.ts mechanism for persistence, I do not need to introduce extra logic/libs.

@jackkav

jackkav commented Jan 21, 2025

Copy link
Copy Markdown
Contributor

By using localstorage.ts mechanism for persistence, I do not need to introduce extra logic/libs.

Good points, I think my criticism is more about the naming of the existing localstorage.ts code, perhaps we can rename it to electronstorage.ts/ElectronStorage to clear this up. But no need to block this PR.

Just remove the input ref and copied behavior and I can approve.

@cwangsmv

cwangsmv commented Jan 22, 2025

Copy link
Copy Markdown
Contributor Author

By using localstorage.ts mechanism for persistence, I do not need to introduce extra logic/libs.

Good points, I think my criticism is more about the naming of the existing localstorage.ts code, perhaps we can rename it to electronstorage.ts/ElectronStorage to clear this up. But no need to block this PR.

Just remove the input ref and copied behavior and I can approve.

I agree, current naming of localstorage.ts is a bit confused with browser native localstorage, #8310 is created for the renaming task.
Also, the input ref has been removed.

jackkav
jackkav previously approved these changes Jan 27, 2025

@jackkav jackkav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

@cwangsmv
cwangsmv force-pushed the feat/insomnai-vault-setting-ui branch from 4db18ca to 76ed107 Compare February 17, 2025 06:25
@cwangsmv
cwangsmv enabled auto-merge (squash) February 17, 2025 06:29
@cwangsmv
cwangsmv merged commit eddeb47 into develop Feb 17, 2025
@cwangsmv
cwangsmv deleted the feat/insomnai-vault-setting-ui branch February 17, 2025 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants