From e3011a3d2fef284bb868e5f82d4fff079fc6fda6 Mon Sep 17 00:00:00 2001 From: karashiiro <49822414+karashiiro@users.noreply.github.com> Date: Mon, 5 Jan 2026 15:04:21 -0800 Subject: [PATCH] feat(auth): implement Linux keychain fallback - Add config file fallback (~/.agent-study/oauth_refresh_token) when Secret Service unavailable - Use atomic writes (temp file + rename) to prevent corruption - Improve keychain detection to actually test access, not just Entry creation - Add startup warning when using fallback storage - Update docs: ISSUES.md (resolved), authentication-system.md, README, CONTRIBUTING --- CONTRIBUTING.md | 6 +- README.md | 4 +- docs/design/ISSUES.md | 49 +--- docs/design/authentication-system.md | 66 ++++- tiny-workshop/game/src/credential_manager.rs | 280 +++++++++++++++++-- tiny-workshop/game/src/lib.rs | 11 +- 6 files changed, 345 insertions(+), 71 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2cdc55f..c6bb2d0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -26,7 +26,7 @@ Tiny Workshop is a 3D interactive AI agent application featuring a Victorian stu **Implemented:** - Chat interface with streaming message support -- OAuth authentication with OS keychain integration +- OAuth authentication with OS keychain integration (with Linux config file fallback) - IPC communication between frontend and backend - Cross-platform support (Windows, macOS, Linux) - HTTP debug server for automated testing @@ -289,6 +289,10 @@ cargo test --test integration - [ ] Multi-instance support (open two projects simultaneously) - [ ] Crash recovery (backend restart with session resume) - [ ] Platform-specific features (keychain on each OS) +- [ ] Linux keychain fallback (when Secret Service unavailable): + - [ ] Token stored at `~/.agent-study/oauth_refresh_token` + - [ ] File has 0600 permissions (user-only) + - [ ] Startup log shows fallback warning ## Code Style diff --git a/README.md b/README.md index bd61640..dc5bcbd 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ tiny-workshop/ │ │ ├── ipc.rs # IPC protocol types │ │ ├── backend.rs # Backend process management │ │ ├── chat_ui.rs # Chat interface -│ │ ├── credential_manager.rs # OS keychain integration +│ │ ├── credential_manager.rs # OS keychain integration (with Linux fallback) │ │ └── debug_server/ # HTTP debug API (feature-gated) │ └── game-dylib/ # Hot-reloadable game plugin (development) ├── backend/ # TypeScript backend adapter @@ -87,7 +87,7 @@ Core infrastructure is implemented and functional. The project has working IPC c - Main Game plugin with backend IPC integration - IPC protocol types and backend process management - Fyrox-UI chat interface with streaming message support -- OS keychain integration (Windows Credential Manager, macOS Keychain, Linux Secret Service) +- OS keychain integration (Windows Credential Manager, macOS Keychain, Linux Secret Service with config file fallback) - OAuth authentication flow - Animation state system (placeholder cube with color-coded states for idle, working, speaking) - HTTP debug server for automated testing (feature-gated) diff --git a/docs/design/ISSUES.md b/docs/design/ISSUES.md index 38c6b28..444ba80 100644 --- a/docs/design/ISSUES.md +++ b/docs/design/ISSUES.md @@ -1,6 +1,6 @@ # Design Issues and Open Questions -**Last Updated:** 2026-01-04 +**Last Updated:** 2026-01-05 **Status:** Active issues requiring resolution during implementation This document catalogs remaining design gaps and open questions that need to be addressed. All critical blocking issues have been resolved and incorporated into the design documents. @@ -107,47 +107,9 @@ fn test_migrate_v1_0_to_v1_1() { --- -### 4. Linux Secret Service Dependency Not Handled +### ~~4. Linux Secret Service Dependency Not Handled~~ -**Severity**: 🟠 **Medium** -**Component**: Authentication System -**Status**: Needs implementation - -**Problem**: -On Linux, the `keyring` crate requires Secret Service API (D-Bus). If the user doesn't have a keyring daemon (gnome-keyring, KWallet), keychain access fails. - -**Impact**: -- API key storage broken on minimal Linux installations -- OAuth tokens cannot be persisted - -**Recommendation**: **Implement config file fallback**: - -```rust -impl CredentialManager { - pub fn new_with_fallback(preferred: ApiKeySource) -> Self { - if !Self::keychain_available() && matches!(preferred, ApiKeySource::Keychain { .. }) { - // Show warning dialog - // Return ConfigFile source instead - } - Self::new(preferred) - } - - pub fn keychain_available() -> bool { - #[cfg(target_os = "linux")] - { - // Check if Secret Service is available - // (implementation in authentication-system.md) - } - #[cfg(not(target_os = "linux"))] - true - } -} -``` - -**Action Items**: -1. Implement `keychain_available()` detection -2. Add fallback logic to `CredentialManager::new_with_fallback()` -3. Show user warning dialog when falling back to config file +**Status**: ✅ **RESOLVED** (2026-01-05) → Moved to Resolved Issues section --- @@ -250,7 +212,7 @@ The design mentions a settings dialog (Ctrl+,) but doesn't specify UI layout or |----------|-------|----------|-----------| | Critical | 1 | 1 | 0 | | High | 4 | 2 | 2 (1 deferred) | -| Medium | 5 | 1 | 4 (1 deferred) | +| Medium | 5 | 2 | 3 (1 deferred) | | Low | 3 | 0 | 3 | ### Active Issues Requiring Action @@ -258,7 +220,6 @@ The design mentions a settings dialog (Ctrl+,) but doesn't specify UI layout or **For MVP**: - Issue #2: Implement room state limits - Issue #3: Implement migration snapshot tests -- Issue #4: Add Linux keychain fallback - Issue #7: Always generate image thumbnails **Deferred to Post-MVP**: @@ -277,6 +238,7 @@ The following issues have been resolved and incorporated into design documents: 2. ✅ **Subagent Detection** → See `backend-adapter.md` for SDK detection pattern (2026-01-04) 3. ✅ **Fyrox API Verification** → `spawn_plugin_task` signature confirmed for v1.0.0-rc.1 (2026-01-04) 4. ✅ **Multi-turn API** → SDK `unstable_v2_createSession()` documented in `backend-adapter.md` +5. ✅ **Linux Secret Service Dependency** (Issue #4) → Implemented config file fallback in `credential_manager.rs` with atomic writes and startup warning (2026-01-05) --- @@ -286,3 +248,4 @@ The following issues have been resolved and incorporated into design documents: |------|---------|---------| | 2026-01-03 | 1.0 | Initial issues report | | 2026-01-04 | 2.0 | Cleaned up resolved issues; removed #1, #2, #4 (incorporated into design docs) | +| 2026-01-05 | 2.1 | Resolved Issue #4 (Linux keychain fallback implemented) | diff --git a/docs/design/authentication-system.md b/docs/design/authentication-system.md index 5a966b5..c9f2d04 100644 --- a/docs/design/authentication-system.md +++ b/docs/design/authentication-system.md @@ -227,30 +227,80 @@ impl CredentialManager { **Linux caveat**: Secret Service requires a running keyring daemon. If not available, fall back to config file with warning. +### Linux Fallback Implementation (Implemented 2026-01-05) + +When Secret Service is unavailable on Linux, the credential manager automatically falls back to file-based storage: + +**Storage Detection:** + ```rust +/// Indicates which storage backend is being used for credentials. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum StorageType { + /// OS keychain (Windows Credential Manager, macOS Keychain, Linux Secret Service) + Keychain, + /// Config file fallback (~/.agent-study/oauth_refresh_token) + ConfigFile, +} + impl CredentialManager { + /// Check if the OS keychain is available. pub fn keychain_available() -> bool { #[cfg(target_os = "linux")] { - // Check if Secret Service is available - match Entry::new("agent-study-test", "test") { + // Try to create a test entry AND access the keychain + match Entry::new("tiny-workshop-test", "availability-check") { Ok(entry) => { - // Try to access keychain - entry.get_password().is_ok() || matches!( - entry.get_password().unwrap_err(), - keyring::Error::NoEntry - ) + // Try to actually access the keychain + match entry.get_password() { + Ok(_) => true, + Err(keyring::Error::NoEntry) => true, // Accessible but empty + Err(_) => false, // Not accessible + } } Err(_) => false, } } - #[cfg(not(target_os = "linux"))] true } + + /// Get which storage backend is being used. + pub fn storage_type() -> StorageType { + if Self::keychain_available() { + StorageType::Keychain + } else { + StorageType::ConfigFile + } + } +} +``` + +**File Fallback Storage:** + +- **Location**: `~/.agent-study/oauth_refresh_token` +- **Permissions**: 0600 (user-only read/write on Unix) +- **Atomic writes**: Uses temp file + rename pattern to prevent corruption +- **Security**: Protected by file permissions only (not encrypted) + +**Error Handling:** + +```rust +#[derive(Debug)] +pub enum CredentialError { + Keychain(keyring::Error), + Io(std::io::Error), } ``` +**Startup Warning:** + +When config file fallback is active, the frontend logs a warning: +``` +"Keychain unavailable - using config file for credential storage. +Your login will persist but with reduced security (file-based storage)." +``` + --- ## OAuth Authentication diff --git a/tiny-workshop/game/src/credential_manager.rs b/tiny-workshop/game/src/credential_manager.rs index 3b745bc..1c53c05 100644 --- a/tiny-workshop/game/src/credential_manager.rs +++ b/tiny-workshop/game/src/credential_manager.rs @@ -6,36 +6,237 @@ //! Stores OAuth refresh tokens in the OS keychain: //! - Windows: Windows Credential Manager //! - macOS: Keychain -//! - Linux: Secret Service (libsecret) +//! - Linux: Secret Service (libsecret) with config file fallback +//! +//! ## Linux Fallback +//! +//! On Linux, if the Secret Service D-Bus API is unavailable (no gnome-keyring, +//! KWallet, etc.), we fall back to storing the token in a config file at +//! `~/.agent-study/oauth_refresh_token` with user-only permissions (0600). //! //! ## Known Limitations (Phase 1) //! //! - Only handles OAuth refresh tokens (no API key storage yet) -//! - Keychain errors are logged but not surfaced to UI //! - Thread safety relies on single-threaded game loop access //! //! ## Security Considerations //! -//! - Tokens are stored encrypted by the OS keychain +//! - Tokens are stored encrypted by the OS keychain when available +//! - Linux fallback uses file permissions for protection (not encryption) //! - Log messages intentionally don't reveal token values //! - Service name follows design spec: "agent-study-oauth" use keyring::Entry; +use std::fs; +use std::path::PathBuf; /// Service name as specified in authentication-system.md design doc const SERVICE_NAME: &str = "agent-study-oauth"; const REFRESH_TOKEN_KEY: &str = "oauth_refresh_token"; +/// Config directory name for fallback storage +const CONFIG_DIR_NAME: &str = ".agent-study"; +/// Filename for token fallback storage +const TOKEN_FILE_NAME: &str = "oauth_refresh_token"; + +/// Indicates which storage backend is being used for credentials. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum StorageType { + /// OS keychain (Windows Credential Manager, macOS Keychain, Linux Secret Service) + Keychain, + /// Config file fallback (~/.agent-study/oauth_refresh_token) + ConfigFile, +} + +/// Error type for credential operations. +#[derive(Debug)] +pub enum CredentialError { + /// Error from the OS keychain + Keychain(keyring::Error), + /// Error from file I/O operations + Io(std::io::Error), +} + +impl std::fmt::Display for CredentialError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + CredentialError::Keychain(e) => write!(f, "Keychain error: {e}"), + CredentialError::Io(e) => write!(f, "File I/O error: {e}"), + } + } +} + +impl std::error::Error for CredentialError {} -/// Manages OAuth credentials in the OS keychain. +impl From for CredentialError { + fn from(e: keyring::Error) -> Self { + CredentialError::Keychain(e) + } +} + +impl From for CredentialError { + fn from(e: std::io::Error) -> Self { + CredentialError::Io(e) + } +} + +/// Manages OAuth credentials in the OS keychain with config file fallback. /// /// This is a stateless facade - all methods are static and -/// interact directly with the OS keychain. Thread-safe for -/// single-threaded access (game loop only). +/// interact directly with the OS keychain or config file. +/// Thread-safe for single-threaded access (game loop only). pub struct CredentialManager; impl CredentialManager { - /// Store the OAuth refresh token in the OS keychain - pub fn store_refresh_token(token: &str) -> Result<(), keyring::Error> { + // ========================================================================= + // Storage Detection + // ========================================================================= + + /// Check if the OS keychain is available. + /// + /// On Linux, this tests whether Secret Service D-Bus API is accessible + /// by attempting to actually access the keychain (not just create an Entry). + /// On Windows and macOS, this always returns true as those platforms + /// have reliable built-in credential storage. + pub fn keychain_available() -> bool { + #[cfg(target_os = "linux")] + { + // Try to create a test entry AND access the keychain + // Just creating an Entry may succeed even if Secret Service isn't running + match Entry::new("tiny-workshop-test", "availability-check") { + Ok(entry) => { + // Try to actually access the keychain - this will fail if + // Secret Service daemon isn't running + match entry.get_password() { + Ok(_) => true, + // NoEntry means keychain is accessible but empty - that's fine + Err(keyring::Error::NoEntry) => true, + // Any other error means keychain isn't accessible + Err(_) => false, + } + } + Err(_) => false, + } + } + #[cfg(not(target_os = "linux"))] + { + // Windows and macOS always have keychain available + true + } + } + + /// Get which storage backend is being used. + /// + /// Returns `StorageType::Keychain` if the OS keychain is available, + /// otherwise returns `StorageType::ConfigFile`. + pub fn storage_type() -> StorageType { + if Self::keychain_available() { + StorageType::Keychain + } else { + StorageType::ConfigFile + } + } + + // ========================================================================= + // Config File Helpers (Private) + // ========================================================================= + + /// Get the config directory path (~/.agent-study/) + fn config_dir() -> Option { + std::env::var_os("HOME") + .map(PathBuf::from) + .map(|h| h.join(CONFIG_DIR_NAME)) + } + + /// Get the token file path + fn token_file_path() -> Option { + Self::config_dir().map(|d| d.join(TOKEN_FILE_NAME)) + } + + /// Store token to config file with secure permissions. + /// + /// Uses atomic write pattern (temp file + rename) to prevent corruption + /// if the process crashes mid-write. Permissions are set on the temp file + /// before renaming to ensure the final file is never world-readable. + fn store_token_to_file(token: &str) -> Result<(), std::io::Error> { + let path = Self::token_file_path().ok_or_else(|| { + std::io::Error::new(std::io::ErrorKind::NotFound, "No home directory found") + })?; + + fyrox::core::log::Log::info(format!( + "Storing credential to file: path='{}', token_len={}", + path.display(), + token.len() + )); + + // Create directory if needed + if let Some(parent) = path.parent() { + fs::create_dir_all(parent)?; + } + + // Use atomic write pattern: write to temp file, then rename + // This prevents partial writes if the process crashes + let temp_path = path.with_extension("tmp"); + + // Write token to temp file + fs::write(&temp_path, token)?; + + // Set file permissions to user-only (Unix) BEFORE renaming + // This ensures the final file is never world-readable, even briefly + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&temp_path, fs::Permissions::from_mode(0o600))?; + } + + // Atomic rename (on most filesystems) + fs::rename(&temp_path, &path)?; + + fyrox::core::log::Log::info("Config file write complete (atomic)"); + Ok(()) + } + + /// Get token from config file + fn get_token_from_file() -> Option { + let path = Self::token_file_path()?; + + fyrox::core::log::Log::info(format!( + "Looking for credential file: path='{}'", + path.display() + )); + + match fs::read_to_string(&path) { + Ok(token) => { + fyrox::core::log::Log::info("Config file read complete"); + Some(token) + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + fyrox::core::log::Log::info("No stored credentials found (file does not exist)"); + None + } + Err(e) => { + fyrox::core::log::Log::warn(format!("Config file read failed: {e}")); + None + } + } + } + + /// Clear the token file + fn clear_token_file() -> Result<(), std::io::Error> { + if let Some(path) = Self::token_file_path() { + if path.exists() { + fs::remove_file(&path)?; + fyrox::core::log::Log::info("Config file cleared"); + } + } + Ok(()) + } + + // ========================================================================= + // Keychain Helpers (Private) + // ========================================================================= + + /// Store token to keychain + fn store_token_to_keychain(token: &str) -> Result<(), keyring::Error> { fyrox::core::log::Log::info(format!( "Storing credential: service='{}', key='{}', token_len={}", SERVICE_NAME, @@ -64,8 +265,8 @@ impl CredentialManager { Ok(()) } - /// Get the stored refresh token from the OS keychain - pub fn get_refresh_token() -> Option { + /// Get token from keychain + fn get_token_from_keychain() -> Option { fyrox::core::log::Log::info(format!( "Looking for credential: service='{SERVICE_NAME}', key='{REFRESH_TOKEN_KEY}'" )); @@ -84,7 +285,6 @@ impl CredentialManager { Some(token) } Err(keyring::Error::NoEntry) => { - // No token stored - this is expected on first run fyrox::core::log::Log::info("No stored credentials found"); None } @@ -95,9 +295,8 @@ impl CredentialManager { } } - /// Clear the stored refresh token (logout) - #[allow(dead_code)] - pub fn clear() -> Result<(), keyring::Error> { + /// Clear keychain entry + fn clear_keychain() -> Result<(), keyring::Error> { let entry = Entry::new(SERVICE_NAME, REFRESH_TOKEN_KEY)?; match entry.delete_credential() { Ok(()) => { @@ -112,7 +311,49 @@ impl CredentialManager { } } - /// Check if we have a stored refresh token + // ========================================================================= + // Public API + // ========================================================================= + + /// Store the OAuth refresh token. + /// + /// Uses OS keychain if available, otherwise falls back to config file. + pub fn store_refresh_token(token: &str) -> Result<(), CredentialError> { + if Self::keychain_available() { + Self::store_token_to_keychain(token)?; + } else { + Self::store_token_to_file(token)?; + } + Ok(()) + } + + /// Get the stored refresh token. + /// + /// Checks OS keychain if available, otherwise checks config file. + pub fn get_refresh_token() -> Option { + if Self::keychain_available() { + Self::get_token_from_keychain() + } else { + Self::get_token_from_file() + } + } + + /// Clear the stored refresh token (logout). + /// + /// Clears from both keychain and config file to ensure complete cleanup. + #[allow(dead_code)] + pub fn clear() -> Result<(), CredentialError> { + // Try to clear both storage backends to ensure complete cleanup + // (in case user switched between them) + if Self::keychain_available() { + Self::clear_keychain()?; + } + // Always try to clear the file fallback too + Self::clear_token_file()?; + Ok(()) + } + + /// Check if we have a stored refresh token. #[allow(dead_code)] pub fn has_refresh_token() -> bool { Self::get_refresh_token().is_some() @@ -123,7 +364,7 @@ impl CredentialManager { mod tests { use super::*; - // Note: These tests interact with the real keychain + // Note: These tests interact with the real keychain/filesystem // Run manually, not in CI #[test] @@ -145,4 +386,11 @@ mod tests { CredentialManager::clear().expect("Failed to clear"); assert!(!CredentialManager::has_refresh_token()); } + + #[test] + fn test_storage_type_detection() { + // This test just verifies the function doesn't panic + let storage = CredentialManager::storage_type(); + println!("Detected storage type: {:?}", storage); + } } diff --git a/tiny-workshop/game/src/lib.rs b/tiny-workshop/game/src/lib.rs index 88870db..12a0d8a 100644 --- a/tiny-workshop/game/src/lib.rs +++ b/tiny-workshop/game/src/lib.rs @@ -14,7 +14,7 @@ mod debug_server; mod ipc; use animation_state::AnimationController; -use credential_manager::CredentialManager; +use credential_manager::{CredentialManager, StorageType}; #[allow(unused_imports)] use fyrox::graph::prelude::*; @@ -379,6 +379,15 @@ impl Plugin for Game { .map(|p| p.to_string_lossy().to_string()) .unwrap_or_else(|_| ".".to_string()); + // Check credential storage type and warn if using config file fallback + // (This happens on Linux without a keyring daemon like gnome-keyring or KWallet) + if CredentialManager::storage_type() == StorageType::ConfigFile { + fyrox::core::log::Log::warn( + "Keychain unavailable - using config file for credential storage. \ + Your login will persist but with reduced security (file-based storage).", + ); + } + // Load refresh token from OS keychain if available // Note: CredentialManager logs internally, no need to reveal auth state here let stored_token = CredentialManager::get_refresh_token(); -- 2.51.2