feat: Add opt-in CAT token authorization

[mondain]

Summary

Adds opt-in CAT-style token authentication and authorization for MOQT relay services.

Changes

• Adds a per-service auth config block for enabling authorization without changing default behavior.
• Adds an internal signed CWT/HMAC token verifier for exp, moqt, and moqt-reval claim handling.
• Validates setup AUTHORIZATION_TOKEN credentials during service routing.
• Authorizes PUBLISH_NAMESPACE, PUBLISH, SUBSCRIBE_NAMESPACE, SUBSCRIBE, FETCH, and TRACK_STATUS requests.
• Preserves existing relay peering token behavior.
• Adds focused auth verifier tests and example config comments.

Notes

This PR uses the existing feature/auth branch contents as requested, including branch-local commits outside the auth implementation commit.

Validation

• ./scripts/format.sh --check passed via the commit hook.
• Full build/test was not completed locally because CMake could not discover required Boost components (context, filesystem, program_options, thread) from the local moxygen install in this environment.

---

This change is 

[mondain]

Docker build validation completed after refreshing the CI-style bookworm moxygen bundle:

• MOQX_PLATFORM=bookworm-amd64 MOQX_SCRATCH_PATH=/tmp/moqx-docker-scratch bash scripts/build.sh setup --use-latest --no-fallback
• staged /tmp/moqx-docker-scratch/moxygen-install as .docker-deps/moxygen
• ./scripts/docker-build.sh moqx-auth-verify

Result: Docker image built successfully as moqx-auth-verify (sha256:df0ce48dce2e8bacc471bda319a1a0a8cb82314c310d8e617e97b7ab8f5b2f3e).

[afrind]

Adding some unwritten documentation so I can understand the flow -- other reviewers may find helpful:

Startup: YAML → Runtime Objects

config.yaml
    │
    ▼ rfl YAML parser
ParsedConfig  (ParsedConfig.h)
  └─ services["svc"].auth: ParsedAuthConfig
       all fields optional / may be absent
    │
    ▼ ConfigResolver::resolve()
       ├─ validateAuth()   ← layer 2 tests go here
       │    checks: enabled→keys present, no dup IDs,
       │            non-empty secrets, token_type fits varint
       └─ resolveAuth()    fills defaults:
            requireSetupToken=true
            allowRequestTokenOverride=true
    │
    ▼
ResolvedConfig
  └─ services["svc"]: ServiceConfig
       └─ auth: AuthConfig   ← typed, no optionals
    │
    ▼ MoqxRelayContext constructor
  MoqxRelayContext
    └─ services_["svc"]
         └─ relay: MoqxRelay(cache, relayID, auth)
                └─ authVerifier_: AuthTokenVerifier(auth)
    │
    ▼ passed (shared_ptr) to both server types:
  MoqxRelayServer  (mvfst/fizz)
  MoqxPicoRelayServer  (picoquic)

---

Connection Time: CLIENT_SETUP → Session Auth

client connects
    │
    ▼ server calls:
MoqxRelayServer::validateAuthority(clientSetup, version, session)
  1. base class check (version negotiation etc.)
  2. context_->validateAuthority(...)
    │
    ▼
MoqxRelayContext::validateAuthority
  1. serviceMatcher_.match(authority, path)  → service name
     └─ miss → INVALID_AUTHORITY (closed)
  2. relay->authenticateSession(clientSetup, session)
    │
    ▼
MoqxRelay::authenticateSession
  ┌─ auth disabled?         → folly::unit (pass)
  ├─ findAuthToken(params)  → scan AUTHORIZATION_TOKEN params by tokenType
  │    └─ no token + requireSetupToken=true  → Missing → UNAUTHORIZED
  ├─ authVerifier_.verify(token)
  │    ├─ wrong tokenType              → WrongTokenType → UNAUTHORIZED
  │    ├─ envelope malformed           → Malformed → MALFORMED_AUTH_TOKEN
  │    ├─ key ID not in config         → BadSignature → UNAUTHORIZED
  │    ├─ HMAC mismatch                → BadSignature → UNAUTHORIZED
  │    ├─ CBOR parse error             → Malformed → MALFORMED_AUTH_TOKEN
  │    └─ expiresAt <= now             → Expired → EXPIRED_AUTH_TOKEN
  └─ allows(grants, ClientSetup, {})
       └─ action not in any scope      → Forbidden → UNAUTHORIZED
  on success:
    sessionAuth_[session.get()] = grants
    │
    ▼ back in MoqxRelayContext
  session->setPublishHandler(relay)
  session->setSubscribeHandler(relay)

---

Per-Request: Subscribe / Publish / Fetch / etc.

client sends Subscribe (or Publish, Fetch, TrackStatus, SubscribeNamespace, PublishNamespace)
    │
    ▼
