Skip to content

feat(security): wip#1267

Open
pophilpo wants to merge 9 commits into
mainfrom
pophilpo/https-compliance
Open

feat(security): wip#1267
pophilpo wants to merge 9 commits into
mainfrom
pophilpo/https-compliance

Conversation

@pophilpo

@pophilpo pophilpo commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Shared client builder (security-utils)

  • Renamed http_client_builder() → client_builder() (leaving the old name as possible to use, but with deprecated warning)

  • TLS 1.3 minimum enforced

  • https_only(true) in production, enforced by clippy ( see below)

  • CA pinning: 9 specific CAs (Amazon Root CA 1–4, Starfield G2, Google Trust Services R1–R4), system trust store disabled

  • SHA256 hash verification of embedded cert bytes at runtime

  • allow-http = []in security-utils feature for crates with local HTTP test servers, only to be used as dev-dependency feature flag.

All production reqwest clients migrated to security_utils :: client_builder()

  • orb-backend-status

  • orb-connd

  • orb-speed-test

  • orb-attest

  • orb-se050-reprovision

Explicit exceptions

  • update-agent + update-agent-loader: system certs retained to keep update path alive after extended offline periods. Both now have TLS 1.3 minimum and https_only(true) set manually.

Clippy enforcement

  • clippy.toml at workspace root bans reqwest::Client::builder, reqwest::Client::new, and blocking equivalents with reason messages pointing to client_builder()from security utils

Pending

  • Test orb-software changes on the orb

  • priv-orb-core: bump orb-security-utils rev, rename http_client_builder() → client_builder(), add clippy.toml — deferred until orb-software changes are tested

  • Check orb-relay-messages

@worldcoin worldcoin deleted a comment from chatgpt-codex-connector Bot Jun 18, 2026
@pophilpo pophilpo marked this pull request as ready for review June 18, 2026 12:21
@pophilpo pophilpo requested a review from a team as a code owner June 18, 2026 12:21
@github-actions

Copy link
Copy Markdown

Found two concrete issues:

  • security-utils/src/reqwest.rs:154: This changes the shared client from TLS 1.2 minimum to TLS 1.3 minimum. That disables TLS 1.2 fallback entirely, so any pinned-CA endpoint that still only supports TLS 1.2 will fail even though the previous helper allowed it and TLS negotiation would still prefer 1.3 when available. Use TLS_1_2 unless the intended policy is explicitly TLS 1.3-only.

  • security-utils/Cargo.toml:18 / security-utils/src/reqwest.rs:156: allow-http can be enabled in any build, including release, and then the shared helper silently disables https_only for all consumers in that Cargo build graph. Since this crate is meant to enforce HTTPS-only, this should have a hard guard like compile_error! for feature = "allow-http" outside debug/test builds, or be split into test-only construction that cannot be selected for production artifacts.

I could not run cargo check in this sandbox because rustup tried to write under a read-only home directory.

@pophilpo pophilpo marked this pull request as draft June 18, 2026 12:51

@vmenge vmenge left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm

@vmenge

vmenge commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Found two concrete issues:

  • security-utils/src/reqwest.rs:154: This changes the shared client from TLS 1.2 minimum to TLS 1.3 minimum. That disables TLS 1.2 fallback entirely, so any pinned-CA endpoint that still only supports TLS 1.2 will fail even though the previous helper allowed it and TLS negotiation would still prefer 1.3 when available. Use TLS_1_2 unless the intended policy is explicitly TLS 1.3-only.
  • security-utils/Cargo.toml:18 / security-utils/src/reqwest.rs:156: allow-http can be enabled in any build, including release, and then the shared helper silently disables https_only for all consumers in that Cargo build graph. Since this crate is meant to enforce HTTPS-only, this should have a hard guard like compile_error! for feature = "allow-http" outside debug/test builds, or be split into test-only construction that cannot be selected for production artifacts.

I could not run cargo check in this sandbox because rustup tried to write under a read-only home directory.

you should admit urself in a scrapyard

@pophilpo pophilpo requested a review from eth44 June 19, 2026 16:10

@eth44 eth44 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: rename allow-http feature to dangerously-allow-http

@pophilpo pophilpo marked this pull request as ready for review June 25, 2026 15:53
@github-actions

Copy link
Copy Markdown

Found one concrete issue:

  • security-utils/src/reqwest.rs:156: allow-http silently disables https_only for any build where the Cargo feature is enabled, including release builds or --all-features builds. Cargo features are not dev-only, so a downstream crate can accidentally ship plaintext-HTTP-capable production clients despite the comment in security-utils/Cargo.toml:18. This should be guarded like update-agent-loader does, e.g. compile_error! for all(feature = "allow-http", not(debug_assertions)), or moved behind an explicit test-only builder/API.

I could not run cargo check because rustup tried to write under the read-only home directory.

@github-actions

Copy link
Copy Markdown

No concrete merge-blocking issues found in the PR diff.

I could not run cargo check/cargo clippy because the sandbox is read-only and Cargo/Rustup need writable target/cache directories, so this review is based on source inspection only.

@github-actions

Copy link
Copy Markdown

Found two concrete issues:

  • security-utils/src/reqwest.rs:82: dangerously-allow-http disables https_only via an additive Cargo feature. Any --all-features release/ad-hoc build of a crate using client_builder() silently allows http:// in production clients. Unlike update-agent-loader’s allow_http, this has no release/debug guard. Please add a compile-time guard or make HTTP allowance test-only.

  • security-utils/src/reqwest.rs:97: the old public http_client_builder() API, including the blocking variant, was removed rather than kept as a deprecated wrapper. That will break downstream users updating orb-security-utils without a lockstep rename. Add deprecated aliases forwarding to client_builder().

I did not run tests in this read-only review environment.

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