Skip to content

Conversation

@darksworm
Copy link
Owner

@darksworm darksworm commented Jan 17, 2026

Summary

Implements automatic login functionality that triggers when authentication fails (token missing or expired). Users are prompted for username/password credentials, with tokens stored securely in the OS keychain.

  • Auto-login using stored credentials from keychain on startup
  • Login modal with username/password input when auto-login fails
  • Cross-platform keychain support (macOS Keychain, Linux secret-service, Windows Credential Manager)
  • Option to save credentials for future auto-login
  • Secure token storage using zalando/go-keyring

Login Flow

  1. App starts without token → attempts auto-login with keychain
  2. If auto-login fails → shows login modal
  3. User enters credentials → optionally saves to keychain
  4. On success → proceeds to load apps

New Files

  • pkg/auth/keychain.go - Cross-platform keychain abstraction
  • pkg/auth/session.go - ArgoCD session API client
  • pkg/auth/manager.go - Auth flow orchestration
  • cmd/app/model_auth.go - Auth-related message handlers
  • cmd/app/view_login.go - Login modal TUI rendering

Test plan

  • Build and run argonaut without a token in ArgoCD config
  • Verify auto-login attempt is made (check logs)
  • Verify login modal appears when no stored credentials
  • Test login with valid credentials
  • Test login with invalid credentials (should show error)
  • Verify "save credentials" checkbox stores to keychain
  • Restart app and verify auto-login works with stored credentials
  • Test on macOS/Linux to verify keychain integration

Summary by CodeRabbit

  • New Features
    • Added interactive login modal with username and password input fields
    • Introduced auto-login functionality to streamline repeated server connections
    • Added option to save credentials for faster future authentication
    • Integrated visual loading indicator during the authentication process
    • Enhanced error handling and messaging for failed login attempts
    • Implemented secure credential storage across sessions

✏️ Tip: You can customize this high-level summary in your review settings.

Implement automatic login functionality that triggers when authentication
fails (token missing or expired). Users are prompted for username/password
credentials, with tokens stored securely in the OS keychain.

Features:
- Auto-login using stored credentials from keychain on startup
- Login modal with username/password input when auto-login fails
- Cross-platform keychain support (macOS, Linux, Windows)
- Option to save credentials for future auto-login
- Secure token storage using zalando/go-keyring

The login flow:
1. App starts without token -> attempts auto-login with keychain
2. If auto-login fails -> shows login modal
3. User enters credentials -> optionally saves to keychain
4. On success -> proceeds to load apps
@coderabbitai
Copy link

coderabbitai bot commented Jan 17, 2026

Walkthrough

Introduces a comprehensive login flow system enabling users to authenticate via username/password credentials when no token is available. Adds authentication services (session, keychain, manager), UI components for login inputs, message types for orchestrating the flow, and state management for tracking login modal state and transitions between authentication modes.

Changes

Cohort / File(s) Summary
Input Components & Handlers
cmd/app/input_components.go, cmd/app/input_handlers.go
Adds username and password text input fields with initialization, focus/blur management, and value getters/setters. Introduces keyboard-driven login mode handler supporting tab/enter/space/esc navigation and form submission with validation.
Authentication Infrastructure
pkg/auth/session.go, pkg/auth/manager.go, pkg/auth/keychain.go
Implements SessionService for HTTP-based ArgoCD login/token validation, KeychainStore for cross-platform credential persistence, and AuthManager coordinating both with auto-login and manual login flows.
Message Types & State
pkg/model/messages.go, pkg/model/state.go, pkg/model/types.go
Defines login-related messages (LoginSubmitMsg, LoginSuccessMsg, LoginErrorMsg, AutoLoginAttemptMsg, AutoLoginResultMsg, LoginCancelMsg, LoginFieldFocusMsg) and extends ModalState with login-specific fields (LoginServerURL, LoginUsername, LoginError, LoginSaveCredentials, LoginFieldFocus, LoginLoading). Adds ModeLogin and ModeLoginLoading constants.
App Core & Handlers
cmd/app/main.go, cmd/app/model.go, cmd/app/model_auth.go, cmd/app/model_init.go
Integrates login flow into main startup: adds NoTokenError type for triggering login, routes login messages (AutoLoginAttemptMsg, LoginSubmitMsg, etc.) to handlers, implements auto-login attempt on missing tokens and authentication errors, and manages state transitions between login/loading/auth modes.
Login UI & Rendering
cmd/app/view.go, cmd/app/view_login.go
Adds view rendering for ModeLogin and ModeLoginLoading, implements renderLoginModal with input fields, checkbox, buttons, and error display, plus renderLoginLoadingView with spinner and status.
Configuration & Dependencies
pkg/config/cli_config.go, go.mod
Adds GetServerURLForCurrentContext() method for login flow server URL retrieval. Introduces indirect dependencies for keychain support (go-keyring, shellescape, dbus, wincred).

