Skip to content

Rustls vulnerability fix#419

Open
jakewimmer wants to merge 4 commits into
prisma:mainfrom
jakewimmer:rustls-vuln-fix
Open

Rustls vulnerability fix#419
jakewimmer wants to merge 4 commits into
prisma:mainfrom
jakewimmer:rustls-vuln-fix

Conversation

@jakewimmer
Copy link
Copy Markdown

@jakewimmer jakewimmer commented May 12, 2026

Background: I'm currently using tiberius in an in-house application at work and hit a wall with
cargo deny after attempting to execute a switch from native TLS to rustls and picking up some
open vulnerabilities.

Anyways, one rabbithole later and here I am with a Pull Request.

If my work isn't too shoddy, I'd be happy to help address the (existing) clippy errors and update
some of the other dependencies.

Summary

Vulnerability fixes

Bumps tokio-rustls 0.24->0.26 (rustls 0.21->0.23) and rustls-native-certs 0.6->0.8 to pull in
rustls-webpki 0.103.13, fixing RUSTSEC-2026-0098, RUSTSEC-2026-0099, and RUSTSEC-2026-0104.

  • Drop rustls-pemfile; PEM parsing now goes through the PemObject trait in rustls-pki-types
    (transitive from tokio-rustls 0.26).
  • Migrate src/client/tls_stream/rustls_tls_stream.rs to the rustls 0.23 API: updated
    NoCertVerifier for the new ServerCertVerifier trait (verify_tls13_signature,
    supported_verify_schemes, Debug bound); switched from Certificate to CertificateDer;
    updated ConfigBuilderExt for rustls-native-certs 0.8.
  • Gate TlsPreloginWrapper, its impls, and all TLS-only imports in src/client/tls.rs behind
    #[cfg(any(feature = "rustls", feature = "native-tls", feature = "vendored-openssl"))]. Same
    gate on PacketHeader::set_type in src/tds/codec/header.rs. Without this,
    --no-default-features builds fail under RUSTFLAGS="-Dwarnings" with dead-code and
    unused-import errors.
  • Switch the crypto provider to aws-lc-rs via builder_with_provider to avoid the
    dual-provider conflict that tokio-rustls 0.26 introduces when ring is also in the
    dependency graph.
  • Pin to TLS 1.2 (with_protocol_versions(&[&version::TLS12])) to prevent TLS 1.3 KeyUpdate
    messages from triggering UnexpectedEof on the macOS CI runner.
  • Bump azure_identity 0.5.0->0.20.0 in dev-dependencies, also closing RUSTSEC-2026-0098,
    RUSTSEC-2026-0099, and RUSTSEC-2026-0104 in the dev build as well. The 0.20.0 API still has
    client_credentials_flow::perform; call-site changes are minimal (raw &str params,
    to_owned() over to_string()). Note that this library is very out-of-date, and bringing it
    up to speed would require a full rewrite of examples/aad-auth.rs.

No public API changes. All rustls code is behind the rustls/native-tls/vendored-openssl
feature gates.

CI fixes

