Skip to content

auth: integrate Catapult CAT token verification#286

Open
mondain wants to merge 4 commits into
feature/authfrom
feature/auth-catapult
Open

auth: integrate Catapult CAT token verification#286
mondain wants to merge 4 commits into
feature/authfrom
feature/auth-catapult

Conversation

@mondain
Copy link
Copy Markdown
Contributor

@mondain mondain commented May 4, 2026

Summary

This is stacked on feature/auth / PR #264 and isolates the Catapult integration requested in review.

  • Adds Quicr/catapult as a submodule and wires it into the CMake and Docker builds.
  • Replaces the local v1 token envelope/CBOR/HMAC parsing with Catapult CWT validation and CAT MOQT claim translation.
  • Keeps the existing relay-facing auth API (AuthTokenVerifier, Grants, and allows) so relay call sites do not churn in this PR.
  • Updates auth tests to issue Catapult CWTs through the local test helper and keeps config resolver coverage from the parent branch.

Notes

The existing config shape is preserved. Configured HMAC secrets are SHA-256 derived into the 32-byte key Catapult expects, so deployments do not need a config format change in this stacked PR.

Validation

Docker-first focused validation on this machine:

docker run --rm -v "$PWD":/src -w /src debian:bookworm bash -lc '
set -e
apt-get update
apt-get install -y --no-install-recommends build-essential ninja-build git ca-certificates python3-pip libssl-dev libunwind-dev libgoogle-glog-dev libgflags-dev libdouble-conversion-dev libevent-dev libsodium-dev libzstd-dev libboost-all-dev libfmt-dev libgtest-dev libgmock-dev zlib1g-dev libc-ares-dev libdwarf-dev
pip install --break-system-packages cmake
cmake -S . -B /tmp/moqx-test -G Ninja -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_PREFIX_PATH=/src/.docker-deps/moxygen -DCMAKE_MODULE_PATH=/src/cmake -DCMAKE_POLICY_VERSION_MINIMUM=3.5 -DCMAKE_FIND_LIBRARY_SUFFIXES=".so;.a" -DGFLAGS_SHARED=ON
cmake --build /tmp/moqx-test --target moqx_auth_test moqx_config_resolver_test -j$(nproc)
ctest --test-dir /tmp/moqx-test -R "Auth|ConfigResolver" --output-on-failure
'

Result: 29/29 tests passed.


This change is Reviewable

@suhasHere
Copy link
Copy Markdown
Contributor

@mondain I will review the PR this week.

@mondain mondain marked this pull request as ready for review May 30, 2026 02:37
@suhasHere
Copy link
Copy Markdown
Contributor

src/auth/Auth.cpp line 188 at r1 (raw file):

  }
  return folly::makeUnexpected(AuthError::BadSignature);
}

this seems fine, but it does add hmac validation per kid even though it is not a match. how about the below change ( based on the latest main)

const auto tokenBytes = toBytes(token.tokenValue);
const auto span = std::span(tokenBytes.data(), tokenBytes.size());

auto tryVerify = [&](const HmacKeyConfig& key)
-> folly::Expected<Grants, AuthError> {
try {
catapult::HmacSha256Algorithm hmac(deriveHmacKey(key.secret));
auto cwt = catapult::Cwt::validateCwt(span, hmac);
auto grants = grantsFromToken(cwt.payload);
if (grants.expiresAt <= std::chrono::system_clock::now()) {
return folly::makeUnexpected(AuthError::Expired);
}
return grants;
} catch (const catapult::CryptoError&) {
return folly::makeUnexpected(AuthError::BadSignature);
} catch (const catapult::CatError&) {
return folly::makeUnexpected(AuthError::Malformed);
}
};

// If kid present, verify only with the matching key
auto header = catapult::Cwt::decodeHeader(span);
if (header.kid.has_value()) {
auto it = std::find_if(config_.hmacKeys.begin(), config_.hmacKeys.end(),
[&](const auto& key) { return key.id == *header.kid; });
if (it == config_.hmacKeys.end()) {
return folly::makeUnexpected(AuthError::BadSignature);
}
return tryVerify(*it);
}

