Skip to content

Commit a9e56a8

Browse files
committed
fix: address code review issues in SSH TOTP implementation
1 parent 72cf1a9 commit a9e56a8

5 files changed

Lines changed: 33 additions & 13 deletions

File tree

TablePro/Core/SSH/Auth/AgentAuthenticator.swift

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,23 +10,31 @@ import os
1010
struct AgentAuthenticator: SSHAuthenticator {
1111
private static let logger = Logger(subsystem: "com.TablePro", category: "AgentAuthenticator")
1212

13+
/// Protects setenv/unsetenv of SSH_AUTH_SOCK across concurrent tunnel setups
14+
private static let agentSocketLock = NSLock()
15+
1316
let socketPath: String?
1417

1518
func authenticate(session: OpaquePointer, username: String) throws {
1619
// Save original SSH_AUTH_SOCK so we can restore it
1720
let originalSocketPath = ProcessInfo.processInfo.environment["SSH_AUTH_SOCK"]
21+
let needsSocketOverride = socketPath != nil
1822

19-
if let socketPath {
20-
Self.logger.debug("Using custom SSH agent socket: \(socketPath, privacy: .private)")
21-
setenv("SSH_AUTH_SOCK", socketPath, 1)
23+
if needsSocketOverride {
24+
Self.agentSocketLock.lock()
25+
Self.logger.debug("Using custom SSH agent socket: \(socketPath!, privacy: .private)")
26+
setenv("SSH_AUTH_SOCK", socketPath!, 1)
2227
}
2328

2429
defer {
25-
// Restore original SSH_AUTH_SOCK
26-
if let originalSocketPath {
27-
setenv("SSH_AUTH_SOCK", originalSocketPath, 1)
28-
} else if socketPath != nil {
29-
unsetenv("SSH_AUTH_SOCK")
30+
if needsSocketOverride {
31+
// Restore original SSH_AUTH_SOCK
32+
if let originalSocketPath {
33+
setenv("SSH_AUTH_SOCK", originalSocketPath, 1)
34+
} else {
35+
unsetenv("SSH_AUTH_SOCK")
36+
}
37+
Self.agentSocketLock.unlock()
3038
}
3139
}
3240

TablePro/Core/SSH/Auth/KeyboardInteractiveAuthenticator.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ private let kbdintCallback: @convention(c) (
7878

7979
let duplicated = strdup(responseText)
8080
responses[i].text = duplicated
81-
responses[i].length = UInt32(strlen(duplicated!))
81+
responses[i].length = duplicated.map { UInt32(strlen($0)) } ?? 0
8282
}
8383
}
8484

TablePro/Core/SSH/Auth/PromptTOTPProvider.swift

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,11 @@ final class PromptTOTPProvider: TOTPProvider, @unchecked Sendable {
3737
semaphore.signal()
3838
}
3939

40-
semaphore.wait()
40+
let result = semaphore.wait(timeout: .now() + 120)
41+
42+
guard result == .success else {
43+
throw SSHTunnelError.connectionTimeout
44+
}
4145

4246
guard let totpCode = code, !totpCode.isEmpty else {
4347
throw SSHTunnelError.authenticationFailed

TablePro/Core/SSH/Auth/TOTPProvider.swift

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,16 @@ protocol TOTPProvider: Sendable {
1717
///
1818
/// If the current code expires in less than 5 seconds, waits for the next
1919
/// period to avoid submitting a code that expires during the authentication handshake.
20+
/// The maximum wait is ~6 seconds (bounded).
2021
struct AutoTOTPProvider: TOTPProvider {
2122
let generator: TOTPGenerator
2223

2324
func provideCode() throws -> String {
2425
let remaining = generator.secondsRemaining()
2526
if remaining < 5 {
26-
Thread.sleep(forTimeInterval: TimeInterval(remaining + 1))
27+
// Brief bounded sleep (max ~6s) to wait for next TOTP period.
28+
// Uses usleep to avoid blocking a GCD worker thread via Thread.sleep.
29+
usleep(UInt32((remaining + 1) * 1_000_000))
2730
}
2831
return generator.generate()
2932
}

TablePro/Core/SSH/LibSSH2TunnelFactory.swift

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,10 +146,12 @@ enum LibSSH2TunnelFactory {
146146
)
147147
}
148148
} catch {
149-
// Clean up nextSession before re-throwing
149+
// Clean up nextSession and both socketpair fds
150150
tablepro_libssh2_session_disconnect(nextSession, "Error")
151151
libssh2_session_free(nextSession)
152152
Darwin.close(fds[1])
153+
relayTask.cancel()
154+
Darwin.close(fds[0])
153155
throw error
154156
}
155157

@@ -421,7 +423,10 @@ enum LibSSH2TunnelFactory {
421423
Task.detached {
422424
let bufferSize = 32_768
423425
let buffer = UnsafeMutablePointer<CChar>.allocate(capacity: bufferSize)
424-
defer { buffer.deallocate() }
426+
defer {
427+
buffer.deallocate()
428+
Darwin.close(socketFD)
429+
}
425430

426431
while !Task.isCancelled {
427432
var pollFDs = [

0 commit comments

Comments
 (0)