feat(password): switch to 99designs/keyring with fallback support #2
Conversation
- Replace zalando/go-keyring with 99designs/keyring for better backend support - Add platform-specific backend priority: - macOS: Keychain → encrypted file - Linux: SecretService → KWallet → encrypted file - Windows: WinCred → encrypted file - Add password dialog for missing passwords (fixes #1) - Derive encrypted file password from machine ID + username - Improve error handling: show password prompt on read failure Closes #1 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughAdds a password dialog UI and integrates a new multi-backend keyring-based password store with file-backed encrypted fallback and machine-ID–derived file passwords; connection flows now prompt for and persist missing passwords and surface structured password storage/read errors. Changes
sequenceDiagram
participant User
participant App
participant Manager as ConnManager
participant Store as PasswordStore
participant Dialog as PasswordDialog
participant DB as Database
User->>App: Select stored connection
App->>Manager: GetConnectionConfigWithPassword(entry)
alt Password present
Manager->>Store: Get(key)
Store-->>Manager: password
Manager-->>App: ConnectionConfigResult{Config with Password}
App->>DB: Connect
DB-->>App: Success/Failure
else Password missing / not found
Manager-->>App: ConnectionConfigResult{PasswordMissing: true}
App->>Dialog: SetConnectionInfo + Show
Dialog->>User: Render prompt
User->>Dialog: Submit(password)
Dialog-->>App: PasswordSubmitMsg{password}
App->>Manager: SavePassword(...)
Manager->>Store: Set(key, password)
Store-->>Manager: nil / PasswordSaveError
App->>Manager: GetConnectionConfigWithPassword(entry)
Manager->>Store: Get(key)
Store-->>Manager: password
Manager-->>App: ConnectionConfigResult{Config with Password}
App->>DB: Connect
DB-->>App: Success/Failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus review on:
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
internal/connection_history/machine_id.go (1)
94-110: Note:wmicis deprecated on modern Windows.The
wmiccommand is deprecated starting with Windows 10 21H1. While it still works on most systems, consider using PowerShell as a more future-proof alternative:(Get-CimInstance -ClassName Win32_ComputerSystemProduct).UUIDThis is low priority since the fallback to hostname ensures functionality.
internal/app/app.go (1)
2858-2870: Consider extracting duplicated Add result handling.The same pattern for handling
connectionHistory.Addresults appears in three places (lines 2858-2870, 3029-3040, 3124-3135). Consider extracting to a helper method:func (a *App) handleConnectionHistoryAdd(config models.ConnectionConfig) { result, err := a.connectionHistory.Add(config) if err != nil { log.Printf("Warning: Failed to save connection to history: %v", err) } else if result != nil && result.PasswordSaveError != nil { log.Printf("Warning: Failed to save password: %v", result.PasswordSaveError) } // Reload history in dialog history := a.connectionHistory.GetRecent(10) a.connectionDialog.SetHistoryEntries(history) }internal/ui/components/password_dialog.go (1)
43-62: Hardcoded colors may conflict with the theme system.The input styles use hardcoded color values (e.g.,
#cba6f7,#cdd6f4) while aThemeis passed to the dialog. Consider usingthcolors for consistency.That said, the
CharLimitof 256 adequately addresses the 64-character password issue from #1.🔎 Suggested improvement
func NewPasswordDialog(th theme.Theme) *PasswordDialog { input := textinput.New() input.Placeholder = "Enter password" input.EchoMode = textinput.EchoPassword input.EchoCharacter = '•' - input.PromptStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("#cba6f7")) - input.TextStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("#cdd6f4")) - input.Cursor.Style = lipgloss.NewStyle().Foreground(lipgloss.Color("#f38ba8")) + input.PromptStyle = lipgloss.NewStyle().Foreground(th.Info) + input.TextStyle = lipgloss.NewStyle().Foreground(th.Foreground) + input.Cursor.Style = lipgloss.NewStyle().Foreground(th.Error) input.CharLimit = 256 input.Width = 40 input.Focus()internal/connection_history/manager.go (2)
64-71: Consider the semantics whenpasswordStoreis nil.Returning
falsewhenpasswordStore == nilimplies "not using fallback" but actually means "no password storage at all." This could be confusing for callers who might interpretfalseas "using native keyring."Consider adding a separate method like
HasPasswordStorage() boolor documenting this edge case clearly.
207-210: Password deletion error is silently ignored.While it's reasonable to not fail the delete operation if password cleanup fails, consider logging the error for debugging purposes.
🔎 Suggested improvement
if m.passwordStore != nil { - _ = m.passwordStore.Delete(entry.Host, entry.Port, entry.Database, entry.User) + if err := m.passwordStore.Delete(entry.Host, entry.Port, entry.Database, entry.User); err != nil { + log.Printf("Warning: Failed to delete password from store: %v", err) + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.mod(3 hunks)internal/app/app.go(11 hunks)internal/connection_history/errors.go(1 hunks)internal/connection_history/machine_id.go(1 hunks)internal/connection_history/manager.go(8 hunks)internal/connection_history/password_store.go(2 hunks)internal/ui/components/password_dialog.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/connection_history/password_store.go (1)
internal/connection_history/errors.go (3)
PasswordSaveError(9-12)ErrPasswordNotFound(6-6)PasswordReadError(26-28)
🪛 OSV Scanner (2.3.0)
go.mod
[HIGH] 30-30: github.com/dvsekhvalnov/jose2go 1.5.0: Denial of service when decrypting attacker controlled input in github.com/dvsekhvalnov/jose2go
(GO-2023-2409)
[HIGH] 30-30: github.com/dvsekhvalnov/jose2go 1.5.0: Denial-of-Service (DoS) via crafted JSON Web Encryption (JWE) token high compression ratio in github.com/dvsekhvalnov/jose2go
(GO-2025-4123)
[HIGH] 30-30: github.com/dvsekhvalnov/jose2go 1.5.0: jose2go vulnerable to denial of service via large p2c value
[HIGH] 30-30: github.com/dvsekhvalnov/jose2go 1.5.0: jose2go is vulnerable to a JWT bomb attack through its decode function
[HIGH] 30-30: github.com/dvsekhvalnov/jose2go 1.5.0: Denial of service when decrypting attack controlled input in github.com/dvsekhvalnov/jose2go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build (windows-latest)
🔇 Additional comments (17)
internal/connection_history/errors.go (1)
1-36: LGTM!Well-structured error types following Go conventions with proper
Unwrap()support for error chaining.internal/app/app.go (3)
597-615: LGTM! Password submission flow is well-implemented.The flow correctly:
- Creates connection config with the submitted password
- Attempts to save the password for future use (with error logging but non-blocking)
- Clears pending state before attempting connection
- Delegates to
performConnectionfor the actual connection
2793-2815: LGTM! History entry connection with password fallback.The implementation properly handles the case where a stored password is missing:
- Creates a copy of the entry to prevent mutation issues
- Initializes the password dialog with connection details
- Transitions cleanly between dialogs
3406-3431: LGTM!The
renderPasswordDialogfollows the same centering pattern as other dialogs, maintaining UI consistency.internal/connection_history/password_store.go (1)
72-89: Fallback detection cannot accurately determine which backend is actually in use at runtime.The
keyring.Open()function attempts backends from the allowed list sequentially, continuing to the next backend on failure until one succeeds. However, theisUsingFallback()function relies onkeyring.AvailableBackends(), which simply checks which backends are registered in thesupportedBackendsmap—a compile-time and early initialization check, not a runtime availability check.A backend can appear "available" during program startup but fail later when actually initialized. For example, SecretServiceBackend is only registered if
dbus.SessionBus()succeeds during init, but the actual service connection (vialibsecret.NewService()) can fail at runtime if the D-Bus service isn't running. Real-world issues confirm this: users report SecretService appearing in "Considering backends" but failing with "The name org.freedesktop.secrets was not provided" at runtime, forcing a fallback to FileBackend.The 99designs/keyring library does not expose a public API to detect which backend was actually opened. The
Keyringinterface returned byOpen()provides no method to determine the backend type, making it impossible forisUsingFallback()to reliably answer the question after the fact.internal/ui/components/password_dialog.go (7)
1-12: LGTM!Imports are appropriate for the Bubble Tea TUI framework components being used.
14-26: LGTM!Zone IDs and message types follow standard Bubble Tea patterns for event handling.
28-41: LGTM!Good design choice keeping the password only in the text input model rather than duplicating it in a separate field.
64-74: LGTM!Appropriately clears the password input and refocuses when displaying a new connection prompt. The description provides clear context for which connection needs the password.
81-101: Consider whether empty password submission should be allowed.Pressing Enter submits the password even if the input is empty. If empty passwords are not valid for connections, you might want to add validation here. If empty passwords are allowed (e.g., for local development DBs), this is fine as-is.
103-166: LGTM!Clean view implementation with appropriate defensive checks and consistent styling. The zone marking for buttons enables proper mouse interaction.
168-197: LGTM!Mouse handling correctly checks button type and action before processing. Getter methods are straightforward.
internal/connection_history/manager.go (5)
3-15: LGTM!Added imports are appropriate for the new error handling and logging functionality.
17-30: LGTM!The
ConnectionConfigResulttype cleanly encapsulates the password retrieval outcome, allowing callers to distinguish between missing passwords and retrieval errors.
42-52: Password store initialization failure is only logged, not exposed to caller.If the password store fails to initialize (
err != nil), the manager continues without password storage capability. The caller has no way to know this happened except by checkingIsUsingFallbackStorage()(which returnsfalsewhenpasswordStoreisnil).This is acceptable for graceful degradation, but consider whether the caller should be informed so it can warn the user that passwords won't be persisted.
107-164: LGTM!Good design separating the password save error from the connection history save error. This allows the connection to be saved even if password storage fails, with the caller able to inform the user appropriately.
218-264: LGTM!Comprehensive handling of password retrieval scenarios. The use of
errors.Isfor checkingErrPasswordNotFoundis correct for sentinel error comparison. The empty password check at lines 247-252 aligns well with the password dialog flow.
- Add UID fallback for username when env vars are empty (containers) - Use errors.Is() instead of == for error comparison 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/connection_history/password_store.go (2)
123-133: LGTM! Previous review concern addressed.The error comparison now correctly uses
errors.Is(err, keyring.ErrKeyNotFound)at line 127, addressing the previous review comment about proper error chain handling.
136-143: LGTM! Previous review concern addressed.The error comparison now correctly uses
!errors.Is(err, keyring.ErrKeyNotFound)at line 139, addressing the previous review comment about consistent error handling.
🧹 Nitpick comments (3)
internal/connection_history/machine_id.go (3)
18-39: Consider simplifying the function signature.The function always returns
nilfor the error, making the error return value unnecessary. If fallbacks are guaranteed to succeed, consider removing the error return to simplify the signature tofunc deriveFilePassword() string.Alternatively, if there are scenarios where password derivation could fail (e.g., insufficient entropy), return an actual error in those cases.
76-80: Add timeout to external command execution.The
exec.Commandcall lacks a timeout, which could cause the application to hang indefinitely ifioregfails to respond. Useexec.CommandContextwith a timeout to ensure bounded execution time.🔎 Proposed fix with timeout
+ "context" + "time" + func getDarwinMachineID() (string, error) { - cmd := exec.Command("ioreg", "-rd1", "-c", "IOPlatformExpertDevice") + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "ioreg", "-rd1", "-c", "IOPlatformExpertDevice") output, err := cmd.Output() if err != nil { return os.Hostname() }
100-104: Add timeout to external command execution.Similar to
getDarwinMachineID, this command lacks a timeout. Useexec.CommandContextwith a timeout to prevent indefinite hangs.🔎 Proposed fix with timeout
func getWindowsMachineID() (string, error) { - cmd := exec.Command("wmic", "csproduct", "get", "UUID") + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "wmic", "csproduct", "get", "UUID") output, err := cmd.Output() if err != nil { return os.Hostname() }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/connection_history/machine_id.go(1 hunks)internal/connection_history/password_store.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/connection_history/password_store.go (1)
internal/connection_history/errors.go (3)
PasswordSaveError(9-12)ErrPasswordNotFound(6-6)PasswordReadError(26-28)
🔇 Additional comments (6)
internal/connection_history/machine_id.go (2)
41-54: LGTM!The platform-specific routing and fallback to hostname for unknown OSes is appropriate.
56-72: LGTM!The implementation correctly checks standard Linux machine-id locations with appropriate fallback.
internal/connection_history/password_store.go (4)
1-18: LGTM!The imports are appropriate and the
PasswordStorestruct is well-designed with fields for the keyring and fallback detection.
21-45: LGTM with dependency note.The constructor properly initializes the keyring with platform-appropriate backends and file fallback. The error handling is appropriate.
Note: The
FilePasswordFuncat line 31 depends onderiveFilePassword()frommachine_id.go, which has a Windows compatibility issue flagged separately.
48-71: LGTM!The platform-specific backend priorities correctly align with the PR objectives:
- macOS: Keychain with file fallback
- Linux: SecretService/KWallet with file fallback
- Windows: WinCred with file fallback
100-120: LGTM!The
Savemethod correctly useskeyring.Itemwith appropriate metadata and returnsPasswordSaveErroron failure.
| if username == "" { | ||
| // Fallback for containers/service accounts without USER env | ||
| username = fmt.Sprintf("uid-%d", os.Getuid()) | ||
| } |
There was a problem hiding this comment.
os.Getuid() fallback is Unix-specific and returns -1 on Windows.
While lines 29-32 add a fallback for containers/service accounts, os.Getuid() always returns -1 on Windows, resulting in the same uid--1 for all Windows users without environment variables. This reduces entropy and fails to provide machine-specific uniqueness.
Consider using a cross-platform approach, such as:
- On Windows, query the Windows registry or use
whoamicommand - Generate a random UUID and persist it in the config directory for consistency across restarts
- Use
os.UserHomeDir()oros.UserCacheDir()as a proxy for user identity
🔎 Proposed cross-platform fix
username := os.Getenv("USER")
if username == "" {
username = os.Getenv("USERNAME") // Windows
}
if username == "" {
- // Fallback for containers/service accounts without USER env
- username = fmt.Sprintf("uid-%d", os.Getuid())
+ // Cross-platform fallback: use home directory as user identifier
+ if homeDir, err := os.UserHomeDir(); err == nil {
+ username = homeDir
+ } else {
+ username = "default-user"
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if username == "" { | |
| // Fallback for containers/service accounts without USER env | |
| username = fmt.Sprintf("uid-%d", os.Getuid()) | |
| } | |
| username := os.Getenv("USER") | |
| if username == "" { | |
| username = os.Getenv("USERNAME") // Windows | |
| } | |
| if username == "" { | |
| // Cross-platform fallback: use home directory as user identifier | |
| if homeDir, err := os.UserHomeDir(); err == nil { | |
| username = homeDir | |
| } else { | |
| username = "default-user" | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/connection_history/machine_id.go around lines 29 to 32, the current
fallback uses os.Getuid(), which is Unix-only and returns -1 on Windows; change
to a cross-platform fallback: first try os/user.Current() (or
os.UserHomeDir()/os.UserCacheDir() as a proxy) to obtain a stable identifier; if
that fails or yields empty on Windows, generate a random UUID, persist it to a
file in the user's config/cache directory (so it survives restarts), and use
that persisted UUID thereafter; ensure all errors are handled and that the
chosen identifier is normalized (no newlines) before returning.
| func isUsingFallback(requestedBackends []keyring.BackendType, ring keyring.Keyring) bool { | ||
| // If file backend is the only option, we're using fallback | ||
| if len(requestedBackends) == 1 && requestedBackends[0] == keyring.FileBackend { | ||
| return true | ||
| } | ||
|
|
||
| // Try to detect by checking available backends | ||
| availableBackends := keyring.AvailableBackends() | ||
| for _, b := range availableBackends { | ||
| if b != keyring.FileBackend { | ||
| // A native backend is available, likely not using fallback | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "password_store.go" -o -name "*password*store*" | head -20Repository: rebelice/lazypg
Length of output: 106
🏁 Script executed:
cat -n internal/connection_history/password_store.go | head -100Repository: rebelice/lazypg
Length of output: 3408
Fallback detection logic makes assumptions about backend selection rather than detecting actual usage.
The function checks keyring.AvailableBackends() which provides system-available backends, but this doesn't reflect which backend was actually selected by keyring.Open(). The 99designs/keyring library doesn't expose a public API to query the active backend—the Keyring interface only provides Get, Set, Remove, and Keys methods.
If a native backend (like SecretService on Linux) is available but fails during initialization, the keyring library would fall back to the file backend, yet this function would incorrectly return false based on availability alone.
For accurate fallback detection, consider:
- Wrapping
keyring.Open()to capture and store which backend succeeded during initialization - Performing a test operation to detect file backend behavior (e.g., checking for encrypted keyring file)
- Accepting this as a known limitation and documenting it clearly if the current behavior is acceptable for your use case
1 participant
Closes #1
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.