MoqxRelay::subscribe / publish / fetch / ...
  authorize(action, request.params, ns, trackName)
    │
    ▼
MoqxRelay::authorize
  ┌─ auth disabled?  → pass
  ├─ allowRequestTokenOverride=true AND token in request params?
  │    ├─ verify(token) → error?   → return that error (no session fallback)
  │    ├─ allows(grants, action, ns, track)?  → pass
  │    └─ else → Forbidden
  └─ no request token → look up sessionAuth_[session.get()]
       ├─ not found       → Missing
       ├─ allows()?       → pass
       └─ else            → Forbidden
  on error:
    subscribe  → SubscribeError UNAUTHORIZED
    publish    → PublishError UNAUTHORIZED
    fetch      → FetchError UNAUTHORIZED
    etc.

---

Session Teardown

session ends
    │
    ▼
MoqxRelayServer::onSessionEnd  (or Pico variant)
  context_->onSessionEnd(session)
    │
    ▼
MoqxRelayContext::onSessionEnd
  for each service:
    relay->removeSessionAuth(session.get())
      └─ erases from sessionAuth_ map

[afrind]

Auth Module Review: src/auth/Auth.h / Auth.cpp

What it does

Three responsibilities: (1) envelope decode + HMAC-SHA256 verification (verify()), (2) CBOR
claims parsing into Grants, (3) authorization check (allows()). These are cleanly separated
and the module has no I/O or threading — good for testing. The token format is an internal v1
envelope (not CWT/PASETO), which is fine for an in-house system.

---

Critical

Prefix, Suffix, and Contains match rules are completely untested
claimsFor() only constructs Exact-match rules. The three other MatchRule::Type values exist,
are handled in bytesMatch(), and are the whole point of the flexible matching system — but none
are exercised. A regression in any of them would not be caught.

findAuthToken() has zero test coverage
This is the entry point that extracts a token from MoQ parameters before verify() is called.
Found/not-found/wrong-type paths are all untested. A bug here would silently bypass auth entirely.

verify() error paths are almost entirely untested
Of the six AuthError values, only BadSignature and Expired are tested. WrongTokenType,
Malformed (truncated envelope, wrong version byte, claims-len overflow), and Forbidden (no
ClientSetup action in scopes) are all reachable and untested.

allows() expiration re-check is untested
allows() takes a now parameter precisely to support testing time-based expiry after grants are
issued. There is no test where verify() succeeds at time T and allows() is called at T+1 where
T+1 > expiresAt. The now parameter exists but its effect is unverified.

---

Important

revalidateEvery is parsed and stored but never used anywhere
grants.revalidateEvery is populated from the moqt-reval CBOR claim, but allows() ignores it
and there is no timer or re-verification mechanism in the relay. A token issuer setting
moqt-reval: 60 gets no enforcement. Either implement re-verification or remove revalidateEvery
from Grants and stop parsing moqt-reval until the relay-side mechanism exists.

signForTest silently truncates key IDs longer than 255 bytes
out.push_back(static_cast<char>(keyID.size())) truncates to one byte with no check or assert.
Add DCHECK_LE(keyID.size(), 255u).

HMAC() is deprecated in OpenSSL 3.0
Deprecated in 3.0, removed in some downstream strict-deprecation builds. Replace with EVP_MAC /
EVP_MAC_CTX which is stable across OpenSSL 1.1 and 3.x.

Empty namespace/track rules vacuously match everything — untested
allRulesMatch() uses std::all_of which returns true on an empty range. Correct behavior for
open scopes, but never tested. A refactor breaking the empty-range case would not be caught.

Multiple configured keys and key selection untested
AuthConfig supports a vector of hmacKeys. The linear scan by key ID in verify() is only
exercised with a single key. Sign with key k2 when k1 and k2 are both configured; assert correct
key is selected. Also test unknown key ID → BadSignature.

allows() with empty scopes is untested (deny-all case)
A token with no moqt claim produces Grants with empty scopes. The deny-all behavior is
correct but never tested. Grants g{}; EXPECT_FALSE(allows(g, Action::Subscribe, ns, "video"));

---

Simplification

parseActions copy-based backtracking is non-obvious
The speculative copy (auto copy = reader; if (copy.readArrayLen(len)) { reader = copy; }) works
correctly but merits a one-line comment: CborReader's copy-as-backtrack is not an obvious idiom.

[afrind]

Auth Config Review

Pattern Consistency

The structure follows existing conventions well. ParsedAuthConfig uses rfl::Description<"...", T>
on every field, required fields are non-optional (enabled), optional fields use std::optional<T>
— same as ParsedCacheConfig and ParsedQuicConfig. AuthConfig is directly embedded in
ServiceConfig (not std::optional) with enabled{false} as the disabled sentinel, which is
exactly the same pattern as CacheConfig. The resolveAuth(nullopt) → default-constructed
disabled config path is correct.

