Skip to content

Harden model loading and inference runtime handling - #2952

Merged
PawelPeczek-Roboflow merged 8 commits into
mainfrom
fix/inference-runtime-security-defaults
Sep 11, 2026
Merged

Harden model loading and inference runtime handling#2952
PawelPeczek-Roboflow merged 8 commits into
mainfrom
fix/inference-runtime-security-defaults

Conversation

@alexnorell

@alexnorell alexnorell commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Description

Harden model loading, media inputs and transport configuration across the server and first-party images.

  • Default benchmark commands and server images to trusted model packages. Use HTTPS gateway defaults, require client certificates when a CA is configured, and reject unsupported offline execution modes at startup.
  • Validate video sources while preserving documented media transports and an explicit opt-in for trusted raw GStreamer pipelines.
  • Isolate OPC connections by the complete credential tuple and validate workflow inputs before shell use.
  • Prevent a usage-collector deadlock when a full in-memory queue contains more distinct execution sessions than it can hold. Retain merged records as one batch for the existing reader, and isolate the Pingback unit test from a live scheduler.
  • Install a pinned, checksum-verified Claude executable for automated review.

Stream API authentication, SDK management calls, image enablement and desktop defaults match current main, including existing workspace authentication and localhost binding behavior. No new stream administration token is required.

The migration guide in docs/server_configuration/security_migration.md documents the remaining configuration changes. The inference-models Unreleased changelog covers its public behavior changes; release versions remain a maintainer decision.

Validation

  • 407 focused HTTP, workspace-auth compatibility, media validation, mTLS, deployment, collector and Pingback tests passed.
  • All 657 SDK HTTP and CLI unit tests passed.
  • Independent review confirmed restoration of stream behavior to main and preservation of the other fixes; 93 stream regression tests passed independently.
  • Formatting and diff checks passed. The full test matrix runs in Linux CI.

The queue saturation regression fails on the original implementation and passes with the fix at three queue capacities. An independent 6,012-record probe preserved session identities, resource outcomes and usage totals. The cause of earlier CI shutdown hangs remains unproven.

Target container, hardware and fleet rollout validation remain prerequisites; no production deployment is included.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

👋 Thanks for the pull request! Here is how automated Claude review works here, so you spend credits (and reviewer time) wisely.

🚦 This PR is marked Ready for review, so automated Claude review will run — and every pass spends real credits.

Warning

💸 The Claude reviewer bills in credits, not vibes

Automated review spins up a real agent that reads real code and spends real credits on every pass. It is glad to help — but it is not a rubber duck, a linter you poke in a loop, or a substitute for reading the contributing guide. Treat it like an expensive senior reviewer whose time you booked, and show up prepared.

Draft when unsure, Ready when you mean it:

  • 🌱 Not sure the PR is in good shape yet? Keep it (or set it back) as a draft — drafts pause review, so you can push and iterate without burning credits on a moving target.
  • 💪 Feel strong about the contents? Mark it Ready for review and the reviewer will take a look.

However you get there, arrive prepared:

  • 🧱 Bring a SOLID, thorough PR. Point your local agent at our skills/ to tune it to our guidelines first — or, if you are one of those fabled carbon-based contributors, read them yourself. A half-baked diff costs exactly the same to review as a finished one.
  • Resolve every comment before you re-request review. Re-requesting with threads still open means paying twice for the same conversation.
  • 🔁 Do not use CI review as an inner loop for a local agent. The reviewer is not a step-by-step debugger — do the unfolding locally and arrive with the answer, not the search.
  • 🙋 If something looks off, ask a human. One question to a maintainer is cheaper and faster than three rounds of agent re-review chasing a misread.

Reviews are not free. A draft costs nothing to review; a Ready PR is a promise that it is worth reviewing.

  • Prefer to skip automated review entirely? Add the skip-claude-review label.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude review started at commit 821ac2468c437348f33513f125c4b3e00f375ff2.

New commits are not auto-reviewed. Add the claude-review label to request a re-review — the label is consumed when the review starts, so just add it again next time.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude review started at commit ec094c20cefe8b9ef7e93b0a884fa9d0eeb9d227.

New commits are not auto-reviewed. Add the claude-review label to request a re-review — the label is consumed when the review starts, so just add it again next time.

@alexnorell alexnorell added the claude-review Use to trigger AI review manually label Sep 9, 2026 — with ChatGPT Codex Connector
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude review started at commit ec094c20cefe8b9ef7e93b0a884fa9d0eeb9d227.

