Skip to content

Fix concurrent SSH authentication state and redact secrets#102

Open
binaryfire wants to merge 1 commit into
php:masterfrom
binaryfire:fix/ssh2-authentication-state
Open

Fix concurrent SSH authentication state and redact secrets#102
binaryfire wants to merge 1 commit into
php:masterfrom
binaryfire:fix/ssh2-authentication-state

Conversation

@binaryfire

Copy link
Copy Markdown

Summary

  • keep keyboard-interactive passwords on the owning SSH session instead of in process-global state
  • clear borrowed password state immediately after authentication
  • mark authentication secrets with SensitiveParameter metadata on PHP 8.2+
  • copy keyboard-interactive responses by length and initialize every response entry

The old global password pointer allowed concurrent authentication attempts to observe each other's credentials. Session-local state removes that shared mutable state without changing the public API.

The PHPT covers parameter metadata and trace redaction. The concurrency property follows from the state ownership change; the test suite does not have a deterministic multi-threaded SSH harness. Response initialization also avoids depending on libssh2's current zero-allocation behavior.

Testing

  • built the extension against PHP 8.4 and libssh2 1.11
  • ran the authentication and sensitive-parameter PHPTs
  • ran the complete test suite against a local OpenSSH fixture and compared its existing fixture-specific failures with an unchanged master build

Move keyboard-interactive password state from a process-global pointer to the owning SSH session so concurrent authentication attempts cannot share credentials. Copy callback responses using the session allocator contract and clear borrowed password state immediately after authentication.

Mark authentication secrets with SensitiveParameter metadata on PHP 8.2 and newer. Add PHPT coverage for sensitive and ordinary parameter metadata, trace redaction, and the distinction between in-memory private keys and key paths.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant