auth: add anonymous_claim grants and per-message token cap

[michalhosna]

Fixes #530

Sorry for late push, Claude was idiotic with this. It very persistently didn't want to use Enums and just pass string_views everywhere for the Action and convert back and forth, even when I handwrote it wanted to revert it.

---

This change is 

[michalhosna]

@michalhosna made 1 comment.
Reviewable status: 0 of 16 files reviewed, all discussions resolved.

---

src/auth/Action.h line 38 at r1 (raw file):

#pragma GCC diagnostic push
#pragma GCC diagnostic error "-Wswitch"

Really not sure if this pragma is the right way to go.
I would very much like a exhaustiveness compile-time check when switching on enums. Given that we allow errors in CI this is a way to do it.
Is there a better way? (Not an expert on C++ tooling).

[mondain]

Reviewable

The max_tokens_per_message policy is not enforced when allow_request_token_override is false. In authorize(), both counting and the TooManyTokens return are inside the allowRequestTokenOverride() branch; the false branch only detects and ignores matching tokens. I reproduced this with a cap of 1, overrides disabled, an anonymous Subscribe grant, and two request tokens: authorization succeeded instead of returning TooManyTokens.

This contradicts docs/config.md, which says every over-cap CLIENT_SETUP or request message is rejected outright. The generated schema description in ParsedConfig.h also says excess tokens are ignored, so there are currently three different semantics.

Please either enforce the count before branching on allowRequestTokenOverride() and add coverage for the disabled-override case, or narrow the documented/config-schema contract to say the cap applies only to tokens selected for verification.

[michalhosna]

@mondain Fixed (hopefully).

[afrind]

@afrind reviewed 16 files and all commit messages, and made 14 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on michalhosna).

---

src/auth/Action.h line 38 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

Really not sure if this pragma is the right way to go.
I would very much like a exhaustiveness compile-time check when switching on enums. Given that we allow errors in CI this is a way to do it.
Is there a better way? (Not an expert on C++ tooling).

Does it work with non-gcc compilers? My quick search suggested essentially what you have, in a nicer hat:

#if defined(_MSC_VER)
    #define ENFORCE_EXHAUSTIVE_SWITCH_BEGIN __pragma(warning(push)) \
                                            __pragma(warning(error : 4061 4062))
    #define ENFORCE_EXHAUSTIVE_SWITCH_END   __pragma(warning(pop))
#elif defined(__GNUC__) || defined(__clang__)
    #define ENFORCE_EXHAUSTIVE_SWITCH_BEGIN _Pragma("GCC diagnostic push") \
                                            _Pragma("GCC diagnostic error \"-Wswitch\"") \
                                            _Pragma("GCC diagnostic error \"-Wswitch-enum\"")
    #define ENFORCE_EXHAUSTIVE_SWITCH_END   _Pragma("GCC diagnostic pop")
#else
    #define ENFORCE_EXHAUSTIVE_SWITCH_BEGIN
    #define ENFORCE_EXHAUSTIVE_SWITCH_END
#endif

though I don't expect to compile on windows anytime soon. At least the gcc ugly is hidden in some helper header.

---

docs/config.md line 215 at r3 (raw file):

  max_tokens_per_message: 4
  anonymous_claim:
    - actions: [subscribe, fetch]

Can actions be repeated so I can grant subscribe/fetch/track status to some namespaces and publish/pub_ns to others?

---

src/auth/Action.h line 21 at r3 (raw file):

enum class Action : uint64_t {
  ClientSetup = 0,

SETUP is now merged right? Does cat4moq need an update?

---

src/auth/Action.h line 70 at r3 (raw file):