// No kid — trial verification across all keys
for (const auto& key : config_.hmacKeys) {
auto result = tryVerify(key);
if (result.hasValue() || result.error() != AuthError::BadSignature) {
return result;
}
}
return folly::makeUnexpected(AuthError::BadSignature);

@suhasHere
Copy link
Copy Markdown
Contributor

src/auth/Auth.cpp line 58 at r1 (raw file):

}

catapult::MoqtBinaryMatch toCatapultMatch(const std::vector<MatchRule>& rules) {

The old allRulesMatch iterated all rules with std::all_of. If any code path
(config, future callers) builds a scope with multiple rules (Prefix + Suffix to express
a range), the serialization quietly drops the extras

@suhasHere
Copy link
Copy Markdown
Contributor

src/auth/Auth.cpp line 188 at r1 (raw file):

  }
  return folly::makeUnexpected(AuthError::BadSignature);
}

This is bit inefficient in terms of number of HMAC operations done if KID is not a match , also Keys don;t change after AuthTokenVerifier Consutrctions. How about we do this

Code snippet:

   std::unordered_map<std::string, catapult::HmacSha256Algorithm> hmacKeyMap_;
  std::vector<catapult::HmacSha256Algorithm*> hmacKeyList_;  // for no-kid fallback

  // In constructor
  for (const auto& key : config.hmacKeys) {
    auto [it, _] = hmacKeyMap_.emplace(key.id,
        catapult::HmacSha256Algorithm(deriveHmacKey(key.secret)));
    hmacKeyList_.push_back(&it->second);
  }

  const auto tokenBytes = toBytes(token.tokenValue);
  const auto span = std::span<const uint8_t>(tokenBytes.data(), tokenBytes.size());

  auto tryVerify = [&](const catapult::HmacSha256Algorithm& hmac)
      -> folly::Expected<Grants, AuthError> {
    try {
      auto cwt = catapult::Cwt::validateCwt(span, hmac);
      auto grants = grantsFromToken(cwt.payload);
      if (grants.expiresAt <= std::chrono::system_clock::now()) {
        return folly::makeUnexpected(AuthError::Expired);
      }
      return grants;
    } catch (const catapult::CryptoError&) {
      return folly::makeUnexpected(AuthError::BadSignature);
    } catch (const catapult::CatError&) {
      return folly::makeUnexpected(AuthError::Malformed);
    }
  };

  auto header = catapult::Cwt::decodeHeader(span);
  if (header.kid.has_value()) {
    auto it = hmacKeyMap_.find(*header.kid);
    if (it == hmacKeyMap_.end()) {
      return folly::makeUnexpected(AuthError::BadSignature);
    }
    return tryVerify(it->second);
  }

  for (const auto* hmac : hmacKeyList_) {
    auto result = tryVerify(*hmac);
    if (result.hasValue() || result.error() != AuthError::BadSignature) {
      return result;
    }
  }
  return folly::makeUnexpected(AuthError::BadSignature);

@suhasHere
Copy link
Copy Markdown
Contributor

src/auth/Auth.h line 75 at r1 (raw file):

  static std::string
  signForTest(std::string_view keyID, std::string_view secret, const Grants& grants);

The old API accepted raw CBOR bytes, enabling tests to construct deliberately
malformed payloads. The new one only produces well-formed CWTs. The tests
RejectsMalformedClaims and most of RejectsMalformedTokenEnvelope were deleted as a
result — this loses negative-path coverage.

how about we do the below

