Add persisted credentials for CLI client #12
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements persisted credentials for the CLI client, enabling users to store and reuse authentication settings across sessions. The implementation adds YAML-based credential storage, updates client initialization to load stored credentials, and enhances the configure command with credential management.
- Added YAML-backed credential persistence at
~/.permify/credentialswith user-only file permissions - Modified client initialization to load stored credentials when no endpoint is explicitly provided, with TLS inference and bearer token authentication support
- Enhanced configure flow to pre-fill prompts from stored credentials and validate certificate/key pairing before persistence
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
| core/client/credentials_storage.go | New file implementing credential loading and storage with YAML serialization and user-only permissions |
| core/client/grpc.go | Updated client initialization to load persisted credentials, infer TLS settings, and apply bearer authentication |
| core/cli/configure.go | Enhanced configuration flow to pre-fill from stored credentials and persist before client creation |
| cmd/permctl/permctl.go | Improved home directory resolution for cross-platform compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return creds, nil | ||
| } | ||
|
|
||
| // StoreCredentials writes the provided credentials to disk with user-only permissions. |
There was a problem hiding this comment.
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. |
| return credentials.NewTLS(&tls.Config{Certificates: []tls.Certificate{certificate}}), nil | ||
| } | ||
|
|
||
| if useTLS { | ||
| return credentials.NewTLS(&tls.Config{}), nil |
There was a problem hiding this comment.
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 |
| return credentials.NewTLS(&tls.Config{Certificates: []tls.Certificate{certificate}}), nil | ||
| } | ||
|
|
||
| if useTLS { | ||
| return credentials.NewTLS(&tls.Config{}), nil |
There was a problem hiding this comment.
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 |
| CertKey: certKey, | ||
| }) | ||
| if storeErr != nil { | ||
| logger.Log.Error(storeErr) |
There was a problem hiding this comment.
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) |
| certificate, err := tls.LoadX509KeyPair(creds.CertPath, creds.CertKey) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("load tls key pair: %w", err) |
There was a problem hiding this comment.
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.
| finalEndpoint = storedCreds.Endpoint | ||
| } | ||
| if finalEndpoint == "" { | ||
| return nil, errors.New("permify endpoint missing: provide an endpoint or store one in ~/.permify/credentials") |
There was a problem hiding this comment.
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") |
|
|
||
| func isSecureEndpoint(endpoint string) bool { | ||
| value := strings.ToLower(endpoint) | ||
| return strings.HasPrefix(value, "https://") || strings.HasPrefix(value, "grpcs://") |
There was a problem hiding this comment.
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://") |
| if finalEndpoint == "" { | ||
| return nil, errors.New("permify endpoint missing: provide an endpoint or store one in ~/.permify/credentials") | ||
| } | ||
|
|
There was a problem hiding this comment.
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") | |
| } |
| if derived, err := os.UserHomeDir(); err == nil { | ||
| home = derived | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| } | |
| } | |
| if home == "" { | |
| // Fallback to the current working directory if home cannot be determined. | |
| if cwd, err := os.Getwd(); err == nil { | |
| home = cwd | |
| } else { | |
| os.Stderr.WriteString("permctl: unable to determine configuration directory (HOME and working directory unavailable)\n") | |
| os.Exit(1) | |
| } | |
| } |
|
|
||
| var creds Credentials | ||
| if err := yaml.Unmarshal(data, &creds); err != nil { | ||
| return Credentials{}, fmt.Errorf("parse credentials file %s: %w", file, err) |
There was a problem hiding this comment.
The function currently returns an empty Credentials struct when the file doesn't exist, but LoadCredentials is also called in the New function where an error during loading would fail the entire client creation. If there's a corrupted credentials file, this could prevent users from using the CLI even with explicit endpoint parameters. Consider whether LoadCredentials should be more lenient in the New function when an explicit endpoint is provided.
| 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 |
1 participant
/claim #2
Added persisted credential handling: new credential struct + YAML load/save under OS default path; user-only perms.
Client construction now pulls stored endpoint/token/cert, infers TLS, loads key pairs when provided, and attaches bearer auth via per-RPC creds.
Configure flow now pre-fills from stored creds, prompts for token/cert, validates cert/key pairing, persists credentials before creating the client, and fails fast on client creation errors.
Files touched: added [credentials_storage.go]; updated [grpc.go]; updated [configure.go].
Credentials persistence: new YAML-backed store at [~/.permify/credentials], user-only perms, fields for endpoint/token/cert path/cert key.
Client init: [New] now pulls stored creds when endpoint is empty, infers TLS (https/grpcs or cert presence), loads keypair when provided, and applies bearer auth via per-RPC creds (secure vs. insecure paths).
Configure flow: prompts pre-filled from stored creds, captures token/cert paths, enforces cert+key pairing, writes credentials before client creation, and surfaces client creation errors early.
To test, run: go run ./cmd/permctl configure
In the video, it showed error because those were not real data. you can see that it return previous api url i entered. Try it will real data, it will show success.
here is the video: https://youtu.be/YiGxpxr3-xA