New commits are not auto-reviewed. Add the claude-review label to request a re-review — the label is consumed when the review starts, so just add it again next time.

@github-actions github-actions Bot removed the claude-review Use to trigger AI review manually label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Review summary — first pass at HEAD ec094c2.

Skills: review-http-api-server, review-sdk, review-cli-cloud-tooling, review-inference-models-pkg, review-packaging-ci, review-core-infra, review-topic-auth-and-tenant-security, review-topic-input-boundary-security, review-topic-secure-gateway-url-wrapping, review-topic-backward-compat-and-versioning, review-topic-concurrency-and-resource-safety, review-topic-external-contract-and-silent-fallback, review-topic-test-hygiene.

Coverage note: No dedicated surface skill covers inference/core/interfaces/camera/**, inference/core/interfaces/stream_manager/**, inference/enterprise/**, examples/**, or docs/** — those were reviewed with the generic review plus the topic skills above only.

What I traced (static, read-only):

  • Stream-management auth middleware and route gating (all /inference_pipelines* routes are gated by ENABLE_STREAM_API; the auth middleware is added alongside; CORS is registered after it and is therefore outer, so preflight is answered before auth; constant-time token compare; root_path stripping handled).
  • SDK pipeline methods all route through the dedicated stream header with allow_redirects=False; unchanged behavior when stream_api_key is unset.
  • OPC UA pool key now binds (url, user_name, password) via a process-local HMAC; release_connection/invalidate_connection require the password (the only no-password release_connection call is the intentional pytest.raises(TypeError) test).
  • Secure-gateway validator (shared by core and inference_models), mTLS ssl_cert_reqs plumbing (Uvicorn kwargs + CLI int serialization + run_uvicorn.sh + Gunicorn --cert-reqs=2), video-reference validation, and offline+modal fail-closed.

No blocking findings. The default flips (ENABLE_STREAM_API=False, ALLOW_INFERENCE_MODELS_UNTRUSTED_PACKAGES=False, benchmark allow_untrusted_packages=False, HTTPS SECURE_GATEWAY, mTLS when SSL_CA_CERTS is set, offline+modal now fatal) are backward-incompatible but intentional and documented in docs/server_configuration/security_migration.md and the inference-models Unreleased changelog.

Commands used: gh pr diff, gh pr view, gh api .../issues|pulls/comments|reviews|commits, grep/sed over the changed files.

Reviewed at HEAD: ec094c2

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Release coordination (maintainer notice — non-blocking):

  • inference-models: this PR changes public, user-visible behavior (SECURE_GATEWAY now defaults to HTTPS and validates/fails at startup; benchmark commands default to allow_untrusted_packages=False). The contributor has correctly added the entry under ## Unreleased in inference_models/docs/changelog.md; a version bump is a maintainer decision at release time.
  • inference (core server / SDK / CLI / images): user-visible security defaults changed (ENABLE_STREAM_API=False + required STREAM_API_KEY, dedicated stream admin token in the SDK, mTLS enforcement when SSL_CA_CERTS is set, offline+modal now fatal, Jetson images drop ALLOW_INFERENCE_MODELS_UNTRUSTED_PACKAGES=True). These are documented in docs/server_configuration/security_migration.md. Maintainers: confirm whether an Execution Engine changelog entry (roboflow/docs workflows/developer-guide/execution-engine-changelog.md) is warranted for the WORKFLOWS_CUSTOM_PYTHON_EXECUTION_MODE fail-closed change and the OPC-writer credential-isolation fix, and coordinate the release-time version change for the inference server images.

Per repo convention, contributors are not asked to select or bump versions.

Reviewed at HEAD: ec094c2

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

😎 PR passes the vibe-check and trust-me-bro verification.

@alexnorell
alexnorell force-pushed the fix/inference-runtime-security-defaults branch from ec094c2 to 7269ed1 Compare September 10, 2026 01:22
@alexnorell alexnorell changed the title Tighten inference authentication and runtime defaults Harden model loading and inference runtime handling Sep 10, 2026
@PawelPeczek-Roboflow
PawelPeczek-Roboflow merged commit e71daab into main Sep 11, 2026
43 of 49 checks passed
@PawelPeczek-Roboflow
PawelPeczek-Roboflow deleted the fix/inference-runtime-security-defaults branch September 11, 2026 14:06
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.

3 participants