Code snippet:

 // For normal test usage:
    static std::string signForTest(std::string_view keyID, std::string_view secret, const
  Grants& grants);

    // For malformed-input tests (signs arbitrary bytes as CWT payload):
    static std::string signRawForTest(std::string_view keyID, std::string_view secret,
  std::span<const uint8_t> rawPayload);

  And alsoadd a test that verifies garbage COSE bytes return Malformed (not crash):

  // test/AuthTest.cpp — add after RejectsEmptyTokenAsMalformed
  TEST(AuthTest, RejectsGarbageBytesAsMalformedOrBadSig) {
    AuthTokenVerifier verifier(makeConfig());
    AuthToken token{.tokenType = 77, .tokenValue = "\xde\xad\xbe\xef", .alias =
  AuthToken::DontRegister};
    auto result = verifier.verify(token);
    ASSERT_TRUE(result.hasError());
    // Either Malformed (invalid COSE) or BadSignature (all keys failed) is acceptable
    EXPECT_THAT(result.error(), testing::AnyOf(AuthError::Malformed,
  AuthError::BadSignature));
  }

@suhasHere
Copy link
Copy Markdown
Contributor

src/auth/Auth.cpp line 44 at r1 (raw file):

  std::vector<uint8_t> key(SHA256_DIGEST_LENGTH);
  SHA256(reinterpret_cast<const unsigned char*>(secret.data()), secret.size(), key.data());
  return key;

I have some concerns here

  1. No domain separation — same secret reused elsewhere yields identical key material
  2. No salt/info binding — not a proper KDF per NIST SP 800-108 or RFC 5869
  3. If the input secret has less than 256 bits of entropy, SHA-256 doesn't stretch it (it's
    not a password-based KDF either)

We can consider 2 options

  1. If we control key generation, then use something like
    // Best: generate a random key and store it directly — no derivation needed
    auto key = catapult::HmacSha256Algorithm::generateSecureKey();

  2. My preferred wat though is to derive HKDF key properly

Code snippet:

// Fixed salt — could be all-zeros (per RFC 5869 §3.1) or a domain-specific constant.
  // Using a fixed non-secret value is fine; it just needs to be stable across
  deployments.
  constexpr std::string_view kHkdfSalt = "moqx-catapult-v1";

  // Info string binds the derived key to this specific usage.
  constexpr std::string_view kHkdfInfo = "moqx-catapult-hmac-token-verify";

  std::vector<uint8_t> deriveHmacKey(std::string_view secret) {
    std::vector<uint8_t> key(32);  // 256-bit output

    auto* ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, nullptr);
    if (!ctx) {
      throw std::runtime_error("EVP_PKEY_CTX_new_id failed");
    }
    auto guard = std::unique_ptr<EVP_PKEY_CTX, decltype(&EVP_PKEY_CTX_free)>(ctx,
  EVP_PKEY_CTX_free);

    size_t keyLen = key.size();
    if (EVP_PKEY_derive_init(ctx) <= 0 ||
        EVP_PKEY_CTX_set_hkdf_md(ctx, EVP_sha256()) <= 0 ||
        EVP_PKEY_CTX_set1_hkdf_salt(
            ctx,
            reinterpret_cast<const unsigned char*>(kHkdfSalt.data()),
            kHkdfSalt.size()) <= 0 ||
        EVP_PKEY_CTX_set1_hkdf_key(
            ctx,
            reinterpret_cast<const unsigned char*>(secret.data()),
            secret.size()) <= 0 ||
        EVP_PKEY_CTX_add1_hkdf_info(
            ctx,
            reinterpret_cast<const unsigned char*>(kHkdfInfo.data()),
            kHkdfInfo.size()) <= 0 ||
        EVP_PKEY_derive(ctx, key.data(), &keyLen) <= 0) {
      throw std::runtime_error("HKDF derivation failed");
    }
    key.resize(keyLen);
    return key;
  }

Copy link
Copy Markdown
Contributor

@suhasHere suhasHere left a comment

Choose a reason for hiding this comment

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

The catapult API usage is correct (CwtMode::MACed, exception hierarchy, MoqtClaims/BinaryMatch factories all look right). Main concern is the loss of key-ID-based lookup in the verify path — lets please restore that before merge since it's both a perf and security regression. Also the multi-rule truncation should at least be loudly documented. Otherwise looks good to land.