  auto begin = name.begin();
  auto end = name.end();
  while (begin != end && std::isspace(static_cast<unsigned char>(*begin))) {

folly has trimWhitespace helpers (I think trim leading/trailing/both)

---

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

  std::string out;
  for (const auto& field : ns.trackNamespace) {
    out.push_back(static_cast<char>((field.size() >> 24) & 0xff));

uint32_t canonicalLen = endian::(big/little don't ask me)(field.size());
out.push_back(4, &canonicalLen);

or something?

oh I see this is just moved. Still.

---

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

// catapult's CBOR decoder (parse_bin_match, cwt.cpp) falls back to EXACT for
// an unrecognized BinaryMatchType, so the XLOG below is a tripwire against

But the compiler macros enforce it's unreachable?

---

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

  if (trackName) {
    const FullTrackName ftn{ns, std::string(*trackName)};
    permitted = allowsAny(requestGrants, action, ftn, now) ||

Do we have any reason to think any one of these is more likely to return true than others? If so we could order them.

---

src/config/Config.h line 197 at r3 (raw file):

  struct AnonymousScope {
    std::vector<auth::Action> actions;
    // nullopt = match any namespace. Must stay optional, not an empty vector:

I would reword this comment. It sort of sounds like an answer to a prompt.

---

src/config/Config.h line 213 at r3 (raw file):

  bool strictClaims{false};
  std::vector<AnonymousScope> anonymousClaim; // empty = no anonymous access (default)
  // Caps how many AUTHORIZATION_TOKEN params (per CLIENT_SETUP or per request)

I find this comment inconsistency annoying. Why is only this member worthy of a 3 line comment?

---

src/config/ConfigResolver.cpp line 579 at r3 (raw file):

  }

  // Validated regardless of `enabled`: resolveAuth() resolves anonymous_claim

Is it odd that auth can be disabled and anonymous claims is enabled? Does "auth enabled" really mean "token required"?

---

src/config/ConfigResolver.cpp line 602 at r3 (raw file):

        errors.push_back(
            prefix + ".actions: '" + name +
            "' is not allowed here -- the anonymous claim never authorizes CLIENT_SETUP/"

CLIENT_SETUP and SERVER_SETUP are old message names.

---

src/config/loader/ParsedConfig.h line 408 at r3 (raw file):

    rfl::Description<
        "CAT4MOQ action names this scope grants without any token (see docs/config.md's action "

We want this in the config doc?

---

src/config/loader/ParsedConfig.h line 440 at r3 (raw file):

      anonymous_claim;
  rfl::Description<
      "Maximum AUTHORIZATION_TOKEN parameters allowed per CLIENT_SETUP or per request; "

CLIENT_SETUP is not a message

---

test/AuthTest.cpp line 510 at r3 (raw file):

TEST(
    AuthTest,
    AuthenticateSetupReturnsForbiddenWhenOneTokenFailsAndAnotherVerifiesWithoutClientSetup

epic test name bro

[michalhosna]

@michalhosna made 3 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on michalhosna).

---

src/auth/Action.h line 21 at r3 (raw file):

Previously, afrind wrote…

SETUP is now merged right? Does cat4moq need an update?

Yes, filled
moq-wg/CAT-4-MOQT#44

---

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

Previously, afrind wrote…

Do we have any reason to think any one of these is more likely to return true than others? If so we could order them.

I would hope this is the correct ordering.
request -> session (setup) -> anonymous

---

test/AuthTest.cpp line 510 at r3 (raw file):

Previously, afrind wrote…

epic test name bro

Do you feel it diverges from the convention here?
Taking suggestions, not that I want to spent time on naming tests 😄

[afrind]

@afrind made 1 comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on michalhosna).

---

test/AuthTest.cpp line 510 at r3 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

Do you feel it diverges from the convention here?
Taking suggestions, not that I want to spent time on naming tests 😄

I'm not paying that close attention but 87 chars is over my personal limit.

[michalhosna]

@michalhosna made 1 comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on michalhosna).

---

docs/config.md line 215 at r3 (raw file):

Previously, afrind wrote…

Can actions be repeated so I can grant subscribe/fetch/track status to some namespaces and publish/pub_ns to others?

  anonymous_claim:
    - actions: [subscribe, fetch]
      namespace_match: {prefix: ["live"]}
    - actions: [publish]
      namespace_match: {prefix: ["personal"]}

Those yaml list can be confusing, its the same as

anonymous_claim:
- {actions: [subscribe, fetch], namespace_match: {prefix: ["live"]}}
- {actions: [publish], namespace_match: {prefix: ["personal"]}}

I am not sure if it should be called claim or claims, honestly got somewhat confused about the nomenclature in c4m, so just went with something. Happy to change that

[michalhosna]

@michalhosna made 9 comments.
Reviewable status: 7 of 17 files reviewed, 5 unresolved discussions (waiting on afrind).

---

src/auth/Action.h line 38 at r1 (raw file):

Previously, afrind wrote…

Does it work with non-gcc compilers? My quick search suggested essentially what you have, in a nicer hat:

#if defined(_MSC_VER)
    #define ENFORCE_EXHAUSTIVE_SWITCH_BEGIN __pragma(warning(push)) \
                                            __pragma(warning(error : 4061 4062))
    #define ENFORCE_EXHAUSTIVE_SWITCH_END   __pragma(warning(pop))
#elif defined(__GNUC__) || defined(__clang__)
    #define ENFORCE_EXHAUSTIVE_SWITCH_BEGIN _Pragma("GCC diagnostic push") \
                                            _Pragma("GCC diagnostic error \"-Wswitch\"") \
                                            _Pragma("GCC diagnostic error \"-Wswitch-enum\"")
    #define ENFORCE_EXHAUSTIVE_SWITCH_END   _Pragma("GCC diagnostic pop")
#else
    #define ENFORCE_EXHAUSTIVE_SWITCH_BEGIN
    #define ENFORCE_EXHAUSTIVE_SWITCH_END
#endif

though I don't expect to compile on windows anytime soon. At least the gcc ugly is hidden in some helper header.

What about this?

---

src/auth/Action.h line 70 at r3 (raw file):

Previously, afrind wrote…

folly has trimWhitespace helpers (I think trim leading/trailing/both)

Done

---

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

Previously, afrind wrote…

uint32_t canonicalLen = endian::(big/little don't ask me)(field.size());
out.push_back(4, &canonicalLen);

or something?

oh I see this is just moved. Still.

Done

---

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

Previously, afrind wrote…

But the compiler macros enforce it's unreachable?

We we didn't had that macro here. Now we do.

---

src/config/Config.h line 197 at r3 (raw file):

Previously, afrind wrote…

I would reword this comment. It sort of sounds like an answer to a prompt.

Like this?

---

src/config/Config.h line 213 at r3 (raw file):

Previously, afrind wrote…

I find this comment inconsistency annoying. Why is only this member worthy of a 3 line comment?

Done.

---

src/config/ConfigResolver.cpp line 602 at r3 (raw file):

Previously, afrind wrote…

CLIENT_SETUP and SERVER_SETUP are old message names.

Done.

---

src/config/loader/ParsedConfig.h line 440 at r3 (raw file):

Previously, afrind wrote…

CLIENT_SETUP is not a message

Done.

---

test/AuthTest.cpp line 510 at r3 (raw file):

Previously, afrind wrote…

I'm not paying that close attention but 87 chars is over my personal limit.

Like this?

[michalhosna]

@michalhosna made 2 comments.
Reviewable status: 7 of 17 files reviewed, 5 unresolved discussions (waiting on afrind and michalhosna).

---

src/config/ConfigResolver.cpp line 579 at r3 (raw file):

Previously, afrind wrote…

Is it odd that auth can be disabled and anonymous claims is enabled? Does "auth enabled" really mean "token required"?

Add warning/error to the validation.

I think the validation still should happen regardless. It's better to get as many errors as possible per run when validating user(operator) input.

---

src/config/loader/ParsedConfig.h line 408 at r3 (raw file):

Previously, afrind wrote…

We want this in the config doc?

Not in something that lives in the binary, fixed.