One inconsistency: every optional field in ParsedQuicConfig documents its default in the
description string — "Idle timeout in milliseconds (default: 30000)", etc.
ParsedAuthConfig's three optional booleans (require_setup_token, allow_request_token_override,
strict_claims) omit their defaults entirely. For a security-sensitive config the defaults matter:
a reader of the YAML schema has no way to know require_setup_token defaults to true without
reading the C++ resolver.

afrind: should we also support hmac key files? This can be done as a follow up unless its trivial here since there's already a lot going on.

---

Critical

Zero test coverage for all auth config validation and resolution
File: test/config/ConfigResolverTest.cpp
Why: ConfigResolverTest.cpp has thorough coverage of every other config section — listeners,
cache (8 tests including inheritance and partial overrides), admin, QUIC, authority/path matching —
but zero tests for auth. Every branch of validateAuth and every field of resolveAuth is
untested. A regression in any auth validation would silently pass.
Fix: Add a makeAuthService() helper mirroring makeDefaultService(), then cover:

• enabled=true, no hmac_keys → error
• enabled=true, empty hmac_keys: [] → error
• Duplicate key IDs → error
• Empty key ID → error; empty secret → error
• token_type >= 2^62 → error
• enabled=false → resolves cleanly, auth.enabled == false in output
• Valid config → all fields round-trip: tokenType, requireSetupToken,
  allowRequestTokenOverride, strictClaims, hmacKeys
• Absent optional fields each default correctly: require_setup_token → true,
  allow_request_token_override → true, strict_claims → false, token_type → 0
• Absent auth section → ServiceConfig::auth.enabled == false

---

Important

token_type validation skips when hmac_keys is missing
File: src/config/ConfigResolver.cpp:326
Why: validateAuth does an early return after the missing-keys error, which means
token_type >= 2^62 is never checked in that case. A config with two problems only reports one.
Fix: Remove the early return; accumulate both errors and let the caller handle them.

strict_claims omitted from config.example.yaml auth section
File: config.example.yaml
Why: It's the only auth field not shown in the commented example. A user copying the example as a
starting point has no visibility into the existence or default of strict_claims.
Fix: Add # strict_claims: false with a brief comment to the auth section.

Description strings for auth booleans don't document their defaults
File: src/config/loader/ParsedConfig.h:170-177
Why: All other optional fields in the file (QUIC, cache) state their defaults inline. A reader of
the generated schema or dump-config-schema output can't tell whether omitting
require_setup_token means auth is not required or required by default. For a security field
this matters.
Fix: Update the three description strings to include their defaults, e.g.:
"Require a valid setup token during CLIENT_SETUP (default: true)".

[afrind]

Auth Wiring Review: Relay + Context + Server

What changed

Server layer (MoqxRelayServer, MoqxPicoRelayServer) — one line each:
onSessionEnd() → onSessionEnd(session), passing the session through for auth cleanup.

Context layer (MoqxRelayContext):

• Constructor passes svc.auth when constructing each MoqxRelay
• onSessionEnd gains session parameter, adds loop calling relay->removeSessionAuth(session.get()) across all services
• validateAuthority was ignoring clientSetup; now calls relay->authenticateSession(), maps AuthError → SessionCloseErrorCode, gates handler assignment on success

Relay layer (MoqxRelay):

• authenticateSession — verifies setup token, stores Grants in sessionAuth_ keyed by raw MoQSession*
• authorize (private) — checks per-request token override first, then session grants
• removeSessionAuth — erases session from map on teardown
• Auth check added to all 6 entry points: publishNamespace, publish, subscribeNamespace, subscribe, fetch, trackStatus

---

Correctness

subscribeNamespace peer relay bypass is correct. The incomingPeerID.empty() guard skips
service auth for peer relays, but getPeerRelayID requires a token of type kRelayAuthTokenType
— a fixed constant distinct from the per-service authVerifier_.tokenType(). A client cannot
fake a peer relay identity to bypass service auth; it would need the relay-specific token type,
which is a separate auth mechanism.

authorize with no session fallback on request token denial is correct. If a request token is
present but denied, it returns Forbidden immediately without consulting session auth. Intentional
design — the presence of a request token means "use this", not "try this first". Needs a test to
prevent accidental revert.

validateAuthority switch has no post-switch fallback.
File: src/MoqxRelayContext.cpp
If a new AuthError value is added without updating the switch, execution falls through the
if (authRes.hasError()) block and sets publish/subscribe handlers on the session despite an auth
failure. Fix: add default: folly::assume_unreachable() or an unconditional
return folly::makeUnexpected(SessionCloseErrorCode::UNAUTHORIZED) after the switch.

