-
Notifications
You must be signed in to change notification settings - Fork 0
TA-4530: Implement secure secret storage #301
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?
Changes from all commits
04f9677
1515972
baf40df
5541bf3
16402e1
0ff7751
0b9a93a
9c60ea8
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,101 @@ | ||
| import { Profile, ProfileSecrets } from "./profile.interface"; | ||
| import { logger } from "../utils/logger"; | ||
|
|
||
| enum PROFILE_SECRET_TYPE { | ||
| API_TOKEN = "apiToken", | ||
| CLIENT_SECRET = "clientSecret", | ||
| REFRESH_TOKEN = "refreshToken", | ||
| } | ||
|
|
||
| const CONTENT_CLI_SERVICE_NAME = "celonis-content-cli"; | ||
|
|
||
| // Lazy load keytar to handle cases where native dependencies are not available | ||
| let keytar: any = null; | ||
| let keytarLoadError: Error | null = null; | ||
|
|
||
| function getKeytar(): any { | ||
| if (keytar !== null) { | ||
| return keytar; | ||
| } | ||
| if (keytarLoadError !== null) { | ||
| return null; | ||
| } | ||
| try { | ||
| keytar = require("keytar"); | ||
| return keytar; | ||
| } catch (error) { | ||
| keytarLoadError = error as Error; | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| export class SecureSecretStorageService { | ||
|
|
||
| public async storeSecrets(profile: Profile): Promise<boolean> { | ||
| let secretsStoredInKeychain = true; | ||
| const secretEntries = [ | ||
| { type: PROFILE_SECRET_TYPE.API_TOKEN, value: profile.apiToken }, | ||
| { type: PROFILE_SECRET_TYPE.CLIENT_SECRET, value: profile.clientSecret }, | ||
| { type: PROFILE_SECRET_TYPE.REFRESH_TOKEN, value: profile.refreshToken }, | ||
| ]; | ||
|
|
||
| for (const secretEntry of secretEntries) { | ||
| if (secretEntry.value) { | ||
| const stored = await this.storeSecret( | ||
| this.getSecretServiceName(profile.name), | ||
| secretEntry.type, | ||
| secretEntry.value | ||
| ); | ||
| if (!stored) { | ||
| secretsStoredInKeychain = false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!secretsStoredInKeychain) { | ||
| logger.warn("⚠️ Failed to store secrets securely. They will be stored in plain text file."); | ||
| return false; | ||
LaberionAjvazi marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
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. 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?
Contributor
Author
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. 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. |
||
| } | ||
|
|
||
| return true; | ||
| } | ||
LaberionAjvazi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| public async getSecrets(profileName: string): Promise<ProfileSecrets | undefined> { | ||
| const keytarModule = getKeytar(); | ||
| if (!keytarModule) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const secrets = await keytarModule.findCredentials(this.getSecretServiceName(profileName)); | ||
|
|
||
| if (!secrets.length) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const profileSecrets = {}; | ||
|
|
||
| for (const secret of secrets) { | ||
| profileSecrets[secret.account] = secret.password | ||
| } | ||
|
|
||
| return profileSecrets as ProfileSecrets; | ||
LaberionAjvazi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| private getSecretServiceName(profileName: string): string { | ||
| return `${CONTENT_CLI_SERVICE_NAME}:${profileName}`; | ||
| } | ||
|
|
||
| private async storeSecret(service: string, account: string, secret: string): Promise<boolean> { | ||
| const keytarModule = getKeytar(); | ||
| if (!keytarModule) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| await keytarModule.setPassword(service, account, secret); | ||
| return true; | ||
| } catch (err) { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
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 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.