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.
|
|
Catapult latest main has following api Code snippet: // include/catapult/cwt.hpp
static CwtHeader decodeHeader(std::span<const uint8_t> cwtBytes);
// It returns:
struct CwtHeader {
int64_t alg;
std::optional<std::string> kid;
std::optional<std::string> typ;
};
//and update the existing code to
auto header = catapult::Cwt::decodeHeader(span);
if (header.kid.has_value()) {
// find matching key by kid, verify only with that key
} else {
// no kid — fall back to trial verification
}
//This eliminates the trial-verify loop for tokens that carry a kid, which is both a performance and security
//improvement (this is similar to earlier psuedocode, but you get the idea). |
|
Previously, mondain (Paul Gregoire) wrote…
Thanks @mondain for being patient on the review here. Since this is a good functionality to be added in catapult , i went ahead and add it over the weekend ( and merged this morning ) Given that, the above code will have some changes along the lines below. Please give it a try. Code snippet: catapult::MoqtCompoundMatch toCatapultMatch(const std::vector<MatchRule>& rules) {
if (rules.empty()) return catapult::MoqtCompoundMatch::any();
std::vector<catapult::MoqtBinaryMatch> conditions;
conditions.reserve(rules.size());
for (const auto& rule : rules) {
switch (rule.type) {
case MatchRule::Type::Exact: conditions.push_back(catapult::MoqtBinaryMatch::exact(rule.value));
break;
case MatchRule::Type::Prefix: conditions.push_back(catapult::MoqtBinaryMatch::prefix(rule.value));
break;
///// so on
}
}
return catapult::MoqtCompoundMatch::all(std::move(conditions));
}
// then
std::vector<MatchRule> fromCatapultMatch(const catapult::MoqtCompoundMatch& match) {
if (match.is_empty()) return {};
std::vector<MatchRule> rules;
rules.reserve(match.conditions().size());
for (const auto& cond : match.conditions()) {
rules.push_back(MatchRule{
.type = fromCatapultMatchType(cond.match_type),
.value = toString(cond.pattern),
});
}
return rules;
}
|
suhasHere
left a comment
There was a problem hiding this comment.
@mondain thanks for addressing the review feedback. the latest catapult ( from this morning) should have decodeHeader and CompoundMatch support. if you can integrate these 2 into your PR, I feel we are very close calling first version of cat support done.
I am sure we can improve library API later as needed, but these give us sufficient functionality to test it
@suhasHere made 2 comments.
Reviewable status: 0 of 7 files reviewed, 5 unresolved discussions (waiting on afrind, akash-a-n, gmarzot, michalhosna, mondain, Oxyd, peterchave, and TimEvens).
src/auth/Auth.cpp line 106 at r3 (raw file):
// MatchRule vector can express several ANDed rules (e.g. Prefix + Suffix). We // can only serialize the first; warn loudly rather than silently dropping the // rest, since that would widen the grant beyond what was configured.
this is true since the idea was to use OR across scopes. But if we feel the functionality is important, i can add it
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