sessionAuth_ keyed by raw pointer is safe given current teardown path, but only because
terminateClientSession is the sole session teardown and always calls through to onSessionEnd
→ removeSessionAuth. If that invariant breaks — a future alternate teardown path, an exception
in server setup — a dead pointer remains in the map. See moxygen follow-up below.

---

Simplicity / Moxygen Follow-up

The sessionAuth_ map in MoqxRelay, the removeSessionAuth method, and the O(services) loop
in onSessionEnd are all artifacts of the same missing primitive: per-session storage on
MoQSession.

Recommendation: Add a setUserData<T> / getUserData<T> API to MoQSession in moxygen.
Implementation is a std::unordered_map<const TypeTag*, std::any> keyed by address of a
per-type static (RTTI-free). This would:

• Eliminate sessionAuth_ from MoqxRelay
• Eliminate removeSessionAuth
• Eliminate the O(services) loop in onSessionEnd — cleanup is automatic when the session
  object is destroyed
• Remove the need for MoqxRelayContext to know about auth during teardown at all

The 6 authorize call sites are repetitive but each uses a different error type — the duplication
is necessary.

---

Test Coverage

MoqxRelayTest.cpp has no auth tests. MoqxRelayContextTest.cpp has no auth tests.

authenticateSession

• Auth disabled → always passes, nothing stored
• No token + requireSetupToken=true → Missing
• No token + requireSetupToken=false → passes, nothing stored in sessionAuth_
• Valid token with ClientSetup action → passes, grants stored
• Expired token → Expired
• Bad signature → BadSignature
• Token without ClientSetup action → Forbidden

authorize

• Auth disabled → always passes
• No session auth, no request token → Missing
• Session auth covers action → passes
• Session auth does not cover action → Forbidden
• Request token present, valid, covers action → passes (session auth not consulted)
• Request token present but denied → Forbidden, does NOT fall back to session auth
• allowRequestTokenOverride=false → request token in params ignored, session auth used

removeSessionAuth

• After removal, authorize for that session returns Missing

Context auth (MoqxRelayContextTest)

• validateAuthority with valid token → EXPIRED_AUTH_TOKEN, MALFORMED_AUTH_TOKEN,
  UNAUTHORIZED each tested with the appropriate token/error
• Successful auth → setPublishHandler / setSubscribeHandler called on session
  (verifiable via MockMoQSession EXPECT_CALL)

[suhasHere]

Thanks for the work on this @mondain . here are few high level thoughts. Can we split this PR into 3 parts

• Auth Config
• Abstract Auth API
• CAT Auth bits.

On the last one, was wondering if you got chance to checkout this repo : https://github.com/Quicr/catapult
It implements cat4moq and dpop with all the crypto bits . I would encourage us to use this library instead of reinventing the wheel , if possible.

@suhasHere made 1 comment.
Reviewable status: 0 of 17 files reviewed, all discussions resolved (waiting on afrind, akash-a-n, gmarzot, michalhosna, Oxyd, peterchave, and TimEvens).

[mondain]

@suhasHere I'll do a deeper dive into catapult, I'm all for not reinventing the wheel and on the surface it looks good to me.

@mondain made 1 comment.
Reviewable status: 0 of 17 files reviewed, all discussions resolved (waiting on afrind, akash-a-n, gmarzot, michalhosna, Oxyd, peterchave, and TimEvens).

[mondain]

Docker focused validation passed:

• Built moqx_auth_test and moqx_config_resolver_test in debian:bookworm using .docker-deps/moxygen.
• Ran ctest -R "Auth|ConfigResolver" --output-on-failure.
• Result: all focused auth/config tests passed.

The Docker test configure needed the same gflags workaround used by the local build script:

  -DCMAKE_FIND_LIBRARY_SUFFIXES=".so;.a"
  -DGFLAGS_SHARED=ON

[mondain]

Thanks for the work on this @mondain . here are few high level thoughts. Can we split this PR into 3 parts

• Auth Config
• Abstract Auth API
• CAT Auth bits.

On the last one, was wondering if you got chance to checkout this repo : https://github.com/Quicr/catapult It implements cat4moq and dpop with all the crypto bits . I would encourage us to use this library instead of reinventing the wheel , if possible.

@suhasHere made 1 comment.
Reviewable status: 0 of 17 files reviewed, all discussions resolved (waiting on afrind, akash-a-n, gmarzot, michalhosna, Oxyd, peterchave, and TimEvens).

I'm creating a separate PR off this work to provide the route for Catapult; separating the original PR into n new PR does not excite me for several reasons, but I'll go with the group, if others think its prudent.

[afrind]

Ok, many small comments I'm sure claude can handle. I don't think this really needs a further split. It is a big review but I made it through. @suhasHere are you ok with catapult as a follow up?

@afrind reviewed 18 files and all commit messages, and made 18 comments.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on akash-a-n, gmarzot, michalhosna, mondain, Oxyd, peterchave, and TimEvens).

---