Replacing the archived actions-rs/* actions shook loose several things that had already broken:

  • Replace archived actions-rs/* actions (Node.js 12) with dtolnay/rust-toolchain and inline
    cargo commands. Pin all action SHAs. Add persist-credentials: false to all checkout steps.
  • Switch from docker-compose v1 (Python binary, gone from GitHub runners) to docker compose
    v2 plugin.
  • Fix psmodulecache v6.2.1 API: the action handles caching internally now and no longer outputs
    modulepath/keygen. Drop the actions/cache step that was failing with "required input not
    provided: key".
  • Renew the test server certificate (expired 2024-12-20). New expiry: 2027-11-02, within the CA's
    2027-11-14 limit. Updated generate-signed-cert.sh with an optional validity-days argument
    (default 1825) for future renewals.
  • Fix Dockerfile certificate setup across all SQL Server versions. COPY --chmod requires
    BuildKit, which Lima-based Docker on macOS lacks; switch to COPY + RUN chmod. MSSQL base
    images end with USER mssql, but COPY produces root-owned files regardless; add USER root
    before file operations. COPY --chown=mssql fails on Lima's classic builder (can't resolve
    named users); use RUN chown instead. SQL Server 2017 has no mssql system user (only 2019+
    images do); drop USER mssql from the 2017 Dockerfile.
  • Migrate the macOS runner to macos-26-intel and add docker/setup-docker-action v5.1.0 (the
    official successor to crazy-max/ghaction-setup-docker, same SHA). Add
    brew install docker-compose; the setup action doesn't bundle the compose plugin on macOS.
  • Replace all manual cargo cache steps with Swatinem/rust-cache v2.9.1. The Linux job
    previously keyed on ${{ runner.os }}-cargo-${{ matrix.features }} with no Cargo.lock hash;
    macOS had no cache at all. Add a sanitization step that replaces commas with + in the matrix
    feature string, since GitHub Actions rejects commas in cache keys and --features=rustls,chrono
    contains them.

Test fixes

Two tests written before #[test_on_runtimes] existed and never updated:

  • cyrillic_collations_should_work previously created a dedicated database with a Cyrillic
    default collation, requiring an admin connection and DROP DATABASE at teardown. The DROP
    raced against open connections on macOS/rustls CI. Replace with a session-local temp table using
    column-level COLLATE clauses. The code path under test (COLMETADATA collation ->
    encoding_rs decode in column_data.rs) is the same: SQL Server sends the effective collation
    per-column regardless of whether it's database-inherited or set explicitly.
  • application_name_should_be_set_correctly needed the application name set before connecting.
    Add APP_NAME_CONN_STR with the name baked into the connection string so the macro-generated
    harness connects with it set.

Known remaining debt (out of scope)

cargo clippy warnings

21 warnings, all pre-existing in code this PR didn't touch:

  • 7x casting FeatureLevel::* to u8 will truncate the value (src/macros.rs:22): all variants
    cast to u8 in a format macro; needs #[repr(u8)] or a proper impl fmt::Display.
  • 4x doc list item without indentation (src/lib.rs): doc comment formatting.
  • 2x this can be std::io::Error::other(_) (src/client/tls.rs, src/tds/codec/decode.rs):
    legacy io::Error::new(io::ErrorKind::Other, ...) calls.
  • 2x usage of a legacy numeric method (src/tds/codec/header.rs, src/tds/numeric.rs):
    integer primitive .pow() calls with a newer form available.
  • 1x manually reimplementing div_ceil (src/tds/codec/token/token_row.rs:180)

macos runner stability

The current macOS CI is on borrowed time. Intel runner pools are shrinking (macos-13 is already
gone, macos-26-intel is the current fallback), and Apple Silicon runners can't run SQL Server
at all: Lima needs nested virtualization to handle x86_64 containers, and GitHub's arm64 runners
don't support it. Microsoft doesn't ship an arm64 image, so there's no current way around this.

The Intel + Lima setup works, but only just. No BuildKit support, leading to explicit permission
workarounds just to run chmod in a Dockerfile. The bulk tests (tests/bulk.rs) fail on nearly
every macOS CI run: all tests collapse within the same second with UnexpectedEof/failed to fill whole buffer,
identically on both the rustls and vendored-openssl paths. TLS version is probably not the cause:
the TLS 1.2 pin in rustls_tls_stream.rs targets SQL Server dropping connections on TLS 1.3
KeyUpdate receipt, which produces sporadic per-connection failures spread across a run, not a
simultaneous collapse. The container is dying, likely Lima networking instability or OOM under
bulk load.

tl;dr if the Intel pools go away before Microsoft ships an arm64 image, macOS SQL Server CI goes with
them.

Resolves

RUSTSEC-2026-0098, RUSTSEC-2026-0099, RUSTSEC-2026-0104

Upgrade tokio-rustls to 0.26, rustls to 0.23, and rustls-native-certs to
0.8 to resolve RUSTSEC-2024-0421 and RUSTSEC-2025-0010.

Migrate the TLS stream to the rustls 0.23 API. Switch the crypto provider
to aws-lc-rs via builder_with_provider to avoid the dual-provider conflict
that tokio-rustls 0.26 introduces when ring is also in the dependency graph.

Pin to TLS 1.2 to prevent TLS 1.3 KeyUpdate messages from triggering
UnexpectedEof on the macOS CI runner.
azure_core 0.20.0 switched from reqwest 0.11 to reqwest 0.12, which
pulls in rustls 0.23 and rustls-webpki 0.103.13. Bumping azure_identity
to 0.20.0 closes RUSTSEC-2026-0098, 0099, and 0104 in the dev build
without any changes to the production stack.

client_credentials_flow::perform now takes &str for the client secret.
Updated aad-auth.rs to pass raw env var strings and dropped the oauth2
ClientId/ClientSecret wrappers. Also bump reqwest 0.11 -> 0.12 and
oauth2 4.2.3 -> 5.0 in dev-dependencies to match.

Remove .cargo/audit.toml - the suppressions are no longer needed.
Migrate the macOS runner to macos-26-intel and add docker/setup-docker-action
so Docker is available for SQL Server. Replace all manual cargo cache steps
with Swatinem/rust-cache and add a sanitization step that replaces commas
with + to keep matrix variants isolated.
Two tests written before #[test_on_runtimes] existed and never updated.

cyrillic_collations_should_work previously created a dedicated database with
a Cyrillic default collation, requiring an admin connection and DROP DATABASE
at teardown. The DROP raced against open connections on macOS/rustls CI,
causing flaky failures. Replace with a session-local temp table using
column-level COLLATE clauses. The code path under test (COLMETADATA collation
-> encoding_rs decode) is identical.

application_name_should_be_set_correctly needed the application name set
before connecting. Add APP_NAME_CONN_STR embedding it in the connection
string so the macro-generated harness connects with it set.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 12, 2026

Review Change Stack

Summary by CodeRabbit

  • Chores
    • Modernized CI/GitHub Actions workflow with updated caching strategies and runner configurations.
    • Upgraded dependencies including TLS libraries and Azure SDK components for improved compatibility.
    • Renewed TLS certificates and enhanced certificate validation and handling.
    • Improved Docker infrastructure with explicit platform specifications and permission management.

Walkthrough

This PR modernizes the Tiberius TLS stack by upgrading to rustls 0.26 and tokio-rustls 0.26, updating client TLS configuration to use newer rustls APIs, regenerating certificates with extended validity, refactoring Docker container certificate handling, upgrading supporting dependencies for examples and tests, and modernizing CI/CD workflows with pinned actions.

Changes

Rustls TLS stack modernization

Layer / File(s) Summary
Rustls and TLS dependency updates
Cargo.toml
Upgraded tokio-rustls to 0.26 and rustls-native-certs to 0.8; removed rustls-pemfile from the rustls feature set.
TLS client implementation for new rustls API
src/client/tls_stream/rustls_tls_stream.rs, examples/aad-auth.rs
Rewrote NoCertVerifier to implement ServerCertVerifier with CertificateDer/UnixTime types; updated ClientConfig construction to use builder_with_provider with aws_lc_rs provider; rewrote CA certificate loading with PEM/CRT/DER parsing and single-cert enforcement; updated AAD example to use oauth2 5.0 String-based credentials API.
TLS feature gating
src/client/tls.rs, src/tds/codec/header.rs
Added conditional compilation guards to TlsPreloginWrapper, AsyncRead/AsyncWrite trait implementations, and PacketHeader::set_type to only compile when TLS features are enabled.
Certificate infrastructure and Docker updates
docker/certs/generate-signed-cert.sh, docker/certs/server.*, docker/certs/customCA.srl, docker-compose.yml, docker/docker-*.dockerfile
Updated certificate generation script to accept configurable validity parameter (default 1825 days); regenerated TLS certificate files; configured docker-compose services with explicit linux/amd64 platform; refactored all MSSQL Dockerfiles to use explicit USER/COPY/chmod/chown steps instead of inline COPY flags.
OAuth2 and dev dependencies
Cargo.toml
Upgraded azure_identity to 0.20.0, oauth2 to 5.0, and reqwest to 0.12 for dev/example tooling.
Test updates
tests/query.rs
Added APP_NAME_CONN_STR constant and converted async-std-specific tests to cross-runtime #[test_on_runtimes] format with new Cyrillic collation and application name verification tests.
CI/CD workflow modernization
.github/workflows/test.yml
Pinned action versions; replaced third-party clippy/rustfmt-check with direct cargo commands; introduced computed RUST_CACHE_KEY per feature; updated macOS runner to macos-26-intel; added docker/setup-docker-action and switched to docker compose syntax; unified caching strategy across platforms using Swatinem/rust-cache.

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Rustls vulnerability fix' accurately captures the primary focus of the changeset, which centers on addressing multiple rustls-related security advisories (RUSTSEC-2026-0098, -0099, -0104) through dependency updates and API migrations.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, detailing vulnerability fixes, dependency version bumps, API migrations, CI improvements, test fixes, and known remaining work.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/client/tls_stream/rustls_tls_stream.rs (1)

223-249: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Return a typed error instead of panicking.

TrustConfig::Default is a normal runtime path, but expect("could not load platform certs") and assert!(!roots.is_empty()) will abort the process on hosts with a broken or empty native trust store. Surface this as crate::Result so callers receive a recoverable connection failure instead of a library panic.

Suggested change
 trait ConfigBuilderExt {
-    fn with_native_roots(self) -> ConfigBuilder<ClientConfig, WantsClientCert>;
+    fn with_native_roots(
+        self,
+    ) -> crate::Result<ConfigBuilder<ClientConfig, WantsClientCert>>;
 }
 
 impl ConfigBuilderExt for ConfigBuilder<ClientConfig, WantsVerifier> {
-    fn with_native_roots(self) -> ConfigBuilder<ClientConfig, WantsClientCert> {
+    fn with_native_roots(
+        self,
+    ) -> crate::Result<ConfigBuilder<ClientConfig, WantsClientCert>> {
         let mut roots = RootCertStore::empty();
         let mut valid_count = 0;
         let mut invalid_count = 0;
 
-        for cert in rustls_native_certs::load_native_certs().expect("could not load platform certs")
-        {
+        for cert in rustls_native_certs::load_native_certs().map_err(|err| Error::Io {
+            kind: IoErrorKind::InvalidData,
+            message: format!("could not load platform certs: {err}"),
+        })? {
             match roots.add(cert) {
                 Ok(_) => valid_count += 1,
                 Err(err) => {
                     event!(Level::DEBUG, "certificate parsing failed: {:?}", err);
                     invalid_count += 1
@@
-        assert!(!roots.is_empty(), "no CA certificates found");
+        if roots.is_empty() {
+            return Err(Error::Io {
+                kind: IoErrorKind::InvalidData,
+                message: "no CA certificates found".to_string(),
+            });
+        }
 
-        self.with_root_certificates(roots)
+        Ok(self.with_root_certificates(roots))
     }
 }

And at the call site:

-                builder.with_native_roots().with_no_client_auth()
+                builder.with_native_roots()?.with_no_client_auth()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/tls_stream/rustls_tls_stream.rs` around lines 223 - 249, The
with_native_roots implementation currently panics on failure; change it to
return a crate::Result so callers can handle errors: update the trait
ConfigBuilderExt signature from fn with_native_roots(self) -> ConfigBuilder<...>
to fn with_native_roots(self) -> crate::Result<ConfigBuilder<ClientConfig,
WantsClientCert>>; replace rustls_native_certs::load_native_certs().expect(...)
with a match or ? that maps the load error into your crate error type, replace
assert!(!roots.is_empty()) with an explicit error return (e.g.,
Err(crate::Error::NoRootCerts)) if roots.is_empty(), and ensure the method
returns Ok(builder) on success; finally update any call sites of
with_native_roots to handle the Result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docker/certs/generate-signed-cert.sh`:
- Around line 10-12: Validate the CERT_DAYS variable before creating keys/CSRs:
ensure CERT_DAYS is a positive integer (e.g., >0) and exit with a clear error if
it is not; reference the variables CERT_DAYS, CERT_KEY_NAME and CERT_FILE to
locate the relevant logic that creates the key/CSR and performs signing. After
validation, when invoking openssl use a quoted expansion of the days parameter
(e.g., "$CERT_DAYS") so non-numeric or spaced values do not get split by the
shell; apply the same validation/quoting wherever CERT_DAYS is used (including
the later signing step).

In `@docker/docker-mssql-2017.dockerfile`:
- Around line 3-7: The Dockerfile currently switches to USER root to copy and
chmod certificate files but never switches back, causing the container to run as
root; after the two RUN chmod 440 /certs/server.* and RUN chmod 440
/certs/customCA.* steps, add a USER mssql instruction to restore the base
image's non-root runtime user so SQL Server runs as mssql rather than root.

In `@docker/docker-mssql-2022.dockerfile`:
- Around line 5-7: Replace the two chmod invocations that set certificate files
to world-readable (the lines "RUN chmod 444 /certs/server.*" and "RUN chmod 444
/certs/customCA.*") so they instead set permissions to 440 to match the other
Dockerfiles; update both occurrences in docker-mssql-2022.dockerfile to use
chmod 440 for consistency and tighter security.

In `@src/client/tls_stream/rustls_tls_stream.rs`:
- Around line 111-150: Update the .crt handling so it first attempts PEM parsing
with CertificateDer::pem_slice_iter and, if one or more PEM certs are returned,
add each parsed cert to the RootCertStore (use cert_store.add for every item)
instead of requiring exactly one and unwrapping; if PEM parsing yields zero
certs for a .crt file, fall back to treating the file as DER with
CertificateDer::from; also accept .pem as before and keep the
unsupported-extension error for other cases—use the existing symbols
CertificateDer::pem_slice_iter, CertificateDer::from, cert_store.add, and
path.to_string_lossy() to locate and update the logic.

---

Outside diff comments:
In `@src/client/tls_stream/rustls_tls_stream.rs`:
- Around line 223-249: The with_native_roots implementation currently panics on
failure; change it to return a crate::Result so callers can handle errors:
update the trait ConfigBuilderExt signature from fn with_native_roots(self) ->
ConfigBuilder<...> to fn with_native_roots(self) ->
crate::Result<ConfigBuilder<ClientConfig, WantsClientCert>>; replace
rustls_native_certs::load_native_certs().expect(...) with a match or ? that maps
the load error into your crate error type, replace assert!(!roots.is_empty())
with an explicit error return (e.g., Err(crate::Error::NoRootCerts)) if
roots.is_empty(), and ensure the method returns Ok(builder) on success; finally
update any call sites of with_native_roots to handle the Result.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9b6aed8e-e84e-498e-8f5b-a170d8bc9fa2

📥 Commits

Reviewing files that changed from the base of the PR and between a6b4fcd and 6247684.

⛔ Files ignored due to path filters (1)
  • docker/certs/server.pem is excluded by !**/*.pem