@suhasHere made 2 comments and resolved 1 discussion.
Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on afrind, akash-a-n, gmarzot, michalhosna, mondain, Oxyd, peterchave, and TimEvens).


src/auth/Auth.cpp line 58 at r1 (raw file):

Previously, suhasHere (Suhas Nandakumar) wrote…

The old allRulesMatch iterated all rules with std::all_of. If any code path
(config, future callers) builds a scope with multiple rules (Prefix + Suffix to express
a range), the serialization quietly drops the extras

how about

catapult::MoqtBinaryMatch toCatapultMatch(const std::vector& rules) {
if (rules.empty()) {
return catapult::MoqtBinaryMatch::any();
}
if (rules.size() > 1) {
XLOG(WARN, "CWT supports single match rule per scope; dropping {} extra rules",
rules.size() - 1);
}
const auto& rule = rules.front();
// ...

- auth: derive each configured HMAC key once at AuthTokenVerifier construction
  and reuse it in verify(), instead of re-deriving per key on every call. A true
  key-id short-circuit isn't possible yet (Catapult has no API to read the CWT
  kid without first validating), so verify() still trial-verifies, but the
  expensive derivation no longer repeats.
- auth: derive the HMAC key via HKDF-SHA256 (RFC 5869) with domain-separated
  salt/info instead of a bare SHA-256 of the secret. Sign and verify share the
  derivation, so this is self-consistent.
- auth: warn loudly in toCatapultMatch when a scope has >1 match rule, since a
  CWT scope carries a single binary match per dimension and the extras are
  dropped (which would widen the grant).
- test: add RejectsGarbageBytesAsMalformedOrBadSig to restore negative-path
  coverage for non-empty, structurally-invalid tokens.
@mondain
Copy link
Copy Markdown
Contributor Author

mondain commented May 30, 2026

src/auth/Auth.cpp line 188 at r1

[AI] Restructured verify() along these lines in f724e37, with one constraint: this Catapult version has no Cwt::decodeHeader (or any other way to read the CWT kid without first validating against a key), so the if (header.kid) short-circuit isn't possible yet. What I did instead: derive each configured key once in the AuthTokenVerifier constructor and reuse it, so the per-key derivation no longer repeats on every verify(); verify() then trial-verifies against the cached keys (same shape -- continue on CryptoError, propagate the first non-BadSignature). That removes the per-call key-derivation cost. The remaining piece (verify only the matching key) needs a kid-peek API in Catapult -- raised that on the main review thread.

@mondain
Copy link
Copy Markdown
Contributor Author

mondain commented May 30, 2026

src/auth/Auth.cpp line 188 at r1

[AI] Agreed on hoisting key setup out of verify(); implemented in f724e37, slightly adapted. I cache the derived key bytes per configured key in the constructor (a small DerivedKey{id, key} vector) rather than an unordered_map<string, HmacSha256Algorithm>, because the kid-keyed map only pays off if we can look up by kid -- and Catapult currently has no way to read the token's kid without validating first (no Cwt::decodeHeader). So verify() trial-verifies over the cached keys; constructing HmacSha256Algorithm from cached bytes is cheap (no re-derivation). I deliberately kept the id on DerivedKey, so if/when Catapult grows a kid-peek API, switching to a kid-keyed map plus the matching-key short-circuit is a localized change here.

@mondain
Copy link
Copy Markdown
Contributor Author

mondain commented May 30, 2026

src/auth/Auth.cpp line 58 at r1

[AI] Done in f724e37 -- toCatapultMatch now XLOG(WARN)s when rules.size() > 1, reporting how many rules were dropped and that the serialized grant will be broader than the configured rules. (The read path fromCatapultMatch can only ever return a single rule since a Catapult MoqtBinaryMatch is one match per dimension, so this serialization step is the only place truncation happens.)

@mondain
Copy link
Copy Markdown
Contributor Author

mondain commented May 30, 2026

src/auth/Auth.cpp line 44 at r1

