auth: integrate Catapult CAT token verification#286
Conversation
|
@mondain I will review the PR this week. |
|
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); auto tryVerify = [&](const HmacKeyConfig& key) // If kid present, verify only with the matching key // No kid — trial verification across all keys |
|
The old allRulesMatch iterated all rules with std::all_of. If any code path |
|
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); |
|
The old API accepted raw CBOR bytes, enabling tests to construct deliberately 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));
} |
|
I have some concerns here
We can consider 2 options
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;
} |
suhasHere
left a comment
There was a problem hiding this comment.
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.
|
[AI] Restructured verify() along these lines in f724e37, with one constraint: this Catapult version has no |
|
[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 |
|
[AI] Done in f724e37 -- |
|
[AI] Went with your preferred option 2 (HKDF) in f724e37. |
|
[AI] Added the garbage-bytes test ( On |
|
[AI] Addressed the r1 comments in f724e37:
One flag on the key-ID-lookup must-fix: this Catapult version exposes no way to read the CWT |
|
@copilot resolve the merge conflicts in this pull request |
Merge conflicts resolved in d490534.
|
Summary
This is stacked on
feature/auth/ PR #264 and isolates the Catapult integration requested in review.Quicr/catapultas a submodule and wires it into the CMake and Docker builds.AuthTokenVerifier,Grants, andallows) so relay call sites do not churn in this PR.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:
Result: 29/29 tests passed.
This change is