feat(pam): NTLM authentication for MSSQL proxy #241
Closed
Claude / Claude Code Review
completed
May 20, 2026 in 13m 5s
Code review found 1 important issue
Found 5 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | packages/pam/handlers/mssql/proxy.go:336-338 |
NTLM challenge ContainsToken byte-scan triggers spurious auth failures |
| 🟡 Nit | packages/pam/handlers/mssql/tds.go:411-422 |
SSPILength boundary: 65535 uses sentinel value, server reads zero bytes |
Annotations
Check failure on line 338 in packages/pam/handlers/mssql/proxy.go
claude / Claude Code Review
NTLM challenge ContainsToken byte-scan triggers spurious auth failures
The new `authenticateNTLM` path checks for server errors with `ContainsToken(challengePayload, TokenError)` (proxy.go:337), but `ContainsToken` is a naive byte scan and the challenge payload is mostly opaque binary (random 8-byte `ServerChallenge`, FILETIME timestamps, `NegotiateFlags`, etc.). Whenever any of those bytes happens to equal `0xAA`, the proxy returns "server rejected NTLM negotiate" even though the server sent a valid challenge — yielding intermittent NTLM auth failures (roughly 3%+
Check warning on line 422 in packages/pam/handlers/mssql/tds.go
claude / Claude Code Review
SSPILength boundary: 65535 uses sentinel value, server reads zero bytes
**nit**: In `Login7Message.Encode` (tds.go:411-422), the SSPI length boundary uses `if sspiLen <= 65535`, which sets `SSPILength = 0xFFFF` and `SSPILongLength = 0` when `sspiLen` is exactly 65535. Per MS-TDS 2.2.6.4, `cbSSPI = 0xFFFF` is a sentinel directing the server to read `cbSSPILong` for the real length — so a 65535-byte payload would be interpreted as a 0-byte SSPI blob. In practice this boundary is unreachable (go-ntlmssp negotiate/authenticate messages are well under 2KB), but the fix i
Loading