[AI] Went with your preferred option 2 (HKDF) in f724e37. deriveHmacKey now uses HKDF-SHA256 via EVP_PKEY_HKDF with salt "moqx-catapult-v1" and info "moqx-catapult-hmac-token-verify" for domain separation, deriving a 256-bit key. Sign (signForTest) and verify both go through it, so it's self-consistent and there's no token-compat break (pre-release). I kept your entropy caveat as a comment -- HKDF doesn't stretch a low-entropy secret, so operators still need a long random secret; I can add a minimum-secret-length check at config load as a follow-up if you'd like. (Didn't use generateSecureKey() since our keys come from operator-provided config secrets rather than being generated by us.)

@mondain
Copy link
Copy Markdown
Contributor Author

mondain commented May 30, 2026

src/auth/Auth.h line 75 at r1

[AI] Added the garbage-bytes test (RejectsGarbageBytesAsMalformedOrBadSig) in f724e37 -- it feeds \xde\xad\xbe\xef and asserts Malformed-or-BadSignature, restoring the "invalid COSE bytes don't crash" coverage.

On signRawForTest(span<const uint8_t> rawPayload): I couldn't implement it cleanly against this Catapult version. Cwt's only construction path is Cwt(alg, CatToken), and createCwt serializes that CatToken via encodePayload() -- there's no entry point to MAC arbitrary raw bytes as the payload, so deliberately-malformed-but-decodable payloads can't be produced through the public API. The existing structural negatives (BadSignature via bit-flip, empty, wrong type, expired) plus the new garbage-bytes test cover the reachable negative paths. If you want raw-payload signing for tests, it'd need a small Catapult-side helper (e.g. createCwtFromRawPayload); happy to file that.

@mondain
Copy link
Copy Markdown
Contributor Author

mondain commented May 30, 2026

Reviewable

[AI] Addressed the r1 comments in f724e37:

  • Key setup hoisted into the AuthTokenVerifier constructor (each key derived once; verify() reuses cached key material), removing the per-call derivation cost.
  • HKDF-SHA256 key derivation with domain-separation salt/info (the deriveHmacKey concern).
  • Loud XLOG(WARN) on multi-rule scope truncation in toCatapultMatch.
  • Restored a negative-path test (garbage bytes -> Malformed/BadSignature).

One flag on the key-ID-lookup must-fix: this Catapult version exposes no way to read the CWT kid without first validating against a key (no Cwt::decodeHeader / header-peek), so the "verify only the matching key" short-circuit isn't implementable yet. verify() still trial-verifies, which is cryptographically sound (the kid is advisory -- a token only validates if some configured key's HMAC matches). The actual perf regression (re-deriving keys on every call) is fixed; the remaining optimization needs a kid-peek on the Catapult side. Since you're close to catapult: would a small Cwt::decodeHeader(span) returning the unprotected header (alg/kid/typ) be reasonable to add? I've kept the key id alongside each cached key, so switching to a kid-keyed map + short-circuit would be localized here once that lands.

@mondain
Copy link
Copy Markdown
Contributor Author

mondain commented May 30, 2026

@copilot resolve the merge conflicts in this pull request

Copy link
Copy Markdown

Copilot AI commented May 30, 2026

@copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in d490534.

  • CMakeLists.txt: kept both the catapult add_subdirectory block and the moqx_cache library target from feature/auth.
  • src/auth/Auth.cpp: kept all catapult implementations (HKDF, fromCatapultMatch, grantsFromToken, etc.); discarded the old CBOR/HMAC helpers (hmacSha256, readU32BE, appendU32BE, parseAction, parseScope, parseClaims) that no longer apply.
  • src/auth/Auth.h: kept catapult-branch comment for signForTest.
  • test/AuthTest.cpp: removed RejectsDuplicateWellKnownClaimKeys (auto-merged in from feature/auth but references CBOR helper functions not present in this branch and calls makeToken with a std::string incompatible with the catapult Grants-based signature; duplicate-claim detection is now Catapult's responsibility).

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