-
Notifications
You must be signed in to change notification settings - Fork 295
feat: support sqlite credential helper #857
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
Conversation
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
pkg/config/cliconfig.go
Outdated
Wincred = "wincred" | ||
Osxkeychain = "osxkeychain" | ||
Secretservice = "secretservice" | ||
Pass = "pass" | ||
File = "file" | ||
Sqlite = "sqlite" |
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 appending CredHelper
to each of these would give them a more descriptive name.
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.
Fixed
pkg/config/cliconfig.go
Outdated
@@ -150,11 +159,11 @@ func ReadCLIConfig(gptscriptConfigFile string) (*CLIConfig, error) { | |||
errMsg := fmt.Sprintf("invalid credential store '%s'", result.CredentialsStore) | |||
switch runtime.GOOS { | |||
case "darwin": | |||
errMsg += " (use 'osxkeychain' or 'file')" | |||
errMsg += " (use 'osxkeychain', 'file', or 'sqlite')" |
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.
These probably won't change much, but you could use strings.Join(darwinHelpers, ", ")
here for simplicity.
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.
Fixed
pkg/credentials/store.go
Outdated
@@ -178,7 +178,7 @@ func (s *Store) getStore(ctx context.Context) (credentials.Store, error) { | |||
} | |||
|
|||
func (s *Store) getStoreByHelper(ctx context.Context, helper string) (credentials.Store, error) { | |||
if helper == "" || helper == config.GPTScriptHelperPrefix+"file" { | |||
if helper == "" || helper == config.GPTScriptHelperPrefix+config.File { |
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 solidifies my desire to change the names of those constants: config.File
is a confusing name.
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.
👍
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
func RepoNameForCredentialStore(store string) string { | ||
switch store { | ||
case config.SqliteCredHelper: | ||
return "gptscript-credential-sqlite" |
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.
Nit: gptscript-credential-sqlite
and gptscript-credential-helpers
shows up multiple and might be a good idea to define those as const.
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 might change this at a later point. Don't want to go through the process of getting approvals on this one again just for that change
This adds support for the new https://github.com/gptscript-ai/gptscript-credential-sqlite credential helper. It is for macOS and Linux. I will add some documentation about how to use it to its repo. By default, secrets will be unencrypted in the database, but we support using k8s EncryptionConfiguration files, which can be used to set up AES encryption, or something more sophisticated with a KMS like AWS KMS.