deps/moxygen line 1 at r3 (raw file):

Subproject commit 36c314edb9b42345fd4ad918b463743edad21a87

What moxygen change is required to support this?

---

CMakeLists.txt line 38 at r3 (raw file):

find_package(moxygen REQUIRED)
find_package(OpenSSL REQUIRED)

Everyone cool with OpenSSL dep here? @suhasHere

---

config.example.yaml line 60 at r3 (raw file):

    # auth:                       # Optional: enable CAT-style authorization for this service
    #   enabled: true
    #   token_type: 0             # MOQT AUTHORIZATION_TOKEN token type

token_type 0 is reserved to mean "nothing" in the spec? So I think we cannot specify 0 (config loader should error)

---

config.example.yaml line 64 at r3 (raw file):

    #     - id: "key-1"
    #       secret: "replace-with-secret"
    #   require_setup_token: true

Do each of these need descriptive comments? I'm not sure what the convention is

---

src/MoqxRelay.cpp line 133 at r3 (raw file):

  }

  auto session = MoQSession::getRequestSession();

Can we pass the session in? Seems like it's already known/needed elsewhere

---

src/MoqxRelay.cpp line 134 at r3 (raw file):

  auto session = MoQSession::getRequestSession();
  auto token = authVerifier_.allowRequestTokenOverride()

If override is not on and the request does contain a new token, should we log/error? Or it's expected behavior?

---

