fix(pem): use CRYPTO_memcmp and cpp_jwt's base64 instead of hand-rolled ones

Review feedback on #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

Author: constanze

src/vizier/services/agent/pem/BUILD.bazel

@@ -75,6 +75,7 @@ pl_cc_library(
         "//src/carnot/udf:cc_library",
         # HS256 verify uses BoringSSL HMAC directly and parses claims via rapidjson.
         "@boringssl//:crypto",
+        "@com_github_arun11299_cpp_jwt//:cpp_jwt",
         "@com_github_grpc_grpc//:grpc++",
         "@com_github_rlyeh_sole//:sole",
         "@com_github_tencent_rapidjson//:rapidjson",

src/vizier/services/agent/pem/direct_query_server.cc

@@ -30,6 +30,7 @@
 // "missing include". The disabled build pays a few KB of unused header
 // parse cost; the .cc emits nothing for them.
 #include <openssl/hmac.h>
+#include <openssl/mem.h>
 #include <openssl/sha.h>
 #include <rapidjson/document.h>
 
@@ -39,10 +40,10 @@
 #include <string>
 #include <vector>
 
-#include <absl/strings/escaping.h>
 #include <absl/strings/str_split.h>
 #include <absl/strings/string_view.h>
 #include <absl/strings/substitute.h>
+#include <jwt/base64.hpp>
 #include <sole.hpp>
 
 // Compile-time kill switch. When PX_PEM_DIRECT_QUERY_DISABLED is defined
@@ -78,10 +79,16 @@ constexpr char kExpectedIssuer[] = "PL";
 // the serviceID, e.g. "dx"). See GenerateJWTForService (claims.go) + jwt.go:56.
 constexpr char kServiceScope[] = "service";
 
-// 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.
+// cpp_jwt mints our outgoing service tokens (shared/manager/manager.cc), but we
+// cannot use it to VERIFY here: HMACSign<>::verify (impl/algorithm.ipp) base64s
+// through BIO_f_base64(). BoringSSL declares that in the public bio.h but
+// implements it in decrepit/bio/base64_bio.c, which @boringssl//:crypto does not
+// build — linking a jwt::decode(..., verify(true)) call fails with
+// `undefined symbol: BIO_f_base64`. (Signing links because HMACSign<>::sign uses
+// HMAC() plus cpp_jwt's header-only base64, no BIO.) Using the library for
+// verification would mean patching the BoringSSL external to add a decrepit
+// target; instead we parse the envelope here and HMAC with BoringSSL natively.
+// Its base64url decoder needs no BIO, so we do reuse that below.
 
 // stripBearerPrefix returns the token slice after a case-insensitive "Bearer "
 // prefix, or an empty string if the prefix is missing. gRPC normalises metadata
@@ -100,39 +107,21 @@ absl::string_view stripBearerPrefix(absl::string_view value) {
   return value.substr(kBearerPrefixLen);
 }
 
-// constantTimeEquals: short-circuit-free byte compare. Mismatched-length inputs
-// trivially differ but we still walk the shorter to keep timing predictable
-// across malformed lengths.
+// constantTimeEquals: BoringSSL's CRYPTO_memcmp, which is the library's own
+// constant-time comparison — no hand-rolled crypto here. Length is compared
+// first: the signature length is a function of the algorithm, not of the
+// secret, so leaking "wrong length" leaks nothing about the key.
 bool constantTimeEquals(absl::string_view a, absl::string_view b) {
   if (a.size() != b.size()) return false;
-  uint8_t acc = 0;
-  for (size_t i = 0; i < a.size(); ++i) {
-    acc |= static_cast<uint8_t>(a[i] ^ b[i]);
-  }
-  return acc == 0;
+  return CRYPTO_memcmp(a.data(), b.data(), a.size()) == 0;
 }
 
-// base64UrlDecode handles RFC 7515 base64url (no padding, '-' / '_' alphabet).
-// Returns false on any non-alphabet character.
+// base64UrlDecode handles RFC 7515 base64url (no padding, '-' / '_' alphabet),
+// delegating the transform to cpp_jwt's header-only decoder (the one part of
+// that library that needs no BIO, so it links against our BoringSSL).
 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);
+  *out = jwt::base64_uri_decode(in.data(), in.size());
+  return true;
 }
 
 // hmacSha256: BoringSSL HMAC over `data`, returns raw 32 bytes.

src/vizier/services/agent/pem/direct_query_server_test.cc

@@ -446,6 +446,23 @@ TEST_F(DirectQueryServerTest, ConcatenatedTokens_Unauthenticated) {
 // is the explicit "alg substitution" attack from RFC 8725 §2.6 (the more
 // classic variant uses RS256 / public-key confusion; we don't sign with
 // asymmetric keys so the HMAC-flavoured variant is what we guard against).
+// Segments containing characters outside the base64url alphabet must be
+// rejected. The decoder itself is allowed to be lenient — cpp_jwt's header-only
+// base64 returns a partial decode rather than an error — so this pins the
+// property at the level that matters: a token whose payload is not valid
+// base64url never authenticates, no matter which decoder is underneath. It is
+// the HMAC over header.payload that closes the door, and the truncated JSON
+// behind it fails to parse anyway.
+TEST_F(DirectQueryServerTest, NonBase64UrlCharInPayload_Unauthenticated) {
+  auto tok = MakeBearerToken(kTestSigningKey, TokenKind::kValid);
+  auto [p_start, p_end] = SegmentIndex(tok, 1);
+  ASSERT_NE(std::string::npos, p_start);
+  ASSERT_GT(p_end, p_start + 1);
+  auto corrupted = tok;
+  corrupted[p_start + 1] = '!';  // '!' is not in the base64url alphabet.
+  EXPECT_EQ(::grpc::StatusCode::UNAUTHENTICATED, CallExecuteScript(corrupted).error_code());
+}
+
 TEST_F(DirectQueryServerTest, AlgConfusion_HS384_Unauthenticated) {
   // Header: {"alg":"HS384","typ":"JWT"} → base64url, no padding.
   constexpr char kHS384Header[] = "eyJhbGciOiJIUzM4NCIsInR5cCI6IkpXVCJ9";