Enable tls-rustls-insecure to support --insecure flag

[fcostaoliveira]

Root cause

--tls upgrades the connection URL to rediss://, but there was no way to disable certificate verification, so the tool could not connect to any server with a self-signed or private-CA cert — the normal case for test/staging/ephemeral benchmark deployments.

The documented redis-rs escape hatch rediss://host:port/#insecure also silently did nothing here:

• redis = { features = ["tokio-comp", "tokio-rustls-comp", "tls-rustls-webpki-roots"] }
• tls-rustls-webpki-roots and tokio-rustls-comp transitively enable tls-rustls
• the #insecure URL fragment is parsed under the tls-rustls gate (present) and sets an internal insecure: true
• but the code that actually relaxes certificate verification is gated on the separate tls-rustls-insecure feature, which was not enabled

So the flag was silently accepted by the URL parser and had zero effect on the actual TLS handshake.

Fix

• Cargo.toml: add tls-rustls-insecure to the redis dependency's feature list.
• src/main.rs: new --insecure / REDIS_TLS_INSECURE CLI flag. Wired into build_redis_url() — the same function that already applies --host/--port/--db/--password/--tls as URL mutations. When --insecure is set:
  • it requires the URL to already have scheme rediss (via --tls or a rediss:// --url), returning a clear error otherwise instead of a silent no-op;
  • it appends the #insecure fragment via url::Url::set_fragment, which now actually disables verification end-to-end since the crate feature is compiled in.
• README.md: documented the new flag in the CLI flags table and added a "TLS and certificate verification" section explaining the caveat, the mechanism, and (per review) the blast-radius/behavior-change note below.
• Added unit tests: build_redis_url_insecure_appends_fragment_with_tls, build_redis_url_insecure_works_with_rediss_url_without_tls_flag, build_redis_url_insecure_without_tls_is_rejected.

Review follow-ups (addressed in the second commit)

1. Stronger URL-parsing assertion. build_redis_url_insecure_appends_fragment_with_tls now also parses the generated URL through redis-rs's own IntoConnectionInfo (the same path redis::Client::open uses) and asserts the resulting ConnectionAddr is TcpTls { insecure: true, .. }, not just a string-suffix check on #insecure. This catches an upstream fragment-name/format change. It does not catch the tls-rustls-insecure Cargo feature itself being dropped — that's a separate failure mode, covered by (2).
2. CI guard against feature regression. Added a step to .github/workflows/ci.yml running cargo tree -e features -i redis --workspace | grep -q tls-rustls-insecure, failing the build if the feature is no longer enabled on the redis crate — so a future dependency edit can't silently turn --insecure back into a no-op while all other tests stay green.
3. Blast-radius note (security-relevant). tls-rustls-insecure is a crate-wide, build-time Cargo feature — it is not gated behind --insecure at runtime. Before this PR, a --url/REDIS_URL that already carried a #insecure fragment (e.g. rediss://host:6379/0#insecure) was silently getting full certificate verification anyway (the feature wasn't compiled in, so the fragment was inert). After this PR, that same pre-existing fragment takes effect automatically and skips verification — even for callers who never pass --insecure themselves. This is the intended fix, but is a real behavior change for anyone already using that fragment, so it's now called out explicitly in the README's TLS section.
4. musl release build. Confirmed tls-rustls-insecure is a pure feature gate on top of the already-present tls-rustls/ring dependency tree — diffed cargo tree before/after the Cargo.toml change and the only difference is the workspace member's own path; no new crate is pulled in. Also did an actual cargo build --release --target x86_64-unknown-linux-musl, which succeeds and produces a working --version-reporting static-pie binary, confirming the reasoning holds in practice (not just by inspection).
5. REDIS_TLS_INSECURE=0/false handling. Verified directly against clap 4.6.6 (the version pinned in Cargo.lock): the derived bool+env flag only recognizes the literals "true"/"false" — "0" is neither and produces a clean parse error, never a silent true from "variable is merely present". Added insecure_env_var_false_resolves_to_false_not_presence_flag, insecure_env_var_true_resolves_to_true, insecure_env_var_zero_is_a_clean_parse_error_not_silent_true, and the same check against the pre-existing --tls/REDIS_TLS pattern (tls_env_var_false_resolves_to_false_not_presence_flag) to lock this in as a regression test.

Test plan (rounds 1-3)

• cargo build succeeds
• cargo check --all-features succeeds
• cargo test — 36 unit tests in src/main.rs (32 + 4 new) + 15 lib tests + 4 integration tests, all pass
• cargo fmt --check — clean
• cargo clippy --all-targets -- -D warnings — clean
• cargo tree -e features -i redis --workspace | grep tls-rustls-insecure — present (the new CI guard)
• cargo build --release --target x86_64-unknown-linux-musl — succeeds; binary runs and reports --version
• Grepped to confirm --insecure reaches build_redis_url() and the #insecure fragment, which flows into redis::Client::open(url) (the actual connection construction), not just argument parsing

Follow-up: --tls was actually non-functional end-to-end (CryptoProvider panic)

While reviewing this fix, a sibling repo in the org (celery-benchmark, same redis crate
version and identical TLS feature set: tokio-comp, tokio-rustls-comp,
tls-rustls-webpki-roots, tls-rustls-insecure) was found via a live TLS handshake test
to have a real runtime bug that goes beyond the tls-rustls-insecure feature-flag issue above:
nothing in the codebase ever installed a process-level rustls CryptoProvider. redis-rs's own
rustls dependency is default-features = false (only rustls/std), so it never enables
rustls's ring or aws-lc-rs crypto backend feature. rustls 0.23 requires the application to
install a provider once before any TLS connection, or every connection panics — on both the
verifying and --insecure paths alike. That means --tls (and therefore --insecure) was not a
"missing feature flag" bug, it was completely non-functional.

This repo has the identical redis feature list and, on inspection
(grep -rn "CryptoProvider|install_default" src/), the identical gap — not yet caught here
because the round-2 review above verified with unit tests, a musl release build, and env-var
parsing tests, but never a real TLS handshake against a live server.

Root cause confirmed and reproduced:

$ cargo tree -i rustls
rustls v0.23.43
├── redis v1.5.0
│   └── resque-bench v0.1.2
└── tokio-rustls v0.26.4
    └── redis v1.5.0 (*)

ring (not aws-lc-rs) is the resolved crypto backend, pulled in transitively via
rustls-webpki. Building the pre-fix binary and running it with --tls against a real
TLS-terminated redis-server (self-signed cert) reproduced the panic exactly as predicted:

thread 'main' (3124295) panicked at rustls-0.23.43/src/crypto/mod.rs:249:14:

Could not automatically determine the process-level CryptoProvider from Rustls crate features.
Call CryptoProvider::install_default() before this point to select a provider manually, or make sure exactly one of the 'aws-lc-rs' and 'ring' features is enabled.
See the documentation of the CryptoProvider type for more information.

stack backtrace:
   ...
   3: redis::connection::create_rustls_config
   4: redis::aio::connection::connect_simple::<redis::aio::tokio::Tokio>::{closure#0}
   5: <redis::client::Client>::get_multiplexed_async_connection_inner::<redis::aio::tokio::Tokio>::{closure#0}
   6: <redis::client::Client>::get_multiplexed_async_connection::{closure#0}
   7: resque_bench::main::{closure#0}
   8: resque_bench::main

Fix (this PR, same shape as the celery-benchmark fix):

• Cargo.toml: added a direct rustls = { version = "0.23", default-features = false, features = ["ring"] } dependency — not used directly in code, it exists purely so Cargo resolves rustls's ring feature for the single resolved rustls package in the tree.
• src/main.rs, top of main(): let _ = rustls::crypto::ring::default_provider().install_default(); — called once before Cli::parse()/any Redis connection, result ignored since a second call in-process would return Err harmlessly.

Verified with a real handshake against a self-signed-cert redis-server on a --tls-port
(cert generated with openssl req -x509 -newkey rsa:2048 -nodes -keyout key.pem -out cert.pem -days 1 -subj /CN=localhost):

Run 1 — --tls without --insecure (expect a clean certificate error, not a panic):

$ ./resque-bench --url rediss://127.0.0.1:6390/0 --tls --jobs 10
warning: could not connect to Redis for tag lookup: invalid peer certificate: Other(OtherError(CaUsedAsEndEntity))

=== resque-bench — unknown ===
    rediss://127.0.0.1:6390/0  jobs=10  queues=default  poll-interval=5000ms

Error: failed to connect to Redis

Caused by:
    0: invalid peer certificate: Other(OtherError(CaUsedAsEndEntity))
    1: invalid peer certificate: Other(OtherError(CaUsedAsEndEntity))
EXIT CODE: 1

Run 2 — --tls --insecure (expect success, real end-to-end work):

$ ./resque-bench --url rediss://127.0.0.1:6390/0 --tls --insecure --jobs 200 --allow-flushdb
warning: --allow-flushdb is set on db 0 — this will destroy ALL keys in the database. Use --db 13 (or any non-zero db) to isolate benchmark data.

=== resque-bench — redis-8.6.0 ===
    rediss://127.0.0.1:6390/0#insecure  jobs=200  queues=default  poll-interval=5000ms

  [  10 workers]      31,607 jobs/s  p50=51.3 ms  p99=54.0 ms  p99.9=54.0 ms  max=54.0 ms
  [  10 workers] idle-poll:        2.0 LPOP/s  (0.2 LPOP/s/worker over 30.0s, 60 calls)
  [  50 workers]      28,107 jobs/s  p50=241.7 ms p99=244.7 ms p99.9=244.9 ms max=244.9 ms
  [  50 workers] idle-poll:       10.0 LPOP/s  (0.2 LPOP/s/worker over 30.0s, 300 calls)
  [ 100 workers]      24,069 jobs/s  p50=537.1 ms p99=540.7 ms p99.9=540.7 ms max=540.7 ms
  [ 100 workers] idle-poll:       20.0 LPOP/s  (0.2 LPOP/s/worker over 30.0s, 600 calls)
  [ 200 workers]      13,673 jobs/s  p50=878.6 ms p99=887.3 ms p99.9=887.3 ms max=887.3 ms
  [ 200 workers] idle-poll:       40.0 LPOP/s  (0.2 LPOP/s/worker over 30.0s, 1,200 calls)

Results saved → resque_bench_redis-8.6.0.json
EXIT CODE: 0

Both runs confirm the fix: verified TLS now fails cleanly on an untrusted cert (no panic) instead
of crashing, and --insecure completes a full real benchmark (INFO fetch, all four worker-count
trials, idle-poll phase, JSON report) over an actual TLS connection.

Re-verified full suite after the fix: cargo build, cargo check --all-features, cargo clippy --all-targets -- -D warnings, cargo fmt --check, and cargo test (15 lib + 36 bin + 4
integration tests) all pass.

Fixes #7

Round 4: reusable CryptoProvider helper, close the #insecure fragment warning gap, and a real CI TLS handshake test

Same root-cause pattern fixed in this round across sibling repos in the org
(celery-benchmark, bullmq-benchmark, sidekiq-benchmark) — three follow-ups to the
CryptoProvider fix above:

1. The startup warning had a gap: it keyed off cli.insecure, not the actual URL.
   A user-supplied --url rediss://host:6380/0#insecure (or a REDIS_URL env var
   carrying that fragment) disabled certificate verification with zero indication
   in the output, since nothing checked for it — only --insecure being explicitly
   passed would have. Fixed by adding url_disables_cert_verification(), which checks
   the final built URL's fragment directly, and keying the warning off that instead
   of cli.insecure:

   if url_disables_cert_verification(&url) {
       eprintln!(
           "warning: TLS certificate verification is DISABLED for this connection \
            (insecure mode) — the server's certificate chain and hostname are not \
            validated. Only use this against trusted networks."
       );
   }

   Verified locally: --url "rediss://127.0.0.1:16391/0#insecure" with no --insecure
   flag now prints the warning and connects insecurely, matching what --insecure
   itself does. Added 5 tests: url_disables_cert_verification_false_by_default,
   _true_via_insecure_flag, _true_via_url_supplied_fragment_without_flag (the
   specific gap this closes), _false_for_plain_tls_without_insecure, and
   _false_for_unparsable_url.

2. install_default() was only reachable from main(), not from tests. Pulled it
   out into a new src/tls.rs module (resque_bench::tls::install_crypto_provider()),
   registered via pub mod tls; in src/lib.rs. It's idempotent (let _ = ... on the
   "already installed" Err, never .expect()/.unwrap()), so both main() and any
   present/future integration test that opens a real rediss:// connection can call it
   safely without risking a double-install panic. Added
   tls::tests::install_crypto_provider_is_idempotent, which calls it three times in a
   row in-process.

3. New CI step: a real TLS handshake, not just unit tests. .github/workflows/ci.yml
   now spins up a dedicated TLS-only redis-server with a throwaway self-signed cert
   (openssl req -x509 -newkey rsa:2048 -nodes -keyout key.pem -out cert.pem -days 1 -subj /CN=localhost, --tls-port, --tls-cluster no --tls-replication no --tls-auth-clients no) and drives the actual built release binary against it:

   • without --insecure: must exit non-zero with a certificate error, and the step
     greps stderr for "panic" and fails if found (guards against the exact
     CryptoProvider regression this PR fixes recurring silently);
   • with --insecure: must complete a full run and print the insecure-mode warning
     from (1) on stderr.

   Dry-ran the exact commands used in the CI step locally against a real
   redis-server (self-signed cert, TLS-only on --tls-port 16391, --port 0):

   • Without --insecure: exit code 1, stderr = invalid peer certificate: Other(OtherError(CaUsedAsEndEntity)), no panic. PASS.
   • With --insecure: exit code 0, ran 200 jobs across 2 workers (22k+ jobs/s),
     errors: 0, printed the warning from (1), redis-cli --tls --insecure shutdown nosave stopped the server cleanly afterward. PASS.

Re-verified full suite after these changes: cargo build, cargo check --all-features, cargo clippy --all-targets -- -D warnings (clean), cargo fmt --check
(clean), and cargo test — now 16 lib tests (+1) + 41 src/main.rs tests (+5) + 4
integration tests = 61 total, all pass.

Round 6: real musl re-verification after the rustls/ring dependency landed, plus a doc-accuracy fix

Two independent-reviewer findings addressed in this round.

1. The musl verification in the "Test plan (rounds 1-3)" checklist above predates the ring dependency and was never redone

Confirmed via git log: the cargo build --release --target x86_64-unknown-linux-musl
recorded above (round 2/3, "musl release build" bullet) ran against the tree as it stood
before the CryptoProvider fix. That fix (commit 5d9cb63, "Follow-up: --tls was
actually non-functional end-to-end (CryptoProvider panic)" section above) is what added
the direct rustls = { ..., features = ["ring"] } dependency to Cargo.toml — before
that commit, ring was only a transitive dependency pulled in via rustls-webpki, and
nothing in this crate's own Cargo.toml forced Cargo to resolve rustls's ring feature
explicitly. ring compiles C and assembly via the cc crate, which needs a working musl C
toolchain — a requirement that didn't exist yet at the time of the original musl
verification. So the musl cross-build had never actually been exercised against the
current, final dependency graph, unlike sibling repos in the org (sidekiq-benchmark,
celery-benchmark, bullmq-benchmark), which all got a dedicated final musl
re-verification round after their own ring dependency landed.

Re-verified now, on this branch's current HEAD (after the CryptoProvider fix, the
round-4/5 follow-ups, and this round's doc fix):

$ rustup target add x86_64-unknown-linux-musl   # already installed
$ sudo apt-get install -y musl-tools            # matches .github/workflows/release.yml's
                                                 # "Install musl toolchain" step exactly
$ cargo build --release --target x86_64-unknown-linux-musl
   Compiling ring v0.17.14
   ...
    Finished `release` profile [optimized] target(s) in 1m 39s

Confirmed the resulting binary is genuinely static, not just "happens to run here":

$ file target/x86_64-unknown-linux-musl/release/resque-bench
... ELF 64-bit LSB pie executable, x86-64, ... static-pie linked, not stripped

$ ldd target/x86_64-unknown-linux-musl/release/resque-bench
	statically linked

$ objdump -T target/x86_64-unknown-linux-musl/release/resque-bench | grep -c GLIBC
0

$ readelf -d target/x86_64-unknown-linux-musl/release/resque-bench
# no DT_NEEDED entries — no shared library dependencies at all

$ ./target/x86_64-unknown-linux-musl/release/resque-bench --help   # runs fine, exit 0

ring's C/assembly compiled cleanly under the musl toolchain, zero GLIBC_* symbols
anywhere in the binary, ldd reports "statically linked", and the binary actually executes
— matching exactly what .github/workflows/release.yml's own "Verify binary architecture"
step checks (file output + a real --version/--help smoke run). No gap found:
release.yml's existing "Install musl toolchain" step (sudo apt-get install -y musl-tools, with a comment already noting "rustls pulls in ring, which compiles C and
assembly, so the musl target needs a musl C toolchain") already correctly anticipates this
requirement — it just hadn't been re-exercised locally against the final dependency graph
until now. No changes needed to release.yml.

2. Doc-accuracy nit: the #insecure-without-the-feature framing was backwards

The "Behavior change / blast radius" framing above (and the matching Cargo.toml comment)
described a pre-existing #insecure-fragment URL as, before this PR, being "silently
ignored" with the connection getting full certificate verification anyway. That's not what
actually happens: per redis-1.5.0/src/connection.rs (~lines 1255-1279), URL fragment
parsing itself isn't feature-gated (a #insecure fragment always sets an internal
insecure: true), but building the TLS config with insecure: true while
tls-rustls-insecure is not compiled in hits an explicit fail!(...) arm and returns
ErrorKind::InvalidClientConfig ("Cannot create insecure client without
tls-rustls-insecure feature") — a hard connection failure, not a silent success with
verification intact. There was never a working prior configuration that carried that
fragment; it simply refused to connect. Corrected the wording in Cargo.toml's comment and
both affected passages in the README's TLS section (including "Behavior change / blast
radius") to describe the hard-fail accurately instead.

Re-verified full suite after this round: cargo build, cargo check --all-features,
cargo clippy --all-targets -- -D warnings (clean), cargo fmt --check (clean), and
cargo test (16 lib + 42 src/main.rs + 4 integration tests = 62 total, all pass).

[github-actions]

🤖 Automated first-pass review — a human maintainer's review is still required before merge.

There's already an earlier automated comment on this PR covering the direct-rustls version-skew risk, the gnu-vs-musl gap in what the new TLS step actually exercises, and the --tag ci-tls shortcut past fetch_tag(). I won't restate those. Three things I don't think are covered anywhere yet, one of which is concrete enough to just fix.

1. The last commit's correction didn't land in all three places. de6dad6 ("docs: fix inaccurate #insecure-fragment-without-feature framing") correctly replaced the "silent no-op" story with the real one — redis-rs parses the fragment regardless and then hard-fails with ErrorKind::InvalidClientConfig, cited to redis-1.5.0/src/connection.rs:1274-1279. That fix went into Cargo.toml and the README's "Behavior change / blast radius" section, but the comment above u.set_fragment(Some("insecure")) in build_redis_url() still says the fragment "is silently parsed and ignored, which was the root cause of this flag not existing before" — which is the exact framing that commit exists to retract. The PR description has the same residue ("silently accepted by the URL parser and had zero effect on the actual TLS handshake" under Root cause, and "was silently getting full certificate verification anyway" in follow-up 3), and since the description tends to become the squash body, the corrected and uncorrected versions of the same claim would ship in one commit. Worth reconciling before merge — it's a one-line comment edit plus a description touch-up, but "the fragment was inert and you got verification" vs. "the connection was refused outright" are materially different things to tell someone debugging an existing REDIS_URL.

2. The new rustls dependency asks for ring and inherits everything else — including, I think, whether TLS 1.2 is compiled in at all. The declaration is rustls = { version = "0.23", default-features = false, features = ["ring"] }. That turns off rustls's default feature set, which in 0.23 includes std, logging and tls12, and only adds ring back. It builds and the handshake works because Cargo unifies with redis-rs's own rustls dep — but per this PR's own citation that dep is default-features = false with only rustls/std, and redis-rs must also be disabling tokio-rustls's defaults (otherwise aws-lc-rs would have been on and there'd have been no CryptoProvider panic to fix). If nothing in the graph enables rustls/tls12, this binary negotiates TLS 1.3 only. I can't confirm the resolved set from the diff, so treat this as "worth checking, not asserted" — cargo tree -e features -i rustls should settle it in one command. If tls12 is indeed absent, the failure mode is the shape this repo has already shipped twice: CI passes (redis 8.6 will happily do 1.3), and the binary fails to handshake against TLS 1.2-pinned or managed endpoints, which is a good share of what a benchmark tool gets pointed at. Either way, I'd spell the intended set out explicitly — features = ["ring", "std", "tls12"] — rather than depending on a transitive dependency's private feature choices to supply std, and extend the cargo tree guard you already added for tls-rustls-insecure to assert ring is enabled on rustls too. That guard currently protects the feature that turns --insecure into a no-op but not the one whose absence caused the panic this PR is actually fixing.

3. std::env::set_var in the new tests is fine on edition 2021 and a hard error on 2024. ENV_MUTEX serializes the four env tests against each other, which is the right instinct, but it doesn't serialize them against the rest of the src/main.rs test binary — set_var is process-global and unsound against a concurrent getenv from any other thread, which is why Rust 2024 made it unsafe. Nothing else in that binary reads the environment today so this almost certainly can't bite right now; it's just that these are the first env-mutating tests in the repo and an edition bump stops compiling them. Not blocking, and I'd rather have the tests than not — the REDIS_TLS_INSECURE=0-is-a-parse-error case in particular is the right thing to pin, given a script exporting =0 to mean "off" silently enabling verification skip would be the genuinely dangerous outcome.

On the parts I did check and think are solid: the #insecure fragment survives the later set_path("/{db}") in build_redis_url() (and the test with db: Some(0) pins that), url_disables_cert_verification()'s exact-match-plus-rediss-scheme gate lines up with how redis-rs derives insecure from the fragment, and keying the warning off the built URL rather than cli.insecure closes a real gap. The negative case in the new CI step is stronger than it looks — asserting the literal invalid peer certificate text proves the handshake got as far as certificate validation, which is exactly what a missing CryptoProvider would prevent, so it pins the panic class and not just "exited non-zero". Coverage looks reasonable against CONTRIBUTING.md's "all new behaviour must be covered by tests"; nothing here touches producer.rs/worker.rs/job.rs, so the wire-level MONITOR evidence AGENTS.md asks for isn't in scope, and the open cluster-mode gap in #5 is untouched.

For the record on process: this repo has no human-authored review comments anywhere in its history, so where the description says "per review" or "round-N review" there's no recorded reviewer position for me to check any of this against — everything above is reasoned from the diff and from AGENTS.md/CONTRIBUTING.md. AGENTS.md's "do not introduce new dependencies without checking with the maintainer" applies to the new direct rustls dep and hasn't been exercised by any prior PR here, so there's no precedent for what that check looks like; a line in the description recording it would be worth having even with the sole maintainer as author. CONTRIBUTING.md's one-maintainer-approval-before-merge line still applies.