Enhanced SOCKS5 implementation and username validation improvements

[derekg]

Summary

This PR enhances ts-ssh with SOCKS5 dynamic port forwarding, improved username validation, and PTY control features. All changes maintain the project's simplicity philosophy while adding essential functionality for modern SSH workflows.

Changes Included

SOCKS5 Dynamic Port Forwarding (Enhanced PR #33)

• ✅ Added -D [bind_address:]port flag for SOCKS5 proxy support
• ✅ Full SOCKS5 protocol implementation (handshake, IPv4/IPv6/domain names)
• ✅ Security validation for bind addresses with warnings for non-localhost binds
• ✅ Improved error handling - distinguishes normal shutdown from errors
• ✅ Context-based lifecycle management for graceful shutdown
• ✅ Comprehensive test coverage - 5 new test suites (355 lines of tests)
• ✅ Compatible with VSCode Remote SSH and other SOCKS proxy tools

Username Validation Enhancement (PR #34)

• ✅ Allow dots in SSH usernames (e.g., first.last)
• ✅ Updated regex validation: ^[a-zA-Z0-9_\-\.]+$
• ✅ Added test cases for dot-containing usernames
• ✅ Common Unix username format now fully supported

PTY Control

• ✅ Added -T flag to disable pseudo-terminal allocation
• ✅ Useful for non-interactive commands and automation
• ✅ Follows standard SSH conventions

Documentation

• ✅ Updated README.md with SOCKS5 examples and security notes
• ✅ Updated CLAUDE.md with new features and usage
• ✅ Added debugging workflows for SOCKS5
• ✅ Documented all new flags and options

Security Improvements

Bind Address Validation:

• Validates bind addresses are valid IPs or localhost
• Warns when binding to non-localhost (0.0.0.0, network IPs)
• Prevents unintended network exposure

Error Handling:

• Better goroutine lifecycle management
• Context-aware shutdown detection
• More informative error messages

Testing

Added comprehensive test coverage:

• TestParseDynamicForwardSpec - Forward spec parsing
• TestSOCKS5AddressParsing - IPv4/IPv6/domain parsing
• TestSOCKS5ProtocolVersions - Protocol version validation
• TestSOCKS5Commands - Command support testing
• TestBindAddressSecurity - Security validation testing

Code Quality

• ✅ All code formatted with go fmt
• ✅ No breaking changes to existing functionality
• ✅ Maintains simplicity philosophy
• ✅ Clear, descriptive commit messages

Examples

SOCKS5 Proxy

# Start SOCKS5 proxy on localhost:1080
ts-ssh -D 1080 hostname

# Bind to specific address with warning
ts-ssh -D 0.0.0.0:1080 hostname

# Combine with other options
ts-ssh -D 1080 -p 2222 user@hostname
Username with Dots
# Now supported
ts-ssh first.last@hostname
ts-ssh -l john.doe hostname
Disable PTY
# Non-interactive commands
ts-ssh -T hostname "cat /etc/hostname"

[claude]

Claude finished @derekg's task —— View job

---

Pull Request Review

Reviewing PR #35: Enhanced SOCKS5 implementation and username validation improvements

🔍 Overall Assessment: EXCELLENT

This PR successfully adds significant functionality while maintaining the project's core philosophy of simplicity. The implementation is well-structured, thoroughly tested, and properly documented.

✅ Strengths

Code Quality & Architecture

• Clean SOCKS5 implementation: The protocol handling in handleSOCKS5() correctly implements the SOCKS5 RFC with proper version checking, address parsing, and response codes
• Excellent error handling: Proper error responses (0x05, 0x07 for general failure, 0x05, 0x05 for connection refused) and graceful degradation
• Context-aware lifecycle management: Using context.WithCancel() for clean shutdown detection at main.go:517-544
• Security-first approach: Input validation on bind addresses and ports before starting listeners

Security Enhancements

• Bind address validation: Smart warnings for non-localhost binds (main.go:493-502) without blocking legitimate use cases
• Username validation improvement: The regex change from ^[a-zA-Z0-9_\-]+$ to ^[a-zA-Z0-9_\-\.]+$ properly supports dotted usernames (validation.go:192)
• Port validation: Proper range checking (1-65535) with appropriate error messages

Test Coverage

• Comprehensive SOCKS5 tests: 5 new test functions covering protocol versions, address parsing, commands, and security validation
• Edge case coverage: Tests for IPv4, IPv6, domain names, and malformed requests
• Security test coverage: TestBindAddressSecurity validates the warning system for network-exposed binds

⚠️ Issues Found

Critical Issues

None - this is high-quality defensive security code.

Minor Issues

1. IPv6 Address Parsing Edge Case (main.go:627-636)

   case 0x04: // IPv6
       if n < 22 {
           // ... error handling
       }
       host = net.IP(buf[4:20]).String()

   The IPv6 parsing assumes 16 bytes but should verify the buffer contains exactly those bytes. Consider adding bounds checking.

2. Dynamic Forward Spec Parsing (main.go:476-483)

   if strings.Contains(forwardSpec, ":") {
       parts := strings.Split(forwardSpec, ":")
       if len(parts) != 2 {
           return fmt.Errorf("invalid dynamic forward specification: %s", forwardSpec)
       }

   This doesn't handle IPv6 bind addresses like [::1]:1080. While not critical since the current implementation works for intended use cases, it could be enhanced.

3. Goroutine Cleanup (main.go:520-544)
   The goroutine handling accepts connections but there's no explicit cleanup mechanism for individual connection handlers. This is acceptable for the current scope but worth noting.

📊 Performance Considerations

Positive

• Efficient I/O handling: Uses io.Copy() for bidirectional data transfer (main.go:674-681)
• Non-blocking accept loop: Proper goroutine spawning for each connection
• Minimal memory allocation: Reuses buffers appropriately

Considerations

• Connection limits: No explicit limit on concurrent SOCKS5 connections (acceptable for personal use)
• Buffer sizing: 256-byte buffer is reasonable for SOCKS5 handshake but could be optimized

🔐 Security Assessment

Excellent Security Practices

1. Input validation: All user inputs (ports, bind addresses, usernames) are validated
2. Injection prevention: Proper escaping and validation prevent command injection
3. Network exposure warnings: Users are warned when binding to non-localhost addresses
4. Error information leakage: Minimal error details exposed to prevent reconnaissance

Security Considerations

• SOCKS5 authentication: Currently implements "no authentication" method (0x00), which is appropriate for local use
• Connection logging: Verbose mode provides good visibility without exposing sensitive data

📚 Documentation Quality

Strengths

• Comprehensive README updates: Clear examples and security warnings
• CLAUDE.md maintenance: Proper documentation of new features and debugging workflows
• Code comments: Well-documented functions with clear purpose statements

🧪 Test Quality Assessment

The test coverage is exemplary:

• TestParseDynamicForwardSpec: Covers parsing edge cases including IPv6
• TestSOCKS5AddressParsing: Tests IPv4, IPv6, and domain name parsing
• TestSOCKS5ProtocolVersions: Validates version handling
• TestSOCKS5Commands: Tests command support (CONNECT vs unsupported)
• TestBindAddressSecurity: Validates security warning system

📋 Recommendations

For This PR ✅ APPROVED

The code is ready to merge as-is. The minor issues identified are enhancements, not blockers.

Future Enhancements (optional)

1. Consider IPv6 bind address support for completeness
2. Add connection limiting for production deployments
3. Consider SOCKS5 username/password authentication for enhanced security scenarios

🎯 Conclusion

This PR exemplifies excellent software engineering:

• ✅ Maintains project simplicity while adding valuable functionality
• ✅ Comprehensive security validation and testing
• ✅ Clean, readable, and maintainable code
• ✅ Thorough documentation and examples
• ✅ No breaking changes to existing functionality

The SOCKS5 implementation follows RFC standards, includes proper error handling, and adds genuine value for SSH workflows like VSCode Remote SSH. The username validation enhancement addresses a real limitation.

Recommendation: MERGE 🚀

---