feat(pem): add authenticated direct-query gRPC endpoint by entlein · Pull Request #2401 · pixie-io/pixie

entlein · 2026-08-10T14:21:47Z

Exposes VizierService.ExecuteScript directly on the normal PEM over a dedicated port (:50305 default), so node-local clients can query Carnot without a broker hop or cloud dependency.

The feature is opt-in (--direct_query_enabled=false by default) so existing PEM deployments are byte-for-byte unchanged until opted in. A compile-time kill switch (--//src/vizier/services/agent/pem:direct_query=disabled) removes the feature entirely from the binary.

Changes:

direct_query_server.{h,cc} (new)

DirectQueryServer implements api::vizierpb::VizierService::ExecuteScript against the live PEM Carnot (reusing its table_store + metadata callback).
AuthenticateRequest: HS256 JWT verifier via BoringSSL HMAC + rapidjson. Requires aud=vizier, iss=PL, Scopes=service, valid exp. Defends against alg:none, wrong-key, expired, tampered-payload attacks.
Fail-soft startup: init failure never brings the PEM data plane down.
ExecuteScript only; mutations return UNIMPLEMENTED.

direct_query_server_test.cc (new)

In-process gRPC fixture exercises the full auth metadata flow.
Auth-negative cases: no token, wrong key, expired, alg:none, truncated, tampered header/payload/sig, wrong aud/iss/scope.
Execution cases: trivial query, projection, time-range, join, concurrent.

BUILD.bazel (modified)

config_setting(:direct_query_disabled) for compile-time kill switch.
New deps on //src/carnot, @boringssl//:crypto, @rapidjson.
New pl_cc_test(:direct_query_server_test).

pem_manager.{h,cc} (modified)

MaybeStartDirectQueryServer() builds and starts the gRPC server after broker registration; StopImpl tears it down.
Flags: --direct_query_enabled (default false), --direct_query_port (default 50305), --direct_query_jwt_signing_key.

shared/manager/manager.cc (modified)

Empty-key guard at Manager::Init: refuses an empty PL_JWT_SIGNING_KEY rather than crashing mid-stream on the first service call.

DIRECT_QUERY_CONTRACT.md, DIRECT_QUERY_SECURITY.md (new)

Behavioral spec, auth requirements, threat model, key-flow diagram, tampering test matrix, TLS transport details, disable instructions.

@rapidjson

Exposes VizierService.ExecuteScript directly on the normal PEM over a dedicated port (:50305 default), so node-local clients can query Carnot without a broker hop or cloud dependency. The feature is opt-in (--direct_query_enabled=false by default) so existing PEM deployments are byte-for-byte unchanged until opted in. A compile-time kill switch (--//src/vizier/services/agent/pem:direct_query=disabled) removes the feature entirely from the binary. Changes: direct_query_server.{h,cc} (new) - DirectQueryServer implements api::vizierpb::VizierService::ExecuteScript against the live PEM Carnot (reusing its table_store + metadata callback). - AuthenticateRequest: HS256 JWT verifier via BoringSSL HMAC + rapidjson. Requires aud=vizier, iss=PL, Scopes=service, valid exp. Defends against alg:none, wrong-key, expired, tampered-payload attacks. - Fail-soft startup: init failure never brings the PEM data plane down. - ExecuteScript only; mutations return UNIMPLEMENTED. direct_query_server_test.cc (new) - In-process gRPC fixture exercises the full auth metadata flow. - Auth-negative cases: no token, wrong key, expired, alg:none, truncated, tampered header/payload/sig, wrong aud/iss/scope. - Execution cases: trivial query, projection, time-range, join, concurrent. BUILD.bazel (modified) - config_setting(:direct_query_disabled) for compile-time kill switch. - New deps on //src/carnot, @boringssl//:crypto, @rapidjson. - New pl_cc_test(:direct_query_server_test). pem_manager.{h,cc} (modified) - MaybeStartDirectQueryServer() builds and starts the gRPC server after broker registration; StopImpl tears it down. - Flags: --direct_query_enabled (default false), --direct_query_port (default 50305), --direct_query_jwt_signing_key. shared/manager/manager.cc (modified) - Empty-key guard at Manager::Init: refuses an empty PL_JWT_SIGNING_KEY rather than crashing mid-stream on the first service call. DIRECT_QUERY_CONTRACT.md, DIRECT_QUERY_SECURITY.md (new) - Behavioral spec, auth requirements, threat model, key-flow diagram, tampering test matrix, TLS transport details, disable instructions. Signed-off-by: entlein <einentlein@gmail.com>

ddelnano · 2026-08-20T13:50:20Z

+// We don't link cpp_jwt's HMAC verifier here because its impl calls
+// BIO_f_base64() which lives in BoringSSL's decrepit/ tree — not exposed as a
+// bazel target on this fork. Instead we parse the JWT envelope manually and
+// HMAC with BoringSSL natively. ~50 lines vs. carrying a boringssl patch.


Is this statement true? I think the cpp_jwt's HMAC path will work with our version of boringssl. If not, it seems like that's true of newer versions of cpp_jwt (I checked the latest main).

ddelnano · 2026-08-20T13:56:49Z

+    defines = select({
+        ":direct_query_disabled": ["PX_PEM_DIRECT_QUERY_DISABLED"],
+        "//conditions:default": [],
+    }),


This should use local_defines otherwise it will propagate to every transitive dependency.

ddelnano · 2026-08-20T13:58:29Z

+config_setting(
+    name = "direct_query_disabled",
+    define_values = {"PX_PEM_DIRECT_QUERY": "disabled"},
+)


Did you try the bazel build … --//src/vizier/services/agent/pem:direct_query=disabled build and verify it works? I believe you need a bool_flag associated with this. I think it would be better as a bool_flag so it can work with --//src/vizier/services/agent/pem:direct_query=false/true

ddelnano · 2026-08-20T14:02:07Z

+    acc |= static_cast<uint8_t>(a[i] ^ b[i]);
+  }
+  return acc == 0;
+}


