Feature/ssh dotnet terminal#2997
Open
joubertdj wants to merge 15 commits intomRemoteNG:v1.78.2-devfrom
Open
Conversation
Phase 1: Foundation & Core Infrastructure - Add VtNetCore dependency to centralized package versioning - Create SSHDotNetDiagnostics helper class with comprehensive logging - Update ProtocolType enum with SSH_DotNet = 15 - Add localization string for SshDotNet protocol - Create SshTerminalControl skeleton with WinForms UserControl support - Register SSH_DotNet protocol in ProtocolFactory Phase 2: Core SSH Connection - Implement SSHAuthenticationProvider with: * Password authentication support * Public key authentication (file-based and string-based) * Keyboard-interactive authentication for 2FA/MFA * Comprehensive error handling and logging - Implement SSHConnectionManager with: * SSH client creation and configuration * Shell stream creation with configurable terminal type * Keep-alive interval configuration * Connection diagnostics and information retrieval - Implement ProtocolSSH_DotNet with: * Full connection lifecycle management (Connecting → Authenticating → Connected) * Async output reading with cancellation support * State machine for connection tracking * Statistics (bytes received/sent, connection duration) * Comprehensive error handling for all SSH failure scenarios Unit Tests (25+ test cases) - SSHAuthenticationProviderTests: 12 tests for authentication methods - SSHConnectionManagerTests: 10 tests for connection management - ProtocolSSH_DotNetTests: 12 tests for protocol behavior Build Status: ✅ SUCCESS - All code compiles without errors Files Created: 8 - SSH_DotNet Feature 20251106.md (Updated plan with completion status) - SSHDotNetDiagnostics.cs - SshTerminalControl.cs - SSHAuthenticationProvider.cs - SSHConnectionManager.cs - ProtocolSSH_DotNet.cs - SSHAuthenticationProviderTests.cs - SSHConnectionManagerTests.cs - ProtocolSSH_DotNetTests.cs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…redentials - Removed _sshDotNetUsername and _sshDotNetPassword private fields from AbstractConnectionRecord.cs - Removed SSHDotNetUsername and SSHDotNetPassword properties from AbstractConnectionRecord.cs - Removed inheritance properties from ConnectionInfoInheritance.cs - Removed localization strings for SSH_DotNet fields from Language.resx - Removed localization accessors from Language.Designer.cs - Updated ProtocolSSH_DotNet.cs to use generic Username and Password properties - Simplified implementation to use existing connection properties instead of protocol-specific ones 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
The Username property was not visible in SSH_DotNet connections because it was missing from the AttributeUsedInProtocol list. Added ProtocolType.SSH_DotNet to allow Username field to appear in the GUI for SSH_DotNet protocol connections. Fixes issue where SSH_DotNet connections could not configure a username in the connection properties panel. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…ocol Integrate VtNetCore (v1.0.24) terminal emulation library into SshTerminalControl with: - VirtualTerminalController and DataConsumer for ANSI/VT100 terminal support - Keyboard input handling: arrow keys, function keys, special keys (Home, End, PageUp/Down, Tab, Delete) - Terminal output rendering with GetScreenText() API - PuTTY-style copy/paste: text selection auto-copies to clipboard, right-click to paste - Mouse event handling (left-click to select, right-click to paste) - Scrollback buffer management (configurable up to 10,000 lines) - Context menu with Copy, Paste, Select All, separator - Color scheme support (background and foreground colors) - Proper terminal resizing and dimension recalculation - Event handler initialization in Initialize() method - Comprehensive error handling and diagnostic logging All Phase 3 tasks completed: ✓ 3.1 - Integrate VtNetCore terminal emulation ✓ 3.2 - Implement keyboard input handling ✓ 3.3 - Implement terminal resize handling ✓ 3.4 - Implement PuTTY-style copy/paste functionality ✓ 3.5 - Implement color scheme support ✓ 3.6 - Implement scrollback buffer ✓ 3.7 - Add context menu to terminal 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Use TerminalFont property (with lazy initialization) instead of directly accessing _terminalFont field in OnPaint method. This ensures a font is always available even if OnPaint is called before Initialize() completes. Fixes: Value cannot be null. (Parameter 'font') error in terminal rendering 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The 'Already initialized' message is expected behavior when Initialize() is called multiple times. Changed from Warning to Debug log level to reduce noise in logs and better reflect that this is normal behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Root cause: SSH.NET's ShellStream.DataAvailable property is unreliable and often returns false even when data is available. This caused ReadOutputAsync to never actually read data, resulting in a blank terminal screen despite successful connection. Solution: Remove the DataAvailable check and use blocking ReadAsync directly. This allows the stream to properly block until data arrives, ensuring all SSH output is read and sent to the terminal control for rendering. Now terminal output should display correctly in the VtNetCore terminal emulator. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Root cause analysis revealed TWO critical missing pieces: 1. **No input writing loop** - Keyboard input was collected in _inputBuffer but NEVER sent to SSH server, causing server to sit idle with no commands. 2. **No verification of VtNetCore rendering** - Need to confirm data flows through entire pipeline: SSH → WriteOutput → DataConsumer → VtNetCore → GetScreenText → OnPaint Changes made: - Added _inputWriteTask parallel to _outputReadTask for bidirectional communication - Implemented WriteInputAsync() that polls terminal for keyboard input every 50ms - Writes input to SSH shell stream with proper flushing - Updated Disconnect() to wait for both input and output tasks - Added comprehensive diagnostic logging: * Terminal: Pushed X bytes to VtNetCore DataConsumer * Terminal: First render - screen text length * Terminal: OnPaint GetScreenText() empty/non-empty status * Input: Sent X bytes to SSH * Input: Starting/ending input writing loop This establishes complete request-response cycle: User types → _inputBuffer → WriteInputAsync → SSH server → server response → ReadOutputAsync → WriteOutput → DataConsumer → VtNetCore → GetScreenText → OnPaint → Display 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…etting _isInitialized Root Cause (Deep Analysis Revealed): The constructor was prematurely setting _isInitialized = true after partial setup, causing Initialize() to return early without creating the DataConsumer. Sequence of the bug: 1. Constructor creates _vtController but NOT _dataConsumer 2. Constructor sets _isInitialized = true ← BUG! 3. Protocol calls Initialize() 4. Initialize() checks _isInitialized, sees true, returns immediately 5. _dataConsumer is NEVER created → stays null 6. WriteOutput() tries to use null _dataConsumer → error This explained the log evidence: - "Terminal: First render - screen text length: 1943 chars" ✓ (vtController exists) - "Terminal: DataConsumer is null" ✗ (never created) - "Terminal: Copied 70 characters to clipboard" ✓ (scrollback exists) Fix: - Removed ALL initialization logic from constructor except basic setup - Let Initialize() method handle complete VtNetCore setup as designed - Added logging: "Created VtNetCore controller and DataConsumer" - Constructor now only does InitializeComponent() and basic properties This ensures DataConsumer is properly created and terminal can render output. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
EXPERIMENTAL FEATURE - NOT PRODUCTION READY This PR introduces a new SSH protocol implementation using SSH.NET and VtNetCore libraries as an experimental alternative for testing purposes. This does NOT replace the existing PuTTY SSH implementation. ## What's Been Achieved ### SSH_DotNet Protocol Implementation - Full SSH terminal emulation using VtNetCore (xterm-256color) - Username/password authentication support - Bidirectional input/output with zero-delay event-driven architecture - Terminal resize support with window change requests (SIGWINCH) - Smart disconnect detection (clean exit vs error disconnect) - Session statistics tracking (bytes sent/received, connection duration) - PuTTY-style copy/paste (selection auto-copies, right-click pastes) - Comprehensive keyboard support (arrows, function keys, Ctrl/Alt sequences) - ANSI color rendering with bold, underline attributes - Scrollback buffer (1000 lines default) ### Trace Logging System - New TraceMsg level added to message filtering infrastructure - Configurable via Options > Notifications UI - Supports all message writers (notification panel, log file, popup) - Log output prefixed with "TRACE -" for clarity - Integrated with SSH_DotNet diagnostics system ### Test Coverage - 109 unit tests covering 5 core components: - SSHConnectionManager (19 tests) - SSHAuthenticationProvider (21 tests) - ShellStreamExtensions (6 tests) - ProtocolSSH_DotNet (17 tests) - SshTerminalControl (46 tests) - Comprehensive parameter validation and error handling tests - Integration tests needed for end-to-end scenarios ## Current Limitations ### Authentication - **Only username/password authentication is currently supported** - SSH key authentication planned but not yet implemented - Keyboard-interactive authentication framework present but needs testing ### What's Missing for Production - SSH key/certificate authentication - Additional terminal size testing with various applications - Performance testing under high data rates - Extensive integration testing with real SSH servers - User feedback on visual elements and terminal rendering - Tab color/styling integration - Connection profiles and saved settings - Error message refinement - Accessibility features ## Technical Details ### Architecture - ProtocolSSH_DotNet: Main protocol implementation (1020 LOC) - SshTerminalControl: Custom terminal renderer (1694 LOC) - SSHConnectionManager: Connection lifecycle management (229 LOC) - SSHAuthenticationProvider: Authentication methods (228 LOC) - ShellStreamExtensions: Terminal resize via reflection (70 LOC) ### Dependencies - Renci.SshNet: SSH protocol implementation - VtNetCore: ANSI/VT100 terminal emulation - .NET 9.0 Windows with WPF support ### Files Changed - New: 6 implementation files - New: 9 test files - Modified: 13 message filtering files for Trace support - Modified: 3 UI files for Trace logging options ## Testing Recommendations This is an early implementation intended for: 1. Visual feedback on terminal rendering 2. Testing basic SSH connectivity 3. Gathering user experience feedback 4. Identifying edge cases and issues ## Next Steps Before Production 1. Implement SSH key authentication 2. Add comprehensive integration tests 3. Performance testing and optimization 4. Visual refinement based on user feedback 5. Tab styling and color scheme integration 6. Documentation and user guide 7. Error handling improvements 8. Accessibility audit ## Review Notes Please test with various SSH servers and terminal applications (htop, vim, etc.) to identify rendering issues or compatibility problems. This PR is the foundation for a full-featured SSH client within mRemoteNG. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This file is for development guidance only and should not be included in feature branches or pull requests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Planning documents should not be included in pull requests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds SSH.NET-based SSH protocol support to mRemoteNG, including a custom terminal emulator built with VtNetCore. The implementation adds a new SSH_DotNet protocol type alongside extensive test coverage and diagnostic capabilities.
Key Changes:
- New SSH.NET-based protocol implementation with terminal emulator (VtNetCore)
- Added
TraceMsgmessage class for detailed diagnostic logging - Comprehensive test suite (9 new test files, 2000+ lines)
- UI updates for trace message configuration in notification settings
Reviewed Changes
Copilot reviewed 37 out of 40 changed files in this pull request and generated 127 comments.
Show a summary per file
| File | Description |
|---|---|
ProtocolSSH_DotNet.cs |
Core SSH protocol implementation with connection management, I/O handling, and smart disconnect detection |
SshTerminalControl.cs |
Full-featured terminal control with VtNetCore integration, rendering, input handling, and copy/paste |
SSHConnectionManager.cs |
SSH connection factory and configuration utilities |
SSHAuthenticationProvider.cs |
Authentication method creation (password, key-based, keyboard-interactive) |
SSHDotNetDiagnostics.cs |
Comprehensive diagnostic logging system with configurable verbosity |
ShellStreamExtensions.cs |
Reflection-based terminal resize support |
| Test files (9 files) | Unit tests for protocol components with 2000+ lines of coverage |
MessageClassEnum.cs |
Added TraceMsg = 4 enum value |
NotificationsPage.* |
UI updates for trace message configuration |
| Message writers/filters | Added trace message handling support |
Directory.Packages.props |
Added VtNetCore 1.0.24 dependency |
Files not reviewed (3)
- mRemoteNG/Language/Language.Designer.cs: Language not supported
- mRemoteNG/Properties/OptionsNotificationsPage.Designer.cs: Language not supported
- mRemoteNG/UI/Forms/OptionsPages/NotificationsPage.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+101
to
+106
| foreach (var authMethod in client.ConnectionInfo.AuthenticationMethods) | ||
| { | ||
| if (authMethod.AllowedAuthentications != null && authMethod.AllowedAuthentications.Any()) | ||
| { | ||
| SSHDotNetDiagnostics.LogDebug($"Connection: Server allows: {string.Join(", ", authMethod.AllowedAuthentications)}"); | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Suggested change
| foreach (var authMethod in client.ConnectionInfo.AuthenticationMethods) | |
| { | |
| if (authMethod.AllowedAuthentications != null && authMethod.AllowedAuthentications.Any()) | |
| { | |
| SSHDotNetDiagnostics.LogDebug($"Connection: Server allows: {string.Join(", ", authMethod.AllowedAuthentications)}"); | |
| } | |
| foreach (var authMethod in client.ConnectionInfo.AuthenticationMethods | |
| .Where(am => am.AllowedAuthentications != null && am.AllowedAuthentications.Any())) | |
| { | |
| SSHDotNetDiagnostics.LogDebug($"Connection: Server allows: {string.Join(", ", authMethod.AllowedAuthentications)}"); |
Comment on lines
+1420
to
+1474
| foreach (var span in row.Spans) | ||
| { | ||
| if (span == null || string.IsNullOrEmpty(span.Text)) | ||
| continue; | ||
|
|
||
| // Parse colors from hex strings (VtNetCore format: #RRGGBB) | ||
| Color foreColor = ParseColor(span.ForgroundColor, _foregroundColor); | ||
| Color backColor = ParseColor(span.BackgroundColor, _backgroundColor); | ||
|
|
||
| // Apply bold attribute by using a bold font | ||
| Font renderFont = span.Bold ? new Font(font, FontStyle.Bold) : font; | ||
|
|
||
| try | ||
| { | ||
| // Calculate span width using FIXED character grid (not variable measurement) | ||
| // This is critical for terminal apps like htop that expect exact column alignment | ||
| int spanWidth = span.Text.Length * _charWidth; | ||
|
|
||
| // Draw background if different from default | ||
| if (backColor != _backgroundColor) | ||
| { | ||
| using (Brush backBrush = new SolidBrush(backColor)) | ||
| { | ||
| e.Graphics.FillRectangle(backBrush, x, y, spanWidth, _charHeight); | ||
| } | ||
| } | ||
|
|
||
| // Draw text with foreground color using TextRenderer for true monospace rendering | ||
| // TextRenderer ensures exact fixed-width character placement (no kerning/subpixel adjustments) | ||
| Rectangle textRect = new Rectangle(x, y, spanWidth, _charHeight); | ||
| TextFormatFlags textFlags = TextFormatFlags.NoPadding | TextFormatFlags.NoPrefix | TextFormatFlags.Left | TextFormatFlags.Top; | ||
| TextRenderer.DrawText(e.Graphics, span.Text, renderFont, textRect, foreColor, textFlags); | ||
|
|
||
| // Draw underline if attribute is set | ||
| if (span.Underline) | ||
| { | ||
| using (Pen underlinePen = new Pen(foreColor, 1)) | ||
| { | ||
| int underlineY = y + _charHeight - 2; | ||
| e.Graphics.DrawLine(underlinePen, x, underlineY, x + spanWidth, underlineY); | ||
| } | ||
| } | ||
|
|
||
| // Move x position for next span using FIXED character grid | ||
| x += spanWidth; | ||
| } | ||
| finally | ||
| { | ||
| // Dispose bold font if created | ||
| if (span.Bold && renderFont != font) | ||
| { | ||
| renderFont.Dispose(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Comment on lines
+132
to
+139
| foreach (var attr in attrs) | ||
| { | ||
| if (attr.GetType().Name.Contains("Extension")) | ||
| { | ||
| isExtension = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| const string hostname = "localhost"; | ||
| const int port = 22; | ||
| const string username = "testuser"; | ||
| var authMethod = new PasswordAuthenticationMethod(username, "password"); |
There was a problem hiding this comment.
Disposable 'PasswordAuthenticationMethod' is created but not disposed.
| const string hostname = "localhost"; | ||
| const int port = 22; | ||
| const string username = "testuser"; | ||
| var authMethod = new PasswordAuthenticationMethod(username, "password"); |
There was a problem hiding this comment.
Disposable 'PasswordAuthenticationMethod' is created but not disposed.
Comment on lines
+1578
to
+1581
| catch (Exception cursorEx) | ||
| { | ||
| SSHDotNetDiagnostics.LogException("Terminal: Error rendering cursor", cursorEx); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
Suggested change
| catch (Exception cursorEx) | |
| { | |
| SSHDotNetDiagnostics.LogException("Terminal: Error rendering cursor", cursorEx); | |
| } | |
| catch (ArgumentException cursorEx) | |
| { | |
| SSHDotNetDiagnostics.LogException("Terminal: Error rendering cursor (ArgumentException)", cursorEx); | |
| } | |
| catch (InvalidOperationException cursorEx) | |
| { | |
| SSHDotNetDiagnostics.LogException("Terminal: Error rendering cursor (InvalidOperationException)", cursorEx); | |
| } | |
| catch (ObjectDisposedException cursorEx) | |
| { | |
| SSHDotNetDiagnostics.LogException("Terminal: Error rendering cursor (ObjectDisposedException)", cursorEx); | |
| } |
Comment on lines
+1666
to
+1668
| catch (Exception ex) | ||
| { | ||
| SSHDotNetDiagnostics.LogException("Terminal: Error during disposal", ex); |
There was a problem hiding this comment.
Generic catch clause.
Suggested change
| catch (Exception ex) | |
| { | |
| SSHDotNetDiagnostics.LogException("Terminal: Error during disposal", ex); | |
| catch (ObjectDisposedException ex) | |
| { | |
| SSHDotNetDiagnostics.LogException("Terminal: Object already disposed during disposal", ex); | |
| } | |
| catch (InvalidOperationException ex) | |
| { | |
| SSHDotNetDiagnostics.LogException("Terminal: Invalid operation during disposal", ex); | |
| } | |
| // Catch all other exceptions to prevent disposal from crashing the application. | |
| // This is intentionally broad to ensure cleanup does not throw. | |
| catch (Exception ex) | |
| { | |
| SSHDotNetDiagnostics.LogException("Terminal: Unexpected error during disposal", ex); |
Comment on lines
+39
to
+42
| catch | ||
| { | ||
| // Ignore cleanup errors | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
Comment on lines
+39
to
+42
| catch | ||
| { | ||
| // Ignore cleanup errors | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
Comment on lines
+138
to
+145
| if (!string.IsNullOrEmpty(passphrase)) | ||
| { | ||
| keyFile = new PrivateKeyFile(keyStream, passphrase); | ||
| } | ||
| else | ||
| { | ||
| keyFile = new PrivateKeyFile(keyStream); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Suggested change
| if (!string.IsNullOrEmpty(passphrase)) | |
| { | |
| keyFile = new PrivateKeyFile(keyStream, passphrase); | |
| } | |
| else | |
| { | |
| keyFile = new PrivateKeyFile(keyStream); | |
| } | |
| keyFile = !string.IsNullOrEmpty(passphrase) | |
| ? new PrivateKeyFile(keyStream, passphrase) | |
| : new PrivateKeyFile(keyStream); |
|
@joubertdj: Any progress on it? |
Contributor
Author
Apart from AI's "response", you are the first human responding ... or are you human???? Have you tested it? |
Contributor
Author
|
Ok, I addressed some Codepilot items @Kvarkas @Neustradamus , but certain items are intended on purpose. So I left them. I also added now SSH tunneling for SSH_DotNet ... it works, but again, needs some test |
|
Contributor
Author
|
I do think we need to add it to one of the nightlies, so that users can maybe test it in bigger "footprint". I have not yet doen any SQL integration etc. There I would nee @Kvarkas 's input. |
Member
|
Technicaly to avoid delay I dont want to add new features to curent build, but will be happy to add to next, so still keeping this open |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add experimental SSH_DotNet protocol implementation
This PR introduces a new SSH_DotNet protocol implementation as an experimental feature for community testing and feedback. This does NOT replace the existing PuTTY SSH implementation - it adds a new protocol option for
testing purposes.
What's Been Achieved
✅ SSH terminal emulation using SSH.NET and VtNetCore
✅ Username/password authentication support
✅ Comprehensive trace logging system (SSHDotNetDiagnostics)
✅ 109 unit tests covering core functionality
✅ Terminal resize support via reflection-based window change requests
✅ Basic input/output flow between SSH stream and terminal (ctrl+ keys, direction keys, F keys, but other combinations needs to be tested)
Current Limitations
❌ Only username/password authentication supported (no private key/certificate authentication yet)
❌ Visual feedback items need sorting (UI/UX improvements pending)
❌ No integration tests (only unit tests for parameter validation and error handling)
❌ Not tested against production SSH servers
❌ Missing comprehensive error recovery
Architecture Overview
Testing Recommendations
Before considering this for production:
Files Changed
Next Steps
This is intended as a starting point for discussion and testing. Community feedback is essential before this can be considered production-ready.
Note: This feature was developed with assistance from Claude Code for codebase
analysis, implementation, code review, and comprehensive testing.
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com