📒 Files selected for processing (17)
  • .github/workflows/test.yml
  • Cargo.toml
  • docker-compose.yml
  • docker/certs/customCA.srl
  • docker/certs/generate-signed-cert.sh
  • docker/certs/server-full.crt
  • docker/certs/server.crt
  • docker/certs/server.key
  • docker/docker-azure-sql-edge.dockerfile
  • docker/docker-mssql-2017.dockerfile
  • docker/docker-mssql-2019.dockerfile
  • docker/docker-mssql-2022.dockerfile
  • examples/aad-auth.rs
  • src/client/tls.rs
  • src/client/tls_stream/rustls_tls_stream.rs
  • src/tds/codec/header.rs
  • tests/query.rs

Comment on lines 10 to 12
CERT_KEY_NAME=$1
CERT_DAYS=${2:-1825}
CERT_FILE=$CERT_KEY_NAME.crt
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Validate CERT_DAYS before generating artifacts.

Now that the validity period is caller-controlled, values like 0 or 30d will fail only at the signing step after the key and CSR have already been created. Reject invalid input up front and quote the expansion when invoking OpenSSL.

Suggested fix
 CERT_KEY_NAME=$1
 CERT_DAYS=${2:-1825}
+if ! [[ "$CERT_DAYS" =~ ^[1-9][0-9]*$ ]]; then
+  echo "CERT_DAYS must be a positive integer" >&2
+  exit 1
+fi
 CERT_FILE=$CERT_KEY_NAME.crt