I think we want to avoid handling constant time crypto ourselves. If we must do it, I think using CRYPTO_memcp would be the way to go.

ddelnano · 2026-08-20T14:04:16Z

+// bazel target on this fork. Instead we parse the JWT envelope manually and
+// HMAC with BoringSSL natively. ~50 lines vs. carrying a boringssl patch.
+
+// stripBearerPrefix returns the token slice after a case-insensitive "Bearer "


I think all of the jwt handling code should go in a new location (/src/shared/services/jwt). Since this has vizier specific information, I think it makes more sense there then a more general src/common or similar directory.

This new directory can hold something like this:

// src/shared/services/jwt/service_token.h Status VerifyServiceJWT(std::string_view token, std::string_view signing_key, const ServiceJWTOptions& opts); // aud, iss, required scopes, leeway

ddelnano · 2026-08-20T14:05:25Z

+// emitSchemaResponses walks the compiled plan once and writes a meta_data-only
+// ExecuteScriptResponse per GRPC_SINK_OPERATOR sink. The client uses these to
+// learn output table names and column types before the data chunks arrive.
+// Mirrors standalone_pem/vizier_server.h:132-173.
+void emitSchemaResponses(const ::px::carnot::planpb::Plan& plan, const std::string& query_id,
+                         ::grpc::ServerWriter<::px::api::vizierpb::ExecuteScriptResponse>* writer) {
+  for (const auto& f : plan.nodes()) {
+    for (const auto& n : f.nodes()) {
+      if (n.op().op_type() != ::px::carnot::planpb::OperatorType::GRPC_SINK_OPERATOR) continue;
+      const auto& sink = n.op().grpc_sink_op();
+      if (!sink.has_output_table()) continue;


Can we reuse all of the code in src/experimental/standalone_pem/sink_server.h and src/experimental/standalone_pem/vizier_server.h ? If the existing standalone pem implementation has slight differences, we should resolve those or adapt it to be general enough for both.

ddelnano · 2026-08-20T14:06:48Z

+package(default_visibility = [
+    "//src/carnot:__subpackages__",
+    "//src/vizier/services/agent:__subpackages__",
+])


I think it'd be best to avoid making this package broadly open to all //src/vizier/services/agent subpackages. I need to think about how to scope this more tightly, but we do have a similar pattern in here.

I think we need to consider this for the other package visibility changes as well. I'll have to think about this more before I have any proposals on how to structure it better.

ddelnano · 2026-08-20T14:09:06Z

+
+// stripBearerPrefix returns the token slice after a case-insensitive "Bearer "
+// prefix, or an empty string if the prefix is missing. gRPC normalises metadata
+// keys to lowercase but does NOT touch values; manager.cc:440 mints with a


In general, I think we should avoid file_name.cc:{line_num} references. These are guaranteed to break when someone refactors the referenced code.

I think github permalinks are the way to go for these reference. Please analyze all of them in this PR.

ddelnano · 2026-08-20T14:11:21Z

+bool base64UrlDecode(absl::string_view in, std::string* out) {
+  // absl handles standard base64 with '+'/'/'; translate URL-safe alphabet and
+  // pad to a multiple of 4 first.
+  std::string std_b64;
+  std_b64.reserve(in.size() + 4);
+  for (char c : in) {
+    if (c == '-') {
+      std_b64.push_back('+');
+    } else if (c == '_') {
+      std_b64.push_back('/');
+    } else if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') ||
+               c == '+' || c == '/') {
+      std_b64.push_back(c);
+    } else {
+      return false;
+    }
+  }
+  while (std_b64.size() % 4 != 0) std_b64.push_back('=');
+  return absl::Base64Unescape(std_b64, out);
+}


The jwt library we use has a header only implementation of this. I'm hoping we can defer to it for the entire crypto part (which would include the base64 part), but if we need to handle the crypto we should use that header only library.

…defines Review feedback on pixie-io#2401. The BUILD comment advertised `--//src/vizier/services/agent/pem:direct_query=disabled`, but no such target existed — `bazel query` fails with "target 'direct_query' not declared" — so the only working off-switch was `--define`. Declare the bool_flag the comment promised and key the config_setting off it, so the documented invocation works and the setting stays scoped to the targets that read it instead of the global --define configuration key. Also switches cc_library from `defines` to `local_defines` so the macro stops propagating to every transitive dependent. Because that macro no longer reaches dependents, direct_query_server_test selects on the same config_setting itself — without that its `#ifdef PX_PEM_DIRECT_QUERY_DISABLED` assertions would silently compile the enabled branch in a disabled build. Verified with --config=x86_64_sysroot: default build compiles the feature in (34 passed / 3 skipped), --//…:direct_query=false compiles the macro into both the library and the test, and the resulting binary behaves identically to the previous --define path (38 tests, 7 pass, 28 fail — unchanged from the old mechanism; those failures are a separate issue). NOTE: --define=PX_PEM_DIRECT_QUERY=disabled no longer has any effect.

The kill-switch build compiled but its tests did not run clean — 28 of 38 failed under --define=PX_PEM_DIRECT_QUERY=disabled before this change, so the disabled configuration was evidently never exercised. Two causes: - Only the two CompiledOut_* tests sat behind the #ifdef. Every other test asserts enabled-path behaviour (valid tokens stream rows, tampered tokens are rejected by the verifier) and ran against the linker stubs, which have none of that behaviour. Guard the enabled-path block, and the trailing benchmark placeholder that uses the exec fixture, with #ifndef. - The CompiledOut_* expectations contradicted the stub they test: DirectQueryServer::ExecuteScript returns UNIMPLEMENTED without consulting credentials, and DIRECT_QUERY_SECURITY.md documents exactly that as the user-visible error, but the tests expected UNAUTHENTICATED. Expect UNIMPLEMENTED, and cover the fail-closed AuthenticateRequest stub directly so 'no token can re-enable the feature' keeps its assertion. kWrongSigningKey is only used by the guarded tests, so it needs [[maybe_unused]] to survive -Wunused-const-variable in a disabled build. Verified with --config=x86_64_sysroot: default 37 tests, 34 passed, 3 skipped --//…:direct_query=false 3 tests, 3 passed

…ed ones Review feedback on pixie-io#2401. constantTimeEquals hand-rolled the XOR-accumulate compare; BoringSSL ships CRYPTO_memcmp for exactly this, so call it. Length is still compared first — the signature length follows from the algorithm, not the secret, so an early exit there leaks nothing about the key. base64UrlDecode hand-rolled the URL-safe alphabet translation on top of absl::Base64Unescape; cpp_jwt's base64_uri_decode does the same job and is the one part of that library that needs no BIO, so it links against our BoringSSL. It is lenient where ours was strict — a non-alphabet byte yields a partial decode instead of an error — which changes no behaviour here: the HMAC over header.payload is verified BEFORE the payload is decoded, and a truncated decode then fails to parse as JSON. NonBase64UrlCharInPayload_Unauthenticated pins that so it cannot regress behind either decoder. Reworded the cpp_jwt rationale comment. BIO_f_base64 IS declared in BoringSSL's public bio.h; it is the implementation in decrepit/bio/base64_bio.c that @boringssl//:crypto does not build. Verified by linking a probe that calls jwt::decode(..., verify(true)): 'ld.lld: error: undefined symbol: BIO_f_base64'. Signing works today (shared/manager/manager.cc) because HMACSign<>::sign takes the header-only base64 path and never touches a BIO. Verified with --config=x86_64_sysroot: default 35 tests, 35 passed --//…:direct_query=false 3 tests, 3 passed

entlein had a problem deploying to pr-actions-approval August 10, 2026 14:22 — with GitHub Actions Error

entlein force-pushed the feat/pem-direct-query branch from 86713e2 to 7f63749 Compare August 10, 2026 14:27

entlein had a problem deploying to pr-actions-approval August 10, 2026 14:28 — with GitHub Actions Error

entlein marked this pull request as ready for review August 11, 2026 09:56

entlein requested a review from a team as a code owner August 11, 2026 09:56

ddelnano reviewed Aug 20, 2026

View reviewed changes

ConstanzeTU had a problem deploying to pr-actions-approval August 20, 2026 16:09 — with GitHub Actions Error

ConstanzeTU requested a deployment to pr-actions-approval August 20, 2026 16:18 — with GitHub Actions Waiting

ConstanzeTU requested a deployment to pr-actions-approval August 20, 2026 17:54 — with GitHub Actions Waiting

Navigation Menu

Uh oh!

feat(pem): add authenticated direct-query gRPC endpoint - #2401

feat(pem): add authenticated direct-query gRPC endpoint#2401
entlein wants to merge 4 commits into
pixie-io:mainfrom
k8sstormcenter:feat/pem-direct-query

entlein commented Aug 10, 2026

Uh oh!

ddelnano Aug 20, 2026

Uh oh!

ddelnano Aug 20, 2026

Uh oh!

ddelnano Aug 20, 2026

Uh oh!

ddelnano Aug 20, 2026

Uh oh!

ddelnano Aug 20, 2026

Uh oh!

ddelnano Aug 20, 2026

Uh oh!

ddelnano Aug 20, 2026

Uh oh!

ddelnano Aug 20, 2026

Uh oh!

ddelnano Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

entlein commented Aug 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants