Ignore cacert for internal requests if not configured#8099
Conversation
94809f3 to
a23169f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/common/http_client.rs`:
- Around line 90-97: The comment is misleading: update the comment near
verify_https_client_certificate where builder.danger_accept_invalid_certs(true)
and danger_accept_invalid_hostnames(true) are set to state that disabling
verification is only required when the server uses certificates not trusted by
the system/root store (e.g., internal/self-signed certs), and that
reqwest/system roots can still verify public CAs when no custom ca_cert is
provided; also clarify that verify_https_client_certificate currently controls
client identity (mTLS) and is being reused here to gate server cert
verification—add a note suggesting introducing a dedicated flag like
verify_https_server_certificate or skip_server_cert_verification to avoid
semantic overload (optional refactor).
| } else if !verify_https_client_certificate { | ||
| // If ca_cert is not provided, and we are not verifying client certificate, | ||
| // there is no way to verify https connection. | ||
| // | ||
| // So we have to disable certificate verification in order to be able to connect to the server. | ||
| builder = builder | ||
| .danger_accept_invalid_certs(true) | ||
| .danger_accept_invalid_hostnames(true); |
There was a problem hiding this comment.
Misleading comment: system root certificates can still verify server certs even without a custom CA.
The comment on lines 91–93 states "there is no way to verify https connection," but reqwest ships with (or delegates to) a default system/root certificate store. Connections to servers with certificates signed by well-known CAs would still be verifiable without a custom ca_cert. The actual intent is to support internal self-signed certs that aren't in the system trust store — consider rewording the comment to reflect that.
Additionally, verify_https_client_certificate semantically controls whether the client presents an identity for mutual TLS (lines 100–105), but here it's being reused to gate server certificate verification. This semantic overload may confuse future readers. A dedicated flag (e.g., verify_https_server_certificate or skip_server_cert_verification) would be clearer, though this is not blocking.
🤖 Prompt for AI Agents
In `@src/common/http_client.rs` around lines 90 - 97, The comment is misleading:
update the comment near verify_https_client_certificate where
builder.danger_accept_invalid_certs(true) and
danger_accept_invalid_hostnames(true) are set to state that disabling
verification is only required when the server uses certificates not trusted by
the system/root store (e.g., internal/self-signed certs), and that
reqwest/system roots can still verify public CAs when no custom ca_cert is
provided; also clarify that verify_https_client_certificate currently controls
client identity (mTLS) and is being reused here to gate server cert
verification—add a note suggesting introducing a dedicated flag like
verify_https_server_certificate or skip_server_cert_verification to avoid
semantic overload (optional refactor).
There was a problem hiding this comment.
We don't and can't use well-known certificates to connect between nodes in cluster
There was a problem hiding this comment.
@generall, thank you for the clarification! You're absolutely right — since this HTTP client is specifically for internal cluster communication between Qdrant nodes, relying on system root certificates for well-known CAs doesn't apply. The code behavior makes perfect sense in that context: when no custom CA certificate is configured and client certificate verification is disabled, accepting invalid certificates is necessary to allow internal node-to-node communication with self-signed or internal certificates.
I appreciate you providing this context about the internal cluster use case!
🐰
(=''=)
(")(")
curious
✏️ Learnings added
Learnt from: generall
Repo: qdrant/qdrant PR: 8099
File: src/common/http_client.rs:90-97
Timestamp: 2026-02-11T14:28:06.018Z
Learning: The `HttpClient` in `src/common/http_client.rs` is used for internal cluster communication between Qdrant nodes, not for general external HTTP requests. Internal cluster communication uses self-signed or internal certificates, not well-known/public CA certificates.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/e2e_tests/tls_cluster/tls_config.yaml`:
- Around line 23-32: The comment above the tls block is incorrect: the TLS file
paths (tls.cert, tls.key, tls.ca_cert) in tls_config.yaml are hardcoded, and the
launch script uses symlinks (ln -sfn "$CERT_DIR" "$WORK_DIR/node$i/tls") rather
than environment-variable substitution; update the comment to reflect that TLS
paths are provided by symlinking the desired certificate directory into each
node's tls folder (or document the actual env-var mechanism if you switch to env
vars), and ensure the comment references the tls keys (cert, key, ca_cert) so
readers know which fields are affected.
🧹 Nitpick comments (2)
tests/e2e_tests/tls_cluster/run_tls_cluster.sh (2)
46-64:cdinlaunch()changes the caller's working directory.
cd "$WORK_DIR/$name"on Line 50 mutates the shell's cwd sincelaunch()isn't run in a subshell. This works today because all subsequent paths are absolute, but it's fragile. Wrapping the body in a subshell(…)would be safer, though you'd need to capture the PID differently.A simpler fix — use
pushd/popdor just pass the working directory via env or-C-style flag if the binary supports it:Minimal fix using a subshell
launch() { local name=$1 http=$2 grpc=$3 p2p=$4; shift 4 echo "Starting $name (HTTP=$http gRPC=$grpc P2P=$p2p)..." - cd "$WORK_DIR/$name" QDRANT__LOG_LEVEL="debug,raft=info,hyper=info,wal=info,h2=info,tower=info,rustls=info" \ QDRANT__SERVICE__HTTP_PORT="$http" \ QDRANT__SERVICE__GRPC_PORT="$grpc" \ QDRANT__SERVICE__STATIC_CONTENT_DIR="$PROJECT_ROOT/static" \ QDRANT__CLUSTER__P2P__PORT="$p2p" \ QDRANT__STORAGE__STORAGE_PATH="$WORK_DIR/$name/storage" \ QDRANT__STORAGE__HNSW_INDEX__MAX_INDEXING_THREADS=1 \ QDRANT_NUM_CPUS=1 \ - "$BINARY" --config-path "$CONFIG_FILE" --disable-telemetry "$@" \ + bash -c 'cd "$1" && shift && exec "$@"' _ "$WORK_DIR/$name" "$BINARY" --config-path "$CONFIG_FILE" --disable-telemetry "$@" \ > "$WORK_DIR/$name/$name.log" 2>&1 & PIDS+=($!) }
66-76:wait_readydoesn't fail the script on timeout.The function prints a warning but returns 0, so under
set -ethe script happily continues even if a node never becomes ready. This is probably fine for a manual test harness — just noting it in case you want stricter behavior (e.g.,return 1after the warning).
| # TLS settings - paths are set via environment variables in the launch script | ||
| tls: | ||
| # Certificate chain file | ||
| cert: ./tls/cert.pem | ||
|
|
||
| # Private key file | ||
| key: ./tls/key.pem | ||
|
|
||
| # CA certificate | ||
| ca_cert: ./tls/cacert.pem |
There was a problem hiding this comment.
Misleading comment: paths are hardcoded, not set via environment variables.
Line 23 says "paths are set via environment variables in the launch script," but the launch script doesn't override these TLS paths with env vars — it relies on symlinks instead (ln -sfn "$CERT_DIR" "$WORK_DIR/node$i/tls"). Consider updating the comment to reflect the actual mechanism.
Suggested fix
-# TLS settings - paths are set via environment variables in the launch script
+# TLS settings - paths are relative to each node's working directory (symlinked by run_tls_cluster.sh)📝 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.
| # TLS settings - paths are set via environment variables in the launch script | |
| tls: | |
| # Certificate chain file | |
| cert: ./tls/cert.pem | |
| # Private key file | |
| key: ./tls/key.pem | |
| # CA certificate | |
| ca_cert: ./tls/cacert.pem | |
| # TLS settings - paths are relative to each node's working directory (symlinked by run_tls_cluster.sh) | |
| tls: | |
| # Certificate chain file | |
| cert: ./tls/cert.pem | |
| # Private key file | |
| key: ./tls/key.pem | |
| # CA certificate | |
| ca_cert: ./tls/cacert.pem |
🤖 Prompt for AI Agents
In `@tests/e2e_tests/tls_cluster/tls_config.yaml` around lines 23 - 32, The
comment above the tls block is incorrect: the TLS file paths (tls.cert, tls.key,
tls.ca_cert) in tls_config.yaml are hardcoded, and the launch script uses
symlinks (ln -sfn "$CERT_DIR" "$WORK_DIR/node$i/tls") rather than
environment-variable substitution; update the comment to reflect that TLS paths
are provided by symlinking the desired certificate directory into each node's
tls folder (or document the actual env-var mechanism if you switch to env vars),
and ensure the comment references the tls keys (cert, key, ca_cert) so readers
know which fields are affected.
* ignore cert verification if not configured * also ignore hostnames * script for manually testing tls clsuter
No description provided.