@@
-  -days $CERT_DAYS
+  -days "$CERT_DAYS"

Also applies to: 37-37

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docker/certs/generate-signed-cert.sh` around lines 10 - 12, Validate the
CERT_DAYS variable before creating keys/CSRs: ensure CERT_DAYS is a positive
integer (e.g., >0) and exit with a clear error if it is not; reference the
variables CERT_DAYS, CERT_KEY_NAME and CERT_FILE to locate the relevant logic
that creates the key/CSR and performs signing. After validation, when invoking
openssl use a quoted expansion of the days parameter (e.g., "$CERT_DAYS") so
non-numeric or spaced values do not get split by the shell; apply the same
validation/quoting wherever CERT_DAYS is used (including the later signing
step).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on the CI, it might not be a bad idea to forego bundling pregenerated key material and simply generate a temporary CA and certificate during the test process.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set chat.allow_non_org_members: true in your configuration.

Comment on lines +3 to +7
USER root
COPY certs/server.* /certs/
RUN chmod 440 /certs/server.*
COPY certs/customCA.* /certs/
RUN chmod 440 /certs/customCA.*
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

What is the default runtime user for the mcr.microsoft.com/mssql/server:2017-latestcontainer image, and should a derived Dockerfile switch back fromUSER root before the final layer?

💡 Result:

Default runtime user for mcr.microsoft.com/mssql/server:2017-latest (SQL Server 2017 14.x) is root. [1][2] Should a derived Dockerfile switch back from USER root before the final layer? Yes—if your goal is to run the container as non-root. Microsoft notes that SQL Server 2017 containers “start as root by default” and provides guidance to build/run a SQL Server 2017 container that starts as a non-root user (using a named non-root user like mssql) via a Dockerfile and/or docker run -u. [2] It also explicitly states you can run the non-root container as root “if necessary,” implying the security-oriented default expectation is to stay non-root. [2] Practical implication: if your derived image temporarily sets USER root to perform privileged filesystem setup during build, switch back to the intended non-root USER for the final runtime layer (e.g., USER mssql) so runtime isn’t root. This matches Microsoft’s documented approach for running SQL Server 2017 containers as a non-root user. [2][3]

Citations:


Switch back to non-root user after provisioning certs.

Leaving USER root as the final instruction makes the SQL Server process run as root in this derived image, which weakens container isolation. The base image's security-hardened default is to run as the mssql user; add USER mssql after the chmod steps to restore it and prevent this regression.

🧰 Tools
🪛 Checkov (3.2.528)

[low] 3-3: Ensure the last USER is not root

(CKV_DOCKER_8)

🪛 Hadolint (2.14.0)

[warning] 3-3: Last USER should not be root

(DL3002)

🪛 Trivy (0.69.3)

[error] 3-3: Image user should not be 'root'

Last USER command in Dockerfile should not be 'root'

Rule: DS-0002

Learn more

(IaC/Dockerfile)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docker/docker-mssql-2017.dockerfile` around lines 3 - 7, The Dockerfile
currently switches to USER root to copy and chmod certificate files but never
switches back, causing the container to run as root; after the two RUN chmod 440
/certs/server.* and RUN chmod 440 /certs/customCA.* steps, add a USER mssql
instruction to restore the base image's non-root runtime user so SQL Server runs
as mssql rather than root.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root user is kept here as the SQL Server 2017 linux container defaults to root. It wasn't until later versions that the container switches to mssql.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set chat.allow_non_org_members: true in your configuration.

