FIX: Remove credential exposure from CI/CD pipelines - #715
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens CI/CD and devcontainer workflows to reduce credential exposure (avoiding bash xtrace, avoiding secrets in process arguments, and limiting secret persistence to restricted files/env).
Changes:
- Removes
-xtracing from multiple bash blocks (set -euo pipefail) and switches several Docker env injections to-e VAR(inherit by name). - Stops passing SQL passwords on
sqlcmdcommand lines by usingSQLCMDPASSWORD/SQLCMDPASSWORD-style env flows and stdin for sensitive SQL statements. - Updates devcontainer SQL startup to use a restricted
--env-fileand storesDB_CONNECTION_STRINGin a user-only env file instead of/etc/environment.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| OneBranchPipelines/stress-test-pipeline.yml | Reduces secret exposure during stress-test DB/container setup (env injection + SQLCMDPASSWORD). |
| OneBranchPipelines/stages/wheel-installation-test-windows-stage.yml | Avoids embedding DB passwords directly in the sqlcmd -Q command line. |
| OneBranchPipelines/stages/wheel-installation-test-stage.yml | Removes bash tracing and shifts Docker/sqlcmd password passing to inherited env vars. |
| OneBranchPipelines/stages/wheel-installation-test-macos-stage.yml | Uses inherited MSSQL_SA_PASSWORD / SQLCMDPASSWORD for container startup and readiness checks. |
| OneBranchPipelines/stages/build-windows-single-stage.yml | Avoids embedding DB passwords directly in the sqlcmd -Q command line. |
| OneBranchPipelines/stages/build-macos-single-stage.yml | Uses SQLCMDPASSWORD instead of -P for container readiness checks. |
| OneBranchPipelines/stages/build-linux-single-stage.yml | Removes bash tracing and shifts Docker/sqlcmd password passing to inherited env vars. |
| eng/pipelines/pypi-package-smoketest.yml | Stops echoing the connection string and passes it into containers via inherited env. |
| eng/pipelines/pr-validation-pipeline.yml | Moves SQL installer password to ACL-restricted config files and switches sqlcmd auth to SQLCMDPASSWORD. |
| .devcontainer/post-create.sh | Uses a restricted Docker env file and stores the connection string in a user-only env file sourced by shells. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Eliminate secret leakage via bash xtrace, process arguments, and world-readable files across ADO/OneBranch pipelines and the devcontainer: - Replace 'set -eux' with 'set -euo pipefail' in all secret-handling blocks and nested shells - Map secrets through step env: and reference shell vars instead of ADO \ macros - Use SQLCMDPASSWORD and stdin for sqlcmd; inherit Docker env vars by name (-e VAR) - Replace SQL Server installer /SAPWD= with an ACL-restricted /ConfigurationFile= deleted in finally - devcontainer: use --env-file (0600) for docker run and a user-only env file instead of world-readable /etc/environment
e0efe43 to
ebe74ee
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
devskim found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Avoid mutating the global umask; create per-file with owner-only perms via a scoped subshell and pre-write chmod.
|
Re: DevSkim ("20+ potential problems") — these are informational (non-blocking per repo convention) and are the expected local-test-scaffolding patterns: |
The PowerShell here-string content sat at column 0, which dedented out of the YAML '|' block and made the ADO parser reject the pipeline. Replace it with an indented PowerShell array written via Set-Content, keeping clean INI output.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 64.1%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 78.3%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 82.5%
mssql_python.row.py: 83.4%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%🔗 Quick Links
|
|
Copilot resolve the merge conflicts in this pull request |
…ial-exposure # Conflicts: # eng/pipelines/pr-validation-pipeline.yml Co-authored-by: gargsaumya <192222169+gargsaumya@users.noreply.github.com>
Merge conflicts resolved in 7d54c59 (true merge commit of Only
|
Escape single quotes before embedding DB_PASSWORD in CREATE LOGIN, disable sqlcmd variable expansion, and make Windows setup fail immediately when sqlcmd rejects the login statement.
Windows PowerShell 5.1 does not reliably pipe the generated SQL text to native sqlcmd. Start sqlcmd directly and write to its redirected standard input so the password stays off the command line and the login statement is encoded correctly.
Azure's Windows PowerShell 5.1 environment writes a UTF-8 BOM through Process.StandardInput, which SQL Server rejects before CREATE LOGIN. Encode the SQL explicitly as BOM-free UTF-8 bytes and tell sqlcmd to read code page 65001.
Process.StandardInput constructs a StreamWriter that can emit its own BOM before raw writes on hosted Windows agents. Write the login statement to a restricted BOM-free UTF-8 file, pass only its path to sqlcmd, and remove it in a finally block.
There was a problem hiding this comment.
🟡 Changes recommended
The SQL Server readiness loops in eng/pipelines/pr-validation-pipeline.yml rely on try/catch around sqlcmd, which won’t catch native-process failures in Windows PowerShell and can lead to infinite waits.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
eng/pipelines/pr-validation-pipeline.yml:270
- Same issue as the SQL2022 readiness loop:
try/catchwon’t catch a failedsqlcmdnative-process exit code in Windows PowerShell, so$attemptwon’t increment and the loop can run forever. Switch to-bplus an explicit$LASTEXITCODEcheck for retries.
Write-Host "Waiting for SQL Server 2025 to start..."
while (-not $connected -and $attempt -lt $maxAttempts) {
try {
sqlcmd -S "localhost" -U "sa" -Q "SELECT 1" -C
$connected = $true
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
…ial-exposure # Conflicts: # eng/pipelines/pr-validation-pipeline.yml Co-authored-by: gargsaumya <192222169+gargsaumya@users.noreply.github.com>
Resolved in merge commit |
PR Performance ReportNo consistent slowdowns detected across all 2 environments. Coverage: 2 of 2 environments completed. Advisory result; does not block merging.
Affected phases and call countsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. No affected phases or call-count changes were recorded. All database tasks and timingsUnix / SQL Server 2022
Unix / SQL Server 2025
Build, commits and measurement detailsPR head:
A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent. The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes. Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency. Raw samples and logs are attached to the ADO run as |
There was a problem hiding this comment.
🔵 Needs a closer look
Four moderate issues remain involving secret-file cleanup, file permissions, and configuration-path quoting.
Review details
Suppressed comments (4)
.devcontainer/post-create.sh:58
- If the post-create script is interrupted while
docker runis starting, this secret-bearing temp file is never removed because cleanup runs only afterdocker runreturns. Register an EXIT/termination cleanup trap immediately aftermktempso an interrupted setup does not leaveMSSQL_SA_PASSWORDin/tmp.
SQL_ENV_FILE="$(mktemp)"
chmod 600 "$SQL_ENV_FILE"
printf 'ACCEPT_EULA=Y\nMSSQL_SA_PASSWORD=%s\n' "$SA_PASSWORD" > "$SQL_ENV_FILE"
.devcontainer/post-create.sh:95
- The secret is written before the final
chmod 600. On a rerun where.mssql_python_envalready has permissive permissions,umaskdoes not change that existing mode, so the new password is briefly written while other users may be able to read it. Create/restrict the file before theprintf(inside the scoped-umask block).
( umask 077; printf "export DB_CONNECTION_STRING='%s'\n" "$DB_CONNECTION_STRING" > "$MSSQL_ENV_FILE" )
chmod 600 "$MSSQL_ENV_FILE"
eng/pipelines/pr-validation-pipeline.yml:261
- This SQL Server 2025 setup invocation has the same path-parsing failure: an unquoted
$configPathis split when the temp directory contains spaces, so setup cannot find the configuration file. Pass the argument with escaped quotes around the path.
Start-Process -FilePath "$env:TEMP\SQL2025Setup\setup.exe" -ArgumentList "/ConfigurationFile=$configPath" -Wait
eng/pipelines/pr-validation-pipeline.yml:155
- The
/ConfigurationFile=argument is passed without quoting the path.Start-Processbuilds a command line, so ifTEMPcontains spaces (for example, on a self-hosted agent), the installer receives a truncated path and cannot load the INI. Include escaped quotes around$configPathin the argument; apply the same fix to the SQL Server 2025 invocation below.
Start-Process -FilePath "$env:TEMP\SQLSetup\setup.exe" -ArgumentList "/ConfigurationFile=$configPath" -Wait
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Ensure legacy credential cleanup runs before failure-prone setup or during exit cleanup.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.devcontainer/post-create.sh:105
- These migration cleanups run only after the failure-prone dependency installation, native build, password generation, and Docker setup. Because the script uses
set -e, an early failure leaves the old/etc/environment, shell-RC, or/tmp/.sqlserver_sa_passwordcredential in place, so an upgrade does not guarantee plaintext removal. Run this legacy cleanup before those operations or register it in the exit cleanup path.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Copilot resolve the merge conflicts in this pull request |
…ial-exposure # Conflicts: # eng/pipelines/pr-validation-pipeline.yml Co-authored-by: gargsaumya <192222169+gargsaumya@users.noreply.github.com>
4d73928
Resolved in merge commit |
There was a problem hiding this comment.
🔵 Needs a closer look
The two moderate findings in eng/pipelines/pr-validation-pipeline.yml must be addressed before approval.
Review details
Suppressed comments (2)
eng/pipelines/pr-validation-pipeline.yml:155
Set-Contentreports write failures as non-terminating errors by default, andStart-Process -Waitdoes not throw for a non-zero installer exit. If the credential-bearing INI cannot be written or setup fails, this step can still print installation completed and let later steps run against an unconfigured instance. Make the write terminating and check the installer exit code before cleanup.
Set-Content -Path $configPath -Value $configLines -Encoding ASCII
Start-Process -FilePath "$env:TEMP\SQLSetup\setup.exe" -ArgumentList "/ConfigurationFile=`"$configPath`"" -Wait
eng/pipelines/pr-validation-pipeline.yml:261
Set-Contentreports write failures as non-terminating errors by default, andStart-Process -Waitdoes not throw for a non-zero installer exit. If the credential-bearing INI cannot be written or setup fails, this step can still print installation completed and let later steps run against an unconfigured instance. Make the write terminating and check the installer exit code before cleanup.
Set-Content -Path $configPath -Value $configLines -Encoding ASCII
Start-Process -FilePath "$env:TEMP\SQL2025Setup\setup.exe" -ArgumentList "/ConfigurationFile=`"$configPath`"" -Wait
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Work Item / Issue Reference
Summary
This pull request improves the handling of sensitive credentials and environment variables for SQL Server setup and testing across development scripts and CI pipelines. The changes enhance security by avoiding plaintext password exposure, standardize environment variable usage, and make scripts more robust and portable.
Temporary credential files now use owner-only permissions, unique names where applicable, termination-safe cleanup, and fail-closed deletion. SQL Server setup configuration paths are quoted so temporary directories containing spaces remain supported.
Credential Management and Security Improvements:
.devcontainer/post-create.sh, SQL Server passwords are now written to a secure temporary environment file (umask 077), and the credentials are kept in a user-only file (~/.mssql_python_env). The script also removes plaintext credentials from/etc/environmentand shell RC files, and ensures credentials are sourced securely in new shell sessions. [1] [2]MSSQL_SA_PASSWORD,SQLCMDPASSWORD) instead of command-line arguments, reducing the risk of leaking secrets in process lists or logs. [1] [2] [3] [4]Pipeline and Script Robustness:
build-linux-single-stage.yml,stress-test-pipeline.yml) have been updated to use stricter error handling (set -euo pipefail) for improved reliability and easier debugging. [1] [2] [3] [4] [5] [6] [7] [8]Cross-Platform and Pipeline Consistency:
sqlcmd, including for database and user creation. This includes changes to how PowerShell scripts interpolate passwords, and how SQL Server is installed/configured on Windows using a secure configuration file. [1] [2] [3] [4] [5] [6] [7] [8] [9]Backward Compatibility and Cleanup:
These changes collectively make the development and CI environments more secure and maintainable by standardizing credential handling and improving script reliability.