src/MoqxRelay.cpp line 142 at r3 (raw file):

      return folly::makeUnexpected(grants.error());
    }
    if (auth::allows(grants.value(), action, ns, trackName)) {

Seems like we could have a single auth::allows call by consolidating this bit with below. eg:

if (token) {
grants = verify();
} else {
grants = sessionAuth_.find();
}
if (!auth::allows(...)) {
}

---

src/MoqxRelay.cpp line 257 at r3 (raw file):

  auto authRes = authorize(auth::Action::PublishNamespace, pubNs.params, pubNs.trackNamespace);
  if (authRes.hasError()) {
    co_return folly::makeUnexpected(PublishNamespaceError{

I think we should log in all these branches - maybe DBG1?

---

src/auth/Auth.h line 72 at r3 (raw file):

  folly::Expected<Grants, AuthError> verify(const moxygen::AuthToken& token) const;

  // Internal v1 envelope used by tests and by local token issuers:

Hm, maybe this could be in a subclass? Also "local token issuers"?

---

src/auth/Auth.h line 87 at r3 (raw file):

    const Grants& grants,
    Action action,
    const moxygen::TrackNamespace& ns,

Maybe split into two signatures, TrackNamespace and FullTrackName, that both call allowsImpl with this signature?

---

src/auth/Auth.cpp line 93 at r3 (raw file):

uint32_t readU32BE(std::string_view data, size_t offset) {
  return (static_cast<uint32_t>(static_cast<unsigned char>(data[offset])) << 24) |

is this just an implement of ntohl or folly::endian::big/little?

---

src/auth/Auth.cpp line 99 at r3 (raw file):

}

void appendU32BE(std::string& out, uint32_t value) {

Same you can do this with a helper to flop your integer and then memcpy, right?

---

src/auth/Auth.cpp line 137 at r3 (raw file):

}

class CborReader {

Minor preference to move CborReader to its own file in case we ever need it for something else.

---

src/auth/Auth.cpp line 179 at r3 (raw file):

    uint8_t major = 0;
    uint64_t len = 0;
    if (!readType(major, len) || (major != 2 && major != 3) || len > data_.size() - pos_) {

Can we add symbolic constants throughout for these magic numbers?

---

src/auth/Auth.cpp line 315 at r3 (raw file):

}

bool parseScope(CborReader& reader, Scope& scope) {

A comment describing the format would make it easier to validate the code is correct for the non-cat experts

---

src/auth/Auth.cpp line 473 at r3 (raw file):

  const auto nsBytes = canonicalNamespace(ns);
  const auto trackBytes = trackName.value_or(std::string_view());
  for (const auto& scope : grants.scopes) {

Can this be a potential performance concern? Could someone supply a token with a large number of scopes? I suppose it's bound by the MOQT 64k control message len limit, but would a hash lookup be better?

---

test/config/ConfigResolverTest.cpp line 467 at r3 (raw file):

  cfg.services.value().clear();
  auto auth = makeAuthConfig();
  auth.token_type = std::optional<uint64_t>{uint64_t{1} << 62};

How will we remember to adjust this when we go to MOQT varints?

[mondain]

@mondain made 1 comment and resolved 1 discussion.
Reviewable status: 5 of 20 files reviewed, 12 unresolved discussions (waiting on afrind, akash-a-n, gmarzot, michalhosna, Oxyd, peterchave, and TimEvens).

[mondain]

src/auth/Auth.h line 87 at r3

[AI] Done in f34554c. There are now two public overloads -- allows(grants, action, TrackNamespace) for namespace-level checks and allows(grants, action, FullTrackName) for track-level checks -- both delegating to a single allowsImpl(..., std::optional<std::string_view> trackName, ...) in the .cpp. The relay and tests were updated to call the appropriate overload.

[mondain]

src/MoqxRelay.cpp line 133 at r3

[AI] Done in f34554c. authorize() now takes the MoQSession by const ref and the callers pass the session they already hold (publish() and trackStatus() now fetch it once up front like the others). This drops the second MoQSession::getRequestSession() lookup that used to happen inside authorize().

[mondain]

src/MoqxRelay.cpp line 134 at r3

[AI] Expected behavior, now logged. When allow_request_token_override is false the grants established at session setup govern, and a per-request token is intentionally ignored rather than rejected (a client re-sending its token shouldn't fail the request). In f34554c we now log that at DBG1 for visibility instead of silently dropping it.

[mondain]

src/MoqxRelay.cpp line 142 at r3

[AI] Done in f34554c. authorize() now resolves grants from exactly one source -- the request token if override is enabled, otherwise the session grants -- into a single pointer, then runs one auth::allows() check, matching the shape you sketched.

[mondain]

src/MoqxRelay.cpp line 257 at r3

[AI] Done in f34554c, centralized rather than per-branch: authorize() now logs at DBG1 on every failure path (no session grants, token verification failure, and action-not-permitted), so all the UNAUTHORIZED call sites are covered in one place instead of adding a log to each of the ~6 callers.

[mondain]

src/auth/Auth.cpp line 93 at r3

[AI] Effectively yes -- replaced with folly in f34554c. readU32BE now memcpy's 4 bytes and calls folly::Endian::big(). Note these are the fixed 32-bit big-endian claims-length fields in our HMAC token envelope, not the QUIC/MOQT varints, so a plain 32-bit BE load is what we want here.

[mondain]

src/auth/Auth.cpp line 99 at r3

[AI] Yes -- done in f34554c. appendU32BE now does folly::Endian::big(value) then appends the 4 bytes, mirroring the reader.

[mondain]

src/auth/Auth.cpp line 137 at r3

[AI] Done. CborReader was moved to its own header src/auth/CborReader.h in d2b9e59, and f34554c restores the dedicated CborReaderTest.cpp that exercises it in isolation.

[mondain]

src/auth/Auth.cpp line 179 at r3

[AI] Done in f34554c. Added a cbor:: constants block in CborReader.h (kMajorUnsigned/Negative/ByteString/TextString/Array/Map, plus kAddlInfo1/2/4/8Byte, kAddlInfoDirectMax, kMajorShift, kAddlInfoMask) and replaced the magic numbers throughout the reader.

[mondain]

src/auth/Auth.cpp line 315 at r3

[AI] Done in f34554c -- added a block comment above parseScope describing the CAT "moqt" claim layout: the [actions, namespace-matches, track-matches] 3-element array, the action scalar-or-array form, and the {match-type => bytes} maps with AND semantics (match-type = MatchRule::Type 0=exact/1=prefix/2=suffix/3=contains).

[mondain]

config.example.yaml line 64 at r3

[AI] Added inline descriptive comments for each auth key in config.example.yaml in f34554c (enabled / token_type / hmac_keys / id / secret / require_setup_token / allow_request_token_override), matching the inline-comment convention used elsewhere in the file.

[mondain]

config.example.yaml line 60 at r3

[AI] Checked the spec on this. Per draft-ietf-moq-transport (AUTHORIZATION_TOKEN "Token Type" definition), Type 0 is "reserved to indicate that the type is not defined in the [IANA] table and is negotiated out-of-band between client and receiver." So 0 is a valid, usable value meaning a private/out-of-band token type -- not "nothing." Since our CAT-style token is exactly such an out-of-band scheme, 0 is a legitimate choice, so the loader accepts it as-is (an omitted token_type also resolves to 0, so the two behave identically). validateAuth still rejects values >= 2^62 (un-encodable as a varint). I clarified this in the example-config comment and in validateAuth in f34554c. If you'd rather the example not imply 0 is special, happy to switch it to a non-zero placeholder.

[mondain]

src/auth/Auth.h line 72 at r3

[AI] "local token issuers" was misleading -- the relay only ever verifies tokens, it never issues them. signForTest is purely a test/tooling helper for building a v1 envelope. I reworded the comment in f34554c to say exactly that. On the subclass question: I'd lean against it for now -- there's a single envelope version, and verify() / signForTest share that one layout, so a subclass would add a type without a second implementation to justify it. If we grow a real (non-test) token issuer later, extracting an interface at that point would be the natural time.

[mondain]

src/auth/Auth.cpp line 473 at r3

[AI] I think linear scan is fine here. Scope count is bounded by the 64k control-message limit you noted (and realistically a token carries a handful of scopes). A hash lookup doesn't really fit the model, though: scope entries aren't exact keys -- they're prefix/suffix/contains/exact match rules evaluated against the canonicalized namespace and track bytes, so there's no single key to hash on; we'd still scan candidate scopes. If this ever shows up hot, the better lever would be indexing scopes by Action first (skip scopes that don't grant the requested action before running the match rules). Left as-is for now.

[mondain]

test/config/ConfigResolverTest.cpp line 467 at r3

[AI] The 2^62 bound isn't a moqx policy that will drift -- it's the maximum value encodable by a QUIC variable-length integer (RFC 9000 Section 16), which MOQT inherits and which is fixed by the wire format. So there's nothing to remember to bump as the drafts evolve. To make that explicit, f34554c names the constant kQuicVarintExclusiveUpperBound (with a comment) in validateAuth; the test deliberately uses 1 << 62 as the first un-encodable value.

[mondain]

CMakeLists.txt line 38 at r3

[AI] For context: OpenSSL is used here for HMAC-SHA256 token verification (EVP_MAC on OpenSSL 3.x, HMAC otherwise). It's already pulled in transitively by the moxygen -> proxygen -> fizz/folly stack, so this find_package(OpenSSL) links what's already present rather than adding a new third-party dependency. HMAC needs some crypto provider, and OpenSSL is the standard one in this ecosystem. @suhasHere if you'd prefer a different provider we can revisit, but it'd likely be the same underlying lib. No code change here pending your call.

[mondain]

deps/moxygen line 1 at r3

[AI] To add to the earlier note: this feature doesn't require any moxygen-side change -- it builds entirely on moxygen's existing AUTHORIZATION_TOKEN support (moxygen::AuthToken, Parameters, TrackRequestParamKey::AUTHORIZATION_TOKEN). The submodule bump in this PR is just a routine sync to openmoq/moxygen main, not a change coupled to the auth work. If the concern is keeping the review clean, I can split the submodule bump into its own commit/PR so it isn't entangled with the auth changes.

[mondain]

Reviewable

[AI] @afrind the r3 inline comments are addressed in f34554c (on top of d2b9e59). Summary:

• allows() split into TrackNamespace + FullTrackName overloads over a shared allowsImpl
• authorize() consolidated to a single allows() check, takes the session as a parameter instead of getRequestSession(), and logs at DBG1 on auth failures and when ignoring a per-request token while override is disabled
• folly::Endian for the u32be claims-length helpers
• named CBOR major-type / additional-info constants in CborReader; CborReader already split into its own header (d2b9e59), with its unit tests restored
• documented the CAT "moqt" scope CBOR layout; clarified the signForTest comment (test/tooling helper -- the relay never issues tokens)
• reject duplicate well-known claim keys (exp/moqt/moqt-reval)
• named the QUIC-varint bound; clarified that token_type 0 is a valid out-of-band type per draft-ietf-moq-transport; descriptive comments on the auth config keys

Replied inline on the discussion-only threads (OpenSSL dependency, token_type 0, the subclass question, scope-scan performance, the varint bound, and the moxygen submodule). The catapult-vs-in-tree and PR-split questions I left for @mondain / @suhasHere.

[TimEvens]

Good starting point, but some comments/nits.

@TimEvens reviewed 21 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on afrind, akash-a-n, gmarzot, michalhosna, mondain, Oxyd, and peterchave).

---

CMakeLists.txt line 38 at r3 (raw file):

Previously, mondain (Paul Gregoire) wrote…

[AI] For context: OpenSSL is used here for HMAC-SHA256 token verification (EVP_MAC on OpenSSL 3.x, HMAC otherwise). It's already pulled in transitively by the moxygen -> proxygen -> fizz/folly stack, so this find_package(OpenSSL) links what's already present rather than adding a new third-party dependency. HMAC needs some crypto provider, and OpenSSL is the standard one in this ecosystem. @suhasHere if you'd prefer a different provider we can revisit, but it'd likely be the same underlying lib. No code change here pending your call.

Should be fine for now, but we'll likely override the find to support boring, and others, which expose OpenSSL apis.

---

src/MoqxRelay.h line 313 at r6 (raw file):

      const moxygen::TrackNamespace& ns,
      const std::shared_ptr<moxygen::MoQSession>& session,
      std::optional<std::string_view> trackName = std::nullopt

Name isn't always a string. Any reason not to use span const ?

---

src/MoqxRelay.cpp line 659 at r6 (raw file):

    // receives namespace announcements as publishers connect.
  }
  if (incomingPeerID.empty()) {

Not super great that the detection if the subNs is via a peer or not is determined by getPeerRelayID().

---

src/auth/Auth.h line 23 at r6 (raw file):

namespace openmoq::moqx::auth {

enum class Action : uint64_t {

Is it really an action? Seems more like Type or Scope...

[afrind]

@afrind reviewed 5 files, made 7 comments, and resolved 12 discussions.
Reviewable status: 13 of 21 files reviewed, 4 unresolved discussions (waiting on akash-a-n, gmarzot, michalhosna, mondain, Oxyd, peterchave, and TimEvens).

---

config.example.yaml line 60 at r3 (raw file):

Previously, mondain (Paul Gregoire) wrote…

[AI] Checked the spec on this. Per draft-ietf-moq-transport (AUTHORIZATION_TOKEN "Token Type" definition), Type 0 is "reserved to indicate that the type is not defined in the [IANA] table and is negotiated out-of-band between client and receiver." So 0 is a valid, usable value meaning a private/out-of-band token type -- not "nothing." Since our CAT-style token is exactly such an out-of-band scheme, 0 is a legitimate choice, so the loader accepts it as-is (an omitted token_type also resolves to 0, so the two behave identically). validateAuth still rejects values >= 2^62 (un-encodable as a varint). I clarified this in the example-config comment and in validateAuth in f34554c. If you'd rather the example not imply 0 is special, happy to switch it to a non-zero placeholder.

I think 0 is wrong. Cat4moq should have an explicit type. moqx should only allow the types in its config that it is configured to validate.

That said, we can land this and follow up.

---

src/MoqxRelay.cpp line 134 at r3 (raw file):

Previously, mondain (Paul Gregoire) wrote…

[AI] Expected behavior, now logged. When allow_request_token_override is false the grants established at session setup govern, and a per-request token is intentionally ignored rather than rejected (a client re-sending its token shouldn't fail the request). In f34554c we now log that at DBG1 for visibility instead of silently dropping it.

DBG seems ok

---

src/MoqxRelay.cpp line 659 at r6 (raw file):

Previously, TimEvens (Tim Evens) wrote…

Not super great that the detection if the subNs is via a peer or not is determined by getPeerRelayID().

@TimEvens let's file a follow up issue -- though I believe that we need some way to indicate that peer relays are trusted (eg do not need to present tokens for auth to verify) or we need to configure the tokens the relay should use for peering request explicitly in config.

---

src/auth/Auth.h line 72 at r3 (raw file):

Previously, mondain (Paul Gregoire) wrote…

[AI] "local token issuers" was misleading -- the relay only ever verifies tokens, it never issues them. signForTest is purely a test/tooling helper for building a v1 envelope. I reworded the comment in f34554c to say exactly that. On the subclass question: I'd lean against it for now -- there's a single envelope version, and verify() / signForTest share that one layout, so a subclass would add a type without a second implementation to justify it. If we grow a real (non-test) token issuer later, extracting an interface at that point would be the natural time.

I'm on record as saying I don't love "test only" APIs, but I'll allow it

---

src/auth/Auth.cpp line 473 at r3 (raw file):
Ok, I'll note our top level requirements doc principles say:

1. Performance is Paramount: Every design decision must prioritize efficient processing, high throughput.

But the token was already signed by the issuer, so it maybe we can handwave past this

---

test/config/ConfigResolverTest.cpp line 467 at r3 (raw file):

Previously, mondain (Paul Gregoire) wrote…

[AI] The 2^62 bound isn't a moqx policy that will drift -- it's the maximum value encodable by a QUIC variable-length integer (RFC 9000 Section 16), which MOQT inherits and which is fixed by the wire format. So there's nothing to remember to bump as the drafts evolve. To make that explicit, f34554c names the constant kQuicVarintExclusiveUpperBound (with a comment) in validateAuth; the test deliberately uses 1 << 62 as the first un-encodable value.

The AI comment is wrong.

MOQT has shipped 64 bit varints in drafts 17 and 18. maybe we should delete the whole test, since it's not possible anymore?

---

deps/moxygen line 1 at r3 (raw file):

Previously, mondain (Paul Gregoire) wrote…

[AI] To add to the earlier note: this feature doesn't require any moxygen-side change -- it builds entirely on moxygen's existing AUTHORIZATION_TOKEN support (moxygen::AuthToken, Parameters, TrackRequestParamKey::AUTHORIZATION_TOKEN). The submodule bump in this PR is just a routine sync to openmoq/moxygen main, not a change coupled to the auth work. If the concern is keeping the review clean, I can split the submodule bump into its own commit/PR so it isn't entangled with the auth changes.

Wait, I don't think we should hard code any bumps here.

[afrind]

@afrind reviewed 14 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on akash-a-n, gmarzot, michalhosna, mondain, Oxyd, peterchave, and TimEvens).

---

deps/moxygen line 1 at r3 (raw file):

Previously, afrind wrote…

Wait, I don't think we should hard code any bumps here.

nvm, reviewable artifact