Skip to content

feat: mssql pam#147

Open
sheensantoscapadngan wants to merge 3 commits intomainfrom
feat/mssql-pam
Open

feat: mssql pam#147
sheensantoscapadngan wants to merge 3 commits intomainfrom
feat/mssql-pam

Conversation

@sheensantoscapadngan
Copy link
Member

@sheensantoscapadngan sheensantoscapadngan commented Mar 12, 2026

Description 📣

This PR adds support for MsSQL PAM

Relevant PR:
Infisical/infisical#5676

Type ✨

  • Bug fix
  • New feature
  • Improvement
  • Breaking change
  • Documentation

Tests 🛠️

# Here's some code block to paste some code snippets

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 12, 2026

Greptile Summary

This PR adds MSSQL PAM (Privileged Access Management) proxy support to the Infisical CLI, implementing the TDS protocol from scratch to intercept client connections, inject managed credentials, and log SQL queries. The integration follows the same structural patterns as the existing PostgreSQL, MySQL, and Redis handlers.

Key issues found:

  • TLS downgrade vulnerability (proxy.go): The EnableTLS field is set in config but never consulted in the proxy. Unlike all other handlers (Postgres, MySQL, Redis), the MSSQL proxy makes its TLS decision solely from the server's PRELOGIN response. If a server advertises EncryptOff, the connection silently falls back to plaintext even when credentials.SSLEnabled=true. The fix is to error out when EnableTLS=true but the server declines encryption, mirroring the Postgres handler.
  • Audit log accuracy: SQL queries are always logged with Output: "OK" regardless of whether the server returned an error. This could misrepresent failed queries in compliance audit trails.
  • ContainsToken false-positive risk: The naive byte scan for 0xAA (TokenError) in login responses could produce false authentication failures, since 0xAA is common in UTF-16LE-encoded text within TDS response payloads.
  • No documentation: No documentation was found in the /docs folder for this feature. How will customers discover MSSQL PAM support and understand its limitations (e.g., no RPC/stored procedure support, no Windows Auth)?
  • Minor: binary.Write error not checked in Encode(), and the injected username is not included in the structured log output per the team's logging convention.

Confidence Score: 2/5

  • Not safe to merge without addressing the TLS downgrade issue — EnableTLS is silently ignored, allowing plaintext connections when encryption is expected.
  • The TLS downgrade bug is a genuine security regression: every other handler in the codebase enforces TLS when SSLEnabled=true, but the MSSQL handler allows a server to silently negotiate it away. The audit log accuracy issue (always logging "OK") is a functional correctness concern for compliance use cases. Combined with missing documentation and the ContainsToken reliability concern, the PR needs fixes before merging.
  • packages/pam/handlers/mssql/proxy.go requires the most attention — specifically the EnableTLS enforcement gap and the correlated request/response logging logic.

Important Files Changed

Filename Overview
packages/pam/handlers/mssql/proxy.go Core MSSQL proxy implementation. Critical issue: EnableTLS config field is defined but never consulted — TLS is negotiated purely based on server's PRELOGIN response, allowing TLS downgrade when SSLEnabled=true. SQL query responses are always logged as "OK" regardless of actual server outcome.
packages/pam/handlers/mssql/tds.go TDS protocol implementation. ContainsToken uses a naive linear byte scan which could produce false positives in edge cases; acknowledged in comments. binary.Write error is unchecked in Encode() (safe for bytes.Buffer but inconsistent with Go idioms). PRELOGIN/LOGIN7 parsing and password mangling appear correct.
packages/pam/pam-proxy.go Adds MSSQL case to HandlePAMProxy switch. Follows the same pattern as MySQL/Postgres/Redis handlers. No major issues.
packages/pam/session/uploader.go Adds ResourceTypeMssql constant and updates the session filename regex pattern to include mssql. Straightforward and correct.
packages/pam/local/database-proxy.go Adds MSSQL connection string display case (sqlserver://...). Simple and correct, follows existing pattern.

Last reviewed commit: 0cb9a5e

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