Comment on lines +5 to +7
RUN chmod 444 /certs/server.*
COPY certs/customCA.* /certs/
RUN chmod 444 /certs/customCA.*
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Certificate permission inconsistency across Dockerfiles.

This Dockerfile uses chmod 444 for certificates, while docker/docker-mssql-2019.dockerfile and docker/docker-azure-sql-edge.dockerfile use chmod 440. For security consistency, all three should use the same permission mode. Since 440 (owner+group read-only) is more restrictive than 444 (world-readable), consider standardizing on 440.

🔒 Proposed fix to align with other Dockerfiles
-RUN chmod 444 /certs/server.*
+RUN chmod 440 /certs/server.*
-RUN chmod 444 /certs/customCA.*
+RUN chmod 440 /certs/customCA.*
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
RUN chmod 444 /certs/server.*
COPY certs/customCA.* /certs/
RUN chmod 444 /certs/customCA.*
RUN chmod 440 /certs/server.*
COPY certs/customCA.* /certs/
RUN chmod 440 /certs/customCA.*
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docker/docker-mssql-2022.dockerfile` around lines 5 - 7, Replace the two
chmod invocations that set certificate files to world-readable (the lines "RUN
chmod 444 /certs/server.*" and "RUN chmod 444 /certs/customCA.*") so they
instead set permissions to 440 to match the other Dockerfiles; update both
occurrences in docker-mssql-2022.dockerfile to use chmod 440 for consistency and
tighter security.

Comment on lines +111 to +150
Some(ext)
if ext.eq_ignore_ascii_case("pem")
|| ext.eq_ignore_ascii_case("crt") =>
{
let pem_certs: Vec<
CertificateDer<'static>,
> = CertificateDer::pem_slice_iter(&buf)
.collect::<Result<Vec<_>, _>>()
.map_err(|e| crate::Error::Io {
kind: IoErrorKind::InvalidData,
message: format!(
"Failed to parse PEM certificate: {e}"
),
})?;
if pem_certs.len() != 1 {
return Err(crate::Error::Io {
kind: IoErrorKind::InvalidInput,
message: format!(
"Certificate file {} contain 0 or more than 1 certs",
path.to_string_lossy()
),
});
}
Some(_) | None => return Err(crate::Error::Io {

pem_certs.into_iter().next().unwrap()
}
Some(ext)
if ext.eq_ignore_ascii_case("der") =>
{
CertificateDer::from(buf)
}
Some(_) | None => {
return Err(crate::Error::Io {
kind: IoErrorKind::InvalidInput,
message: "Provided CA certificate with unsupported file-extension! Supported types are pem, crt and der.".to_string(),
}),
};
})
}
};
let mut cert_store = RootCertStore::empty();
cert_store.add(&cert)?;
cert_store.add(cert)?;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle .crt inputs as either PEM or DER, and allow certificate bundles.

Lines 111-150 force .crt through the PEM parser and then reject anything except exactly one parsed cert. That breaks two common TrustConfig::CaCertificateLocation inputs: DER-encoded .crt files and PEM bundles/root+intermediate chains. RootCertStore can hold multiple trust anchors, so this should add every parsed PEM cert and only fall back to DER for .crt when PEM parsing does not produce certificates.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/tls_stream/rustls_tls_stream.rs` around lines 111 - 150, Update
the .crt handling so it first attempts PEM parsing with
CertificateDer::pem_slice_iter and, if one or more PEM certs are returned, add
each parsed cert to the RootCertStore (use cert_store.add for every item)
instead of requiring exactly one and unwrapping; if PEM parsing yields zero
certs for a .crt file, fall back to treating the file as DER with
CertificateDer::from; also accept .pem as before and keep the
unsupported-extension error for other cases—use the existing symbols
CertificateDer::pem_slice_iter, CertificateDer::from, cert_store.add, and
path.to_string_lossy() to locate and update the logic.

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