TA-4530: Implement secure secret storage #301
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| **Best Practices:** | ||
| - Ensure your system keychain is properly configured and accessible | ||
| - If you see warnings about plaintext storage, consider re-creating the profile to enable secure storage |
There was a problem hiding this comment.
I see you mentioned a migration solution being intentionally skipped in the PR description, but I think it would be very straightforward to enable users on it without re-create. Something like content-cli profile secure <profile> and re-use logic the already implemented logic from this PR.
| @@ -1,3 +1,3 @@ | |||
| { | |||
| "name": "@celonis/content-cli", | |||
| "version": "1.4.1", | |||
There was a problem hiding this comment.
Changes probably deserve a major release
There was a problem hiding this comment.
Let's decide that after merging and testing.
| } | ||
| } | ||
|
|
||
| export const secureSecretStorageService = new SecureSecretStorageService(); No newline at end of file |
There was a problem hiding this comment.
This diverges from the Content CLI structure of services. Can we change to an injectable class?
|
|
||
| if (!secretsStoredInKeychain) { | ||
| logger.warn("⚠️ Failed to store secrets securely. They will be stored in plain text file."); | ||
| return false; |
There was a problem hiding this comment.
Not sure if this is the best approach. Maybe we should just give a message that storing failed, and then propose to the non-recommended storage, which can be an option in the command. What do you think?
There was a problem hiding this comment.
That would be a breaking change and can affect automated scripts that use content-cli. The implemented approach graciously fails and falls back to the insecure secret storage, and is a pattern that is commonly used by cli tools - especially those using keytar.
|
Description
Implemented secure secret storage using keytar lib with fallback to the current plain text storage for cases where storing the secrets in the system keychain fails or is not available (eg. on linux due to libsecret dependency).
A warning will be shown to users when creating or using a profile with insecure secret storage.
Solutions for migrating existing profile secrets automatically or providing a helper command will be considered separately.
Relevant links
Checklist
Note
Introduces secure storage for profile secrets with automatic use of the system keychain and plaintext fallback when unavailable.
SecureSecretStorageServiceusingkeytarto save/retrieveapiToken,clientSecret, andrefreshTokenundercelonis-content-cli:<profile-name>Profilemodel updated (ProfileSecrets,secretsStoredSecurelyflag);storeProfilenow async, strips secrets when saved securely;findProfileloads secrets from keychain when flaggedProfileCommandService/ProfileServiceupdated to awaitstoreProfileand persist refreshed tokens securelypackage.jsonaddskeytar; extensive unit tests added for secure storage, fallback, and keytar-unavailable scenarios; lockfile updatedWritten by Cursor Bugbot for commit 9c60ea8. This will update automatically on new commits. Configure here.