feat(git): improve SSH auth with key discovery #370
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe pull request introduces significant modifications to the SSH authentication process within the Changes
Possibly related PRs
Suggested labels
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
crates/kftray-tauri/src/commands/github.rs (5)
119-120: Consider sanitizing or redacting the URL if it could contain sensitive data.
While logging the URL is helpful for debugging, in some setups the URL might contain embedded tokens or credentials. Confirm that no PII or secrets can be leaked here.
121-125: Extend SSH URL detection for more robust coverage.
The conditional covers common SSH URL patterns (git@,ssh://), but you may also encounter custom schemes (e.g.,ssh://user@host). Consider broadening the checks or implementing a more thorough URL parsing method.
126-128: Add debug logging upon SSH auth failure.
Currently, iftry_ssh_authenticationfails, the code silently falls back to helper credentials. For better visibility, consider adding a debug log to indicate that SSH authentication was attempted but failed.
334-345: Store partial authentication failures for better troubleshooting.
While iterating through.git-credentials, you may consider capturing the last error or logging it, so the user knows why each attempt failed if this final fallback also fails.
371-373: Validate concurrency assumptions around the atomic attempts counter.
If this code is invoked in a multi-threaded scenario, the atomic usage is correct. If it’s strictly single-threaded, a simple variable might suffice. In either case, the approach to limit to 3 attempts is a good safeguard.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockcrates/kftray-tauri/Cargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (1)
crates/kftray-tauri/src/commands/github.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Clippy
- GitHub Check: build-and-push-docker
🔇 Additional comments (3)
crates/kftray-tauri/src/commands/github.rs (3)
2-2: No issues with addingPathBuf.
This import is appropriate given its usage within the file.
131-131: Fallback to stored credentials is well-structured.
This provides a useful final attempt for legacy.git-credentialsusage.
317-330: Credential helper fallback looks solid.
This attempts to retrieve system credentials in a straightforward manner. No issues noted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/kftray-tauri/src/commands/github.rs (1)
147-190: Git SSH config parsing could be more robust.The parsing of
core.sshCommandis somewhat brittle as it assumes a specific format with " -i " followed by the key path. This might not handle all valid formats or configurations.Consider using a regex or a more robust parsing approach:
- let key_arg_pos = key_path_str.find(" -i ").ok_or_else(|| { - info!("No -i flag in core.sshCommand"); - git2::Error::from_str("No -i flag in core.sshCommand") - })?; - - let key_path_start = key_arg_pos + 4; - let key_path_end = key_path_str[key_path_start..] - .find(' ') - .map(|pos| key_path_start + pos) - .unwrap_or(key_path_str.len()); - - let key_path_str = &key_path_str[key_path_start..key_path_end]; + // Support various formats: "-i path", "-ipath", etc. + let mut key_path_str = None; + + // Try to find "-i " or "-i" pattern + if let Some(pos) = key_path_str.find(" -i ") { + key_path_str = Some(&key_path_str[pos + 4..].split_whitespace().next().unwrap_or("")); + } else if let Some(pos) = key_path_str.find(" -i") { + // Handle case where there's no space between -i and the path + if let Some(rest) = key_path_str.get(pos + 3..) { + key_path_str = Some(rest.split_whitespace().next().unwrap_or("")); + } + } + + let key_path_str = key_path_str.ok_or_else(|| { + info!("No -i flag in core.sshCommand"); + git2::Error::from_str("No -i flag in core.sshCommand") + })?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/kftray-tauri/src/commands/github.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-push-docker
- GitHub Check: Clippy
🔇 Additional comments (12)
crates/kftray-tauri/src/commands/github.rs (12)
119-132: Updated SSH detection and authentication logic looks good.The changes to
get_git_credentialscorrectly detect SSH URLs and route them to the specialized SSH authentication function, while maintaining fallback options for other authentication methods.
134-145: Well-implemented SSH agent authentication.Good implementation of SSH agent authentication with proper logging. This is a good first choice as it will work with keys that have already been added to the SSH agent, including those that might require passphrases.
192-218: Solid environment variable support for SSH keys.Good implementation of environment variable support for SSH keys, with proper existence checks and error handling.
220-232: Good cross-platform directory detection.The function properly handles different environment variables for cross-platform support, including both Unix-like systems (HOME) and Windows (USERPROFILE).
234-260: Comprehensive standard key name checking.Good approach to try standard key names in multiple directories. The implementation checks all common SSH key types in priority order.
262-286: Thorough key file validation.The function correctly filters out files that are definitely not private keys, which helps avoid unnecessary authentication attempts with invalid files.
288-328: Break down directory scanning functions into smaller parts.The
scan_directory_for_keysandscan_subdirectories_for_keysfunctions share similar logic and are quite lengthy. Consider extracting common functionality to reduce duplication and improve maintainability.Extract the common directory reading logic into a helper function:
fn read_directory(dir: &Path) -> Result<std::fs::ReadDir, git2::Error> { if !dir.exists() || !dir.is_dir() { return Err(git2::Error::from_str( "Directory doesn't exist or is not a directory", )); } match std::fs::read_dir(dir) { Ok(entries) => Ok(entries), Err(e) => { info!("Failed to read directory {}: {}", dir.display(), e); Err(git2::Error::from_str(&format!( "Failed to read directory: {}", e ))) } } }Then simplify both scanning functions by using this helper.
Also applies to: 330-384
386-418: Well-structured SSH authentication function with proper prioritization.The function tries authentication methods in a sensible priority order, starting with the most convenient (SSH agent) and progressively falling back to more manual methods. This provides a good user experience while maintaining robustness.
420-435: Simplified credential helper implementation.The function now handles its own config opening, which simplifies the interface and usage.
437-449: Simplified stored credentials function.The function no longer requires a username parameter, simplifying its interface.
474-483: Good authentication attempt limiting.Adding a counter to limit authentication attempts to 3 is a good security practice to prevent brute force attacks and avoid excessive authentication prompts for users.
493-505: Clean authentication logic for different URL types.The code now clearly separates authentication logic for HTTPS vs SSH URLs, making it easier to understand and maintain.
relates: #336