-
Notifications
You must be signed in to change notification settings - Fork 14
Add persisted credentials for CLI client #12
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: main
Are you sure you want to change the base?
Changes from all commits
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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -97,12 +97,54 @@ func validateFlags(cmd *cobra.Command, args []string) error { | |||||||
| func runE(cmd *cobra.Command, _ []string) error { | ||||||||
| configFile, _ := cmd.Flags().GetString("config") | ||||||||
|
|
||||||||
| url, err := tui.StringPrompt("enter permify url", "", config.CliConfig.PermifyURL) | ||||||||
| storedCreds, err := client.LoadCredentials() | ||||||||
| if err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
|
|
||||||||
| defaultURL := config.CliConfig.PermifyURL | ||||||||
| if defaultURL == "" { | ||||||||
| defaultURL = storedCreds.Endpoint | ||||||||
| } | ||||||||
|
|
||||||||
| url, err := tui.StringPrompt("enter permify url", "", defaultURL) | ||||||||
| if err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
|
|
||||||||
| token, err := tui.StringPrompt("enter api token (optional)", "", storedCreds.Token) | ||||||||
| if err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
|
|
||||||||
| certPath, err := tui.StringPrompt("enter client certificate path (optional)", "", storedCreds.CertPath) | ||||||||
| if err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
|
|
||||||||
| certKey, err := tui.StringPrompt("enter client certificate key path (optional)", "", storedCreds.CertKey) | ||||||||
| if err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
|
|
||||||||
| if (certPath == "") != (certKey == "") { | ||||||||
| return fmt.Errorf("both certificate path and key must be provided or omitted") | ||||||||
| } | ||||||||
|
|
||||||||
|
Comment on lines
+130
to
+133
|
||||||||
| if (certPath == "") != (certKey == "") { | |
| return fmt.Errorf("both certificate path and key must be provided or omitted") | |
| } |
Copilot
AI
Jan 6, 2026
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.
Stored credentials that fail to save only log an error but don't stop execution. This could lead to users thinking their credentials were saved when they weren't. Consider returning the error to the user, or at minimum, displaying a clear warning message that credentials were not persisted.
| logger.Log.Error(storeErr) | |
| logger.Log.Error(storeErr) | |
| return fmt.Errorf("failed to store credentials: %w", storeErr) |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,74 @@ | ||||||||||||
| package client | ||||||||||||
|
|
||||||||||||
| import ( | ||||||||||||
| "errors" | ||||||||||||
| "fmt" | ||||||||||||
| "io/fs" | ||||||||||||
| "os" | ||||||||||||
| "path/filepath" | ||||||||||||
|
|
||||||||||||
| "gopkg.in/yaml.v3" | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| // Credentials represents persisted client configuration for the Permify CLI. | ||||||||||||
| type Credentials struct { | ||||||||||||
| Endpoint string `yaml:"endpoint"` | ||||||||||||
| Token string `yaml:"token"` | ||||||||||||
| CertPath string `yaml:"cert_path"` | ||||||||||||
| CertKey string `yaml:"cert_key"` | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // credentialsFilePath resolves the OS-specific location for the credentials file. | ||||||||||||
| func credentialsFilePath() (string, error) { | ||||||||||||
| home, err := os.UserHomeDir() | ||||||||||||
| if err != nil { | ||||||||||||
| return "", fmt.Errorf("resolve user home directory: %w", err) | ||||||||||||
| } | ||||||||||||
| return filepath.Join(home, ".permify", "credentials"), nil | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // LoadCredentials fetches stored credentials from disk, returning an empty struct when none exist. | ||||||||||||
| func LoadCredentials() (Credentials, error) { | ||||||||||||
| file, err := credentialsFilePath() | ||||||||||||
| if err != nil { | ||||||||||||
| return Credentials{}, err | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| data, err := os.ReadFile(file) | ||||||||||||
| if errors.Is(err, fs.ErrNotExist) { | ||||||||||||
| return Credentials{}, nil | ||||||||||||
| } | ||||||||||||
| if err != nil { | ||||||||||||
| return Credentials{}, fmt.Errorf("read credentials file %s: %w", file, err) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| var creds Credentials | ||||||||||||
| if err := yaml.Unmarshal(data, &creds); err != nil { | ||||||||||||
| return Credentials{}, fmt.Errorf("parse credentials file %s: %w", file, err) | ||||||||||||
|
||||||||||||
| return Credentials{}, fmt.Errorf("parse credentials file %s: %w", file, err) | |
| // Treat corrupted credentials as if no credentials are stored, so callers | |
| // (e.g. client constructors with explicit endpoint params) are not blocked. | |
| return Credentials{}, nil |
Copilot
AI
Jan 6, 2026
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.
The comment states "user-only permissions" but doesn't document the specific permission values (0700 for directory, 0600 for file) or explain the security rationale. Consider adding a brief note about why these restrictive permissions are necessary for credential storage.
| // StoreCredentials writes the provided credentials to disk with user-only permissions. | |
| // StoreCredentials writes the provided credentials to disk with user-only permissions: | |
| // the credentials directory is created with 0700 and the file with 0600. These restrictive | |
| // modes ensure that only the current user can read or modify the stored credentials, preventing | |
| // other local users from accessing sensitive endpoint tokens or certificates. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,19 +2,91 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "crypto/tls" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "errors" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| permify "github.com/Permify/permify-go/v1" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "google.golang.org/grpc" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "google.golang.org/grpc/credentials" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "google.golang.org/grpc/credentials/insecure" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // New initializes a new permify client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // New initializes a new permify client with optional persisted credentials. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func New(endpoint string) (*permify.Client, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| storedCreds, err := LoadCredentials() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, err | |
| // If no endpoint is provided and credentials cannot be loaded, fail. | |
| if endpoint == "" { | |
| return nil, err | |
| } | |
| // If an endpoint is explicitly provided, proceed with empty credentials. | |
| storedCreds = Credentials{} |
Copilot
AI
Jan 6, 2026
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.
The error message hardcodes the path as "~/.permify/credentials", but on Windows this path might not be accurate. Consider making the error message more generic or dynamically including the actual resolved path.
| return nil, errors.New("permify endpoint missing: provide an endpoint or store one in ~/.permify/credentials") | |
| return nil, errors.New("permify endpoint missing: provide an endpoint or configure stored credentials") |
Copilot
AI
Jan 6, 2026
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.
The logic determines TLS usage based on endpoint prefix OR certificate presence. However, if a user provides certificate paths but uses an insecure endpoint (e.g., "grpc://"), the certificates will be loaded but might not be properly utilized due to conflicting security settings. Consider validating this combination or clarifying the precedence.
| // Validate conflicting configuration: insecure endpoint scheme with TLS certificates. | |
| lowerEndpoint := strings.ToLower(finalEndpoint) | |
| if (strings.HasPrefix(lowerEndpoint, "grpc://") || strings.HasPrefix(lowerEndpoint, "http://")) && | |
| (storedCreds.CertPath != "" || storedCreds.CertKey != "") { | |
| return nil, errors.New("insecure endpoint scheme (grpc:// or http://) cannot be used with TLS certificates; use grpcs:// or https:// or remove certificate configuration") | |
| } |
Copilot
AI
Jan 6, 2026
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.
The options slice is initialized as an empty slice using []grpc.DialOption{}, but this can be simplified to var options []grpc.DialOption or just []grpc.DialOption{} without the type annotation, as it's redundant given the function's return type.
| options := []grpc.DialOption{} | |
| var options []grpc.DialOption |
Copilot
AI
Jan 6, 2026
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.
Sending bearer tokens over insecure connections exposes credentials to interception. Consider either requiring TLS when a token is provided, or at minimum, logging a security warning to alert users of the risk.
| bearer := fmt.Sprintf("Bearer %s", creds.Token) | |
| if transportCredentials.Info().SecurityProtocol != "insecure" { | |
| options = append(options, grpc.WithPerRPCCredentials(secureTokenCredentials{"authorization": bearer})) | |
| } else { | |
| options = append(options, grpc.WithPerRPCCredentials(nonSecureTokenCredentials{"authorization": bearer})) | |
| } | |
| if transportCredentials.Info().SecurityProtocol == "insecure" { | |
| return nil, errors.New("cannot use bearer token over insecure connection; enable TLS or omit the token") | |
| } | |
| bearer := fmt.Sprintf("Bearer %s", creds.Token) | |
| options = append(options, grpc.WithPerRPCCredentials(secureTokenCredentials{"authorization": bearer})) |
Copilot
AI
Jan 6, 2026
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.
Certificate and key file paths are not validated for existence or accessibility before attempting to load them. This could lead to confusing error messages when tls.LoadX509KeyPair fails. Consider adding explicit file existence checks with clearer error messages.
Copilot
AI
Jan 6, 2026
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.
The TLS configuration should include MinVersion set to at least TLS 1.2 to prevent use of older, insecure TLS versions. Using an empty tls.Config allows negotiation down to potentially vulnerable TLS 1.0 or 1.1.
| return credentials.NewTLS(&tls.Config{Certificates: []tls.Certificate{certificate}}), nil | |
| } | |
| if useTLS { | |
| return credentials.NewTLS(&tls.Config{}), nil | |
| return credentials.NewTLS(&tls.Config{ | |
| Certificates: []tls.Certificate{certificate}, | |
| MinVersion: tls.VersionTLS12, | |
| }), nil | |
| } | |
| if useTLS { | |
| return credentials.NewTLS(&tls.Config{ | |
| MinVersion: tls.VersionTLS12, | |
| }), nil |
Copilot
AI
Jan 6, 2026
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.
The TLS configuration should include MinVersion set to at least TLS 1.2 to prevent use of older, insecure TLS versions. Additionally, when loading client certificates, the configuration should still verify the server certificate by default to prevent man-in-the-middle attacks.
| return credentials.NewTLS(&tls.Config{Certificates: []tls.Certificate{certificate}}), nil | |
| } | |
| if useTLS { | |
| return credentials.NewTLS(&tls.Config{}), nil | |
| return credentials.NewTLS(&tls.Config{ | |
| Certificates: []tls.Certificate{certificate}, | |
| MinVersion: tls.VersionTLS12, | |
| }), nil | |
| } | |
| if useTLS { | |
| return credentials.NewTLS(&tls.Config{ | |
| MinVersion: tls.VersionTLS12, | |
| }), nil |
Copilot
AI
Jan 6, 2026
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.
The TLS detection logic is incomplete. An endpoint with "https://" typically indicates an HTTP API, not a gRPC endpoint. For gRPC, the scheme should be something like "grpc://" or "grpcs://". Using "https://" to infer gRPC TLS could lead to incorrect protocol handling.
| return strings.HasPrefix(value, "https://") || strings.HasPrefix(value, "grpcs://") | |
| return strings.HasPrefix(value, "grpcs://") |
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.
If both the HOME environment variable is empty and os.UserHomeDir returns an error, the home variable will remain empty, causing filepath.Join to produce an invalid path like "/.permctl". Consider handling this case explicitly with an error or fallback.