Sequence Diagram(s)

sequenceDiagram
    participant User as User
    participant UI as UI/Model
    participant AuthMgr as AuthManager
    participant Keychain as KeychainStore
    participant SessionSvc as SessionService
    
    rect rgba(100, 200, 255, 0.5)
    Note over UI,SessionSvc: Auto-Login Attempt (on token absence or expiry)
    User->>UI: App starts / Token invalid
    UI->>AuthMgr: TryAutoLogin(ctx)
    AuthMgr->>Keychain: LoadToken(serverURL)
    alt Token found and valid
        Keychain-->>AuthMgr: token
        AuthMgr->>SessionSvc: ValidateToken(ctx, token)
        SessionSvc-->>AuthMgr: nil (valid)
        AuthMgr-->>UI: AuthResult{Token, Username}
        UI->>UI: Update mode to loading, store token
    else Token not found or invalid
        Keychain-->>AuthMgr: error / empty
        AuthMgr->>Keychain: LoadCredentials(serverURL)
        Keychain-->>AuthMgr: username, password
        AuthMgr->>SessionSvc: Login(ctx, username, password)
        SessionSvc-->>AuthMgr: token
        AuthMgr->>Keychain: StoreToken(serverURL, token)
        AuthMgr-->>UI: AuthResult{Token, Username}
        UI->>UI: Update mode to loading, store token
    else All attempts fail
        AuthMgr-->>UI: error
        UI->>UI: Show login modal (ModeLogin)
    end
    end
Loading
sequenceDiagram
    participant User as User
    participant LoginUI as Login UI
    participant InputMgr as InputComponents
    participant UI as Model/Handlers
    participant AuthMgr as AuthManager
    participant SessionSvc as SessionService
    
    rect rgba(100, 200, 255, 0.5)
    Note over User,SessionSvc: Manual Login (user provides credentials)
    User->>LoginUI: Focus username field
    User->>InputMgr: Type username
    InputMgr->>UI: UpdateUsernameInput(msg)
    User->>LoginUI: Tab to password field
    User->>InputMgr: Type password
    InputMgr->>UI: UpdatePasswordInput(msg)
    User->>LoginUI: Tab to "Save Credentials" checkbox
    User->>LoginUI: Toggle with space / Enter
    User->>LoginUI: Click Login button / Press Enter
    LoginUI->>UI: LoginSubmitMsg{Username, Password, SaveCredentials}
    UI->>AuthMgr: LoginWithCredentials(ctx, username, password, saveCredentials)
    AuthMgr->>SessionSvc: Login(ctx, username, password)
    SessionSvc-->>AuthMgr: token or error
    alt Login successful
        AuthMgr->>AuthMgr: StoreToken(serverURL, token)
        alt SaveCredentials enabled
            AuthMgr->>AuthMgr: StoreCredentials(serverURL, username, password)
        end
        AuthMgr-->>UI: AuthResult{Token, Username}
        UI->>UI: LoginSuccessMsg
        UI->>UI: Update token, clear login inputs, switch to loading
    else Login failed
        AuthMgr-->>UI: error
        UI->>UI: LoginErrorMsg{Error}
        UI->>UI: Clear loading state, display error, refocus username
    end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • feat: open resources in k9s #151: Modifies central key-event routing in cmd/app/input_handlers.go to add mode-specific handlers; shares the same function and orchestrates keyboard navigation patterns similar to the login flow integration.
🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Tests ⚠️ Warning PR introduces 779 lines of new authentication functionality across 5 files with zero unit tests, despite repository's strong testing culture. Add comprehensive unit tests for pkg/auth and cmd/app files covering keychain operations, session management, auto-login flows, credential storage, and message routing.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: automatic login with keychain credential storage' accurately describes the primary feature added in the changeset, which implements automatic login with OS keychain integration for credential storage.
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link

greptile-apps bot commented Jan 17, 2026

Greptile Summary

This PR implements automatic login with OS keychain integration for ArgoCD authentication. The implementation adds a seamless authentication flow that attempts to auto-login using stored credentials from the system keychain (macOS Keychain, Linux secret-service, Windows Credential Manager) before falling back to a manual login modal.

Key Changes:

  • New auth package (pkg/auth/) with three core components:
    • keychain.go: Cross-platform credential storage using zalando/go-keyring with SHA-256 hashed server keys
    • session.go: ArgoCD session API client with token validation via /api/v1/session/userinfo
    • manager.go: Auth flow orchestration with graceful fallbacks (token → stored credentials → manual login)
  • Login modal TUI (cmd/app/view_login.go): Professional login interface with masked input, keyboard navigation, and optional credential saving
  • Auto-login integration (cmd/app/model_init.go): Automatically triggers on missing/invalid sessions with proper error handling

Authentication Flow:

  1. App checks for existing session → validates via API
  2. If invalid/missing → attempts auto-login with keychain
  3. If auto-login fails → shows login modal for user input
  4. Credentials can be optionally saved to keychain for future auto-login

Security Considerations:

  • Passwords properly masked using EchoPassword mode with bullet characters
  • Keychain errors handled as non-fatal warnings (won't crash app if keychain unavailable)
  • Uses system-native credential storage (macOS Keychain, Linux secret-service, Windows Credential Manager)
  • Existing InsecureSkipVerify usage is configuration-driven (not introduced by this PR)

Minor Issue Found:

  • containsAny function in pkg/auth/keychain.go:195-206 uses case-sensitive substring matching for error detection, which may miss some keychain error messages with different casing

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The implementation is well-architected with proper separation of concerns, comprehensive error handling, and secure credential management. The only issue found is a minor style improvement for case-insensitive error matching. The code follows established patterns in the codebase, uses a well-maintained keychain library (zalando/go-keyring), and properly handles edge cases like keychain unavailability and connection errors.
  • Pay attention to pkg/auth/keychain.go for the minor case-sensitivity improvement suggested in the review comment

Important Files Changed

Filename Overview
pkg/auth/keychain.go Cross-platform keychain abstraction using zalando/go-keyring. Uses SHA-256 hash truncation for server keys. Minor issue: containsAny function uses case-sensitive substring matching which may miss some error messages.
pkg/auth/session.go ArgoCD session API client with proper timeout configuration and error handling. Validates tokens via /api/v1/session/userinfo endpoint.
pkg/auth/manager.go Orchestrates auth flow with token validation fallback to credentials. Properly handles keychain errors as non-fatal warnings.
cmd/app/model_auth.go Handles auth message flow with proper timeout (30s) and pre-fills username from keychain. Clean separation of concerns.
cmd/app/view_login.go Login modal TUI with password masking (EchoPassword mode). Proper keyboard navigation and field focus management.
cmd/app/input_components.go Adds username/password input components with proper masking. Password field uses EchoPassword mode with bullet character.
cmd/app/model_init.go Triggers auto-login when token is missing/invalid. Falls back to login modal on connection errors.

Sequence Diagram

sequenceDiagram
    participant User
    participant App as Argonaut
    participant Init as Initialization
    participant AuthMgr as Auth Manager
    participant Store as Keychain
    participant API as ArgoCD API
    participant Modal as Login UI

    User->>App: Start
    App->>Init: validateAuthentication()
    
    alt No existing session
        Init->>AuthMgr: TryAutoLogin()
        AuthMgr->>Store: Check for stored session
        
        alt Session exists
            Store-->>AuthMgr: Existing session
            AuthMgr->>API: Validate session
            
            alt Valid session
                API-->>AuthMgr: Valid
                AuthMgr-->>Init: Success
                Init-->>App: Load apps
            else Invalid session
                API-->>AuthMgr: Invalid
                AuthMgr->>Store: Get stored login info
            end
        else No session
            AuthMgr->>Store: Get stored login info
        end
        
        alt Login info available
            Store-->>AuthMgr: Login info
            AuthMgr->>API: Authenticate
            API-->>AuthMgr: New session
            AuthMgr->>Store: Save session
            AuthMgr-->>Init: Success
            Init-->>App: Load apps
        else No login info
            AuthMgr-->>Init: Needs user input
            Init->>Modal: Show form
            Modal-->>User: Display
            
            User->>Modal: Enter details
            User->>Modal: Submit
            Modal->>AuthMgr: Process login
            AuthMgr->>API: Authenticate
            API-->>AuthMgr: New session
            AuthMgr->>Store: Save session
            AuthMgr->>Store: Save login info (optional)
            AuthMgr-->>Modal: Success
            Modal-->>App: LoginSuccess
            App-->>User: Load apps
        end
    else Session exists
        Init->>API: Validate existing
        API-->>Init: Status
        
        alt Still valid
            Init-->>App: Load apps
        else Expired
            Init->>AuthMgr: TryAutoLogin()
            Note over Init,AuthMgr: Retry with stored info
        end
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

17 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +195 to +206
func containsAny(s string, substrs ...string) bool {
for _, substr := range substrs {
if len(s) >= len(substr) {
for i := 0; i <= len(s)-len(substr); i++ {
if s[i:i+len(substr)] == substr {
return true
}
}
}
}
return false
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: case-sensitive substring matching may not catch all keychain errors

The containsAny function performs case-sensitive matching, but error messages from different keychain backends may use different casing (e.g., "DBus" vs "dbus", "Keychain" vs "keychain"). Consider using strings.ToLower() on the error string before matching.

Suggested change
func containsAny(s string, substrs ...string) bool {
for _, substr := range substrs {
if len(s) >= len(substr) {
for i := 0; i <= len(s)-len(substr); i++ {
if s[i:i+len(substr)] == substr {
return true
}
}
}
}
return false
}
func containsAny(s string, substrs ...string) bool {
s = strings.ToLower(s)
for _, substr := range substrs {
substr = strings.ToLower(substr)
if len(s) >= len(substr) {
for i := 0; i <= len(s)-len(substr); i++ {
if s[i:i+len(substr)] == substr {
return true
}
}
}
}
return false
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/auth/keychain.go
Line: 195:206

Comment:
**style:** case-sensitive substring matching may not catch all keychain errors

The `containsAny` function performs case-sensitive matching, but error messages from different keychain backends may use different casing (e.g., "DBus" vs "dbus", "Keychain" vs "keychain"). Consider using `strings.ToLower()` on the error string before matching.

```suggestion
func containsAny(s string, substrs ...string) bool {
	s = strings.ToLower(s)
	for _, substr := range substrs {
		substr = strings.ToLower(substr)
		if len(s) >= len(substr) {
			for i := 0; i <= len(s)-len(substr); i++ {
				if s[i:i+len(substr)] == substr {
					return true
				}
			}
		}
	}
	return false
}
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cmd/app/input_components.go (1)

85-89: Call BlurLoginInputs() in handleLoginError() before re-focusing the username input.

In handleLoginError() (model_auth.go, ~line 131), add m.inputComponents.BlurLoginInputs() before calling m.inputComponents.FocusUsernameInput(). This ensures that if the password input had focus before the error, it will be properly blurred. The same applies to handleLoginSuccess() after clearing the inputs.

🤖 Fix all issues with AI agents
In `@go.mod`:
- Line 50: The go.mod currently lists github.com/zalando/go-keyring as an
indirect dependency but it is directly imported in pkg/auth/keychain.go; update
go.mod by moving github.com/zalando/go-keyring v0.2.6 into the main require
block (remove the "// indirect" comment) so it becomes a direct requirement,
then run go mod tidy to ensure module metadata is consistent; reference the
import usage in pkg/auth/keychain.go to verify the change.

In `@pkg/auth/keychain.go`:
- Around line 94-117: In StoreCredentials
(DefaultKeychainStore.StoreCredentials) if storing the password fails after the
username was already stored, remove the earlier username entry to avoid leaving
partial credentials; specifically, after passwordFullKey Set fails (and if
!isKeychainUnavailable(err)), call the keyring delete/remove function for
usernameFullKey (e.g., keyring.Delete/Remove) and return a wrapped error that
includes the original password storage error and, if the cleanup delete itself
fails, include that cleanup error as well; if isKeychainUnavailable(err) return
ErrKeychainUnavailable immediately without attempting cleanup.

In `@pkg/auth/session.go`:
- Around line 61-65: The TLS config block that sets InsecureSkipVerify should
also set a minimum TLS version to avoid weak protocol negotiation; update the
tls.Config assigned to transport.TLSClientConfig (the block where cfg.Insecure
is true and InsecureSkipVerify is set) to include MinVersion = tls.VersionTLS12
(or tls.VersionTLS13 if you require only TLS1.3), so the config explicitly
enforces the minimum allowed TLS protocol.
🧹 Nitpick comments (8)
pkg/auth/keychain.go (2)

195-206: Prefer strings.Contains from the standard library.

This custom implementation duplicates functionality available in the strings package and is harder to read.

♻️ Proposed refactor
 func containsAny(s string, substrs ...string) bool {
 	for _, substr := range substrs {
-		if len(s) >= len(substr) {
-			for i := 0; i <= len(s)-len(substr); i++ {
-				if s[i:i+len(substr)] == substr {
-					return true
-				}
-			}
+		if strings.Contains(s, substr) {
+			return true
 		}
 	}
 	return false
 }

Note: You'll need to add "strings" to the imports.


182-193: Consider case-insensitive matching for error detection.

The string matching for keychain availability errors is case-sensitive, which may miss capitalized variants like "Secret Service" or "DBus" that could appear in error messages from different platforms or library versions.

♻️ Proposed refactor
 func isKeychainUnavailable(err error) bool {
 	if err == nil {
 		return false
 	}
-	errStr := err.Error()
+	errStr := strings.ToLower(err.Error())
 	return errors.Is(err, keyring.ErrUnsupportedPlatform) ||
 		containsAny(errStr, "secret service", "dbus", "keychain", "credential manager")
 }
pkg/auth/manager.go (1)

144-159: Inconsistent error handling in ClearStoredCredentials.

The method logs a warning for DeleteToken failure but continues, while DeleteCredentials failure both logs a warning and returns the error. This inconsistency could leave the keychain in a partial state if token deletion fails silently.

Consider either returning errors for both failures or documenting why token deletion errors are non-fatal.

♻️ Option A: Return error for token deletion failure
 func (m *AuthManager) ClearStoredCredentials() error {
 	log := cblog.With("component", "auth-manager")
 
 	if err := m.keychain.DeleteToken(m.serverURL); err != nil {
 		log.Warn("Failed to delete token from keychain", "err", err)
+		return err
 	}
 
 	if err := m.keychain.DeleteCredentials(m.serverURL); err != nil {
 		log.Warn("Failed to delete credentials from keychain", "err", err)
 		return err
 	}
 
 	log.Info("Cleared stored credentials from keychain")
 	return nil
 }
♻️ Option B: Make both errors non-fatal
 func (m *AuthManager) ClearStoredCredentials() error {
 	log := cblog.With("component", "auth-manager")
+	var lastErr error
 
 	if err := m.keychain.DeleteToken(m.serverURL); err != nil {
 		log.Warn("Failed to delete token from keychain", "err", err)
+		lastErr = err
 	}
 
 	if err := m.keychain.DeleteCredentials(m.serverURL); err != nil {
 		log.Warn("Failed to delete credentials from keychain", "err", err)
-		return err
+		lastErr = err
 	}
 
-	log.Info("Cleared stored credentials from keychain")
-	return nil
+	if lastErr == nil {
+		log.Info("Cleared stored credentials from keychain")
+	}
+	return lastErr
 }
cmd/app/main.go (1)

451-458: Consider defining error sentinels for more robust detection.

The string-based error matching ("auth token", "no token", "argocd login") is fragile if error messages change in dependent packages. Consider defining sentinel errors or error types in the config package for more reliable detection.

♻️ Example approach

In pkg/config/cli_config.go, define:

var ErrMissingToken = errors.New("missing auth token")

Then in ToServerConfig(), return wrapped errors:

return nil, fmt.Errorf("%w: please run argocd login", ErrMissingToken)

And here, check with:

if errors.Is(err, config.ErrMissingToken) {
    // ...
}
cmd/app/input_handlers.go (1)

1244-1247: Minor: ctrl+c is redundant here.

ctrl+c is already handled globally at line 1426-1428 before reaching mode-specific handlers. This case is unreachable but harmless.

♻️ Suggested cleanup
 	case "esc":
 		return m.cancelLogin()
-	case "ctrl+c":
-		return m.cancelLogin()
cmd/app/view_login.go (1)

46-55: Consider using setter methods instead of direct field access.

Lines 47 and 53 directly access m.inputComponents.usernameInput and m.inputComponents.passwordInput to call SetWidth(). While this works since the fields are in the same package, it's inconsistent with the pattern of using methods like RenderUsernameInput() and RenderPasswordInput() on lines 48 and 54. Consider adding SetUsernameInputWidth and SetPasswordInputWidth methods for consistency, or accept this as an acceptable internal coupling.

cmd/app/model_auth.go (2)

121-134: Consider clearing password input on login failure.

Currently, the password remains in the input field after a login error, allowing users to retry easily. However, this could be a minor security concern on shared devices. Consider whether m.inputComponents.passwordInput.SetValue("") should be called here, depending on your security posture and UX preferences.


160-168: Minor: New AuthManager created just to retrieve stored username.

A new AuthManager is instantiated solely to call GetStoredUsername(). This works but creates unnecessary overhead. Consider whether this method could be made static/standalone on the KeychainStore directly if this pattern becomes common. This is a minor optimization concern and acceptable as-is.

github.com/tidwall/match v1.2.0 // indirect
github.com/tidwall/pretty v1.2.1 // indirect
github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect
github.com/zalando/go-keyring v0.2.6 // indirect
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check for direct imports of zalando/go-keyring
echo "=== Searching for direct imports of go-keyring ==="
rg -n --type=go 'import.*go-keyring|github.com/zalando/go-keyring' -C 2

echo -e "\n=== Checking go.mod structure ==="
cat go.mod | head -100

Repository: darksworm/argonaut

Length of output: 2544


Move zalando/go-keyring to direct dependencies.

The package github.com/zalando/go-keyring is directly imported in pkg/auth/keychain.go but marked as // indirect in go.mod. Move it from the indirect dependencies to the main require block without the comment.

🤖 Prompt for AI Agents
In `@go.mod` at line 50, The go.mod currently lists github.com/zalando/go-keyring
as an indirect dependency but it is directly imported in pkg/auth/keychain.go;
update go.mod by moving github.com/zalando/go-keyring v0.2.6 into the main
require block (remove the "// indirect" comment) so it becomes a direct
requirement, then run go mod tidy to ensure module metadata is consistent;
reference the import usage in pkg/auth/keychain.go to verify the change.

Comment on lines +94 to +117
// StoreCredentials stores username and password for a server
func (k *DefaultKeychainStore) StoreCredentials(serverURL, username, password string) error {
urlKey := serverKey(serverURL)

// Store username
usernameFullKey := fmt.Sprintf("%s:%s:%s", serviceName, urlKey, usernameKey)
if err := keyring.Set(serviceName, usernameFullKey, username); err != nil {
if isKeychainUnavailable(err) {
return ErrKeychainUnavailable
}
return fmt.Errorf("failed to store username in keychain: %w", err)
}

// Store password
passwordFullKey := fmt.Sprintf("%s:%s:%s", serviceName, urlKey, passwordKey)
if err := keyring.Set(serviceName, passwordFullKey, password); err != nil {
if isKeychainUnavailable(err) {
return ErrKeychainUnavailable
}
return fmt.Errorf("failed to store password in keychain: %w", err)
}

return nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Partial credential storage on failure leaves inconsistent keychain state.

If storing the username succeeds but storing the password fails, the keychain is left with a username but no password. This could cause LoadCredentials to return stale or mismatched data on subsequent calls.

Consider either storing both atomically (if supported) or cleaning up the username if password storage fails.

🔧 Proposed fix
 // StoreCredentials stores username and password for a server
 func (k *DefaultKeychainStore) StoreCredentials(serverURL, username, password string) error {
 	urlKey := serverKey(serverURL)
 
 	// Store username
 	usernameFullKey := fmt.Sprintf("%s:%s:%s", serviceName, urlKey, usernameKey)
 	if err := keyring.Set(serviceName, usernameFullKey, username); err != nil {
 		if isKeychainUnavailable(err) {
 			return ErrKeychainUnavailable
 		}
 		return fmt.Errorf("failed to store username in keychain: %w", err)
 	}
 
 	// Store password
 	passwordFullKey := fmt.Sprintf("%s:%s:%s", serviceName, urlKey, passwordKey)
 	if err := keyring.Set(serviceName, passwordFullKey, password); err != nil {
+		// Clean up username to avoid partial state
+		_ = keyring.Delete(serviceName, usernameFullKey)
 		if isKeychainUnavailable(err) {
 			return ErrKeychainUnavailable
 		}
 		return fmt.Errorf("failed to store password in keychain: %w", err)
 	}
 
 	return nil
 }
📝 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.

Suggested change
// StoreCredentials stores username and password for a server
func (k *DefaultKeychainStore) StoreCredentials(serverURL, username, password string) error {
urlKey := serverKey(serverURL)
// Store username
usernameFullKey := fmt.Sprintf("%s:%s:%s", serviceName, urlKey, usernameKey)
if err := keyring.Set(serviceName, usernameFullKey, username); err != nil {
if isKeychainUnavailable(err) {
return ErrKeychainUnavailable
}
return fmt.Errorf("failed to store username in keychain: %w", err)
}
// Store password
passwordFullKey := fmt.Sprintf("%s:%s:%s", serviceName, urlKey, passwordKey)
if err := keyring.Set(serviceName, passwordFullKey, password); err != nil {
if isKeychainUnavailable(err) {
return ErrKeychainUnavailable
}
return fmt.Errorf("failed to store password in keychain: %w", err)
}
return nil
}
// StoreCredentials stores username and password for a server
func (k *DefaultKeychainStore) StoreCredentials(serverURL, username, password string) error {
urlKey := serverKey(serverURL)
// Store username
usernameFullKey := fmt.Sprintf("%s:%s:%s", serviceName, urlKey, usernameKey)
if err := keyring.Set(serviceName, usernameFullKey, username); err != nil {
if isKeychainUnavailable(err) {
return ErrKeychainUnavailable
}
return fmt.Errorf("failed to store username in keychain: %w", err)
}
// Store password
passwordFullKey := fmt.Sprintf("%s:%s:%s", serviceName, urlKey, passwordKey)
if err := keyring.Set(serviceName, passwordFullKey, password); err != nil {
// Clean up username to avoid partial state
_ = keyring.Delete(serviceName, usernameFullKey)
if isKeychainUnavailable(err) {
return ErrKeychainUnavailable
}
return fmt.Errorf("failed to store password in keychain: %w", err)
}
return nil
}
🤖 Prompt for AI Agents
In `@pkg/auth/keychain.go` around lines 94 - 117, In StoreCredentials
(DefaultKeychainStore.StoreCredentials) if storing the password fails after the
username was already stored, remove the earlier username entry to avoid leaving
partial credentials; specifically, after passwordFullKey Set fails (and if
!isKeychainUnavailable(err)), call the keyring delete/remove function for
usernameFullKey (e.g., keyring.Delete/Remove) and return a wrapped error that
includes the original password storage error and, if the cleanup delete itself
fails, include that cleanup error as well; if isKeychainUnavailable(err) return
ErrKeychainUnavailable immediately without attempting cleanup.

Comment on lines +61 to +65
if cfg.Insecure {
transport.TLSClientConfig = &tls.Config{
InsecureSkipVerify: true,
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Set TLS MinVersion to avoid weak protocol negotiation.

The TLS configuration is missing MinVersion, which means Go's default (TLS 1.2 for clients) will be used. While TLS 1.2 is acceptable, explicitly setting MinVersion is a security best practice, especially when InsecureSkipVerify is already being used (which weakens security posture).

As flagged by static analysis: this configuration should explicitly declare the minimum TLS version.

🔒 Proposed fix
 	if cfg.Insecure {
 		transport.TLSClientConfig = &tls.Config{
 			InsecureSkipVerify: true,
+			MinVersion:         tls.VersionTLS12,
 		}
 	}
📝 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.

Suggested change
if cfg.Insecure {
transport.TLSClientConfig = &tls.Config{
InsecureSkipVerify: true,
}
}
if cfg.Insecure {
transport.TLSClientConfig = &tls.Config{
InsecureSkipVerify: true,
MinVersion: tls.VersionTLS12,
}
}
🧰 Tools
🪛 ast-grep (0.40.5)

[warning] 61-63: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures

(missing-ssl-minversion-go)

🤖 Prompt for AI Agents
In `@pkg/auth/session.go` around lines 61 - 65, The TLS config block that sets
InsecureSkipVerify should also set a minimum TLS version to avoid weak protocol
negotiation; update the tls.Config assigned to transport.TLSClientConfig (the
block where cfg.Insecure is true and InsecureSkipVerify is set) to include
MinVersion = tls.VersionTLS12 (or tls.VersionTLS13 if you require only TLS1.3),
so the config explicitly enforces the minimum allowed TLS protocol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants